[2.6 patch] acpi/battery.c: make 2 functions static

[2.6 patch] acpi/battery.c: make 2 functions static

[Adrian Bunk]

This patch makes the following functions static instead of global inline:
- acpi_battery_present()
- acpi_battery_units()

Signed-off-by: Adrian Bunk <bunk@kernel.org>

---

 drivers/acpi/battery.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

9cfc2b0a3162d8e2f23537faefb6937c97513f38 foobar
diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
index f6215e8..d941c5a 100644
--- a/drivers/acpi/battery.c
+++ b/drivers/acpi/battery.c
@@ -116,7 +116,7 @@ struct acpi_battery {
 
 #define to_acpi_battery(x) container_of(x, struct acpi_battery, bat);
 
-inline int acpi_battery_present(struct acpi_battery *battery)
+static int acpi_battery_present(struct acpi_battery *battery)
 {
 	return battery->device->status.battery_present;
 }
@@ -235,7 +235,7 @@ static enum power_supply_property energy_battery_props[] = {
 #endif
 
 #ifdef CONFIG_ACPI_PROCFS_POWER
-inline char *acpi_battery_units(struct acpi_battery *battery)
+static char *acpi_battery_units(struct acpi_battery *battery)
 {
 	return (battery->power_unit)?"mA":"mW";
 }

Re: [2.6 patch] acpi/battery.c: make 2 functions static

[Alexey Starikovskiy]

May I keep them inline?

Thanks,
Alex.
Adrian Bunk wrote:
> This patch makes the following functions static instead of global inline:
> - acpi_battery_present()
> - acpi_battery_units()
>
> Signed-off-by: Adrian Bunk <bunk@kernel.org>
>
> ---
>
>  drivers/acpi/battery.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> 9cfc2b0a3162d8e2f23537faefb6937c97513f38 foobar
> diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
> index f6215e8..d941c5a 100644
> --- a/drivers/acpi/battery.c
> +++ b/drivers/acpi/battery.c
> @@ -116,7 +116,7 @@ struct acpi_battery {
>  
>  #define to_acpi_battery(x) container_of(x, struct acpi_battery, bat);
>  
> -inline int acpi_battery_present(struct acpi_battery *battery)
> +static int acpi_battery_present(struct acpi_battery *battery)
>  {
>  	return battery->device->status.battery_present;
>  }
> @@ -235,7 +235,7 @@ static enum power_supply_property energy_battery_props[] = {
>  #endif
>  
>  #ifdef CONFIG_ACPI_PROCFS_POWER
> -inline char *acpi_battery_units(struct acpi_battery *battery)
> +static char *acpi_battery_units(struct acpi_battery *battery)
>  {
>  	return (battery->power_unit)?"mA":"mW";
>  }
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>   

Re: [2.6 patch] acpi/battery.c: make 2 functions static

[Adrian Bunk]

On Sat, Mar 01, 2008 at 09:26:41PM +0300, Alexey Starikovskiy wrote:
> May I keep them inline?

The problem with such manual inlines is that we force gcc to always 
inline them - and history has shown that functions grow without the 
"inline" being removed.

And for static functions gcc should (at least in theory) know best when 
to inline them.

> Thanks,
> Alex.
> Adrian Bunk wrote:
>> This patch makes the following functions static instead of global inline:
>> - acpi_battery_present()
>> - acpi_battery_units()
>>
>> Signed-off-by: Adrian Bunk <bunk@kernel.org>
>>
>> ---
>>
>>  drivers/acpi/battery.c |    4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> 9cfc2b0a3162d8e2f23537faefb6937c97513f38 foobar
>> diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
>> index f6215e8..d941c5a 100644
>> --- a/drivers/acpi/battery.c
>> +++ b/drivers/acpi/battery.c
>> @@ -116,7 +116,7 @@ struct acpi_battery {
>>   #define to_acpi_battery(x) container_of(x, struct acpi_battery, bat);
>>  -inline int acpi_battery_present(struct acpi_battery *battery)
>> +static int acpi_battery_present(struct acpi_battery *battery)
>>  {
>>  	return battery->device->status.battery_present;
>>  }
>> @@ -235,7 +235,7 @@ static enum power_supply_property energy_battery_props[] = {
>>  #endif
>>   #ifdef CONFIG_ACPI_PROCFS_POWER
>> -inline char *acpi_battery_units(struct acpi_battery *battery)
>> +static char *acpi_battery_units(struct acpi_battery *battery)
>>  {
>>  	return (battery->power_unit)?"mA":"mW";
>>  }

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] acpi/battery.c: make 2 functions static

[Alexey Starikovskiy]

Adrian Bunk wrote:
> On Sat, Mar 01, 2008 at 09:26:41PM +0300, Alexey Starikovskiy wrote:
>   
>> May I keep them inline?
>>     
>
> The problem with such manual inlines is that we force gcc to always 
> inline them - and history has shown that functions grow without the 
> "inline" being removed.
>
>   
> And for static functions gcc should (at least in theory) know best when 
> to inline them.
>
>   
Right. Acked-by: Alexey Starikovskiy <astarikovskiy@suse.de>

As I understand, you have some automation tools for finding out these
non-static functions, etc; are they available somewhere?

Regards,
Alex.
>> Thanks,
>> Alex.
>> Adrian Bunk wrote:
>>     
>>> This patch makes the following functions static instead of global inline:
>>> - acpi_battery_present()
>>> - acpi_battery_units()
>>>
>>> Signed-off-by: Adrian Bunk <bunk@kernel.org>
>>>
>>> ---
>>>
>>>  drivers/acpi/battery.c |    4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> 9cfc2b0a3162d8e2f23537faefb6937c97513f38 foobar
>>> diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
>>> index f6215e8..d941c5a 100644
>>> --- a/drivers/acpi/battery.c
>>> +++ b/drivers/acpi/battery.c
>>> @@ -116,7 +116,7 @@ struct acpi_battery {
>>>   #define to_acpi_battery(x) container_of(x, struct acpi_battery, bat);
>>>  -inline int acpi_battery_present(struct acpi_battery *battery)
>>> +static int acpi_battery_present(struct acpi_battery *battery)
>>>  {
>>>  	return battery->device->status.battery_present;
>>>  }
>>> @@ -235,7 +235,7 @@ static enum power_supply_property energy_battery_props[] = {
>>>  #endif
>>>   #ifdef CONFIG_ACPI_PROCFS_POWER
>>> -inline char *acpi_battery_units(struct acpi_battery *battery)
>>> +static char *acpi_battery_units(struct acpi_battery *battery)
>>>  {
>>>  	return (battery->power_unit)?"mA":"mW";
>>>  }
>>>       
>
> cu
> Adrian
>
>   

Re: [2.6 patch] acpi/battery.c: make 2 functions static

[Adrian Bunk]

On Sat, Mar 01, 2008 at 09:42:59PM +0300, Alexey Starikovskiy wrote:
> Adrian Bunk wrote:
>> On Sat, Mar 01, 2008 at 09:26:41PM +0300, Alexey Starikovskiy wrote:
>>   
>>> May I keep them inline?
>>>     
>>
>> The problem with such manual inlines is that we force gcc to always  
>> inline them - and history has shown that functions grow without the  
>> "inline" being removed.
>>
>>   And for static functions gcc should (at least in theory) know best 
>> when to inline them.
>>
>>   
> Right. Acked-by: Alexey Starikovskiy <astarikovskiy@suse.de>
>
> As I understand, you have some automation tools for finding out these
> non-static functions, etc; are they available somewhere?

Type "make namespacecheck" after compiling the kernel.  

> Regards,
> Alex.

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] acpi/battery.c: make 2 functions static

[Ingo Molnar]

* Adrian Bunk <bunk@kernel.org> wrote:

> On Sat, Mar 01, 2008 at 09:26:41PM +0300, Alexey Starikovskiy wrote:
> > May I keep them inline?
> 
> The problem with such manual inlines is that we force gcc to always 
> inline them - and history has shown that functions grow without the 
> "inline" being removed.

what do you mean by "we force gcc to always inline them"? gcc is free to 
decide whether to inline or to not inline. (and CONFIG_FORCED_INLINING 
got removed from 2.6.25)

	Ingo

Re: [2.6 patch] acpi/battery.c: make 2 functions static

[Adrian Bunk]

On Mon, Mar 03, 2008 at 09:57:20AM +0100, Ingo Molnar wrote:
> 
> * Adrian Bunk <bunk@kernel.org> wrote:
> 
> > On Sat, Mar 01, 2008 at 09:26:41PM +0300, Alexey Starikovskiy wrote:
> > > May I keep them inline?
> > 
> > The problem with such manual inlines is that we force gcc to always 
> > inline them - and history has shown that functions grow without the 
> > "inline" being removed.
> 
> what do you mean by "we force gcc to always inline them"?

#define inline          inline          __attribute__((always_inline))

> gcc is free to decide whether to inline or to not inline.

Not with __attribute__((always_inline)).

> (and CONFIG_FORCED_INLINING got removed from 2.6.25)

CONFIG_FORCED_INLINING never had any effect.

> 	Ingo

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] acpi/battery.c: make 2 functions static

[Ingo Molnar]

* Adrian Bunk <bunk@kernel.org> wrote:

> On Mon, Mar 03, 2008 at 09:57:20AM +0100, Ingo Molnar wrote:
> > 
> > * Adrian Bunk <bunk@kernel.org> wrote:
> > 
> > > On Sat, Mar 01, 2008 at 09:26:41PM +0300, Alexey Starikovskiy wrote:
> > > > May I keep them inline?
> > > 
> > > The problem with such manual inlines is that we force gcc to always 
> > > inline them - and history has shown that functions grow without the 
> > > "inline" being removed.
> > 
> > what do you mean by "we force gcc to always inline them"?
> 
> #define inline          inline          __attribute__((always_inline))
> 
> > gcc is free to decide whether to inline or to not inline.
> 
> Not with __attribute__((always_inline)).

but that wasnt used in the code you patched:

  -inline int acpi_battery_present(struct acpi_battery *battery)
  +static int acpi_battery_present(struct acpi_battery *battery)

> > (and CONFIG_FORCED_INLINING got removed from 2.6.25)
> 
> CONFIG_FORCED_INLINING never had any effect.

my experience was that it had effects. Why do you say it 'never had any 
effect'?

	Ingo

Re: [2.6 patch] acpi/battery.c: make 2 functions static

[Sam Ravnborg]

On Mon, Mar 03, 2008 at 10:17:14AM +0100, Ingo Molnar wrote:
> 
> * Adrian Bunk <bunk@kernel.org> wrote:
> 
> > On Mon, Mar 03, 2008 at 09:57:20AM +0100, Ingo Molnar wrote:
> > > 
> > > * Adrian Bunk <bunk@kernel.org> wrote:
> > > 
> > > > On Sat, Mar 01, 2008 at 09:26:41PM +0300, Alexey Starikovskiy wrote:
> > > > > May I keep them inline?
> > > > 
> > > > The problem with such manual inlines is that we force gcc to always 
> > > > inline them - and history has shown that functions grow without the 
> > > > "inline" being removed.
> > > 
> > > what do you mean by "we force gcc to always inline them"?
> > 
> > #define inline          inline          __attribute__((always_inline))
> > 
> > > gcc is free to decide whether to inline or to not inline.
> > 
> > Not with __attribute__((always_inline)).
> 
> but that wasnt used in the code you patched:
> 
>   -inline int acpi_battery_present(struct acpi_battery *battery)
>   +static int acpi_battery_present(struct acpi_battery *battery)

>From compiler-gcc.h:

#define inline          inline          __attribute__((always_inline))

So unless I am missing something obvious then each time we
say inline to a function we require gcc to inline the function.

It is my impression that today we only say inline if really needed
and otherwise let gcc decide. So in almost all cases inlise should
just be nuked?

	Sam

Re: [2.6 patch] acpi/battery.c: make 2 functions static

[Adrian Bunk]

On Mon, Mar 03, 2008 at 10:31:03AM +0100, Sam Ravnborg wrote:
> On Mon, Mar 03, 2008 at 10:17:14AM +0100, Ingo Molnar wrote:
> > 
> > * Adrian Bunk <bunk@kernel.org> wrote:
> > 
> > > On Mon, Mar 03, 2008 at 09:57:20AM +0100, Ingo Molnar wrote:
> > > > 
> > > > * Adrian Bunk <bunk@kernel.org> wrote:
> > > > 
> > > > > On Sat, Mar 01, 2008 at 09:26:41PM +0300, Alexey Starikovskiy wrote:
> > > > > > May I keep them inline?
> > > > > 
> > > > > The problem with such manual inlines is that we force gcc to always 
> > > > > inline them - and history has shown that functions grow without the 
> > > > > "inline" being removed.
> > > > 
> > > > what do you mean by "we force gcc to always inline them"?
> > > 
> > > #define inline          inline          __attribute__((always_inline))
> > > 
> > > > gcc is free to decide whether to inline or to not inline.
> > > 
> > > Not with __attribute__((always_inline)).
> > 
> > but that wasnt used in the code you patched:
> > 
> >   -inline int acpi_battery_present(struct acpi_battery *battery)
> >   +static int acpi_battery_present(struct acpi_battery *battery)
> 
> >From compiler-gcc.h:
> 
> #define inline          inline          __attribute__((always_inline))
> 
> So unless I am missing something obvious then each time we
> say inline to a function we require gcc to inline the function.

Exactly.

> It is my impression that today we only say inline if really needed
> and otherwise let gcc decide. So in almost all cases inlise should
> just be nuked?

The rule is:
- all static functions in headers should be marked inline
- no functions in .c files should be marked inline

For the latter there are a _few_ exceptions in hotpaths where doing so 
brings measurable advantages.

But generally (and especially long-term) it's best to let gcc decide 
which static functions in .c files should be inlined.

> 	Sam

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] acpi/battery.c: make 2 functions static

[Ingo Molnar]

* Sam Ravnborg <sam@ravnborg.org> wrote:

> >From compiler-gcc.h:
> >
> > #define inline          inline          __attribute__((always_inline))
> 
> So unless I am missing something obvious then each time we say inline 
> to a function we require gcc to inline the function.
> 
> It is my impression that today we only say inline if really needed and 
> otherwise let gcc decide. So in almost all cases inlise should just be 
> nuked?

no, what we should nuke is this always_inline definition. That was 
always the intention of FORCED_INLINE, and the removal of FORCED_INLINE 
was to _remove the forcing_, not to make it unconditional.

so Adrian, if you knew about this bug all along, you might as well have 
reported it :-/

	Ingo

Re: [2.6 patch] acpi/battery.c: make 2 functions static

[Adrian Bunk]

On Mon, Mar 03, 2008 at 11:39:33AM +0100, Ingo Molnar wrote:
> 
> * Sam Ravnborg <sam@ravnborg.org> wrote:
> 
> > >From compiler-gcc.h:
> > >
> > > #define inline          inline          __attribute__((always_inline))
> > 
> > So unless I am missing something obvious then each time we say inline 
> > to a function we require gcc to inline the function.
> > 
> > It is my impression that today we only say inline if really needed and 
> > otherwise let gcc decide. So in almost all cases inlise should just be 
> > nuked?
> 
> no, what we should nuke is this always_inline definition. That was 
> always the intention of FORCED_INLINE, and the removal of FORCED_INLINE 
> was to _remove the forcing_, not to make it unconditional.

It was always unconditional, and neither adding, toggling nor removing 
of CONFIG_FORCED_INLINING changed this invariant.

And what we should do is to attack the excessive wrong usage of inlines 
in .c files, not messing with a global #define in a way that the results 
on 24 architectures with 7 different releases of gcc would be unpredictable.

> so Adrian, if you knew about this bug all along, you might as well have 
> reported it :-/

http://lkml.org/lkml/2007/1/19/36
http://lkml.org/lkml/2007/4/9/363

are the result of a quick Google search of me stating this previously on 
linux-kernel. It might have been more often, but I'm too lame too 
search further.

> 	Ingo

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] acpi/battery.c: make 2 functions static

[Ingo Molnar]

* Adrian Bunk <bunk@kernel.org> wrote:

> are the result of a quick Google search of me stating this previously on 
> linux-kernel. It might have been more often, but I'm too lame too 
> search further.
> 
> http://lkml.org/lkml/2007/1/19/36

that's a side-note, not a bugreport and not a patch to fix it.

> http://lkml.org/lkml/2007/4/9/363

this second one is this very thread that i replied to.

> > no, what we should nuke is this always_inline definition. That was 
> > always the intention of FORCED_INLINE, and the removal of 
> > FORCED_INLINE was to _remove the forcing_, not to make it 
> > unconditional.
> 
> It was always unconditional, and neither adding, toggling nor removing 
> of CONFIG_FORCED_INLINING changed this invariant.
> 
> And what we should do is to attack the excessive wrong usage of 
> inlines in .c files, not messing with a global #define in a way that 
> the results on 24 architectures with 7 different releases of gcc would 
> be unpredictable.

i see, so you never properly reported and fixed it because you prefer a 
1000 small crappy changes over one change. You could have significantly 
contributed to truly making Linux smaller, but you decided not to do it.

and i disagree with your notion that flipping it around is risky in any 
unacceptable or unmanageable way. It has far less risks on the compiler 
than say CONFIG_CC_OPTIMIZE_FOR_SIZE. It has far less risks than 
changing to a new compiler version. Why you think it's "unpredictable" 
is a mystery to me.

It almost seems to me you were happy with having that bug in the kernel? 
Please tell me that i'm wrong about that impression!

i'll reinstate this .config option and let it do the right thing. Forced 
inlining was supposed to be _phased out_, not phased in.

	Ingo

Re: [2.6 patch] acpi/battery.c: make 2 functions static

[Adrian Bunk]

On Mon, Mar 03, 2008 at 12:45:34PM +0100, Ingo Molnar wrote:
> 
> * Adrian Bunk <bunk@kernel.org> wrote:
> 
> > are the result of a quick Google search of me stating this previously on 
> > linux-kernel. It might have been more often, but I'm too lame too 
> > search further.
> > 
> > http://lkml.org/lkml/2007/1/19/36
> 
> that's a side-note, not a bugreport and not a patch to fix it.
> 
> > http://lkml.org/lkml/2007/4/9/363
> 
> this second one is this very thread that i replied to.
>...


That's a lie.


> 	Ingo

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] acpi/battery.c: make 2 functions static

[Ingo Molnar]

* Adrian Bunk <bunk@kernel.org> wrote:

> > > http://lkml.org/lkml/2007/4/9/363
> > 
> > this second one is this very thread that i replied to.
> >...
> 
> That's a lie.

oops, i was simply wrong - it looked very much like this current thread 
...

(It would be a lie had i been writing that intentinally. Thanks for 
giving me the benefit of the doubt.)

	Ingo

Re: [2.6 patch] acpi/battery.c: make 2 functions static

[Adrian Bunk]

On Mon, Mar 03, 2008 at 01:10:15PM +0100, Ingo Molnar wrote:
> 
> * Adrian Bunk <bunk@kernel.org> wrote:
> 
> > > > http://lkml.org/lkml/2007/4/9/363
> > > 
> > > this second one is this very thread that i replied to.
> > >...
> > 
> > That's a lie.
> 
> oops, i was simply wrong - it looked very much like this current thread 
> ...
> 
> (It would be a lie had i been writing that intentinally. Thanks for 
> giving me the benefit of the doubt.)

Thanks for assuming only the best intentions from me.

I can only repeat that I did state several times on linux-kernel that it 
never worked.

If you consider it my fault that noone reads my emails then you are 
right that it's my fault...

> 	Ingo

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] acpi/battery.c: make 2 functions static

[Ingo Molnar]

* Adrian Bunk <bunk@kernel.org> wrote:

> I can only repeat that I did state several times on linux-kernel that 
> it never worked.
> 
> If you consider it my fault that noone reads my emails then you are 
> right that it's my fault...

well, i'm trying to assume the best, so please explain the following 
sequence of events to me:

1) as you said you knew about this bug - which bug causes more inlining
   overhead than hundreds of your uninlining patches combined. The bug
   was introduced ~2 years ago in -mm - before the feature hit mainline
   in v2.6.16.

