Message-ID: <CADvZEfVNs9Z1=Re5rNgQ2CyQcuMinH=psbdONWxaeJqdUVZFDA@mail.gmail.com>
Date:	Thu, 18 Jun 2015 22:11:28 -0500
From:	Orestes Leal Rodriguez <lukes357@...il.com>
To:	Richard Weinberger <richard@....at>
Cc:	bp@...en8.de, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/1] lib: small update for strlen, strnlen, use less cpu instructions

thank you for your comments, begginer mistakes (I guess),

On 6/17/15, Richard Weinberger <richard@....at> wrote:
> Am 17.06.2015 um 00:51 schrieb Orestes Leal Rodriguez:
>>> Use the force^Wcheckpatch.pl.
>> This is the output of checkpatch.pl:
>> output of checkpatch: total: 0 errors, 0 warnings, 42 lines checked
>> /root/string.c.patch has no obvious style problems and is ready for
>> submission
>
> But it does not apply at all.
> Did you test it? I fear your mail client did some whitespace damage.

yes, I patched my original source tree with it.

>
>>> You need to explain that in the commit message, my young padawan.
>> Very small update to strlen and strnlen that now use less cpu instructions
>> by using a counter to avoid memory address
>> arithmetic, which cause that the compiler adds more machine
>> instructions for computing the length of the string just before
>> returning from the functions, the old machine code is like the
>> following:
>>
>>  mov    -0x4(%ebp),%edx
>>  mov    0x8(%ebp),%eax
>>  sub    %eax,%edx
>>  mov    %edx,%eax
>>  leave
>>  ret
>>
>>
>> now in the new versions the value is not calculated anymore,
>> instead  he value of the counter is put on eax after the
>> condition inside the loop no longer holds, and then return:
>>
>>  mov    -0x4(%ebp),%eax
>>  leave
>>  ret
>>
>> With this a few cpu instructions are saved.
>
> x86_32 does not matter here as we have already an optimized strlen() in
> arch/x86/lib/string_32.c.
> Did you check whether the optimization is worth on other archs?
> Hint: grep __HAVE_ARCH_STRLEN

I only have access to x86 cpus, but thanks for the hint

>
>>
>> Signed-off-by: Orestes Leal Rodriguez <lukes357@...il.com>
>> ---
>>
>> Signed-off-by: Orestes Leal Rodriguez <lukes357@...il.com>
>
> What does this 2nd SoB here?

Again, confusion about where to put the sob.

>
>> diff --git a/lib/string.c b/lib/string.c
>> index 992bf30..c873436 100644
>> --- a/lib/string.c
>> +++ b/lib/string.c
>> @@ -17,6 +17,10 @@
>>   * * Sat Feb 09 2002, Jason Thomas <jason@...ic.com.au>,
>>   *                    Matthew Hawkins <matt@...dropbear.id.au>
>>   * -  Kissed strtok() goodbye
>> + *
>> + * * Tuesday June 16 2015, Orestes Leal Rodriguez <lukes357@...il.com>
>> + * - strlen, strnlen: by using a single counter we use less cpu
>> instructions
>> + *   by avoiding substracting the memory addresses before return
>
> No need to add anything here. These days we have git. :-)

Ups!

>
>>   */
>>
>>  #include <linux/types.h>
>> @@ -401,11 +405,11 @@ EXPORT_SYMBOL(strim);
>>   */
>>  size_t strlen(const char *s)
>>  {
>> -    const char *sc;
>> +    size_t sz = 0;
>>
>> -    for (sc = s; *sc != '\0'; ++sc)
>> -        /* nothing */;
>> -    return sc - s;
>> +    for (; *s++ != '\0'; sz++)
>> +        /* empty */;
>
> <nitpick>
> Why suddenly "empty" instead of "nothing"?
> </nitpick>

I was bored.

>
>> +    return sz;
>>  }
>>  EXPORT_SYMBOL(strlen);
>>  #endif
>> @@ -418,12 +422,13 @@ EXPORT_SYMBOL(strlen);
>>   */
>>  size_t strnlen(const char *s, size_t count)
>>  {
>> -    const char *sc;
>> +    size_t sz = 0;
>>
>> -    for (sc = s; count-- && *sc != '\0'; ++sc)
>> -        /* nothing */;
>> -    return sc - s;
>> +    for (; count-- && *s++ != '\0'; sz++)
>> +        /* empty */;
>
> Same here.
>
> Thanks,
> //R2D2
>

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

Powered by blists - more mailing lists