Date: Fri, 08 Mar 2002 14:28:27 -0500 (EST) From: John Baldwin <jhb@FreeBSD.org> To: Matthew Dillon <dillon@apollo.backplane.com> Cc: FreeBSD current users <current@FreeBSD.ORG>, Robert Watson <rwatson@FreeBSD.ORG>, Jake Burkholder <jake@locore.ca> Subject: Re: Patch for critical_enter()/critical_exit() & interrupt assem Message-ID: <XFMail.020308142827.jhb@FreeBSD.org> In-Reply-To: <200203080923.g289N1N74926@apollo.backplane.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On 08-Mar-02 Matthew Dillon wrote: > >: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. Hmm, I agree with getting rid of td_savecrit as an MI field and changing the API for cpu_critical_*. I also agree that cpu_critical_* is abused in many cases. I just fixed a few that can be fixed. I think most others can be fixed with the explicit intr_disable/intr_restore API which shouldn't be a problem since it's basically what cpu_critical_* is now but just misnamed. This would fix the remaining instance in witness for example. ast() is harder to solve, and although I don't like having stuff duplicated all over the place making maintenance harder, moving the loop and test out to MD code in either asm or C as Jake suggested would work fine. > 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. No, the savecrit does assume that, but the critnest is still correct and will still work fine. By definition, you don't switch inside a critical section (taht's what critical sections do) so critnest will always be 1 here. Any interrupt or what not that comes in if interrutps aren't disabled might dink with teh count but the count will still be 1 by the time we get to releasing sched_lock. Alternatively, we could set td_critnest to 1 in fork() which would be ok with me. > 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 :-) I always said that Peter's hack was gross. I didn't like it when he did it originally either. Actually, doing cpu_critical_exit() though is safe since all we need is for the critnest to be >= 1 to prevent context switches (which is all critical sections do if you read the manpage that documents them). We only ever need to defer bottom-half code (or Primary Interrupt Context as Apple likes to call it) while holding a spin mutex. The MD code does abuse cpu_critical_* which I am quite happy to fix using intr_*(). Once this is done cpu_critical_* should be out of bogus land and you should be able to adjust your patches to change that API's implementation. I'm willing to do all the work to fixup the cpu_critical_* abuse right now before doing anything else. It really needs to be done anyway. >: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. They won't stay one line. That's the whole point. The entire reason we have MI versions is that I committed part of the preemption tree specifically to try and minimize the amount of the preemption code not currently checked in. The MI versions are still a WIP part of the preemption tree. They wouldn't even exist if it weren't for the preemption tree. I defer to Jake's opinions on the style and profiling issues. All that I ask is that you let me cleanup and fix the cpu_critical_* bogusness which will then allow you to implement your changes in that API while not breaking the kernel preemption WIP. -- 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-current" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?XFMail.020308142827.jhb>