From owner-freebsd-current@FreeBSD.ORG Thu Nov 17 21:35:09 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 6F21D106564A; Thu, 17 Nov 2011 21:35:09 +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 2F7608FC12; Thu, 17 Nov 2011 21:35:09 +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 832A946B06; Thu, 17 Nov 2011 16:35:08 -0500 (EST) Received: from jhbbsd.localnet (unknown [209.249.190.124]) by bigwig.baldwin.cx (Postfix) with ESMTPSA id 264B98A050; Thu, 17 Nov 2011 16:35:08 -0500 (EST) From: John Baldwin To: Andriy Gapon Date: Thu, 17 Nov 2011 16:35:07 -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> <201111171409.37629.jhb@freebsd.org> <4EC563BB.60209@FreeBSD.org> In-Reply-To: <4EC563BB.60209@FreeBSD.org> MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Message-Id: <201111171635.07278.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:35:08 -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:35:09 -0000 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? -- John Baldwin