Re: [Xen-devel] [PATCH 4/5] Xen/MCE: Abort live migration when vMCE occur

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Liu, Jinsong writes ("[Xen-devel] [PATCH 4/5] Xen/MCE: Abort live migration 
when vMCE occur"):
> This patch monitor the critical area of live migration (from vMCE
> point of view, the copypages stage of migration is the critical area
> while other areas are not).

Sorry for the delay reviewing this.

Just to be clear, can you explain what a vMCE is ?  I think I know but
I'm not entirely sure and it would be helpful if you'd confirm, as I
seem to have missed the background here.  I couldn't easily find the
0/5 posting of your series (in part because the tool you're using to
send your series doesn't link the messages together in the same
thread).

> If a vMCE occur at the critical area of live migration, there is
> risk that error data may be copied to the target. Currently we don't
> have convenient way to handle this case, so for the sake of safe, we
> abort it and try migration later (at that time broken page would not
> be mapped and copied to the target).

The "error data" that you refer to is erroneous page contents, or
something else ?

When you say "we abort it and try migration later", that's not
actually implemented in your patch, is it ?  What actually happens is
that the migration is aborted and the user is expected to retry later.

I think this situation deserves a specific error code at the very
least.  That specific error code should be plumbed up to libxl.

Wouldn't it be better to actually restart the migration somehow ?

I have some more minor comments about the implementation:

> @@ -1109,6 +1111,12 @@
>          goto out;
>      }
>  
> +    if ( xc_domain_vmce_monitor_start(xch, dom) )
> +    {
> +        PERROR("Error when start vmce monitor\n");

"Error starting vmc monitor" would be better English.  Messages sent
with PERROR should not have a \n.

> +    vmce_while_monitor = xc_domain_vmce_monitor_end(xch, dom);
> +    if ( vmce_while_monitor < 0 )
> +    {
> +        PERROR("Error when end vmce monitor\n");

Grammar and \n again.

> +    else if ( vmce_while_monitor > 0 )
> +    {
> +        fprintf(stderr, "vMCE occurred, abort this time and try later.\n");
> +        goto out;

This message should be sent with one of the logging macros, not
fprintf.  ERROR, probably.

Ian.

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