CONFIG_OPTIMIZE_INLINING

CONFIG_OPTIMIZE_INLINING

[David Miller]

Ingo, why the heck is this hiding under arch/x86/ and not in
lib/Kconfig.debug where it belongs?

    x86: add optimized inlining

If there are platforms that can't handle it, we can add a conditional
to lib/Kconfig.debug, but by default it should be made available
generically.

Where did this original patch get posted for review and discussion?

Tell me where!?!?!

The only references I see is where Stephen Rothwell complained that it
broke the powerpc build, and then on the git commit list when Linus
sucked this crud in.

Re: CONFIG_OPTIMIZE_INLINING

[Ingo Molnar]

* David Miller <davem@davemloft.net> wrote:

> Ingo, why the heck is this hiding under arch/x86/ and not in 
> lib/Kconfig.debug where it belongs?
> 
>     x86: add optimized inlining
> 
> If there are platforms that can't handle it, we can add a conditional 
> to lib/Kconfig.debug, but by default it should be made available 
> generically.
>
> Where did this original patch get posted for review and discussion?
> 
> Tell me where!?!?!

here, two months ago:

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

i made it x86-only due to this FUD that went against it:

  "messing with a global #define in a way that the results on 24 
   architectures with 7 different releases of gcc would be 
   unpredictable."

... as i saw no reason why this feature, which i found rather useful, 
should be delayed another year or so. I'd be more than happy to promote 
this feature back to lib/Kconfig.debug, sparc64 interest would make that 
a strong argument.

and here is the pull request that i posted to lkml, with the full patch 
included as well:

  http://www.gossamer-threads.com/lists/linux/kernel/911104?page=last

> The only references I see is where Stephen Rothwell complained that it 
> broke the powerpc build, and then on the git commit list when Linus 
> sucked this crud in.

i think there must something wrong about your lkml feed.

	Ingo

Re: CONFIG_OPTIMIZE_INLINING

[Sam Ravnborg]

On Sat, Apr 26, 2008 at 09:55:13PM -0700, David Miller wrote:
> 
> Ingo, why the heck is this hiding under arch/x86/ and not in
> lib/Kconfig.debug where it belongs?
> 
>     x86: add optimized inlining
> 
> If there are platforms that can't handle it, we can add a conditional
> to lib/Kconfig.debug, but by default it should be made available
> generically.
> 
> Where did this original patch get posted for review and discussion?

The first version of the patch was this one I think:
http://lkml.org/lkml/2008/3/3/122

	Sam

Re: CONFIG_OPTIMIZE_INLINING

[David Miller]

From: Ingo Molnar <mingo@elte.hu>
Date: Sun, 27 Apr 2008 07:59:43 +0200

> ... as i saw no reason why this feature, which i found rather useful, 
> should be delayed another year or so. I'd be more than happy to promote 
> this feature back to lib/Kconfig.debug, sparc64 interest would make that 
> a strong argument.

So you caved in to FUD in order to pad your commit and signoff count?

Because anyone who is paying attention can see clearly that you're
trying to push as much stuff as quickly as possible into Linus's tree
with your singoffs and authorship on it this merge window.

Gee, I wonder why...

Re: CONFIG_OPTIMIZE_INLINING

[Christoph Hellwig]

On Sun, Apr 27, 2008 at 07:59:43AM +0200, Ingo Molnar wrote:
> here, two months ago:
> 
>   http://lkml.org/lkml/2008/3/3/122
> 
> i made it x86-only due to this FUD that went against it:
> 
>   "messing with a global #define in a way that the results on 24 
>    architectures with 7 different releases of gcc would be 
>    unpredictable."
> 
> ... as i saw no reason why this feature, which i found rather useful, 
> should be delayed another year or so. I'd be more than happy to promote 
> this feature back to lib/Kconfig.debug, sparc64 interest would make that 
> a strong argument.
> 
> and here is the pull request that i posted to lkml, with the full patch 
> included as well:
> 
>   http://www.gossamer-threads.com/lists/linux/kernel/911104?page=last

This still does not explain why it's two commits.   It also doesn't
explain the horrible cpp and ifdef abuse in there - having a config
symbol in one architecture only and then a cpp symbol to prevent the ill
effects of it in one signle architecture is simply utterly braindead,
sorry.

