Date: Sat, 22 Dec 2012 23:20:05 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Attilio Rao <attilio@freebsd.org> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, Bruce Evans <brde@optusnet.com.au> Subject: Re: svn commit: r244582 - head/sys/kern Message-ID: <20121222230402.P1765@besplex.bde.org> In-Reply-To: <CAJ-FndCazK%2BE9w8fGvO4uou-sCOgnf8cVFu0nzXjUeTu2RYF4w@mail.gmail.com> References: <201212220937.qBM9bYQK050680@svn.freebsd.org> <20121222204409.V1410@besplex.bde.org> <CAJ-FndCazK%2BE9w8fGvO4uou-sCOgnf8cVFu0nzXjUeTu2RYF4w@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sat, 22 Dec 2012, Attilio Rao wrote:
> On Sat, Dec 22, 2012 at 10:51 AM, Bruce Evans <brde@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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20121222230402.P1765>