[patch] i386 spinlocks: disable interrupts only if we enabled them

[patch] i386 spinlocks: disable interrupts only if we enabled them

[Chuck Ebbert]

_raw_spin_lock_flags() is entered with interrupts disabled.  If it
cannot obtain a spinlock, it checks the flags that were passed and
re-enables interrupts before spinning if that's how the flags are set.
When the spinlock might be available, it disables interrupts (even if
they are already disabled) before trying to get the lock.  Change that
so interrupts are only disabled if they have been enabled.  This costs
nine bytes of duplicated spinloop code.

Fastpath before patch:
        jle <keep looping>      not-taken conditional jump
        cli                     disable interrupts
        jmp <try for lock>      unconditional jump

Fastpath after patch, if interrupts were not enabled:
        jg <try for lock>       taken conditional branch

Signed-off-by: Chuck Ebbert <76306.1226@compuserve.com>

--- 2.6.16-rc5-d2.orig/include/asm-i386/spinlock.h
+++ 2.6.16-rc5-d2/include/asm-i386/spinlock.h
@@ -35,18 +35,23 @@
 #define __raw_spin_lock_string_flags \
 	"\n1:\t" \
 	"lock ; decb %0\n\t" \
-	"jns 4f\n\t" \
+	"jns 5f\n" \
 	"2:\t" \
 	"testl $0x200, %1\n\t" \
-	"jz 3f\n\t" \
-	"sti\n\t" \
+	"jz 4f\n\t" \
+	"sti\n" \
 	"3:\t" \
 	"rep;nop\n\t" \
 	"cmpb $0, %0\n\t" \
 	"jle 3b\n\t" \
 	"cli\n\t" \
 	"jmp 1b\n" \
-	"4:\n\t"
+	"4:\t" \
+	"rep;nop\n\t" \
+	"cmpb $0, %0\n\t" \
+	"jg 1b\n\t" \
+	"jmp 4b\n" \
+	"5:\n\t"
 
 static inline void __raw_spin_lock(raw_spinlock_t *lock)
 {
-- 
Chuck
"Penguins don't come from next door, they come from the Antarctic!"

Re: [patch] i386 spinlocks: disable interrupts only if we enabled them

[Andrew Morton]

Chuck Ebbert <76306.1226@compuserve.com> wrote:
>
> _raw_spin_lock_flags() is entered with interrupts disabled.  If it
> cannot obtain a spinlock, it checks the flags that were passed and
> re-enables interrupts before spinning if that's how the flags are set.
> When the spinlock might be available, it disables interrupts (even if
> they are already disabled) before trying to get the lock.  Change that
> so interrupts are only disabled if they have been enabled.  This costs
> nine bytes of duplicated spinloop code.
> 
> Fastpath before patch:
>         jle <keep looping>      not-taken conditional jump
>         cli                     disable interrupts
>         jmp <try for lock>      unconditional jump
> 
> Fastpath after patch, if interrupts were not enabled:
>         jg <try for lock>       taken conditional branch
> 

Well no.  The fastpath is:

	jns	4f		we got the lock.

> 
> --- 2.6.16-rc5-d2.orig/include/asm-i386/spinlock.h
> +++ 2.6.16-rc5-d2/include/asm-i386/spinlock.h
> @@ -35,18 +35,23 @@
>  #define __raw_spin_lock_string_flags \
>  	"\n1:\t" \
>  	"lock ; decb %0\n\t" \
> -	"jns 4f\n\t" \
> +	"jns 5f\n" \
>  	"2:\t" \
>  	"testl $0x200, %1\n\t" \
> -	"jz 3f\n\t" \
> -	"sti\n\t" \
> +	"jz 4f\n\t" \
> +	"sti\n" \
>  	"3:\t" \
>  	"rep;nop\n\t" \
>  	"cmpb $0, %0\n\t" \
>  	"jle 3b\n\t" \
>  	"cli\n\t" \
>  	"jmp 1b\n" \
> -	"4:\n\t"
> +	"4:\t" \
> +	"rep;nop\n\t" \
> +	"cmpb $0, %0\n\t" \
> +	"jg 1b\n\t" \
> +	"jmp 4b\n" \
> +	"5:\n\t"
>  

So this is speeding up the slowpath, which really shouldn't matter unless
we have bigger problems.

And it's increasing text size.  Which wouldn't be a big problem if the
spinning code was still in an out-of-line section, but it isn't any more.

(I forget why we undid that optimisation.  What was wrong with it?)

Re: [patch] i386 spinlocks: disable interrupts only if we enabled them

[Ingo Molnar]

* Andrew Morton <akpm@osdl.org> wrote:

> And it's increasing text size.  Which wouldn't be a big problem if the 
> spinning code was still in an out-of-line section, but it isn't any 
> more.
> 
> (I forget why we undid that optimisation.  What was wrong with it?)

we dont inline that code anymore. So i think the optimization is fine.

Acked-by: Ingo Molnar <mingo@elte.hu>

	Ingo

Re: [patch] i386 spinlocks: disable interrupts only if we enabled them

[Benjamin LaHaise]

On Wed, Mar 08, 2006 at 01:43:08AM +0100, Ingo Molnar wrote:
> we dont inline that code anymore. So i think the optimization is fine.

Why is that?  It adds memory traffic that has to be synchronized 
before the lock occurs and clobbered registers now in the caller.

		-ben
-- 
"Time is of no importance, Mr. President, only life is important."
Don't Email: <dont@kvack.org>.

Re: [patch] i386 spinlocks: disable interrupts only if we enabled them

[Andrew Morton]

Benjamin LaHaise <bcrl@kvack.org> wrote:
>
> On Wed, Mar 08, 2006 at 01:43:08AM +0100, Ingo Molnar wrote:
>  > we dont inline that code anymore. So i think the optimization is fine.
> 
>  Why is that?  It adds memory traffic that has to be synchronized 
>  before the lock occurs and clobbered registers now in the caller.

Is the inlined lock;decb+jns likely to worsen the text size?  I doubt it. 
Overall text will get bigger due to the out-of-line stuff, but that's OK.

I'm sure we went over all this, but I don't recall the thinking.

Re: [patch] i386 spinlocks: disable interrupts only if we enabled them

[Nick Piggin]

Andrew Morton wrote:
> Benjamin LaHaise <bcrl@kvack.org> wrote:
> 
>>On Wed, Mar 08, 2006 at 01:43:08AM +0100, Ingo Molnar wrote:
>> > we dont inline that code anymore. So i think the optimization is fine.
>>
>> Why is that?  It adds memory traffic that has to be synchronized 
>> before the lock occurs and clobbered registers now in the caller.
> 
> 
> Is the inlined lock;decb+jns likely to worsen the text size?  I doubt it. 
> Overall text will get bigger due to the out-of-line stuff, but that's OK.
> 
> I'm sure we went over all this, but I don't recall the thinking.

Seems like a very good idea not to clobber any registers in
lock fastpaths. I don't see how that could have been a win
(especially for i386) but still, Ingo must have had a reason
behind it.

-- 
SUSE Labs, Novell Inc.
Send instant messages to your online friends http://au.messenger.yahoo.com 

Re: [patch] i386 spinlocks: disable interrupts only if we enabled them

[Chuck Ebbert]

In-Reply-To: <20060307161550.27941df5.akpm@osdl.org>

On Tue, 7 Mar 2006 16:15:50, Andrew Morton wrote:

> Chuck Ebbert <76306.1226@compuserve.com> wrote:
> > Fastpath before patch:
> >         jle <keep looping>      not-taken conditional jump
> >         cli                     disable interrupts
> >         jmp <try for lock>      unconditional jump
> > 
> > Fastpath after patch, if interrupts were not enabled:
> >         jg <try for lock>       taken conditional branch
> > 
> 
> Well no.  The fastpath is:
> 
>       jns     4f              we got the lock.

 That's debatable.  Once the spinlock is available, jumping back to
try and get it becomes fastpath code, even though the spinloop itself
is not.  Any delay seen here means lost cycles that could be doing work.

 My only real question is how long 'cli' takes if interrupts are already
disabled.

> And it's increasing text size.  Which wouldn't be a big problem if the
> spinning code was still in an out-of-line section, but it isn't any more.

 __raw_spin_lock_flags is inlined, but only one place, in
_spin_lock_irqsave(), which is no longer an inline.  So there's no real
need to out-of-line the spin code anymore.  This adds nine bytes, which
should be acceptable in an out-of-line function. (Only the unlock functions
are inlined, and even then only if !CONFIG_PREEMPT).


-- 
Chuck
"Penguins don't come from next door, they come from the Antarctic!"