Cc: <paulmck@...>, Peter Zijlstra <peterz@...>, <linux-kernel@...>, Linus Torvalds <torvalds@...>, Nick Piggin <nickpiggin@...>, David Howells <dhowells@...>
I'm having a quick look at mdb-2.6.27-rc4-ia32-08-20-08.patch at the
moment. Speaking of debug_lock()...:
You use spin_trylock_irqsave((spinlock_t *)&rlock->lock, rlock->flags)
in there. Minor nit: The pointer type cast is unnecessary.
Major problem: rlock->flags is wrong in this call. Use an on-stack
flags variable for the initial spin_trylock_irqsave. Ditto in the
following call of spin_trylock_irqsave.
Next major problem with debug_lock() and debug_unlock(): The reference
counting doesn't work. You need an atomic_t counter. Have a look at
the struct kref accessors for example, or even make use of the kref API.
Or if it isn't feasible to fix with atomic_t, add a second spinlock to
rlock_t to ensure integrity of .count (and of the .processor if necessary).
Furthermore, I have doubts about the loop which is entered by CPU B
while CPU A holds the rlock. You are fully aware that atomic_read(a) &&
!atomic_read(b) in its entirety is not atomic, I hope.
All this aside, I don't see *anything* in debug_lock and _unlock which
would necessitate volatile. Well, volatile might have papered over some
of these bugs.
PS:
Try to cut down on #if/#endif clutter. It should be possible to reduce
them at least in .c files; .h are a different matter. For example,
#if MDB_DEBUG_DEBUGGER
DBGPrint("something");
#endif
can be trivially reduced to
dbg_mdb_printk("something");
where dbg_mdb_printk() is defined as an inline function which does
nothing when MDB_DEBUG_DEBUGGER is false.
PS2:
Why are there this many debug printks anyway?
--
Stefan Richter
-=====-==--- =--- =-=-=
http://arcgraph.de/sr/
--
