Date: Fri, 1 Oct 2004 11:15:19 -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: <200410011815.i91IFJQq019971@apollo.backplane.com> References: <1096610130.21577.219.camel@palm.tree.com> <1096644427.25800.26.camel@palm.tree.com>
next in thread | previous in thread | raw e-mail | index | archive | help
: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@backplane.com>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200410011815.i91IFJQq019971>