re: CVS commit: src/sys/arch

[matthew green <mrg%eterna.com.au@localhost>]

> >> 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.


.mrg.