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

Andrew Morton
2006-03-08T00:13:43Z

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?)