From owner-freebsd-current@FreeBSD.ORG Thu Nov 17 21:38:04 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 F0DB51065675; Thu, 17 Nov 2011 21:38:04 +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 B6C2E8FC13; Thu, 17 Nov 2011 21:38:04 +0000 (UTC) Received: from bigwig.baldwin.cx (66.111.2.69.static.nyinternet.net [66.111.2.69]) by cyrus.watson.org (Postfix) with ESMTPSA id 66B4A46B0A; Thu, 17 Nov 2011 16:38:04 -0500 (EST) Received: from jhbbsd.localnet (unknown [209.249.190.124]) by bigwig.baldwin.cx (Postfix) with ESMTPSA id C50E38A050; Thu, 17 Nov 2011 16:38:03 -0500 (EST) From: John Baldwin To: Andriy Gapon Date: Thu, 17 Nov 2011 16:38:03 -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> <4EC563BB.60209@FreeBSD.org> <201111171635.07278.jhb@freebsd.org> In-Reply-To: <201111171635.07278.jhb@freebsd.org> MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Message-Id: <201111171638.03208.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.2.6 (bigwig.baldwin.cx); Thu, 17 Nov 2011 16:38:04 -0500 (EST) Cc: Kostik Belousov , Alexander Motin , 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, 17 Nov 2011 21:38:05 -0000 On Thursday, November 17, 2011 4:35:07 pm John Baldwin wrote: > On Thursday, November 17, 2011 2:42:51 pm Andriy Gapon wrote: > > on 17/11/2011 21:09 John Baldwin said the following: > > > On Thursday, November 17, 2011 11:58:03 am Andriy Gapon wrote: > > >> on 17/11/2011 18:37 John Baldwin said the following: > > >>> On Thursday, November 17, 2011 4:47:42 am Andriy Gapon wrote: > > >>>> on 17/11/2011 10:34 Andriy Gapon said the following: > > >>>>> on 17/11/2011 10:15 Kostik Belousov said the following: > > >>>>>> I have the following change for eons on my test boxes. Without it, > > >>>>>> I simply cannot get _any_ dump. > > >>>>>> > > >>>>>> diff --git a/sys/cam/cam_xpt.c b/sys/cam/cam_xpt.c > > >>>>>> index 10b89c7..a38e42f 100644 > > >>>>>> --- a/sys/cam/cam_xpt.c > > >>>>>> +++ b/sys/cam/cam_xpt.c > > >>>>>> @@ -4230,7 +4230,7 @@ xpt_done(union ccb *done_ccb) > > >>>>>> TAILQ_INSERT_TAIL(&cam_simq, sim, links); > > >>>>>> mtx_unlock(&cam_simq_lock); > > >>>>>> sim->flags |= CAM_SIM_ON_DONEQ; > > >>>>>> - if (first) > > >>>>>> + if (first && panicstr == NULL) > > >>>>>> swi_sched(cambio_ih, 0); > > >>>>>> } > > >>>>>> } > > >>>>> > > >>>>> I think that this (or similar) change should go into the patch and the tree. > > >>>>> > > >>>> > > >>>> And, BTW, I still would like to do something like the following (perhaps with > > >>>> td_oncpu = NOCPU and td_flags &= ~TDF_NEEDRESCHED also moved to the common code): > > >>>> > > >>>> Index: sys/kern/sched_ule.c > > >>>> =================================================================== > > >>>> --- sys/kern/sched_ule.c (revision 227608) > > >>>> +++ sys/kern/sched_ule.c (working copy) > > >>>> @@ -1790,7 +1790,6 @@ sched_switch(struct thread *td, struct thread *new > > >>>> td->td_oncpu = NOCPU; > > >>>> if (!(flags & SW_PREEMPT)) > > >>>> td->td_flags &= ~TDF_NEEDRESCHED; > > >>>> - td->td_owepreempt = 0; > > >>>> tdq->tdq_switchcnt++; > > >>>> /* > > >>>> * The lock pointer in an idle thread should never change. Reset it > > >>>> Index: sys/kern/kern_synch.c > > >>>> =================================================================== > > >>>> --- sys/kern/kern_synch.c (revision 227608) > > >>>> +++ sys/kern/kern_synch.c (working copy) > > >>>> @@ -406,6 +406,8 @@ mi_switch(int flags, struct thread *newtd) > > >>>> ("mi_switch: switch must be voluntary or involuntary")); > > >>>> KASSERT(newtd != curthread, ("mi_switch: preempting back to ourself")); > > >>>> > > >>>> + td->td_owepreempt = 0; > > >>>> + > > >>>> /* > > >>>> * Don't perform context switches from the debugger. > > >>>> */ > > >>>> Index: sys/kern/sched_4bsd.c > > >>>> =================================================================== > > >>>> --- sys/kern/sched_4bsd.c (revision 227608) > > >>>> +++ sys/kern/sched_4bsd.c (working copy) > > >>>> @@ -940,7 +940,6 @@ sched_switch(struct thread *td, struct thread *new > > >>>> td->td_lastcpu = td->td_oncpu; > > >>>> if (!(flags & SW_PREEMPT)) > > >>>> td->td_flags &= ~TDF_NEEDRESCHED; > > >>>> - td->td_owepreempt = 0; > > >>>> td->td_oncpu = NOCPU; > > >>>> > > >>>> /* > > >>>> > > >>>> Does anybody see any potential problems with such a change? > > >>> > > >>> Hmm, does this mean the preemption will be lost if you break into the > > >>> debugger and continue in the non-panic case? > > >> > > >> Not sure which exact scenario you have in mind. > > >> Please note that the above diff just moves resetting of td_owepreempt to an > > >> earlier place. As far as I can see there are no checks of td_owepreempt value > > >> between the new place and the old places. > > > > > > I'm worried that you are clearing td_owepreempt even in cases where a context > > > switch is not performed. So say you enter DDB with td_owepreempt set and that > > > DDB bails on a context switch. With your change it will now clear td_owepreempt > > > and "lose" the preemption. > > > > > > > And without the change we get the recursion and double-fault because of > > kdb_switch -> thread_unlock -> spinlock_exit -> critical_exit -> > > mi_switch in this case ? > > > > BTW, it is my opinion that we really should not let the debugger code call > > mi_switch for any reason. > > 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? Or you could let the debugger run in a permament critical section (though perhaps that breaks too many other things like debugger routines that try to use locks like the 'kill' command (which is useful!)). Effectively what you are trying to do is having the debugger run in a critical section until the debugger is exited. It would be cleanest to let it run that way explicitly if possible since then you don't have to catch as many edge cases. -- John Baldwin