Received-SPF: pass (google.com: domain of linux-kernel+bounces-61779-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Mon, 12 Feb 2024 13:56:54 +0000
From: Mark Rutland <mark.rutland@arm.com>
To: Max Kellermann <max.kellermann@ionos.com>
Cc: linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 00/35] Fast kernel headers: reduce header dependencies
Message-ID: <ZcojpuqGNToctwNi@FVFF77S0Q05N.cambridge.arm.com>
References: <20240211123000.3359365-1-max.kellermann@ionos.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240211123000.3359365-1-max.kellermann@ionos.com>

Hi Max,

On Sun, Feb 11, 2024 at 01:29:25PM +0100, Max Kellermann wrote:
> This patch set aims to reduce the dependencies between headers, in
> order to have cleaner code and speed up the build.  It continues
> previous efforts by other developers.

While the intent of the series is admirable, as it currently stands it's very
painful to review:

* Patch 1 is *gigantic*. 3MiB+ of plain text cannot reasonable be reviewed by a
  human. This needs to be split into smaller patches, and that needs a more
  thorough commit message (e.g. *how* you determined specific headers were
  necessary).

  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 indirect
  includes).

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

> As a preparation, the first patch adds "#include" directives to source
> files that were missing previously, but due to indirect includes, this
> was never noticed.  After the cleanup, many missing directives would
> result in a compiler failure.
> 
> The second patch removes superfluous "#include" directives, some of
> which may be a leftover from refactoring patches.
> 
> The third patch replaces existing "#include" directives with narrower
> ones, e.g. use "spinlock_types.h" instead of "spinlock.h".  This
> continues the work others have done over the years.
> 
> The remaining patches add new "XXX_types.h" headers with lighter
> dependencies.  They have only basic struct/enum/const/macro
> definitions and maybe a few trivial inline functions, but no "extern"
> functions and no complex header dependencies.
> 
> Just like the other attempts to reduce header dependencies in the
> past, this is just the beginning.  There are still too many
> dependencies, and the speedup gained by this large patch set is not
> yet impressive.
> 
> Prior to this patch set:
> 
>  real	0m34.677s
>  user	23m13.045s
>  sys	2m26.007s
> 
> With this patch set:
> 
>  real	0m34.464s
>  user	22m19.073s
>  sys	2m15.246s
> 
> (Building the directories kernel,lib,mm on ARM64 "allyesconfig".)
> 
> I have tested this patch set with:
> 
> - ARCH=arm allyesconfig
> - ARCH=arm defconfig
> - ARCH=arm64 allyesconfig
> - ARCH=arm64 defconfig
> - ARCH=mips defconfig
> - ARCH=riscv defconfig
> - ARCH=x86_64 allyesconfig
> - ARCH=xtensa defconfig
> 
> Pretty sure, other architectures may fail to build, but before I test
> all of them, I'd like to get some feedback on whether my approach
> would be accepted.
> 
> For more gains, huge headers like "linux/mm.h", "linux/fs.h" and
> "linux/sched.h" would need to be optimized.  Nearly everybody includes
> them, and they include nearly everything.

IIUC the same is true of kernel.h.

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

Mark.