[PATCH] tools/libxenguest: Fix migration's debug option

[PATCH] tools/libxenguest: Fix migration's debug option

[Andrew Cooper]

The code has gone through many refactors, but the first refactor was the one
which broke it by inverting the check with respect to checkpointed streams.

Fixes: 7449fb36c6c8 ("migration/save: pass checkpointed_stream from libxl to libxc")
Reported-by: Olaf Hering <olaf@aepfle.de>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Ian Jackson <iwj@xenproject.org>
CC: Wei Liu <wl@xen.org>
CC: Olaf Hering <olaf@aepfle.de>

`xl migrate --debug` might not be perfect, but this at least brings it back to
mostly working.

I don't think dropping it is a sensible move.  In particular, it is invaluable
for testing the logdirty infrastructure when migrating a memtest VM.

If anyone has a clever idea to fix the grant problem, then we can.  It is
after all a debug option, without any specific prescribed behaviour.
---
 tools/libs/guest/xg_sr_save.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/libs/guest/xg_sr_save.c b/tools/libs/guest/xg_sr_save.c
index 2ba7c3200cd5..f0e2bd048d37 100644
--- a/tools/libs/guest/xg_sr_save.c
+++ b/tools/libs/guest/xg_sr_save.c
@@ -752,7 +752,7 @@ static int send_domain_memory_live(struct xc_sr_context *ctx)
     if ( rc )
         goto out;
 
-    if ( ctx->save.debug && ctx->stream_type != XC_STREAM_PLAIN )
+    if ( ctx->save.debug && ctx->stream_type == XC_STREAM_PLAIN )
     {
         rc = verify_frames(ctx);
         if ( rc )
-- 
2.11.0

Re: [PATCH] tools/libxenguest: Fix migration's debug option

[Olaf Hering]

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

Am Fri, 2 Jul 2021 20:03:42 +0100
schrieb Andrew Cooper <andrew.cooper3@citrix.com>:

> The code has gone through many refactors, but the first refactor was the one
> which broke it by inverting the check with respect to checkpointed streams.
> 
> Fixes: 7449fb36c6c8 ("migration/save: pass checkpointed_stream from libxl to libxc")
> Reported-by: Olaf Hering <olaf@aepfle.de>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Olaf Hering <olaf@aepfle.de>

Olaf

[-- Attachment #2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] tools/libxenguest: Fix migration's debug option

[Jan Beulich]

On 02.07.2021 21:03, Andrew Cooper wrote:
> The code has gone through many refactors, but the first refactor was the one
> which broke it by inverting the check with respect to checkpointed streams.
> 
> Fixes: 7449fb36c6c8 ("migration/save: pass checkpointed_stream from libxl to libxc")
> Reported-by: Olaf Hering <olaf@aepfle.de>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

> ---
> CC: Ian Jackson <iwj@xenproject.org>
> CC: Wei Liu <wl@xen.org>
> CC: Olaf Hering <olaf@aepfle.de>
> 
> `xl migrate --debug` might not be perfect, but this at least brings it back to
> mostly working.
> 
> I don't think dropping it is a sensible move.  In particular, it is invaluable
> for testing the logdirty infrastructure when migrating a memtest VM.
> 
> If anyone has a clever idea to fix the grant problem, then we can.  It is
> after all a debug option, without any specific prescribed behaviour.

What is "the grant problem" referring to here? Neither anything above
nor the offending original commit has any reference to grants, or a
problem with them.

Jan

Re: [PATCH] tools/libxenguest: Fix migration's debug option

[Olaf Hering]

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

Am Mon, 5 Jul 2021 09:57:21 +0200
schrieb Jan Beulich <jbeulich@suse.com>:

> What is "the grant problem" referring to here? Neither anything above
> nor the offending original commit has any reference to grants, or a
> problem with them.

When the guest is paused during final transit, the backends will continue to write into domU memory. As a result the final additional iteration to verify memory on both sides will always see errors. The code has no way to know for which pfn such mismatches in page content can safely be ignored.


Olaf

[-- Attachment #2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] tools/libxenguest: Fix migration's debug option

[Jan Beulich]

On 05.07.2021 10:02, Olaf Hering wrote:
> Am Mon, 5 Jul 2021 09:57:21 +0200
> schrieb Jan Beulich <jbeulich@suse.com>:
> 
>> What is "the grant problem" referring to here? Neither anything above
>> nor the offending original commit has any reference to grants, or a
>> problem with them.
> 
> When the guest is paused during final transit, the backends will
> continue to write into domU memory. As a result the final additional
> iteration to verify memory on both sides will always see errors.

I see. A similar problem then exists with at least the FIFO event
channel per-vCPU control blocks?

> The code has no way to know for which pfn such mismatches in page
> content can safely be ignored.

Well, in principle this can be known, but it's expensive: For a
paused domain the grant table can't change anymore. Any pages
referenced by a valid non-r/o grant table entry could in principle
change.

Jan

Re: [PATCH] tools/libxenguest: Fix migration's debug option

[Olaf Hering]

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

Am Mon, 5 Jul 2021 10:23:00 +0200
schrieb Jan Beulich <jbeulich@suse.com>:

> I see. A similar problem then exists with at least the FIFO event
> channel per-vCPU control blocks?

I have not done any debugging how the pages differ and what they are actually used for.

My guess was that it might be activity from the backends, particularly netback. I found no API to query the usage of a page, so I declared the interface broken..


Olaf

[-- Attachment #2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] tools/libxenguest: Fix migration's debug option

[Jan Beulich]

On 05.07.2021 10:32, Olaf Hering wrote:
> Am Mon, 5 Jul 2021 10:23:00 +0200
> schrieb Jan Beulich <jbeulich@suse.com>:
> 
>> I see. A similar problem then exists with at least the FIFO event
>> channel per-vCPU control blocks?
> 
> I have not done any debugging how the pages differ and what they are actually used for.
> 
> My guess was that it might be activity from the backends, particularly
> netback. I found no API to query the usage of a page, so I declared the
> interface broken..

"The interface" being which one? The tool stack can map the guest's
grant table, so it is in the position to find out about all grants
without further hypervisor help. Afaict the same isn't true for the
FIFO event channel control blocks, though, and maybe there are
further ways by which pages may get modified for a paused guest.

Jan

Re: [PATCH] tools/libxenguest: Fix migration's debug option

[Olaf Hering]

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

Am Mon, 5 Jul 2021 11:19:59 +0200
schrieb Jan Beulich <jbeulich@suse.com>:

> "The interface" being which one? The tool stack can map the guest's
> grant table, so it is in the position to find out about all grants
> without further hypervisor help.

The interface means the code behind verify_frames.

If there are indeed ways to query which pages belong to grants, how would the toolstack need to do that?


Olaf

[-- Attachment #2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] tools/libxenguest: Fix migration's debug option

[Jan Beulich]

On 05.07.2021 11:25, Olaf Hering wrote:
> Am Mon, 5 Jul 2021 11:19:59 +0200
> schrieb Jan Beulich <jbeulich@suse.com>:
> 
>> "The interface" being which one? The tool stack can map the guest's
>> grant table, so it is in the position to find out about all grants
>> without further hypervisor help.
> 
> The interface means the code behind verify_frames.
> 
> If there are indeed ways to query which pages belong to grants, how would the toolstack need to do that?

Map the grant table of the guest and walk it, recording any MFN for
which at least one valid r/w grant exists.

Jan

Re: [PATCH] tools/libxenguest: Fix migration's debug option

[Andrew Cooper]

On 05/07/2021 10:31, Jan Beulich wrote:
> On 05.07.2021 11:25, Olaf Hering wrote:
>> Am Mon, 5 Jul 2021 11:19:59 +0200
>> schrieb Jan Beulich <jbeulich@suse.com>:
>>
>>> "The interface" being which one? The tool stack can map the guest's
>>> grant table, so it is in the position to find out about all grants
>>> without further hypervisor help.
>> The interface means the code behind verify_frames.
>>
>> If there are indeed ways to query which pages belong to grants, how would the toolstack need to do that?
> Map the grant table of the guest and walk it, recording any MFN for
> which at least one valid r/w grant exists.

That doesn't help - Its still racy with in-flight IO.  Also with updates
from Xen such as the wallclocks.

The only way to fix the IO problem is to disconnect the blk/net rings
before doing the final sweep for frames, but that clobbers any ability
to restart the VM on the source side if things go wrong at the destination.

I don't have an answer at all for the vcpu info frames.

~Andrew

Re: [PATCH] tools/libxenguest: Fix migration's debug option

[Jan Beulich]

On 05.07.2021 12:06, Andrew Cooper wrote:
> On 05/07/2021 10:31, Jan Beulich wrote:
>> On 05.07.2021 11:25, Olaf Hering wrote:
>>> Am Mon, 5 Jul 2021 11:19:59 +0200
>>> schrieb Jan Beulich <jbeulich@suse.com>:
>>>
>>>> "The interface" being which one? The tool stack can map the guest's
>>>> grant table, so it is in the position to find out about all grants
>>>> without further hypervisor help.
>>> The interface means the code behind verify_frames.
>>>
>>> If there are indeed ways to query which pages belong to grants, how would the toolstack need to do that?
>> Map the grant table of the guest and walk it, recording any MFN for
>> which at least one valid r/w grant exists.
> 
> That doesn't help - Its still racy with in-flight IO.

Well, I meant the recorded data to be used to simply not verify
those frames.

>  Also with updates from Xen such as the wallclocks.

This doesn't occur for a paused domain, does it?

> The only way to fix the IO problem is to disconnect the blk/net rings
> before doing the final sweep for frames, but that clobbers any ability
> to restart the VM on the source side if things go wrong at the destination.
> 
> I don't have an answer at all for the vcpu info frames.

Yeah, they fall in the same category as the FIFO control pages, as
they contain evtchn_{pending_sel,upcall_pending}.

Jan