On Fri, Jul 06, 2007 at 08:22:59AM +0200, Stefan Reinauer wrote: > * Uwe Hermann <[EMAIL PROTECTED]> [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
signature.asc
Description: Digital signature
-- linuxbios mailing list [email protected] http://www.linuxbios.org/mailman/listinfo/linuxbios
