Re: [PATCH 2/2] Introduce migration precopy policy

[Paul Durrant <Paul.Durrant@citrix.com>]

> -----Original Message-----
> From: Wei Liu [mailto:wei.liu2@citrix.com]
> Sent: 18 September 2017 14:20
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: Jennifer Herbert <jennifer.herbert@citrix.com>; Ian Jackson
> <Ian.Jackson@citrix.com>; Wei Liu <wei.liu2@citrix.com>; xen-
> devel@lists.xenproject.org; jtotto@uwaterloo.ca
> Subject: Re: [Xen-devel] [PATCH 2/2] Introduce migration precopy policy
> 
> On Mon, Sep 18, 2017 at 10:05:07AM +0100, Paul Durrant wrote:
> > > -----Original Message-----
> > > From: Xen-devel [mailto:xen-devel-bounces@lists.xen.org] On Behalf Of
> > > Jennifer Herbert
> > > Sent: 14 September 2017 16:34
> > > To: Ian Jackson <Ian.Jackson@citrix.com>; Wei Liu <wei.liu2@citrix.com>;
> > > xen-devel@lists.xenproject.org; jtotto@uwaterloo.ca
> > > Cc: Jennifer Herbert <jennifer.herbert@citrix.com>
> > > Subject: [Xen-devel] [PATCH 2/2] Introduce migration precopy policy
> > >
> > > This Patch allows a migration precopy policy to be specified.
> > >
> > > The precopy phase of the xc_domain_save() live migration algorithm has
> > > historically been implemented to run until either a) (almost) no pages
> > > are dirty or b) some fixed, hard-coded maximum number of precopy
> > > iterations has been exceeded.  This policy and its implementation are
> > > less than ideal for a few reasons:
> > > - the logic of the policy is intertwined with the control flow of the
> > >   mechanism of the precopy stage
> > > - it can't take into account facts external to the immediate
> > >   migration context, such external state transfer state, interactive
> > >   user input, or the passage of wall-clock time.
> > > - it does not permit the user to change their mind, over time, about
> > >   what to do at the end of the precopy (they get an unconditional
> > >   transition into the stop-and-copy phase of the migration)
> > >
> > > To permit callers to implement arbitrary higher-level policies governing
> > > when the live migration precopy phase should end, and what should be
> > > done next:
> > > - add a precopy_policy() callback to the xc_domain_save() user-supplied
> > >   callbacks
> > > - during the precopy phase of live migrations, consult this policy after
> > >   each batch of pages transmitted and take the dictated action, which
> > >   may be to a) abort the migration entirely, b) continue with the
> > >   precopy, or c) proceed to the stop-and-copy phase.
> > > - provide an implementation of the old policy, used when
> > >   precopy_policy callback  is not provided.
> > >
> > > Signed-off-by: Jennifer Herbert <Jennifer.Herbert@citrix.com>
> > >
> > > ---
> > >
> > > This is updated/modified subset of patch 7/20, part of
> > > Joshua Otto's "Add postcopy live migration support." patch,
> > > dated 27th March 2017.  As indicated on the original thread,
> > > I wish to make use of this this within the XenServer product.
> > > I hope this will aid Josh in pushing the remainder of his series.
> >
> > Does this patch need to carry Joshua's s-o-b, or at least 'suggested-by'?
> 
> I agree. We need to retain Joshua's s-o-b because this patch is based on
> his.
> 
> >
> > > ---
> > >  tools/libxc/include/xenguest.h |  19 ++++++++
> > >  tools/libxc/xc_sr_common.h     |   7 ++-
> > >  tools/libxc/xc_sr_save.c       | 102 +++++++++++++++++++++++++++++--
> ----
> > > ------
> > >  3 files changed, 94 insertions(+), 34 deletions(-)
> > >
> > > diff --git a/tools/libxc/include/xenguest.h
> b/tools/libxc/include/xenguest.h
> > > index 6626f0c..d5908dc 100644
> > > --- a/tools/libxc/include/xenguest.h
> > > +++ b/tools/libxc/include/xenguest.h
> > > @@ -39,6 +39,14 @@
> > >   */
> > >  struct xenevtchn_handle;
> > >
> > > +/* For save's precopy_policy(). */
> > > +struct precopy_stats
> > > +{
> > > +    unsigned iteration;
> > > +    unsigned total_written;
> > > +    long dirty_count; /* -1 if unknown */
> > > +};
> > > +
> > >  /* callbacks provided by xc_domain_save */
> > >  struct save_callbacks {
> > >      /* Called after expiration of checkpoint interval,
> > > @@ -46,6 +54,17 @@ struct save_callbacks {
> > >       */
> > >      int (*suspend)(void* data);
> > >
> > > +    /* Called after every batch of page data sent during the precopy
> phase of
> > > a
> > > +     * live migration to ask the caller what to do next based on the current
> > > +     * state of the precopy migration.
> > > +     */
> > > +#define XGS_POLICY_ABORT          (-1) /* Abandon the migration entirely
> > > and
> > > +                                        * tidy up. */
> > > +#define XGS_POLICY_CONTINUE_PRECOPY 0  /* Remain in the precopy
> > > phase. */
> > > +#define XGS_POLICY_STOP_AND_COPY    1  /* Immediately suspend
> and
> > > transmit the
> > > +                                        * remaining dirty pages. */
> > > +    int (*precopy_policy)(struct precopy_stats stats, void *data);
> >
> > Do we really want to be passing the struct, rather than a pointer to it?
> >
> 
> IIRC that was discussed in the past. We passed the struct because we
> couldn't pass pointers across process boundary.

Ok, that's fine. As long as there is a good reason :-)

  Paul

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