Re: [Xen-devel] [PATCH 2/6] libxl: events: Deregister xenstore watch fd when not needed
Ian Campbell writes ("Re: [PATCH 2/6] libxl: events: Deregister xenstore watch
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 xenstore watch fd:
...
> > Signed-off-by: Ian Jackson
>
> Acked-by: Ian Campbell
Thanks. In fact you had acked this already but somehow I have dropped
that.
Ian.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 2/6] libxl: events: Deregister xenstore watch 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 xenstore watch fd: > > * Track the total number of active watches. > * When deregistering a watch, or when watch registration fails, check > whether there are now no watches and if so deregister the fd. > * On libxl teardown, the watch fd should therefore be unregistered. > assert that this is the case. > > Signed-off-by: Ian Jackson Acked-by: Ian Campbell ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
[Xen-devel] [PATCH 2/6] libxl: events: Deregister xenstore watch fd when not needed
We want to have no fd events registered when we are idle.
In this patch, deal with the xenstore watch fd:
* Track the total number of active watches.
* When deregistering a watch, or when watch registration fails, check
whether there are now no watches and if so deregister the fd.
* On libxl teardown, the watch fd should therefore be unregistered.
assert that this is the case.
Signed-off-by: Ian Jackson
---
tools/libxl/libxl.c |2 +-
tools/libxl/libxl_event.c| 11 +++
tools/libxl/libxl_internal.h |2 +-
3 files changed, 13 insertions(+), 2 deletions(-)
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 55ef535..a238621 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -164,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));
-libxl__ev_fd_deregister(gc, &ctx->watch_efd);
+assert(!libxl__ev_fd_isregistered(&ctx->watch_efd));
libxl__ev_fd_deregister(gc, &ctx->evtchn_efd);
libxl__ev_fd_deregister(gc, &ctx->sigchld_selfpipe_efd);
diff --git a/tools/libxl/libxl_event.c b/tools/libxl/libxl_event.c
index 22b1227..da0a20e 100644
--- a/tools/libxl/libxl_event.c
+++ b/tools/libxl/libxl_event.c
@@ -524,6 +524,13 @@ static char *watch_token(libxl__gc *gc, int slotnum,
uint32_t counterval)
return libxl__sprintf(gc, "%d/%"PRIx32, slotnum, counterval);
}
+static void watches_check_fd_deregister(libxl__gc *gc)
+{
+assert(CTX->nwatches>=0);
+if (!CTX->nwatches)
+libxl__ev_fd_deregister(gc, &CTX->watch_efd);
+}
+
int libxl__ev_xswatch_register(libxl__gc *gc, libxl__ev_xswatch *w,
libxl__ev_xswatch_callback *func,
const char *path /* copied */)
@@ -579,6 +586,7 @@ int libxl__ev_xswatch_register(libxl__gc *gc,
libxl__ev_xswatch *w,
w->slotnum = slotnum;
w->path = path_copy;
w->callback = func;
+CTX->nwatches++;
libxl__set_watch_slot_contents(use, w);
CTX_UNLOCK;
@@ -590,6 +598,7 @@ int libxl__ev_xswatch_register(libxl__gc *gc,
libxl__ev_xswatch *w,
if (use)
LIBXL_SLIST_INSERT_HEAD(&CTX->watch_freeslots, use, empty);
free(path_copy);
+watches_check_fd_deregister(gc);
CTX_UNLOCK;
return rc;
}
@@ -614,6 +623,8 @@ void libxl__ev_xswatch_deregister(libxl__gc *gc,
libxl__ev_xswatch *w)
libxl__ev_watch_slot *slot = &CTX->watch_slots[w->slotnum];
LIBXL_SLIST_INSERT_HEAD(&CTX->watch_freeslots, slot, empty);
w->slotnum = -1;
+CTX->nwatches--;
+watches_check_fd_deregister(gc);
} else {
LOG(DEBUG, "watch w=%p: deregister unregistered", w);
}
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index a38f695..9695f18 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -352,7 +352,7 @@ struct libxl__ctx {
LIBXL_TAILQ_HEAD(, libxl__ev_time) etimes;
libxl__ev_watch_slot *watch_slots;
-int watch_nslots;
+int watch_nslots, nwatches;
LIBXL_SLIST_HEAD(, libxl__ev_watch_slot) watch_freeslots;
uint32_t watch_counter; /* helps disambiguate slot reuse */
libxl__ev_fd watch_efd;
--
1.7.10.4
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 2/6] libxl: events: Deregister xenstore watch fd when not needed
On Fri, 2014-11-28 at 14:56 +, Ian Jackson wrote:
> Ian Campbell writes ("Re: [PATCH 2/6] libxl: events: Deregister xenstore
> watch fd when not needed"):
> > On Thu, 2014-11-27 at 18:27 +, Ian Jackson wrote:
> > > * On libxl teardown, the watch fd should therefore be unregistered.
> > > assert that this is the case.
> >
> > A bunch of the patches in this series basically assume that the ctx is
> > idle when it is freed, i.e. it requires everything to be explicitly
> > cancelled rather than implicitly doing so on free.
>
> libxl_ctx_free explicitly disables all the application-requested event
> generators. (free_disable_deaths and libxl__evdisable_disk_eject.)
So it does, I was looking for that before commenting but didn't see
those calls for what they were, despite the comment right there...
> Destroying the ctx during the execution of an asynchronous operation
> is forbidden by this text in libxl.h (near line 813):
> * *ao_how does not need to remain valid after the initiating function
> * returns. All other parameters must remain valid for the lifetime of
> * the asynchronous operation, unless otherwise specified.
> That implies that the ctx must remain valid during the ao, so it may
> not be destroyed beforehand.
>
> Those are the two ways that, even without any threads inside libxl, a
> ctx can be other than idle.
>
> It should be obvious to the application programmer that destroying the
> ctx when there are other threads inside libxl is not going to work.
> Indeed a programmer who tries to destroy the ctx when they have
> threads which might be inside libxl cannot ensure that the ctx is
> valid even on entry to libxl.
>
> > I think this is a fine restriction, but it probably ought to be written
> > down.
>
> Does the above demonstrate that the existing restrictions are
> documented ?
Yes.
> I'd rather avoid writing the restrictions twice if it
> can be avoided - these docs are long enough as they are.
Indeed.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 2/6] libxl: events: Deregister xenstore watch fd when not needed
Ian Campbell writes ("Re: [PATCH 2/6] libxl: events: Deregister xenstore watch
fd when not needed"):
> On Thu, 2014-11-27 at 18:27 +, Ian Jackson wrote:
> > * On libxl teardown, the watch fd should therefore be unregistered.
> > assert that this is the case.
>
> A bunch of the patches in this series basically assume that the ctx is
> idle when it is freed, i.e. it requires everything to be explicitly
> cancelled rather than implicitly doing so on free.
libxl_ctx_free explicitly disables all the application-requested event
generators. (free_disable_deaths and libxl__evdisable_disk_eject.)
Destroying the ctx during the execution of an asynchronous operation
is forbidden by this text in libxl.h (near line 813):
* *ao_how does not need to remain valid after the initiating function
* returns. All other parameters must remain valid for the lifetime of
* the asynchronous operation, unless otherwise specified.
That implies that the ctx must remain valid during the ao, so it may
not be destroyed beforehand.
Those are the two ways that, even without any threads inside libxl, a
ctx can be other than idle.
It should be obvious to the application programmer that destroying the
ctx when there are other threads inside libxl is not going to work.
Indeed a programmer who tries to destroy the ctx when they have
threads which might be inside libxl cannot ensure that the ctx is
valid even on entry to libxl.
> I think this is a fine restriction, but it probably ought to be written
> down.
Does the above demonstrate that the existing restrictions are
documented ? I'd rather avoid writing the restrictions twice if it
can be avoided - these docs are long enough as they are.
Ian.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 2/6] libxl: events: Deregister xenstore watch 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 xenstore watch fd: > > * Track the total number of active watches. > * When deregistering a watch, or when watch registration fails, check > whether there are now no watches and if so deregister the fd. > * On libxl teardown, the watch fd should therefore be unregistered. > assert that this is the case. > > Signed-off-by: Ian Jackson Acked-by: Ian Campbell ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 2/6] libxl: events: Deregister xenstore watch fd when not needed
On Thu, 2014-11-27 at 18:27 +, Ian Jackson wrote: > * On libxl teardown, the watch fd should therefore be unregistered. > assert that this is the case. A bunch of the patches in this series basically assume that the ctx is idle when it is freed, i.e. it requires everything to be explicitly cancelled rather than implicitly doing so on free. I think this is a fine restriction, but it probably ought to be written down. Ian. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
[Xen-devel] [PATCH 2/6] libxl: events: Deregister xenstore watch fd when not needed
We want to have no fd events registered when we are idle.
In this patch, deal with the xenstore watch fd:
* Track the total number of active watches.
* When deregistering a watch, or when watch registration fails, check
whether there are now no watches and if so deregister the fd.
* On libxl teardown, the watch fd should therefore be unregistered.
assert that this is the case.
Signed-off-by: Ian Jackson
---
tools/libxl/libxl.c |2 +-
tools/libxl/libxl_event.c| 11 +++
tools/libxl/libxl_internal.h |2 +-
3 files changed, 13 insertions(+), 2 deletions(-)
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index c111f27..12a1013 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -164,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));
-libxl__ev_fd_deregister(gc, &ctx->watch_efd);
+assert(!libxl__ev_fd_isregistered(&ctx->watch_efd));
libxl__ev_fd_deregister(gc, &ctx->evtchn_efd);
libxl__ev_fd_deregister(gc, &ctx->sigchld_selfpipe_efd);
diff --git a/tools/libxl/libxl_event.c b/tools/libxl/libxl_event.c
index 22b1227..da0a20e 100644
--- a/tools/libxl/libxl_event.c
+++ b/tools/libxl/libxl_event.c
@@ -524,6 +524,13 @@ static char *watch_token(libxl__gc *gc, int slotnum,
uint32_t counterval)
return libxl__sprintf(gc, "%d/%"PRIx32, slotnum, counterval);
}
+static void watches_check_fd_deregister(libxl__gc *gc)
+{
+assert(CTX->nwatches>=0);
+if (!CTX->nwatches)
+libxl__ev_fd_deregister(gc, &CTX->watch_efd);
+}
+
int libxl__ev_xswatch_register(libxl__gc *gc, libxl__ev_xswatch *w,
libxl__ev_xswatch_callback *func,
const char *path /* copied */)
@@ -579,6 +586,7 @@ int libxl__ev_xswatch_register(libxl__gc *gc,
libxl__ev_xswatch *w,
w->slotnum = slotnum;
w->path = path_copy;
w->callback = func;
+CTX->nwatches++;
libxl__set_watch_slot_contents(use, w);
CTX_UNLOCK;
@@ -590,6 +598,7 @@ int libxl__ev_xswatch_register(libxl__gc *gc,
libxl__ev_xswatch *w,
if (use)
LIBXL_SLIST_INSERT_HEAD(&CTX->watch_freeslots, use, empty);
free(path_copy);
+watches_check_fd_deregister(gc);
CTX_UNLOCK;
return rc;
}
@@ -614,6 +623,8 @@ void libxl__ev_xswatch_deregister(libxl__gc *gc,
libxl__ev_xswatch *w)
libxl__ev_watch_slot *slot = &CTX->watch_slots[w->slotnum];
LIBXL_SLIST_INSERT_HEAD(&CTX->watch_freeslots, slot, empty);
w->slotnum = -1;
+CTX->nwatches--;
+watches_check_fd_deregister(gc);
} else {
LOG(DEBUG, "watch w=%p: deregister unregistered", w);
}
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 4361421..728fe2c 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -352,7 +352,7 @@ struct libxl__ctx {
LIBXL_TAILQ_HEAD(, libxl__ev_time) etimes;
libxl__ev_watch_slot *watch_slots;
-int watch_nslots;
+int watch_nslots, nwatches;
LIBXL_SLIST_HEAD(, libxl__ev_watch_slot) watch_freeslots;
uint32_t watch_counter; /* helps disambiguate slot reuse */
libxl__ev_fd watch_efd;
--
1.7.10.4
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
