prepare kconfig inline optimization for all architectures

Fix the OPTIMIZE inline functionality such that it can be used by all architectures.
To use it the arch maintainers shall add a
"select HAVE_OPTIMIZE_INLINING" in their arch specific Kconfig file.

This patch enables OPTIMIZE_INLINING for x86, sparc and sparc64. If all archs by into the approach the requirment for the select can be dropped.

The indirection with ARCH__SUPPORTS_OPTIMIZED_INLINING was killed as this is not needed when using select to decide what archs supports this.

---
Largely untested.
This patch was made as a consequence of the CONFIG_OPTIMIZE_INLINING thread on lkml.

Sam

index 49590f8..f08509e 100644
@@ -68,6 +68,7 @@ config SPARC
 	default y
 	select HAVE_IDE
 	select HAVE_OPROFILE
+	select HAVE_OPTIMIZE_INLINING
 
 # Identify this as a Sparc32 build
 config SPARC32

index 8acc5cc..01ecd33 100644
@@ -9,6 +9,7 @@ config SPARC
 	bool
 	default y
 	select HAVE_OPROFILE
+	select HAVE_OPTIMIZE_INLINING
 	select HAVE_KPROBES
 	select HAVE_KRETPROBES
 

index 2fadf79..d5e22ef 100644
@@ -20,6 +20,7 @@ config X86
 	def_bool y
 	select HAVE_IDE
 	select HAVE_OPROFILE
+	select HAVE_OPTIMIZE_INLINING
 	select HAVE_KPROBES
 	select HAVE_KRETPROBES
 	select HAVE_KVM if ((X86_32 && !X86_VOYAGER && !X86_VISWS && !X86_NUMAQ) || X86_64)

@@ -142,9 +143,6 @@ config AUDIT_ARCH
 config ARCH_SUPPORTS_AOUT
 	def_bool y
 
-config ARCH_SUPPORTS_OPTIMIZED_INLINING
-	def_bool y
-
 # Use the generic interrupt handling code in kernel/irq/:
 config GENERIC_HARDIRQS
 	bool

index 5b1979a..23956ea 100644
@@ -258,15 +258,3 @@ config CPA_DEBUG
 
 endmenu
 
-config OPTIMIZE_INLINING
-	bool "Allow gcc to uninline functions marked 'inline'"
-	default y
-	help
-	  This option determines if the kernel forces gcc to inline the functions
-	  developers have marked 'inline'. Doing so takes away freedom from gcc to
-	  do what it thinks is best, which is desirable for the gcc 3.x series of
-	  compilers. The gcc 4.x series have a rewritten inlining algorithm and
-	  disabling this option will generate a smaller kernel there. Hopefully
-	  this algorithm is so good that allowing gcc4 to make the decision can
-	  become the default in the future, until then this option is there to
-	  test gcc for this.

index b2fd754..79b7a34 100644
@@ -31,8 +31,7 @@
 /*
  * Force always-inline if the user requests it so via the .config:
  */
-#if !defined(CONFIG_ARCH_SUPPORTS_OPTIMIZED_INLINING) || \
-    !defined(CONFIG_OPTIMIZE_INLINING) && (__GNUC__ >= 4)
+#if !(defined(CONFIG_OPTIMIZE_INLINING) && (__GNUC__ >= 4))
 # define inline		inline		__attribute__((always_inline))
 # define __inline__	__inline__	__attribute__((always_inline))
 # define __inline	__inline	__attribute__((always_inline))

index ba3a389..5d8ef89 100644
@@ -508,6 +508,25 @@ config CC_OPTIMIZE_FOR_SIZE
 
 	  If unsure, say N.
 
+# archs shall "select HAVE_OPTIMIZE_INLINING" to be offered the
+# possibility to enable OPTIMIZE_INLINING
+config HAVE_OPTIMIZE_INLINING
+	def_bool n
+
+config OPTIMIZE_INLINING
+	bool "Enable gcc 4.0 inline algorithm for functions marked 'inline'"
+	depends on HAVE_OPTIMIZE_INLINING
+	help
+	  This option determines if the kernel forces gcc to inline the
+	  functions developers have marked 'inline'. Doing so takes away
+	  freedom from gcc to do what it thinks is best, which is desirable
+	  for the gcc 3.x series of compilers.
+	  The gcc 4.x series have a rewritten inlining algorithm and
+	  enabling this option will generate a smaller kernel there.
+	  Hopefully this algorithm is so good that allowing gcc4 to
+	  make the decision can become the default in the future,
+	  until then this option is there to test gcc for this.
+
 config SYSCTL
 	bool
 

My opinion on this is still:
"OPTIMIZE" means "work around bugs in the kernel".

Bugs of the following kinds:
- usage of "inline" in .c files (it should virtually never be used there)
- too big functions in header files

And this "inline optimization" reverts only the effects of some of these bugs.

And if the wrong code gets not inlined we can even get size increases at some places.

Denys and Ilpo have already done some work in fixing such bugs.

After the dust of this merge window has settled I'll try to get us a similar result without this "optimization".

cu
Adrian

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

So then let us name it: GCC_INLINING

Suggested patch below.
I named it _GCC_ because this is a gcc specific feature.

Sam

index 49590f8..eec5cee 100644
@@ -68,6 +68,7 @@ config SPARC
 	default y
 	select HAVE_IDE
 	select HAVE_OPROFILE
+	select HAVE_GCC_INLINING
 
 # Identify this as a Sparc32 build
 config SPARC32

index 8acc5cc..98a736c 100644
@@ -9,6 +9,7 @@ config SPARC
 	bool
 	default y
 	select HAVE_OPROFILE
+	select HAVE_GCC_INLINING
 	select HAVE_KPROBES
 	select HAVE_KRETPROBES
 

index 2fadf79..b6b7795 100644
@@ -20,6 +20,7 @@ config X86
 	def_bool y
 	select HAVE_IDE
 	select HAVE_OPROFILE
+	select HAVE_GCC_INLINING
 	select HAVE_KPROBES
 	select HAVE_KRETPROBES
 	select HAVE_KVM if ((X86_32 && !X86_VOYAGER && !X86_VISWS && !X86_NUMAQ) || X86_64)

@@ -142,9 +143,6 @@ config AUDIT_ARCH
 config ARCH_SUPPORTS_AOUT
 	def_bool y
 
-config ARCH_SUPPORTS_OPTIMIZED_INLINING
-	def_bool y
-
 # Use the generic interrupt handling code in kernel/irq/:
 config GENERIC_HARDIRQS
 	bool

index 5b1979a..23956ea 100644
@@ -258,15 +258,3 @@ config CPA_DEBUG
 
 endmenu
 
-config OPTIMIZE_INLINING
-	bool "Allow gcc to uninline functions marked 'inline'"
-	default y
-	help
-	  This option determines if the kernel forces gcc to inline the functions
-	  developers have marked 'inline'. Doing so takes away freedom from gcc to
-	  do what it thinks is best, which is desirable for the gcc 3.x series of
-	  compilers. The gcc 4.x series have a rewritten inlining algorithm and
-	  disabling this option will generate a smaller kernel there. Hopefully
-	  this algorithm is so good that allowing gcc4 to make the decision can
-	  become the default in the future, until then this option is there to
-	  test gcc for this.

index b2fd754..9e1cdd5 100644
@@ -31,8 +31,7 @@
 /*
  * Force always-inline if the user requests it so via the .config:
  */
-#if !defined(CONFIG_ARCH_SUPPORTS_OPTIMIZED_INLINING) || \
-    !defined(CONFIG_OPTIMIZE_INLINING) && (__GNUC__ >= 4)
+#if !(defined(CONFIG_GCC_INLINING) && (__GNUC__ >= 4))
 # define inline		inline		__attribute__((always_inline))
 # define __inline__	__inline__	__attribute__((always_inline))
 # define __inline	__inline	__attribute__((always_inline))

