Re: [2.6 patch] kill __always_inline

Re: [2.6 patch] kill __always_inline

[Andi Kleen]

Nigel Cunningham <ncunningham@linuxmail.org> writes:
>
> Excuse me if I'm being ignorant, but I thought always_inline was
> introduced because with some recent versions of gcc, inline wasn't doing
> the job (suspend2, which requires a working inline, was broken by it for
> example). That is to say, doesn't the definition of always_inline vary
> with the compiler version?

It is just inline on some old gcc versions that didn't have 
__attribute__((always_inline)). But on these plain inline is usually fine 
- it will just inline. Only later versions of gcc broke their inlining
algorithm, but they luckily added this attribute as a workaround.

When you have functions that require inlining always mark them __always_inline

-Andi

[2.6 patch] kill __always_inline

[Adrian Bunk]

An issue that we already discussed at 2.6.8-rc2-mm2 times:

2.6.9-rc1 includes __always_inline which was formerly in  -mm.
__always_inline doesn't make any sense:

__always_inline is _exactly_ the same as __inline__, __inline and inline .


The patch below removes __always_inline again:


Signed-off-by: Adrian Bunk <bunk@fs.tum.de>

--- linux-2.6.9-rc1-mm2-full/include/linux/compiler.h.old	2004-08-31 23:30:51.000000000 +0200
+++ linux-2.6.9-rc1-mm2-full/include/linux/compiler.h	2004-08-31 23:31:09.000000000 +0200
@@ -124,8 +124,4 @@
 #define noinline
 #endif
 
-#ifndef __always_inline
-#define __always_inline inline
-#endif
-
 #endif /* __LINUX_COMPILER_H */
--- linux-2.6.9-rc1-mm2-full/include/asm-i386/fixmap.h.old	2004-08-31 23:31:35.000000000 +0200
+++ linux-2.6.9-rc1-mm2-full/include/asm-i386/fixmap.h	2004-08-31 23:31:47.000000000 +0200
@@ -129,7 +129,7 @@
  * directly without tranlation, we catch the bug with a NULL-deference
  * kernel oops. Illegal ranges of incoming indices are caught too.
  */