This kind of thing should be discussed at least on linux-arch and input
from at least partially wise people would have led too a much better
solution.  Like, umm moving the option to a global config file and let
the architecture default it to y/n where needed.

Re: CONFIG_OPTIMIZE_INLINING

[Ingo Molnar]

* David Miller <davem@davemloft.net> wrote:

> From: Ingo Molnar <mingo@elte.hu>
> Date: Sun, 27 Apr 2008 07:59:43 +0200
> 
> > ... as i saw no reason why this feature, which i found rather 
> > useful, should be delayed another year or so. I'd be more than happy 
> > to promote this feature back to lib/Kconfig.debug, sparc64 interest 
> > would make that a strong argument.
> 
> So you caved in to FUD in order to pad your commit and signoff count?

David, stop this nonsense already.

Had you read the lkml discussion thread i linked to:

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

... you'd realize that the first thing i did was to make it a generic 
feature. But it was claimed on lkml that this would break an unknown 
number of architectures:

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

Where were you in that thread defending my position? Andrew showed 
interest, gave me feedback and i incorporated that feedback.

I am fully willing and ready to move this feature to a generic file (via 
the patch below), in fact i implemented it that way from the get go, but 
only:

   _if other architecture maintainers want it too_

Nobody countered the "it would break other architectures" position in 
that discussion, and i'm not interested in endless bikeshed painting 
either.

lets look at a counter-example where other architecture maintainers 
showed interest: the recent bitops generalizations we did for x86 and 
extended to all other architectures. It was welcome by other maintainers 
and we did it that way. See the merge commit 9b79ed952bd734 for an 
overview of this topic:

    x86, bitops: select the generic bitmap search functions
    x86: include/asm-x86/pgalloc.h/bitops.h: checkpatch cleanups - formatting
    x86: finalize bitops unification
    x86, UML: remove x86-specific implementations of find_first_bit
    x86: optimize find_first_bit for small bitmaps
    x86: switch 64-bit to generic find_first_bit
    x86: generic versions of find_first_(zero_)bit, convert i386
    bitops: use __fls for fls64 on 64-bit archs
    generic: implement __fls on all 64-bit archs
    generic: introduce a generic __fls implementation
    x86: merge the simple bitops and move them to bitops.h
    x86, generic: optimize find_next_(zero_)bit for small constant-size bitmap
    x86, uml: fix uml with generic find_next_bit for x86
    x86: change x86 to use generic find_next_bit
    uml: Kconfig cleanup
    uml: fix build error

	Ingo

-------------------------------->
Subject: generic: optimize inlining
From: Ingo Molnar <mingo@elte.hu>
Date: Sun Apr 27 09:36:29 CEST 2008

Suggested-by: David Miller <davem@davemloft.net>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 arch/x86/Kconfig.debug |   12 ------------
 init/Kconfig           |   13 +++++++++++++
 2 files changed, 13 insertions(+), 12 deletions(-)

Index: linux/arch/x86/Kconfig.debug
===================================================================
--- linux.orig/arch/x86/Kconfig.debug
+++ linux/arch/x86/Kconfig.debug
@@ -355,15 +355,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/init/Kconfig
===================================================================
--- linux.orig/init/Kconfig
+++ linux/init/Kconfig
@@ -520,6 +520,19 @@ config CC_OPTIMIZE_FOR_SIZE
 
 	  If unsure, say N.
 
+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.
+
 config SYSCTL
 	bool
 

Re: CONFIG_OPTIMIZE_INLINING

[SL Baur]

On 4/26/08, David Miller <davem@davemloft.net> wrote:

>  Because anyone who is paying attention can see clearly that you're
>  trying to push as much stuff as quickly as possible into Linus's tree

I'm giving up for the moment trying to review anything.  I can't contribute
by compile testing a lot of stuff - I don't have a farm of computers that can
build and boot thousands of kernels overnight, nor a machine that can build
a kernel in 2 minutes, nor a suite of cross-compilers that can test
build patches
across all the architectures we support.  I'm not paid to follow any of this, my
only paid concern is Linux userland and so following Linux Kernel is a hobby.

If you, Ingo, are going to push patches in before they can be reviewed, what
reason is there for anyone to bother trying?  (The patch I was trying to review
that you merged before review was complete was different than this one).