index ba3a389..8d6eb70 100644
@@ -508,6 +508,25 @@ config CC_OPTIMIZE_FOR_SIZE
 
 	  If unsure, say N.
 
+# archs shall "select HAVE_GCC_INLINING" to be offered the
+# possibility to enable GCC_INLINING
+config HAVE_GCC_INLINING
+	def_bool n
+
+config GCC_INLINING
+	bool "Enable gcc 4.0 inline algorithm for functions marked 'inline'"
+	depends on HAVE_GCC_INLINING
+	help
+	  This option determines if the kernel forces gcc to inline the
+	  functions developers have marked 'inline'. Doing so takes away
+	  freedom from gcc to do what it thinks is best, which is desirable
+	  for the gcc 3.x series of compilers.
+	  The gcc 4.x series have a rewritten inlining algorithm and
+	  enabling this option will generate a smaller kernel there.
+	  Hopefully this algorithm is so good that allowing gcc4 to
+	  make the decision can become the default in the future,
+	  until then this option is there to test gcc for this.
+
 config SYSCTL
 	bool
 

i object to that - please keep it named the way we came up with:
OPTIMIZE_INLINING=y - because that's what it does.

It goes along the same line as CC_OPTIMIZE_FOR_SIZE=y. Perhaps rename it to CC_OPTIMIZE_INLINING=y?

Ingo

i.e. like the patch below.

Ingo

-------------------->
Subject: enable gcc inline optimization for all architectures
From: Sam Ravnborg <[email protected]>
Date: Sun, 27 Apr 2008 13:56:04 +0200

architectures that want to enable this feature should select HAVE_CC_OPTIMIZE_INLINING.

---
 arch/sparc/Kconfig           |    1 +
 arch/sparc64/Kconfig         |    1 +
 arch/x86/Kconfig             |    4 +---
 arch/x86/Kconfig.debug       |   12 ------------
 include/linux/compiler-gcc.h |    3 +--
 init/Kconfig                 |   19 +++++++++++++++++++
 6 files changed, 23 insertions(+), 17 deletions(-)

Index: linux/arch/sparc/Kconfig
===================================================================
@@ -68,6 +68,7 @@ config SPARC
 	default y
 	select HAVE_IDE
 	select HAVE_OPROFILE
+	select HAVE_CC_OPTIMIZE_INLINING
 
 # Identify this as a Sparc32 build
 config SPARC32

Index: linux/arch/sparc64/Kconfig
===================================================================
@@ -9,6 +9,7 @@ config SPARC
 	bool
 	default y
 	select HAVE_OPROFILE
+	select HAVE_CC_OPTIMIZE_INLINING
 	select HAVE_KPROBES
 	select HAVE_KRETPROBES
 

Index: linux/arch/x86/Kconfig
===================================================================
@@ -20,6 +20,7 @@ config X86
 	def_bool y
 	select HAVE_IDE
 	select HAVE_OPROFILE
+	select HAVE_CC_OPTIMIZE_INLINING
 	select HAVE_KPROBES
 	select HAVE_KRETPROBES
 	select HAVE_KVM if ((X86_32 && !X86_VOYAGER && !X86_VISWS && !X86_NUMAQ) || X86_64)

@@ -142,9 +143,6 @@ config AUDIT_ARCH
 config ARCH_SUPPORTS_AOUT
 	def_bool y
 
-config ARCH_SUPPORTS_OPTIMIZED_INLINING
-	def_bool y
-
 # Use the generic interrupt handling code in kernel/irq/:
 config GENERIC_HARDIRQS
 	bool

Index: linux/arch/x86/Kconfig.debug
===================================================================
@@ -258,15 +258,3 @@ config CPA_DEBUG
 
 endmenu
 
