Date: Thu, 3 Jan 2002 12:02:05 -0800 (PST) From: Matthew Dillon <dillon@apollo.backplane.com> To: John Baldwin <jhb@FreeBSD.ORG> Cc: Peter Jeremy <peter.jeremy@alcatel.com.au>, Michal Mertl <mime@traveller.cz>, Bruce Evans <bde@zeta.org.au>, Mike Smith <msmith@FreeBSD.ORG>, Bernd Walter <ticso@cicely8.cicely.de>, arch@FreeBSD.ORG Subject: Re: When to use atomic_ functions? (was: 64 bit counters) Message-ID: <200201032002.g03K25g72318@apollo.backplane.com> References: <XFMail.020103092809.jhb@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
:To be honest, I find 'td' more readable then 'curthread' so for me this isn't
:that big of a problem. Are you saying you want the compiler to automatically
:cache the value in a temporary variable on its own?
It sure would be nice. We were able to do that for curproc in -stable
before the page table changes.
:> No, what I am saying is that they are designed in a manner that
:> makes it pretty much impossible for GCC or a human to optimize. The
:> fact that you cannot get a stable pointer to the per-cpu area (without
:> being in a critical section) and then access the variables via standard
:> structural indirection has led to all manner of bloat in the API.
:> It turns what ought to be a simple matter of a cpu-relative access
:> into a mess of macros and makes it difficult to take advantage of
:> the per-cpu data area for anything general purpose.
:
:Err, PCPU_PTR returns a pointer. The fact that you have to be in a critical
:section is a separate issue due to the fact that we allow threads preempted by
:an interrupt to migrate. Well, there are two choices as far as allowing
:migration as I see it, either 'critical_exit()' is a migration point as well as
:any setrunqueue()'s, or you just make setrunqueue()'s migration points and pin
:a thread when it is preempted by critical_exit().
Maybe I am not being clear. If critical_enter() and critical_exit()
were considered 'very fast' (aka two instructions inlined for
critical_enter), then not otherwise having a stable per-cpu area is
just fine with me. But at the moment critical_enter() and critical_exit()
are too heavy weight. They are almost as bad as mutex overhead, in
fact. If critical_enter/exit are going to remain slow then I want the
per-cpu area to be stable.
:...
:> We have the same problem with these functions that we had with the spl*()
:> code before we optimized it. People were uncomfortable with the routines
:> and the API and often did all sorts of weird things to try to avoid
:> them due to their perceived overhead. We had (and still have)
:> unnecessarily long spl'd loops in -stable because of that.
:
:The spl code was only optimized on i386. It wasn't on alpha. However, you are
:blatantly ignoring one issue that I've brought up several times here: if you
:don't disable interrupts in a critical section, then you can't use locks in the
:bottom half of the kernel. However, we _need_ to use the icu_lock to disable
:an interrupt source. Also, if you want to allow other CPU's to snoop
:interrupts from other CPU's then you need a lock for that. Which must be
:acquired when writing the data, not just when the other CPU reads it.
The spl code for the alpha was never finished. It was a hack from the
start and, gee, it's still a hack now. That is no excuse for turning
everything into the lowest common denominator.
When an interrupt actually *occurs*, interrupts are disabled by the
processor. So I don't see why there would be any problem obtaining
the ICU lock inside the interrupt itself. Again, it's exactly the same
problem the spl*() code has - that part of the interrupt runs outside
Giant in -stable too you know.
:...
:> also add, that this also greatly improves the flexibility we have in
:> dealing with interrupts. It would even be possible to have an idle cpu
:> check an active cpu's pending interrupt queue and steal one.
:
:But when I exit the interrupt and return back to the kernel interrupts are
:enabled again. Combine this with a level-triggered interrupt and you get a
:deadlock. One idea buzzing around my head is to allow one interrupt in a
:critical section (save's an ithread pointer in the thread) and then disable
:interrupts in the trapframe so when we return we run with interrupts disabled
:until we exit the outermost critical_exit(). This could work as it would allow
:us to defer the ICU lock as well. This does mean that mulitple CPU's might
:schedule the same ithread (or run the same fast interrupt handler) but I think
:we will be fine with that. Humm, except that fast interrupt handlers want a
:trapframe to work with (at least clock interrupts do) so this would break clock
:interrupts. Eww. This we could work around by special saving a
Leaving interrupts disabled on the iret is a trick that has been used
(or at least discussed) before. It would work, but again I don't think
it is necessary. The interrupt can be masked in the ICU while in the
interrupt assembly, just like we do in the SPL code.
:>:Err, I was going to use separate procedures precisely to avoid having to
:>:complicate critical_enter/exit. critical_enter/exit_spinlock or some such
:>:which would only be used in kern_mutex.c and sys/mutex.h in one place in each
:>:file.
I still don't see why you have play with critical_enter/exit inside
the spin code. All you need to do is check to see if any interrupts
have been queued. If one has been and you are inside a critical section,
there is nothing preventing the spin code from scheduling the deferred
interrupts right there and calling setrunqueue().
:> Yuch. It is a simple matter to standardize the meaning of
:> td->td_critnest but the full implementation of 'critical_enter' and
:> 'critical_exit' should not be split up into MD and MI parts. It just
:> makes the code less readable. They should simply be MD.
:
:Incrementing a counter is MD? :)
:
:They are split up like this because critical_exit() is going to grow some
:more MI code for preemption, and it is going to grow some more MI code for this
:optimization as well.
They about a dozen lines of code, splitting them up into four files
makes no sense. It just makes it difficult to follow the code.
If the routines were larger then, sure, I can see splitting them.
But they aren't. They are *tiny*, and you have strewn them over four
(or is it five?) files when they should only be in two. You have
reduced the potential performance of the code by introducing a
conditional that some architectures (e.g. i386) can do without.
Again, I am willing to clean all this stuff up.
:...
:>:how
:>:it is implemented.
:>
:> The basic _cpl concept was and still is platform independant. The
:> concept of 'defering an interrupt' is certainly platform independant.
:> I don't see how this could be any less independant.
:
:Err, the optimization of deferring interrupts is very i386 specific. The
:original spl's when spl's were first used disabled lower-priority interrupts on
:the PDP. We do the same on the Alpha. It's equivalent to cli/sti except
:_much_ more expensive. Each spl on the alpha involved 1 or 2 PAL calls. The
:way you are approaching this is very i386 specific.
I disagree. You can implement the same concept on Alpha too if you want.
I am not an Alpha assembly programmer so I can't do it, but I sure as
hell can fix i386. If someone cares about the Alpha enough there is
nothing stopping them from doing thet same optimization for the alpha.
Again, the 'lowest common denominator' API concept is just plain a bad
idea. APIs have to be designed to be able to take advantage of the
strengths of the underlying architectures, not make it impossible to
optimize to those strengths.
:> started to scrap the _cpl stuff, incorporate Giant, and implement
:> the idle thread.
:
:Uh. The i386 code on SMP used a mplock that it _disabled interrupts_ with in
:order work around the problems I've brought up. Using cli/sti or their
:equivalents is how we handled these problems before. You weren't crying foul
:then.
:
:--
:
:John Baldwin <jhb@FreeBSD.org> <>< http://www.FreeBSD.org/~jhb/
An mplock is just fine *IF* you don't have to screw around with
it in the critical path. The actual interrupt processing code can
afford to be slightly heavier weight if it can turn all the spl*()
(or critical_enter/exit) routines into extremely light weight entities.
If critical_enter() is just ++td->td_critcount or whatever it's called,
and critical_exit() is something like:
static __inline
critical_exit()
{
...
if (--td->td_critcount == 0 && td->td_critsignal)
_critical_exit(); /* more complex processing */
}
Then we get the best of both worlds. We can nice, fast, streamlined
inlines that nobody will have to think twice about using, and we
get incredible flexibility. For something like the mutex spin we
do:
if (td->td_critsignal)
critical_process_signals();
Or whatever we want to call it, which will be responsible for
scheduling any queued interrupts and calling setrunqueue(), for
example.
-Matt
Matthew Dillon
<dillon@backplane.com>
To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe freebsd-arch" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200201032002.g03K25g72318>