Oh and Thank You! David for starting to post your git pull messages.  Idiots
like me read those sorts of things and appreciate it *very* much.

-sb  (I *like* reading patches and I want to contribute any way I can)

Re: CONFIG_OPTIMIZE_INLINING

[Dave Airlie]

On Sun, Apr 27, 2008 at 7:10 PM, SL Baur <steve@xemacs.org> wrote:
> On 4/26/08, David Miller <davem@davemloft.net> wrote:
>
>  >  Because anyone who is paying attention can see clearly that you're
>  >  trying to push as much stuff as quickly as possible into Linus's tree
>
>  I'm giving up for the moment trying to review anything.  I can't contribute
>  by compile testing a lot of stuff - I don't have a farm of computers that can
>  build and boot thousands of kernels overnight, nor a machine that can build
>  a kernel in 2 minutes, nor a suite of cross-compilers that can test
>  build patches
>  across all the architectures we support.  I'm not paid to follow any of this, my
>  only paid concern is Linux userland and so following Linux Kernel is a hobby.
>
>  If you, Ingo, are going to push patches in before they can be reviewed, what
>  reason is there for anyone to bother trying?  (The patch I was trying to review
>  that you merged before review was complete was different than this one).
>
>  Oh and Thank You! David for starting to post your git pull messages.  Idiots
>  like me read those sorts of things and appreciate it *very* much.
>
>  -sb  (I *like* reading patches and I want to contribute any way I can)
>

The thing is merging doesn't change the code, reviewing patches in
Linus' tree or patches on there way to Linus' tree,
shouldn't really affect review unless its in the creating a new
userspace API area.

so please continue to review the patches even if they make it
upstream, you can still provide feedback,

just because something lands in the core kernel tree doesn't mean it
is actually perfect, far from it..

Dave.

Re: CONFIG_OPTIMIZE_INLINING

[David Miller]

From: "Dave Airlie" <airlied@gmail.com>
Date: Sun, 27 Apr 2008 19:16:31 +1000

> just because something lands in the core kernel tree doesn't mean it
> is actually perfect, far from it..

This is entirely missing the point.

We get patches reviewed before they hit the tree, not afterwards.

Ingo is making that impossible.

Re: CONFIG_OPTIMIZE_INLINING

[Sam Ravnborg]

On Sun, Apr 27, 2008 at 02:26:00AM -0400, Christoph Hellwig wrote:
> On Sun, Apr 27, 2008 at 07:59:43AM +0200, Ingo Molnar wrote:
> > here, two months ago:
> > 
> >   http://lkml.org/lkml/2008/3/3/122
> > 
> > i made it x86-only due to this FUD that went against it:
> > 
> >   "messing with a global #define in a way that the results on 24 
> >    architectures with 7 different releases of gcc would be 
> >    unpredictable."
> > 
> > ... as i saw no reason why this feature, which i found rather useful, 
> > should be delayed another year or so. I'd be more than happy to promote 
> > this feature back to lib/Kconfig.debug, sparc64 interest would make that 
> > a strong argument.
> > 
> > and here is the pull request that i posted to lkml, with the full patch 
> > included as well:
> > 
> >   http://www.gossamer-threads.com/lists/linux/kernel/911104?page=last
> 
> This still does not explain why it's two commits.   It also doesn't
> explain the horrible cpp and ifdef abuse in there - having a config
> symbol in one architecture only and then a cpp symbol to prevent the ill
> effects of it in one signle architecture is simply utterly braindead,
> sorry.
> 
> This kind of thing should be discussed at least on linux-arch and input
> from at least partially wise people would have led too a much better
> solution.  Like, umm moving the option to a global config file and let
> the architecture default it to y/n where needed.

I just posted a patch to linux arch that to some extent do as you sketch above.
Then I hope some of the "partially wise" people will nack or ack or suggest
better ways to do it.

	Sam

Re: CONFIG_OPTIMIZE_INLINING

[Ingo Molnar]

* David Miller <davem@davemloft.net> wrote:

