sched_switch (sched_4bsd) may be preempted

[Matthew Dillon]

    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 at 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();