Messages in this thread | | | Date | Sat, 09 Aug 2008 10:04:03 +0200 | From | Stefan Richter <> | Subject | Re: [ANNOUNCE] mdb-2.6.27-rc2-ia32-08-07-08.patch |
| |
Jeremy Fitzhardinge wrote: > Stefan Richter wrote: >> jmerkey@wolfmountaingroup.com wrote: >>> ftp://ftp.wolfmountaingroup.org/pub/mdb/mdb-2.6.27-rc2-ia32-08-07-08.patch [...] >> The accessors rspin_lock() and rspin_try_lock() peek into spinlock_t and >> may therefore not be fully portable. Also, they and rspin_unlock() >> don't look SMP safe: >> >> >>> +// >>> +// returns 0 - atomic lock occurred, processor assigned >>> +// 1 - recusive count increased >>> +// >>> + >>> +unsigned long rspin_lock(volatile rlock_t *rlock) >>> +{ >>> +#if defined(CONFIG_SMP) >>> + register unsigned long proc = get_processor_id(); >>> + register unsigned long retCode; >>> + >>> + if (rlock->lock.raw_lock.slock && rlock->processor == proc) >>> > > Ticket locks will almost always have a non-zero slock. It doesn't > indicate anything about the locked/unlocked state. But this looks like > it's effectively doing a trylock: > > if (!spin_trylock(rlock) && rlock->processor == proc) { > rlock->count++; > ... > } else { > rlock->processor = proc; > ... > }
Right. This implemention also looks free of race conditions, provided that
- rspin_lock, rspin_try_lock, and rspin_unlock are only called in contexts with disabled preemption and disabled local interrupts,
- rspin_unlock() rewrites rlock->processor to "no CPU" before it drops the lock. (The implementation in mdb-2.6.27-rc2-ia32-08-07-08.patch does so.)
BTW, the rspin_try_lock() in that patch wrong: It always returns 0 instead of having three branches of execution which return 0/1/-1. -- Stefan Richter -=====-==--- =--- -=--= http://arcgraph.de/sr/
| |