-static __always_inline unsigned long fix_to_virt(const unsigned int idx)
+static inline unsigned long fix_to_virt(const unsigned int idx)
 {
 	/*
 	 * this branch gets completely eliminated after inlining,

Re: [2.6 patch] kill __always_inline

[Andrew Morton]

Adrian Bunk <bunk@fs.tum.de> wrote:
>
> An issue that we already discussed at 2.6.8-rc2-mm2 times:
> 
> 2.6.9-rc1 includes __always_inline which was formerly in  -mm.
> __always_inline doesn't make any sense:
> 
> __always_inline is _exactly_ the same as __inline__, __inline and inline .
> 
> 
> The patch below removes __always_inline again:

But what happens if we later change `inline' so that it doesn't do
the `always inline' thing?

An explicit usage of __always_inline is semantically different than
boring old `inline'.

Re: [2.6 patch] kill __always_inline

[Adrian Bunk]

On Tue, Aug 31, 2004 at 03:36:49PM -0700, Andrew Morton wrote:
> Adrian Bunk <bunk@fs.tum.de> wrote:
> >
> > An issue that we already discussed at 2.6.8-rc2-mm2 times:
> > 
> > 2.6.9-rc1 includes __always_inline which was formerly in  -mm.
> > __always_inline doesn't make any sense:
> > 
> > __always_inline is _exactly_ the same as __inline__, __inline and inline .
> > 
> > 
> > The patch below removes __always_inline again:
> 
> But what happens if we later change `inline' so that it doesn't do
> the `always inline' thing?
> 
> An explicit usage of __always_inline is semantically different than
> boring old `inline'.

Who audits all current users of inline whether they are __always_inline?

Who ensures, that in the future there will always be the right one of 
inline and __always_inline choosen in the kernel?


If it doesn't give a compile or runtime error for anyone when it's 
wrong, many wrong inline/__always_inline will go into the kernel over 
time.

The intention might be a different semantics, but in the end, it won't 
work.


E.g., check how many wrong __init/__exit annotations show up in 2.6, 
each of whom would have been a compile error in 2.4 (and different 
from a wrong inline/__always_inline, a wrong __init/__exit annotation 
can cause real problems for users).


If you want to change inline at some point, you will have to audit all  
users of inline anyway - so why bother if you don't intend to change 
inline in the forseeable future?


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

Re: [2.6 patch] kill __always_inline

[Andrew Morton]

Adrian Bunk <bunk@fs.tum.de> wrote:
>
> If you want to change inline at some point, you will have to audit all  
> users of inline anyway - so why bother if you don't intend to change 
> inline in the forseeable future?

inline was set to do `always inline' when it was discovered that new gcc
was doing dopey things.  If gcc gets fixed then we don't need that any more.

But functions which *must* be inlined should be marked __always_inline
regardless of anything else.

Re: [2.6 patch] kill __always_inline

[Nigel Cunningham]

Hi.

On Wed, 2004-09-01 at 08:52, Adrian Bunk wrote:
> On Tue, Aug 31, 2004 at 03:36:49PM -0700, Andrew Morton wrote:
> > Adrian Bunk <bunk@fs.tum.de> wrote:
> > >
> > > An issue that we already discussed at 2.6.8-rc2-mm2 times:
> > > 
> > > 2.6.9-rc1 includes __always_inline which was formerly in  -mm.
> > > __always_inline doesn't make any sense:
> > > 
> > > __always_inline is _exactly_ the same as __inline__, __inline and inline .
> > > 
> > > 
> > > The patch below removes __always_inline again:
> > 
> > But what happens if we later change `inline' so that it doesn't do
> > the `always inline' thing?
> > 
> > An explicit usage of __always_inline is semantically different than
> > boring old `inline'.

Excuse me if I'm being ignorant, but I thought always_inline was
introduced because with some recent versions of gcc, inline wasn't doing
the job (suspend2, which requires a working inline, was broken by it for
example). That is to say, doesn't the definition of always_inline vary
with the compiler version?

Regards,

Nigel

-- 
Nigel Cunningham
Christian Reformed Church of Tuggeranong
PO Box 1004, Tuggeranong, ACT 2901

Many today claim to be tolerant. But true tolerance can cope with others
being intolerant.

Re: [2.6 patch] kill __always_inline

[Andrew Morton]

Nigel Cunningham <ncunningham@linuxmail.org> wrote:
>
> Hi.
> 
> On Wed, 2004-09-01 at 08:52, Adrian Bunk wrote:
> > On Tue, Aug 31, 2004 at 03:36:49PM -0700, Andrew Morton wrote:
> > > Adrian Bunk <bunk@fs.tum.de> wrote:
> > > >
> > > > An issue that we already discussed at 2.6.8-rc2-mm2 times:
> > > > 
> > > > 2.6.9-rc1 includes __always_inline which was formerly in  -mm.
> > > > __always_inline doesn't make any sense:
> > > > 
> > > > __always_inline is _exactly_ the same as __inline__, __inline and inline .
> > > > 
> > > > 
> > > > The patch below removes __always_inline again:
> > > 
> > > But what happens if we later change `inline' so that it doesn't do
> > > the `always inline' thing?
> > > 
> > > An explicit usage of __always_inline is semantically different than
> > > boring old `inline'.
> 
> Excuse me if I'm being ignorant, but I thought always_inline was
> introduced because with some recent versions of gcc, inline wasn't doing
> the job (suspend2, which requires a working inline, was broken by it for
> example).

IIRC, the compiler was generating out-of-line versions of functions in
every compilation unit whcih included the header file.  When we the
developers just wanted `inline' to mean `inline, dammit'.

If that broke swsusp in some manner then the relevant swsusp functions
should be marked always_inline, because they have some special needs.

> That is to say, doesn't the definition of always_inline vary
> with the compiler version?

If the compiler supports attribute((always_inline)) then the kernel build
system will use that.  If the compiler doesn't support
attribute((always_inline)) then we just emit `inline' from cpp and hope
that it works out.

Re: [2.6 patch] kill __always_inline

[Nigel Cunningham]

Hi.

On Wed, 2004-09-01 at 09:39, Andrew Morton wrote:
> IIRC, the compiler was generating out-of-line versions of functions in
> every compilation unit whcih included the header file.  When we the
> developers just wanted `inline' to mean `inline, dammit'.
> 
> If that broke swsusp in some manner then the relevant swsusp functions
> should be marked always_inline, because they have some special needs.

Yes, that's exactly right. Suspend relies upon inline always inlining,
and as I work around I added...

#undef inline
#define inline __inline__ __attribute__(always_inline))

while this discussion was going on. I should switch this to
always_inline now that it's merged.

> > That is to say, doesn't the definition of always_inline vary
> > with the compiler version?
> 
> If the compiler supports attribute((always_inline)) then the kernel build
> system will use that.  If the compiler doesn't support
> attribute((always_inline)) then we just emit `inline' from cpp and hope
> that it works out.

Thanks.

Nigel
-- 
Nigel Cunningham
Christian Reformed Church of Tuggeranong
PO Box 1004, Tuggeranong, ACT 2901

Many today claim to be tolerant. But true tolerance can cope with others
being intolerant.

Re: [2.6 patch] kill __always_inline

[Adrian Bunk]

On Tue, Aug 31, 2004 at 04:39:14PM -0700, Andrew Morton wrote:
> Nigel Cunningham <ncunningham@linuxmail.org> wrote:
> >
> > Hi.
> > 
> > On Wed, 2004-09-01 at 08:52, Adrian Bunk wrote:
> > > On Tue, Aug 31, 2004 at 03:36:49PM -0700, Andrew Morton wrote:
> > > > Adrian Bunk <bunk@fs.tum.de> wrote:
> > > > >
> > > > > An issue that we already discussed at 2.6.8-rc2-mm2 times:
> > > > > 
> > > > > 2.6.9-rc1 includes __always_inline which was formerly in  -mm.
> > > > > __always_inline doesn't make any sense:
> > > > > 
> > > > > __always_inline is _exactly_ the same as __inline__, __inline and inline .
> > > > > 
> > > > > 
> > > > > The patch below removes __always_inline again:
> > > > 
> > > > But what happens if we later change `inline' so that it doesn't do
> > > > the `always inline' thing?
> > > > 
> > > > An explicit usage of __always_inline is semantically different than
> > > > boring old `inline'.
> > 
> > Excuse me if I'm being ignorant, but I thought always_inline was
> > introduced because with some recent versions of gcc, inline wasn't doing
> > the job (suspend2, which requires a working inline, was broken by it for
> > example).
> 
> IIRC, the compiler was generating out-of-line versions of functions in
> every compilation unit whcih included the header file.  When we the
> developers just wanted `inline' to mean `inline, dammit'.

`inline, dammit' is
  __attribute__((always_inline))

The original `inline' never guarantees that the function will be 
inlined.

Therefore, `inline' is already #define'd in the Linux kernel to always 
be __attribute__((always_inline)).

> If that broke swsusp in some manner then the relevant swsusp functions
> should be marked always_inline, because they have some special needs.
> 
> > That is to say, doesn't the definition of always_inline vary
> > with the compiler version?
> 
> If the compiler supports attribute((always_inline)) then the kernel build
> system will use that.  If the compiler doesn't support
> attribute((always_inline)) then we just emit `inline' from cpp and hope
> that it works out.

That's exactly how `inline' is already #define'd in the Linux kernel.

And __always_inline is currently simply #define'd to `inline' ...

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

Re: [2.6 patch] kill __always_inline

[Tim Bird]

Adrian Bunk wrote:
>>>>>>The patch below removes __always_inline again:
>>>>>

>>If the compiler supports attribute((always_inline)) then the kernel build
>>system will use that.  If the compiler doesn't support
>>attribute((always_inline)) then we just emit `inline' from cpp and hope
>>that it works out.
> 
> 
> That's exactly how `inline' is already #define'd in the Linux kernel.
> 
> And __always_inline is currently simply #define'd to `inline' ...

Sorry, but the way this is currently done in the Linux kernel is, IMHO,
braindead.

I ran into a lot of problems with inline handling while I was working
on a piece of instrumentation code that used gcc's -finstrument-functions.
I found that the kernel redefinitions for inline were not being universally
applied (see the patch below for how I fixed it for my situation). I found
that the compiler version affected whether I really got an
attribute(always_inline) or not.  Also, for some uses, the include order
(based on configuration options) affected whether I got the redefinition.

I didn't do a full survey of all inline uses.  But the errors I found
with this made me nervous.

IMHO, a better approach, if it really is desired to force always_inline,
would be to globally replace 'inline' (and friends) with kern_inline
or something similar, so you at least get a compiler error if you're
picking up the traditional gcc semantics instead of the kernel-mandated
semantics.

But my preference would be to do as Andi Kleen has suggested, which
is to revert to the use of (un-redefined) 'inline' for optional inlines,
and use '__always_inline' for those special cases that REALLY need to be
inlines.  Most (but probably not all) of these are marked with 'extern
inline' in the code today. (Unfortunately, there are a few optional
ones marked with 'extern inline' as well - which complicates things).
I realize this is riskier, but it seems to make more sense in the
long run.

Finally, I think it's bad form to change the meaning of a compiler
keyword.  It misleading for 'inline' to mean something different
in the kernel than it does everywhere else.  It means a developer
can't use standard gcc documentation to understand kernel code, without
inside knowledge.  This can be painful for casual or new kernel
developers.

Regards,
   -- Tim

diff -ruN -X ../../dontdiff linux-2.6.7/include/asm-ppc/delay.h branch_KFI/include/asm-ppc/delay.h
--- linux-2.6.7/include/asm-ppc/delay.h	2004-06-15 22:19:42.000000000 -0700
+++ branch_KFI/include/asm-ppc/delay.h	2004-08-11 15:30:22.000000000 -0700
@@ -2,6 +2,7 @@
  #ifndef _PPC_DELAY_H
  #define _PPC_DELAY_H

+#include <linux/compiler.h>	/* for inline weirdness */
  #include <asm/param.h>

  /*
diff -ruN -X ../../dontdiff linux-2.6.7/include/asm-ppc/processor.h branch_KFI/include/asm-ppc/processor.h
--- linux-2.6.7/include/asm-ppc/processor.h	2004-06-15 22:18:38.000000000 -0700
+++ branch_KFI/include/asm-ppc/processor.h	2004-08-11 15:35:25.000000000 -0700
@@ -10,6 +10,7 @@

  #include <linux/config.h>
  #include <linux/stringify.h>
+#include <linux/compiler.h>	/* for inline weirdness */

  #include <asm/ptrace.h>
  #include <asm/types.h>
diff -ruN -X ../../dontdiff linux-2.6.7/include/linux/compiler-gcc3.h branch_KFI/include/linux/compiler-gcc3.h
--- linux-2.6.7/include/linux/compiler-gcc3.h	2004-06-15 22:18:45.000000000 -0700
+++ branch_KFI/include/linux/compiler-gcc3.h	2004-08-11 14:16:34.000000000 -0700
@@ -3,7 +3,7 @@
  /* These definitions are for GCC v3.x.  */
  #include <linux/compiler-gcc.h>

-#if __GNUC_MINOR__ >= 1  && __GNUC_MINOR__ < 4
+#if __GNUC_MINOR__ >= 1
  # define inline		__inline__ __attribute__((always_inline))
  # define __inline__	__inline__ __attribute__((always_inline))
  # define __inline	__inline__ __attribute__((always_inline))


=============================
Tim Bird
Architecture Group Co-Chair, CE Linux Forum
Senior Staff Engineer, Sony Electronics
E-mail: tim.bird@am.sony.com
=============================

Re: [2.6 patch] kill __always_inline

[Andrew Morton]

Tim Bird <tim.bird@am.sony.com> wrote:
>
> Finally, I think it's bad form to change the meaning of a compiler
> keyword.  It misleading for 'inline' to mean something different
> in the kernel than it does everywhere else.  It means a developer
> can't use standard gcc documentation to understand kernel code, without
> inside knowledge.  This can be painful for casual or new kernel
> developers.

yes, it's horrid.  But until and unless gcc gets fixed, what choice do we
have?  Without this hack, kernel text size increases significantly.

We don't want to have to patch the kernel source in a zillion places for
what is hopefully a temporary problem.  I'd much rather add `--dwim-dammit'
to the gcc command line.

> +#include <linux/compiler.h>	/* for inline weirdness */
>   #include <asm/param.h>

Mozilla white-space-stuffed your patch.  You'll have to use attachments.