Re: [Xen-devel] [PATCH 10/23] libxl: events: Provide libxl__ev_evtchn*

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Ian Campbell writes ("Re: [PATCH 10/23] libxl: events: Provide 
libxl__ev_evtchn*"):
> On Tue, 2013-12-17 at 18:35 +0000, Ian Jackson wrote:
> > This also involves providing a gc for the latter part of
> > libxl_ctx_alloc.
> 
> Didn't you do that bit in an earlier patch? I guess you forgot to update
> this message when you refactored.

Yes.

> > +static void evtchn_fd_callback(libxl__egc *egc, libxl__ev_fd *ev,
> > +                               int fd, short events, short revents)
> > +{
> > +    EGC_GC;
> > +    libxl__ev_evtchn *evev;
> > +    int port, r, rc;
> 
> Should port be evtchn_port_or_error_t ? (from the use I don't think
> plain evtchn_port_t would be valid)

Fixed.

> > [...]
> > +struct libxl__ev_evtchn {
> > +    /* caller must fill these in, and they must all remain valid */
> > +    libxl__ev_evtchn_callback *callback;
> > +    int port;
> 
> evtchn_port_t?

Leaving it as "int" means that the caller can put -1 in it if the
struct isn't in use or the port not allocated.  That avoids the caller
needing to have a separate copy of the value, or a separate boolean.
And later, we do.

> > @@ -343,6 +354,10 @@ struct libxl__ctx {
> >      uint32_t watch_counter; /* helps disambiguate slot reuse */
> >      libxl__ev_fd watch_efd;
> >  
> > +    xc_evtchn *xce; /* for waiting use only libxl__ev_evtchn* */
> 
> The comment means "don't use directly, use libxl__ev-evtchn"?
>
> Or does it imply that for uses other than waiting you may use it
> directly?

The latter.  I'm open to suggestions for improved wording.

> > +    LIBXL_LIST_HEAD(, libxl__ev_evtchn) evtchns_waiting;
> 
> Are there any locking requirements relating to this list?

They are the same as all the other event-related data structures in
the ctx: the ctx mutex protects them.  This isn't particularly
explicit here but I think it's fairly obvious.  Only the event code
looks at this list anyway and it all takes a gc.

> >  /*
> > + * The evtchn facility is one-shot per call t libxl__ev_evtchn_wait.
> 
> s/ t / to /

Fixed.

> > + * You must not wait on the same port twice at once (that is, with
> > + * two separate libxl__ev_evtchn's).
> 
> Doing so would require per wait xce_handles? Worth avoiding.

It would be possible to improve the data structure and event code to
support this.  The obvious way would be to keep scanning the
evtchns_waiting list after we found one waiter, but you'd have to do
something fiddly to avoid reentering the same waiter again right away.

Thanks,
Ian.

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