Date: Thu, 03 Jan 2002 12:26:07 -0800 (PST) From: John Baldwin <jhb@FreeBSD.org> To: Matthew Dillon <dillon@apollo.backplane.com> Cc: arch@FreeBSD.ORG, Bernd Walter <ticso@cicely8.cicely.de>, Mike Smith <msmith@FreeBSD.ORG>, Bruce Evans <bde@zeta.org.au>, Michal Mertl <mime@traveller.cz>, Peter Jeremy <peter.jeremy@alcatel.com.au> Subject: Re: When to use atomic_ functions? (was: 64 bit counters) Message-ID: <XFMail.020103122607.jhb@FreeBSD.org> In-Reply-To: <200201032002.g03K25g72318@apollo.backplane.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On 03-Jan-02 Matthew Dillon wrote: >: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. We need to make sure we don't design thigns such that we have to make other archs look like a 386 to work though. > 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. Err, what if the top-half code you are interrupting already holds the ICU lock. What then? You either panic cause the lock isn't recursive, or you corrupt your ICU if you let the lock recurse. >: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. Not if we have to defer the ICU stuff because we can't use the ICU lock in the low-level code. If we have to defer the ICU stuff and have a level-triggered interrupt we will deadlock. Doing a lazy disabling of interrupts on the first interrupt in a critical section would work around this problem while avoiding the cost of disable interrupts except when the race condition occurs. >:>: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(). *sigh* Nevermind. This idea was to only disable interrupts for spinlock enter/exits and just increment the counter for other enter/exits. >:> 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. Read what I said. The routines will get larger. Also, they are spread across 2 files right now: kern/kern_switch.c and machine/cpufunc.h. >:>: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. You are missing the point. You are looking at the world as if all machines work like a 386. They _don't_. When you design things, you have to take into account other architectures. > 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. That comes from someone who only works on cares about one arch. :) Anyways, I think we aren't completely opposed to each other. Lazy disabling of interrupts would work and wouldn't really cost much since by the time we do it we have already taken an interrupt. The lazy interrupt disabling would also work on all the platforms we currently support and I think should work on ones in the future. It also doesn't need any locks in the bottom half which is a critical feature. Note that not having locks does place some restrictions: we can't use a list embedded in struct ithd since bad things can happen if the same interrupt goes to two CPU's at the same time when both are in critical sections. Tbus, we allow one interrupt then leave interrupts disabled until we exit the section. Secondly, CPU's can't steal pending interrupts from each other since that would need a lock. -- John Baldwin <jhb@FreeBSD.org> <>< http://www.FreeBSD.org/~jhb/ "Power Users Use the Power to Serve!" - http://www.FreeBSD.org/ 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?XFMail.020103122607.jhb>