Date: Fri, 8 Mar 2002 01:23:01 -0800 (PST) From: Matthew Dillon <dillon@apollo.backplane.com> To: Jake Burkholder <jake@locore.ca> Cc: Robert Watson <rwatson@FreeBSD.ORG>, FreeBSD current users <current@FreeBSD.ORG> Subject: Re: Patch for critical_enter()/critical_exit() & interrupt assem Message-ID: <200203080923.g289N1N74926@apollo.backplane.com> References: <200203072143.g27LhaL97112@harmony.village.org> <Pine.NEB.3.96L.1020307171254.23264D-100000@fledge.watson.org> <20020307205844.C12044@locore.ca> <200203080304.g2834n571759@apollo.backplane.com> <20020308024653.B14181@locore.ca>
next in thread | previous in thread | raw e-mail | index | archive | help
:I agree that the use of cpu_critical_enter/exit could use cleaning up.
:Can you give an example of where critical_enter is used improperly?
:You mean in fork_exit? Your changes to cpu_switch solve that problem
:with critical_exit almost unchanged. The savecrit stuff should really
:just be made MD. If cpu_critical_exit was changed to take the thread
:as its argument as I suggested before this would be easy.
fork_exit() is a good example. The existing code explicitly
initializes td->td_critnest to 1 and then sets td->td_savecrit
to CRITICAL_FORK:
td->td_critnest = 1;
td->td_savecrit = CRITICAL_FORK;
It then proceeds to unlock the sched_lock spin lock.
This code only works because interrupts are hard-disabled in the
fork trampoline code. In otherwords, the code assumes that
cpu_critical_enter() and cpu_critical_exit() play with hard interrupt
disablement. If interrupts were enabled there would be two severe
race conditions in the code: The first upon entering fork_exit
prior to ke_oncpu being set, and the second when td->td_critnest is set
to 1 prior to td_savecrit being set to CRITICAL_FORK.
The current ast() code is another example. This code calls
cpu_critical_enter() and cpu_critical_exit() without bothering to bump
the critical nesting count (i.e. bypasses critical_enter() and
critical_exit()).
Peter's hack to fix IPI deadlocks (no longer in -current) is a
third example. He enters a critical section using critical_enter()
and then calls cpu_critical_exit() while still holding td_critnest = 1,
which breaks even a conservative reading of the API :-)
--
There are also a huge number of places where cpu_critical_*() is
used specifically in the I386 code for interrupt disablement. I am
happy to clean it up, but not until after the multiple stages of
my current patch set are in the tree because there are just too many
places that have to be modified for me to feel comfortable doing both
at once. Nor do I believe it makes sense to clean up cpu_critical_*()
just to try to keep critical_*() in MI code (see below two sections).
:Specifically I think that all of the current uses of cpu_critical_enter
:exit outside of critical_enter exit are bugs. The use in ktr is bogus because
Certainly in MI code, I agree. These cases can be attacked incrementally.
fork_exit() will be fixed completely by my patches since it has to be
for my code to work properly. I was planning on leaving ast() alone
for the moment (because I am not planning on making cpu_critical_enter()
a NOP any time soon). I would be willing to work on this after my
patch is staged in (carrot and stick).
:there in the first place. I think that witness is an example of where
:we need a different specifically defined to be hard interrupt disable api.
:This is why I suggested the intr_disable/intr_restore api, which should only
:be used in MD code and in dire circumstances otherwise, of which this case
:qualifies. The code in ast is just structured wrong. I think that before
:the loop was in assembler outside of the routine itself, so that it didn't
Witness is a special case allright. I don't know if I agree with
extending a hard interrupt disablement API out to MI code though.
:make so many assumptions about interrupt disablement. doreti which calls
:it should look like this:
:
:loop:
: cli;
: if (there is an ast we can handle)
: goto ast;
: iret;
:ast:
: sti;
: call ast;
: goto loop;
:
:In which case ast doesn't need to loop or use critical sections at all.
:All of the MD code I could find I think should use a different api for
:hard interrupt disablement. They are all very MD and need this specifically,
:which cpu_critical_enter need not necessarily do.
I would agree with this methodology.
:With these changes I don't see why the critical_enter/exit functions can't
:or shouldn't remain MI.
Cleaning up cpu_critical_enter()/exit would require a huge number of
changes that I am not prepared to do right now, not only in i386, but
in all architectures using cpu_critical_enter() and cpu_critical_exit()
- alpha, i386 (many places), and ia64, not to mention ast(), fork_exit(),
witness, subr_trap.c, and ktr.c. I suppose the routines could simply
be renamed in the MD sections but the MI sections are going to be
harder to fix.
It doesn't make much sense to me to spend so much effort to leave
two essentially one-line procedures, critical_enter() and critical_exit(),
(++td->td_critnest + MD call, --td->td_critnest + MD call) MI when all
the rest of the work they have to do is MD.
Why are you so rabid about keeping critical_enter() and critical_exit()
in MI sections? From my point of view leaving them MI simply makes the
code more obfuscated, less understandable, less flexible, and, yes,
the 'O' word...harder to optimize. I see absolutely no reason to do it
that way.
:...
:> In short, critical_enter()/critical_exit() are TIGHTLY INTEGRATED
:> with cpu_critical_enter/exit despite your attempt to make
:> critical_enter/exit MI. What my patch does is accept as true the
:> fact that the two sets of routines are, in fact, tightly integrated,
:> and then implements them in a more sensible way (or, more to the
:> point, allows each architecture to implement them in the proper
:> manner as befits the architecture).
:
:I do not agree that having cpu_critical_enter separate in any way hinders
:an architecture's ability to implement critical_enter properly. The MI
:code that calls it expects it to do certain things, one of them is to call
:cpu_critical_enter and cpu_critical_exit exactly once for each non-nested
:critical_enter exit pair. Wether or not this actually disables interrupts
:is up to the MD code.
Assuming that all other uses of cpu_critical_*() are cleaned up, then
I agree. But by the same token there is no reason to construct MD
procedures called cpu_critical_enter() and cpu_critical_exit() whos
SOLE purpose and SOLE use is to be called from tiny little MI procedures
calls critical_enter() and critical_exit(). At that point you might
as well do away with cpu_critical_enter() and cpu_critical_exit()
entirely and simply have critical_enter() and critical_exit() be
MD procedures. The MI portion of critical_enter() and critical_exit()
is effectively just one line of code, the MD portion is all the rest.
Putting it all in a single MD module is less fuss, less muss, easier
to understand, more flexible, and, yes, the 'O' word too.
So, again, from my point of view it makes a whole lot more sense for
critical_enter() and critical_exit() to be MD. If I turn out to be
wrong I could turn it back into MI in a later commit, but I don't
think I'm wrong. I think that we want the flexibility even if we
have MI support for things like preemption (another two lines of
code in critical_exit()).
I can't imagine critical_exit() having more then four total lines of
MI code, and all the rest (potentially dozens of lines of code) would
be MD so, again, it makes sense to just make critical_*() MD in the
first place.
:> Actually all I did there was square up icu_vector.s so it looked
:> almost the same as apic_vector.s. I would consider that an improvement.
:> Besides, I find it very difficult to work on those macros without
:> tabbing out the backslashes. I moved some of the #defines around due
:> to it otherwise being a forward reference. This turned out not to be
:> an issue since the macros aren't actually used until later in the code,
:> but I don't like forward referenced macros anyway so I left it in.
:
:These are the kinds of changes that should be done separately.
:
:I disagree that the tabbing changes are an improvement. They make it
:difficult to see what actually changed, and I don't think that you
:changed every line. I happen to find the slashes up against the instructions
:convenient because you don't have to adjust the tabs when a change to the
:instructions is made. I use this style in sparc64 assembler.
:
:The reordering is really bad, it looks like even less actually changed.
:
:Please make both of these changes separately.
I would prefer not to touch these source files at this time. They
work, they are readable, and frankly it is a whole lot easier to
understand the patch by reading the post-patched assembly rather then
the patch itself, for both apic_vector.s and icu_vector.s.
If it takes two side-by-side commits separating out the code changes
from the tabbing in order for your to agree to allow the code to be
committed, however, I'll do it. Otherwise I think it's a waste of time.
The assembly is not and has never been stable so an incremental patch
is not really that necessary for continuity.
:> Insofar as diffs go, I'm afraid even the most conservative changes
:> to the assembly to support interrupt deferral would make for a pretty
:> big diff set.
:>
:> I'm not going to change them now, no, but I don't see this as being
:> a show stopper in the least.
:
:Do stack traces in ddb work as expected from inside unpended interrupt
:handlers? Please correct me if I'm wrong but I don't think they will.
:It should look like a normal interrupt with the eip in call_fast_unpend.
:
:Do they also work as expected in kgdb?
:
:Jake
I haven't tested it but they should work just fine since the fast unpend
assembly routines are inside the bintr/eintr MCOUNT_LABEL's. So they
should be treated the same as a normal interrupt. If ddb> can handle
a normal interrupt it should be able to handle a fast-unpend.
If it doesn't work then it's almost certainly a simple fix to adjust
the fake trap frame which can be done post-commit.
-Matt
Matthew Dillon
<dillon@backplane.com>
To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe freebsd-current" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200203080923.g289N1N74926>