On 7 January 2012 12:31, matthew green <m...@eterna.com.au> wrote: > >> >> On Fri, Jan 06, 2012 at 03:15:28PM +0000, Cherry G. Mathew wrote: >> >> > Modified Files: >> >> > src/sys/arch/x86/x86: pmap.c >> >> > src/sys/arch/xen/x86: cpu.c >> >> > >> >> > Log Message: >> >> > Address those pesky DIAGNOSTIC messages. \n >> >> > Take a performance hit at fork() for not DTRT. \n >> >> > Note: Only applicable for kernels built with "options DIAGNOSTIC" \n >> >> > >> >> > [...] >> >> > +#ifdef DIAGNOSTIC >> >> > + pmap_kremove(object, PAGE_SIZE); >> >> > +#endif /* DIAGNOSTIC */ >> >> > [plus two more like that] >> >> >> >> Uh... even if that's correct, which doesn't seem too likely on the >> >> surface of things, it doesn't seem desirable. >> > >> > I'm not sure if it's what David meant, but it seems wrong to have >> > different behavior based on DIAGNOSTIC. I think DIAGNOSTIC is supposed >> > to be about enabling inexpensive consistency checks, only. So if it's >> > necessary to have consistency to have that call, it should be >> > unconditional. And if not, it shouldn't exist in any case. >> > >> > >> > Do you understand precisely what's going on? Is this about omitting >> > steps necessary for consistency, but in the case where the new state >> > will be immediately discarded? >> > >> >> From my reply to David: "The pmap_kremove()s are harmless, because we >> know that they're not in >> use elsewhere" >> >> Please see the diffs in context. The one in pmap.c is in >> pmap_pdp_ctor() - this call constructs the PDP frame for a new pmap - >> this happens at fork() as a consequence of a new address space being >> setup by uvm. At this point nothing else is using the PDP frame, and >> the pmaps list is locked, so it can't get accidentally traversed or >> loaded ( this would be an error anyway, as the PDP would crash the cpu >> on which it is loaded since setup is incomplete at this stage. >> >> The other ones are in xen/x86/cpu.c:pmap_cpu_init_late() which is part >> of vcpu setup. The frame entries in this case are CPU local and can't >> be accessed by other cpus (except during setup), so we know that the >> frame is not in use. >> >> In any case, these diffs are all XEN specific, and I'm pretty sure I >> know what I'm doing, unless I've missed something glaringly obvious - >> please let me know. >> >> The correct solution to this is to update pmap_protect() to handle >> kernel entries. > > the problem is that now the code performs different operations > based upon DIAGNOSTIC or not. it's not about whether it's wrong > or right, but that it's different. DIAGNOSTIC shouldn't do more > or different things, just check stuff and assert if bad. > > if the hack works for DIAGNOSTIC, it seems generally right and > should be enabled for everyone now, until the real fix is > implemented. >
Ah, right - sorry, I got distracted a bit by the description of the "paper-over". I'll remove the #ifdef's presently. -- ~Cherry