Re: [Xen-devel] [PATCH 4 of 5 V3] tools/libxl: Control network buffering in remus callbacks [and 1 more messages]

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Shriram Rajagopalan writes ("Re: [PATCH 4 of 5 V3] tools/libxl: Control network 
buffering in remus callbacks"):
> Nanosleep was more of my personal preference,

I don't think that's a good enough reason for the churn, but as I say
this really ought to be replaced with use of a timeout event.

> just as the '!!' above was Andrew's suggestion, due to the fact that
> libxl__domain_suspend_common_callback() does not strictly return
> boolean.

Are you looking at a different tree to me ?  I have checked the return
statements in libxl__domain_suspend_common_callback and they are all
"return 0" or "return 1".

> But I agree that renaming that variable makes more sense.

Good :-).  Thanks.

> As far as using the event loop timers, I couldn't find anything
> suitable in libxl_event.h/c that I could use instead of
> usleep/nanosleep.  Specifically, they were all asynchronous, while
> the caller of these functions is libxc's xc_domain_save infinite
> loop (synchronous).

No, you need to make it an asynchronous call.

The libxl save helper machinery arranges to run xc_domain_save in a
subprocess so that the main program can continue asynchronously.

It's true that the suspend checkpoint is currently synchronous.  It
needs to be made synchronous by telling the stub generator
(libxl_save_msgs_gen.pl) to generate an asynchronous binding, like it
does for switch_qemu_logdirty.

Perhaps it would be helpful if I provided a pre-patch to make that
change for you.

>     > +    if (!remus_ctx->netbuf_ctx) return is_suspended;
>     > +
>     > +    if (is_suspended) {
>     > +        if (libxl__remus_netbuf_start_new_epoch(gc, dss->domid,
>     > +                                                remus_ctx))
>     > +            return !is_suspended;
>
>     This construction is rather odd.  Why return !is_suspended when you
>     know that !!is_suspended ?
...
> If a new network buffer cannot be created (for the next epoch), then
> return an error. This ends up terminating the checkpointing process.

Yes.  But why not "return 0" ?

Thanks,
Ian.

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