Re: [Xen-devel] [PATCH 07/10 V7] libxl: use the API to setup/teardown network buffering [and 1 more messages]

[Shriram Rajagopalan <rshriram@xxxxxxxxx>]

On May 2, 2014 11:08 AM, "Ian Jackson" <Ian.Jackson@xxxxxxxxxxxxx> wrote:
>
> Shriram Rajagopalan writes ("Re: [PATCH 07/10 V7] libxl: use the API to setup/teardown network buffering [and 1 more messages]"):
> > On Wed, Apr 23, 2014 at 11:02 AM, Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>
> > wrote:
> > Â Â 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.
>
> Right.
>
> > 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.
>
> Yes.
>
> > 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).
>
> This isn't quite the right distinction. ÂYou should use an
> asynchronous style for anything which might block. ÂSo it is OK to
> synchronously make fast syscalls. Â(I assume that the disk/network
> control syscalls are fast.)
>
> And in this context, blocking includes child processes, and calls to
> (u)sleep.
>

Isn't that what the patches are doing currently?
Setup, teardown and (u)sleep are AO subops (following the option 1 style you suggested), while the rest of Remus code runs every checkpoint in non AO fashion (syscalls to disk, libnl calls (which in turn issue syscalls) for network)

> > 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?
>
> I'm not sure I quite follow

The indirection helps separate the control flow between Remus and non Remus executions. Currently both end up in domain_suspend_done which as you stated, seems ad-hoc, bolting the Remus error path into a function that is used for indicating completion of AO libxl calls like domain suspend, save, etc.
This suggestion was more inline with the option (1) you suggested.

> but I think you are still mixing the two
> approaches I discuss above.

But which parts of the code? As I said earlier, one part of the code (setup, teardown, etc) follows option 1, while the other path (per checkpoint execution) follows option 2, because they are not blocking in anyway.

>ÂI would like you to pick one.
>
> If you're not sure, you should probably pick option 1.
>
> Thanks,
> Ian.
>

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