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

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

[Tanzir Hasan]

This diff uses an open source tool include-what-you-use (IWYU) to modify
the include list, changing indirect includes to direct includes. IWYU is
implemented using the IWYUScripts github repository which is a tool that
is currently undergoing development. These changes seek to improve build
times.

This change to vdso/extable.c resulted in a preprocessed size of
vdso/extable.i from 64332 lines to 45377 lines (-27%) for the x86
defconfig.

Signed-off-by: Tanzir Hasan <tanzirh@google.com>
---
 arch/x86/entry/vdso/extable.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/x86/entry/vdso/extable.c b/arch/x86/entry/vdso/extable.c
index afcf5b65beef..8221231917ec 100644
--- a/arch/x86/entry/vdso/extable.c
+++ b/arch/x86/entry/vdso/extable.c
@@ -1,10 +1,14 @@
 // SPDX-License-Identifier: GPL-2.0
-#include <linux/err.h>
 #include <linux/mm.h>
+#include <linux/sched.h>
+#include <linux/stddef.h>
+#include <linux/types.h>
 #include <asm/current.h>
-#include <asm/traps.h>
+#include <asm/trapnr.h>
 #include <asm/vdso.h>
 
+struct pt_regs;
+
 struct vdso_exception_table_entry {
 	int insn, fixup;
 };

---
base-commit: f5837722ffecbbedf1b1dbab072a063565f0dad1
change-id: 20231228-extable-18f095e0aeba

Best regards,
-- 
Tanzir Hasan <tanzirh@google.com>

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

[Borislav Petkov]

On Thu, Dec 28, 2023 at 08:56:35PM +0000, Tanzir Hasan wrote:
> This diff uses an open source tool include-what-you-use (IWYU) to modify
> the include list, changing indirect includes to direct includes. IWYU is
> implemented using the IWYUScripts github repository which is a tool that
> is currently undergoing development. These changes seek to improve build
> times.
> 
> This change to vdso/extable.c resulted in a preprocessed size of
> vdso/extable.i from 64332 lines to 45377 lines (-27%) for the x86
> defconfig.

Maybe I'm missing something but this sounds like the build-time
"improvement" is so minute that you have to look at preprocessed sizes.

Do you have any, more "noticeable" numbers to justify this churn?

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

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

[Tanzir Hasan]

Hi Boris,

> Maybe I'm missing something but this sounds like the build-time
> "improvement" is so minute that you have to look at preprocessed sizes.
>
> Do you have any, more "noticeable" numbers to justify this churn?

These changes are mainly targeted at reducing preprocessing size which is why
it was brought up. You're not wrong about build times. The changes in build time
are small. To build the original file as is, it takes .69 seconds on
average on my
devices. This changes it to be .55 seconds on average so ~17%.

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.

Best,
Tanzir

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

[Borislav Petkov]

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.

And I'd understand it if the benefits were higher than what you're
seeing. But right now it looks like 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. And then do it
again in a couple of releases, when it becomes necessary again.

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.

But let's see what the others think first.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

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

[Nick Desaulniers]

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

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

[Borislav Petkov]

On Wed, Jan 03, 2024 at 11:36:35AM -0800, Nick Desaulniers wrote:
> 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.

I know, but you have to realize that header files are not a static thing
- we do "refactor" them all the time. So it's not like we'll refactor
them and they'll be done. No, we'll refactor them again later.

> 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.

Yap, the need for some tool or facility to keep include files proper and
optimal is indisputable. There's a reason it is called the "Include
Hell".

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

There's the other side of that coin: if we keep applying those, it'll
turn into a never-ending stream of silly fixes. Like spellchecking and
whitespace cleanups. And maintainers will start ignoring them.

> 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.

You could do a single patch *set* and say, before and after, we get
these improvements. And the whole set goes in in one fell swoop.

> > 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.

That's why I said "when it becomes necessary". :)

> 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.

Yap, that would be best. Because it'll stop from the whole include hell
from even ensuing in the first place.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

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

[Nick Desaulniers]

On Thu, Dec 28, 2023 at 12:56 PM Tanzir Hasan <tanzirh@google.com> wrote:
>
> This diff uses an open source tool include-what-you-use (IWYU) to modify
> the include list, changing indirect includes to direct includes. IWYU is
> implemented using the IWYUScripts github repository which is a tool that
> is currently undergoing development. These changes seek to improve build
> times.
>
> This change to vdso/extable.c resulted in a preprocessed size of
> vdso/extable.i from 64332 lines to 45377 lines (-27%) for the x86
> defconfig.
>
> Signed-off-by: Tanzir Hasan <tanzirh@google.com>
> ---
>  arch/x86/entry/vdso/extable.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/entry/vdso/extable.c b/arch/x86/entry/vdso/extable.c
> index afcf5b65beef..8221231917ec 100644
> --- a/arch/x86/entry/vdso/extable.c
> +++ b/arch/x86/entry/vdso/extable.c
> @@ -1,10 +1,14 @@
>  // SPDX-License-Identifier: GPL-2.0
> -#include <linux/err.h>
>  #include <linux/mm.h>

^ I think we can get more aggressive about avoiding linux/mm.h even.
I think you could replace linux/mm.h with

#include <linux/mm_types.h>  // for mm_struct, mm_struct::(anonymous)
#include <asm/mmu.h>         // for mm_context_t

assuming it's ok to include asm/* files in .c files under arch/* (more below).

> +#include <linux/sched.h>
> +#include <linux/stddef.h>
> +#include <linux/types.h>
>  #include <asm/current.h>
> -#include <asm/traps.h>
> +#include <asm/trapnr.h>
>  #include <asm/vdso.h>
>
> +struct pt_regs;
> +

^ This forward declaration shouldn't be necessary and looks wrong;
pt_regs is not used as an opaque type in this TU.

What I suspect is happening:
1. debug output from your tooling seems to think pt_regs should come
from linux/ptrace.h.
2. your tooling has a rule to replace recommendations for asm/ptrace.h
with linux/ptrace.h.  I may have misunderstood Al's point in an
earlier thread; perhaps it's ok to use asm/*.h in .c files under
arch/*/, but linux/*.h should be used everywhere else?  If that's the
case then perhaps we can break up that table such that asm-generic/*.h
-> asm/*.h is one table that's always used, and asm/*.h -> linux/*.h
is another table that's conditionally used when the .c file is not
under arch/*/?

To avoid pinging maintainers too many times, why don't we do an
internal code review for v2 before sending it publicly?  I appreciate
you making progress by sending a v1 while I was on holiday; for v2,
let's review internally first.
-- 
Thanks,
~Nick Desaulniers