2) the fix was really trivial and the intention of the feature was well 
   understood - but the feature stayed as a NOP in the upstream kernel 
   for 2 years.

still, while you clearly had interest in this general area of the kernel 
(for example you wrote hundreds of tiny uninlining patches that work 
towards a similar goal), but strangely at the same time you neither 
fixed, nor properly escallated this _far_ bigger bug that causes +2.3% 
of text bloat on x86 [more than 120K of kernel text]. In fact:

- you created bugzillas for far smaller bugs in the past, but you never
  created a bugzilla for this that i'm aware of.

- you never directly raised this issue with us: "look guys, this thing
  really is broken - please reply to me with a fix".

- you never said "this is a regression that should be fixed" to any of
  the regression lists.

in other words: for about two years you knew about a bug that should 
have been fixed the day after it got introduced.

i obviously cannot know what your intentions were with this conduct, so 
i'm eagerly awaiting your explanation for it. Thanks,

	Ingo

Re: [2.6 patch] acpi/battery.c: make 2 functions static

[Adrian Bunk]

On Mon, Mar 03, 2008 at 01:50:27PM +0100, Ingo Molnar wrote:
> 
> * Adrian Bunk <bunk@kernel.org> wrote:
> 
> > I can only repeat that I did state several times on linux-kernel that 
> > it never worked.
> > 
> > If you consider it my fault that noone reads my emails then you are 
> > right that it's my fault...
> 
> well, i'm trying to assume the best, so please explain the following 
> sequence of events to me:
> 
> 1) as you said you knew about this bug - which bug causes more inlining
>    overhead than hundreds of your uninlining patches combined. The bug
>    was introduced ~2 years ago in -mm - before the feature hit mainline
>    in v2.6.16.

