sched_switch (sched_4bsd) may be preempted

[Matthew Dillon]

:The core is wrapped in the sched_lock. And since it is a spin lock it is
:running in a critical section with interrupts disabled.
:
:The additional (recursive) critical_enter is just an abusive way to tell
:maybe_preempt* that it should not immediately switch.
:( Yes - eventually there should be a better way to do this)

    Umm.  So you are saying that the code is intentionally breaking the
    API that should otherwise be protecting it from a preemptive thread
    switch by making the assumption that the only critical section count
    will come from an intentional scheduler mutex obtained ONLY for the
    purpose of calling sched_add(), and thus only two or more critical section
    counts means that the originally interrupted code desires no preemption?
    No wonder the scheduler is broken!  That sounds like a recipe for disaster!

    But the solution is simple enough... move the maybe_preempt() call out
    of sched_add().  That is, remove the flawed assumption instead of adding
    further hacks to work around the flawed assumption.  Instead, 
    just conditionally set TDP_OWEPREEMPT there, don't actually try to switch.
    Then simply check for TDP_OWEPREEMPT either just after the scheduler 
    mutex is released, or just before it would otherwise be released.  The
    recursion is happening because the original code was badly designed,
    not because it is an inevitable consequence of implementing preemption.
    But this problem looks *REALLY* easy to fix... NOT by adding more hacks,
    but by fixing the originally flawed code.

:>     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.
:
:This is all running in critical section and we just decided to switch
:and either have or will pick the best thread. Interrupts are locked. The
:additional critical section just prevents recursion problems by delaying
:unwanted switches in maybe_preempt* . Resetting TDP_OWEPREEMPT is
:perfectly save since we switch to the thread chosen while everything has
:been locked.
:
:	Stephan

    I sorta see that, but then again newtd is already set so you are
    assuming that no side effects have occured (from calling other scheduler
    related routines) since newtd was last chosen.  But it is clear that
    there are a ton of opporunities for side effects either to occur or
    to occur in the future as the code continues to be modified, which makes
    this sort of assumption very dangerous and makes the resulting code very
    fragile.  For example, if someone ever wanted to avoid physically 
    disabling interrupts with a 'cli' in critical_enter() (and this is 
    something that could very well happen since neither the original 4.x code
    or the DragonFly code disables interrupts in this case, as an 
    optimization), that breaks all of your assumptions.   In fact, the
    interrupt disablement is being done in the machine-dependant (MD) code,
    and you are assuming it in machine-independant (MI) code.  This makes
    your assumption even MORE unsafe.

    Your goal, with all the problems that the scheduler is having now, should
    be to make the code more robust, NOT make it more fragile.

					-Matt
					Matthew Dillon 
					<dillon at backplane.com>