-config OPTIMIZE_INLINING
-	bool "Allow gcc to uninline functions marked 'inline'"
-	default y
-	help
-	  This option determines if the kernel forces gcc to inline the functions
-	  developers have marked 'inline'. Doing so takes away freedom from gcc to
-	  do what it thinks is best, which is desirable for the gcc 3.x series of
-	  compilers. The gcc 4.x series have a rewritten inlining algorithm and
-	  disabling this option will generate a smaller kernel there. Hopefully
-	  this algorithm is so good that allowing gcc4 to make the decision can
-	  become the default in the future, until then this option is there to
-	  test gcc for this.

Index: linux/include/linux/compiler-gcc.h
===================================================================
@@ -31,8 +31,7 @@
 /*
  * Force always-inline if the user requests it so via the .config:
  */
-#if !defined(CONFIG_ARCH_SUPPORTS_OPTIMIZED_INLINING) || \
-    !defined(CONFIG_OPTIMIZE_INLINING) && (__GNUC__ >= 4)
+#if !(defined(CONFIG_CC_OPTIMIZE_INLINING) && (__GNUC__ >= 4))
 # define inline		inline		__attribute__((always_inline))
 # define __inline__	__inline__	__attribute__((always_inline))
 # define __inline	__inline	__attribute__((always_inline))

Index: linux/init/Kconfig
===================================================================
@@ -508,6 +508,25 @@ config CC_OPTIMIZE_FOR_SIZE
 
 	  If unsure, say N.
 
+# archs shall "select HAVE_CC_OPTIMIZE_INLINING" to be offered the
+# possibility to enable CC_OPTIMIZE_INLINING
+config HAVE_CC_OPTIMIZE_INLINING
+	def_bool n
+
+config CC_OPTIMIZE_INLINING
+	bool "Allow gcc to uninline functions marked 'inline'"
+	depends on HAVE_CC_OPTIMIZE_INLINING
+	help
+	  This option determines if the kernel forces gcc to inline the
+	  functions developers have marked 'inline'. Doing so takes away
+	  freedom from gcc to do what it thinks is best, which is desirable
+	  for the gcc 3.x series of compilers.
+	  The gcc 4.x series have a rewritten inlining algorithm and
+	  enabling this option will generate a smaller kernel there.
+	  Hopefully this algorithm is so good that allowing gcc4 to
+	  make the decision can become the default in the future,
+	  until then this option is there to test gcc for this.
+
 config SYSCTL
 	bool
 

No.

It means that

- gcc used to (long ago) always honor "inline", and we had kernel code that depended on that in various ways (ie required that there was no return etc).

We've been mostly replacing the ones we know about with "__always_inline", but there may be some that remain. We'll find out, I guess.

- gcc was a total and utter piece of horrible crap in the inlining department, doign insane things and changing their documentation to match the new behaviour (and some people then claimed that it was always documented that way).

It would not inline big functions even when they statically collapsed to nothing, etc.

As a result, we really couldn't afford to let gcc make any inlining decisions, because the compiler was simply *broken*.

Linus

I'm looking at it from a different angle, all code in the kernel should follow the following rules [1]:
- no functions in .c files should be marked inline
- all functions in headers should be static inline
- all functions in headers should either be very small or collapse to become very small after inlining

I can simply not see any usecase for a non-forced inline in the kernel, and fixing the kernel should give a superset of the space savings of this "inline optimization".

cu
Adrian

[1] there might be rare exceptions

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Your whole argument is premised on the assumption that the compiler does the right thing.

That's a *known*to*be*bogus* assumption.

Modern versions of gcc may do the right thing. Note the two very important
code-words: "modern" and "may".

I'm just telling you that

- older versions of gcc (and by "older" I do not mean "really ancient" or "deprecated", but stuff that is still in use) are known to be total and utter crap when it comes to inlining

- even absent that, there are historical reasons stemming from even more ancient versions of gcc that are no longer in use.

In other words, my arguments have nothing to do with "I wish". They are plain facts. Why argue with them?

Linus

