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

[Andrew Morton <akpm@osdl.org>]

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?)
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/