[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]
Re: CVS commit: src/sys/arch
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
Ah, right - sorry, I got distracted a bit by the description of the
I'll remove the #ifdef's presently.
Main Index |
Thread Index |