> From: Ingo Molnar <mingo@elte.hu>
> Date: Sun, 27 Apr 2008 07:59:43 +0200
> 
> > ... as i saw no reason why this feature, which i found rather 
> > useful, should be delayed another year or so. I'd be more than happy 
> > to promote this feature back to lib/Kconfig.debug, sparc64 interest 
> > would make that a strong argument.
> 
> So you caved in to FUD in order to pad your commit and signoff count?
> 
> Because anyone who is paying attention can see clearly that you're 
> trying to push as much stuff as quickly as possible into Linus's tree 
> with your singoffs and authorship on it this merge window.
> 
> Gee, I wonder why...

That suggestion is ridiculous but i guess i have to reply to it.

Firstly, i've been given more credit in Linux already than is enough for 
a lifetime ;-) 1000+ changes down the line the joy of having yet another 
commit upstream is ... minimal. I'm more interested in seeing Linux 
progress as a whole and most of my pride goes towards the whole 
structure not towards individual changes.

Secondly, you dont even have to take my word on this one. Was there even 
a shred of truth to the claim above i'd surely must have split up this 
recent x86 commit into many small commits:

| commit a4928cffe6435caf427ae673131a633c1329dbf3
| Author: Ingo Molnar <mingo@elte.hu>
| Date:   Wed Apr 23 13:20:56 2008 +0200
|
|    "make namespacecheck" fixes
|
|    Signed-off-by: Ingo Molnar <mingo@elte.hu>

  arch/x86/kernel/apic_32.c     |    2 +-
  arch/x86/kernel/apic_64.c     |    4 ++--
  arch/x86/kernel/process_32.c  |    2 +-
  arch/x86/kernel/process_64.c  |    2 +-
  arch/x86/kernel/setup_32.c    |    4 ++--
  arch/x86/kernel/smpboot.c     |   12 ++++++------
  arch/x86/kernel/tlb_64.c      |    2 +-
  arch/x86/kernel/vsyscall_64.c |    2 +-
  arch/x86/mm/dump_pagetables.c |    2 +-
  arch/x86/mm/pageattr.c        |    2 +-
  arch/x86/mm/srat_64.c         |    2 +-
  include/asm-x86/smp.h         |    1 -
  include/asm-x86/tsc.h         |    1 -
  13 files changed, 18 insertions(+), 20 deletions(-)

i could have made that 13 separate commits - as it is done with these 
kinds of commits all around the tree (networking included, see commit 
263173af).

Right?

And i routinely backmerge my fixlets into the body of the patch. In the 
current merge window alone:

 $ earth4:~/v> git-log v2.6.25.. | grep ' \[ mingo@elte'
    [ mingo@elte.hu: minor cleanups. ]
    [ mingo@elte.hu: redesign, splitups, cleanups. ]
    [ mingo@elte.hu: heavily modified, simplified and cleaned up. ]
    [ mingo@elte.hu: do it on gbpages kernels too, there's no clear reason
    >     [ mingo@elte.hu: build fix ]
    [ mingo@elte.hu: build fix ]
    [ mingo@elte.hu: fix boot regression. ]
    [ mingo@elte.hu: x86: fix the pagetable dumper ]

... without creating a separate commit for the fixes.

But it's more than that: in fact i change the code in the _majority of 
commits_ that i accept (various minor errors are very common), without 
creating small fixlets. Most of the patches have to be edited trivially 
for one or another reason - and that's just the code portion - 99% of 
the commit messages of them has to be edited, most of them 
substantially.

So if i were creating commits for each of those edits we'd have ~500-600 
more trivial commits in this merge window alone. And i know you do this 
fix backmerging too and it's the correct maintenance practice IMO.

As an example for this practice, look at the current raw, unprocessed, 
not-yet-backmerged ftrace tree that i just posted to lkml. It has 34 
commits from me:

> >   Ingo Molnar (34):

about 30 of those commits will go away completely by the time i post 
that for upstream.

So not only is that claim patently untrue, it's the _exact opposite_ of 
what i'm doing day to day.

	Ingo

Re: CONFIG_OPTIMIZE_INLINING

[Ingo Molnar]

* David Miller <davem@davemloft.net> wrote:

> From: "Dave Airlie" <airlied@gmail.com>
> Date: Sun, 27 Apr 2008 19:16:31 +1000
> 
> > just because something lands in the core kernel tree doesn't mean it 
> > is actually perfect, far from it..
> 
> This is entirely missing the point.
> 
> We get patches reviewed before they hit the tree, not afterwards.
> 
> Ingo is making that impossible.

hrmpf. David, i can only repeat that what you say is plain out false. 
The CONFIG_OPTIMIZE_INLINING patch was posted to lkml originally, about 
two months ago:

  http://lkml.org/lkml/2008/3/3/122
 
then it was re-posted at the time of the pull request as well:

  http://www.gossamer-threads.com/lists/linux/kernel/911104?page=last

and i just posted a (trivial) RFC patch to lkml today that would turn it 
into a generic feature:

  http://lkml.org/lkml/2008/4/27/47

so i'm not sure what this big fuss is about ...

Moving this (now apparently hotly desired!) feature from 
arch/x86/Kconfig to init/Kconfig is no big deal and lets continue with 
more important issues. No puppies got hurt, really :)

