[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 5/6] libxl: events: Deregister evtchn fd when not needed [and 1 more messages]
Ian Campbell writes ("Re: [PATCH 5/6] libxl: events: Deregister evtchn fd when not needed"): > There were other comments further down my original review which you > haven't answered. I don't think they were (all) predicated on a > particular answer to the first question (although some were). Sorry, I didn't see those buried in down the patch ... Ian Campbell writes ("Re: [PATCH 5/6] libxl: events: Deregister evtchn fd when not needed"): > On Thu, 2014-11-27 at 18:27 +0000, Ian Jackson wrote: > > @@ -733,14 +733,10 @@ int libxl__ctx_evtchn_init(libxl__gc *gc) { > > goto out; > > } > > > > - fd = xc_evtchn_fd(xce); > > - assert(fd >= 0); > > + CTX->evtchn_fd = xc_evtchn_fd(xce); > > + assert(CTX->evtchn_fd >= 0); > > Given that you can always retrieve this no demand with xc_evtchn_fd(xce) > and that it is cheap do you need to stash it in the ctx? Good point. > > @@ -758,6 +760,13 @@ int libxl__ev_evtchn_wait(libxl__gc *gc, > > libxl__ev_evtchn *evev) > > DBG("ev_evtchn=%p port=%d wait (was waiting=%d)", > > evev, evev->port, evev->waiting); > > > > + rc = libxl__ctx_evtchn_init(gc); > > + if (rc) goto out; > > + > > + rc = libxl__ev_fd_register(gc, &CTX->evtchn_efd, > > + evtchn_fd_callback, CTX->evtchn_fd, POLLIN); > > + if (rc) goto out; > > Do you not need to do this only if evtchns_waiting is currently empty or > the efd is idle? In fact, I should check libxl__ev_fd_isregistered. That makes the fragment idempotent. I'm surprised this worked for you as it was... > > if (evev->waiting) > > return 0; > > If you hit this you leave the stuff above done. Which may or may not > matter depending on the answer above. Given the change above, I don't think it matters, because if evev->waiting, all of the other stuff is definitely already set up anyway. It is clearest to put the new initialisation fragment next to the existing one. I will resend with the two changes above. The diff between the previous version of this patch and the forthcoming new one is below. Thanks for the careful review. Ian. diff --git a/tools/libxl/libxl_event.c b/tools/libxl/libxl_event.c index 716f318..a36e6d9 100644 --- a/tools/libxl/libxl_event.c +++ b/tools/libxl/libxl_event.c @@ -721,7 +721,7 @@ static void evtchn_fd_callback(libxl__egc *egc, libxl__ev_fd *ev, int libxl__ctx_evtchn_init(libxl__gc *gc) { xc_evtchn *xce; - int rc; + int rc, fd; if (CTX->xce) return 0; @@ -733,10 +733,10 @@ int libxl__ctx_evtchn_init(libxl__gc *gc) { goto out; } - CTX->evtchn_fd = xc_evtchn_fd(xce); - assert(CTX->evtchn_fd >= 0); + fd = xc_evtchn_fd(xce); + assert(fd >= 0); - rc = libxl_fd_set_nonblock(CTX, CTX->evtchn_fd, 1); + rc = libxl_fd_set_nonblock(CTX, fd, 1); if (rc) goto out; CTX->xce = xce; @@ -763,9 +763,11 @@ int libxl__ev_evtchn_wait(libxl__gc *gc, libxl__ev_evtchn *evev) rc = libxl__ctx_evtchn_init(gc); if (rc) goto out; - rc = libxl__ev_fd_register(gc, &CTX->evtchn_efd, - evtchn_fd_callback, CTX->evtchn_fd, POLLIN); - if (rc) goto out; + if (!libxl__ev_fd_isregistered(&CTX->evtchn_efd)) { + rc = libxl__ev_fd_register(gc, &CTX->evtchn_efd, evtchn_fd_callback, + xc_evtchn_fd(CTX->xce), POLLIN); + if (rc) goto out; + } if (evev->waiting) return 0; diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h index 2eeba1e..9695f18 100644 --- a/tools/libxl/libxl_internal.h +++ b/tools/libxl/libxl_internal.h @@ -359,7 +359,6 @@ struct libxl__ctx { xc_evtchn *xce; /* waiting must be done only with libxl__ev_evtchn* */ LIBXL_LIST_HEAD(, libxl__ev_evtchn) evtchns_waiting; - int evtchn_fd; libxl__ev_fd evtchn_efd; LIBXL_TAILQ_HEAD(libxl__evgen_domain_death_list, libxl_evgen_domain_death) _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |