[ANNOUNCE] mdb-2.6.27-rc2-ia32-08-07-08.patch

[ANNOUNCE] mdb-2.6.27-rc2-ia32-08-07-08.patch

[jmerkey]

The mdb-rc2 patch was posted this morning with the changes for a modular
kernel debugger using kprobes.

ftp://ftp.wolfmountaingroup.org/pub/mdb/mdb-2.6.27-rc2-ia32-08-07-08.patch

Jeffrey Vernon Merkey

Re: [ANNOUNCE] mdb-2.6.27-rc2-ia32-08-07-08.patch

[Stefan Richter]

jmerkey@wolfmountaingroup.com wrote:
> The mdb-rc2 patch was posted this morning with the changes for a modular
> kernel debugger using kprobes.
> 
> ftp://ftp.wolfmountaingroup.org/pub/mdb/mdb-2.6.27-rc2-ia32-08-07-08.patch
> 
> Jeffrey Vernon Merkey


Quoting from this patch:

> +typedef struct _RLOCK 
> +{
> +#if defined(CONFIG_SMP)
> +    spinlock_t lock;
> +#endif
> +    unsigned long flags;
> +    unsigned long processor;
> +    unsigned long count;    
> +} rlock_t;

Is this something along the lines of a counting semaphore?  As far as I
understand its accessor functions, an rlock
  - can be taken by one CPU multiple times,
  - will block the other CPUs as long as the first CPU hasn't unlocked
    the rlock as many times as it locked it.

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)
> +   {
> +      rlock->count++;
> +      retCode = 1;
> +   }
> +   else
> +   {
> +      spin_lock_irqsave((spinlock_t *)&rlock->lock, rlock->flags);
> +      rlock->processor = proc;
> +      retCode = 0;
> +   }
> +   return retCode;
> +#else
> +   return 0;
> +#endif
> +}

In general, a lot can happen between the access to
rlock->lock.raw_lock.slock and the access to rlock->processor.

Even rlock->count++ in itself can go wrong.

The usage of "volatile" here looks a lot like DWIM to me.


> +volatile unsigned long debuggerActive;
> +volatile unsigned long ProcessorHold[MAX_PROCESSORS]; 
> +volatile unsigned long ProcessorState[MAX_PROCESSORS]; 
> +volatile unsigned long ProcessorMode[MAX_PROCESSORS]; 

Are these datasets shared between CPUs and do you access them without
lock protection?  (It looks like that from a quick glance.)  If yes,
instead of the volatile qualifier, can't you use memory barriers in
order to define the access semantics more clearly (and more effective)
than volatile can?

If there are interdependencies in these datasets, with which mechanisms
if not locks do you serialize critical sections?


