From owner-svn-src-head@FreeBSD.ORG Sat Dec 22 12:20:09 2012 Return-Path: Delivered-To: svn-src-head@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id 5270A4D0; Sat, 22 Dec 2012 12:20:09 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail36.syd.optusnet.com.au (mail36.syd.optusnet.com.au [211.29.133.76]) by mx1.freebsd.org (Postfix) with ESMTP id D75018FC0A; Sat, 22 Dec 2012 12:20:08 +0000 (UTC) Received: from c122-106-175-26.carlnfd1.nsw.optusnet.com.au (c122-106-175-26.carlnfd1.nsw.optusnet.com.au [122.106.175.26]) by mail36.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id qBMCK5ko020370 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Sat, 22 Dec 2012 23:20:06 +1100 Date: Sat, 22 Dec 2012 23:20:05 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Attilio Rao Subject: Re: svn commit: r244582 - head/sys/kern In-Reply-To: Message-ID: <20121222230402.P1765@besplex.bde.org> References: <201212220937.qBM9bYQK050680@svn.freebsd.org> <20121222204409.V1410@besplex.bde.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.0 cv=CJhyxmXD c=1 sm=1 a=psoCjdI5atsA:10 a=kj9zAlcOel0A:10 a=PO7r1zJSAAAA:8 a=JzwRw_2MAAAA:8 a=M4roAWbnUW4A:10 a=I29nFAbqzGM88kPPTRwA:9 a=CjuIK1q_8ugA:10 a=bxQHXO5Py4tHmhUgaywp5w==:117 Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, Bruce Evans X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 22 Dec 2012 12:20:09 -0000 On Sat, 22 Dec 2012, Attilio Rao wrote: > On Sat, Dec 22, 2012 at 10:51 AM, Bruce Evans 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 >>> +#include >>> #include >>> #include >>> #include >>> @@ -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