Re: [Xen-devel] [PATCH 15/19] libxl: suspend: Async xenstore pvcontrol wait

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Thu, 2014-03-13 at 18:26 +0000, Ian Jackson wrote:
> Ian Campbell writes ("Re: [PATCH 15/19] libxl: suspend: Async xenstore 
> pvcontrol wait"):
> > On Tue, 2014-03-04 at 14:56 +0000, Ian Jackson wrote:
> > > When negotiating guest suspend via the xenstore pvcontrol protocol
> > > (ie when the guest does NOT support the evtchn fast suspend protocol):
> ...
> > > -    /*
> > > -     * Final check for guest acknowledgement. The guest may have
> > > -     * acknowledged while we were cancelling the request in which
> > > -     * case we lost the race while cancelling and should continue.
> > 
> > This behaviour has gone completely now?
> 
> No, this possibility still exists.  This is handled by the rc ==
> ERROR_TIMEDOUT branch in domain_suspend_common_pvcontrol_suspending.
> The comment there (now) reads:
> 
>         /*
>          * Guest appears to not be responding. Cancel the suspend
>          * request.
>          *
>          * We re-read the suspend node and clear it within a
>          * transaction in order to handle the case where we race
>          * against the guest catching up and acknowledging the request
>          * at the last minute.
>          */

I was querying it because the original code appears to have both of the
above ("Guest appears to not..." and "Final check for..."). IOW even
after the final check transaction to clear it checks state again.

I think what you've done is make that "final" check within the
transaction loop with:
    if (domain_suspend_pvcontrol_acked(state))
        break;

Would it make sense to include something akin to the "Final check"
comment just before this to clarify?

> 
> > > +    assert(domain_suspend_pvcontrol_acked(state));
> > >      LOG(DEBUG, "guest acknowledged suspend request");
> > > +
> > > +    libxl__xs_transaction_abort(gc, &t);
> > 
> > Is this right/proper in the success case?
> 
> Yes.  Because it has then already been committed.  This is exactly the
> pattern from the usage comment for libxl__xs_transaction_* in
> libxl_internal.h.

That usage comment doesn't have an abort after a successful commit
though i.e. in the "if (!rc) break;" case it drops straight through to
the success and return something case.

I think the subtlety is that the transaction loop here differs slightly
from the canonical example because of the "final check" break quoted
above -- since that exits the transaction successfully without actually
needing to complete the transaction.

Assuming aborting a committed transaction is ok then a comment might be
worthwhile. Or do the abort just before the exceptional success break,
which is a bit unconventional but then so is the check itself.

Ian.

> > If rc != TIMEOUT t may not have been started, although it is initialised
> > so I suppose this is intended to be ok.
> 
> Yes.  Precisely.
> 
> Thanks for the careful review :-).
> 
> Ian.



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