Date: Sun, 24 Feb 2002 21:03:53 -0800 (PST) From: Matthew Dillon <dillon@apollo.backplane.com> To: Jake Burkholder <jake@locore.ca> Cc: Bruce Evans <bde@zeta.org.au>, Terry Lambert <tlambert2@mindspring.com>, Alfred Perlstein <alfred@FreeBSD.ORG>, Bosko Milekic <bmilekic@unixdaemons.com>, Seigo Tanimura <tanimura@r.dl.itc.u-tokyo.ac.jp>, current@FreeBSD.ORG, John Baldwin <jhb@FreeBSD.ORG> Subject: Re: Patch for critical_enter()/critical_exit() & interrupt assembly revamp, please review! Message-ID: <200202250503.g1P53rd29132@apollo.backplane.com> References: <20020224131027.I31343-100000@gamplex.bde.org> <200202241912.g1OJCMx95238@apollo.backplane.com> <20020224224927.D35990@locore.ca>
next in thread | previous in thread | raw e-mail | index | archive | help
:...
:interrupts; hard interrupts are rarely masked. The queueing is written
:in assmebler and runs outside of the kernel so it is fast. Traps in
:general are fast because they don't touch memory until the trapframe
:is written out, so I don't see much point in changing this do the masking
:in software and avoid the soft interrupt.
I have no idea what you are talking about Jake. Could you supply
some context?
:In regards to the patch I think that making critical_enter/exit MD in
:certain cases is a mistake. cpu_critical_enter/exit are already MD
:and it looks you could hide this behind them with some minor changes.
:
:Like in the critical_enter case, make cpu_critical_enter take a thread
:as its argument. It could be a null macro in the i386 case, which would
:optimize out the test.
:
:if (td->td_critnest == 0)
: cpu_critical_enter(td);
:td->td_critnest++;
:
:Likewise with cpu_critical_exit, make it take the thread as its argument.
:
:If (td->td_critnest == 1) {
: td->td_critnest = 0;
: cpu_critical_exit(td);
:} else
: td->td_critnest--;
:(This is equivalent to if (--td->td_critnest == 0), its a matter of taste.)
I'm afraid I have to strongly disagree. I believe that
critical_enter()/exit is *ALREADY* being severely misused in current.
For example, fork_exit() makes assumptions about the underlying hardware
in order to initialize td_critnest and the scheduler mutex safely. This
is just plain broken. fork_exit() should not make any such assumptions.
It is the clear responsibility of either the trampoline code or
cpu_fork() to properly set up td_critnest. One can make an argument
that the sched_lock fixup is in fork_exit()'s domain but you cannot
make the argument that the initialization of critnest (and any associated
hardware state) is MI. It just isn't.
Additionally, critical_enter()/exit themselves are certainly much
more MD then MI. It makes little sense to abstract out an MI interface
that forces unnecessary overhead when all you have to do is define an
MD API that has a few MI requirements, like td_critnest being a critical
nesting count that the MI code can count on. But as MI code the existing
critical_enter()/exit impose additional MD fields and do not give the
MD code the option of not using them (except in a degenerate fashion).
Specifically, the use of the td_savecrit field is under the partial
contorl of the MI code when it shouldn't be, not just in fork_exit()
but also in critical_enter() and critical_exit() themselves.
The existing critical_enter/exit code is far too abstracted for its
low-level purpose and I *WILL* be moving them into MD sections of the
system where they belong. These are really MD routines, not MI routines.
:I notice the you are using cpu_critical_exit for the purpose of disabling
:interrupts only, like cli, sti. I think that you should use api that tmm
:wrote for sparc64 for this so that it can do more. i386 does not really
:have cli/sti equivalents otherwise.
:...
:s = intr_disable() disables interrupts and returns the old state.
:intr_restore(s) restores the state s.
:
:I don't really like this change in general because it complicates things
:more than I'd like, but I also don't have a bearing on how expensive cli/sti
:are. It would seem to me that it just takes a few more clock cycles, which
:isn't that important. I understand that it increases latency for the fast
:part of the interrupt handler, but they are not able to run in critical
:sections anyway due to the software masking.
:
:Jake
Removing cli and sti from the critical functions saves us 300 ns on
every single system call, interrupt, and most traps. And that's with the
sysctl instrumentation and without them being inlined. With the sysctl
instrumentation removed and with critical_enter() and critical_exit()
inlined my guess is that we will save on the order of 500 ns. This
is not insignificant on a P3, and it will probably save even more on
a P4.
With critical_enter() and critical_exit() moved into MD sections, we
can do away with cpu_critical_enter() and cpu_critical_exit() (that is,
make them specific to the MD implementation of critical_enter() and
critical_exit() on those platforms that want to keep the two-layer
abstraction). But for I386 I suppose I could just rename them
to intr_disable() and intr_restore(). I don't understand your
comment about functionality, they work just fine. It's just the name
that needs changing.
-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?200202250503.g1P53rd29132>