I don't remember having ever said this.

Your choices are:
[ ] prove your accusation that I said I
    "knew about this bug before the feature hit mainline"
[ ] apologize
[ ] be the firest person ever in my killfile

>...
> still, while you clearly had interest in this general area of the kernel 
> (for example you wrote hundreds of tiny uninlining patches that work 
> towards a similar goal),

I'm not sure with whom you confuse me on this one.
Perhaps with Ilpo?

I have sending some bigger uninlining patches on my TODO list for quite 
some time (since this is really the right thing to solve these issues), 
but I've not yet gotten there.

> but strangely at the same time you neither 
> fixed, nor properly escallated this _far_ bigger bug that causes +2.3% 
> of text bloat on x86 [more than 120K of kernel text]. In fact:
> 
> - you created bugzillas for far smaller bugs in the past, but you never
>   created a bugzilla for this that i'm aware of.

I'm well aware of the fact that our Bugzilla is mostly a /dev/null for 
the currently at about 1500 open bugs there.

That's why I tend [1] to only open bugs there for getting them into 
Rafael's regression lists.

>...
> - you never said "this is a regression that should be fixed" to any of
>   the regression lists.

This isn't a regression.

>...
> 	Ingo

cu
Adrian

[1] there are a few exceptions when I tried opening some bugs there

