Re: CVS commit: src/sys/arch

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

On 7 January 2012 12:31, matthew green <mrg%eterna.com.au@localhost> 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