Received-SPF: pass (google.com: domain of linux-kernel+bounces-4105-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20231214-libstringheader-v2-0-0f195dcff204@google.com>
 <20231214-libstringheader-v2-1-0f195dcff204@google.com> <ZXx5DyidpHFhRevy@smile.fi.intel.com>
 <CAE-cH4p5VJ_A91BAkURBN67ACA0_u7T8UhApUYLQDWeeRY6FWA@mail.gmail.com> <CAHp75Vdo=g7ZBAuk79PtLOYs-YHo5sCTp1WVFESHA0xN0zGEmQ@mail.gmail.com>
In-Reply-To: <CAHp75Vdo=g7ZBAuk79PtLOYs-YHo5sCTp1WVFESHA0xN0zGEmQ@mail.gmail.com>
From: Nick Desaulniers <ndesaulniers@google.com>
Date: Mon, 18 Dec 2023 08:57:59 -0800
Message-ID: <CAKwvOdmMqJacYRfwohY-DXBbmNmz_M4EKUL1KuTv=tT2dO_p1g@mail.gmail.com>
Subject: Re: [PATCH v2 1/2] sh: Added kernel.h to word-at-a-time
To: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: Tanzir Hasan <tanzirh@google.com>, Andy Shevchenko <andy@kernel.org>, 
	Kees Cook <keescook@chromium.org>, linux-hardening@vger.kernel.org, 
	linux-kernel@vger.kernel.org, Nick DeSaulniers <nnn@google.com>, 
	Andrew Morton <akpm@linux-foundation.org>, llvm@lists.linux.dev, 
	Al Viro <viro@zeniv.linux.org.uk>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Fri, Dec 15, 2023 at 11:09=E2=80=AFAM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Fri, Dec 15, 2023 at 8:31=E2=80=AFPM Tanzir Hasan <tanzirh@google.com>=
 wrote:
> > On Fri, Dec 15, 2023 at 8:04=E2=80=AFAM Andy Shevchenko <andy@kernel.or=
g> wrote:
> >> On Thu, Dec 14, 2023 at 09:06:12PM +0000, tanzirh@google.com wrote:
>
> ...
>
> >> > +#include <linux/kernel.h>
> >>
> >> I highly discourage from doing that. Instead, split what is needed to
> >> the separate (new) header and include that one.
> >
> >
> > I think it would make the most sense to do this in a separate patch.
> > What word-at-a-time.h needs from kernel.h is REPEAT_BYTE and to my know=
ledge,
> > almost every other version of word-at-a-time.h includes kernel.h gets t=
his by
> > including kernel.h. A future change could be removing REPEAT_BYTE
> > out of kernel.h
>
> Just create a patch that either moves that macro (along with upper_*()
> and lower_*() APIs) to a more distinguishable header
> (maybe bytes.h or words.h or wordpart.h, etc) and use it in your case
> and fix others.

Andy,
These are good suggestions and we should do them...

...and Tanzir only has 3 weeks left of his internship.  I don't want
him to get bogged down chasing build regressions from modifying the
headers themselves.  I think what's best for him from here through the
remainder of his internship is to stay focused on applying suggestions
from IWYU to just modify the #include list of .c files, and not start
splitting .h files.  Splitting the .h files will be the next step, and
is made easier by having the codebase not have so many indirect
includes (via IWYU), but we need time to soak header changes, and time
Tanzir does not have.  Please can we keep the suggestions focused on
whether the modifications to the header includes (and the tangential
cleanups) are correct?

While REPEAT_BYTE has a manageable number of users, upper_* and
lower_* have significantly more; I worry about moving those causing
regressions.  We can move them, but such changes would need
significantly more soak time than this series IMO.  Tanzir is also
working on statistical analysis; I suspect if he analyzes
include/linux/kernel.h, he can comment on whether the usage of
REPEAT_BYTE is correlated with the usage of upper_* and lower_* in
order to inform whether they should be grouped together or not.
--=20
Thanks,
~Nick Desaulniers