Re: [patch] uninline init_waitqueue_*() functions

Ingo Molnar
2006-07-05T22:35:18Z

* Linus Torvalds <torvalds@osdl.org> wrote:

> On Wed, 5 Jul 2006, Ingo Molnar wrote:
> >
> > yeah, i'd not want to skip over some interesting and still unexplained
> > effect either, but 35 bytes isnt all that outlandish and from everything
> > i've seen it's a real win. Here is an actual example:
> >
> > c0fb6137: c7 44 24 08 00 00 00 movl $0x0,0x8(%esp)
> > c0fb613e: 00
> > c0fb613f: c7 44 24 08 01 00 00 movl $0x1,0x8(%esp)
> > c0fb6146: 00
> > c0fb6147: c7 43 60 00 00 00 00 movl $0x0,0x60(%ebx)
> > c0fb614e: 8b 44 24 08 mov 0x8(%esp),%eax
> > c0fb6152: 89 43 5c mov %eax,0x5c(%ebx)
> > c0fb6155: 8d 43 64 lea 0x64(%ebx),%eax
> > c0fb6158: 89 40 04 mov %eax,0x4(%eax)
> > c0fb615b: 89 43 64 mov %eax,0x64(%ebx)
>
> Ahh, it's _that_ old gcc problem.
>
> That's actually a different thing.
>
> Gcc is HORRIBLY BAD at doing the simple
>
> some_structure = (struct somestruct) { INITIAL };
>
> assignments. It is so ludicrously bad that it's sad. It tends to do
> that as a local "struct somestruct" on the stack that gets
> initialized, followed by a memcpy().
>
> In this case, the problem appears to be the spinlock initialization
> code.
>
> In other words, I suspect 90% of your improvement was because you got
> that braindamage out of line.
>
> It would be _much_ better to just fix "spin_lock_init()" instead. That
> would help a lot of _other_ users too, not just the waitqueue
> initializations.
>
> Making that a real function (and inline only for the non-debug case,
> at which point it's just a simple and small store) would be much
> better.

in the debug case it's already a function. (by virtue of lockdep) But
what happens here is CONFIG_PREEMPT and break_lock and thus we get two
fields which get initialized in that stupid way. I'll try a better
initialization sequence. This was with gcc 4.0.1.

Ingo