No, you seem to be misunderstanding what I am saying.

Status Quo as of 2.6.25:
- we force the compiler to always inline with "inline"
- we have inline's in .c files and too big functions in headers, and both of them are wrong

"inline optimization":
- we leave the compiler the choice whether or not to inline with "inline"
- we still have inline's in .c files and too big functions in headers, and both of them are wrong, but this "inline optimization" works around these in some of the cases

What I want instead:
- we continue to force the compiler to always inline with "inline"
- we remove the inline's in .c files and make too big functions in headers out-of-line

cu
Adrian

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

It doesn't matter whether we use inline or always_inline to force inlining of a function, that's just syntactical sugar. What is rather annoying is that with the config option we'd have something marked inline without actually meaning it's inline. And what's even worse is that this is depending on a user-visible config option which is entirely stupid.

inline => the developer hints the compiler that it may
be a good idea to inline this function

always_inline => the developer tell gcc that for some obscure reasons
MUST always inline uses of this function

With the config option we pass the inline hint to gcc (if enabled). So with the config option we have the possibility to pass a _hint_ to gcc about inlining.

Before the config option there were no difference between static int alwyas_inline foo() {}
and
static int inline foo() {}

With the config option we now have a situation where they actually differ as they should do (assuming gcc > 4.x).

So you say that it is safe to assume all places where we really need always_inline are annotedted such - and we do not need a simple config option that the user can uncheck.
Fine by me - I prefer the simpler solution.

Sam

As Linus mentioned the hint doesn't make any sense because gcc will get it wrong anyway. In fact when you look at kernel code it tends to inline the everything and the kitchensink as long as there's just one caller and this bloat the stack but doesn't inline where it needs to. Better don't try to mess with that and do it explicit.

I don't say it is that, it certainly isn't on powerpc and probably most other architectures right now, because only x86 got the fixup so far. But making it a user-visible option instead of an architecture opt in/out selection doesn't make any sense.

The thing is, the "inline" vs "always_inline" thing _could_ make sense, but sadly doesn't much.

Part of it is that gcc imnsho inlines too aggressively anyway in the absense of "inline", so there's no way "inline" can mean "you might inline" this, because gcc will do that anyway even without it. As a result, in _practice_ "inline" and "always_inline" end up being very close to each other - perhaps more so than they should.

I do obviously think that we're right to move into the direction that "inline" should be a hint. In fact, the biggest issue I have with the new kconfig option is that I think it should probably be unconditional, but I suspect that compiler issues and architecture issues make that not be a good idea.

It will take time before we've sorted out all the fall-out, because I bet there is still code out there that _should_ use __always_inline, but doesn't.

Linus

Yes, absolutely. In recent gcc versions it's in fact so bad that xfs
had to add a hack to mark every function not explicitly marker inline noinline because otherwise gcc would inline gazillions of big functions which combines with it's horrible liveness analysis led to enormous stack useage. (I think the liveness part is getting better these days to be fair)

Given that __always_inline only shows up in non-x86 arch code in homeopathic quantity it's a fair bet that there's a lot of breakage waiting there. Which was one of the initial complains about the way it's done..

yeah, we i think had one such issue in x86 (merged upstream prior and independently of this feature iirc). We also know that something on powerpc doesnt work right out of box with inline being a hint. And that's OK and expected - for years we had "inline == always_inline" and there was just no force that moved against that. It was in fact surprising how well it all worked out on x86 when we reintroduced this feature.

btw., another new aspect of this whole area is that a number of major distributions now do CONFIG_CC_OPTIMIZE_FOR_SIZE=y in their kernels, and that seems to have brought new life into -Os development and with every new gcc generation we get denser and denser code generated. It is an interesting fact that generating good _small_ code needs a very good compiler that understands the code flow very accurately and is able to find the smallest possible representation for that.

