From owner-freebsd-current@FreeBSD.ORG Thu Dec 1 21:27:47 2011 Return-Path: Delivered-To: freebsd-current@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 20D44106564A; Thu, 1 Dec 2011 21:27:47 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from cyrus.watson.org (cyrus.watson.org [65.122.17.42]) by mx1.freebsd.org (Postfix) with ESMTP id ECDD78FC08; Thu, 1 Dec 2011 21:27:46 +0000 (UTC) Received: from bigwig.baldwin.cx (bigwig.baldwin.cx [96.47.65.170]) by cyrus.watson.org (Postfix) with ESMTPSA id 8785A46B0D; Thu, 1 Dec 2011 16:27:46 -0500 (EST) Received: from jhbbsd.localnet (unknown [209.249.190.124]) by bigwig.baldwin.cx (Postfix) with ESMTPSA id 16472B94B; Thu, 1 Dec 2011 16:27:46 -0500 (EST) From: John Baldwin To: Andriy Gapon Date: Thu, 1 Dec 2011 15:53:34 -0500 User-Agent: KMail/1.13.5 (FreeBSD/8.2-CBSD-20110714-p8; KDE/4.5.5; amd64; ; ) References: <20111113083215.GV50300@deviant.kiev.zoral.com.ua> <201112011349.50502.jhb@freebsd.org> <4ED7E6B0.30400@FreeBSD.org> In-Reply-To: <4ED7E6B0.30400@FreeBSD.org> MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Message-Id: <201112011553.34432.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.2.7 (bigwig.baldwin.cx); Thu, 01 Dec 2011 16:27:46 -0500 (EST) Cc: freebsd-current@freebsd.org Subject: Re: Stop scheduler on panic X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 01 Dec 2011 21:27:47 -0000 On Thursday, December 01, 2011 3:42:24 pm Andriy Gapon wrote: > on 01/12/2011 20:49 John Baldwin said the following: > > On Thursday, December 01, 2011 11:59:10 am Andriy Gapon wrote: > >> > >> [cc list trimmed] > >> > >> on 21/11/2011 18:32 John Baldwin said the following: > >>> On Friday, November 18, 2011 4:59:32 pm Andriy Gapon wrote: > >>>> on 17/11/2011 23:38 John Baldwin said the following: > >>>>> On Thursday, November 17, 2011 4:35:07 pm John Baldwin wrote: > >>>>>> Hmmm, you could also make critical_exit() not perform deferred preemptions > >>>>>> if SCHEDULER_STOPPED? That would fix the recursion and still let the > >>>>>> preemption "work" when resuming from the debugger? > >> > >> > >> Just to clarify, probably you are actually suggesting to not perform deferred > >> preemptions if kdb_active == TRUE. Because that's where we get the recursion (via > >> kdb_switch). > >> > >> I think that if we get into the mi_switch in a state where !kdb_active && > >> SCHEDULER_STOPPED(), then we probably should just - I don't know - panic again? > >> > >> [the following is preserved for context] > > > > Hmmm. I'd be tempted to just ignore pending preemptions anytime > > SCHEDULER_STOPPED() is true. If it's stopped for a reason other than being > > in the debugger (e.g. panic), I'd rather make a best effort at getting a dump > > than panic again. > > Yep, me too. It's just that I assumed that ending up at mi_switch in the panic > thread/context meant that something had gone very wrong already. But I am not > sure if this was a valid assumption. > > Returning to critical_exit, what do you think about the following patch? > I guess that it could be committed independently of / before the > SCHEDULER_STOPPED thing. > > commit ee3d1a04985e86911a68d854439ae8c5429b7bd5 > Author: Andriy Gapon > Date: Thu Dec 1 18:53:36 2011 +0200 > > critical_exit: ignore td_owepreempt if kdb_active > > calling mi_switch in such a context result in a recursion via > kdb_switch > > diff --git a/sys/kern/kern_switch.c b/sys/kern/kern_switch.c > index 93cbf7b..885dc22 100644 > --- a/sys/kern/kern_switch.c > +++ b/sys/kern/kern_switch.c > @@ -200,7 +200,7 @@ critical_exit(void) > > if (td->td_critnest == 1) { > td->td_critnest = 0; > - if (td->td_owepreempt) { > + if (td->td_owepreempt && !kdb_active) { > td->td_critnest = 1; > thread_lock(td); > td->td_critnest--; I think this is fine, but I'd probably change this to SCHEDULER_STOPPED() in the SCHEDULER_STOPPED() patch. > Would it make sense wrap kdb_active check with __predict_false? I don't think so. -- John Baldwin