* [Xen-devel] [PATCH] x86/shadow: don't enable shadow mode with too small a shadow allocation (part 2)
@ 2019-09-04 7:55 Jan Beulich
2019-09-04 11:28 ` Andrew Cooper
2019-09-05 7:35 ` Tim Deegan
0 siblings, 2 replies; 4+ messages in thread
From: Jan Beulich @ 2019-09-04 7:55 UTC (permalink / raw)
To: xen-devel
Cc: George Dunlap, Andrew Cooper, Tim Deegan, Wei Liu, Roger Pau Monné
Commit 2634b997af ("x86/shadow: don't enable shadow mode with too small
a shadow allocation") was incomplete: The adjustment done there to
shadow_enable() is also needed in shadow_one_bit_enable(). The (new)
problem report was (apparently) a failed PV guest migration followed by
another migration attempt for that same guest. Disabling log-dirty mode
after the first one had left a couple of shadow pages allocated (perhaps
something that also wants fixing), and hence the second enabling of
log-dirty mode wouldn't have allocated anything further.
Reported-by: James Wang <jnwang@suse.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/arch/x86/mm/shadow/common.c
+++ b/xen/arch/x86/mm/shadow/common.c
@@ -2864,7 +2864,8 @@ static int shadow_one_bit_enable(struct
mode |= PG_SH_enable;
- if ( d->arch.paging.shadow.total_pages == 0 )
+ if ( d->arch.paging.shadow.total_pages <
+ sh_min_allocation(d) + d->arch.paging.shadow.p2m_pages )
{
/* Init the shadow memory allocation if the user hasn't done so */
if ( shadow_set_allocation(d, 1, NULL) != 0 )
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Xen-devel] [PATCH] x86/shadow: don't enable shadow mode with too small a shadow allocation (part 2)
2019-09-04 7:55 [Xen-devel] [PATCH] x86/shadow: don't enable shadow mode with too small a shadow allocation (part 2) Jan Beulich
@ 2019-09-04 11:28 ` Andrew Cooper
2019-09-04 12:02 ` Jan Beulich
2019-09-05 7:35 ` Tim Deegan
1 sibling, 1 reply; 4+ messages in thread
From: Andrew Cooper @ 2019-09-04 11:28 UTC (permalink / raw)
To: Jan Beulich, xen-devel
Cc: George Dunlap, Tim Deegan, Wei Liu, Roger Pau Monné
On 04/09/2019 08:55, Jan Beulich wrote:
> Commit 2634b997af ("x86/shadow: don't enable shadow mode with too small
> a shadow allocation") was incomplete: The adjustment done there to
> shadow_enable() is also needed in shadow_one_bit_enable(). The (new)
> problem report was (apparently) a failed PV guest migration followed by
> another migration attempt for that same guest. Disabling log-dirty mode
> after the first one had left a couple of shadow pages allocated (perhaps
> something that also wants fixing), and hence the second enabling of
> log-dirty mode wouldn't have allocated anything further.
>
> Reported-by: James Wang <jnwang@suse.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/xen/arch/x86/mm/shadow/common.c
> +++ b/xen/arch/x86/mm/shadow/common.c
> @@ -2864,7 +2864,8 @@ static int shadow_one_bit_enable(struct
>
> mode |= PG_SH_enable;
>
> - if ( d->arch.paging.shadow.total_pages == 0 )
> + if ( d->arch.paging.shadow.total_pages <
> + sh_min_allocation(d) + d->arch.paging.shadow.p2m_pages )
This logic looks suspect. The change from == 0 to < min looks fine, and
feels like the right thing to do.
However, sh_min_allocation() should by definition be the minimum
quantity of pages necessary for things to function, which makes the + on
the end look wrong.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Xen-devel] [PATCH] x86/shadow: don't enable shadow mode with too small a shadow allocation (part 2)
2019-09-04 11:28 ` Andrew Cooper
@ 2019-09-04 12:02 ` Jan Beulich
0 siblings, 0 replies; 4+ messages in thread
From: Jan Beulich @ 2019-09-04 12:02 UTC (permalink / raw)
To: Andrew Cooper
Cc: George Dunlap, xen-devel, Tim Deegan, Wei Liu, Roger Pau Monné
On 04.09.2019 13:28, Andrew Cooper wrote:
> On 04/09/2019 08:55, Jan Beulich wrote:
>> Commit 2634b997af ("x86/shadow: don't enable shadow mode with too small
>> a shadow allocation") was incomplete: The adjustment done there to
>> shadow_enable() is also needed in shadow_one_bit_enable(). The (new)
>> problem report was (apparently) a failed PV guest migration followed by
>> another migration attempt for that same guest. Disabling log-dirty mode
>> after the first one had left a couple of shadow pages allocated (perhaps
>> something that also wants fixing), and hence the second enabling of
>> log-dirty mode wouldn't have allocated anything further.
>>
>> Reported-by: James Wang <jnwang@suse.com>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> --- a/xen/arch/x86/mm/shadow/common.c
>> +++ b/xen/arch/x86/mm/shadow/common.c
>> @@ -2864,7 +2864,8 @@ static int shadow_one_bit_enable(struct
>>
>> mode |= PG_SH_enable;
>>
>> - if ( d->arch.paging.shadow.total_pages == 0 )
>> + if ( d->arch.paging.shadow.total_pages <
>> + sh_min_allocation(d) + d->arch.paging.shadow.p2m_pages )
>
> This logic looks suspect. The change from == 0 to < min looks fine, and
> feels like the right thing to do.
>
> However, sh_min_allocation() should by definition be the minimum
> quantity of pages necessary for things to function, which makes the + on
> the end look wrong.
Well, the change here brings shadow_one_bit_enable() in line with
shadow_enable(). What you suggest is a 2nd change, also requiring
an adjustment to shadow_set_allocation(). Back when putting
together 2634b997af for some reason I thought it would be
correct to move the p2m_pages addition into sh_min_allocation(),
but looking again now I think this ought to be quite fine.
There's a possible argument against doing this (or against it
being correct), though: When p2m_pages is already non-zero, why
would it be correct for sh_min_allocation() to add in that value
_and_ also account for to-be-allocated P2M pages itself?
Shouldn't it then rather be the maximum of the current and
prospected values? (To me this is a clear argument for not
folding in such an adjustment into the patch here.)
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Xen-devel] [PATCH] x86/shadow: don't enable shadow mode with too small a shadow allocation (part 2)
2019-09-04 7:55 [Xen-devel] [PATCH] x86/shadow: don't enable shadow mode with too small a shadow allocation (part 2) Jan Beulich
2019-09-04 11:28 ` Andrew Cooper
@ 2019-09-05 7:35 ` Tim Deegan
1 sibling, 0 replies; 4+ messages in thread
From: Tim Deegan @ 2019-09-05 7:35 UTC (permalink / raw)
To: Jan Beulich
Cc: George Dunlap, xen-devel, Roger Pau Monné, Wei Liu, Andrew Cooper
At 09:55 +0200 on 04 Sep (1567590940), Jan Beulich wrote:
> Commit 2634b997af ("x86/shadow: don't enable shadow mode with too small
> a shadow allocation") was incomplete: The adjustment done there to
> shadow_enable() is also needed in shadow_one_bit_enable(). The (new)
> problem report was (apparently) a failed PV guest migration followed by
> another migration attempt for that same guest. Disabling log-dirty mode
> after the first one had left a couple of shadow pages allocated (perhaps
> something that also wants fixing), and hence the second enabling of
> log-dirty mode wouldn't have allocated anything further.
>
> Reported-by: James Wang <jnwang@suse.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Tim Deegan <tim@xen.org>
> --- a/xen/arch/x86/mm/shadow/common.c
> +++ b/xen/arch/x86/mm/shadow/common.c
> @@ -2864,7 +2864,8 @@ static int shadow_one_bit_enable(struct
>
> mode |= PG_SH_enable;
>
> - if ( d->arch.paging.shadow.total_pages == 0 )
> + if ( d->arch.paging.shadow.total_pages <
> + sh_min_allocation(d) + d->arch.paging.shadow.p2m_pages )
> {
> /* Init the shadow memory allocation if the user hasn't done so */
> if ( shadow_set_allocation(d, 1, NULL) != 0 )
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-09-05 7:35 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-04 7:55 [Xen-devel] [PATCH] x86/shadow: don't enable shadow mode with too small a shadow allocation (part 2) Jan Beulich
2019-09-04 11:28 ` Andrew Cooper
2019-09-04 12:02 ` Jan Beulich
2019-09-05 7:35 ` Tim Deegan
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.