Re: [Xen-devel] [PATCH 8/9] libxl: Introduce libxl__ev_devstate

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Tue, 2012-01-24 at 17:33 +0000, Ian Jackson wrote:
> Ian Campbell writes ("Re: [Xen-devel] [PATCH 8/9] libxl: Introduce 
> libxl__ev_devstate"):
> > On Fri, 2012-01-13 at 19:25 +0000, Ian Jackson wrote:
> > > Provide a new-style asynchronous facility for waiting for device
> > > states on xenbus.  This will replace libxl__wait_for_device_state,
> > > after the callers have been updated in later patches.
> > 
> > Is event-with-timeout likely to be a useful/common enough pattern to be
> > worth baking into the infrastructure/helpers rather than implementing
> > just for this one event type? (if yes then, "I will refactor for the
> > second user is a valid response").
> 
> I'm not convinced.  I thought of this but I think it would result in
> flabby code - all the libxl__ev_register functions would gain a new
> timeout parameter (and note that the timeout machinery has both
> absolute and relative timeouts...)
> 
> I think when we have a second user it might be worth seeing if some
> commonality could be extracted but TBH I doubt it would make the code
> smaller or simpler.

Right, lets leave it then.

> > > +static void devstate_timeout(libxl__egc *egc, libxl__ev_time *ev,
> > > +                             const struct timeval *requested_abs)
> > > +{
> > > +    EGC_GC;
> > > +    libxl__ev_devstate *ds = CONTAINER_OF(ev, *ds, timeout);
> > > +    LIBXL__LOG(CTX, LIBXL__LOG_DEBUG, "backend %s wanted state %d "
> > > +               " timed out", ds->watch.path, ds->wanted);
> > > +    libxl__ev_devstate_cancel(gc, ds);
> > 
> > What prevents racing here with the watch happening? Might the caller see
> > two callbacks?
> 
>   static inline void libxl__ev_devstate_cancel(libxl__gc *gc,
>                                                libxl__ev_devstate *ds)
>   {
>       libxl__ev_time_deregister(gc,&ds->timeout);
>       libxl__ev_xswatch_deregister(gc,&ds->watch);
>   }
> 
> So, no.  When the timeout happens, the ev xswatch is deregistered and
> can thereafter no longer generate callbacks.  If there are any
> xenstore watch events in the pipeline for deregistered ev_xswatch's,
> they're discarded by watchfd_callback.

What happens if the watch occurs and is delivered (in a different
thread) e.g. just before devstate_timeout calls
libxl__ev_devstate_cancel? The watch callback will be delivered but we
will unconditionally go on to deliver the cancel callback as well.




_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel