Re: [PATCH v2] x86/PoD: shorten certain operations on higher order ranges

[George Dunlap <george.dunlap@citrix.com>]

On 30/09/15 13:12, Jan Beulich wrote:
>>>> On 29.09.15 at 18:45, <George.Dunlap@eu.citrix.com> wrote:
>> On Mon, Sep 28, 2015 at 3:30 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>> Now that p2m->get_entry() always returns a valid order, utilize this
>>> to accelerate some of the operations in PoD code. (There are two uses
>>> of p2m->get_entry() left which don't easily lend themselves to this
>>> optimization.)
>>>
>>> Also adjust a few types as needed and remove stale comments from
>>> p2m_pod_cache_add() (to avoid duplicating them yet another time).
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> So I was just looking at my own suggestion, and I want to withdraw my
>> "reviewed-by" temporarily to ask a question...
> 
> Okay, meaning we'd need to revert.
> 
>>> @@ -649,7 +656,8 @@ p2m_pod_zero_check_superpage(struct p2m_
>>>      p2m_type_t type, type0 = 0;
>>>      unsigned long * map = NULL;
>>>      int ret=0, reset = 0;
>>> -    int i, j;
>>> +    unsigned long i, n;
>>> +    unsigned int j;
>>>      int max_ref = 1;
>>>      struct domain *d = p2m->domain;
>>>
>>> @@ -668,10 +676,13 @@ p2m_pod_zero_check_superpage(struct p2m_
>>>
>>>      /* Look up the mfns, checking to make sure they're the same mfn
>>>       * and aligned, and mapping them. */
>>> -    for ( i=0; i<SUPERPAGE_PAGES; i++ )
>>> +    for ( i = 0; i < SUPERPAGE_PAGES; i += n )
>>>      {
>>>          p2m_access_t a;
>>> -        mfn = p2m->get_entry(p2m, gfn + i, &type, &a, 0, NULL, NULL);
>>> +        unsigned int cur_order;
>>> +
>>> +        mfn = p2m->get_entry(p2m, gfn + i, &type, &a, 0, &cur_order, NULL);
>>> +        n = 1UL << min(cur_order, SUPERPAGE_ORDER + 0U);
>>>
>>>          if ( i == 0 )
>>>          {
>>
>> The check at the bottom of this loop not only checks to see that all
>> the mfns are contiguous; they also check that each of the indivual
>> (4k) mfns has certain properties; namely:
>>
>> + None of the mfns are used as pagetables, or allocated via xenheap
>> + None of the mfns are likely to be mapped elsewhere (refcount 2 or
>> less for shadow, 1 for hap)
>>
>> This change makes it so that if the p2m entry is 2M or larger, only
>> the first mfn actually gets checked.  But I don't think we can assume
>> that just because the first page of a superpage is not used as a
>> pagetable, or mapped elsewhere, that none of the pages are.  (Please
>> correct me if I'm wrong here.)
>>
>> If that's the case, then we would need to run this loop all the way
>> through, even if cur_order is SUPERPAGE_ORDER.
>>
>> I suppose we could have two loops, one which checks for superpage
>> integrity, and another which checks for the necessary properties for
>> each individual mfn in the superpage.
>>
>> Thoughts?
> 
> You're right - those checks not depending on individual page
> attributes in that condition have misled me to assume all are of
> that kind. We indeed do need an inner loop dealing with per-page 
> properties. I suppose you won't mind cleaning this up a little in the
> course of the anyway necessary transformation.

Yes, I've got a prototype patch I'll probably send out tomorrow.

 -George