You can use the patch i posted or you've got my conceptual Acked-by for 
touching arch/x86/Kconfig or can do it without asking - i dont mind -, 
it's an obviously correct change that i not only wanted all along but 
also implemented that way originally (twice!), until stupid lkml 
objections forced it into arch/x86 as i went the path of least 
resistance.

Yes, in hindsight, i should have stood up for that change and should 
have made a stink about it on linux-arch but there's just so many 
flamewars that fit into a day ;-)

	Ingo

Re: CONFIG_OPTIMIZE_INLINING

[Pavel Machek]

Hi!

> > ... as i saw no reason why this feature, which i found rather useful, 
> > should be delayed another year or so. I'd be more than happy to promote 
> > this feature back to lib/Kconfig.debug, sparc64 interest would make that 
> > a strong argument.
> 
> So you caved in to FUD in order to pad your commit and signoff count?
> 
> Because anyone who is paying attention can see clearly that you're
> trying to push as much stuff as quickly as possible into Linus's tree
> with your singoffs and authorship on it this merge window.
> 
> Gee, I wonder why...

What is going on here?

Yes, Ingo wants to merge a lots of stuff so that he does not have to
maintain it for two more months. That's the result of 14-days merge
window decission. ... 

No, I don't like that, either, but I don't think you can blame Ingo
for the merge window.
							Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: CONFIG_OPTIMIZE_INLINING

[Adrian Bunk]

On Sun, Apr 27, 2008 at 07:25:43PM +0200, Pavel Machek wrote:
> Hi!
> 
> > > ... as i saw no reason why this feature, which i found rather useful, 
> > > should be delayed another year or so. I'd be more than happy to promote 
> > > this feature back to lib/Kconfig.debug, sparc64 interest would make that 
> > > a strong argument.
> > 
> > So you caved in to FUD in order to pad your commit and signoff count?
> > 
> > Because anyone who is paying attention can see clearly that you're
> > trying to push as much stuff as quickly as possible into Linus's tree
> > with your singoffs and authorship on it this merge window.
> > 
> > Gee, I wonder why...
> 
> What is going on here?
> 
> Yes, Ingo wants to merge a lots of stuff so that he does not have to
> maintain it for two more months. That's the result of 14-days merge
> window decission. ... 
> 
> No, I don't like that, either, but I don't think you can blame Ingo
> for the merge window.

Do you remember the IDE disaster during 2.5?
It was caused by a maintainer sending unreviewed patches directly
to Linus.

What gets merged during the merge window should have been reviewed weeks 
or months earlier, and have gotten tested for some weeks by people 
(that's what linux-next and -mm are for).

If someone has fresh stuff during a merge window the best solution in 
many cases is to keep it for the next merge window, and get it properly 
reviewed and tested in the meantime.

We have serious problems to cope with the regressions that get merged 
during a merge window, and the options are:
- very buggy stable kernels
- longer release cycles (which in turn result in even more stuff being
  merged during merge windows, making all problems even worse)
- get stuff reviewed and tested *before* it hits mainline

We are currently changing more than one million lines per release, 
and we somehow need to ensure that we don't ruin Linux' reputation
as a stable OS due to a lack of QA.

> 							Pavel

cu
Adrian

BTW:
This obviously doesn't strictly apply to e.g. new drivers or simple
patches (although we really need a better review for drivers before 
merging, since usually once they are in Linus' tree noone (except Al)
does serious cleanups on them).

-- 

       "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