-- 

       "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] acpi/battery.c: make 2 functions static

[Adrian Bunk]

On Mon, Mar 03, 2008 at 04:54:13PM +0200, Adrian Bunk wrote:
> On Mon, Mar 03, 2008 at 01:50:27PM +0100, Ingo Molnar wrote:
> > 
> > * Adrian Bunk <bunk@kernel.org> wrote:
> > 
> > > I can only repeat that I did state several times on linux-kernel that 
> > > it never worked.
> > > 
> > > If you consider it my fault that noone reads my emails then you are 
> > > right that it's my fault...
> > 
> > well, i'm trying to assume the best, so please explain the following 
> > sequence of events to me:
> > 
> > 1) as you said you knew about this bug - which bug causes more inlining
> >    overhead than hundreds of your uninlining patches combined. The bug
> >    was introduced ~2 years ago in -mm - before the feature hit mainline
> >    in v2.6.16.
> 
> I don't remember having ever said this.
> 
> Your choices are:
> [ ] prove your accusation that I said I
>     "knew about this bug before the feature hit mainline"
> [ ] apologize
> [ ] be the firest person ever in my killfile
>...

s/firest/first/

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] acpi/battery.c: make 2 functions static

[Ingo Molnar]

* Adrian Bunk <bunk@stusta.de> wrote:

> > well, i'm trying to assume the best, so please explain the following 
> > sequence of events to me:
> > 
> > 1) as you said you knew about this bug - which bug causes more inlining
> >    overhead than hundreds of your uninlining patches combined. The bug
> >    was introduced ~2 years ago in -mm - before the feature hit mainline
> >    in v2.6.16.
> 
> I don't remember having ever said this.
> 
> Your choices are:
> [ ] prove your accusation that I said I
>     "knew about this bug before the feature hit mainline"
> [ ] apologize
> [ ] be the firest person ever in my killfile

Adrian, you must be misunderstanding something. Where exactly in the 
above sentences do i assert that you "knew about this bug before the 
feature hit mainline"? I dont say that and cannot say that - and it's 
rather irrelevant. All i say is that you knew about this bug for a long 
time.

> >...
> > still, while you clearly had interest in this general area of the kernel 
> > (for example you wrote hundreds of tiny uninlining patches that work 
> > towards a similar goal),
> 
> I'm not sure with whom you confuse me on this one.
> Perhaps with Ilpo?

i mean you send tons of trivial patches along the lines of:

|  Author: Adrian Bunk <bunk@kernel.org>
|  Date:   Fri Feb 22 21:58:37 2008 +0200
|
|    x86: don't make swapper_pg_pmd global

to reduce size of the kernel. At 50 bytes of image savings a pop, the 
127,000 bytes savings my patch gives would be equivalent to more than 
2500 of such patches of yours.

In other words: you knew about a bug that would have the same kernel 
image size reduction equivalent to 2500 of your own tiny patches. Still 
you didnt feel the need to pursue the issue?

I'm not sure about you, but that sure looks weird to me, and i'm really 
curious what your interpretation of it is. (which, AFAICS, you have not 
offered so far. You have rejected my common-sense explanation of your 
actions rather forcefully, so logically there must be some other 
explanation.)

	Ingo

Re: [2.6 patch] acpi/battery.c: make 2 functions static

[Adrian Bunk]

On Tue, Mar 04, 2008 at 02:16:00PM +0100, Ingo Molnar wrote:
> 
> * Adrian Bunk <bunk@stusta.de> wrote:
> 
> > > well, i'm trying to assume the best, so please explain the following 
> > > sequence of events to me:
> > > 
> > > 1) as you said you knew about this bug - which bug causes more inlining
> > >    overhead than hundreds of your uninlining patches combined. The bug
> > >    was introduced ~2 years ago in -mm - before the feature hit mainline
> > >    in v2.6.16.
> > 
> > I don't remember having ever said this.
> > 
> > Your choices are:
> > [ ] prove your accusation that I said I
> >     "knew about this bug before the feature hit mainline"
> > [ ] apologize
> > [ ] be the firest person ever in my killfile
> 
> Adrian, you must be misunderstanding something. Where exactly in the 
> above sentences do i assert that you "knew about this bug before the 
> feature hit mainline"? I dont say that and cannot say that -

Please explain your statement "before the feature hit mainline in 
v2.6.16" in the above sentence of you in a reasonable way other than 
that it should say I knew about it before the feature hit mainline.

> and it's 
> rather irrelevant.

Perhaps for you.

For me it's not irrelevant what I'm publically being accused of.

And it's not the first time in the thread that you use things that are 
objectively not true in accusations against me.

> All i say is that you knew about this bug for a long 
> time.
>...

I found the bug.

I repeatedly told about this bug on linux-kernel.

I explained to you who claimed just yesterday "my experience was that 
it had effects." why it couldn't have possibly worked (no matter why 
your experience was differently).

And now you start big flames against me why I hadn't stated more boldly 
that this patch never worked.
Not against Arjan who seems to have sent a patch that never worked.
Not against Harvey who independently spotted the same bug.
Not against the people who didn't care when I said on linux-kernel that 
it didn't work (one of the threads even had CONFIG_FORCED_INLINING in 
the subject).

Thanks a lot, Mr. Molnar.

> 	Ingo

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] acpi/battery.c: make 2 functions static

[Ingo Molnar]

* Adrian Bunk <bunk@kernel.org> wrote:

