Re: [PATCH 4 of 6 V3] libxl: support suspend_cancel in domain_resume

[Ian Campbell <Ian.Campbell@citrix.com>]

On Thu, 2012-02-09 at 18:21 +0000, Shriram Rajagopalan wrote:
> On 2012-02-09, at 1:16 AM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> 
> > On Fri, 2012-02-03 at 06:50 +0000, rshriram@cs.ubc.ca wrote:
> >> # 
> > 
> > Previously this code would have been equivalent to passing 0 not 1. It
> > may be ok to change this but it should be separate.
> 
> > However I'm a bit
> > dubious about changing it without adding some code to detect if the
> > guest supports it.
> 
> It requires writing the suspend-cancel entry to xenstore on domain creation (if xen says that the guest supports it)
> 
>  And then read this field once during initialization in checkpoint/Remus code.
> 
> I haven't properly investigated the domain create code path. Any pointers on where I should start ?
> 
> > (I know libxl_domain_resume is currently broken for
> > the non-cooperative suspend
> 
> It's broken in such a way that neither domain checkpoint or Remus work.
> I ll take up your comment on the previous version of the patch:
>  Add a new public (or internal) API 
>  libxl_domain_suspend_cancel
> 
> and make the checkpoint and Remus related code call this instead of the domain_resume.

I don't think there is any need for that unless you think that is
actually a better API in its own right. Otherwise please just leave the
semantics of the existing calls to libxl_domain_resume alone when you
add the flag (i.e. pass 0 as the co-operative flag at all existing call
sites, retaining the existing behaviour). Other than that your patch was
fine.

Changing existing places to use co-operative resume without also
validating that the guest supports it is wrong in the normal migration
case and also masks a real bug in libxl_domain_resume which needs to be
fixed (it's on my TODO list) not papered over by assuming that all
guests know about cooperative resume.

If you need to make that argument conditional on Remus in subsequent
patches then that is fine since Remus implies, at least to some degree,
support for co-operative resume.

Ian.

> 
> Later, after adding the suspend-cancel xenstore entry creation patch, I can switch the code to using the above function with the cooperative flag.
> 
> > case but we shouldn't paper over that)
> > 
> > Ian.
> > 
> >