Date: Fri, 8 Mar 2002 02:46:54 -0500 From: Jake Burkholder <jake@locore.ca> To: Matthew Dillon <dillon@apollo.backplane.com> Cc: Robert Watson <rwatson@FreeBSD.ORG>, FreeBSD current users <current@FreeBSD.ORG> Subject: Re: Patch for critical_enter()/critical_exit() & interrupt assem Message-ID: <20020308024653.B14181@locore.ca> In-Reply-To: <200203080304.g2834n571759@apollo.backplane.com>; from dillon@apollo.backplane.com on Thu, Mar 07, 2002 at 07:04:49PM -0800 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>
next in thread | previous in thread | raw e-mail | index | archive | help
Apparently, On Thu, Mar 07, 2002 at 07:04:49PM -0800, Matthew Dillon said words to the effect of; > :Yes. What I would like and what I mentioned before is for this to be > :hidden behind cpu_critical_enter/exit. Specifically, cpu_critical_enter > :would be a null macro for i386, which would turn critical_enter into > :just an increment, as Matt wanted. cpu_critical_exit would do all the > :magic of unpending interrupts. The reason for this is that I would > :like to see critical_exit handled any pending preemptions for a thread. > :We do not yet know exactly how this will work so I would like this to be > :done in MI code to start with. The code must not make assumptions that are > :not valid on all platforms. This is easiest if they use the same code. > :This is not about duplication of code in several MD files. > > We can't, because the MI code makes some rather blatent assumptions > in regards to cpu_critical_enter and exit. Take the witness code, > for example. So we cannot safely null-out those macros. In fact, > the MI code also makes some rather blatent assumptions in regards > to critical_enter() and exit which I have also had to clean up > (and which will need considerably more cleanup). 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. 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 the increment of ktr_idx is done atomically. I don't know why this was 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 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. With these changes I don't see why the critical_enter/exit functions can't or shouldn't remain MI. > > These assumptions include, just as an example: The witness code > calling cpu_critical_enter()/exit without holding td_critnest count, > and Peter's now withdrawn code which explicitly released the cpu > critical section while holding a non-zero td_critnest count. In > otherwords, really aweful hacks. So we can't just NULL out those > inlines. > > Trying to keep critical_enter()/critical_exit() MI and > cpu_critical_enter()/cpu_critical_exit() MD and separated doesn't > make much sense to me, because frankly critical_enter() and > critical_exit() are tiny little routines (and will remain tiny even > after SWI and preemption is added) and I can't think of any reason > to force higher complexity in the routines due to the separation. > > 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. > > The API's are still MI, but the implementation is MD. > > :Bruce also had some comments which were shrugged off, I thought they > :were important. Specifically, please do not make unnecessary changes > :to the assembler code. Macros do not need to be defined before they > :are used, I believe this was the justification for some of the reordering > :in apic_vector.s which makes the patch confusing. Please do not tab > :out the "; \" at the end of the lines in the INTR and FAST_INTR macros > :in icu_vector.s. This just makes unnecessary diffs. The PUSH_DUMMY > :macro must push a reasonable eip value, in addition to the code segment, > :so that profiling and stack traces work right. If you have not already, > :please make sure that a trace from inside an interrupt handler that was > :unpended looks somewhat normal. Please also make sure that kgdb is able > :to decode the frame properly. It assumes that the eip of the frame will > :be near certain kernel symbols in order to determine what kind of frame > :it is. > : > :Jake > > 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. > > 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 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?20020308024653.B14181>