> On Tue, Mar 04, 2008 at 02:16:00PM +0100, Ingo Molnar wrote:
> > 
> > * Adrian Bunk <bunk@stusta.de> wrote:
> > 
> > > > well, i'm trying to assume the best, so please explain the following 
> > > > sequence of events to me:
> > > > 
> > > > 1) as you said you knew about this bug - which bug causes more inlining
> > > >    overhead than hundreds of your uninlining patches combined. The bug
> > > >    was introduced ~2 years ago in -mm - before the feature hit mainline
> > > >    in v2.6.16.
> > > 
> > > I don't remember having ever said this.
> > > 
> > > Your choices are:
> > > [ ] prove your accusation that I said I
> > >     "knew about this bug before the feature hit mainline"
> > > [ ] apologize
> > > [ ] be the firest person ever in my killfile
> > 
> > Adrian, you must be misunderstanding something. Where exactly in the 
> > above sentences do i assert that you "knew about this bug before the 
> > feature hit mainline"? I dont say that and cannot say that -
> 
> Please explain your statement "before the feature hit mainline in 
> v2.6.16" in the above sentence of you in a reasonable way other than 
> that it should say I knew about it before the feature hit mainline.

do you mean this paragraph:

| 1) as you said you knew about this bug - which bug causes more 
| inlining overhead than hundreds of your uninlining patches combined. 
| The bug was introduced ~2 years ago in -mm - before the feature hit 
| mainline in v2.6.16.

sorry, but i know of no rule of grammar that could read your 
interpretation into my two sentences.

(and that's not surprising at all, because i never intended to even 
suggest that you knew about this breakage "before it went mainline" - 
why would i even care about such a detail?)

	Ingo

Re: [2.6 patch] acpi/battery.c: make 2 functions static

[Jörn Engel]

On Tue, 4 March 2008 15:22:48 +0100, Ingo Molnar wrote:
> 
> do you mean this paragraph:

Could you two please refrain from sending any more of this?  Who said
what when and where doesn't help anyone, not even either of your ego's
or hurt feelings.

A huge paperbag bug was found and I'm happy it's gone.  It could and
should have been fixed long ago.  As for personal motives, I just
couldn't care less.  And this thread has devolved way beyond a
metadiscussion about motives.

Jörn

-- 
Debugging is twice as hard as writing the code in the first place.
Therefore, if you write the code as cleverly as possible, you are,
by definition, not smart enough to debug it.
-- Brian W. Kernighan
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [2.6 patch] acpi/battery.c: make 2 functions static

[Ingo Molnar]

* Jörn Engel <joern@logfs.org> wrote:

> On Tue, 4 March 2008 15:22:48 +0100, Ingo Molnar wrote:
> > 
> > do you mean this paragraph:
> 
> Could you two please refrain from sending any more of this?  Who said 
> what when and where doesn't help anyone, not even either of your ego's 
> or hurt feelings.

well, there's no hurt feelings on my side. I'm not sure about you, but i 
tend to reply when i get accused of lying:

   http://lkml.org/lkml/2008/3/3/119

at least up to a certain point. Which point we reached right now, so i'm 
zapping this thread now ;-)

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[patch] x86: phase out forced inlining

[Ingo Molnar]

* Ingo Molnar <mingo@elte.hu> wrote:

> > > no, what we should nuke is this always_inline definition. That was 
> > > always the intention of FORCED_INLINE, and the removal of 
> > > FORCED_INLINE was to _remove the forcing_, not to make it 
> > > unconditional.
> > 
> > It was always unconditional, and neither adding, toggling nor 
> > removing of CONFIG_FORCED_INLINING changed this invariant.
> > 
> > And what we should do is to attack the excessive wrong usage of 
> > inlines in .c files, not messing with a global #define in a way that 
> > the results on 24 architectures with 7 different releases of gcc 
> > would be unpredictable.
> 
> i see, so you never properly reported and fixed it because you prefer 
> a 1000 small crappy changes over one change. You could have 
> significantly contributed to truly making Linux smaller, but you 
> decided not to do it.
> 
> and i disagree with your notion that flipping it around is risky in 
> any unacceptable or unmanageable way. It has far less risks on the 
> compiler than say CONFIG_CC_OPTIMIZE_FOR_SIZE. It has far less risks 
> than changing to a new compiler version. Why you think it's 
> "unpredictable" is a mystery to me.
> 
> It almost seems to me you were happy with having that bug in the 
> kernel? Please tell me that i'm wrong about that impression!
> 
> i'll reinstate this .config option and let it do the right thing. 
> Forced inlining was supposed to be _phased out_, not phased in.

i just implemented the trivial fix below and it gave me a massive, 2.3% 
text size reduction (!) on a typical .config. That's more than 120K 
shaved off the vmlinux.

Why, despite being aware of this bug, you never fixed this properly is a 
mystery to me - this is more .text size savings than you have done so 
far with all your uninlining patches of the past few years combined, and 
it's probably more than you could have achieved in the next 5 years.

	Ingo

---------------------->
Subject: x86: phase out forced inlining
From: Ingo Molnar <mingo@elte.hu>
Date: Mon Mar 03 12:38:52 CET 2008

allow gcc to optimize the kernel image's size by uninlining
functions that have been marked 'inline'. Previously gcc was
forced by Linux to always-inline these functions via a gcc
attribute:

 #define inline	inline __attribute__((always_inline))

Especially when the user has already selected
CONFIG_OPTIMIZE_FOR_SIZE=y this can make a huge difference in
kernel image size (using a standard Fedora .config):

   text    data     bss     dec           hex filename
   5613924  562708 3854336 10030968    990f78 vmlinux.before
   5486689  562708 3854336  9903733    971e75 vmlinux.after

that's a 2.3% text size reduction (!).

Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 arch/x86/Kconfig.debug            |   13 +++++++++++++
 arch/x86/configs/i386_defconfig   |    1 +
 arch/x86/configs/x86_64_defconfig |    1 +
 include/linux/compiler-gcc.h      |   12 +++++++++---
 4 files changed, 24 insertions(+), 3 deletions(-)

Index: linux-x86.q/arch/x86/Kconfig.debug
===================================================================
--- linux-x86.q.orig/arch/x86/Kconfig.debug
+++ linux-x86.q/arch/x86/Kconfig.debug
@@ -336,3 +336,16 @@ config CPA_DEBUG
 	  Do change_page_attr() self-tests every 30 seconds.
 
 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-x86.q/arch/x86/configs/i386_defconfig
===================================================================
--- linux-x86.q.orig/arch/x86/configs/i386_defconfig
+++ linux-x86.q/arch/x86/configs/i386_defconfig
@@ -1421,6 +1421,7 @@ CONFIG_DEBUG_BUGVERBOSE=y
 # CONFIG_DEBUG_VM is not set
 # CONFIG_DEBUG_LIST is not set
 # CONFIG_FRAME_POINTER is not set
+CONFIG_OPTIMIZE_INLINING=y
 # CONFIG_RCU_TORTURE_TEST is not set
 # CONFIG_LKDTM is not set
 # CONFIG_FAULT_INJECTION is not set
Index: linux-x86.q/arch/x86/configs/x86_64_defconfig
===================================================================
--- linux-x86.q.orig/arch/x86/configs/x86_64_defconfig
+++ linux-x86.q/arch/x86/configs/x86_64_defconfig
@@ -1346,6 +1346,7 @@ CONFIG_DEBUG_BUGVERBOSE=y
 # CONFIG_DEBUG_VM is not set
 # CONFIG_DEBUG_LIST is not set
 # CONFIG_FRAME_POINTER is not set
