On Wed, Apr 23, 2014 at 11:02 AM, Ian Jackson <Ian.Jackson@eu.citrix.com>wrote:

> Yang Hongyang writes ("[PATCH V9 05/12] remus: remus device core and APIs
> to setup/teardown"):
> > diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> > index 33b62a2..421ae24 100644
> > --- a/tools/libxl/libxl_internal.h
> > +++ b/tools/libxl/libxl_internal.h
> > @@ -2457,8 +2457,35 @@ typedef struct libxl__logdirty_switch {
> >      libxl__ev_time timeout;
> >  } libxl__logdirty_switch;
> >
> > +typedef struct libxl__remus_state {
> > +    libxl__domain_suspend_state *dss;
> > +    libxl__egc *egc;
> > +
> > +    /* private */
> > +    int saved_rc;
> > +    /* Opaque context containing device related stuff */
> > +    void *device_state;
> > +} libxl__remus_state;
>
> I'm afraid that the interface between the remus code and the rest of
> the code is still not very clear.
>
> Earlier, I wrote:
>
>   > [...]  I wonder if it might not be better to provide a firmer
>   > interface between the remus code and the rest of the save/restore
>   > machinery.  That is, have an explicit callback function recorded
>   > by the save/restore code which is called back by the remus
>   > machinery when it has done its work.  What do you think ?
>   >
>   > I think having the flow of control spring off into libxl_remus.c and
>   > magically come back by libxl_remus.c knowing to call
>   > domain_suspend_done is rather opaque.
>
> I think you have basically two options:
>
> 1. Make the remus part of this be a fully self-contained standard
>    asynchronous callback-based suboperation, like libxl__xswait,
>    libxl__bootloader, et al.
>
>    In this case you should rigorously follow the existing patterns,
>    defining a clear interface between the two parts, providing a
>    callback function set by the caller, etc.
>
> 2. Integrate the remus part into the suspend/resume code in an
>    ad hoc fashion, with extremely clear comments everywhere about the
>    expected interface, and no extraneous moving parts.
>
> At the moment you seem to have mixed these two approaches.
>

Sorry, I missed the previous comment of yours. The two options you
note are bit more clearer than the previous comment. And I also
agree that the current approach is mixing options 1 & 2.

The entire Remus code (executed from start to end) is one giant async op.
Internally, per checkpoint, the code executes for no more than tens of
milliseconds at max, with the exception of sleeping until the next
checkpoint
needs to be taken.

Doing checkpoint related work (i.e., syscalls to control disk/network
buffers)
in an async op is an overkill. So, they are integrated into the
suspend/resume
infrastructure (option 2)

The async op is useful (please correct me if I am wrong) if the op runs for
a long time, such that you don't want users of libxl to block. Which is
exactly
why the setup/teardown and the sleep_until_next_checkpoint operations
are ao suboperations. (option 1).


All said, perhaps, it may be more clear to add a level of indirection:
 make domain_suspend_done a callback field in libxl__domain_state.

Change all direct callers of domain_suspend_done to invoke this callback.

In this way,
* domain_suspend -> domain_suspend_done (for normal suspend/save operations)
* domain_remus_start->domain_remus_terminated

What do you think?