* [PATCH] x86/PoD: tighten conditions for checking super page
@ 2015-10-30 17:39 Jan Beulich
2015-10-30 18:57 ` Andrew Cooper
2015-11-02 16:29 ` George Dunlap
0 siblings, 2 replies; 7+ messages in thread
From: Jan Beulich @ 2015-10-30 17:39 UTC (permalink / raw)
To: xen-devel; +Cc: George Dunlap, Andrew Cooper
[-- 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] x86/PoD: tighten conditions for checking super page
2015-10-30 17:39 [PATCH] x86/PoD: tighten conditions for checking super page Jan Beulich
@ 2015-10-30 18:57 ` Andrew Cooper
2015-11-02 16:29 ` George Dunlap
1 sibling, 0 replies; 7+ messages in thread
From: Andrew Cooper @ 2015-10-30 18:57 UTC (permalink / raw)
To: Jan Beulich, xen-devel; +Cc: 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>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] x86/PoD: tighten conditions for checking super page
2015-10-30 17:39 [PATCH] x86/PoD: tighten conditions for checking super page Jan Beulich
2015-10-30 18:57 ` Andrew Cooper
@ 2015-11-02 16:29 ` George Dunlap
2015-11-02 16:50 ` Jan Beulich
2015-11-05 16:43 ` Jan Beulich
1 sibling, 2 replies; 7+ messages in thread
From: George Dunlap @ 2015-11-02 16:29 UTC (permalink / raw)
To: Jan Beulich, xen-devel; +Cc: George Dunlap, 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>
>
> --- 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] x86/PoD: tighten conditions for checking super page
2015-11-02 16:29 ` George Dunlap
@ 2015-11-02 16:50 ` Jan Beulich
2015-11-02 17:03 ` George Dunlap
2015-11-05 16:43 ` Jan Beulich
1 sibling, 1 reply; 7+ messages in thread
From: Jan Beulich @ 2015-11-02 16:50 UTC (permalink / raw)
To: George Dunlap; +Cc: George Dunlap, Andrew Cooper, xen-devel
>>> 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] x86/PoD: tighten conditions for checking super page
2015-11-02 16:50 ` Jan Beulich
@ 2015-11-02 17:03 ` George Dunlap
0 siblings, 0 replies; 7+ messages in thread
From: George Dunlap @ 2015-11-02 17:03 UTC (permalink / raw)
To: Jan Beulich; +Cc: George Dunlap, Andrew Cooper, xen-devel
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] x86/PoD: tighten conditions for checking super page
2015-11-02 16:29 ` George Dunlap
2015-11-02 16:50 ` Jan Beulich
@ 2015-11-05 16:43 ` Jan Beulich
2015-11-09 9:31 ` George Dunlap
1 sibling, 1 reply; 7+ messages in thread
From: Jan Beulich @ 2015-11-05 16:43 UTC (permalink / raw)
To: George Dunlap; +Cc: George Dunlap, Andrew Cooper, xen-devel
>>> 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] x86/PoD: tighten conditions for checking super page
2015-11-05 16:43 ` Jan Beulich
@ 2015-11-09 9:31 ` George Dunlap
0 siblings, 0 replies; 7+ messages in thread
From: George Dunlap @ 2015-11-09 9:31 UTC (permalink / raw)
To: Jan Beulich; +Cc: Andrew Cooper, George Dunlap, xen-devel
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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2015-11-09 9:31 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-10-30 17:39 [PATCH] x86/PoD: tighten conditions for checking super page Jan Beulich
2015-10-30 18:57 ` Andrew Cooper
2015-11-02 16:29 ` George Dunlap
2015-11-02 16:50 ` Jan Beulich
2015-11-02 17:03 ` George Dunlap
2015-11-05 16:43 ` Jan Beulich
2015-11-09 9:31 ` George Dunlap
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.