+CONFIG_OPTIMIZE_INLINING=y
 # CONFIG_RCU_TORTURE_TEST is not set
 # CONFIG_LKDTM is not set
 # CONFIG_FAULT_INJECTION is not set
Index: linux-x86.q/include/linux/compiler-gcc.h
===================================================================
--- linux-x86.q.orig/include/linux/compiler-gcc.h
+++ linux-x86.q/include/linux/compiler-gcc.h
@@ -28,9 +28,15 @@
 #define __must_be_array(a) \
   BUILD_BUG_ON_ZERO(__builtin_types_compatible_p(typeof(a), typeof(&a[0])))
 
-#define inline		inline		__attribute__((always_inline))
-#define __inline__	__inline__	__attribute__((always_inline))
-#define __inline	__inline	__attribute__((always_inline))
+/*
+ * Force always-inline if the user requests it so via the .config:
+ */
+#ifndef CONFIG_OPTIMIZE_INLINING
+# define inline		inline		__attribute__((always_inline))
+# define __inline__	__inline__	__attribute__((always_inline))
+# define __inline	__inline	__attribute__((always_inline))
+#endif
+
 #define __deprecated			__attribute__((deprecated))
 #define __packed			__attribute__((packed))
 #define __weak				__attribute__((weak))

Re: [patch] x86: phase out forced inlining

[Sam Ravnborg]

Hi Ingo.

> Subject: x86: phase out forced inlining

Any particular reason you made the patch x86 specific?

It is preferred to gain this size decrease for other arch too
and having a single definiton of OPTIMIZE_FOR_SIZE
would help here.

We have this for other gcc options already.



> +config OPTIMIZE_INLINING

Other (not all) config options that deal with gcc behaviour are
named CC_*. But they mostly impact gcc options.
CC_OPTIMIZE_INLINING would match the naming of CC_OPTIMIZE_SIZE,
except in the latter OPTIMIZE refer to the -O option.

CC_DEFAULT_INLINE may give the right associations?


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

Would it be worth here to mention that stuff that really
needs inlining should use __always_inle and not inline?

> + */
> +#ifndef CONFIG_OPTIMIZE_INLINING
> +# define inline		inline		__attribute__((always_inline))
> +# define __inline__	__inline__	__attribute__((always_inline))
> +# define __inline	__inline	__attribute__((always_inline))
> +#endif

A quick google did not tell me the difference between inline, __inline, __inline__.
But it turned out the december 2005 thread where there was a lenghty discussion about
trusting gcc with respect to inlining.
It is not the subject of this patch but I just wondered why we need all these variants.

	Sam

Re: [patch] x86: phase out forced inlining

[Ingo Molnar]

* Sam Ravnborg <sam@ravnborg.org> wrote:

> > Subject: x86: phase out forced inlining
> 
> Any particular reason you made the patch x86 specific?

to keep it simple for now. Some of the other 24 architectures are 
seriously under-tested and while we can make sure x86 works well, i dont 
test the others. If it works out fine on x86 it can be generalized.

> > +config OPTIMIZE_INLINING
> 
> Other (not all) config options that deal with gcc behaviour are named 
> CC_*. But they mostly impact gcc options. CC_OPTIMIZE_INLINING would 
> match the naming of CC_OPTIMIZE_SIZE, except in the latter OPTIMIZE 
> refer to the -O option.
> 
> CC_DEFAULT_INLINE may give the right associations?

i really wanted to name it 'optimize' - because that's what it does. We 
just lost 2 years of uninlining advantage due to CONFIG_FORCED_INLINING 
not working and nobody actually connecting the dots that the lack of 
'forced inlining' should have resulted in a 'smaller image' and report 
it as a bug.

> > + test gcc for this.
> 
> Would it be worth here to mention that stuff that really needs 
> inlining should use __always_inle and not inline?

i think people know that, but i'll add it.

> > + */
> > +#ifndef CONFIG_OPTIMIZE_INLINING
> > +# define inline		inline		__attribute__((always_inline))
> > +# define __inline__	__inline__	__attribute__((always_inline))
> > +# define __inline	__inline	__attribute__((always_inline))
> > +#endif
> 
> A quick google did not tell me the difference between inline, 
> __inline, __inline__. But it turned out the december 2005 thread where 
> there was a lenghty discussion about trusting gcc with respect to 
> inlining. It is not the subject of this patch but I just wondered why 
> we need all these variants.

i dont know why they there are so many variants, but all of them seem to 
be used throughout the kernel:

   inline    : 25648
 __inline__  : 1380
 __inline    : 368

so obviously the patch has to cover them.

a few stats about inlines btw:

- in v2.6.24 there were 26452 inlines in the kernel in 8083114 lines of 
  code - or one inline per 305.6 lines of code.

- in v2.6.25-rc3 there are 27396 inlines in the kernel in 8387992 lines 
  of code - or one inline per 308.2 lines of code.

at that rate, all inlines will be removed in about 117.5 kernel cycles - 
which, if we count with 90 day release cycles, will be finished in about 
29 years.

if we only look at include/linux/ files [which have the largest inlining 
effect], the rate of inline removal is in fact negative: in v2.6.24 we 
had one inline per 59.1 lines, in 2.6.25-to-be we have one inline per 
57.9 lines.

so i'm not holding my breath and i'm going for the much more immediate 
benefit of CONFIG_OPTIMIZE_INLINING=y.

	Ingo

Re: [patch] x86: phase out forced inlining

[Harvey Harrison]

On Tue, 2008-03-04 at 17:46 +0100, Ingo Molnar wrote:
> * Sam Ravnborg <sam@ravnborg.org> wrote:

> i dont know why they there are so many variants, but all of them seem to 
> be used throughout the kernel:
> 
>    inline    : 25648

I'll assume this is the preferred way of saying it.

>  __inline__  : 1380

Lots of them in include/asm-*...not sure if there is a reason for this.

>  __inline    : 368

Almost all of them in drivers/scsi

Cheers,

Harvey

Re: [patch] x86: phase out forced inlining

[H. Peter Anvin]

Harvey Harrison wrote:
> On Tue, 2008-03-04 at 17:46 +0100, Ingo Molnar wrote:
>> * Sam Ravnborg <sam@ravnborg.org> wrote:
> 
>> i dont know why they there are so many variants, but all of them seem to 
>> be used throughout the kernel:
>>
>>    inline    : 25648
> 
> I'll assume this is the preferred way of saying it.
> 
>>  __inline__  : 1380
> 
> Lots of them in include/asm-*...not sure if there is a reason for this.
> 

Preferred form for code that's exported to userspace (since gcc 
complains with -ansi -pedantic otherwise.)

	-hpa

Re: [patch] x86: phase out forced inlining

[Harvey Harrison]

On Tue, 2008-03-04 at 10:09 -0800, H. Peter Anvin wrote:
> Harvey Harrison wrote:
> > 
> >>  __inline__  : 1380
> > 
> > Lots of them in include/asm-*...not sure if there is a reason for this.
> > 
> 
> Preferred form for code that's exported to userspace (since gcc 
> complains with -ansi -pedantic otherwise.)
> 

Figured it would be something like that.  Would it be reasonable to move
towards eliminating __inline?

