Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1501415 > unrolled thread

[PATCH] MD-RAID: Use seq_putc() in three status functions

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH] MD-RAID: Use seq_putc() in three status functions SF Markus Elfring <elfring@users.sourceforge.net> - 2016-10-16 10:30 +0200
    Re: [PATCH] MD-RAID: Use seq_putc() in three status functions Hannes Reinecke <hare@suse.de> - 2016-10-16 19:00 +0200
      Re: MD-RAID: Use seq_putc() in three status functions? SF Markus Elfring <elfring@users.sourceforge.net> - 2016-10-16 19:20 +0200
        Re: MD-RAID: Use seq_putc() in three status functions? SF Markus Elfring <elfring@users.sourceforge.net> - 2016-10-16 19:20 +0200
        Re: MD-RAID: Use seq_putc() in three status functions? Hannes Reinecke <hare@suse.de> - 2016-10-17 08:00 +0200
          Re: MD-RAID: Use seq_putc() in three status functions? SF Markus Elfring <elfring@users.sourceforge.net> - 2016-10-17 09:50 +0200
            Re: MD-RAID: Use seq_putc() in three status functions? Hannes Reinecke <hare@suse.de> - 2016-10-17 10:20 +0200
              Re: MD-RAID: Use seq_putc() in three status functions? SF Markus Elfring <elfring@users.sourceforge.net> - 2016-10-17 11:10 +0200
                Re: MD-RAID: Use seq_putc() in three status functions? Hannes Reinecke <hare@suse.de> - 2016-10-17 12:00 +0200
                  Re: MD-RAID: Use seq_putc() in three status functions? SF Markus Elfring <elfring@users.sourceforge.net> - 2016-10-17 13:20 +0200
                    Re: MD-RAID: Use seq_putc() in three status functions? SF Markus Elfring <elfring@users.sourceforge.net> - 2016-10-17 13:50 +0200
                      Re: MD-RAID: Use seq_putc() in three status functions? Hannes Reinecke <hare@suse.de> - 2016-10-17 16:10 +0200
                        Re: MD-RAID: Use seq_putc() in three status functions? SF Markus Elfring <elfring@users.sourceforge.net> - 2016-10-17 16:40 +0200
                          Re: MD-RAID: Use seq_putc() in three status functions? Hannes Reinecke <hare@suse.de> - 2016-10-17 17:40 +0200
                            Re: MD-RAID: Use seq_putc() in three status functions? SF Markus Elfring <elfring@users.sourceforge.net> - 2016-10-17 18:10 +0200
                              Re: MD-RAID: Use seq_putc() in three status functions? Hannes Reinecke <hare@suse.de> - 2016-10-17 19:20 +0200
                                Re: MD-RAID: Use seq_putc() in three status functions? SF Markus Elfring <elfring@users.sourceforge.net> - 2016-10-18 20:30 +0200
                                  Re: MD-RAID: Use seq_putc() in three status functions? SF Markus Elfring <elfring@users.sourceforge.net> - 2016-10-20 14:30 +0200
                    Re: MD-RAID: Use seq_putc() in three status functions? Bernd Petrovitsch <bernd@petrovitsch.priv.at> - 2016-10-17 14:20 +0200
                    Re: MD-RAID: Use seq_putc() in three status functions? Hannes Reinecke <hare@suse.de> - 2016-10-17 16:10 +0200

#1501415 — [PATCH] MD-RAID: Use seq_putc() in three status functions

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 16 Oct 2016 10:10:28 +0200

A single character (a closing square bracket) should be put into a sequence
at the end in these functions.
Thus use the corresponding function "seq_putc".

This issue was detected also by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/md/raid1.c  | 2 +-
 drivers/md/raid10.c | 2 +-
 drivers/md/raid5.c  | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
index 1961d82..fd97f65 100644
--- a/drivers/md/raid1.c
+++ b/drivers/md/raid1.c
@@ -1418,7 +1418,7 @@ static void raid1_status(struct seq_file *seq, struct mddev *mddev)
 			   rdev && test_bit(In_sync, &rdev->flags) ? "U" : "_");
 	}
 	rcu_read_unlock();
-	seq_printf(seq, "]");
+	seq_putc(seq, ']');
 }
 
 static void raid1_error(struct mddev *mddev, struct md_rdev *rdev)
diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index be1a9fc..7490b28 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -1499,7 +1499,7 @@ static void raid10_status(struct seq_file *seq, struct mddev *mddev)
 		seq_printf(seq, "%s", rdev && test_bit(In_sync, &rdev->flags) ? "U" : "_");
 	}
 	rcu_read_unlock();
-	seq_printf(seq, "]");
+	seq_putc(seq, ']');
 }
 
 /* check if there are enough drives for
diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index 92ac251..9eb45c4 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -7077,7 +7077,7 @@ static void raid5_status(struct seq_file *seq, struct mddev *mddev)
 		seq_printf (seq, "%s", rdev && test_bit(In_sync, &rdev->flags) ? "U" : "_");
 	}
 	rcu_read_unlock();
-	seq_printf (seq, "]");
+	seq_putc(seq, ']');
 }
 
 static void print_raid5_conf (struct r5conf *conf)
-- 
2.10.1

[toc] | [next] | [standalone]

#1501527

On 10/16/2016 10:20 AM, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sun, 16 Oct 2016 10:10:28 +0200
>
> A single character (a closing square bracket) should be put into a sequence
> at the end in these functions.
> Thus use the corresponding function "seq_putc".
>
> This issue was detected also by using the Coccinelle software.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/md/raid1.c  | 2 +-
>  drivers/md/raid10.c | 2 +-
>  drivers/md/raid5.c  | 2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index 1961d82..fd97f65 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
> @@ -1418,7 +1418,7 @@ static void raid1_status(struct seq_file *seq, struct mddev *mddev)
>  			   rdev && test_bit(In_sync, &rdev->flags) ? "U" : "_");
>  	}
>  	rcu_read_unlock();
> -	seq_printf(seq, "]");
> +	seq_putc(seq, ']');
>  }
>
>  static void raid1_error(struct mddev *mddev, struct md_rdev *rdev)
> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> index be1a9fc..7490b28 100644
> --- a/drivers/md/raid10.c
> +++ b/drivers/md/raid10.c
> @@ -1499,7 +1499,7 @@ static void raid10_status(struct seq_file *seq, struct mddev *mddev)
>  		seq_printf(seq, "%s", rdev && test_bit(In_sync, &rdev->flags) ? "U" : "_");
>  	}
>  	rcu_read_unlock();
> -	seq_printf(seq, "]");
> +	seq_putc(seq, ']');
>  }
>
>  /* check if there are enough drives for
> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index 92ac251..9eb45c4 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
> @@ -7077,7 +7077,7 @@ static void raid5_status(struct seq_file *seq, struct mddev *mddev)
>  		seq_printf (seq, "%s", rdev && test_bit(In_sync, &rdev->flags) ? "U" : "_");
>  	}
>  	rcu_read_unlock();
> -	seq_printf (seq, "]");
> +	seq_putc(seq, ']');
>  }
>
>  static void print_raid5_conf (struct r5conf *conf)
>
The point of this patch being ... what?
Does it improve code? Does it improve anything?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)

[toc] | [prev] | [next] | [standalone]

#1501539 — Re: MD-RAID: Use seq_putc() in three status functions?

> Does it improve code? Does it improve anything?

Yes. - I got such an impression.

* Is it more efficient to call the function "seq_printf" for the desired data processing
  for a single character than to pass it to the function "" in a string?

* Will the required data transfer shrink a bit for the affected functions because of
  such a change?

Regards,
Markus

[toc] | [prev] | [next] | [standalone]

#1501540 — Re: MD-RAID: Use seq_putc() in three status functions?

> Yes. - I got such an impression.

Correction:

* Is it more efficient to call the function "seq_putc" for the desired data processing
  for a single character than to pass it to the function "seq_printf" in a string?

* Will the required data transfer shrink a bit for the affected functions because of
  such a change?
  How do you think about to reduce the transmission for such tiny strings
  containing delimiters?

Regards,
Markus

[toc] | [prev] | [next] | [standalone]

#1501618 — Re: MD-RAID: Use seq_putc() in three status functions?

On 10/16/2016 07:10 PM, SF Markus Elfring wrote:
>> Does it improve code? Does it improve anything?
> 
> Yes. - I got such an impression.
> 
> * Is it more efficient to call the function "seq_printf" for the desired data processing
>   for a single character than to pass it to the function "" in a string?
> 
> * Will the required data transfer shrink a bit for the affected functions because of
>   such a change?
> 
Which are questions _you_ should be able to answer.
It's your patch, after all.
Once you do (and prove that the answer is 'yes' to the above two
questions) the patch will be applied.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

[toc] | [prev] | [next] | [standalone]

#1501662 — Re: MD-RAID: Use seq_putc() in three status functions?

>>> Does it improve code? Does it improve anything?
>>
>> Yes. - I got such an impression.
>>
>> * Is it more efficient to call the function "seq_printf" for the desired data processing
>>   for a single character than to pass it to the function "" in a string?
>>
>> * Will the required data transfer shrink a bit for the affected functions because of
>>   such a change?
>>
> Which are questions _you_ should be able to answer.

I wonder that the answers are not obvious for you already.

Calling the function "seq_putc" will be more efficient than "seq_printf"
in this case because of the following reasons.

1. How does the distribution look like for supported processor architectures
   where the data transfer for bytes (as a function call parameter)
   is faster than for (string) pointers?

2. Did anybody measure already how many the execution times can vary
   for these functions?

3. seq_printf() provides more functionality as this kind of programming
   interface was designed for a bigger purpose.
   How much do you care for consequences when such general functions
   are called with input data they were not designed for mainly?

4. The seq_putc() implementation is so simple.
   http://lxr.free-electrons.com/source/fs/seq_file.c?v=4.8#L657

   Where do you get doubts about its efficiency for the data processing
   of a single character?


> It's your patch, after all.

Yes. - I published a special update suggestion once again.


> Once you do (and prove that the answer is 'yes' to the above two
> questions) the patch will be applied.

How do you think about to share a bit more from your software development
and testing experience?
Which call frequencies do you observe for the affected functions?

1. raid1_status
2. raid10_status
3. raid5_status

Regards,
Markus

[toc] | [prev] | [next] | [standalone]

#1501723 — Re: MD-RAID: Use seq_putc() in three status functions?

On 10/17/2016 09:39 AM, SF Markus Elfring wrote:
>>>> Does it improve code? Does it improve anything?
>>>
>>> Yes. - I got such an impression.
>>>
>>> * Is it more efficient to call the function "seq_printf" for the desired data processing
>>>   for a single character than to pass it to the function "" in a string?
>>>
>>> * Will the required data transfer shrink a bit for the affected functions because of
>>>   such a change?
>>>
>> Which are questions _you_ should be able to answer.
> 
> I wonder that the answers are not obvious for you already.
> 
> Calling the function "seq_putc" will be more efficient than "seq_printf"
> in this case because of the following reasons.
> 
> 1. How does the distribution look like for supported processor architectures
>    where the data transfer for bytes (as a function call parameter)
>    is faster than for (string) pointers?
> 
How would I know? I would assume that _you_ did some measurements here;
after all, _you_ are trying to push this patch.
I could easily claim that seq_printf() is more efficient than
seq_putc(), and won't apply your patch.
So _you_ have to prove that your patch is more efficient.

> 2. Did anybody measure already how many the execution times can vary
>    for these functions?
> 
Probably not.
But referring to the previous topic:
Unless _you_ prove that _your_ patch is more efficient it won't get
applied. _You_ want us to apply your patch, so the burden is on _you_ to
provide the required data.


>    Where do you get doubts about its efficiency for the data processing
>    of a single character?
> 
Because it's being called at the end of a function calling seq_printf()
already. So exchanging a single call is probably not helping anything,
as the compiler will optimize it anyway.
Case in point: with your patch the x86_64 compiler generates nearly
identical code for driver/md/raid1.c, but with one instruction _more_
after your patch has been applied.

So it's not immediately obvious that your patch is an improvement.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

[toc] | [prev] | [next] | [standalone]

#1501796 — Re: MD-RAID: Use seq_putc() in three status functions?

>> Calling the function "seq_putc" will be more efficient than "seq_printf"
>> in this case because of the following reasons.
>>
>> 1. How does the distribution look like for supported processor architectures
>>    where the data transfer for bytes (as a function call parameter)
>>    is faster than for (string) pointers?
>>
> How would I know?

How many processor architecture characteristics do you know already?

* Is a string pointer often longer than a byte?

* I imagine that it can become also interesting to check byte level data access
  under constraints of machine word sizes and alignment.


> I would assume that _you_ did some measurements here;

How much would you trust in any concrete numbers I could present
for this use case?

Do you give more trust to a reference testing platform?


> I could easily claim that seq_printf() is more efficient than
> seq_putc(), and won't apply your patch.

This is also possible in principle.


> So _you_ have to prove that your patch is more efficient.

How many results would we like to clarify from various hardware
and software combinations?


>> 2. Did anybody measure already how many the execution times can vary
>>    for these functions?
>>
> Probably not.

Thanks for this information.

How important are the mentioned functions for you within the Linux
programming interface so far?


> Unless _you_ prove that _your_ patch is more efficient it won't get applied.

Which data would you accept as a "prove" in this case?


>>    Where do you get doubts about its efficiency for the data processing
>>    of a single character?
>>
> Because it's being called at the end of a function calling seq_printf() already.

Interesting view …


> So exchanging a single call is probably not helping anything,
> as the compiler will optimize it anyway.

How common is the discussed software transformation between implementations
for optimising compilers?


> Case in point: with your patch the x86_64 compiler generates nearly
> identical code for driver/md/raid1.c, but with one instruction _more_
> after your patch has been applied.

Which software versions and command parameters did you try out
for this information (from an unspecified run time environment)?


> So it's not immediately obvious that your patch is an improvement.

I agree that there are system properties and constraints which can be
considered further.

Regards,
Markus

[toc] | [prev] | [next] | [standalone]

#1501830 — Re: MD-RAID: Use seq_putc() in three status functions?

On 10/17/2016 11:00 AM, SF Markus Elfring wrote:
>>> Calling the function "seq_putc" will be more efficient than "seq_printf"
>>> in this case because of the following reasons.
>>>
>>> 1. How does the distribution look like for supported processor architectures
>>>    where the data transfer for bytes (as a function call parameter)
>>>    is faster than for (string) pointers?
>>>
>> How would I know?
> 
> How many processor architecture characteristics do you know already?
> 
x86, s390x, ppc/ppc64.

> * Is a string pointer often longer than a byte?
> 
Always.

(Which up to now I thought was basic programming knowledge...)

> * I imagine that it can become also interesting to check byte level data access
>   under constraints of machine word sizes and alignment.
> 
So, another test for you to do.

> 
>> I would assume that _you_ did some measurements here;
> 
> How much would you trust in any concrete numbers I could present
> for this use case?
> 
> Do you give more trust to a reference testing platform?
> 
At the moment _any_ test would do.
With every response from your side you just keep on asking further
questions. But so far you haven't delivered any answers nor measurements.

> 
>> I could easily claim that seq_printf() is more efficient than
>> seq_putc(), and won't apply your patch.
> 
> This is also possible in principle.
> 
No, this is what's going to happen if you don't show any measurements.

> 
>> So _you_ have to prove that your patch is more efficient.
> 
> How many results would we like to clarify from various hardware
> and software combinations?
> 
See above. At the moment _any_ test result from your side would do.

> 
>>> 2. Did anybody measure already how many the execution times can vary
>>>    for these functions?
>>>
>> Probably not.
> 
> Thanks for this information.
> 
> How important are the mentioned functions for you within the Linux
> programming interface so far?
> 
Not very. The interface is only used in a slow path, and the execution
time doesn't affect I/O performance in any way.

> 
>> Unless _you_ prove that _your_ patch is more efficient it won't get applied.
> 
> Which data would you accept as a "prove" in this case?
> 
Again: You want something from us. We don't have to prove anything, you
need to convince us. And it is really hard to convince anyone by asking
questions.

> 
>>>    Where do you get doubts about its efficiency for the data processing
>>>    of a single character?
>>>
>> Because it's being called at the end of a function calling seq_printf() already.
> 
> Interesting view …
> 
> 
>> So exchanging a single call is probably not helping anything,
>> as the compiler will optimize it anyway.
> 
> How common is the discussed software transformation between implementations
> for optimising compilers?
> 
> 
>> Case in point: with your patch the x86_64 compiler generates nearly
>> identical code for driver/md/raid1.c, but with one instruction _more_
>> after your patch has been applied.
> 
> Which software versions and command parameters did you try out
> for this information (from an unspecified run time environment)?
> 
> 
# gcc --version
gcc (SUSE Linux) 4.8.5
Copyright (C) 2015 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is
NO warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR
PURPOSE.

git tree from git.kernel.org/mkp/u/4.10/scsi-queue

_I_ did some measurements.
I'm still waiting from results from your side.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

[toc] | [prev] | [next] | [standalone]

#1501886 — Re: MD-RAID: Use seq_putc() in three status functions?

>> * Is a string pointer often longer than a byte?
>>
> Always.

I have got doubts for this specific information.


> (Which up to now I thought was basic programming knowledge...)

By the way:
Run time environments still exist where the size of a pointer can be also
just one byte, don't they?


>> How many results would we like to clarify from various hardware
>> and software combinations?
>>
> See above. At the moment _any_ test result from your side would do.

I imagine that another single result might not be representative.
How many lessons from test statistics will usually be also relevant here?


>> How important are the mentioned functions for you within the Linux
>> programming interface so far?
>>
> Not very. The interface is only used in a slow path, and the execution
> time doesn't affect I/O performance in any way.

Thanks for another interesting information.


>>> Case in point: with your patch the x86_64 compiler generates nearly
>>> identical code for driver/md/raid1.c, but with one instruction _more_
>>> after your patch has been applied.
>>
>> Which software versions and command parameters did you try out
>> for this information (from an unspecified run time environment)?
>>
> # gcc --version
> gcc (SUSE Linux) 4.8.5

Thanks for this detail.

* Did you choose any special optimisation settings for your quick check?

* Will any compilation results matter if "optimisation" would be
  switched off there?


> I'm still waiting from results from your side.

Would any other software developers or testers dare to add related information?

Regards,
Markus

[toc] | [prev] | [next] | [standalone]

#1501903 — Re: MD-RAID: Use seq_putc() in three status functions?

>>> See above. At the moment _any_ test result from your side would do.
>>
>> I imagine that another single result might not be representative.
> 
> Publish not only results but also everything (complete!) so that anyone
> can *easily* follow it to check and reproduce the results - especially
> if you want people with knowledge of other architectures to comment
> (otherwise they probably won't bother).

Am I the only software developer so far who would dare to reconsider
implementation details from three status functions?

Regards,
Markus

[toc] | [prev] | [next] | [standalone]

#1501992 — Re: MD-RAID: Use seq_putc() in three status functions?

On 10/17/2016 01:43 PM, SF Markus Elfring wrote:
>>>> See above. At the moment _any_ test result from your side would do.
>>>
>>> I imagine that another single result might not be representative.
>>
>> Publish not only results but also everything (complete!) so that anyone
>> can *easily* follow it to check and reproduce the results - especially
>> if you want people with knowledge of other architectures to comment
>> (otherwise they probably won't bother).
> 
> Am I the only software developer so far who would dare to reconsider
> implementation details from three status functions?
> 
No.
But we're waiting for you showing is that it is an improvement.
Which at the moment we don't see.
Hence we're waiting from a proof or validation from your side.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

[toc] | [prev] | [next] | [standalone]

#1502040 — Re: MD-RAID: Use seq_putc() in three status functions?

>> Am I the only software developer so far who would dare to reconsider
>> implementation details from three status functions?
>>
> No.

Thanks for this kind of promising feedback.


> But we're waiting for you showing is that it is an improvement.

Can this aspect also be clarified to some degree from a logical point of view?

* Would you really like to know under which circumstances data processing
  will be faster for a single character instead of using a string pointer
  and corresponding two characters?

* Do you care for a changed memory allocation characteristic?

* Will it occasionally be useful to avoid the storage for another string literal?

Regards,
Markus

[toc] | [prev] | [next] | [standalone]

#1502111 — Re: MD-RAID: Use seq_putc() in three status functions?

On 10/17/2016 04:30 PM, SF Markus Elfring wrote:
>>> Am I the only software developer so far who would dare to reconsider
>>> implementation details from three status functions?
>>>
>> No.
>
> Thanks for this kind of promising feedback.
>
>
>> But we're waiting for you showing is that it is an improvement.
>
> Can this aspect also be clarified to some degree from a logical point of view?
>
I sincerely doubt that.
We've discussed the logical implications already, and failed to come to 
a consensus. So we need some proof (as in: on this architecture I'm 
seeing this and that performance improvements).
Which you have to deliver.

> * Would you really like to know under which circumstances data processing
>   will be faster for a single character instead of using a string pointer
>   and corresponding two characters?
>
It's not a problem of the interface, it's a problem of the resulting 
code (ie assembler output). We can discuss all we like, if the compiler 
decides to throw in an optimisation none of the arguments even apply.

> * Do you care for a changed memory allocation characteristic?
>
> * Will it occasionally be useful to avoid the storage for another string literal?
>
Occasionally: yes.
In this particular case: hardly.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)

[toc] | [prev] | [next] | [standalone]

#1502153 — Re: MD-RAID: Use seq_putc() in three status functions?

>> * Would you really like to know under which circumstances data processing
>>   will be faster for a single character instead of using a string pointer
>>   and corresponding two characters?
>>
> It's not a problem of the interface, it's a problem of the resulting code
> (ie assembler output).

How do you think about to discuss concrete generated code any further?


> We can discuss all we like, if the compiler decides to throw in
> an optimisation none of the arguments even apply.

Would it make sense to clarify assembler output with optimisation switched off?

Do you eventually care for code from non-optimising compilers?


>> * Will it occasionally be useful to avoid the storage for another string literal?
>>
> Occasionally: yes.
> In this particular case: hardly.

I am curious when such a software design aspect can become more relevant.
Would it be nice to get rid of three questionable string terminators (null bytes)
for example?

Regards,
Markus

[toc] | [prev] | [next] | [standalone]

#1502284 — Re: MD-RAID: Use seq_putc() in three status functions?

On 10/17/2016 06:08 PM, SF Markus Elfring wrote:
>>> * Would you really like to know under which circumstances data processing
>>>   will be faster for a single character instead of using a string pointer
>>>   and corresponding two characters?
>>>
>> It's not a problem of the interface, it's a problem of the resulting code
>> (ie assembler output).
>
> How do you think about to discuss concrete generated code any further?
>
Sure. Show me the generated code and point out where the benefits are.

>> We can discuss all we like, if the compiler decides to throw in
>> an optimisation none of the arguments even apply.
>
> Would it make sense to clarify assembler output with optimisation switched off?
>
> Do you eventually care for code from non-optimising compilers?
>
No. This is the linux kernel. There is a very, _very_ limited benefit of 
trying to use a non-standard compiler.

>
>>> * Will it occasionally be useful to avoid the storage for another string literal?
>>>
>> Occasionally: yes.
>> In this particular case: hardly.
>
> I am curious when such a software design aspect can become more relevant.
> Would it be nice to get rid of three questionable string terminators (null bytes)
> for example?
>
Again, all this does it trying to out-guess what the compiler might be 
doing during compilation. For which the easiest method is checking.
So back to the original task for you: Show me in the generated output 
where the benefits are.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)

[toc] | [prev] | [next] | [standalone]

#1503277 — Re: MD-RAID: Use seq_putc() in three status functions?

> So back to the original task for you: Show me in the generated output where the benefits are.

I can offer a bit more information for this software development discussion.


The afffected source files can be compiled for the processor architecture "x86_64"
by a tool like "GCC 6.2.1+r239849-1.3" from the software distribution
"openSUSE Tumbleweed" with the following command examples.

git checkout next-20161014 && my_cc=/usr/bin/gcc-6 && make HOSTCC="${my_cc}" allmodconfig && make -j6 HOSTCC="${my_cc}" drivers/md/
git checkout next_usage_of_seq_putc_in_md_raid_1 && my_cc=/usr/bin/gcc-6 && make HOSTCC="${my_cc}" allmodconfig && make -j6 HOSTCC="${my_cc}" drivers/md/


The tool "objdump" from the software package "binutils 2.27-1.3" can be used
to get corresponding disassemblies for a file like "drivers/md/raid1.obj"
which can then be compared as follows.


--- ../disassembly-md-raid1-next-20161014-1.txt	2016-10-18 18:00:12.341222741 +0200
+++ ../disassembly-md-raid1-seq_putc-1.txt	2016-10-18 18:03:54.135887333 +0200
@@ -3349,7 +3349,7 @@
     37ad:	85 c0                	test   %eax,%eax
     37af:	74 0d                	je     37be <raid1_status+0x9e>
     37b1:	80 3d 00 00 00 00 00 	cmpb   $0x0,0x0(%rip)        # 37b8 <raid1_status+0x98>
-    37b8:	0f 84 1d 01 00 00    	je     38db <raid1_status+0x1bb>
+    37b8:	0f 84 1b 01 00 00    	je     38d9 <raid1_status+0x1b9>
     37be:	4c 89 ff             	mov    %r15,%rdi
     37c1:	31 db                	xor    %ebx,%ebx
     37c3:	e8 00 00 00 00       	callq  37c8 <raid1_status+0xa8>
@@ -3404,42 +3404,43 @@
     3891:	85 c0                	test   %eax,%eax
     3893:	74 09                	je     389e <raid1_status+0x17e>
     3895:	80 3d 00 00 00 00 00 	cmpb   $0x0,0x0(%rip)        # 389c <raid1_status+0x17c>
-    389c:	74 6e                	je     390c <raid1_status+0x1ec>
+    389c:	74 6c                	je     390a <raid1_status+0x1ea>
     389e:	48 c7 c2 00 00 00 00 	mov    $0x0,%rdx
     38a5:	be 01 00 00 00       	mov    $0x1,%esi
     38aa:	48 c7 c7 00 00 00 00 	mov    $0x0,%rdi
     38b1:	65 ff 0d 00 00 00 00 	decl   %gs:0x0(%rip)        # 38b8 <raid1_status+0x198>
     38b8:	e8 00 00 00 00       	callq  38bd <raid1_status+0x19d>
     38bd:	4c 89 f7             	mov    %r14,%rdi
-    38c0:	48 c7 c6 00 00 00 00 	mov    $0x0,%rsi
-    38c7:	e8 00 00 00 00       	callq  38cc <raid1_status+0x1ac>
-    38cc:	48 83 c4 10          	add    $0x10,%rsp
-    38d0:	5b                   	pop    %rbx
-    38d1:	41 5c                	pop    %r12
-    38d3:	41 5d                	pop    %r13
-    38d5:	41 5e                	pop    %r14
-    38d7:	41 5f                	pop    %r15
-    38d9:	5d                   	pop    %rbp
-    38da:	c3                   	retq   
-    38db:	e8 00 00 00 00       	callq  38e0 <raid1_status+0x1c0>
-    38e0:	84 c0                	test   %al,%al
-    38e2:	0f 85 d6 fe ff ff    	jne    37be <raid1_status+0x9e>
-    38e8:	48 c7 c2 00 00 00 00 	mov    $0x0,%rdx
-    38ef:	be 69 03 00 00       	mov    $0x369,%esi
-    38f4:	48 c7 c7 00 00 00 00 	mov    $0x0,%rdi
-    38fb:	c6 05 00 00 00 00 01 	movb   $0x1,0x0(%rip)        # 3902 <raid1_status+0x1e2>
-    3902:	e8 00 00 00 00       	callq  3907 <raid1_status+0x1e7>
-    3907:	e9 b2 fe ff ff       	jmpq   37be <raid1_status+0x9e>
-    390c:	e8 00 00 00 00       	callq  3911 <raid1_status+0x1f1>
-    3911:	84 c0                	test   %al,%al
-    3913:	75 89                	jne    389e <raid1_status+0x17e>
-    3915:	48 c7 c2 00 00 00 00 	mov    $0x0,%rdx
-    391c:	be 9c 03 00 00       	mov    $0x39c,%esi
-    3921:	48 c7 c7 00 00 00 00 	mov    $0x0,%rdi
-    3928:	c6 05 00 00 00 00 01 	movb   $0x1,0x0(%rip)        # 392f <raid1_status+0x20f>
-    392f:	e8 00 00 00 00       	callq  3934 <raid1_status+0x214>
-    3934:	e9 65 ff ff ff       	jmpq   389e <raid1_status+0x17e>
-    3939:	0f 1f 80 00 00 00 00 	nopl   0x0(%rax)
+    38c0:	be 5d 00 00 00       	mov    $0x5d,%esi
+    38c5:	e8 00 00 00 00       	callq  38ca <raid1_status+0x1aa>
+    38ca:	48 83 c4 10          	add    $0x10,%rsp
+    38ce:	5b                   	pop    %rbx
+    38cf:	41 5c                	pop    %r12
+    38d1:	41 5d                	pop    %r13
+    38d3:	41 5e                	pop    %r14
+    38d5:	41 5f                	pop    %r15
+    38d7:	5d                   	pop    %rbp
+    38d8:	c3                   	retq   
+    38d9:	e8 00 00 00 00       	callq  38de <raid1_status+0x1be>
+    38de:	84 c0                	test   %al,%al
+    38e0:	0f 85 d8 fe ff ff    	jne    37be <raid1_status+0x9e>
+    38e6:	48 c7 c2 00 00 00 00 	mov    $0x0,%rdx
+    38ed:	be 69 03 00 00       	mov    $0x369,%esi
+    38f2:	48 c7 c7 00 00 00 00 	mov    $0x0,%rdi
+    38f9:	c6 05 00 00 00 00 01 	movb   $0x1,0x0(%rip)        # 3900 <raid1_status+0x1e0>
+    3900:	e8 00 00 00 00       	callq  3905 <raid1_status+0x1e5>
+    3905:	e9 b4 fe ff ff       	jmpq   37be <raid1_status+0x9e>
+    390a:	e8 00 00 00 00       	callq  390f <raid1_status+0x1ef>
+    390f:	84 c0                	test   %al,%al
+    3911:	75 8b                	jne    389e <raid1_status+0x17e>
+    3913:	48 c7 c2 00 00 00 00 	mov    $0x0,%rdx
+    391a:	be 9c 03 00 00       	mov    $0x39c,%esi
+    391f:	48 c7 c7 00 00 00 00 	mov    $0x0,%rdi
+    3926:	c6 05 00 00 00 00 01 	movb   $0x1,0x0(%rip)        # 392d <raid1_status+0x20d>
+    392d:	e8 00 00 00 00       	callq  3932 <raid1_status+0x212>
+    3932:	e9 67 ff ff ff       	jmpq   389e <raid1_status+0x17e>
+    3937:	66 0f 1f 84 00 00 00 	nopw   0x0(%rax,%rax,1)
+    393e:	00 00 
 
 0000000000003940 <print_conf>:
     3940:	e8 00 00 00 00       	callq  3945 <print_conf+0x5>
@@ -11134,7 +11135,7 @@
 
 0000000000000000 <_GLOBAL__sub_D_65535_0_raid1.c>:
    0:	55                   	push   %rbp
-   1:	be 35 00 00 00       	mov    $0x35,%esi
+   1:	be 34 00 00 00       	mov    $0x34,%esi
    6:	48 c7 c7 00 00 00 00 	mov    $0x0,%rdi
    d:	48 89 e5             	mov    %rsp,%rbp
   10:	e8 00 00 00 00       	callq  15 <_GLOBAL__sub_D_65535_0_raid1.c+0x15>
@@ -11145,7 +11146,7 @@
 
 0000000000000000 <_GLOBAL__sub_I_65535_1_raid1.c>:
    0:	55                   	push   %rbp
-   1:	be 35 00 00 00       	mov    $0x35,%esi
+   1:	be 34 00 00 00       	mov    $0x34,%esi
    6:	48 c7 c7 00 00 00 00 	mov    $0x0,%rdi
    d:	48 89 e5             	mov    %rsp,%rbp
   10:	e8 00 00 00 00       	callq  15 <_GLOBAL__sub_I_65535_1_raid1.c+0x15>


Does this kind of data display contain differences which are worth for further considerations?

Regards,
Markus

[toc] | [prev] | [next] | [standalone]

#1504845 — Re: MD-RAID: Use seq_putc() in three status functions?

>> So back to the original task for you: Show me in the generated output where the benefits are.

I can offer another bit of information for this software development discussion.

The following build settings were active in my "Makefile" for this Linux test case.

…
HOSTCFLAGS   = -Wall -Wmissing-prototypes -Wstrict-prototypes -O0 -fomit-frame-pointer -std=gnu89
…


The afffected source files can be compiled for the processor architecture "x86_64"
by a tool like "GCC 6.2.1+r239849-1.4" from the software distribution
"openSUSE Tumbleweed" with the following command example.

my_original=${my_build_dir}unchanged/test/ \
&& my_fixing=${my_build_dir}patched/test/ \
&& mkdir -p ${my_original} ${my_fixing} \
&& my_cc=/usr/bin/gcc-6 \
&& my_module=drivers/md/raid1.s \
&& git checkout next-20161014 \
&& make -j6 O="${my_original}" HOSTCC="${my_cc}" allmodconfig ${my_module} \
&& git checkout next_usage_of_seq_putc_in_md_raid_1 \
&& make -j6 O="${my_fixing}" HOSTCC="${my_cc}" allmodconfig ${my_module} \
&& diff -u "${my_original}${my_module}" "${my_fixing}${my_module}" > "${my_build_dir}assembler_code_comparison_$(date -I)_1.diff"


Unfortunately, the generated file got the size "311 KiB". I guess that
this is too big to send such a file around on the Linux mailing list.

Is this kind of assembler code comparison still useful to clarify relevant
differences further?

Regards,
Markus

[toc] | [prev] | [next] | [standalone]

#1501921 — Re: MD-RAID: Use seq_putc() in three status functions?

On Mon, 2016-10-17 at 13:10 +0200, SF Markus Elfring wrote:
[...]
> > (Which up to now I thought was basic programming knowledge...)
> 
> By the way:
> Run time environments still exist where the size of a pointer can
> be also just one byte, don't they?

In the context of the Linux kernel: No.

[ Side note: there might be some DSP out there with a running Linux
kernel which cannot really address a "byte" (meaning 8bits) but only in
register sized quantities (and also aligned for that). But no one cares
here really deeply as that is a so fundamental difference that the C-
compiler must cope with that anyways in the first place. ]

[...]
> > See above. At the moment _any_ test result from your side would do.
> 
> I imagine that another single result might not be representative.

Publish not only results but also everything (complete!) so that anyone
can *easily* follow it to check and reproduce the results - especially
if you want people with knowledge of other architectures to comment
(otherwise they probably won't bother).

Kind regards,
	Bernd
-- 
Bernd Petrovitsch                  Email : bernd@petrovitsch.priv.at
                     LUGA : http://www.luga.at

[toc] | [prev] | [next] | [standalone]

#1501983 — Re: MD-RAID: Use seq_putc() in three status functions?

On 10/17/2016 01:10 PM, SF Markus Elfring wrote:
>>> * Is a string pointer often longer than a byte?
>>>
>> Always.
> 
> I have got doubts for this specific information.
> 
> 
>> (Which up to now I thought was basic programming knowledge...)
> 
> By the way:
> Run time environments still exist where the size of a pointer can be also
> just one byte, don't they?
> 
Really? Name one.
You can only fit a point in one byte if you are on an 8-bit system.
Which I don't think linux is running on.

>>> How many results would we like to clarify from various hardware
>>> and software combinations?
>>>
>> See above. At the moment _any_ test result from your side would do.
> 
> I imagine that another single result might not be representative.
> How many lessons from test statistics will usually be also relevant here?
> 
> 
As said above, _any_ statistic will do at this point.

>>> How important are the mentioned functions for you within the Linux
>>> programming interface so far?
>>>
>> Not very. The interface is only used in a slow path, and the execution
>> time doesn't affect I/O performance in any way.
> 
> Thanks for another interesting information.
> 
> 
>>>> Case in point: with your patch the x86_64 compiler generates nearly
>>>> identical code for driver/md/raid1.c, but with one instruction _more_
>>>> after your patch has been applied.
>>>
>>> Which software versions and command parameters did you try out
>>> for this information (from an unspecified run time environment)?
>>>
>> # gcc --version
>> gcc (SUSE Linux) 4.8.5
> 
> Thanks for this detail.
> 
> * Did you choose any special optimisation settings for your quick check?
> 
> * Will any compilation results matter if "optimisation" would be
>   switched off there?
> 
> 
These were the results when calling 'make' in the kernel source tree.
With all applicable options.

>> I'm still waiting from results from your side.
> 
> Would any other software developers or testers dare to add related information?
> 
No. It's your patch, _you_ have to do it.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web