[LinuxBIOS] [PATCH][v3] Spinlock fixes

[Uwe Hermann]

See patch.

Uwe.

[Stefan Reinauer]

* Uwe Hermann  [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
> 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.

[Uwe Hermann]

On Fri, Jul 06, 2007 at 08:22:59AM +0200, Stefan Reinauer wrote:
> Uwe Hermann  [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.