From owner-freebsd-arch@FreeBSD.ORG Fri Oct 1 07:57:48 2004 Return-Path: Delivered-To: freebsd-arch@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 21D0016A4CE for ; Fri, 1 Oct 2004 07:57:48 +0000 (GMT) Received: from apollo.backplane.com (apollo.backplane.com [216.240.41.2]) by mx1.FreeBSD.org (Postfix) with ESMTP id CDDB043D53 for ; Fri, 1 Oct 2004 07:57:47 +0000 (GMT) (envelope-from dillon@apollo.backplane.com) Received: from apollo.backplane.com (localhost [127.0.0.1]) i917vkvA017412; Fri, 1 Oct 2004 00:57:46 -0700 (PDT) (envelope-from dillon@apollo.backplane.com) Received: (from dillon@localhost) by apollo.backplane.com (8.12.9p2/8.12.9/Submit) id i917vjac017409; Fri, 1 Oct 2004 00:57:45 -0700 (PDT) (envelope-from dillon) Date: Fri, 1 Oct 2004 00:57:45 -0700 (PDT) From: Matthew Dillon Message-Id: <200410010757.i917vjac017409@apollo.backplane.com> To: Stephan Uphoff References: <1096610130.21577.219.camel@palm.tree.com> cc: Peter Holm cc: Julian Elischer cc: "freebsd-arch@freebsd.org" Subject: Re: sched_switch (sched_4bsd) may be preempted X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.1 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 01 Oct 2004 07:57:48 -0000 I would put the entire scheduler core in a critical section, not just the part you think needs to be there. It's just too critical a subsystem to be able to make operational assumptions of that nature in. It is what I do in the DragonFly LWKT core, BTW, and crazy as I am I would never even consider trying to move the critical section further in. I would not reset TDP_OWEPREEMPT there. If I understand its function correctly you need to leave it intact in order to detect preemption request races against the scheduler. Since at that point newtd may be non-NULL and thus not cause another scheduling queue check to be made before the next switch, you cannot safely clear the flag where you are clearing it. If you want to optimize operation of the flag I recommend storing the preempting entity's priority at the same point where TDP_OWEPREEMPT is set and then do a quick priority comparison in critical_exit() to avoid unnecessary mi_switch()'s. I would also not put the TDP_OWEPREEMPT flag in the thread structure. It really belongs in the globaldata structure so it remains properly intact through the thread switch, else you have more potential races even while *in* the critical section. Your TDP_OWEPREEMPT flag has almost exactly the same function as DFly's gd_reqflags word and I spent a long time thinking through where I would store it, and came to the conclusion that the globaldata structure was the best place. e.g. so FreeBSD's critical_exit() code would become this (note: I might have the priority comparison backwards, I forget how FBsd does it): if (td->td_critnest == 1) { #ifdef PREEMPTION mtx_assert(&sched_lock, MA_NOTOWNED); if (gd->gd_pflags & GDP_OWEPREEMPT) { <<< CHG TO gd gd->gd_pflags &= ~GDP_OWEPREEMPT; <<< CHG TO gd if (gd->gd_preempt_priority < td->td_priority) { << ADD mtx_lock_spin(&sched_lock); mi_switch(SW_INVOL, NULL); mtx_unlock_spin(&sched_lock); } } #endif td->td_critnest = 0; cpu_critical_exit(td); And the code which sets GDP_OWEPREEMPT would become this: [checks whether preemption is desired] if (ctd->td_critnest > 1) { CTR1(KTR_PROC, "maybe_preempt: in critical section %d", ctd->td_critnest); if ((gd->gd_pflags & GDP_OWEPREEMPT) == 0 || << ADD (gd) pri < gd->gd_preempt_priority) { << ADD (gd) gd->gd_pflags |= GDP_OWEPREEMPT; << CHG (gd) gd->gd_preempt_priority = pri; << ADD (gd) } return (0); } -Matt Matthew Dillon :sched_switch (sched_4bsd) may be preempted in setrunqueue or slot_fill. :This could be ugly. :Wrapping it into a critical section and resetting TDP_OWEPREEMPT should :work. : :Hand trimmed patch: : :RCS file: /cvsroot/src/sys/kern/sched_4bsd.c,v :retrieving revision 1.65 :diff -u -r1.65 sched_4bsd.c :--- sys/kern/sched_4bsd.c 16 Sep 2004 07:12:59 -0000 1.65 :+++ sys/kern/sched_4bsd.c 1 Oct 2004 05:35:28 -0000 :@@ -823,6 +823,7 @@ : TD_SET_CAN_RUN(td); : else { : td->td_ksegrp->kg_avail_opennings++; :+ critical_enter(); : if (TD_IS_RUNNING(td)) { : /* Put us back on the run queue (kse and all). :*/ : setrunqueue(td, SRQ_OURSELF|SRQ_YIELDING); :@@ -834,6 +835,8 @@ : */ : slot_fill(td->td_ksegrp); : } :+ critical_exit(); :+ td->td_pflags &= ~TDP_OWEPREEMPT; : } : if (newtd == NULL) : newtd = choosethread();