> +void MDBInitializeDebugger(void)
> +{
...
> +   for (i=0; i < MAX_PROCESSORS; i++)
> +   {
> +      BreakMask[i] = 0;
> +      ProcessorHold[i] = 0; 
> +      ProcessorState[i] = 0; 
> +      ProcessorMode[i] = 0; 
> +   }

Not necessary, because static (and extern) variables are already
implicitly initialized to zero.
-- 
Stefan Richter
-=====-==--- =--- --===
http://arcgraph.de/sr/

Re: [ANNOUNCE] mdb-2.6.27-rc2-ia32-08-07-08.patch

[jmerkey]

> Stefan Richter wrote:
>> jmerkey@wolfmountaingroup.com wrote:
>>> ftp://ftp.wolfmountaingroup.org/pub/mdb/mdb-2.6.27-rc2-ia32-08-07-08.patch
> ...
>>> +typedef struct _RLOCK
>>> +{
>>> +#if defined(CONFIG_SMP)
>>> +    spinlock_t lock;
>>> +#endif
>>> +    unsigned long flags;
>>> +    unsigned long processor;
>>> +    unsigned long count;
>>> +} rlock_t;
> ...
>> 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:
> ...
>>> +   if (rlock->lock.raw_lock.slock && rlock->processor == proc)
>
> Correction:  They _are_ not portable.  Look at the first hit in
> http://lxr.linux.no/linux+v2.6.26/+code=raw_spinlock_t .


OK.  One more to fix.  I have to be able to take nested page faults inside
the debugger in the event I am debugging a page fault handler or paging
event and someone's working set routine crashes.

rspin locks are for these types of cases -- so if I fault on the same
processor I took the lock on it just bumps a counter -- yes, it is atomic
and SMP safe to do it this way.  You are correct that its non-portable
but the file this is in is named mdb-IA32 and is not meant as portable to
anything other than intel anyway.

As for coding style and misuse of a raw lock structure, I agree with you
completely, and I will look for a better way to do this without exposing
this structure.

Jeff

> --
> Stefan Richter
> -=====-==--- =--- --===
> http://arcgraph.de/sr/
>

Re: [ANNOUNCE] mdb-2.6.27-rc2-ia32-08-07-08.patch

[Stefan Richter]

Stefan Richter wrote:
> jmerkey@wolfmountaingroup.com wrote:
>> ftp://ftp.wolfmountaingroup.org/pub/mdb/mdb-2.6.27-rc2-ia32-08-07-08.patch
...
>> +typedef struct _RLOCK 
>> +{
>> +#if defined(CONFIG_SMP)
>> +    spinlock_t lock;
>> +#endif
>> +    unsigned long flags;
>> +    unsigned long processor;
>> +    unsigned long count;    
>> +} rlock_t;
...
> 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:
...
>> +   if (rlock->lock.raw_lock.slock && rlock->processor == proc)

Correction:  They _are_ not portable.  Look at the first hit in
http://lxr.linux.no/linux+v2.6.26/+code=raw_spinlock_t .
-- 
Stefan Richter
-=====-==--- =--- --===
http://arcgraph.de/sr/

Re: [ANNOUNCE] mdb-2.6.27-rc2-ia32-08-07-08.patch

[jmerkey]

>
> If there are interdependencies in these datasets, with which mechanisms
> if not locks do you serialize critical sections?
>
>
>> +void MDBInitializeDebugger(void)
>> +{
> ...
>> +   for (i=0; i < MAX_PROCESSORS; i++)
>> +   {
>> +      BreakMask[i] = 0;
>> +      ProcessorHold[i] = 0;
>> +      ProcessorState[i] = 0;
>> +      ProcessorMode[i] = 0;
>> +   }
>
> Not necessary, because static (and extern) variables are already
> implicitly initialized to zero.
> --
> Stefan Richter
> -=====-==--- =--- --===
> http://arcgraph.de/sr/


Another M$ legacy relict. On Microsoft C compilers (older ones) failure to
initialize global data produces undefined results since they do not always
zero out memory and global data -- they still do not in all cases.

I will remove this a well.  Still more checkpatch.pl stuff for me to do.  
am down to about 3000+ noisy messages now ...


Jeff

>

Re: [ANNOUNCE] mdb-2.6.27-rc2-ia32-08-07-08.patch

[jmerkey]

> jmerkey@wolfmountaingroup.com wrote:
>> rspin locks are for these types of cases -- so if I fault on the same
>> processor I took the lock on it just bumps a counter -- yes, it is
>> atomic
>> and SMP safe to do it this way.
>
> Only if all contexts which take rlocks are not preemptible.  Which I
> don't know whether they are; I'm just a driver guy.  You use
> spin_lock_irqsave() rather than plain spin_lock() though, which
> indicates that you want to be able to take the locks from preemptible
> contexts too.  In that case, your accessors are subtly buggy.
> --
> Stefan Richter
> -=====-==--- =--- --===
> http://arcgraph.de/sr/
>

check mdb-main.c -- I disable preemption before rspin_lock is attempted. 
Since the only processor which sets the proc number does do inside the
spin lock, and the other processors only read it, unless memory is
corrupted or the machine is severely broken, its SMP safe to this.

I use the debug_lock rspin_lock to prevent the other processors from
entering the debugger command console when one of them has the console...

Jeff

Re: [ANNOUNCE] mdb-2.6.27-rc2-ia32-08-07-08.patch

[Stefan Richter]

jmerkey@wolfmountaingroup.com wrote:
> rspin locks are for these types of cases -- so if I fault on the same
> processor I took the lock on it just bumps a counter -- yes, it is atomic
> and SMP safe to do it this way.

Only if all contexts which take rlocks are not preemptible.  Which I 
don't know whether they are; I'm just a driver guy.  You use 
spin_lock_irqsave() rather than plain spin_lock() though, which 
indicates that you want to be able to take the locks from preemptible 
contexts too.  In that case, your accessors are subtly buggy.
-- 
Stefan Richter
-=====-==--- =--- --===
http://arcgraph.de/sr/

Re: [ANNOUNCE] mdb-2.6.27-rc2-ia32-08-07-08.patch

[Stefan Richter]

jmerkey@wolfmountaingroup.com wrote:
>> jmerkey@wolfmountaingroup.com wrote:
>>> rspin locks are for these types of cases -- so if I fault on the same
>>> processor I took the lock on it just bumps a counter -- yes, it is
>>> atomic
>>> and SMP safe to do it this way.
>> Only if all contexts which take rlocks are not preemptible.
[...]
> check mdb-main.c -- I disable preemption before rspin_lock is attempted. 
> Since the only processor which sets the proc number does do inside the
> spin lock, and the other processors only read it, unless memory is
> corrupted or the machine is severely broken, its SMP safe to this.

Then it is recommendable that you document the call context requirements 
at the functions.  And you can and IMO should drop the _irq_save and 
_irq_restore from the spinlock accessors in the rlock accessors.  And 
drop the volatile qualifier of the rlock accessor argument while you are 
at it.

I see that you are calling save_flags/ restore_flags in 
mdb-main.c::mdb().  These are marked as deprecated.  Would 
local_irq_save/ local_irq_restore be correct at these places?
-- 
Stefan Richter
-=====-==--- =--- --===
http://arcgraph.de/sr/

Re: [ANNOUNCE] mdb-2.6.27-rc2-ia32-08-07-08.patch

[Stefan Richter]

I wrote:
> jmerkey@wolfmountaingroup.com wrote:
>> check mdb-main.c -- I disable preemption before rspin_lock is 
>> attempted.
...
> Then it is recommendable that you document the call context requirements 
> at the functions.

An alternative would of course be to establish the non-preemptible 
context from inside the lock accessors, either redundantly to what the 
call site is doing, or instead of doing it at the call site if that can 
be arranged.
-- 
Stefan Richter
-=====-==--- =--- --===
http://arcgraph.de/sr/

Re: [ANNOUNCE] mdb-2.6.27-rc2-ia32-08-07-08.patch

[jmerkey]

> jmerkey@wolfmountaingroup.com wrote:
>>> jmerkey@wolfmountaingroup.com wrote:
>>>> rspin locks are for these types of cases -- so if I fault on the same
>>>> processor I took the lock on it just bumps a counter -- yes, it is
>>>> atomic
>>>> and SMP safe to do it this way.
>>> Only if all contexts which take rlocks are not preemptible.
> [...]
>> check mdb-main.c -- I disable preemption before rspin_lock is attempted.
>> Since the only processor which sets the proc number does do inside the
>> spin lock, and the other processors only read it, unless memory is
>> corrupted or the machine is severely broken, its SMP safe to this.
>
> Then it is recommendable that you document the call context requirements
> at the functions.  And you can and IMO should drop the _irq_save and
> _irq_restore from the spinlock accessors in the rlock accessors.  And
> drop the volatile qualifier of the rlock accessor argument while you are
> at it.
>
> I see that you are calling save_flags/ restore_flags in
> mdb-main.c::mdb().  These are marked as deprecated.  Would
> local_irq_save/ local_irq_restore be correct at these places?


You have sharp eyes.  Yes, you are correct.  added to the list of
corrections.

Jeff

> --
> Stefan Richter
> -=====-==--- =--- --===
> http://arcgraph.de/sr/
>

Re: [ANNOUNCE] mdb-2.6.27-rc2-ia32-08-07-08.patch

[jmerkey]

> I wrote:
>> jmerkey@wolfmountaingroup.com wrote:
>>> check mdb-main.c -- I disable preemption before rspin_lock is
>>> attempted.
> ...
>> Then it is recommendable that you document the call context requirements
>> at the functions.
>
> An alternative would of course be to establish the non-preemptible
> context from inside the lock accessors, either redundantly to what the
> call site is doing, or instead of doing it at the call site if that can
> be arranged.
> --
> Stefan Richter
> -=====-==--- =--- --===
> http://arcgraph.de/sr/
>

It needs to get turned off right away, well before we get to the locks.

Keff

Re: [ANNOUNCE] mdb-2.6.27-rc2-ia32-08-07-08.patch

[Parag Warudkar]

 <jmerkey <at> wolfmountaingroup.com> writes:

> 
> The mdb-rc2 patch was posted this morning with the changes for a modular
> kernel debugger using kprobes.
> 
> ftp://ftp.wolfmountaingroup.org/pub/mdb/mdb-2.6.27-rc2-ia32-08-07-08.patch

This one fails with build error - send_IPI_mask_bitmask is not exported but used
by mdb.

Parag

Re: [ANNOUNCE] mdb-2.6.27-rc2-ia32-08-07-08.patch

[Parag Warudkar]

Jeff,

This definitely looks interesting and it even worked for me - breaking
in couple of times, trying various commands and resuming back to X all
works. Never tried a kernel debugger before but most things were
obvious after I figured out that I need to be on the VT to be able to
press break and be dropped into the debugger.

That said, it triggers soft lockups on all the suspended CPUs - not a
big deal and probably expected, but I guess that can be cured by
touching the soft lockup watch dog or something?

Thanks

Parag

Re: [ANNOUNCE] mdb-2.6.27-rc2-ia32-08-07-08.patch

[jmerkey]

> Jeff,
>
> This definitely looks interesting and it even worked for me - breaking
> in couple of times, trying various commands and resuming back to X all
> works. Never tried a kernel debugger before but most things were
> obvious after I figured out that I need to be on the VT to be able to
> press break and be dropped into the debugger.
>
> That said, it triggers soft lockups on all the suspended CPUs - not a
> big deal and probably expected, but I guess that can be cured by
> touching the soft lockup watch dog or something?

Yep.

It does with this kprobes interface, I noticed it last night.  I will run
this down.  I am touching the watchdog in the keyboard handler.

Jeff

>
> Thanks
>
> Parag
>

Re: [ANNOUNCE] mdb-2.6.27-rc2-ia32-08-07-08.patch

[jmerkey]

>> Jeff,
>>
>> This definitely looks interesting and it even worked for me - breaking
>> in couple of times, trying various commands and resuming back to X all
>> works. Never tried a kernel debugger before but most things were
>> obvious after I figured out that I need to be on the VT to be able to
>> press break and be dropped into the debugger.
>>
>> That said, it triggers soft lockups on all the suspended CPUs - not a
>> big deal and probably expected, but I guess that can be cured by
>> touching the soft lockup watch dog or something?
>
> Yep.
>
> It does with this kprobes interface, I noticed it last night.  I will run
> this down.  I am touching the watchdog in the keyboard handler.
>
> Jeff


Reproduced it.   It just spits out the noisy message but the system
continues and the processors resume running.  This is is an esoteric
issue, but I will run it down.  It does not happen on kernel versions
prior to 2.6.26.

Jeff

>
>>
>> Thanks
>>
>> Parag
>>
>
>
>

Re: [ANNOUNCE] mdb-2.6.27-rc2-ia32-08-07-08.patch

[Jeremy Fitzhardinge]

Stefan Richter wrote:
> jmerkey@wolfmountaingroup.com wrote:
>   
>> The mdb-rc2 patch was posted this morning with the changes for a modular
>> kernel debugger using kprobes.
>>
>> ftp://ftp.wolfmountaingroup.org/pub/mdb/mdb-2.6.27-rc2-ia32-08-07-08.patch
>>
>> Jeffrey Vernon Merkey
>>     
>
>
> Quoting from this patch:
>
>   
>> +typedef struct _RLOCK 
>> +{
>> +#if defined(CONFIG_SMP)
>> +    spinlock_t lock;
>> +#endif
>> +    unsigned long flags;
>> +    unsigned long processor;
>> +    unsigned long count;    
>> +} rlock_t;
>>     
>
> Is this something along the lines of a counting semaphore?  As far as I
> understand its accessor functions, an rlock
>   - can be taken by one CPU multiple times,
>   - will block the other CPUs as long as the first CPU hasn't unlocked
>     the rlock as many times as it locked it.
>
> 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;
		...
	}


    J

Re: [ANNOUNCE] mdb-2.6.27-rc2-ia32-08-07-08.patch

[Stefan Richter]

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/

Re: [ANNOUNCE] mdb-2.6.27-rc2-ia32-08-07-08.patch

[jmerkey]

> 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.


.... On linux it does -- on another OS it does something quite different. 
I changed that case last night when I added spin)is_locked calls.   Is
there
to do a "debugger bust spinlocks" if the system ever hangs in the
debugger.  probably should code it different.

Jeff

:-)

Jeff

> --
> Stefan Richter
> -=====-==--- =--- -=--=
> http://arcgraph.de/sr/
>