Re: [PATCH] compiler.h: Move instrumentation_begin()/end() into new <linux/instrumentation.h> header

[Ingo Molnar]

* Ingo Molnar <mingo@xxxxxxxxxx> wrote:

> 
> * Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
> 
> > On Mon, Jun 1, 2020 at 6:08 AM Ingo Molnar <mingo@xxxxxxxxxx> wrote:
> > >
> > >  include/linux/compiler.h            | 53 +++++++++++++++++++++++
> > 
> > I have pulled this, but do we really want to add this to a header file
> > that is _so_ core that it gets included for basically every single
> > file built?
> > 
> > I don't even see those instrumentation_begin/end() things used
> > anywhere right now.
> > 
> > It seems excessive. That 53 lines is maybe not a lot, but it pushed
> > that header file to over 12kB, and while it's mostly comments, it's
> > extra IO and parsing basically for _every_ single file compiled in the
> > kernel.
> > 
> > For what appears to be absolutely zero upside right now, and I really
> > don't see why this should be in such a core header file!
> > 
> > I don't even see this as having anything at all to do with
> > "compiler.h" in the first place.
> > 
> > I really think we should think twice about making core header files
> > bigger like this. No, we're nowhere the disaster that C++ project
> > headers are, but tokenization and parsing is actually a pretty big
> > part of the build costs (which may surprise some people who think it's
> > all the fancy optimizations that cost a lot of CPU time).
> 
> Fully agreed - and I made the attached patch to address this.
> 
> The code got cleaner and better structured, but it didn't help much in 
> terms of inclusion count:
> 
>    2616  total .o files
> 
>    2447  <linux/types.h>
>    2436  <linux/compiler.h>
>    2404  <linux/bug.h>
> 
> The reason is that <linux/bug.h> is included almost everywhere as 
> well, and the instrumentation_begin()/end() annotations affect the 
> BUG*() and WARN*() primitives as well.
> 
> At least non-x86 would have less instrumentation related noise, for 
> now at least.

>  arch/x86/include/asm/bug.h       |  1 +
>  include/linux/compiler.h         | 53 -------------------------------------
>  include/linux/context_tracking.h |  2 ++
>  include/linux/instrumentation.h  | 57 ++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 60 insertions(+), 53 deletions(-)

The tested v2 version of the patch also needed the include in 
asm-generic/bug.h (see the fix attached below), because for 
completeness the generic version was annotated as well - even though 
only x86 has objtool support for now.

The readability improvement is real though.

Thanks,

	Ingo


diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
index 384b5c835ced..c43b5906e0dc 100644
--- a/include/asm-generic/bug.h
+++ b/include/asm-generic/bug.h
@@ -3,6 +3,7 @@
 #define _ASM_GENERIC_BUG_H
 
 #include <linux/compiler.h>
+#include <linux/instrumentation.h>
 
 #define CUT_HERE		"------------[ cut here ]------------\n"