A side-effect of that is if we do -Os _and_ turn inline into a hint via CONFIG_CC_OPTIMIZE_INLINING=y, gcc tends to do a better job than before. It cannot just go and inline stuff agressively in stark contrast to the -Os goal that was given to it. It still does not do a perfect job though
- but now at least it has a _chance_ to improve. With always_inline we gave gcc no leeway at all.

a second aspect is that there's always the chance that some other compiler comes along that does things better than gcc. Again, with inline being a necessity we make it impossible for even sane compilers to do the right thing.

and a third aspect is that in the current -git kernel we have exactly 11801 inlines in non-include files. Fighting the constant influx of them was a futile act 3 years ago when Arjan and me created this feature for the first time and it is futile today. It is a compiler domain problem and we should not waste any manpower "fighting" them. Life is too short :)

Ingo

It is a kernel bug that we have inline's at the wrong places.

I will "waste" my manpower on fixing these bugs.

It is a well-known fact that much code that gets added to the kernel is crap, but that shouldn't be an excuse for not fixing bugs in the kernel.

cu
Adrian

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

yup.. but this patch doesn't change that... the "no inline keyword" case is totally unaffected...

(actually, other than some obscure commandline options, the only sane way to avoid gcc doing this too agressive is using -Os)

If you want to reach me at my work email, use arjan-VuQAYsv1563Yd54FQh9/[email protected]
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Well, CC_OPTIMIZE_FOR_SIZE has been defaulting to 'y' for a *loong* time, but it's hidden behind a EXPERIMENTAL (unless you were on some embedded architectures), so many people won't see it.

Perhaps it is time to remove the EXPERIMENTAL? I think the gcc warnings were mostly bogus - it's not as if there haven't been compiler bugs without -Os too..

Linus

i was wondering about that when doing the OPTIMIZE_INLINING change - lets do something like the patch below?

Ingo

-------------->
Subject: make CC_OPTIMIZE_FOR_SIZE non-experimental
From: Ingo Molnar <[email protected]>
Date: Mon Apr 28 01:39:43 CEST 2008

this option has been the default on a wide range of distributions for a long time - time to make it non-experimental.

---
 init/Kconfig |    6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

Index: linux-x86.q/init/Kconfig
===================================================================
@@ -496,16 +496,12 @@ source "usr/Kconfig"
 endif
 
 config CC_OPTIMIZE_FOR_SIZE
-	bool "Optimize for size (Look out for broken compilers!)"
+	bool "Optimize for size"
 	default y
-	depends on ARM || H8300 || SUPERH || EXPERIMENTAL
 	help
 	  Enabling this option will pass "-Os" instead of "-O2" to gcc
 	  resulting in a smaller kernel.
 
-	  WARNING: some versions of gcc may generate incorrect code with this
-	  option.  If problems are observed, a gcc upgrade may be needed.
-
 	  If unsure, say N.
 
 config SYSCTL

The tendency is that gcc inline *more than we whish* - not less. Which is why we have noinline - to cover the cases where we do not want stuff inlined.

What the patch in question does is to make a difference between always_inle and inline.
Previously they were the same. With the patch applied and with a gcc > 4.0 inline is now a hint.

Did you actually read the patch?

If you read the patch you will see that the architectures that want to enable this has to do an explicit HAVE_CC_INLINE_HINT so powerpc is not impacted by this until they request it. Exactly the reason why this was not widely enabled in the first place (but implemnted in a too x86 specific way).

Updated patch below. I btw. renamed the option to say this is about a INLINE_HINT.

Sam

index 49590f8..9645a6f 100644
@@ -68,6 +68,7 @@ config SPARC
 	default y
 	select HAVE_IDE
 	select HAVE_OPROFILE
+	select HAVE_CC_INLINE_HINT
 
 # Identify this as a Sparc32 build
 config SPARC32

index 8acc5cc..3c8ba36 100644
@@ -9,6 +9,7 @@ config SPARC
 	bool
 	default y
 	select HAVE_OPROFILE
