Received-SPF: pass (google.com: domain of linux-kernel+bounces-61833-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240211123000.3359365-1-max.kellermann@ionos.com> <ZcojpuqGNToctwNi@FVFF77S0Q05N.cambridge.arm.com>
In-Reply-To: <ZcojpuqGNToctwNi@FVFF77S0Q05N.cambridge.arm.com>
From: Max Kellermann <max.kellermann@ionos.com>
Date: Mon, 12 Feb 2024 15:41:50 +0100
Message-ID: <CAKPOu+_GXMK=wv=THXqa9knS-vBf4JWd0VaTz93-hxucAS18vg@mail.gmail.com>
Subject: Re: [PATCH v3 00/35] Fast kernel headers: reduce header dependencies
To: Mark Rutland <mark.rutland@arm.com>
Cc: linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Mon, Feb 12, 2024 at 2:57=E2=80=AFPM Mark Rutland <mark.rutland@arm.com>=
 wrote:
> * Patch 1 is *gigantic*. 3MiB+ of plain text cannot reasonable be reviewe=
d by a
>   human. This needs to be split into smaller patches, and that needs a mo=
re
>   thorough commit message (e.g. *how* you determined specific headers wer=
e
>   necessary).

I understand that this is a huge patch that's difficult to review; but
really a hell lot of "#include" directives are missing, and I didn't
know how to split this patch in a way that would make review easier.
There would be many small patches all with the same commit message
essentially doing the same, just for different source files or for
different target headers - is that really an improvement?

>   This could probably be a series on its own, or could be split up along =
more
>   logical lines (e.g. have a series cleaning up a *particular* case of in=
direct
>   includes).

I can do that if that's the consensus.
So, let's say one patch that adds "linux/kobject.h" everywhere it's
missing in all sources across the tree - and then one patch that adds
"linux/sprintf.h" across the tree and so on?

I have no real opinion - I just kept refreshing that patch in stgit
with all the includes I found were missing because I had thousands and
thousands of build failures while working on the following patches.

> * There have been three versions of the series in two days. We usually ex=
pect
>   several says (e.g. a week) between versions, and posting multiple versi=
ons so
>   quickly just spams reviewers' inboxes and doesn't give people sufficien=
t time
>   to provide any meaningful review.

Sorry for that. There were so many combinations of architectures and
configurations that kept failing to build in LKP, and I didn't want to
keep them unfixed for too long, so I reposted with all the fixes, and
then more LKP failures arrived... after I ran "randconfig" in a loop
on several architectures for hours. The kernel headers are a fragile
mess, and unfortunately my attempt to clean it up is more messy than I
wanted it to be. I'll avoid to repost with fixes so often.

> IIUC the same is true of kernel.h.

True, and this patch set contains a few patches to address
linux/kernel.h. With my patches, only very few headers depend on
linux/kernel.h, which I achieved by moving very basic macros such as
lower_32_bits() to a separate header.

> How have you analyzed this? Are you using any specific tooling, or has th=
is all
> been manual analysis?

Manual analysis. This started off as a naive attempt to write a kernel
module in C++. As you can imagine, there are lots of problems with C++
compatibility in kernel headers, and because I'm lazy and didn't want
to fix all of those, I tried to reduce the set of headers I had to
include from my C++ module. With each C++ compiler error, I checked
whether including this header was really necessary; many weren't
necessary, but removing them caused build errors in hundreds of other
source files unrelated to my work. This turned out to be a rather deep
rabbit hole.

(Spoiler: I was able to build my C++ kernel module even before I
submitted the first version of this patch series - I have a few dozen
patches on my stgit stack for C++ compatibility which I will publish
when they're ready. But first the header dependency patches should be
merged, as they're a precondition for my C++ patch set.)

Max