Re: [PATCH] Fix performance problems with mprotect()

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

>>> On 1/7/2008 at 10:45 AM, in message <C3A815C0.11D69%Keir.Fraser@cl.cam.ac.uk>,
Keir Fraser <Keir.Fraser@cl.cam.ac.uk> wrote:
> On 7/1/08 17:05, "Bruce Rogers" <BROGERS@novell.com> wrote:
> 
>>> An additional piece of concern regarding the bit assignments of
>>> MMU_FLAG_RANGE_UPDATE's val parameter (Keir, maybe you need to
>>> comment on this one): The whole mmu_update interface, being
>>> defined in public/xen.h, is supposed to be sufficiently architecture
>>> neutral, which it won't be anymore in the way it currently is being
>>> modified. But maybe I'm mistaken and the interface's declaration is
>>> just badly placed (would apply to the mmuext interface then, too)?
>> I should point out that the MMU_FLAG_RANGE_UPDATE hypercall didn't end up
>> being needed for improving the SAP test, but does get invoked by other uses 
> of
>> mprotect() and feel that it is useful.  As it stands I've only implemented 
> x86
>> version of this, other architecture owners would need to do the same.
>> Perhaps as far as what is presented in public/xen.h, we should just have a
>> more generic description (not pointing out the format) and leave it up to 
> each
>> architecture as to what specific format for the val member makes sense.
> 
> No other architecture uses do_mmu_update(). The public definitions are
> simply in the wrong header file.
> 
> Is MMU_FLAG_RANGE_UPDATE really useful if you have MMU_ATOMIC_PT_UPDATE? If
> it is it's presumably a performance issue, and the question should be: why
> is a sequence MMU_ATOMIC_PT_UPDATE slower than MMU_FLAG_RANGE_UPDATE, and
> can we make it fast enough that MMU_FLAG_RANGE_UPDATE is redundant?
I did find a quite measurable performance difference between using the two methods, but I consider it useful mainly because of its compactness in terms of being able to update all ptes in a page table without needing a large array to initialize from.  That said,  I am not beholden to this flag range hypercall, since the other one is more generic and we should be able to make it fast enough.  If you prefer, we can just go with the MMU_ATOMIC_PT_UPDATE hypercall only.
> 
> Also, the documentation for MMU_ATOMIC_PT_UDATE in xen.h is possibly
> incorrect. It doesn't make sense to me -- are your meanings for ptr[2:] and
> val actually correct?
Oh,you are correct.  I had originally coded  MMU_ATOMIC_PT_UDATE as it is described, since doing it that way was giving me about 14% better performance than doing the batching one mmu_update_t structure at a time, but I again opted for backing off to the more general case and forgot to update that description for that hypercall.
I'll get that fixed.
> 
>  -- Keir