Re: [ANNOUNCE] mdb: Merkey's Linux Kernel Debugger 2.6.27-rc4 released

[Stefan Richter <stefanr@s5r6.in-berlin.de>]

jmerkey@wolfmountaingroup.com wrote:
> It's simple to reproduce.  Take away the volatile declaration for the
> rlock_t structure in mdb-ia32.c (rlock_t debug_lock) in all code
> references and watch the thing lock up in SMP with multiple processors in
> the debugger each stuck with their own local copy of debug_lock.

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/