* [PATCH] x86/memshr: fix preemption in relinquish_shared_pages()
@ 2013-12-17 9:07 Jan Beulich
2013-12-17 10:43 ` Andrew Cooper
2013-12-17 14:44 ` Tim Deegan
0 siblings, 2 replies; 6+ messages in thread
From: Jan Beulich @ 2013-12-17 9:07 UTC (permalink / raw)
To: xen-devel; +Cc: Tim Deegan, Andres Lagar-Cavilla
[-- Attachment #1: Type: text/plain, Size: 1498 bytes --]
For one, should hypercall_preempt_check() return false the first time
it gets called, it would never have got called again (because count,
being checked for equality, didn't get reset to zero).
And then, if there were a huge range of unshared pages, with count not
getting incremented at all in that case there would also not be any
preemption.
Fix this by using a biased increment (ratio 1:16 for unshared vs shared
pages), and check for certain bits in count to be clear rather than for
a specific value (the alternative would be to do a greater-or-equal
comparison and flush the value to zero in case of a "false" return from
hypercall_preempt_check()).
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/arch/x86/mm/mem_sharing.c
+++ b/xen/arch/x86/mm/mem_sharing.c
@@ -1290,11 +1290,13 @@ int relinquish_shared_pages(struct domai
set_rc = p2m->set_entry(p2m, gfn, _mfn(0), PAGE_ORDER_4K,
p2m_invalid, p2m_access_rwx);
ASSERT(set_rc != 0);
- count++;
+ count += 0x10;
}
+ else
+ ++count;
- /* Preempt every 2MiB. Arbitrary */
- if ( (count == 512) && hypercall_preempt_check() )
+ /* Preempt every 2MiB (shared) or 32MiB (unshared) - arbitrary. */
+ if ( !(count & 0x1ff0) && hypercall_preempt_check() )
{
p2m->next_shared_gfn_to_relinquish = gfn + 1;
rc = -EAGAIN;
[-- Attachment #2: x86-memshr-cleanup-preemption.patch --]
[-- Type: text/plain, Size: 1551 bytes --]
x86/memshr: fix preemption in relinquish_shared_pages()
For one, should hypercall_preempt_check() return false the first time
it gets called, it would never have got called again (because count,
being checked for equality, didn't get reset to zero).
And then, if there were a huge range of unshared pages, with count not
getting incremented at all in that case there would also not be any
preemption.
Fix this by using a biased increment (ratio 1:16 for unshared vs shared
pages), and check for certain bits in count to be clear rather than for
a specific value (the alternative would be to do a greater-or-equal
comparison and flush the value to zero in case of a "false" return from
hypercall_preempt_check()).
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/arch/x86/mm/mem_sharing.c
+++ b/xen/arch/x86/mm/mem_sharing.c
@@ -1290,11 +1290,13 @@ int relinquish_shared_pages(struct domai
set_rc = p2m->set_entry(p2m, gfn, _mfn(0), PAGE_ORDER_4K,
p2m_invalid, p2m_access_rwx);
ASSERT(set_rc != 0);
- count++;
+ count += 0x10;
}
+ else
+ ++count;
- /* Preempt every 2MiB. Arbitrary */
- if ( (count == 512) && hypercall_preempt_check() )
+ /* Preempt every 2MiB (shared) or 32MiB (unshared) - arbitrary. */
+ if ( !(count & 0x1ff0) && hypercall_preempt_check() )
{
p2m->next_shared_gfn_to_relinquish = gfn + 1;
rc = -EAGAIN;
[-- 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] 6+ messages in thread
* Re: [PATCH] x86/memshr: fix preemption in relinquish_shared_pages()
2013-12-17 9:07 [PATCH] x86/memshr: fix preemption in relinquish_shared_pages() Jan Beulich
@ 2013-12-17 10:43 ` Andrew Cooper
2013-12-17 14:44 ` Tim Deegan
1 sibling, 0 replies; 6+ messages in thread
From: Andrew Cooper @ 2013-12-17 10:43 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, Tim Deegan, Andres Lagar-Cavilla
[-- Attachment #1.1: Type: text/plain, Size: 1763 bytes --]
On 17/12/13 09:07, Jan Beulich wrote:
> For one, should hypercall_preempt_check() return false the first time
> it gets called, it would never have got called again (because count,
> being checked for equality, didn't get reset to zero).
>
> And then, if there were a huge range of unshared pages, with count not
> getting incremented at all in that case there would also not be any
> preemption.
>
> Fix this by using a biased increment (ratio 1:16 for unshared vs shared
> pages), and check for certain bits in count to be clear rather than for
> a specific value (the alternative would be to do a greater-or-equal
> comparison and flush the value to zero in case of a "false" return from
> hypercall_preempt_check()).
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
>
> --- a/xen/arch/x86/mm/mem_sharing.c
> +++ b/xen/arch/x86/mm/mem_sharing.c
> @@ -1290,11 +1290,13 @@ int relinquish_shared_pages(struct domai
> set_rc = p2m->set_entry(p2m, gfn, _mfn(0), PAGE_ORDER_4K,
> p2m_invalid, p2m_access_rwx);
> ASSERT(set_rc != 0);
> - count++;
> + count += 0x10;
> }
> + else
> + ++count;
>
> - /* Preempt every 2MiB. Arbitrary */
> - if ( (count == 512) && hypercall_preempt_check() )
> + /* Preempt every 2MiB (shared) or 32MiB (unshared) - arbitrary. */
> + if ( !(count & 0x1ff0) && hypercall_preempt_check() )
> {
> p2m->next_shared_gfn_to_relinquish = gfn + 1;
> rc = -EAGAIN;
>
>
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
[-- Attachment #1.2: Type: text/html, Size: 2645 bytes --]
[-- Attachment #2: 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] 6+ messages in thread
* Re: [PATCH] x86/memshr: fix preemption in relinquish_shared_pages()
2013-12-17 9:07 [PATCH] x86/memshr: fix preemption in relinquish_shared_pages() Jan Beulich
2013-12-17 10:43 ` Andrew Cooper
@ 2013-12-17 14:44 ` Tim Deegan
2013-12-17 15:23 ` [PATCH v2] " Jan Beulich
1 sibling, 1 reply; 6+ messages in thread
From: Tim Deegan @ 2013-12-17 14:44 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, Andres Lagar-Cavilla
At 09:07 +0000 on 17 Dec (1387267627), Jan Beulich wrote:
> For one, should hypercall_preempt_check() return false the first time
> it gets called, it would never have got called again (because count,
> being checked for equality, didn't get reset to zero).
>
> And then, if there were a huge range of unshared pages, with count not
> getting incremented at all in that case there would also not be any
> preemption.
>
> Fix this by using a biased increment (ratio 1:16 for unshared vs shared
> pages), and check for certain bits in count to be clear rather than for
> a specific value (the alternative would be to do a greater-or-equal
> comparison and flush the value to zero in case of a "false" return from
> hypercall_preempt_check()).
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/xen/arch/x86/mm/mem_sharing.c
> +++ b/xen/arch/x86/mm/mem_sharing.c
> @@ -1290,11 +1290,13 @@ int relinquish_shared_pages(struct domai
> set_rc = p2m->set_entry(p2m, gfn, _mfn(0), PAGE_ORDER_4K,
> p2m_invalid, p2m_access_rwx);
> ASSERT(set_rc != 0);
> - count++;
> + count += 0x10;
> }
> + else
> + ++count;
>
> - /* Preempt every 2MiB. Arbitrary */
> - if ( (count == 512) && hypercall_preempt_check() )
> + /* Preempt every 2MiB (shared) or 32MiB (unshared) - arbitrary. */
> + if ( !(count & 0x1ff0) && hypercall_preempt_check() )
This will check after each of for the first 15 gfns unless one of them
is shared (and potentially again after each rollover). I think it
would be clearer just to check for > 2000 and reset to 0.
Cheers,
Tim.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2] x86/memshr: fix preemption in relinquish_shared_pages()
2013-12-17 14:44 ` Tim Deegan
@ 2013-12-17 15:23 ` Jan Beulich
2013-12-17 15:26 ` Tim Deegan
0 siblings, 1 reply; 6+ messages in thread
From: Jan Beulich @ 2013-12-17 15:23 UTC (permalink / raw)
To: xen-devel; +Cc: Tim Deegan, Andres Lagar-Cavilla
[-- Attachment #1: Type: text/plain, Size: 1655 bytes --]
For one, should hypercall_preempt_check() return false the first time
it gets called, it would never have got called again (because count,
being checked for equality, didn't get reset to zero).
And then, if there were a huge range of unshared pages, with count not
getting incremented at all in that case there would also not be any
preemption.
Fix this by using a biased increment (ratio 1:16 for unshared vs shared
pages), and flushing the count to zero in case of a "false" return from
hypercall_preempt_check().
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Adjust continuation detection as per Tim's request.
--- b/xen/arch/x86/mm/mem_sharing.c
+++ b/xen/arch/x86/mm/mem_sharing.c
@@ -1290,15 +1290,21 @@ int relinquish_shared_pages(struct domai
set_rc = p2m->set_entry(p2m, gfn, _mfn(0), PAGE_ORDER_4K,
p2m_invalid, p2m_access_rwx);
ASSERT(set_rc != 0);
- count++;
+ count += 0x10;
}
+ else
+ ++count;
- /* Preempt every 2MiB. Arbitrary */
- if ( (count == 512) && hypercall_preempt_check() )
+ /* Preempt every 2MiB (shared) or 32MiB (unshared) - arbitrary. */
+ if ( count >= 0x2000 )
{
- p2m->next_shared_gfn_to_relinquish = gfn + 1;
- rc = -EAGAIN;
- break;
+ if ( hypercall_preempt_check() )
+ {
+ p2m->next_shared_gfn_to_relinquish = gfn + 1;
+ rc = -EAGAIN;
+ break;
+ }
+ count = 0;
}
}
[-- Attachment #2: x86-memshr-cleanup-preemption.patch --]
[-- Type: text/plain, Size: 1708 bytes --]
x86/memshr: fix preemption in relinquish_shared_pages()
For one, should hypercall_preempt_check() return false the first time
it gets called, it would never have got called again (because count,
being checked for equality, didn't get reset to zero).
And then, if there were a huge range of unshared pages, with count not
getting incremented at all in that case there would also not be any
preemption.
Fix this by using a biased increment (ratio 1:16 for unshared vs shared
pages), and flushing the count to zero in case of a "false" return from
hypercall_preempt_check().
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Adjust continuation detection as per Tim's request.
--- b/xen/arch/x86/mm/mem_sharing.c
+++ b/xen/arch/x86/mm/mem_sharing.c
@@ -1290,15 +1290,21 @@ int relinquish_shared_pages(struct domai
set_rc = p2m->set_entry(p2m, gfn, _mfn(0), PAGE_ORDER_4K,
p2m_invalid, p2m_access_rwx);
ASSERT(set_rc != 0);
- count++;
+ count += 0x10;
}
+ else
+ ++count;
- /* Preempt every 2MiB. Arbitrary */
- if ( (count == 512) && hypercall_preempt_check() )
+ /* Preempt every 2MiB (shared) or 32MiB (unshared) - arbitrary. */
+ if ( count >= 0x2000 )
{
- p2m->next_shared_gfn_to_relinquish = gfn + 1;
- rc = -EAGAIN;
- break;
+ if ( hypercall_preempt_check() )
+ {
+ p2m->next_shared_gfn_to_relinquish = gfn + 1;
+ rc = -EAGAIN;
+ break;
+ }
+ count = 0;
}
}
[-- 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] 6+ messages in thread
* Re: [PATCH v2] x86/memshr: fix preemption in relinquish_shared_pages()
2013-12-17 15:23 ` [PATCH v2] " Jan Beulich
@ 2013-12-17 15:26 ` Tim Deegan
2013-12-17 16:40 ` Andrés Lagar-Cavilla
0 siblings, 1 reply; 6+ messages in thread
From: Tim Deegan @ 2013-12-17 15:26 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, Andres Lagar-Cavilla
At 15:23 +0000 on 17 Dec (1387290199), Jan Beulich wrote:
> For one, should hypercall_preempt_check() return false the first time
> it gets called, it would never have got called again (because count,
> being checked for equality, didn't get reset to zero).
>
> And then, if there were a huge range of unshared pages, with count not
> getting incremented at all in that case there would also not be any
> preemption.
>
> Fix this by using a biased increment (ratio 1:16 for unshared vs shared
> pages), and flushing the count to zero in case of a "false" return from
> hypercall_preempt_check().
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Tim Deegan <tim@xen.org>
> ---
> v2: Adjust continuation detection as per Tim's request.
>
> --- b/xen/arch/x86/mm/mem_sharing.c
> +++ b/xen/arch/x86/mm/mem_sharing.c
> @@ -1290,15 +1290,21 @@ int relinquish_shared_pages(struct domai
> set_rc = p2m->set_entry(p2m, gfn, _mfn(0), PAGE_ORDER_4K,
> p2m_invalid, p2m_access_rwx);
> ASSERT(set_rc != 0);
> - count++;
> + count += 0x10;
> }
> + else
> + ++count;
>
> - /* Preempt every 2MiB. Arbitrary */
> - if ( (count == 512) && hypercall_preempt_check() )
> + /* Preempt every 2MiB (shared) or 32MiB (unshared) - arbitrary. */
> + if ( count >= 0x2000 )
> {
> - p2m->next_shared_gfn_to_relinquish = gfn + 1;
> - rc = -EAGAIN;
> - break;
> + if ( hypercall_preempt_check() )
> + {
> + p2m->next_shared_gfn_to_relinquish = gfn + 1;
> + rc = -EAGAIN;
> + break;
> + }
> + count = 0;
> }
> }
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] x86/memshr: fix preemption in relinquish_shared_pages()
2013-12-17 15:26 ` Tim Deegan
@ 2013-12-17 16:40 ` Andrés Lagar-Cavilla
0 siblings, 0 replies; 6+ messages in thread
From: Andrés Lagar-Cavilla @ 2013-12-17 16:40 UTC (permalink / raw)
To: Tim Deegan; +Cc: xen-devel, Andres Lagar-Cavilla, Jan Beulich
Acked-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
On Dec 17, 2013, at 12:26 PM, Tim Deegan <tim@xen.org> wrote:
> At 15:23 +0000 on 17 Dec (1387290199), Jan Beulich wrote:
>> For one, should hypercall_preempt_check() return false the first time
>> it gets called, it would never have got called again (because count,
>> being checked for equality, didn't get reset to zero).
>>
>> And then, if there were a huge range of unshared pages, with count not
>> getting incremented at all in that case there would also not be any
>> preemption.
>>
>> Fix this by using a biased increment (ratio 1:16 for unshared vs shared
>> pages), and flushing the count to zero in case of a "false" return from
>> hypercall_preempt_check().
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> Reviewed-by: Tim Deegan <tim@xen.org>
>
>> ---
>> v2: Adjust continuation detection as per Tim's request.
>>
>> --- b/xen/arch/x86/mm/mem_sharing.c
>> +++ b/xen/arch/x86/mm/mem_sharing.c
>> @@ -1290,15 +1290,21 @@ int relinquish_shared_pages(struct domai
>> set_rc = p2m->set_entry(p2m, gfn, _mfn(0), PAGE_ORDER_4K,
>> p2m_invalid, p2m_access_rwx);
>> ASSERT(set_rc != 0);
>> - count++;
>> + count += 0x10;
>> }
>> + else
>> + ++count;
>>
>> - /* Preempt every 2MiB. Arbitrary */
>> - if ( (count == 512) && hypercall_preempt_check() )
>> + /* Preempt every 2MiB (shared) or 32MiB (unshared) - arbitrary. */
>> + if ( count >= 0x2000 )
>> {
>> - p2m->next_shared_gfn_to_relinquish = gfn + 1;
>> - rc = -EAGAIN;
>> - break;
>> + if ( hypercall_preempt_check() )
>> + {
>> + p2m->next_shared_gfn_to_relinquish = gfn + 1;
>> + rc = -EAGAIN;
>> + break;
>> + }
>> + count = 0;
>> }
>> }
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-12-17 16:40 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-12-17 9:07 [PATCH] x86/memshr: fix preemption in relinquish_shared_pages() Jan Beulich
2013-12-17 10:43 ` Andrew Cooper
2013-12-17 14:44 ` Tim Deegan
2013-12-17 15:23 ` [PATCH v2] " Jan Beulich
2013-12-17 15:26 ` Tim Deegan
2013-12-17 16:40 ` Andrés Lagar-Cavilla
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.