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 [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 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 ian.jack...@eu.citrix.com Release-Acked-by: Konrad Rzeszutek Wilk konrad.w...@oracle.com Acked-by: Ian Campbell ian.campb...@citrix.com ___ 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 ian.jack...@eu.citrix.com
Release-Acked-by: Konrad Rzeszutek Wilk konrad.w...@oracle.com
---
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 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 ian.jack...@eu.citrix.com
---
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
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 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
[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 ian.jack...@eu.citrix.com
---
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
