Received-SPF: pass (google.com: domain of linux-kernel+bounces-166607-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Precedence: bulk
MIME-Version: 1.0
References: <20240502141359.89567-1-harry021633@gmail.com>
In-Reply-To: <20240502141359.89567-1-harry021633@gmail.com>
From: Andy Shevchenko <andy.shevchenko@gmail.com>
Date: Thu, 2 May 2024 17:59:58 +0300
Message-ID: <CAHp75Vd9PibrQA=tgZLHuv-kDXana9rGcu5s_aPqyxW6tDBYGw@mail.gmail.com>
Subject: Re: [PATCH 2/2] string: improve strlen performance
To: "Hsin-Yu.Chen" <harry021633@gmail.com>
Cc: keescook@chromium.org, andy@kernel.org, akpm@linux-foundation.org, 
	linux-hardening@vger.kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Thu, May 2, 2024 at 5:14=E2=80=AFPM Hsin-Yu.Chen <harry021633@gmail.com>=
 wrote:
>
> Port `strlen` in gcc,

This is the Linux kernel project. What does this mean?
Also note we refer to the function as strlen() (and no [back]quotes).

> which enhance performance over 10 times

>
> Please refer to these following articles
> 1. [Determine if a word has a byte less than n]
>    (https://graphics.stanford.edu/~seander/bithacks.html#HasLessInWord)
> 2. [Determine if a word has a zero byte]
>    (https://graphics.stanford.edu/~seander/bithacks.html#ZeroInWord)

Make these proper Link: tags

Link: URL#1 [1]
Link: URL#2 [2]

..

> -       const char *sc;
> +       const char *char_ptr;

No need to change the var name for this.

> +       const unsigned long *longword_ptr;
> +       unsigned long longword, himagic, lomagic;

Keep it in reversed xmas tree order.

..

> +       /* Handle the first few characters by reading one character at a =
time.
> +        * Do this until CHAR_PTR is aligned on a longword boundary.
> +        */

/*
 * This is the wrong comment style. You may
 * use this example.
 */

..

> +       for (char_ptr =3D s; ((unsigned long) char_ptr
> +               & (sizeof(longword) - 1)) !=3D 0;
> +               ++char_ptr)

This is too verbose (too many unneeded symbols) and why pre-increment?
What is special about it?

..

> +       /* All these elucidatory comments refer to 4-byte longwords,
> +        * but the theory applies equally well to 8-byte longwords.
> +        */

Use proper style.

> +       longword_ptr =3D (unsigned long *) char_ptr;

No space after casting and why do you need it?

..

> +       /* Bits 31, 24, 16, and 8 of this number are zero.
> +        * Call these bits the "holes."
> +        * Note that there is a hole just to the left of
> +        * each byte, with an extra at the end:
> +        * bits:  01111110 11111110 11111110 11111111
> +        * bytes: AAAAAAAA BBBBBBBB CCCCCCCC DDDDDDDD
> +        * The 1-bits make sure that carries propagate to the next 0-bit.
> +        * The 0-bits provide holes for carries to fall into.
> +        */

Use proper style.

..

> +               /* 64-bit version of the magic. */
> +               /* Do the shift in two steps to avoid a warning if long h=
as 32 bits.
> +                */

Ditto.

..

> +       if (sizeof(longword) > 8)
> +               abort();

Huh?!

..

> +       /* Instead of the traditional loop which tests each character,
> +        * we will test a longword at a time.  The tricky part is testing
> +        * if *any of the four* bytes in the longword in question are zer=
o.
> +        */

Proper style, please.

..

> +       for (;;) {
> +               longword =3D *longword_ptr++;
> +               if (((longword - lomagic) & ~longword & himagic) !=3D 0) =
{
> +
> +                       /* Which of the bytes was the zero?
> +                        * If none of them were, it was a misfire; contin=
ue the search.
> +                        */
> +                       const char *cp =3D (const char *) (longword_ptr -=
 1);

> +                       if (cp[0] =3D=3D 0)
> +                               return cp - s;
> +                       else if (cp[1] =3D=3D 0)
> +                               return cp - s + 1;
> +                       else if (cp[2] =3D=3D 0)
> +                               return cp - s + 2;
> +                       else if (cp[3] =3D=3D 0)
> +                               return cp - s + 3;

> +                       if (sizeof(longword) > 4) {

  if (... <=3D 4)
    continue;

> +                               if (cp[4] =3D=3D 0)
> +                                       return cp - s + 4;
> +                               else if (cp[5] =3D=3D 0)
> +                                       return cp - s + 5;
> +                               else if (cp[6] =3D=3D 0)
> +                                       return cp - s + 6;
> +                               else if (cp[7] =3D=3D 0)
> +                                       return cp - s + 7;

A lot of redundant 'else':s.

> +                       }
> +               }
> +       }

--=20
With Best Regards,
Andy Shevchenko