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

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

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 1772 bytes --]

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;
     }
 
     /* Process as long as:




[-- Attachment #2: x86-PoD-emerg-avoid-check-super.patch --]
[-- Type: text/plain, Size: 1821 bytes --]

x86/PoD: tighten conditions for checking super page

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;
     }
 
     /* Process as long as:

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

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

[Andrew Cooper]

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>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

[George Dunlap]

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?

* 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.

* steal_for_cache may now be wrong.  I realize that since now ram == 0
that all the subsequent "steal_for_cache" expressions will end up as
"false" anyway, but leaving invariants in an invalid state is sort of
asking for trouble.

I'd prefer you just update steal_for_cache; but if not, at least leave a
comment there saying that it may be wrong and why it doesn't matter.

Thanks,
 -George

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

[Jan Beulich]

>>> 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).

> * steal_for_cache may now be wrong.  I realize that since now ram == 0
> that all the subsequent "steal_for_cache" expressions will end up as
> "false" anyway, but leaving invariants in an invalid state is sort of
> asking for trouble.
> 
> I'd prefer you just update steal_for_cache; but if not, at least leave a
> comment there saying that it may be wrong and why it doesn't matter.

I'll see whether updating is reasonably straightforward.

Jan

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

[George Dunlap]

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

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

[Jan Beulich]

>>> On 02.11.15 at 17:29, <george.dunlap@citrix.com> wrote:
> * steal_for_cache may now be wrong.  I realize that since now ram == 0
> that all the subsequent "steal_for_cache" expressions will end up as
> "false" anyway, but leaving invariants in an invalid state is sort of
> asking for trouble.
> 
> I'd prefer you just update steal_for_cache; but if not, at least leave a
> comment there saying that it may be wrong and why it doesn't matter.

I've just done the other things, but I don't think steal_for_cache
can have changed at this point: p2m_pod_cache_add() increments
p2m->pod.count by the same value by which
p2m_pod_zero_check_superpage() bumps p2m->pod.entry_count
right after having called p2m_pod_cache_add(). I could leave a
comment of ASSERT() to that effect, unless I'm overlooking
something.

Jan

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

[George Dunlap]

On Thu, Nov 5, 2015 at 4:43 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 02.11.15 at 17:29, <george.dunlap@citrix.com> wrote:
>> * steal_for_cache may now be wrong.  I realize that since now ram == 0
>> that all the subsequent "steal_for_cache" expressions will end up as
>> "false" anyway, but leaving invariants in an invalid state is sort of
>> asking for trouble.
>>
>> I'd prefer you just update steal_for_cache; but if not, at least leave a
>> comment there saying that it may be wrong and why it doesn't matter.
>
> I've just done the other things, but I don't think steal_for_cache
> can have changed at this point: p2m_pod_cache_add() increments
> p2m->pod.count by the same value by which
> p2m_pod_zero_check_superpage() bumps p2m->pod.entry_count
> right after having called p2m_pod_cache_add(). I could leave a
> comment of ASSERT() to that effect, unless I'm overlooking
> something.

Ah, yes of course.

 -George