svn commit: r244582 - head/sys/kern

[Bruce Evans]

On Sat, 22 Dec 2012, Attilio Rao wrote:

> On Sat, Dec 22, 2012 at 10:51 AM, Bruce Evans <brde at optusnet.com.au> wrote:
>> On Sat, 22 Dec 2012, Attilio Rao wrote:
>>
>>> Log:
>>>  Fixup r240424: On entering KDB backends, the hijacked thread to run
>>>  interrupt context can still be idlethread. At that point, without the
>>>  panic condition, it can still happen that idlethread then will try to
>>>  acquire some locks to carry on some operations.
>>>
>>>  Skip the idlethread check on block/sleep lock operations when KDB is
>>>  active.
>>
>> This seems backwards to me.  It is an error to go near normal locking
>> code when kdb is active.
>
> I completely agree, but this is not what happens nowadays with FreeBSD kernel.
> In my view, KDB should not call into normal code, but in special
> wrappers which skip locking entirely, in particular because other cpus
> are stopped, so there is no race going on.
> However, this requires a big change and as long as this doesn't happen
> we need to stuck with similar hacks.

But this sort of hack only breaks accidental detection of a bug (maybe
the bug causes deadlock or data corruption soon).  The type of hack
that helps is 'if (kdb_active) skip_locking();' deep in code that
shouldn't even be called if kdb is active.  Here it is 'if (kdb_active)
skip_checking();'

>>> Modified: head/sys/kern/kern_lock.c
>>>
>>> ==============================================================================
>>> --- head/sys/kern/kern_lock.c   Sat Dec 22 07:48:09 2012        (r244581)
>>> +++ head/sys/kern/kern_lock.c   Sat Dec 22 09:37:34 2012        (r244582)
>>> @@ -35,6 +35,7 @@
>>> __FBSDID("$FreeBSD$");
>>>
>>> #include <sys/param.h>
>>> +#include <sys/kdb.h>
>>> #include <sys/ktr.h>
>>> #include <sys/lock.h>
>>> #include <sys/lock_profile.h>
>>> @@ -477,7 +478,7 @@ __lockmgr_args(struct lock *lk, u_int fl
>>>         KASSERT((flags & LK_INTERLOCK) == 0 || ilk != NULL,
>>>             ("%s: LK_INTERLOCK passed without valid interlock @ %s:%d",
>>>             __func__, file, line));
>>> -       KASSERT(!TD_IS_IDLETHREAD(curthread),
>>> +       KASSERT(kdb_active != 0 || !TD_IS_IDLETHREAD(curthread),
>>>             ("%s: idle thread %p on lockmgr %s @ %s:%d", __func__,
>>> curthread,
>>>             lk->lock_object.lo_name, file, line));
>>
>>
>> This is backwards from:
>>
>>         KASSERT(kdb_active == 0);
>>
>> which makes it fatal for any thread to call here.
>
> I do not understand. For kdb_active == 0 it still checks for
> IDLETHREAD if it is not idlethread it doesn't panic, it panics
> otherwise, which seems the right to me.

I just mean that the correct kdb_active KASSERT() is independent of the
idlethread one.  It should also have a different message.  I forgot to
provide a message.

Bruce