Also, since the exported headers already go through unifdef, could we
move to using inline everywhere in the kernel and add a processing step
to make it __inline__ in the exported headers?

Harvey

Re: [patch] x86: phase out forced inlining

[Harvey Harrison]

On Tue, 2008-03-04 at 10:09 -0800, H. Peter Anvin wrote:
> Harvey Harrison wrote:
> > On Tue, 2008-03-04 at 17:46 +0100, Ingo Molnar wrote:
> >> * Sam Ravnborg <sam@ravnborg.org> wrote:
> > 
> >> i dont know why they there are so many variants, but all of them seem to 
> >> be used throughout the kernel:
> >>
> >>    inline    : 25648
> > 
> > I'll assume this is the preferred way of saying it.
> > 
> >>  __inline__  : 1380
> > 
> > Lots of them in include/asm-*...not sure if there is a reason for this.
> > 
> 
> Preferred form for code that's exported to userspace (since gcc 
> complains with -ansi -pedantic otherwise.)
> 
> 	-hpa

Re: [patch] x86: phase out forced inlining

[Arjan van de Ven]

On Mon, 3 Mar 2008 13:13:35 +0100
Ingo Molnar <mingo@elte.hu> wrote:


> 
> * Ingo Molnar <mingo@elte.hu> wrote:
> 
> > > > no, what we should nuke is this always_inline definition. That
> > > > was always the intention of FORCED_INLINE, and the removal of 
> > > > FORCED_INLINE was to _remove the forcing_, not to make it 
> > > > unconditional.
> > > 
> > > It was always unconditional, and neither adding, toggling nor 
> > > removing of CONFIG_FORCED_INLINING changed this invariant.
> > > 
> > > And what we should do is to attack the excessive wrong usage of 
> > > inlines in .c files, not messing with a global #define in a way
> > > that the results on 24 architectures with 7 different releases of
> > > gcc would be unpredictable.
> > 
> > i see, so you never properly reported and fixed it because you
> > prefer a 1000 small crappy changes over one change. You could have 
> > significantly contributed to truly making Linux smaller, but you 
> > decided not to do it.
> > 
> > and i disagree with your notion that flipping it around is risky in 
> > any unacceptable or unmanageable way. It has far less risks on the 
> > compiler than say CONFIG_CC_OPTIMIZE_FOR_SIZE. It has far less
> > risks than changing to a new compiler version. Why you think it's 
> > "unpredictable" is a mystery to me.
> > 
> > It almost seems to me you were happy with having that bug in the 
> > kernel? Please tell me that i'm wrong about that impression!
> > 
> > i'll reinstate this .config option and let it do the right thing. 
> > Forced inlining was supposed to be _phased out_, not phased in.
> 
> i just implemented the trivial fix below and it gave me a massive,
> 2.3% text size reduction (!) on a typical .config. That's more than
> 120K shaved off the vmlinux.
> 
> Why, despite being aware of this bug, you never fixed this properly
> is a mystery to me - this is more .text size savings than you have
> done so far with all your uninlining patches of the past few years
> combined, and it's probably more than you could have achieved in the
> next 5 years.
> 
> 	Ingo
> 
> ---------------------->
> Subject: x86: phase out forced inlining
> From: Ingo Molnar <mingo@elte.hu>
> Date: Mon Mar 03 12:38:52 CET 2008
> 
> allow gcc to optimize the kernel image's size by uninlining
> functions that have been marked 'inline'. Previously gcc was
> forced by Linux to always-inline these functions via a gcc
> attribute:
> 
>  #define inline	inline __attribute__((always_inline))
> 
> Especially when the user has already selected
> CONFIG_OPTIMIZE_FOR_SIZE=y this can make a huge difference in
> kernel image size (using a standard Fedora .config):
> 
>    text    data     bss     dec           hex filename
>    5613924  562708 3854336 10030968    990f78 vmlinux.before
>    5486689  562708 3854336  9903733    971e75 vmlinux.after
> 
> that's a 2.3% text size reduction (!).
> 
> Signed-off-by: Ingo Molnar <mingo@elte.hu>
> ---
>  arch/x86/Kconfig.debug            |   13 +++++++++++++
>  arch/x86/configs/i386_defconfig   |    1 +
>  arch/x86/configs/x86_64_defconfig |    1 +
>  include/linux/compiler-gcc.h      |   12 +++++++++---
>  4 files changed, 24 insertions(+), 3 deletions(-)
> 
> Index: linux-x86.q/arch/x86/Kconfig.debug
> ===================================================================
> --- linux-x86.q.orig/arch/x86/Kconfig.debug


eh.. we ALREADY HAD THIS.
someone seems to have removed this wronly; not forcing inline should have been
the default after the removal (in 2.6.25-rc)!

So lets fix it that way, rather than putting the config option back under a different name.

Re: [patch] x86: phase out forced inlining

[Harvey Harrison]

> eh.. we ALREADY HAD THIS.
> someone seems to have removed this wronly; not forcing inline should have been
> the default after the removal (in 2.6.25-rc)!
> 
> So lets fix it that way, rather than putting the config option back under a different name.

The removal was my patch.  I did notice that the config option had no
effect, I figured that was why it was on the removal schedule, as
it had been made the default.

My bad,

Harvey

Re: [patch] x86: phase out forced inlining

[Andrew Morton]

On Mon, 3 Mar 2008 13:13:35 +0100 Ingo Molnar <mingo@elte.hu> wrote:

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

urgh.  This will cause whatever problem
4507a6a59cfc6997e532cd812a8bd244181e6205 fixed five years ago to resurface
for incautious gcc-3.x users.

I'd suggest that this

> +#ifndef CONFIG_OPTIMIZE_INLINING

become something along the lines of

> +#ifndef CONFIG_OPTIMIZE_INLINING && (__GNUC__ > 3)

It would be nice to be able to feed the gcc version into the Kconfig logic,
really..

Re: [patch] x86: phase out forced inlining

[Ingo Molnar]

* Andrew Morton <akpm@linux-foundation.org> wrote:

> urgh.  This will cause whatever problem 
> 4507a6a59cfc6997e532cd812a8bd244181e6205 fixed five years ago to 
> resurface for incautious gcc-3.x users.

hm, commit 4507a6a59cfc6997e532cd812a8bd244181e6205 does not exist:

  fatal: bad object 4507a6a59cfc6997e532cd812a8bd244181e6205

but i suspect it must be something along the lines of the known problem 
of really old gcc versions creating huge stackframes? Those pristine gcc 
versions were practically unusable for distro kernels anyway (and were 
patched by distros) - but i have no problem with restricting this 
feature to gcc4x. gcc4x creates more compact -Os code too, so it's 
recommended for smaller image sizes.

> I'd suggest that this
> 
> > +#ifndef CONFIG_OPTIMIZE_INLINING
> 
> become something along the lines of
> 
> > +#ifndef CONFIG_OPTIMIZE_INLINING && (__GNUC__ > 3)

good point, fixed that.

> It would be nice to be able to feed the gcc version into the Kconfig 
> logic, really..

yeah, instead of littering our include files with those quirks.

	Ingo

Re: [patch] x86: phase out forced inlining

[Andrew Morton]

On Tue, 4 Mar 2008 08:32:48 +0100 Ingo Molnar <mingo@elte.hu> wrote:

> 
> * Andrew Morton <akpm@linux-foundation.org> wrote:
> 
> > urgh.  This will cause whatever problem 
> > 4507a6a59cfc6997e532cd812a8bd244181e6205 fixed five years ago to 
> > resurface for incautious gcc-3.x users.
> 
> hm, commit 4507a6a59cfc6997e532cd812a8bd244181e6205 does not exist:
> 
>   fatal: bad object 4507a6a59cfc6997e532cd812a8bd244181e6205

This was 2.5.x - you'll need to look in the historical-git tree.

Here it is:



: commit 4507a6a59cfc6997e532cd812a8bd244181e6205
: Author: akpm <akpm>
: Date:   Tue Mar 11 07:42:00 2003 +0000
: 
:     [PATCH] work around gcc-3.x inlining bugs
:     
:     Force inlining even when gcc-3.x is too confused to do it for us.
:     
:     BKrev: 3e6d9348GA9aKzeN-bjzQzMMt85t8g
: 
: diff --git a/include/linux/compiler.h b/include/linux/compiler.h
: index e92f472..a28d0d5 100644
: --- a/include/linux/compiler.h
: +++ b/include/linux/compiler.h
: @@ -1,6 +1,12 @@
:  #ifndef __LINUX_COMPILER_H
:  #define __LINUX_COMPILER_H
:  
: +#if (__GNUC__ > 3) || (__GNUC__ == 3 && __GNUC_MINOR__ >= 1)
: +#define inline		__inline__ __attribute__((always_inline))
: +#define __inline__	__inline__ __attribute__((always_inline))
: +#define __inline	__inline__ __attribute__((always_inline))
: +#endif
: +
:  /* Somewhere in the middle of the GCC 2.96 development cycle, we implemented
:     a mechanism by which the user can annotate likely branch directions and
:     expect the blocks to be reordered appropriately.  Define __builtin_expect
: 

I was very bad about changelogging that one.  I do remember there was a bit
of to-and-fro before we decided to do it this way.  Some googling would be
needed.  

> but i suspect it must be something along the lines of the known problem 
> of really old gcc versions creating huge stackframes?

iirc gcc was failing to inline functions which we'd marked `inline' and it
was generating poorer code as a result.  It might also have been generating
an out-of-line copy for each compilation unit which called the inline (it
would have to do this?)

> Those pristine gcc 
> versions were practically unusable for distro kernels anyway (and were 
> patched by distros) - but i have no problem with restricting this 
> feature to gcc4x. gcc4x creates more compact -Os code too, so it's 
> recommended for smaller image sizes.

yup.

Re: [patch] x86: phase out forced inlining

[Andi Kleen]

Andrew Morton <akpm@linux-foundation.org> writes:
>
> This was 2.5.x - you'll need to look in the historical-git tree.
>
> Here it is:
>
>
>
> : commit 4507a6a59cfc6997e532cd812a8bd244181e6205
> : Author: akpm <akpm>
> : Date:   Tue Mar 11 07:42:00 2003 +0000
> : 
> :     [PATCH] work around gcc-3.x inlining bugs
> :     
> :     Force inlining even when gcc-3.x is too confused to do it for us.

I think these old inlining bugs were just caused by missing __always_inline
(e.g. in the vsyscall code which requires forced inlining or in copy_*_user)
AFAIK these all have __always_inline these days and if any are still missing these
are easy to change over as needed.

So Ingo's change is likely ok.

-Andi
 

Re: [patch] x86: phase out forced inlining

[Sam Ravnborg]

On Mon, Mar 03, 2008 at 10:42:31PM -0800, Andrew Morton wrote:
> On Mon, 3 Mar 2008 13:13:35 +0100 Ingo Molnar <mingo@elte.hu> wrote:
> 
> > +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.
> 
> urgh.  This will cause whatever problem
> 4507a6a59cfc6997e532cd812a8bd244181e6205 fixed five years ago to resurface
> for incautious gcc-3.x users.
> 
> I'd suggest that this
> 
> > +#ifndef CONFIG_OPTIMIZE_INLINING
> 
> become something along the lines of
> 
> > +#ifndef CONFIG_OPTIMIZE_INLINING && (__GNUC__ > 3)
> 
> It would be nice to be able to feed the gcc version into the Kconfig logic,
> really..
We can do this using a few environment options.

Somewhere in a Makefile:
export CC_MAJOR=`magic`
export CC_MINOR=`more magic`

And in a Kconfig file:

config CC_MAJOR
	string
	option env=CC_MAJOR


And then we can do:
config CC_DEFAULT_INLINE
	bool "Force inline of functions annotated inline"
	default y if CC_MAJOR = 04

Shall I try to cook up a patch for this?
[I may get access to my dev box tonight - cannot promise..]

	Sam

Re: [patch] x86: phase out forced inlining

[Andrew Morton]

On Tue, 4 Mar 2008 09:03:59 +0100 Sam Ravnborg <sam@ravnborg.org> wrote:

> > It would be nice to be able to feed the gcc version into the Kconfig logic,
> > really..
> We can do this using a few environment options.
> 
> Somewhere in a Makefile:
> export CC_MAJOR=`magic`
> export CC_MINOR=`more magic`
> 
> And in a Kconfig file:
> 
> config CC_MAJOR
> 	string
> 	option env=CC_MAJOR
> 
> 
> And then we can do:
> config CC_DEFAULT_INLINE
> 	bool "Force inline of functions annotated inline"
> 	default y if CC_MAJOR = 04

Sounds neat.  I guess we should do HOST_CC too.

> Shall I try to cook up a patch for this?
> [I may get access to my dev box tonight - cannot promise..]

accuracy=1/speed.  There's no rush on this ;)

Re: [2.6 patch] acpi/battery.c: make 2 functions static

[Adrian Bunk]

On Mon, Mar 03, 2008 at 10:17:14AM +0100, Ingo Molnar wrote:
> 
> * Adrian Bunk <bunk@kernel.org> wrote:
> 
> > On Mon, Mar 03, 2008 at 09:57:20AM +0100, Ingo Molnar wrote:
> > > 
> > > * Adrian Bunk <bunk@kernel.org> wrote:
> > > 
> > > > On Sat, Mar 01, 2008 at 09:26:41PM +0300, Alexey Starikovskiy wrote:
> > > > > May I keep them inline?
> > > > 
> > > > The problem with such manual inlines is that we force gcc to always 
> > > > inline them - and history has shown that functions grow without the 
> > > > "inline" being removed.
> > > 
> > > what do you mean by "we force gcc to always inline them"?
> > 
> > #define inline          inline          __attribute__((always_inline))
> > 
> > > gcc is free to decide whether to inline or to not inline.
> > 
> > Not with __attribute__((always_inline)).
> 
> but that wasnt used in the code you patched:
> 
>   -inline int acpi_battery_present(struct acpi_battery *battery)
>   +static int acpi_battery_present(struct acpi_battery *battery)

It was used, since the #define affects all inline's in the kernel.

> > > (and CONFIG_FORCED_INLINING got removed from 2.6.25)
> > 
> > CONFIG_FORCED_INLINING never had any effect.
> 
> my experience was that it had effects. Why do you say it 'never had any 
> effect'?

I don't see how it could have possibly had any effect.

Which effects did you experience?

> 	Ingo

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