+	select HAVE_CC_INLINE_HINT
 	select HAVE_KPROBES
 	select HAVE_KRETPROBES
 

index 2fadf79..48e352b 100644
@@ -20,6 +20,7 @@ config X86
 	def_bool y
 	select HAVE_IDE
 	select HAVE_OPROFILE
+	select HAVE_CC_INLINE_HINT
 	select HAVE_KPROBES
 	select HAVE_KRETPROBES
 	select HAVE_KVM if ((X86_32 && !X86_VOYAGER && !X86_VISWS && !X86_NUMAQ) || X86_64)

@@ -142,9 +143,6 @@ config AUDIT_ARCH
 config ARCH_SUPPORTS_AOUT
 	def_bool y
 
-config ARCH_SUPPORTS_OPTIMIZED_INLINING
-	def_bool y
-
 # Use the generic interrupt handling code in kernel/irq/:
 config GENERIC_HARDIRQS
 	bool

index 5b1979a..23956ea 100644
@@ -258,15 +258,3 @@ config CPA_DEBUG
 
 endmenu
 
-config OPTIMIZE_INLINING
-	bool "Allow gcc to uninline functions marked 'inline'"
-	default y
-	help
-	  This option determines if the kernel forces gcc to inline the functions
-	  developers have marked 'inline'. Doing so takes away freedom from gcc to
-	  do what it thinks is best, which is desirable for the gcc 3.x series of
-	  compilers. The gcc 4.x series have a rewritten inlining algorithm and
-	  disabling this option will generate a smaller kernel there. Hopefully
-	  this algorithm is so good that allowing gcc4 to make the decision can
-	  become the default in the future, until then this option is there to
-	  test gcc for this.

index b2fd754..e49fade 100644
@@ -31,8 +31,7 @@
 /*
  * Force always-inline if the user requests it so via the .config:
  */
-#if !defined(CONFIG_ARCH_SUPPORTS_OPTIMIZED_INLINING) || \
-    !defined(CONFIG_OPTIMIZE_INLINING) && (__GNUC__ >= 4)
+#if !(defined(CONFIG_HAVE_CC_INLINE_HINT) && (__GNUC__ >= 4))
 # define inline		inline		__attribute__((always_inline))
 # define __inline__	__inline__	__attribute__((always_inline))
 # define __inline	__inline	__attribute__((always_inline))

index ba3a389..3568647 100644
@@ -508,6 +508,13 @@ config CC_OPTIMIZE_FOR_SIZE
 
 	  If unsure, say N.
 
+# archs shall "select HAVE_CC_INLINE_HINT"
+# to avoid forcing all function annotated inline
+# to be inline.
+# Note: This is only enforced for gcc > 4.0
+config HAVE_CC_INLINE_HINT
+	def_bool n
+
 config SYSCTL
 	bool
 

Which is exactly the wrong way around. noinline should be the default. But that's probaby no fixable without touching gcc or doing ugly hacks as in xfs.

Yes. As as I said I think having them the same, or rather only having one of them makes more sense than having a hint in addition to force.

Yes, and that part of your patch is a good thing, and makes it at least sensible unlike ingos. The patch in this mail is the one we should probably go for short term, although the right way to sort it out is the long one, aka goign through all users of inline and deciding what they really want.

Sure, I can agree with that as a mostly good goal, but you're still ignoring the fact that nobody should really expect the compiler to always do a good job at deciding high-level issues.

For example, what's wrong with having "inline" on functions in .c files if the author thinks they are small enough? He's likely right. Considering past behaviour, he's quite often more right than the compiler.

Just as an example of this: gcc will often inline even big functions, if they are called from only one call-site. In fact, ask a compiler guy, and he'll likely say that that is obviously a good thing.

But ask somebody who debugs the resulting oops reports, and he may well disagree violently.

In other words, inlining is about much more than pure optimization.

Sometimes it's about forcing it (or not forcing it) for simple correctness issues when the compiler doesn't understand that the code in question has specific rules (for example, we sometimes want to *force* certain functions to be in specific segments).

