Re: [PATCH] x86/vdso: shrink vdso/extable.i via IWYU

From: Nick Desaulniers <ndesaulniers@google.com>
To: Borislav Petkov <bp@alien8.de>, Tanzir Hasan <tanzirh@google.com>
Cc: Andy Lutomirski <luto@kernel.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@redhat.com>,
	 Dave Hansen <dave.hansen@linux.intel.com>,
	x86@kernel.org,  "H. Peter Anvin" <hpa@zytor.com>,
	linux-kernel@vger.kernel.org,  Kees Cook <keescook@chromium.org>,
	Alexander Viro <viro@zeniv.linux.org.uk>,
	 Andy Shevchenko <andy@kernel.org>
Subject: Re: [PATCH] x86/vdso: shrink vdso/extable.i via IWYU
Date: Wed, 3 Jan 2024 11:36:35 -0800	[thread overview]
Message-ID: <CAKwvOd=aSQq3mcxGe-csNcygFWCbq03CPYFyc5DhjyvL_qYPAQ@mail.gmail.com> (raw)
In-Reply-To: <20231229093942.GBZY6T3knaGKpeRA9a@fat_crate.local>

On Fri, Dec 29, 2023 at 1:40 AM Borislav Petkov <bp@alien8.de> wrote:
>
> On Thu, Dec 28, 2023 at 02:01:20PM -0800, Tanzir Hasan wrote:
> > The intention with these changes is to add up the impact of these small changes
> > over time to improve build time and also use more direct inclusions where
> > possible.
>
> The problem I see with such "changes" is that it'll be a never-ending
> stream of them because over time, headers get reworked, moved, changed,
> split, etc and one would have to do such reorganization again.

By being more precise in what's necessary via making more specific use
of includes (making "indirect" includes into "direct" includes) in the
.c files, it will allow us to more easily refactor the header files
themselves.  Otherwise changing the .h files may necessitate updating
multiple .c files to fix this anyways first.

And yes, these inclusion lists will change over time.  As you say
below, they can be cleaned up again every couple of releases.  Though
right now most files have never been cleaned up at all.  If smaller
translation units like arch/x86/entry/vdso/extable.c don't change
often, I don't expect churn to those files even between multiple
releases.

> And I'd understand it if the benefits were higher than what you're
> seeing. But right now it looks like unnecessary churn.

Over time, they will add up. But not if we reject the patches as
unnecessary churn.

> Dunno, perhaps one could do those in one single patch for, say,
> on arch/x86/ or so, and show a before and after wrt numbers.

Maybe the overall numbers are interesting, but landing one patch that
updates numerous .c files' inclusion lists seems error prone.  I
suspect it's more likely that a more incremental approach of smaller
patches allows progress if there are any issues; a build
failure/mistake doesn't block the whole thing from landing.

Overall numbers can also be collected after the fact.

> And then do it
> again in a couple of releases, when it becomes necessary again.

That's the use case I had in mind.  Though I suspect the initial run
of this tooling will result in the most changes, as some files in tree
are hardly touched between releases. For those, I don't expect any
automated tooling to be churning those files after the initial
cleaning.

> Or one could run that tool on patches which haven't been applied yet,
> patches on lkml.
>
> Patches which touch headers or add #include directives and then reply
> with an automatic report to them, saying something along the lines of
>
> "your patch has a suboptimal include - if you include this and that,
> it'll improve build time by this and that"
>
> and then people would incorporate those changes and right then and
> there.

Yeah, the idea is that the tooling results in repeatable processes by
others.  One could imagine build bots running this, or integrating it
into checkpatch, or git presubmit hooks, or w/e.

But Tanzir's internship ends Friday, so he won't be building such a
service by then.  Something for me to keep in mind though for sure.

> But let's see what the others think first.

I have some feedback on the v1; I'll post it inline in another reply.
--
Thanks,
~Nick Desaulniers