Re: [PATCH] respin of mprotect performance patch

["Bruce Rogers" <BROGERS@novell.com>]

>>> On 1/10/2008 at 8:52 AM, in message <C3ABEFCB.1A5C8%Keir.Fraser@cl.cam.ac.uk>,
Keir Fraser <Keir.Fraser@cl.cam.ac.uk> wrote:
> It's taken me a while to work out that MMU_ATOMIC_PT_UPDATE is actually
> 'atomic' in that it preserves the existing A/D bits (and should ignore the
> ones specified in the given new value). Am I correct?
>From the point where the new bits are determined to the point where the pte is actually updated, the only way that the new value can be out of date is if the hardware mmu has updated (set) the A/D bits.  I don't believe they could have been cleared.  So I see it as not ignoring the A/D bits, but accepting that they may have been updated (set) and allowing for that change.
> 
> In which case:
>  (1) The name and description in xen.h should be better. E.g.,
> MMU_PT_UPDATE_PRESERVE_AD, with a corresponding comment explaining the
> atomicity/consistency guarantee.
That would be fine.  I didn't know what other architectures might have slightly different semantics so I stuck with a general (wider coverage) *atomic* nomenclature.  Since it's been pointed out that mmu_ops is x86 specific, then specifying that we're talking about the treatment of the A/D bits would be fine.
>  (2) The implementation is actually wrong, as it only attempts to propagate
> A/D bits into the 'new' value if the first cmpxchg attempt fails. Otherwise
> the A/D values passed in by the guest are used, but the PTE might change
> before Xen itself reads the ol1e.
Yes - I see that. So we need to OR in those bits from ol1e into the new value before the cmpxchg.  Thanks for catching that blunder.
>  (3) In general, the hypercall implementation can probably be slimmed down a
> bunch. There's lots of code duplication.
Agreed.  I wasn't sure how much I wanted to disrupt the other (existing) code paths, but it could certainly be combined.
> 
> I can take a swing at the Xen side of this, if I haven't got the wrong end
> of the stick?
That would be great.

Do you have any comment on whether the second hypercall is useful enough to keep?  It would be quite simple for me to rip it out if not.

- Bruce
> 
>  -- Keir
> 
> On 8/1/08 01:05, "Bruce Rogers" <brogers@novell.com> wrote:
> 
>> I've incorporated the comments of Jan and Keir into the attached patches, 
> with
>> the exception of moving mmu_ops related entries from public/xen.h into
>> architecture specific headers.
>> Thanks for your review.  Let me know if I've missed anything.
>> 
>> Signed-off-by: Bruce Rogers <brogers@novell.com>
>> 
>> - Bruce
>> 
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xensource.com 
>> http://lists.xensource.com/xen-devel