Re: [PATCH] x86/PoD: tighten conditions for checking super page

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

On 02/11/15 16:50, Jan Beulich wrote:
>>>> On 02.11.15 at 17:29, <george.dunlap@citrix.com> wrote:
>> On 30/10/15 17:39, Jan Beulich wrote:
>>> Since calling the function isn't cheap, try to avoid the call when we
>>> know up front it won't help; see the code comment for details on those
>>> conditions.
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>
>>> --- a/xen/arch/x86/mm/p2m-pod.c
>>> +++ b/xen/arch/x86/mm/p2m-pod.c
>>> @@ -522,7 +522,6 @@ p2m_pod_decrease_reservation(struct doma
>>>      if ( unlikely(d->is_dying) )
>>>          goto out_unlock;
>>>  
>>> -recount:
>>>      pod = nonpod = ram = 0;
>>>  
>>>      /* Figure out if we need to steal some freed memory for our cache */
>>> @@ -562,15 +561,20 @@ recount:
>>>          goto out_entry_check;
>>>      }
>>>  
>>> -    /* Try to grab entire superpages if possible.  Since the common case is 
>> for drivers
>>> -     * to pass back singleton pages, see if we can take the whole page back 
>> and mark the
>>> -     * rest PoD. */
>>> -    if ( steal_for_cache
>>> -         && p2m_pod_zero_check_superpage(p2m, gpfn & ~(SUPERPAGE_PAGES-1)))
>>> -    {
>>> -        /* Since order may be arbitrary, we may have taken more or less
>>> -         * than we were actually asked to; so just re-count from scratch */
>>> -        goto recount;
>>> +    /*
>>> +     * Try to grab entire superpages if possible.  Since the common case is 
>> for
>>> +     * drivers to pass back singleton pages, see if we can take the whole 
>> page
>>> +     * back and mark the rest PoD.
>>> +     * No need to do this though if
>>> +     * - order >= SUPERPAGE_ORDER (the loop below will take care of this)
>>> +     * - not all of the pages were RAM (now knowing order < SUPERPAGE_ORDER)
>>> +     */
>>> +    if ( steal_for_cache && order < SUPERPAGE_ORDER && (ram >> order) &&
>>> +         p2m_pod_zero_check_superpage(p2m, gpfn & ~(SUPERPAGE_PAGES - 1)) )
>>> +    {
>>> +        pod += ram;
>>> +        nonpod -= ram;
>>> +        ram = 0;
>>
>> +1 for the idea; a couple of comments:
>>
>> * I think it would be clearer to use "(ram == 1 << order)" instead of
>> "ram >> order".  I understand (ram >> order) will be non-zero only if
>> ram == 1 << order, but why make people spend brain cycles trying to
>> figure that out?
> 
> Because I originally thought it makes the CPU spend less cycles
> on the calculation. But that I think about it again, I guess I was
> wrong (I would have been right only when order > 0 and the
> compiler would be able to prove this and it would actually make
> use of the knowledge using the status flags from the shift
> instead of doing a subsequent test to get those flags set).
> 
> So - yes, will do.
> 
>> * If we're going to assume that "ram >> order" implies "all the entries
>> are ram", and furthermore that a positive return value implies "all ram
>> was changed to pod", wouldn't it be better to do something like the
>> following?
>>
>>   pod = 1 << order
>>   nonpod = ram = 0
>>
>> This would be more clearly correct if we change the comparison to ram ==
>> 1 << order.
> 
> Well, yes, I can do that too. Here I really tried to avoid establishing
> an unnecessary dependency between the if() condition and the body
> (i.e. with how it is, the condition could change quite a bit without the
> calculations getting wrong, whereas with what you want the
> restrictions would be more tight).

Well in fact, one of the reasons I made my suggestion is because there
*is* a dependency between the if() condition and the body.  If you take
out (order < SUPERPAGE_ORDER), then (ram >> order) isn't a correct check
any more; and (for example) if order == SUPERPAGE_ORDER+1, then
subtracting ram is incorrect.  I wanted to make it more obvious.

Thanks,
 -George