From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Jackson Subject: Re: [PATCH 10/23] libxl: events: Provide libxl__ev_evtchn* Date: Thu, 19 Dec 2013 17:47:40 +0000 Message-ID: <21171.12604.288109.290100@mariner.uk.xensource.com> References: <1387305337-15355-1-git-send-email-ian.jackson@eu.citrix.com> <1387305337-15355-11-git-send-email-ian.jackson@eu.citrix.com> <1387460612.9925.75.camel@kazak.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1387460612.9925.75.camel@kazak.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Campbell Cc: Shriram Rajagopalan , George Dunlap , xen-devel@lists.xensource.com, Stefano Stabellini List-Id: xen-devel@lists.xenproject.org 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.