First kernel patch (optimization)

First kernel patch (optimization)

[Eric Curtin]

My first kernel patch, hope I did everything correctly! Instead of calling strlen on every iteration of the for loop, just call it once instead and store in a variable.

Signed-off-by: Eric Curtin <ericcurtin17@gmail.com>

diff --git a/tools/usb/usbip/src/usbip_detach.c b/tools/usb/usbip/src/usbip_detach.c
index 05c6d15..9db9d21 100644
--- a/tools/usb/usbip/src/usbip_detach.c
+++ b/tools/usb/usbip/src/usbip_detach.c
@@ -47,7 +47,9 @@ static int detach_port(char *port)
 	uint8_t portnum;
 	char path[PATH_MAX+1];
 
-	for (unsigned int i = 0; i < strlen(port); i++)
+	unsigned int port_len = strlen(port);
+
+	for (unsigned int i = 0; i < port_len; i++)
 		if (!isdigit(port[i])) {
 			err("invalid port %s", port);
 			return -1;

Re: First kernel patch (optimization)

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1673 bytes --]

On Tue, Sep 15, 2015 at 08:53:28PM +0100, Eric Curtin wrote:
> My first kernel patch, hope I did everything correctly! Instead of calling strlen on every iteration of the for loop, just call it once instead and store in a variable.

this should be broken up at 72 characters. Also, your subject and commit
log make no sense from a project history perspective. You don't need to
mention it's your first patch, just go straight to the details.

For the subject, I'd use something like:

tools: usbip: detach: avoid calling strlen() at each iteration

and for the commit log:

Instead of calling strlen() on every iterations of the for loop,
just call it once and cache the result in a temporary local variable
which will be used in the for loop instead.

cheers

