Date: Fri, 1 Oct 2004 00:57:45 -0700 (PDT) From: Matthew Dillon <dillon@apollo.backplane.com> To: Stephan Uphoff <ups@tree.com> Cc: "freebsd-arch@freebsd.org" <freebsd-arch@freebsd.org> Subject: Re: sched_switch (sched_4bsd) may be preempted Message-ID: <200410010757.i917vjac017409@apollo.backplane.com> References: <1096610130.21577.219.camel@palm.tree.com>
next in thread | previous in thread | raw e-mail | index | archive | help
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
<dillon@backplane.com>
: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();
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200410010757.i917vjac017409>