Re: CVS commit: src/sys/arch

["Cherry G. Mathew" <cherry.g.mathew%gmail.com@localhost>]

On 7 January 2012 05:53, Greg Troxel <gdt%ir.bbn.com@localhost> wrote:
>
> David Holland <dholland-sourcechanges%netbsd.org@localhost> writes:
>
>> 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.

-- 
~Cherry