> Signed-off-by: Eric Curtin <ericcurtin17@gmail.com>
> 
> diff --git a/tools/usb/usbip/src/usbip_detach.c b/tools/usb/usbip/src/usbip_detach.c
> index 05c6d15..9db9d21 100644
> --- a/tools/usb/usbip/src/usbip_detach.c
> +++ b/tools/usb/usbip/src/usbip_detach.c
> @@ -47,7 +47,9 @@ static int detach_port(char *port)
>  	uint8_t portnum;
>  	char path[PATH_MAX+1];
>  
> -	for (unsigned int i = 0; i < strlen(port); i++)
> +	unsigned int port_len = strlen(port);
> +
> +	for (unsigned int i = 0; i < port_len; i++)
>  		if (!isdigit(port[i])) {
>  			err("invalid port %s", port);
>  			return -1;
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: First kernel patch (optimization)

[Steve Calfee]

On Tue, Sep 15, 2015 at 12:53 PM, Eric Curtin <ericcurtin17@gmail.com> wrote:
> Signed-off-by: Eric Curtin <ericcurtin17@gmail.com>
>
> diff --git a/tools/usb/usbip/src/usbip_detach.c b/tools/usb/usbip/src/usbip_detach.c
> index 05c6d15..9db9d21 100644
> --- a/tools/usb/usbip/src/usbip_detach.c
> +++ b/tools/usb/usbip/src/usbip_detach.c
> @@ -47,7 +47,9 @@ static int detach_port(char *port)
>         uint8_t portnum;
>         char path[PATH_MAX+1];
>
> -       for (unsigned int i = 0; i < strlen(port); i++)
> +       unsigned int port_len = strlen(port);
> +
> +       for (unsigned int i = 0; i < port_len; i++)
>                 if (!isdigit(port[i])) {
>                         err("invalid port %s", port);
>                         return -1;
>
> --

Hi Eric,

This is fine, but what kind of wimpy compiler optimizer will not move
the constant initializer out of the loop? I bet if you compare binary
sizes/code it will be exactly the same, and you added some characters
of code. Reorganizing code for readability is fine, but for compiler
(in)efficiency seems like a bad idea.

Regards, Steve

Re: First kernel patch (optimization)

[Austin S Hemmelgarn]

[-- Attachment #1: Type: text/plain, Size: 1691 bytes --]

On 2015-09-15 20:09, Steve Calfee wrote:
> On Tue, Sep 15, 2015 at 12:53 PM, Eric Curtin <ericcurtin17@gmail.com> wrote:
>> Signed-off-by: Eric Curtin <ericcurtin17@gmail.com>
>>
>> diff --git a/tools/usb/usbip/src/usbip_detach.c b/tools/usb/usbip/src/usbip_detach.c
>> index 05c6d15..9db9d21 100644
>> --- a/tools/usb/usbip/src/usbip_detach.c
>> +++ b/tools/usb/usbip/src/usbip_detach.c
>> @@ -47,7 +47,9 @@ static int detach_port(char *port)
>>          uint8_t portnum;
>>          char path[PATH_MAX+1];
>>
>> -       for (unsigned int i = 0; i < strlen(port); i++)
>> +       unsigned int port_len = strlen(port);
>> +
>> +       for (unsigned int i = 0; i < port_len; i++)
>>                  if (!isdigit(port[i])) {
>>                          err("invalid port %s", port);
>>                          return -1;
>>
>> --
>
> Hi Eric,
>
> This is fine, but what kind of wimpy compiler optimizer will not move
> the constant initializer out of the loop? I bet if you compare binary
> sizes/code it will be exactly the same, and you added some characters
> of code. Reorganizing code for readability is fine, but for compiler
> (in)efficiency seems like a bad idea.
While I agree with your argument, I would like to point out that it is a 
well established fact that GCC's optimizers are kind of brain-dead at 
times and need their hands held.

I'd be willing to bet that the code will be marginally larger (because 
of adding another variable), but might run slightly faster too (because 
in my experience, GCC doesn't always catch things like this), and should 
compile a little faster (because the optimizers don't have to do as much 
work).


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 3019 bytes --]

RE: First kernel patch (optimization)

[David Laight]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2236 bytes --]

From: Austin S Hemmelgarn
> Sent: 16 September 2015 12:46
> On 2015-09-15 20:09, Steve Calfee wrote:
> > On Tue, Sep 15, 2015 at 12:53 PM, Eric Curtin <ericcurtin17@gmail.com> wrote:
> >> Signed-off-by: Eric Curtin <ericcurtin17@gmail.com>
> >>
> >> diff --git a/tools/usb/usbip/src/usbip_detach.c b/tools/usb/usbip/src/usbip_detach.c
> >> index 05c6d15..9db9d21 100644
> >> --- a/tools/usb/usbip/src/usbip_detach.c
> >> +++ b/tools/usb/usbip/src/usbip_detach.c
> >> @@ -47,7 +47,9 @@ static int detach_port(char *port)
> >>          uint8_t portnum;
> >>          char path[PATH_MAX+1];
> >>
> >> -       
> >> +       unsigned int port_len = strlen(port);
> >> +
> >> +       for (unsigned int i = 0; i < port_len; i++)
> >>                  if (!isdigit(port[i])) {
> >>                          err("invalid port %s", port);
> >>                          return -1;
> >>
> >> --
> >
> > Hi Eric,
> >
> > This is fine, but what kind of wimpy compiler optimizer will not move
> > the constant initializer out of the loop? I bet if you compare binary
> > sizes/code it will be exactly the same, and you added some characters
> > of code. Reorganizing code for readability is fine, but for compiler
> > (in)efficiency seems like a bad idea.
> While I agree with your argument, I would like to point out that it is a
> well established fact that GCC's optimizers are kind of brain-dead at
> times and need their hands held.
> 
> I'd be willing to bet that the code will be marginally larger (because
> of adding another variable), but might run slightly faster too (because
> in my experience, GCC doesn't always catch things like this), and should
> compile a little faster (because the optimizers don't have to do as much
> work).

The compiler probably can't optimise the strlen().
If isdigit() is a real function (the locale specific one probably is)
then the compile cannot assume that port[n] isn't changed by the call
to isdigit.

A simpler change would be:
	for (unsigned int i = 0; port[i] != 0; i++)

Much better would be to use strtoul() instead of atoi().

	David

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: First kernel patch (optimization)

[Greg KH]

On Wed, Sep 16, 2015 at 07:45:53AM -0400, Austin S Hemmelgarn wrote:
> On 2015-09-15 20:09, Steve Calfee wrote:
> >On Tue, Sep 15, 2015 at 12:53 PM, Eric Curtin <ericcurtin17@gmail.com> wrote:
> >>Signed-off-by: Eric Curtin <ericcurtin17@gmail.com>
> >>
> >>diff --git a/tools/usb/usbip/src/usbip_detach.c b/tools/usb/usbip/src/usbip_detach.c
> >>index 05c6d15..9db9d21 100644
> >>--- a/tools/usb/usbip/src/usbip_detach.c
> >>+++ b/tools/usb/usbip/src/usbip_detach.c
> >>@@ -47,7 +47,9 @@ static int detach_port(char *port)
> >>         uint8_t portnum;
> >>         char path[PATH_MAX+1];
> >>
> >>-       for (unsigned int i = 0; i < strlen(port); i++)
> >>+       unsigned int port_len = strlen(port);
> >>+
> >>+       for (unsigned int i = 0; i < port_len; i++)
> >>                 if (!isdigit(port[i])) {
> >>                         err("invalid port %s", port);
> >>                         return -1;
> >>
> >>--
> >
> >Hi Eric,
> >
> >This is fine, but what kind of wimpy compiler optimizer will not move
> >the constant initializer out of the loop? I bet if you compare binary
> >sizes/code it will be exactly the same, and you added some characters
> >of code. Reorganizing code for readability is fine, but for compiler
> >(in)efficiency seems like a bad idea.
> While I agree with your argument, I would like to point out that it is a
> well established fact that GCC's optimizers are kind of brain-dead at times
> and need their hands held.
> 
> I'd be willing to bet that the code will be marginally larger (because of
> adding another variable), but might run slightly faster too (because in my
> experience, GCC doesn't always catch things like this), and should compile a
> little faster (because the optimizers don't have to do as much work).

Fun thing is, there's no speed issues with this portion of the code, and
it's in userspace as well, so simplicity is the key that should be
followed here.

greg k-h

Re: First kernel patch (optimization)

[Eric Curtin]

Hi Greg,

As I said in the subject of the mail (which I have been since told I
shouldn't have done this), I'm a noob to kernel code. I tried to pick
off something super simple to just see what the process of getting a
patch in is. Youtube videos and documentation only get you so far.

>From reading your response, should I refrain from sending in these
micro-optimizations in future? Getting in smaller patches is easier
for me as I only do this in my spare time, which I don't have a lot
of!

On 16 September 2015 at 14:24, Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Wed, Sep 16, 2015 at 07:45:53AM -0400, Austin S Hemmelgarn wrote:
> > On 2015-09-15 20:09, Steve Calfee wrote:
> > >On Tue, Sep 15, 2015 at 12:53 PM, Eric Curtin <ericcurtin17@gmail.com> wrote:
> > >>Signed-off-by: Eric Curtin <ericcurtin17@gmail.com>
> > >>
> > >>diff --git a/tools/usb/usbip/src/usbip_detach.c b/tools/usb/usbip/src/usbip_detach.c
> > >>index 05c6d15..9db9d21 100644
> > >>--- a/tools/usb/usbip/src/usbip_detach.c
> > >>+++ b/tools/usb/usbip/src/usbip_detach.c
> > >>@@ -47,7 +47,9 @@ static int detach_port(char *port)
> > >>         uint8_t portnum;
> > >>         char path[PATH_MAX+1];
> > >>
> > >>-       for (unsigned int i = 0; i < strlen(port); i++)
> > >>+       unsigned int port_len = strlen(port);
> > >>+
> > >>+       for (unsigned int i = 0; i < port_len; i++)
> > >>                 if (!isdigit(port[i])) {
> > >>                         err("invalid port %s", port);
> > >>                         return -1;
> > >>
> > >>--
> > >
> > >Hi Eric,
> > >
> > >This is fine, but what kind of wimpy compiler optimizer will not move
> > >the constant initializer out of the loop? I bet if you compare binary
> > >sizes/code it will be exactly the same, and you added some characters
> > >of code. Reorganizing code for readability is fine, but for compiler
> > >(in)efficiency seems like a bad idea.
> > While I agree with your argument, I would like to point out that it is a
> > well established fact that GCC's optimizers are kind of brain-dead at times
> > and need their hands held.
> >
> > I'd be willing to bet that the code will be marginally larger (because of
> > adding another variable), but might run slightly faster too (because in my
> > experience, GCC doesn't always catch things like this), and should compile a
> > little faster (because the optimizers don't have to do as much work).
>
> Fun thing is, there's no speed issues with this portion of the code, and
> it's in userspace as well, so simplicity is the key that should be
> followed here.
>
> greg k-h

Re: First kernel patch (optimization)

[Theodore Ts'o]

On Wed, Sep 16, 2015 at 05:03:39PM +0100, Eric Curtin wrote:
> Hi Greg,
> 
> As I said in the subject of the mail (which I have been since told I
> shouldn't have done this), I'm a noob to kernel code. I tried to pick
> off something super simple to just see what the process of getting a
> patch in is. Youtube videos and documentation only get you so far.
> 
> From reading your response, should I refrain from sending in these
> micro-optimizations in future? Getting in smaller patches is easier
> for me as I only do this in my spare time, which I don't have a lot
> of!

What I'd ask you to consider is what your end goal?  Is it just to
collect a scalp (woo hoo!  I've gotten a patch into the kernel)?  Or
is it to actually make things better for yourself or other users?  Or
are you trying to get make your self more employable, etc.

Micro-optimizations is often not particularly useful for anything
other than the first goal, and it really doesn't help anyone.

If you're just doing this in your spare time, then hopefully I hope
you are being choosy about what's the best way to use your spare time,
so the question of what your goals are going to be is a very important
thing for you to figure out.  Regardless of whether it's worthwhile to
get this patch into the kernel, doing any *more* micro-optimizations
is probably not a good use of your time or anyone else's.

I'd strongly encourage you to move on to something more than just
micro-optimizations as quickly as possible.

Best regards,

							- Ted

Re: First kernel patch (optimization)

[Raymond Jennings]

On 09/16/15 09:40, Theodore Ts'o wrote:
> On Wed, Sep 16, 2015 at 05:03:39PM +0100, Eric Curtin wrote:
>> Hi Greg,
>>
>> As I said in the subject of the mail (which I have been since told I
>> shouldn't have done this), I'm a noob to kernel code. I tried to pick
>> off something super simple to just see what the process of getting a
>> patch in is. Youtube videos and documentation only get you so far.
>>
>>  From reading your response, should I refrain from sending in these
>> micro-optimizations in future? Getting in smaller patches is easier
>> for me as I only do this in my spare time, which I don't have a lot
>> of!
> What I'd ask you to consider is what your end goal?  Is it just to
> collect a scalp (woo hoo!  I've gotten a patch into the kernel)?  Or
> is it to actually make things better for yourself or other users?  Or
> are you trying to get make your self more employable, etc.
It could well be that he's wanting to practice getting used to the 
development process.

https://lkml.org/lkml/2004/12/20/255
> Micro-optimizations is often not particularly useful for anything
> other than the first goal, and it really doesn't help anyone.
>
> If you're just doing this in your spare time, then hopefully I hope
> you are being choosy about what's the best way to use your spare time,
> so the question of what your goals are going to be is a very important
> thing for you to figure out.  Regardless of whether it's worthwhile to
> get this patch into the kernel, doing any *more* micro-optimizations
> is probably not a good use of your time or anyone else's.
>
> I'd strongly encourage you to move on to something more than just
> micro-optimizations as quickly as possible.
Tytso is right here.  If you want to be useful you should find something 
with real impact once you've learned the ropes.


>
> Best regards,
>
> 							- Ted
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: First kernel patch (optimization)

[Josh Boyer]

Hi Eric,

First of all, thanks for your attempt and I really hope you haven't
been totally discouraged from future participation.  Getting a patch
into the kernel is hard, but I'm pretty disappointed with the
responses you've gotten so far.

On Wed, Sep 16, 2015 at 12:40 PM, Theodore Ts'o <tytso@mit.edu> wrote:
> On Wed, Sep 16, 2015 at 05:03:39PM +0100, Eric Curtin wrote:
>> Hi Greg,
>>
>> As I said in the subject of the mail (which I have been since told I
>> shouldn't have done this), I'm a noob to kernel code. I tried to pick
>> off something super simple to just see what the process of getting a
>> patch in is. Youtube videos and documentation only get you so far.
>>
>> From reading your response, should I refrain from sending in these
>> micro-optimizations in future? Getting in smaller patches is easier
>> for me as I only do this in my spare time, which I don't have a lot
>> of!
>
> What I'd ask you to consider is what your end goal?  Is it just to
> collect a scalp (woo hoo!  I've gotten a patch into the kernel)?  Or
> is it to actually make things better for yourself or other users?  Or
> are you trying to get make your self more employable, etc.

He literally said he wanted to see what the process to get a patch in
was.  Instead of having seasoned kernel developers help him walk
through this, he gets patronizing responses.  Felipe has the only
reply in this thread that is actually good, so thanks Felipe.

> Micro-optimizations is often not particularly useful for anything
> other than the first goal, and it really doesn't help anyone.

That isn't true.  It helps the submitter understand the workflow and
expectations.  What you meant to say is that it doesn't help you.

> If you're just doing this in your spare time, then hopefully I hope
> you are being choosy about what's the best way to use your spare time,
> so the question of what your goals are going to be is a very important
> thing for you to figure out.  Regardless of whether it's worthwhile to
> get this patch into the kernel, doing any *more* micro-optimizations
> is probably not a good use of your time or anyone else's.
>
> I'd strongly encourage you to move on to something more than just
> micro-optimizations as quickly as possible.

Yes, sure.  But in the meantime, actually being encouraging and
helpful to a new contributor might pay off more in the long run.  You
guys might seriously want to consider doing that recruitment topic at
kernel summit.

josh

Re: First kernel patch (optimization)

[Greg KH]

On Wed, Sep 16, 2015 at 05:03:39PM +0100, Eric Curtin wrote:
> Hi Greg,
> 
> As I said in the subject of the mail (which I have been since told I
> shouldn't have done this), I'm a noob to kernel code. I tried to pick
> off something super simple to just see what the process of getting a
> patch in is. Youtube videos and documentation only get you so far.
> 
> >From reading your response, should I refrain from sending in these
> micro-optimizations in future? Getting in smaller patches is easier
> for me as I only do this in my spare time, which I don't have a lot
> of!

micro-optimizations are great, if you can actually measure them and they
matter :)

If you are looking for someplace to start, might I recommend the
drivers/staging/ directory?  I take all sorts of "basic" cleanup
patches, and there are loads of things to do in there.  Look at the
drivers/staging/*/TODO files for specific examples of what needs to be
done to get these files cleaned up and fixed properly.

Hope this helps,

greg k-h

Re: First kernel patch (optimization)

[Eric Curtin]

On 16 September 2015 at 21:02, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Wed, Sep 16, 2015 at 05:03:39PM +0100, Eric Curtin wrote:
>> Hi Greg,
>>
>> As I said in the subject of the mail (which I have been since told I
>> shouldn't have done this), I'm a noob to kernel code. I tried to pick
>> off something super simple to just see what the process of getting a
>> patch in is. Youtube videos and documentation only get you so far.
>>
>> >From reading your response, should I refrain from sending in these
>> micro-optimizations in future? Getting in smaller patches is easier
>> for me as I only do this in my spare time, which I don't have a lot
>> of!
>
> micro-optimizations are great, if you can actually measure them and they
> matter :)
>
> If you are looking for someplace to start, might I recommend the
> drivers/staging/ directory?  I take all sorts of "basic" cleanup
> patches, and there are loads of things to do in there.  Look at the
> drivers/staging/*/TODO files for specific examples of what needs to be
> done to get these files cleaned up and fixed properly.
>
> Hope this helps,
>
> greg k-h

I have realized this now after reading
http://kernelnewbies.org/FirstKernelPatch, should have done a bit more
reading before I submitted something. I'm looking at the
drivers/staging/android stuff now.

Eric Curtin

Re: First kernel patch (optimization)

[Greg KH]

On Wed, Sep 16, 2015 at 09:21:58PM +0100, Eric Curtin wrote:
> On 16 September 2015 at 21:02, Greg KH <gregkh@linuxfoundation.org> wrote:
> > On Wed, Sep 16, 2015 at 05:03:39PM +0100, Eric Curtin wrote:
> >> Hi Greg,
> >>
> >> As I said in the subject of the mail (which I have been since told I
> >> shouldn't have done this), I'm a noob to kernel code. I tried to pick
> >> off something super simple to just see what the process of getting a
> >> patch in is. Youtube videos and documentation only get you so far.
> >>
> >> >From reading your response, should I refrain from sending in these
> >> micro-optimizations in future? Getting in smaller patches is easier
> >> for me as I only do this in my spare time, which I don't have a lot
> >> of!
> >
> > micro-optimizations are great, if you can actually measure them and they
> > matter :)
> >
> > If you are looking for someplace to start, might I recommend the
> > drivers/staging/ directory?  I take all sorts of "basic" cleanup
> > patches, and there are loads of things to do in there.  Look at the
> > drivers/staging/*/TODO files for specific examples of what needs to be
> > done to get these files cleaned up and fixed properly.
> >
> > Hope this helps,
> >
> > greg k-h
> 
> I have realized this now after reading
> http://kernelnewbies.org/FirstKernelPatch, should have done a bit more
> reading before I submitted something. I'm looking at the
> drivers/staging/android stuff now.

The android code is all pretty "clean" right now, nothing there for
anyone who is not an Android kernel developer to do.  Parts are about to
be deleted, and others (i.e. ION), has to stay as-is while we wait for
other kernel bits to get merged (see the updated README file in
linux-next for details about that.)

So I'd recommend something else in the staging tree, android is a bit of
an exception as to why it is in that part of the kernel at the moment...

thanks,

greg k-h

Re: First kernel patch (optimization)

[Jaime Arrocha]

On 09/16/2015 07:56 AM, David Laight wrote:
> From: Austin S Hemmelgarn
>> Sent: 16 September 2015 12:46
>> On 2015-09-15 20:09, Steve Calfee wrote:
>>> On Tue, Sep 15, 2015 at 12:53 PM, Eric Curtin <ericcurtin17@gmail.com> wrote:
>>>> Signed-off-by: Eric Curtin <ericcurtin17@gmail.com>
>>>>
>>>> diff --git a/tools/usb/usbip/src/usbip_detach.c b/tools/usb/usbip/src/usbip_detach.c
>>>> index 05c6d15..9db9d21 100644
>>>> --- a/tools/usb/usbip/src/usbip_detach.c
>>>> +++ b/tools/usb/usbip/src/usbip_detach.c
>>>> @@ -47,7 +47,9 @@ static int detach_port(char *port)
>>>>           uint8_t portnum;
>>>>           char path[PATH_MAX+1];
>>>>
>>>> -
>>>> +       unsigned int port_len = strlen(port);
>>>> +
>>>> +       for (unsigned int i = 0; i < port_len; i++)
>>>>                   if (!isdigit(port[i])) {
>>>>                           err("invalid port %s", port);
>>>>                           return -1;
>>>>
>>>> --
>>> Hi Eric,
>>>
>>> This is fine, but what kind of wimpy compiler optimizer will not move
>>> the constant initializer out of the loop? I bet if you compare binary
>>> sizes/code it will be exactly the same, and you added some characters
>>> of code. Reorganizing code for readability is fine, but for compiler
>>> (in)efficiency seems like a bad idea.
>> While I agree with your argument, I would like to point out that it is a
>> well established fact that GCC's optimizers are kind of brain-dead at
>> times and need their hands held.
>>
>> I'd be willing to bet that the code will be marginally larger (because
>> of adding another variable), but might run slightly faster too (because
>> in my experience, GCC doesn't always catch things like this), and should
>> compile a little faster (because the optimizers don't have to do as much
>> work).
> The compiler probably can't optimise the strlen().
> If isdigit() is a real function (the locale specific one probably is)
> then the compile cannot assume that port[n] isn't changed by the call
> to isdigit.
>
> A simpler change would be:
> 	for (unsigned int i = 0; port[i] != 0; i++)
>
> Much better would be to use strtoul() instead of atoi().
>
> 	David
>
I actually took some time to verify this. GCC makes this optimization 
with -O2 at least on gcc 4.7.2.
One interesting observation I found was that in O0 and O2, it does make 
a call to strlen while in O1 it calculates
the length of the string using:

repnz scas    %es:(%rdi),%al
not                %rcx
sub               $0x2,%rcx

Why does it do that? Is the code above faster? If yes, why not do it in 
O2 too?
Is this still a topic for this forum?


gcc version 4.7.2 (Debian 4.7.2-5)
code

void conv_input(char *port)
{
     int portnum;

     for(int i = 0; i <strlen(port); i++)
         if(!isdigit(port[i])) {
             printf("invalid port %s", port);
             exit (1);
         }

     portnum = atoi(port);
     printf("Port number: %d\n", portnum);
}

Optimization done?
              O0    O1    O2
x86        No    No    Yes
amd64   No   No    Yes

RE: First kernel patch (optimization)

[David Laight]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 982 bytes --]

From: Jaime Arrocha
> Sent: 17 September 2015 02:50
..
> One interesting observation I found was that in O0 and O2, it does make
> a call to strlen while in O1 it calculates
> the length of the string using:
> 

You want an 'xor %rcx,%rcx' here.
> repnz scas    %es:(%rdi),%al
> not                %rcx
> sub               $0x2,%rcx
> 
> Why does it do that? Is the code above faster? If yes, why not do it in
> O2 too?

Because 'repnz scasb' is slow, especially on some cpu types.
It may win for -Os on 32 bit systems.
Pentium 4 netburst have about 40 clocks setup for all the 'rep' instructions,
later cpus are better but you might still be talking double figures.
On 64 bit cpu there are much faster ways of detecting a zero byte in a
64 bit word by using shifts and masks - so the function call can be a win.

	David

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: First kernel patch (optimization)

[Theodore Ts'o]

On Wed, Sep 16, 2015 at 01:26:51PM -0400, Josh Boyer wrote:
> 
> That isn't true.  It helps the submitter understand the workflow and
> expectations.  What you meant to say is that it doesn't help you.


The problem is that workflow isn't the hard part.  It's the part that
can be taught most easily, sure.  But people seem to get really hung
up on it, and I fear that we have people who never progress beyond
sending trivial patches and spelling fixes and white space fixes and
micro-optimizations.

If the "you too can be a kernel developer" classes and web sites and
tutorials also taught people how to take performance measurements, and
something about the scientific measurement, that would be something.
Or if it taught people how to create tests and to run regression
testing.  Or if it taught people how to try to do fuzz testing, and
then once they find a sequence which causes crash, how to narrow down
the failure to a specific part of the kernel, and how to fix and
confirm that the kernel no longer crashes with the fix --- that would
be useful.

If they can understand kernel code; if they can understand the
scientific measurement; if they can understand how to do performance
measurements --- being able to properly format patches is something
which most kernel developers can very easily guide a new contributor
to do correctly.  Or in the worst case, it doesn't take much time for
me to fix a whitespace problem and just tell the contributor --- by
the way, I fixed up this minor issue; could you please make sure you
do this in the future?

But if a test hasn't been tested, or if the contributor things it's a
micro-optimization, but it actually takes more CPU time and/or more
stack space and/or bloats the kernel --- that's much more work for the
kernel maintainer to have to deal with when reviewing a patch.

So I have a very strong disagreement with the belief that teaching
people the workflow is the more important thing.  In my mind, that's
like first focusing on the proper how to properly fill out a golf
score card, and the ettiquette and traditions around handicaps, etc
--- before making sure the prospective player is good at putting and
driving.  Personally, I'm terrible at putting and driving, so spending
a lot of time learning how to fill out a golf score card would be a
waste of my time.

A good kernel programmer has to understand systems thinking; how to
figure out abstractions and when it's a good thing to add a new layer
of abstraction and when it's better to rework an exsting abstraction
layer.

If we have someone who knows the workflow, but which doesn't
understand systems thinking, or how to do testing, then what?  Great,
we've just created another Nick Krause.  Do you think encouraging a
Nick Krause helps anyone?

If people really are hung up on learning the workflow, I don't mind if
they want to learn that part and send some silly micro-optimization or
spelling fix or whitespace fix.  But it's really, really important
that they move beyond that.  And if they aren't capable of moving
beyond that, trying to inflate are recruitment numbers by encouraging
someone who can only do trivial fixes means that we may be get what we
can easily measure --- but it may not be what we really need as a
community.

Cheers,

						- Ted

Re: First kernel patch (optimization)

[Greg KH]

On Thu, Sep 17, 2015 at 11:12:51PM -0400, Theodore Ts'o wrote:
> On Wed, Sep 16, 2015 at 01:26:51PM -0400, Josh Boyer wrote:
> > 
> > That isn't true.  It helps the submitter understand the workflow and
> > expectations.  What you meant to say is that it doesn't help you.
> 
> 
> The problem is that workflow isn't the hard part.  It's the part that
> can be taught most easily, sure.  But people seem to get really hung
> up on it, and I fear that we have people who never progress beyond
> sending trivial patches and spelling fixes and white space fixes and
> micro-optimizations.
> 
> If the "you too can be a kernel developer" classes and web sites and
> tutorials also taught people how to take performance measurements, and
> something about the scientific measurement, that would be something.
> Or if it taught people how to create tests and to run regression
> testing.  Or if it taught people how to try to do fuzz testing, and
> then once they find a sequence which causes crash, how to narrow down
> the failure to a specific part of the kernel, and how to fix and
> confirm that the kernel no longer crashes with the fix --- that would
> be useful.
> 
> If they can understand kernel code; if they can understand the
> scientific measurement; if they can understand how to do performance
> measurements --- being able to properly format patches is something
> which most kernel developers can very easily guide a new contributor
> to do correctly.  Or in the worst case, it doesn't take much time for
> me to fix a whitespace problem and just tell the contributor --- by
> the way, I fixed up this minor issue; could you please make sure you
> do this in the future?
> 
> But if a test hasn't been tested, or if the contributor things it's a
> micro-optimization, but it actually takes more CPU time and/or more
> stack space and/or bloats the kernel --- that's much more work for the
> kernel maintainer to have to deal with when reviewing a patch.
> 
> So I have a very strong disagreement with the belief that teaching
> people the workflow is the more important thing.  In my mind, that's
> like first focusing on the proper how to properly fill out a golf
> score card, and the ettiquette and traditions around handicaps, etc
> --- before making sure the prospective player is good at putting and
> driving.  Personally, I'm terrible at putting and driving, so spending
> a lot of time learning how to fill out a golf score card would be a
> waste of my time.
> 
> A good kernel programmer has to understand systems thinking; how to
> figure out abstractions and when it's a good thing to add a new layer
> of abstraction and when it's better to rework an exsting abstraction
> layer.
> 
> If we have someone who knows the workflow, but which doesn't
> understand systems thinking, or how to do testing, then what?  Great,
> we've just created another Nick Krause.  Do you think encouraging a
> Nick Krause helps anyone?
> 
> If people really are hung up on learning the workflow, I don't mind if
> they want to learn that part and send some silly micro-optimization or
> spelling fix or whitespace fix.  But it's really, really important
> that they move beyond that.  And if they aren't capable of moving
> beyond that, trying to inflate are recruitment numbers by encouraging
> someone who can only do trivial fixes means that we may be get what we
> can easily measure --- but it may not be what we really need as a
> community.

Ted, you are full of crap.

Where do you think that "new developers" come from?  Do they show up in
our inbox, with full knowledge of kernel internals and OS theory yet
they somehow just can't grasp how to submit a patch correctly?  Yes,
they sometimes rarely do.  But for the majority of people who got into
Linux, that is not the case at all.

People need to start with something simple, and easy, to get over the
hurdles of:
	- generating a patch
	- sending an email
	- fixing the email client as it just corrupted the patch
	- fix the subject line as it was incorrect
	- fix the changelog as it was missing
	- fix the email client again as it corrupted the patch in a
	  different way
	- giving up on using a web email client as it just will not work
	- figuring out who to send the patch to
	- fixing the email client as the mailing list bounced the email

Those are non-trivial tasks.  And by starting with "remove this space"
you take the worry away from the specific content of the patch, and let
them worry about the "hard" part first.

Then, after all of the above is finished, and working, then they can
start submitting real patches, that do real things, in patch series, as
they can focus on the content much more, as the problems of how to make
the patch into an acceptable format is not an issue anymore.

I see this every single day with patches in the staging tree, which is
why it is there, and is why I don't just go and remove all coding style
errors in one fell-swoop tomorrow.  We need to provide those "simple"
tasks to get people involved in our community and over the hurdle of
sending a patch in.

And from there, then people go on to contribute "real" things.  There
are _many_ subsystem maintainers that started out submitting whitespace
patches.  There are even more developers who have gotten "real" jobs by
starting out submitting whitespace patches and personally I find that a
much more satisfying metric than more subsystem maintainers.

Yes, not everyone who sends in cleanup patches will go on to be a
maintainer, or get a job, but the thing is, you can't judge who will, or
will not, be that person.  What we have to do, and what we must do, is
accept everyone into our community as somewhere, someone is out there
that you will want to take over for your subsystem when you are tired of
it.  And by turning away people from doing things to get over our first
hurdles by adding to it by forcing someone to make a "real"
contribution, you just lost that person forever.

So don't take cleanup patches for your code, that's fine, and I totally
understand why _you_ don't want to do that.  But to blow off the effort
as being somehow trivial and not worthy of us, that's totally missing
the point, and making things harder on yourself in the end.

Just point people at staging, that's what it is there for, and is what
I, and the developers who help maintain it, do every single day because
we know it is essential for the future of Linux to do so.

And if they don't move on beyond whitespace cleanups, that's fine too,
I'll still gladly take those patches, and do so.  But almost always the
person moves from that into doing something "real" or they tire of it
and stop contributing.  By somehow pre-rejecting these people, you are
pre-judging them as well, a _VERY_ dangerous thing to do to anyone.

Do you know what _my_ first email was to lkml all those years ago?  "How
do you make a patch in the proper format?"  And I asked it because I saw
a "trivial" change that I was able to make.  And do you know who
answered that question?  Someone who ended up being my manager for many
years, and now runs SuSE Labs.  He was wise enough to take the time to
help a "newbie" like myself because he knew that we all started
somewhere, and that we never know just who that "newbie" might turn out
to be in the end.

So don't be a jerk, and spout off as to how we only need "skilled"
people contributing.  Of course we need that, but the way we get those
people is to help them become skilled, not requiring them to show up
with those skills automatically.

greg k-h

Re: First kernel patch (optimization)

[Raymond Jennings]

On 09/18/15 00:42, Greg KH wrote:
> On Thu, Sep 17, 2015 at 11:12:51PM -0400, Theodore Ts'o wrote:
>> On Wed, Sep 16, 2015 at 01:26:51PM -0400, Josh Boyer wrote:
>>> That isn't true.  It helps the submitter understand the workflow and
>>> expectations.  What you meant to say is that it doesn't help you.
>> The problem is that workflow isn't the hard part.  It's the part that
>> can be taught most easily, sure.  But people seem to get really hung
>> up on it, and I fear that we have people who never progress beyond
>> sending trivial patches and spelling fixes and white space fixes and
>> micro-optimizations.
>>
>> If the "you too can be a kernel developer" classes and web sites and
>> tutorials also taught people how to take performance measurements, and
>> something about the scientific measurement, that would be something.
>> Or if it taught people how to create tests and to run regression
>> testing.  Or if it taught people how to try to do fuzz testing, and
>> then once they find a sequence which causes crash, how to narrow down
>> the failure to a specific part of the kernel, and how to fix and
>> confirm that the kernel no longer crashes with the fix --- that would
>> be useful.
>>
>> If they can understand kernel code; if they can understand the
>> scientific measurement; if they can understand how to do performance
>> measurements --- being able to properly format patches is something
>> which most kernel developers can very easily guide a new contributor
>> to do correctly.  Or in the worst case, it doesn't take much time for
>> me to fix a whitespace problem and just tell the contributor --- by
>> the way, I fixed up this minor issue; could you please make sure you
>> do this in the future?
>>
>> But if a test hasn't been tested, or if the contributor things it's a
>> micro-optimization, but it actually takes more CPU time and/or more
>> stack space and/or bloats the kernel --- that's much more work for the
>> kernel maintainer to have to deal with when reviewing a patch.
>>
>> So I have a very strong disagreement with the belief that teaching
>> people the workflow is the more important thing.  In my mind, that's
>> like first focusing on the proper how to properly fill out a golf
>> score card, and the ettiquette and traditions around handicaps, etc
>> --- before making sure the prospective player is good at putting and
>> driving.  Personally, I'm terrible at putting and driving, so spending
>> a lot of time learning how to fill out a golf score card would be a
>> waste of my time.
>>
>> A good kernel programmer has to understand systems thinking; how to
>> figure out abstractions and when it's a good thing to add a new layer
>> of abstraction and when it's better to rework an exsting abstraction
>> layer.
>>
>> If we have someone who knows the workflow, but which doesn't
>> understand systems thinking, or how to do testing, then what?  Great,
>> we've just created another Nick Krause.  Do you think encouraging a
>> Nick Krause helps anyone?
>>
>> If people really are hung up on learning the workflow, I don't mind if
>> they want to learn that part and send some silly micro-optimization or
>> spelling fix or whitespace fix.  But it's really, really important
>> that they move beyond that.  And if they aren't capable of moving
>> beyond that, trying to inflate are recruitment numbers by encouraging
>> someone who can only do trivial fixes means that we may be get what we
>> can easily measure --- but it may not be what we really need as a
>> community.
> Ted, you are full of crap.
>
> Where do you think that "new developers" come from?  Do they show up in
> our inbox, with full knowledge of kernel internals and OS theory yet
> they somehow just can't grasp how to submit a patch correctly?  Yes,
> they sometimes rarely do.  But for the majority of people who got into
> Linux, that is not the case at all.
>
> People need to start with something simple, and easy, to get over the
> hurdles of:
> 	- generating a patch
> 	- sending an email
> 	- fixing the email client as it just corrupted the patch
> 	- fix the subject line as it was incorrect
> 	- fix the changelog as it was missing
> 	- fix the email client again as it corrupted the patch in a
> 	  different way
> 	- giving up on using a web email client as it just will not work
> 	- figuring out who to send the patch to
> 	- fixing the email client as the mailing list bounced the email
>
> Those are non-trivial tasks.  And by starting with "remove this space"
> you take the worry away from the specific content of the patch, and let
> them worry about the "hard" part first.
+1 for this.

For example, I for one cannot tell you how many times gmail snuck html 
sections into my outgoing emails before I finally caught it red handed 
and switched to using a local native client.

> Then, after all of the above is finished, and working, then they can
> start submitting real patches, that do real things, in patch series, as
> they can focus on the content much more, as the problems of how to make
> the patch into an acceptable format is not an issue anymore.

Did anyone read linus torvald's post that I linked to earlier?

It was very much relevant to the present situation and covers something 
like this that happened before with a newbie developer.

> I see this every single day with patches in the staging tree, which is
> why it is there, and is why I don't just go and remove all coding style
> errors in one fell-swoop tomorrow.  We need to provide those "simple"
> tasks to get people involved in our community and over the hurdle of
> sending a patch in.
>
> And from there, then people go on to contribute "real" things.  There
> are _many_ subsystem maintainers that started out submitting whitespace
> patches.  There are even more developers who have gotten "real" jobs by
> starting out submitting whitespace patches and personally I find that a
> much more satisfying metric than more subsystem maintainers.
>
> Yes, not everyone who sends in cleanup patches will go on to be a
> maintainer, or get a job, but the thing is, you can't judge who will, or
> will not, be that person.  What we have to do, and what we must do, is
> accept everyone into our community as somewhere, someone is out there
> that you will want to take over for your subsystem when you are tired of
> it.  And by turning away people from doing things to get over our first
> hurdles by adding to it by forcing someone to make a "real"
> contribution, you just lost that person forever.
>
> So don't take cleanup patches for your code, that's fine, and I totally
> understand why _you_ don't want to do that.  But to blow off the effort
> as being somehow trivial and not worthy of us, that's totally missing
> the point, and making things harder on yourself in the end.
>
> Just point people at staging, that's what it is there for, and is what
> I, and the developers who help maintain it, do every single day because
> we know it is essential for the future of Linux to do so.
>
> And if they don't move on beyond whitespace cleanups, that's fine too,
> I'll still gladly take those patches, and do so.  But almost always the
> person moves from that into doing something "real" or they tire of it
> and stop contributing.  By somehow pre-rejecting these people, you are
> pre-judging them as well, a _VERY_ dangerous thing to do to anyone.
>
> Do you know what _my_ first email was to lkml all those years ago?  "How
> do you make a patch in the proper format?"  And I asked it because I saw
> a "trivial" change that I was able to make.  And do you know who
> answered that question?  Someone who ended up being my manager for many
> years, and now runs SuSE Labs.  He was wise enough to take the time to
> help a "newbie" like myself because he knew that we all started
> somewhere, and that we never know just who that "newbie" might turn out
> to be in the end.
>
> So don't be a jerk, and spout off as to how we only need "skilled"
> people contributing.  Of course we need that, but the way we get those
> people is to help them become skilled, not requiring them to show up
> with those skills automatically.
>
> greg k-h
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: First kernel patch (optimization)

[Austin S Hemmelgarn]

[-- Attachment #1: Type: text/plain, Size: 5793 bytes --]

On 2015-09-18 05:31, Raymond Jennings wrote:
> On 09/18/15 00:42, Greg KH wrote:
>> On Thu, Sep 17, 2015 at 11:12:51PM -0400, Theodore Ts'o wrote:
>>> On Wed, Sep 16, 2015 at 01:26:51PM -0400, Josh Boyer wrote:
>>>> That isn't true.  It helps the submitter understand the workflow and
>>>> expectations.  What you meant to say is that it doesn't help you.
>>> The problem is that workflow isn't the hard part.  It's the part that
>>> can be taught most easily, sure.  But people seem to get really hung
>>> up on it, and I fear that we have people who never progress beyond
>>> sending trivial patches and spelling fixes and white space fixes and
>>> micro-optimizations.
>>>
>>> If the "you too can be a kernel developer" classes and web sites and
>>> tutorials also taught people how to take performance measurements, and
>>> something about the scientific measurement, that would be something.
>>> Or if it taught people how to create tests and to run regression
>>> testing.  Or if it taught people how to try to do fuzz testing, and
>>> then once they find a sequence which causes crash, how to narrow down
>>> the failure to a specific part of the kernel, and how to fix and
>>> confirm that the kernel no longer crashes with the fix --- that would
>>> be useful.
>>>
>>> If they can understand kernel code; if they can understand the
>>> scientific measurement; if they can understand how to do performance
>>> measurements --- being able to properly format patches is something
>>> which most kernel developers can very easily guide a new contributor
>>> to do correctly.  Or in the worst case, it doesn't take much time for
>>> me to fix a whitespace problem and just tell the contributor --- by
>>> the way, I fixed up this minor issue; could you please make sure you
>>> do this in the future?
>>>
>>> But if a test hasn't been tested, or if the contributor things it's a
>>> micro-optimization, but it actually takes more CPU time and/or more
>>> stack space and/or bloats the kernel --- that's much more work for the
>>> kernel maintainer to have to deal with when reviewing a patch.
>>>
>>> So I have a very strong disagreement with the belief that teaching
>>> people the workflow is the more important thing.  In my mind, that's
>>> like first focusing on the proper how to properly fill out a golf
>>> score card, and the ettiquette and traditions around handicaps, etc
>>> --- before making sure the prospective player is good at putting and
>>> driving.  Personally, I'm terrible at putting and driving, so spending
>>> a lot of time learning how to fill out a golf score card would be a
>>> waste of my time.
>>>
>>> A good kernel programmer has to understand systems thinking; how to
>>> figure out abstractions and when it's a good thing to add a new layer
>>> of abstraction and when it's better to rework an exsting abstraction
>>> layer.
>>>
>>> If we have someone who knows the workflow, but which doesn't
>>> understand systems thinking, or how to do testing, then what?  Great,
>>> we've just created another Nick Krause.  Do you think encouraging a
>>> Nick Krause helps anyone?
>>>
>>> If people really are hung up on learning the workflow, I don't mind if
>>> they want to learn that part and send some silly micro-optimization or
>>> spelling fix or whitespace fix.  But it's really, really important
>>> that they move beyond that.  And if they aren't capable of moving
>>> beyond that, trying to inflate are recruitment numbers by encouraging
>>> someone who can only do trivial fixes means that we may be get what we
>>> can easily measure --- but it may not be what we really need as a
>>> community.
>> Ted, you are full of crap.
>>
>> Where do you think that "new developers" come from?  Do they show up in
>> our inbox, with full knowledge of kernel internals and OS theory yet
>> they somehow just can't grasp how to submit a patch correctly?  Yes,
>> they sometimes rarely do.  But for the majority of people who got into
>> Linux, that is not the case at all.
>>
>> People need to start with something simple, and easy, to get over the
>> hurdles of:
>>     - generating a patch
>>     - sending an email
>>     - fixing the email client as it just corrupted the patch
>>     - fix the subject line as it was incorrect
>>     - fix the changelog as it was missing
>>     - fix the email client again as it corrupted the patch in a
>>       different way
>>     - giving up on using a web email client as it just will not work
>>     - figuring out who to send the patch to
>>     - fixing the email client as the mailing list bounced the email
>>
>> Those are non-trivial tasks.  And by starting with "remove this space"
>> you take the worry away from the specific content of the patch, and let
>> them worry about the "hard" part first.
> +1 for this.
>
> For example, I for one cannot tell you how many times gmail snuck html
> sections into my outgoing emails before I finally caught it red handed
> and switched to using a local native client.
>
It's also worth pointing out that if we don't have people submitting 
code cleanups and typo fixes and stuff like that, then either we end up 
over time with unmaintainable gibberish, or the people who do know 
systems programming spending most of their time trying to deal with 
cleaning up the code (and this is coming from someone who works for a 
company who's software department used to be almost 50 people and is now 
5, and we have exactly this problem (that and for some reason no-one 
wants to do any kind of code-review, but that's a separate issue)).  So 
in a way, they are contributing something useful, just not something 
that directly impacts users in any immediate time-frame.


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 3019 bytes --]

Re: First kernel patch (optimization)

[Theodore Ts'o]

On Fri, Sep 18, 2015 at 12:42:48AM -0700, Greg KH wrote:
> So don't take cleanup patches for your code, that's fine, and I totally
> understand why _you_ don't want to do that.  But to blow off the effort
> as being somehow trivial and not worthy of us, that's totally missing
> the point, and making things harder on yourself in the end.

It's not that I think cleanup patches are "trivial and not worthy of
us", although I'll note that cleanup patches can end up causing real
bug fixes (including possibly fixes that address security problems) to
not apply to stable kernels, which means someone needs to deal with
failed patches --- or what is much more likely, the failed patch gets
bounced back to the overworked maintainer who then drops the patch
because he or she doesn't have the bandwidth to manually backport the
patch to N different stable trees, and then we all suffer.  So cleanup
patches *do* have a cost.

Rather, what concerns me is that we aren't pushing people to go
*beyond* cleanup patches.  We have lots of tutorials about how to
create perfectly formed patches; but we don't seem to have patches
about how to do proper benchmarking.  Or how to check system call and
ioctl interfaces to make sure the code is doing appropriate input
checking.  So I don't want to pre-reject these people; but to rather
push them and to help them to try their hand at more substantive work.

What surprises me is the apparent assumption that people need a huge
amount of hand-holding on how to properly form a patch, but that
people will be able to figure out how to do proper benchmarking, or
design proper abstractions, etc., as skills that will magically appear
full-formed into the new kernel programmer's mind without any help or
work on our part.

> So don't be a jerk, and spout off as to how we only need "skilled"
> people contributing.  Of course we need that, but the way we get those
> people is to help them become skilled, not requiring them to show up
> with those skills automatically.

I think where we disagree is in how to make them become skilled.  I
don't think just focusing on contents of SubmittingPatches is
sufficient, and there seems to be a "Step 2: And then a miracle
occurs" between the SubmittingPatches step and the "skilled
contributor" end goal.

Cheers,

						- Ted

Re: First kernel patch (optimization)

[Sudip Mukherjee]

On Fri, Sep 18, 2015 at 10:26:24PM -0400, Theodore Ts'o wrote:
> On Fri, Sep 18, 2015 at 12:42:48AM -0700, Greg KH wrote:
>
> Rather, what concerns me is that we aren't pushing people to go
> *beyond* cleanup patches.  We have lots of tutorials about how to
> create perfectly formed patches; but we don't seem to have patches
> about how to do proper benchmarking.  Or how to check system call and
> ioctl interfaces to make sure the code is doing appropriate input
> checking.  So I don't want to pre-reject these people; but to rather
> push them and to help them to try their hand at more substantive work.
> 
> What surprises me is the apparent assumption that people need a huge
> amount of hand-holding on how to properly form a patch, but that
> people will be able to figure out how to do proper benchmarking, or
> design proper abstractions, etc., as skills that will magically appear
> full-formed into the new kernel programmer's mind without any help or
> work on our part.
Reading Ted's earlier mail I was thinking since I don't have some of the
skills mentioned, maybe I am in wrong place. But what Ted said now is
almost the same what I said in the Ksummit discussion about recruitment.
I will copy-paste here for reference:
"In my opinion the main problem is lack of direction or guidance. As a 
newbie I send my first patch, it gets accepted, I have a party to
celebrate and do more style correction and few more patches are accepted.
But by that time I am getting bored with just style correction and want
to do something more.
Now the problem starts. No one is there to guide me and I as a newbie
will not be that much capable enough to find things to do on my own. And
I start loosing the interest. Newbies who are coming from Eudyptula or
starting on their own will face this. But on the otherhand participants
of Outreachy will get a Mentor to guide them and gets a stipend to keep
them motivated. Stipend may not matter to the right candidate who has
interest but having a mentor is the big difference."

This is from my own experience as I have gone through that time phrase.
But now after one year I know there are numerous things to do. But still
I don't have some of the skills Ted mentioned.

I know I will get the skills which I don't have now but since I am on my
own that will be a time consuming process. Even more time consuming as
this is not part of my dayjob. But if I had a mentor/guide who could
have given some hint the process might have been much much faster.

regards
sudip

Re: First kernel patch (optimization)

[Greg KH]

On Fri, Sep 18, 2015 at 10:26:24PM -0400, Theodore Ts'o wrote:
> On Fri, Sep 18, 2015 at 12:42:48AM -0700, Greg KH wrote:
> > So don't take cleanup patches for your code, that's fine, and I totally
> > understand why _you_ don't want to do that.  But to blow off the effort
> > as being somehow trivial and not worthy of us, that's totally missing
> > the point, and making things harder on yourself in the end.
> 
> It's not that I think cleanup patches are "trivial and not worthy of
> us", although I'll note that cleanup patches can end up causing real
> bug fixes (including possibly fixes that address security problems) to
> not apply to stable kernels, which means someone needs to deal with
> failed patches --- or what is much more likely, the failed patch gets
> bounced back to the overworked maintainer who then drops the patch
> because he or she doesn't have the bandwidth to manually backport the
> patch to N different stable trees, and then we all suffer.  So cleanup
> patches *do* have a cost.

All patches have a "cost", and that cost is something that you have to
deal with as a maintainer.  Nothing new here at all, and if you don't
like cleanup patches, great, don't take them.  But never go around
saying that people should NOT send cleanup patches to subsystems you
don't maintain like you did.  That's rude and unhelpful to everyone
involved.

The best way to ensure that you never get whitespace patches, is to
clean your subsystem up.  And frankly, it needs a lot of work, have you
run checkpatch on it in a while?  You could resolve this issue for
yourself with a simple afternoon's worth of work.  Why you don't do so
is a mystery to me.

> Rather, what concerns me is that we aren't pushing people to go
> *beyond* cleanup patches.

On the contrary, I do this ALL THE TIME!  Maybe you don't see it as you
aren't on the staging mailing lists or other places where I do this.
Maybe if you started accepting cleanup patches to your subsystem, you
could start doing this yourself as well.

> We have lots of tutorials about how to
> create perfectly formed patches; but we don't seem to have patches
> about how to do proper benchmarking.  Or how to check system call and
> ioctl interfaces to make sure the code is doing appropriate input
> checking.  So I don't want to pre-reject these people; but to rather
> push them and to help them to try their hand at more substantive work.

You first have to crawl before you can run.

And write those tutorials please.  The act of writing the "write your
first kernel patch" tutorial showed that it is NOT a trivial thing to
do, and that there are tons of steps involved now that people use web
email clients and email servers that corrupt text emails (i.e.
Exchange), things that when we started out doing kernel development were
not an issue at all.

> What surprises me is the apparent assumption that people need a huge
> amount of hand-holding on how to properly form a patch, but that
> people will be able to figure out how to do proper benchmarking, or
> design proper abstractions, etc., as skills that will magically appear
> full-formed into the new kernel programmer's mind without any help or
> work on our part.

I have never said that at all, and have actively worked for the past 2
years to help provide guidance and work to get people from the "cleanup
patch" to "writing real code."  And it works, look at the Outreachy
program for loads of examples of this (i.e. almost every person who goes
through it gets a job offer.)

I have been saying for years that we have a lack of real projects /
tasks / ideas for people who are skilled, yet have no idea what to do.
I know of well over a hundred people I have email addresses of that have
asked me for these types of things, and have patches in the kernel that
are non-trivial to prove that they have the skill to do real things.

It's a real problem, and one that I don't have an answer for.  We need
these types of tasks, and I don't have them, and every maintainer I ask
about it also doesn't have them.  What we are asking for is people to
somehow come up with tasks on their own, as if they know what needs to
be done.

Remember, when we started, the number of things that needed to be done
was hugely obvious, everywhere we turned needed lots of work.  For
people new to the kernel, this isn't the case anymore.  Linux has taken
over the world because it is so capable and feature rich.  Picking up a
week-end task is almost impossible because of this.  I know, I have no
week-end-length tasks on my TODO list.

So, have any tasks that people can do on a week-end that you know of?

> > So don't be a jerk, and spout off as to how we only need "skilled"
> > people contributing.  Of course we need that, but the way we get those
> > people is to help them become skilled, not requiring them to show up
> > with those skills automatically.
> 
> I think where we disagree is in how to make them become skilled.  I
> don't think just focusing on contents of SubmittingPatches is
> sufficient, and there seems to be a "Step 2: And then a miracle
> occurs" between the SubmittingPatches step and the "skilled
> contributor" end goal.

Not at all, I have been working on making people skilled on numerous
projects, and to be frank, it's been working (i.e I have real numbers
and data to back it up and have presented at numerous conferences in
2014 all about it).  But the lack of "what do I do now that I have the
skill" is the major piece we are missing at the moment.

And again, don't knock the basic whitespace patch.  It is non-trivial,
see the tutorials for proof of that.

And please, NEVER chide someone for contributing whitespace patches,
it's a sure way to ensure that this person never contributes again,
something that I hope you agree is toxic to our future.

thanks,

greg k-h

Re: First kernel patch (optimization)

[Theodore Ts'o]

On Fri, Sep 18, 2015 at 10:18:27PM -0700, Greg KH wrote:
> And again, don't knock the basic whitespace patch.  It is non-trivial,
> see the tutorials for proof of that.
> 
> And please, NEVER chide someone for contributing whitespace patches,
> it's a sure way to ensure that this person never contributes again,
> something that I hope you agree is toxic to our future.

Not less than 24 hours ago, I received a "my second kernel patch[1]"
contribution which doesn't even compile (and no, it wasn't from Nick
Krause).  It was a hundred+ lines of diffs of the form:

-	printf("Hello, world\n");
+	printf("Hello,
+			world\n");

and

-	foo(a, b, c, d); /* nowhere near 80 characters */
+	foo(a, b,        /* Oooookay; so what was the point? */
+	    c, d);

(Reference ommitted because I don't want to embarass the contributor;
people who really want to find it should have no trouble doing so.)

I didn't childe the contributor for submitting a whitespace patch.  I
childed him on creating a patch which didn't build after you applied
it.  And I pointed him at kvm-xfstests and gce-xfstests, for which I
have spent a lot of time making them as turn key and as easy to use as
possible, specifically because I *am* trying to make life easier for
newcomers.  (Although if we have people who aren't clear on the
concept of valid C code, I'm not sure how much any tutorials or making
test suites more accessible is really going to help.  :-)

I am sure that it is indeed very hard for a certain class of people to
create a basic whitespace patch.  What I haven't seen the data for is
that which addresses the question: for the people for which creating a
whitespace patch is difficult, what percentage of them have the
capability to eventually develop the advanced skills we need for
kernel development?  And how much effort do we need to invest for them
to accomplish this, and how does the amount of investment required to
get them to that stage relate to amount of return these people will
contribute to the community --- that is, is this a charitable donation
where we are asking maintainers to invest in something that will
ultimately not benefit them, or is this a wise long-term investment?

People may still be willing to devote energy if it achieves some other
desirable long-term goal (i.e., increasing the representation of
under-represented groups), even if it doesn't help their subsystem
directly.  But it's useful to know how to pitch the "ask" in terms of
what their expectations should be.


> I have been saying for years that we have a lack of real projects /
> tasks / ideas for people who are skilled, yet have no idea what to do.
> I know of well over a hundred people I have email addresses of that have
> asked me for these types of things, and have patches in the kernel that
> are non-trivial to prove that they have the skill to do real things.
> 
> It's a real problem, and one that I don't have an answer for.  We need
> these types of tasks, and I don't have them, and every maintainer I ask
> about it also doesn't have them.  What we are asking for is people to
> somehow come up with tasks on their own, as if they know what needs to
> be done.

How about maintaining a list of people who are in this state?  It
could be as simple as a list of git commit ID's --- their e-mail
addresses will be in the git commit.  Republish it once a month, with
some kind of auto-expiry mechanism unless (a) the author of said git
commit requests that it be removed, or (b) after some time period
unless the author is continuing to submit at least some non-trivial
patch.

At least that way maintainers who are looking for some minions will at
least have a starting point.

As far as creating tasks for people who _are_ skilled, the question is
how picky are these people, and how much time does it take to delegate
work to them?  If it takes 8 hours of effort to delegate 16 hours (a
weekend's worth) of effort, with a 50% probability that the person
won't flaky out, and another 4 hours of work afterwards cleaning up
and debugging the patch.  It has negative overall value to the
maintainer.

Or if the work isn't directly kernel programming, but instead work in
support of kernel programming (i.e., improving documentation, fixing
up regression tests, auditing interfaces looking for security holes),
is this work that "graduates" from the "my first patch" programs
willing to do?

There are almost certainly "bottle-washing" duty style tasks
available, but the question is are they tasks that people will want to
do?  And how much work is a "week-end task" really?  If it takes two
hours to delegate a task the maintainer can do in an hour, but it
takes the newcomer a weekend, then it only makes sense for the
maintainer to do this if (a) they are doing it as a cherity, or (b)
there is a sufficiently high probability the newcomer to stick around.

> Remember, when we started, the number of things that needed to be done
> was hugely obvious, everywhere we turned needed lots of work.  For
> people new to the kernel, this isn't the case anymore.  Linux has taken
> over the world because it is so capable and feature rich.  Picking up a
> week-end task is almost impossible because of this.  I know, I have no
> week-end-length tasks on my TODO list.

I wonder if we're looking at this problem in the wrong way.  If Linux
is so feature rich, why are we stressing out about our "recruitment
problem"?  Presumably, because there are lots of companies who are
complaining that they can't find enough Linux kernel programmers.
What do many of these companies need the Linux kernel programmers to
do?  I'm going to go out on a limb and say, "hardware enablement".
(That is, the companies are not looking for new subsystem maintainers,
they are looking for device driver cannon fodder.)

So maybe the answer is that we need to figure out some kind of
internship program so people can work for more than just a weekend,
and create a device driver for said company.  And maybe our payment in
kind is that companies who ship product kernels (not just for
development boards, but real SDK's for real commercial products) get
priority --- and the driver has to be developed in an upstream first
basis, or the intern doesn't get a community mentor.

Yes, it will still be a negative ROI for the community mentor, unless
you count the hope that maybe we won't be still be shipping brand-new
products using 3.10 kernel in another year's time.  :-)

						- Ted

Re: First kernel patch (optimization)

[Alexander Holler]

Am 19.09.2015 um 07:18 schrieb Greg KH:

> I have been saying for years that we have a lack of real projects /
> tasks / ideas for people who are skilled, yet have no idea what to do.
> I know of well over a hundred people I have email addresses of that have
> asked me for these types of things, and have patches in the kernel that
> are non-trivial to prove that they have the skill to do real things.
>
> It's a real problem, and one that I don't have an answer for.  We need
> these types of tasks, and I don't have them, and every maintainer I ask
> about it also doesn't have them.  What we are asking for is people to
> somehow come up with tasks on their own, as if they know what needs to
> be done.

I've recently posted a proof of concept for wiping files, or in other 
words to really delete files, And it was a disaster because if someone 
posts imperfect pathhes on this list, people have fun trying to eat you 
(because they seem bored or whatever).

Even posting perfect patches is a game, because there exists always a 
space, newline or variable name which might be used to start annoying 
discussions.

So, there is no reason to wonder about the lack of such tasks.

So even if you don't agree, here as task: wipe files in real. ;)

Regards,

Alexander Holler

Re: First kernel patch (optimization)

[Alexander Holler]

Am 19.09.2015 um 14:52 schrieb Alexander Holler:
> Am 19.09.2015 um 07:18 schrieb Greg KH:
>
>> I have been saying for years that we have a lack of real projects /
>> tasks / ideas for people who are skilled, yet have no idea what to do.
>> I know of well over a hundred people I have email addresses of that have
>> asked me for these types of things, and have patches in the kernel that
>> are non-trivial to prove that they have the skill to do real things.
>>
>> It's a real problem, and one that I don't have an answer for.  We need
>> these types of tasks, and I don't have them, and every maintainer I ask
>> about it also doesn't have them.  What we are asking for is people to
>> somehow come up with tasks on their own, as if they know what needs to
>> be done.
>
> I've recently posted a proof of concept for wiping files, or in other
> words to really delete files, And it was a disaster because if someone
> posts imperfect pathhes on this list, people have fun trying to eat you
> (because they seem bored or whatever).
>
> Even posting perfect patches is a game, because there exists always a
> space, newline or variable name which might be used to start annoying
> discussions.
>
> So, there is no reason to wonder about the lack of such tasks.
>
> So even if you don't agree, here as task: wipe files in real. ;)

By the way, in that discussion (about wiping files) I was blamed for 
writing bugs in bugzilla without offering a patch, for something they 
believe it is a feature (whereas I still believe it is a bug to not 
really delete files). That leaded me to the question if there is 
somewhere a feature request tracker, for which I've got, of course, no 
answer.

So, if there is a lack of real projects / tasks / ideas, maybe it might 
make sense to setup such a feature request tracker (or idea pool), maybe 
with the possibility to let people up/down vote ideas.

Regards,

Alexander Holler

Re: First kernel patch (optimization)

[Theodore Ts'o]

On Sat, Sep 19, 2015 at 02:52:06PM +0200, Alexander Holler wrote:
> 
> I've recently posted a proof of concept for wiping files, or in other words
> to really delete files, And it was a disaster because if someone posts
> imperfect pathhes on this list, people have fun trying to eat you (because
> they seem bored or whatever).

People gave you feedback on how what would be necessary to make the
patch acceptable, and you rejected the advice complaining that it
would take you months of "unpaid time".  You were then complaining
that the people who gave you feedback wouldn't fix the patch for you,
and that you threatened that if we didn't integrate your racy patch
into the core VFS, you would go switch to FreeBSD.

> Even posting perfect patches is a game, because there exists always a space,
> newline or variable name which might be used to start annoying discussions.

Trust me, the issues with your patch went *way* beyond extra spaces or
newlines.

> So, there is no reason to wonder about the lack of such tasks.

Greg was specifically looking for patches that could be done in a
weekend.

If you want to raise the argument that we should lower the standards
for accepting patches so that more patches can accomplished within a
weekend's worth of work, we can have that discussion --- but I'm not
sure it will have the end result that you are hoping for.

Best regards,

					- Ted

Re: First kernel patch (optimization)

[Alexander Holler]

Am 19.09.2015 um 16:22 schrieb Theodore Ts'o:
> On Sat, Sep 19, 2015 at 02:52:06PM +0200, Alexander Holler wrote:
>>
>> I've recently posted a proof of concept for wiping files, or in other words
>> to really delete files, And it was a disaster because if someone posts
>> imperfect pathhes on this list, people have fun trying to eat you (because
>> they seem bored or whatever).
>
> People gave you feedback on how what would be necessary to make the
> patch acceptable, and you rejected the advice complaining that it
> would take you months of "unpaid time".  You were then complaining
> that the people who gave you feedback wouldn't fix the patch for you,
> and that you threatened that if we didn't integrate your racy patch
> into the core VFS, you would go switch to FreeBSD.

Sorry, but I can happily live without feedback like "charming".

I've complained about the requirement to post perfect patches without 
even the promise that they will ever have a chance to end up in the 
kernel. But where should someone get such an OK for an idea without the 
possibility to post preliminary patches without earning insulting comments?

Anyway, I've accepted that I'm the error here which is why I almost 
don't post any patches here anymore.

>> Even posting perfect patches is a game, because there exists always a space,
>> newline or variable name which might be used to start annoying discussions.
>
> Trust me, the issues with your patch went *way* beyond extra spaces or
> newlines.

It was (and is, imho) a working proof of concept. I've posted the patch 
to describe the idea, and NOT as a perfect patch ready for inclusion. 
I'm fully aware that there are million reasons why some parts of a file 
might not be deleted, but that is a matter how you describe the 
functionality. And the concept works imho for many use cases. At least 
much, much, much better than just nothing.

But obviously, one of biggest failures one can do, is to post imperfect 
patches on this list. Regardless how you describe or mark them (mine had 
a RFC and WIP in front of), people will use every small detail against 
you, in order to have something to criticise or to make some fun of.

>> So, there is no reason to wonder about the lack of such tasks.
>
> Greg was specifically looking for patches that could be done in a
> weekend.

Sorry, I've missed that detail. So maybe create a tracker for such 
weekend-tasks.

> If you want to raise the argument that we should lower the standards
> for accepting patches so that more patches can accomplished within a
> weekend's worth of work, we can have that discussion --- but I'm not
> sure it will have the end result that you are hoping for.

No. I don't want to lower the standards. Maybe in regard to silly style 
stuff, but not in regard to code quality (and I mean real bugs like 
races, deadlocks or such, and not if a line has more than 80 
characters). I would have liked some comments like "good or bad idea" 
but this list is imho the wrong place to search for such useful 
comments. I haven't searched for comments on the code, as I was FULLY 
aware that the code is ugly and NOT ready for inclusion),

Alexander Holler

Re: First kernel patch (optimization)

[Theodore Ts'o]

On Sat, Sep 19, 2015 at 07:47:22PM +0200, Alexander Holler wrote:
> No. I don't want to lower the standards. Maybe in regard to silly style
> stuff, but not in regard to code quality (and I mean real bugs like races,
> deadlocks or such, and not if a line has more than 80 characters). I would
> have liked some comments like "good or bad idea" but this list is imho the
> wrong place to search for such useful comments. I haven't searched for
> comments on the code, as I was FULLY aware that the code is ugly and NOT
> ready for inclusion),

That was asked and answered on the original thread, but perhaps it got
lost amongst some of the noise --- some of which was contributed by
yourself, but be that as it may.

The *feature* is in and of itself not an insane idea.  "Secure delete"
is something that Linux has had before (in fact I did the original
implementation for ext2 a decade+ ago), and indeed the interface,
using the FS_SECRM_FL flag, is still present in Linux even if it is
not supported by any of the file systems in the tree at the moment.
It got ripped out because doing it right in the face of ongoing
interface changes and performance improvements (going back to the
pre-2.0, and possibly the pre-1.2 days), it was simpler to rip it out
rather than to reimplement it.

I wasn't responsible for ripping it out, back in mists of pre-history,
but I didn't care enough to reimplement it --- or complain about it,
either.  And no one else cared enough to complain to Linus about this
being a user-visible change that broke them.  So it's a great example
of how a feature and functionality that no one cares about *can* get
ripped out of the kernel.

Perhaps not so surprisingly, over a decade later, it is not currently
at the top of the priority list of any of the current file system or
VFS developers, as far as I know.  One of the reasons for that is that
there are a number of other ways of achieving the same functionality.
These include using tmpfs, or using file system level encryption.
They require a bit more system administrator setup than just being
able to set the FS_SECR_FL flag, true, but just because it's more
convenient doesn't mean that it's worth doing.

So.... this is a feature request.  It's a reasonable feature request,
in that if someone would like to pay $$$ for some consultant to
implement it in a way that is bug-free, I suspect it could go
upstream.  Someone who was very motivated and with the sufficient
skills could also invest their own effort to make a patch that can go
upstream too.  You've elected not, to because you believe it would
take you months of "unpaid time".  That's purely within your rights to
do.  But you don't have the right to try to tell other people what
work to do on their behalf --- not unless you are paying their salary.


And of course, if you want to maintain an out-of-tree patch, there's
nothing wrong with that.  I've implemented code that has never gone
upstream because the weight of the other file system developers were
afraid that Enterise Linux customers would use the feature
incorrectly, and it would cause potentially cause a security failure
if used inappropriately, which would be a PR and support nightmare for
them.

Seeing that the weight of the other file system developers are against
the patch, it's never gone into the mainline Linux kernel, even though
I could have forced the feature into ext4.  However, this patch is in
active use in practically every single data center kernel for Google,
and it's in use in at least one other very large publically traded
company that uses cluster file systems such as Hadoopfs.  And if
someone wants a copy of the FALLOC_FL_NO_HIDE_STALE patch for ext4,
I'm happy to give it to them.  But it hasn't gone upstream, and I'm OK
with that.

As far as what you want to do next, you have a personal "proof of
concept" patch that seems to work well enough for you.  Great!  I'm
sure you can keep using it for your own purposes.  If you can convince
someone with the skills to get the patch to an upstreamable state, it
is my judgement that this is doable, so this puts your feature in a
much better state than the FALLOC_FL_NO_HIDE_STALE flag.  However,
there is still a non-trivial amount of work left to do to turn your
"proof of concept" patch into something that is upstremable, including
changing the interface to using the FS_SECRM_FL flag.  And your
whining that other people should change *their* priorities to match
*yours* is not likely to help.

						- Ted

Re: First kernel patch (optimization)

[Alexander Holler]

Am 20.09.2015 um 04:21 schrieb Theodore Ts'o:
> On Sat, Sep 19, 2015 at 07:47:22PM +0200, Alexander Holler wrote:

> Perhaps not so surprisingly, over a decade later, it is not currently
> at the top of the priority list of any of the current file system or
> VFS developers, as far as I know.  One of the reasons for that is that
> there are a number of other ways of achieving the same functionality.
> These include using tmpfs, or using file system level encryption.
> They require a bit more system administrator setup than just being
> able to set the FS_SECR_FL flag, true, but just because it's more
> convenient doesn't mean that it's worth doing.

Again, I don't think that encryption is an alternative. Besides that 
there is always the thread that strong encrytion will become regulated, 
there is also the very real thread that someone might end up in jail 
when using encryption and throwing away the key to delete stuff. E.g., 
as to my knowledge, in the UK you might end up in jail if you don't hand 
out a password. So what happens if you've deleted the key and are really 
unable to hand it out and the people which have an interest in what 
you've once stored don't believe you?

> So.... this is a feature request.  It's a reasonable feature request,
> in that if someone would like to pay $$$ for some consultant to
> implement it in a way that is bug-free, I suspect it could go
> upstream.  Someone who was very motivated and with the sufficient
> skills could also invest their own effort to make a patch that can go
> upstream too.  You've elected not, to because you believe it would
> take you months of "unpaid time".  That's purely within your rights to
> do.  But you don't have the right to try to tell other people what
> work to do on their behalf --- not unless you are paying their salary.

First I haven't request that someone implements it for me. Besides that 
what you're describing is what maintainers do all the time. Of course, 
it's their job to request quality, but, in my humble opinion, very often 
they are requesting stuff just to request something.

And that "month of unpaid time" was for sure a cynical exaggeration I've 
done while having been angry. In fact I believe the way I've outlined 
with the ugly code (proof of concept) could be implemented by someone 
like you in a weekend. For me it needs quiet some more time because I 
had and still have almost zero knowledge about all locks and whatever 
else is used in the filesystem code. But nevertheless I was able to fix 
up a lot of stuff during another afternoon. E.g. I've added checks if a 
file is in use or if AT_WIPE was called on a directory and then returned 
errors in those cases. Unfortunately the code changed in 4.2 and that 
patch doesn't apply anymore and now, because I don't really need those 
implementation details (I'm aware of the problems of my patch), I've 
thrown the patch into the waste bin. Besides that my concept doesn't 
work on BTRFS what I'm currently using for various reasons (mainly 
compression) on most of my systems. And I have no idea if it ever will 
(because I don't know why discard on BTRFS doesn't really discard what I 
think it should discard. ;) ).


> Seeing that the weight of the other file system developers are against
> the patch, it's never gone into the mainline Linux kernel, even though
> I could have forced the feature into ext4.  However, this patch is in
> active use in practically every single data center kernel for Google,
> and it's in use in at least one other very large publically traded
> company that uses cluster file systems such as Hadoopfs.  And if
> someone wants a copy of the FALLOC_FL_NO_HIDE_STALE patch for ext4,
> I'm happy to give it to them.  But it hasn't gone upstream, and I'm OK
> with that.

Sure, but please don't forget its quiet some difference if someone does 
stuff without being paid and all he earns are unfriendly comments. In 
fact I still don't care much about if any code from me ends up in 
mainline, but I dislike quiet a lot the tone used by many maintainers to 
refuse things someone offered in a good believe.

E.g. recently I've read that a maintainer requested that patch posters 
should be aware of his calendar (like conferences he visits, merge 
windows he has to care for and similar stuff. ?!?

> As far as what you want to do next, you have a personal "proof of
> concept" patch that seems to work well enough for you.  Great!  I'm
> sure you can keep using it for your own purposes.  If you can convince
> someone with the skills to get the patch to an upstreamable state, it
> is my judgement that this is doable, so this puts your feature in a
> much better state than the FALLOC_FL_NO_HIDE_STALE flag.  However,
> there is still a non-trivial amount of work left to do to turn your
> "proof of concept" patch into something that is upstremable, including
> changing the interface to using the FS_SECRM_FL flag.  And your
> whining that other people should change *their* priorities to match
> *yours* is not likely to help.

Besides that I have absolutely no knowledge about 
FALLOC_FL_NO_HIDE_STALE or the FS_SECRM_FL flag, I've never whined. I've 
complained about the tone very often used on this list. And it doesn't 
help if you are suggesting I'm whining. It just proves my assumption, at 
least in my eyes.

Thanks for the discussion.

Alexander Holler

Re: First kernel patch (optimization)

[Austin S Hemmelgarn]

[-- Attachment #1: Type: text/plain, Size: 5246 bytes --]

On 2015-09-20 06:41, Alexander Holler wrote:
> Am 20.09.2015 um 04:21 schrieb Theodore Ts'o:
>> On Sat, Sep 19, 2015 at 07:47:22PM +0200, Alexander Holler wrote:
>
>> Perhaps not so surprisingly, over a decade later, it is not currently
>> at the top of the priority list of any of the current file system or
>> VFS developers, as far as I know.  One of the reasons for that is that
>> there are a number of other ways of achieving the same functionality.
>> These include using tmpfs, or using file system level encryption.
>> They require a bit more system administrator setup than just being
>> able to set the FS_SECR_FL flag, true, but just because it's more
>> convenient doesn't mean that it's worth doing.
>
> Again, I don't think that encryption is an alternative. Besides that
> there is always the thread that strong encrytion will become regulated,
> there is also the very real thread that someone might end up in jail
> when using encryption and throwing away the key to delete stuff. E.g.,
> as to my knowledge, in the UK you might end up in jail if you don't hand
> out a password. So what happens if you've deleted the key and are really
> unable to hand it out and the people which have an interest in what
> you've once stored don't believe you?
First off, this is why I will never live in the UK.  Secondly, this is 
why if it's something that you don't want to store for a long time (that 
is, longer than the system will be powered on), you use an ephemeral 
encryption key.  Thirdly, if there is some chance you can lose the key, 
you make a secure backup (or do like I do and algorithmically generate 
passwords/encryption keys in a reproducible manner (which is itself 
secure if you do it right, and of course tell no-one the algorithm you 
use to generate them)).
>> So.... this is a feature request.  It's a reasonable feature request,
>> in that if someone would like to pay $$$ for some consultant to
>> implement it in a way that is bug-free, I suspect it could go
>> upstream.  Someone who was very motivated and with the sufficient
>> skills could also invest their own effort to make a patch that can go
>> upstream too.  You've elected not, to because you believe it would
>> take you months of "unpaid time".  That's purely within your rights to
>> do.  But you don't have the right to try to tell other people what
>> work to do on their behalf --- not unless you are paying their salary.
>
> First I haven't request that someone implements it for me. Besides that
> what you're describing is what maintainers do all the time. Of course,
> it's their job to request quality, but, in my humble opinion, very often
> they are requesting stuff just to request something.
The problem I see with this argument is:
1. There's a lot of code in the kernel that wouldn't be merged today in 
the state it's in, this creates a false sense of what quality is 
expected for new code (BTRFS in particular comes to mind here).
2. If the code can be proven to be racy, you fix it, period.  Adding 
known racy code to the kernel should never happen.  The same goes for 
unsafe usage of locking, RCU, or any subsystem related macros/functions. 
  This should probably be better spelled out in SubmittingPatches.
3. Subsystem maintainers became maintainers because they have a very 
high degree of knowledge relating to that subsystem, and usually about 
general kernel programming as well.  If a maintainer is asking you to 
fix something in your patch, I'd be more than willing to bet that they 
are right in asking you to fix it.
> And that "month of unpaid time" was for sure a cynical exaggeration I've
> done while having been angry. In fact I believe the way I've outlined
> with the ugly code (proof of concept) could be implemented by someone
> like you in a weekend. For me it needs quiet some more time because I
> had and still have almost zero knowledge about all locks and whatever
> else is used in the filesystem code. But nevertheless I was able to fix
> up a lot of stuff during another afternoon. E.g. I've added checks if a
> file is in use or if AT_WIPE was called on a directory and then returned
> errors in those cases. Unfortunately the code changed in 4.2 and that
> patch doesn't apply anymore and now, because I don't really need those
> implementation details (I'm aware of the problems of my patch), I've
> thrown the patch into the waste bin. Besides that my concept doesn't
> work on BTRFS what I'm currently using for various reasons (mainly
> compression) on most of my systems. And I have no idea if it ever will
> (because I don't know why discard on BTRFS doesn't really discard what I
> think it should discard. ;) ).
Discard has never really worked properly in BTRFS, although it is being 
worked on.  If you actually care about security though, you shouldn't be 
using discard except when re-provisioning your storage (there are 
numerous papers about why on the web), trying to use that for secure 
deletion is creating a false sense of security.

If you're using in-line compression, then that at least means that it 
will take somewhat more effort to get a file off of the disk that has 
been deleted.


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 3019 bytes --]

Re: First kernel patch (optimization)

[Alexander Holler]

Am 21.09.2015 um 17:47 schrieb Austin S Hemmelgarn:

> The problem I see with this argument is:
> 1. There's a lot of code in the kernel that wouldn't be merged today in
> the state it's in, this creates a false sense of what quality is
> expected for new code (BTRFS in particular comes to mind here).

Just to say it a last time, THE CODE I'VE POSTED WAS NEVER MEANT FOR 
MERGING in that state. Regardless how many people will still come by and 
repeat that it was ugly, bad and broken to just use that as an 
additional argument against me.

I've absolutely no idea how you all start to test an idea and 
demonstrate it others, but I don't waste time on such tasks with looking 
for style, (premature) optimization or even races.

I know how write production ready code, doing such since a long time and 
I know how much time that costs, and I know that only fools (or students 
which have to show that they can write good code) spend this time for 
code which might end up in the waste bin anyway.

Alexander Holler

Re: First kernel patch (optimization)

[Alexander Holler]

Am 21.09.2015 um 17:47 schrieb Austin S Hemmelgarn:
> On 2015-09-20 06:41, Alexander Holler wrote:
>> Am 20.09.2015 um 04:21 schrieb Theodore Ts'o:
>>> On Sat, Sep 19, 2015 at 07:47:22PM +0200, Alexander Holler wrote:

>>
>> Again, I don't think that encryption is an alternative. Besides that
>> there is always the thread that strong encrytion will become regulated,
>> there is also the very real thread that someone might end up in jail
>> when using encryption and throwing away the key to delete stuff. E.g.,
>> as to my knowledge, in the UK you might end up in jail if you don't hand
>> out a password. So what happens if you've deleted the key and are really
>> unable to hand it out and the people which have an interest in what
>> you've once stored don't believe you?
> First off, this is why I will never live in the UK.  Secondly, this is

(First, it should, of course, read threat, not thread, my English 
becomes worse when I'm getting angry, besides that I was working on a 
problem with threads just before.)

Just in case of, I have not used the UK as an example because I might 
hate it or similar (nothing of that is the case). I've used the UK as an 
example to make it clear that such can happen in every country (besides 
that I know that some kernel maintainers live there).. And e.g. the US 
has had a time with regulated encryption (and I think there recently was 
another attempt to regulate it again). And besides states, there might 
be other people which might getting some unwanted ideas if they might 
believe that there is something of value for them encrypted somewhere by 
you. So, in my humble opinion, it's always better to get rid of 
something clearly. That also helps against the problem that the 
encryption used today, might be worthless tomorrow.

Regards,

Alexander Holler

Re: First kernel patch (optimization)

[Linus Torvalds]

On Tue, Sep 15, 2015 at 12:53 PM, Eric Curtin <ericcurtin17@gmail.com> wrote:
> My first kernel patch, hope I did everything correctly! Instead of calling strlen on every iteration of the for loop, just call it once instead and store in a variable.

Heh. Ok, that resulted in a rather long email thread.

Anyway, I'd actually prefer to merge this patch, for two reasons:

 - the "termination calculation is expensive" problem is a real
problem, and while in this case the compiler may be able to notice
that the "strlen()" is constant over the loop and can be hoisted up,
that is not at all necessarily the case most of the time.

    So I actually think patches like this are good things. Not because
this particular code site necessarily matters, but because people who
write code with things like "strlen()" in the terminating condition
need to learn that it's *wrong*.

 - I'd much rather see this kind of trivial patch than the usual
trivial patch that is clearly just "let's run some script on the
kernel and fix up warnings it generates mindlessly".

   In contrast to that, *this* trivial patch was about somebody who
thought about code generation and efficiency. And *that* is the kind
of trivial patches we want to encourage, not necessariyl because this
particular case was so important, but because that's the kind of
people and thinking we want to encourage.

So quite frankly, I'd just take this directly, but I'd like more of
real changelog.

But I wonder if Eric is even reading the emails any more ;)

                 Linus

Re: First kernel patch (optimization)

[Eric Curtin]

On 22 September 2015 at 18:38, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Tue, Sep 15, 2015 at 12:53 PM, Eric Curtin <ericcurtin17@gmail.com> wrote:
>> My first kernel patch, hope I did everything correctly! Instead of calling strlen on every iteration of the for loop, just call it once instead and store in a variable.
>
> Heh. Ok, that resulted in a rather long email thread.
>
> Anyway, I'd actually prefer to merge this patch, for two reasons:
>
>  - the "termination calculation is expensive" problem is a real
> problem, and while in this case the compiler may be able to notice
> that the "strlen()" is constant over the loop and can be hoisted up,
> that is not at all necessarily the case most of the time.
>
>     So I actually think patches like this are good things. Not because
> this particular code site necessarily matters, but because people who
> write code with things like "strlen()" in the terminating condition
> need to learn that it's *wrong*.
>
>  - I'd much rather see this kind of trivial patch than the usual
> trivial patch that is clearly just "let's run some script on the
> kernel and fix up warnings it generates mindlessly".
>
>    In contrast to that, *this* trivial patch was about somebody who
> thought about code generation and efficiency. And *that* is the kind
> of trivial patches we want to encourage, not necessariyl because this
> particular case was so important, but because that's the kind of
> people and thinking we want to encourage.
>
> So quite frankly, I'd just take this directly, but I'd like more of
> real changelog.
>
> But I wonder if Eric is even reading the emails any more ;)
>
>                  Linus

Hi Linus & Everyone else,

I can deal with the patronizing remarks made at the start of the
thread, I've been called worse names! But I would encourage people to
avoid this behavior especially with newbies, as many may leave the
community and never attempt to submit a patch again. It does not leave
a great first impression.

We so not come out the womb expert kernel developers, you gotta learn
how to crawl, before you can walk.

Yeah, I'm still reading this email thread and learned lots from it.
I'm working on something more meaningful, but it's not going to be
ground breaking of course, there is a led on my capslock key on a new
machine I won at work that does not switch off properly after it is
switched on. I think it is something to do with the LED_CAPSL variable
in here:

drivers/hid/usbhid/usbkbd.c

I'll figure it out, and get it fixed. Decided, I'd do something more
meaty. Then I'll start looking at the drivers/staging stuff. I can
only look at this stuff for a few hours every second day, with my
normal 9 to 5 job being first priority.

I can clean up this patch re-submit no problem, in fact I already did
and got more feedback, it's subject is "tools: usbip: detach: avoid
calling strlen() at each iteration". But I moved on to the keyboard
driver because of the feedback I received about wasting peoples time!

Regards,

Eric

Re: First kernel patch (optimization)

[Alexander Holler]

Am 20.09.2015 um 12:41 schrieb Alexander Holler:
> Am 20.09.2015 um 04:21 schrieb Theodore Ts'o:

>> As far as what you want to do next, you have a personal "proof of
>> concept" patch that seems to work well enough for you.  Great!  I'm
>> sure you can keep using it for your own purposes.  If you can convince
>> someone with the skills to get the patch to an upstreamable state, it
>> is my judgement that this is doable, so this puts your feature in a
>> much better state than the FALLOC_FL_NO_HIDE_STALE flag.  However,
>> there is still a non-trivial amount of work left to do to turn your
>> "proof of concept" patch into something that is upstremable, including
>> changing the interface to using the FS_SECRM_FL flag.  And your
>> whining that other people should change *their* priorities to match
>> *yours* is not likely to help.
>
> Besides that I have absolutely no knowledge about
> FALLOC_FL_NO_HIDE_STALE or the FS_SECRM_FL flag, I've never whined. I've
> complained about the tone very often used on this list. And it doesn't

Just to explain why I'm still quiet happy with my very simple approach 
to use a simple modified discard mechanism to wipe files:

My main use case (an that of several other people I know) is to have a 
simple way to wipe photos on sd-cards or to wipe other files I've copied 
once onto (vfat-formatted) usb-sticks while never having modified these 
files afterwards.

And that works like a charm and doesn't need complicated patches which 
might be very different for every filesystem.

And the check (and returning an error) if a file is already in use when 
trying to wipe it, makes it unnecessary to introduce something more 
complicated to schedule wiping. I also don't care if something is left 
in swap or similiar. So instead of trying to perfectly solve a big 
problem (which already was unsuccessful tried in case of the Linux 
kernel), I've just reduced the problem to fit the main use cases most 
people have and solved that with a very simple, but working approach.

Alexander Holler

Re: First kernel patch (optimization)

[Dmitry Torokhov]

Hi Erik,

>
> Yeah, I'm still reading this email thread and learned lots from it.
> I'm working on something more meaningful, but it's not going to be
> ground breaking of course, there is a led on my capslock key on a new
> machine I won at work that does not switch off properly after it is
> switched on.

Is it Debian-derivative by any chance? Their capslock setup is wonky
because CapsLock key does no actually set up as a CapsLock but another
modifier. Also is it in X or is it on text console? Because X handles
led state on its own...

> I think it is something to do with the LED_CAPSL variable
> in here:
>
> drivers/hid/usbhid/usbkbd.c

I do not think you are using usbkbd driver - it is for keyboards in
"boot protocol" and barely anyone users them in such mode. You need to
look into drivers/hid/hid-input.c.

Thanks.

-- 
Dmitry

Re: First kernel patch (optimization)

[Eric Curtin]

Hi Dimitry,

> Is it Debian-derivative by any chance? Their capslock setup is wonky
> because CapsLock key does no actually set up as a CapsLock but another
> modifier. Also is it in X or is it on text console? Because X handles
> led state on its own...

I'm on Fedora 22. Yeah, you're correct X is handling this, the led does
not turn on at all in text console. Tried Wayland also for the first time
to see if it occurred there also, it does. Does this mean I should not be
looking at kernel code all to fix this issue and I should be looking at
X/Wayland?

>> I think it is something to do with the LED_CAPSL variable
>> in here:
>>
>> drivers/hid/usbhid/usbkbd.c
>
> I do not think you are using usbkbd driver - it is for keyboards in
> "boot protocol" and barely anyone users them in such mode. You need to
> look into drivers/hid/hid-input.c.

Yeah, I realize this now (see "Problems with printk logs and my driver"
email thread), they are talking about renaming this file now as I am not
the first one to make this mistake.

Re: First kernel patch (optimization)

[Thiago Farina]

On Sat, Sep 19, 2015 at 2:18 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Fri, Sep 18, 2015 at 10:26:24PM -0400, Theodore Ts'o wrote:
>> On Fri, Sep 18, 2015 at 12:42:48AM -0700, Greg KH wrote:
>> > So don't take cleanup patches for your code, that's fine, and I totally
>> > understand why _you_ don't want to do that.  But to blow off the effort
>> > as being somehow trivial and not worthy of us, that's totally missing
>> > the point, and making things harder on yourself in the end.
>>
>> It's not that I think cleanup patches are "trivial and not worthy of
>> us", although I'll note that cleanup patches can end up causing real
>> bug fixes (including possibly fixes that address security problems) to
>> not apply to stable kernels, which means someone needs to deal with
>> failed patches --- or what is much more likely, the failed patch gets
>> bounced back to the overworked maintainer who then drops the patch
>> because he or she doesn't have the bandwidth to manually backport the
>> patch to N different stable trees, and then we all suffer.  So cleanup
>> patches *do* have a cost.
>
> All patches have a "cost", and that cost is something that you have to
> deal with as a maintainer.  Nothing new here at all, and if you don't
> like cleanup patches, great, don't take them.  But never go around
> saying that people should NOT send cleanup patches to subsystems you
> don't maintain like you did.  That's rude and unhelpful to everyone
> involved.
>
> The best way to ensure that you never get whitespace patches, is to
> clean your subsystem up.  And frankly, it needs a lot of work, have you
> run checkpatch on it in a while?  You could resolve this issue for
> yourself with a simple afternoon's worth of work.  Why you don't do so
> is a mystery to me.
I think what Theodore was just trying to say is that there are much more
important and pressuring things to do in the kernel (that helps our society)
then doing cleanup patches (what you will tell to your interviewer? That you
fixed the spelling of a word or removed the whitespaces of a source file?).
I think he is just pushing for a more substancial work, that fixes a crash,
improves performance, fixes a subtle bug in some important driver, fixes a
memory leak, etc.

And many maintainers of open source projects that I know off, are not
very welcome
to cleanup patches, especially when the project is mature. They just
call it churn and
turn it down.

-- 
Thiago Farina

Re: First kernel patch (optimization)

[Greg KH]

On Mon, Sep 28, 2015 at 03:54:20AM -0300, Thiago Farina wrote:
> And many maintainers of open source projects that I know off, are not
> very welcome to cleanup patches, especially when the project is
> mature. They just call it churn and turn it down.

Then those are not very mature developers as code cruft always needs to
be culled and cleaned on an active project, otherwise it gets harder to
work with over time, as others have already pointed out on this thread.

thanks,

greg k-h

Re: First kernel patch (optimization)

[Austin S Hemmelgarn]

[-- Attachment #1: Type: text/plain, Size: 803 bytes --]

On 2015-09-26 09:28, Eric Curtin wrote:
> Hi Dimitry,
>
>> Is it Debian-derivative by any chance? Their capslock setup is wonky
>> because CapsLock key does no actually set up as a CapsLock but another
>> modifier. Also is it in X or is it on text console? Because X handles
>> led state on its own...
>
> I'm on Fedora 22. Yeah, you're correct X is handling this, the led does
> not turn on at all in text console. Tried Wayland also for the first time
> to see if it occurred there also, it does. Does this mean I should not be
> looking at kernel code all to fix this issue and I should be looking at
> X/Wayland?
If the led doesn't turn on on the console either, then it may be worth 
looking at kernel code, but it may be in the console driver instead of 
the input layer driver.


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 3019 bytes --]

Re: First kernel patch (optimization)

[Eric Curtin]

On 29 September 2015 at 14:51, Austin S Hemmelgarn <ahferroin7@gmail.com> wrote:
> On 2015-09-26 09:28, Eric Curtin wrote:
>>
>> Hi Dimitry,
>>
>>> Is it Debian-derivative by any chance? Their capslock setup is wonky
>>> because CapsLock key does no actually set up as a CapsLock but another
>>> modifier. Also is it in X or is it on text console? Because X handles
>>> led state on its own...
>>
>>
>> I'm on Fedora 22. Yeah, you're correct X is handling this, the led does
>> not turn on at all in text console. Tried Wayland also for the first time
>> to see if it occurred there also, it does. Does this mean I should not be
>> looking at kernel code all to fix this issue and I should be looking at
>> X/Wayland?
>
> If the led doesn't turn on on the console either, then it may be worth
> looking at kernel code, but it may be in the console driver instead of the
> input layer driver.
>

I may have been incomplete earlier. The led does not turn on at all in
console. In X the led does turn on, but does not switch off correctly.
I'm looking at kernel code, still assuming I can fix it in here.

Re: First kernel patch (optimization)

[Alexander Duyck]

On 09/15/2015 12:52 PM, Eric Curtin wrote:
> My first kernel patch, hope I did everything correctly! Instead of calling strlen on every iteration of the for loop, just call it once instead and store in a variable.
>
> Signed-off-by: Eric Curtin <ericcurtin17@gmail.com>
>
> diff --git a/tools/usb/usbip/src/usbip_detach.c b/tools/usb/usbip/src/usbip_detach.c
> index 05c6d15..9db9d21 100644
> --- a/tools/usb/usbip/src/usbip_detach.c
> +++ b/tools/usb/usbip/src/usbip_detach.c
> @@ -47,7 +47,9 @@ static int detach_port(char *port)
>   	uint8_t portnum;
>   	char path[PATH_MAX+1];
>
> -	for (unsigned int i = 0; i < strlen(port); i++)
> +	unsigned int port_len = strlen(port);
> +
> +	for (unsigned int i = 0; i < port_len; i++)
>   		if (!isdigit(port[i])) {
>   			err("invalid port %s", port);
>   			return -1;
>

You should probably run this through scripts/checkpatch.pl as I don't 
think declaring i inside the loop is consistent with the kernel coding 
standard.  Also you don't want the space between port_len and the 
declaration of path.

Also you might want to consider just running the loop from port_len down 
to 0 doing something like:
	while (port_len) {
		if (!isdigit(port[--port_len])) {
			err("invalid port %s", port);		
			return -1;

The advantage to running the loop backwards is that you have to carry 
one less variable which means one less register to mess with in the 
final compiled code.

- Alex

First kernel patch (optimization)

[Eric Curtin]

My first kernel patch, hope I did everything correctly! Instead of calling strlen on every iteration of the for loop, just call it once instead and store in a variable.

Signed-off-by: Eric Curtin <ericcurtin17@gmail.com>

diff --git a/tools/usb/usbip/src/usbip_detach.c b/tools/usb/usbip/src/usbip_detach.c
index 05c6d15..9db9d21 100644
--- a/tools/usb/usbip/src/usbip_detach.c
+++ b/tools/usb/usbip/src/usbip_detach.c
@@ -47,7 +47,9 @@ static int detach_port(char *port)
 	uint8_t portnum;
 	char path[PATH_MAX+1];
 
-	for (unsigned int i = 0; i < strlen(port); i++)
+	unsigned int port_len = strlen(port);
+
+	for (unsigned int i = 0; i < port_len; i++)
 		if (!isdigit(port[i])) {
 			err("invalid port %s", port);
 			return -1;