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