[Xen-devel] [PATCH 5/6] libxl: events: Deregister evtchn fd when not needed
We want to have no fd events registered when we are idle. In this patch, deal with the evtchn fd: * Defer setup of the evtchn handle to the first use. * Defer registration of the evtchn fd; register as needed on use. * When cancelling an evtchn wait, or when wait setup fails, check whether there are now no evtchn waits and if so deregister the fd. * On libxl teardown, the evtchn fd should therefore be unregistered. assert that this is the case. Signed-off-by: Ian Jackson Release-Acked-by: Konrad Rzeszutek Wilk --- v2: Do not bother putting evtchn_fd in the ctx; instead, get it from xc_evtchn_fd when we need it. (Cosmetic.) Do not register the evtchn fd multiple times: check it's not registered before we call libxl__ev_fd_register. (Bugfix.) --- tools/libxl/libxl.c |4 +--- tools/libxl/libxl_event.c | 21 + 2 files changed, 18 insertions(+), 7 deletions(-) diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c index 8f06043..50a8928 100644 --- a/tools/libxl/libxl.c +++ b/tools/libxl/libxl.c @@ -118,8 +118,6 @@ int libxl_ctx_alloc(libxl_ctx **pctx, int version, rc = ERROR_FAIL; goto out; } -rc = libxl__ctx_evtchn_init(gc); - *pctx = ctx; return 0; @@ -166,7 +164,7 @@ int libxl_ctx_free(libxl_ctx *ctx) for (i = 0; i < ctx->watch_nslots; i++) assert(!libxl__watch_slot_contents(gc, i)); assert(!libxl__ev_fd_isregistered(&ctx->watch_efd)); -libxl__ev_fd_deregister(gc, &ctx->evtchn_efd); +assert(!libxl__ev_fd_isregistered(&ctx->evtchn_efd)); assert(!libxl__ev_fd_isregistered(&ctx->sigchld_selfpipe_efd)); /* Now there should be no more events requested from the application: */ diff --git a/tools/libxl/libxl_event.c b/tools/libxl/libxl_event.c index da0a20e..a36e6d9 100644 --- a/tools/libxl/libxl_event.c +++ b/tools/libxl/libxl_event.c @@ -739,10 +739,6 @@ int libxl__ctx_evtchn_init(libxl__gc *gc) { rc = libxl_fd_set_nonblock(CTX, fd, 1); if (rc) goto out; -rc = libxl__ev_fd_register(gc, &CTX->evtchn_efd, - evtchn_fd_callback, fd, POLLIN); -if (rc) goto out; - CTX->xce = xce; return 0; @@ -751,6 +747,12 @@ int libxl__ctx_evtchn_init(libxl__gc *gc) { return rc; } +static void evtchn_check_fd_deregister(libxl__gc *gc) +{ +if (CTX->xce && LIBXL_LIST_EMPTY(&CTX->evtchns_waiting)) +libxl__ev_fd_deregister(gc, &CTX->evtchn_efd); +} + int libxl__ev_evtchn_wait(libxl__gc *gc, libxl__ev_evtchn *evev) { int r, rc; @@ -758,6 +760,15 @@ 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; + +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; @@ -773,6 +784,7 @@ int libxl__ev_evtchn_wait(libxl__gc *gc, libxl__ev_evtchn *evev) return 0; out: +evtchn_check_fd_deregister(gc); return rc; } @@ -786,6 +798,7 @@ void libxl__ev_evtchn_cancel(libxl__gc *gc, libxl__ev_evtchn *evev) evev->waiting = 0; LIBXL_LIST_REMOVE(evev, entry); +evtchn_check_fd_deregister(gc); } /* -- 1.7.10.4 ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 5/6] libxl: events: Deregister evtchn fd when not needed
On Tue, 2014-12-09 at 15:54 +, Ian Jackson wrote: > We want to have no fd events registered when we are idle. > In this patch, deal with the evtchn fd: > > * Defer setup of the evtchn handle to the first use. > * Defer registration of the evtchn fd; register as needed on use. > * When cancelling an evtchn wait, or when wait setup fails, check >whether there are now no evtchn waits and if so deregister the fd. > * On libxl teardown, the evtchn fd should therefore be unregistered. >assert that this is the case. > > Signed-off-by: Ian Jackson > Release-Acked-by: Konrad Rzeszutek Wilk Acked-by: Ian Campbell ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
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 +, 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@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 5/6] libxl: events: Deregister evtchn fd when not needed
On Tue, 2014-12-09 at 11:22 +, Ian Jackson wrote: > Ian Campbell writes ("Re: [PATCH 5/6] libxl: events: Deregister evtchn fd > when not needed"): > > On Fri, 2014-11-28 at 14:47 +, Ian Jackson wrote: > > > libxl__ev_evtchn_* is always called with the ctx lock held. > > > > For the most part this is implicit due to the caller being in an ao > > callback, right? > > Yes. > > > > However, that they don't take the lock is contrary to the rules stated > > > for libxl__ev_* in the doc comment. That should be fixed for 4.6. > > > > OK. > > Should I take this as an ack ? 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). Ian. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 5/6] libxl: events: Deregister evtchn fd when not needed
Ian Campbell writes ("Re: [PATCH 5/6] libxl: events: Deregister evtchn fd when not needed"): > On Fri, 2014-11-28 at 14:47 +, Ian Jackson wrote: > > libxl__ev_evtchn_* is always called with the ctx lock held. > > For the most part this is implicit due to the caller being in an ao > callback, right? Yes. > > However, that they don't take the lock is contrary to the rules stated > > for libxl__ev_* in the doc comment. That should be fixed for 4.6. > > OK. Should I take this as an ack ? Thanks, Ian. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 5/6] libxl: events: Deregister evtchn fd when not needed
On Fri, 2014-11-28 at 14:47 +, Ian Jackson wrote: > Ian Campbell writes ("Re: [PATCH 5/6] libxl: events: Deregister evtchn fd > when not needed"): > > On Thu, 2014-11-27 at 18:27 +, Ian Jackson wrote: > > > We want to have no fd events registered when we are idle. > > > In this patch, deal with the evtchn fd: > > > > > > * Defer setup of the evtchn handle to the first use. > > > * Defer registration of the evtchn fd; register as needed on use. > > > * When cancelling an evtchn wait, or when wait setup fails, check > > >whether there are now no evtchn waits and if so deregister the fd. > > > * On libxl teardown, the evtchn fd should therefore be unregistered. > > >assert that this is the case. > > > > Is there no locking required when registering/deregistering? Since there > > are list manipulations in most of these places I presume it already > > exists, but I thought I should check. > > libxl__ev_evtchn_* is always called with the ctx lock held. For the most part this is implicit due to the caller being in an ao callback, right? > However, that they don't take the lock is contrary to the rules stated > for libxl__ev_* in the doc comment. That should be fixed for 4.6. OK. > libxl__ev_fd_* already take and release the lock to protect their own > data structures etc. > > Ian. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 5/6] libxl: events: Deregister evtchn fd when not needed
Ian Campbell writes ("Re: [PATCH 5/6] libxl: events: Deregister evtchn fd when not needed"): > On Thu, 2014-11-27 at 18:27 +, Ian Jackson wrote: > > We want to have no fd events registered when we are idle. > > In this patch, deal with the evtchn fd: > > > > * Defer setup of the evtchn handle to the first use. > > * Defer registration of the evtchn fd; register as needed on use. > > * When cancelling an evtchn wait, or when wait setup fails, check > >whether there are now no evtchn waits and if so deregister the fd. > > * On libxl teardown, the evtchn fd should therefore be unregistered. > >assert that this is the case. > > Is there no locking required when registering/deregistering? Since there > are list manipulations in most of these places I presume it already > exists, but I thought I should check. libxl__ev_evtchn_* is always called with the ctx lock held. However, that they don't take the lock is contrary to the rules stated for libxl__ev_* in the doc comment. That should be fixed for 4.6. libxl__ev_fd_* already take and release the lock to protect their own data structures etc. Ian. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 5/6] libxl: events: Deregister evtchn fd when not needed
On Thu, 2014-11-27 at 18:27 +, Ian Jackson wrote: > We want to have no fd events registered when we are idle. > In this patch, deal with the evtchn fd: > > * Defer setup of the evtchn handle to the first use. > * Defer registration of the evtchn fd; register as needed on use. > * When cancelling an evtchn wait, or when wait setup fails, check >whether there are now no evtchn waits and if so deregister the fd. > * On libxl teardown, the evtchn fd should therefore be unregistered. >assert that this is the case. Is there no locking required when registering/deregistering? Since there are list manipulations in most of these places I presume it already exists, but I thought I should check. > > Signed-off-by: Ian Jackson > --- > tools/libxl/libxl.c |4 +--- > tools/libxl/libxl_event.c| 27 +++ > tools/libxl/libxl_internal.h |1 + > 3 files changed, 21 insertions(+), 11 deletions(-) > > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c > index 785253d..e0db4eb 100644 > --- a/tools/libxl/libxl.c > +++ b/tools/libxl/libxl.c > @@ -118,8 +118,6 @@ int libxl_ctx_alloc(libxl_ctx **pctx, int version, > rc = ERROR_FAIL; goto out; > } > > -rc = libxl__ctx_evtchn_init(gc); > - > *pctx = ctx; > return 0; > > @@ -166,7 +164,7 @@ int libxl_ctx_free(libxl_ctx *ctx) > for (i = 0; i < ctx->watch_nslots; i++) > assert(!libxl__watch_slot_contents(gc, i)); > assert(!libxl__ev_fd_isregistered(&ctx->watch_efd)); > -libxl__ev_fd_deregister(gc, &ctx->evtchn_efd); > +assert(!libxl__ev_fd_isregistered(&ctx->evtchn_efd)); > assert(!libxl__ev_fd_isregistered(&ctx->sigchld_selfpipe_efd)); > > /* Now there should be no more events requested from the application: */ > diff --git a/tools/libxl/libxl_event.c b/tools/libxl/libxl_event.c > index da0a20e..716f318 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, fd; > +int rc; > > if (CTX->xce) > return 0; > @@ -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? > -rc = libxl_fd_set_nonblock(CTX, fd, 1); > -if (rc) goto out; > - > -rc = libxl__ev_fd_register(gc, &CTX->evtchn_efd, > - evtchn_fd_callback, fd, POLLIN); > +rc = libxl_fd_set_nonblock(CTX, CTX->evtchn_fd, 1); > if (rc) goto out; > > CTX->xce = xce; > @@ -751,6 +747,12 @@ int libxl__ctx_evtchn_init(libxl__gc *gc) { > return rc; > } > > +static void evtchn_check_fd_deregister(libxl__gc *gc) > +{ > +if (CTX->xce && LIBXL_LIST_EMPTY(&CTX->evtchns_waiting)) > +libxl__ev_fd_deregister(gc, &CTX->evtchn_efd); > +} > + > int libxl__ev_evtchn_wait(libxl__gc *gc, libxl__ev_evtchn *evev) > { > int r, rc; > @@ -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? > + > 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. Ian. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
[Xen-devel] [PATCH 5/6] libxl: events: Deregister evtchn fd when not needed
We want to have no fd events registered when we are idle. In this patch, deal with the evtchn fd: * Defer setup of the evtchn handle to the first use. * Defer registration of the evtchn fd; register as needed on use. * When cancelling an evtchn wait, or when wait setup fails, check whether there are now no evtchn waits and if so deregister the fd. * On libxl teardown, the evtchn fd should therefore be unregistered. assert that this is the case. Signed-off-by: Ian Jackson --- tools/libxl/libxl.c |4 +--- tools/libxl/libxl_event.c| 27 +++ tools/libxl/libxl_internal.h |1 + 3 files changed, 21 insertions(+), 11 deletions(-) diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c index 785253d..e0db4eb 100644 --- a/tools/libxl/libxl.c +++ b/tools/libxl/libxl.c @@ -118,8 +118,6 @@ int libxl_ctx_alloc(libxl_ctx **pctx, int version, rc = ERROR_FAIL; goto out; } -rc = libxl__ctx_evtchn_init(gc); - *pctx = ctx; return 0; @@ -166,7 +164,7 @@ int libxl_ctx_free(libxl_ctx *ctx) for (i = 0; i < ctx->watch_nslots; i++) assert(!libxl__watch_slot_contents(gc, i)); assert(!libxl__ev_fd_isregistered(&ctx->watch_efd)); -libxl__ev_fd_deregister(gc, &ctx->evtchn_efd); +assert(!libxl__ev_fd_isregistered(&ctx->evtchn_efd)); assert(!libxl__ev_fd_isregistered(&ctx->sigchld_selfpipe_efd)); /* Now there should be no more events requested from the application: */ diff --git a/tools/libxl/libxl_event.c b/tools/libxl/libxl_event.c index da0a20e..716f318 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, fd; +int rc; if (CTX->xce) return 0; @@ -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); -rc = libxl_fd_set_nonblock(CTX, fd, 1); -if (rc) goto out; - -rc = libxl__ev_fd_register(gc, &CTX->evtchn_efd, - evtchn_fd_callback, fd, POLLIN); +rc = libxl_fd_set_nonblock(CTX, CTX->evtchn_fd, 1); if (rc) goto out; CTX->xce = xce; @@ -751,6 +747,12 @@ int libxl__ctx_evtchn_init(libxl__gc *gc) { return rc; } +static void evtchn_check_fd_deregister(libxl__gc *gc) +{ +if (CTX->xce && LIBXL_LIST_EMPTY(&CTX->evtchns_waiting)) +libxl__ev_fd_deregister(gc, &CTX->evtchn_efd); +} + int libxl__ev_evtchn_wait(libxl__gc *gc, libxl__ev_evtchn *evev) { int r, rc; @@ -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; + if (evev->waiting) return 0; @@ -773,6 +782,7 @@ int libxl__ev_evtchn_wait(libxl__gc *gc, libxl__ev_evtchn *evev) return 0; out: +evtchn_check_fd_deregister(gc); return rc; } @@ -786,6 +796,7 @@ void libxl__ev_evtchn_cancel(libxl__gc *gc, libxl__ev_evtchn *evev) evev->waiting = 0; LIBXL_LIST_REMOVE(evev, entry); +evtchn_check_fd_deregister(gc); } /* diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h index 728fe2c..481fbd5 100644 --- a/tools/libxl/libxl_internal.h +++ b/tools/libxl/libxl_internal.h @@ -359,6 +359,7 @@ 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) -- 1.7.10.4 ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel