[LinuxBIOS] [PATCH][v3] Spinlock fixes

Uwe Hermann uwe at hermann-uwe.de
Fri Jul 6 17:07:54 CEST 2007


On Fri, Jul 06, 2007 at 08:22:59AM +0200, Stefan Reinauer wrote:
> * Uwe Hermann <uwe at hermann-uwe.de> [070706 02:05]:
> > Various spinlock-related cleanups:
> > 
> >  - Revert back to spinlock_t (instead of struct spinlock). This is fine
> >    in this special case, as the contents of spinlock_t are not meant to
> >    ever be accessed directly (only by "accessor" functions).
>  
> NACK.
> 
> Why would that be required? The code in svn needs no fixing. Let's not
> invent typedefs without any need for them

It's not strictly required, but as the original code had a spinlock_t
(which we removed because I made some stupid comments before actually
thinking), I thought we could add it back.

It makes sense in this case, as we'll never access anything in the
struct directly. spinlock_t could probably be implemented differently,
and that's the whole point of the typedef in this case -- we don't care
how it's implemented, we only ever access the variables via
spin_lock(foo) and spin_unlock(foo), never via foo->lock directly.

 
> >  - Drop the spin_lock_string and spin_unlock_string macros, they're pretty
> >    useless as they're only used in a single place and a macro doesn't make
> >    this code any more readable, IMO.
> 
> NACK.
> 
> They are not useless at all as they are _the_ (one and only) spinlock
> implementation on x86. The "only single place" is the declaration of the
> x86 specific spin_lock and spin_unlock functions. 

Huh? I don't understand.

My remark was meant to say the following: version a) is not more
readable than b), so let's use b).


a)

#define spin_lock_string \
        "\n1:\t" \
        "lock ; decb %0\n\t" \
        "js 2f\n" \
        ".section .text.lock,\"ax\"\n" \
        "2:\t" \
        "cmpb $0,%0\n\t" \
        "rep;nop\n\t" \
        "jle 2b\n\t" \
        "jmp 1b\n" \
        ".previous"

static inline __attribute__((always_inline)) void spin_lock(spinlock_t *lock)
{
        __asm__ __volatile__(
                spin_lock_string
                :"=m" (lock->lock) : : "memory");
}


b)

static inline __attribute__((always_inline)) void spin_lock(spinlock_t *lock)
{
      __asm__ __volatile__("\n1:\t"
                           "lock ; decb %0\n\t"
                           "js 2f\n"
                           ".section .text.lock,\"ax\"\n"
                           "2:\t"
                           "cmpb $0,%0\n\t"
                           "rep;nop\n\t"
                           "jle 2b\n\t"
                           "jmp 1b\n"
                           ".previous"
                           :"=m" (lock->lock) : : "memory");
}

Are there any drawbacks if we use version b)? I think not. This is
indeed x86 specific, a spinlock implementation for PowerPC for example
would be in a different file (arch/powerpc/arch/spinlock.h) and would
implement spin_lock() from scratch anyway.

It's not like we define another spin_lock_string for PowerPC and use it
in a generic spin_lock() function. If we do that, then the spin_lock()
should not be in include/arch/x86/arch/spinlock.h (x86-specific), but rather
in include/spinlock.h (generic, arch-independent), correct?


Uwe.
-- 
http://www.hermann-uwe.de  | http://www.holsham-traders.de
http://www.crazy-hacks.org | http://www.unmaintained-free-software.org
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://www.coreboot.org/pipermail/coreboot/attachments/20070706/7e3d47de/attachment.sig>


More information about the coreboot mailing list