And sometimes it's about debugging. For the kernel, backtraces posted by random users are one of the main debug facilities, and unlike many other projects, it's not reasonable to ask people to recompile with "-O0 -g" to get better backtraces. The bulk of all reports will come from people who use precompiled images from a distribution.

And that means that inlining has a *huge* impact on debuggability.

I have vey often cursed gcc inlining some biggish function - who the f*ck cares if a thousand-instruction function can shave a couple of instructions of call overhead, when it then causes the call trace to be really hard to read?

So quite frankly, my preferred optimization would be:

- Heavily discourage gcc from inlining functions that aren't marked "inline". I suspect it hurts kernel debugging more than many other projects (because other projects aren't as dependent on the traces)

- I do agree 100% with you that header file functions should be small (unless they use __builtin_constant_p() or other tricks to guarantee a much smaller static footprint than dynamic one)

- I also suspect we should have some way for developers to ask fo *hints* from the compiler, ie instead of having gcc inline on its own by default, have the people who care about it ask the compiler to warn about cases where inlining would be a big win.

- Make "inline" mean "you may want to inline this", and "forced_inline" mean "you *have* to inline this". Ie the "inline" is where the compiler can make a subtle choice (and we need that, because sometimes architecture or config options means that the programmer should not make the choice statically!)

In short, in general I actually wish we'd inline much much less than we do. And yes, part of that is that we have way too much code in our header files.

Linus

Actually looking at older code in the tree he's most likely wrong :) Probably as bad as the compiler. But the nice part about the code is that we can fix it easily.

Good point.

It *would* be really interesting to have some way to check our assumptions (both ways - warn about over-large inlines and small-and-hot non-inlines).

I considered making sparse give some size estimate for inlines and warn about ones that generate a lot of code, but I was never able to do it sanely.

Linus

Ingo's commit in your tree just broke this assumption.

And he justified breaking it with a 2.3% size decrease.

And after
- grep'ing for "inline" through drivers/
- remembering how driver authors use likely/unlikely
- remembering the practically non-existing quality criteria you set for new drivers [1]
"He's likely right." is likely not right for the average driver author.

You might be smarter than gcc, but most kernel developers (including me) are not.

cu
Adrian

[1] you said you really wanted this driver merged in this state in 2.6.25:

grep -C4 volatile drivers/infiniband/hw/nes/nes_nic.c

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Note that our problem is too much inlining, not too little.

I'm actually happier with gcc not deciding to inline (despite having an "inline") than I am with gcc deciding to inline (in violation of _not_ having an "inline").

So I don't disagree with Ingo's commit per se.

The only problem with not inlining is a historical one: exactly because gcc _used_ to always do what people asked for, Linux has historically treated "inline" as a "force_inline". And I was very unhappy when gcc changed that, just because it broke historically good code.

In many ways, it might have been better if we had a "__may_inline" thing to tell the compiler "you can inline this if you think it's worth it"). Both gcc (long ago) and Ingo (now) decided to just make plain "inline" mean that, but with a pretty strong bias. It was wrong for gcc to do so, imho, and it may have been wrong for this OPTIMIZE_INLINE thing too.

Linus

What is wrong with that? I believe the term is 'directive'.

Yes, correct the source.

How did it come to pass that we invented such a thing as an optional directive?

-Mike

It's even worse than that, that may not even get it right. That's why we need always_inline to force it when we know we need it. Now for the cases where we don't know know that we either need or really really strongly want it we should just not mark it inline at all.

Here's a good counterexample: kernel/mutex.c.

__mutex_lock_common wants to be inlined into __mutex_lock_*_slowpath. and *_slowpath *shouldn't* be inlined into mutex_lock_*.

Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

If we really want to force gcc to emit 6 copies of this not so small function then Ingo's commit in Linus' tree has already broken it on x86.

"noinline" is nothing anyone wants to change.

cu
Adrian

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed