* [PATCH 1/6] libxl: events: Assert that libxl_ctx_free is not called from a hook
2014-11-27 18:27 ` [PATCH for-4.5 0/6] libxl: events: Tear down fd interests when idle Ian Jackson
@ 2014-11-27 18:27 ` Ian Jackson
2014-11-28 12:42 ` Ian Campbell
2014-11-27 18:27 ` [PATCH 2/6] libxl: events: Deregister xenstore watch fd when not needed Ian Jackson
` (6 subsequent siblings)
7 siblings, 1 reply; 43+ messages in thread
From: Ian Jackson @ 2014-11-27 18:27 UTC (permalink / raw)
To: xen-devel; +Cc: Jim Fehlig, Ian Jackson, Ian Campbell
No-one in their right mind would do this, and if they did everything
would definitely collapse. Arrange that if this happens, we crash
ASAP.
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
tools/libxl/libxl.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index de23fec..c111f27 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -148,6 +148,8 @@ int libxl_ctx_free(libxl_ctx *ctx)
{
if (!ctx) return 0;
+ assert(!ctx->osevent_in_hook);
+
int i;
GC_INIT(ctx);
--
1.7.10.4
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH 1/6] libxl: events: Assert that libxl_ctx_free is not called from a hook
2014-11-27 18:27 ` [PATCH 1/6] libxl: events: Assert that libxl_ctx_free is not called from a hook Ian Jackson
@ 2014-11-28 12:42 ` Ian Campbell
0 siblings, 0 replies; 43+ messages in thread
From: Ian Campbell @ 2014-11-28 12:42 UTC (permalink / raw)
To: Ian Jackson; +Cc: Jim Fehlig, xen-devel
On Thu, 2014-11-27 at 18:27 +0000, Ian Jackson wrote:
> No-one in their right mind would do this, and if they did everything
> would definitely collapse. Arrange that if this happens, we crash
> ASAP.
>
> Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
> ---
> tools/libxl/libxl.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index de23fec..c111f27 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -148,6 +148,8 @@ int libxl_ctx_free(libxl_ctx *ctx)
> {
> if (!ctx) return 0;
>
> + assert(!ctx->osevent_in_hook);
> +
> int i;
> GC_INIT(ctx);
>
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH 2/6] libxl: events: Deregister xenstore watch fd when not needed
2014-11-27 18:27 ` [PATCH for-4.5 0/6] libxl: events: Tear down fd interests when idle Ian Jackson
2014-11-27 18:27 ` [PATCH 1/6] libxl: events: Assert that libxl_ctx_free is not called from a hook Ian Jackson
@ 2014-11-27 18:27 ` Ian Jackson
2014-11-28 13:06 ` Ian Campbell
2014-11-28 13:19 ` Ian Campbell
2014-11-27 18:27 ` [PATCH 3/6] libxl: events: Deregister, don't just modify, sigchld pipe fd Ian Jackson
` (5 subsequent siblings)
7 siblings, 2 replies; 43+ messages in thread
From: Ian Jackson @ 2014-11-27 18:27 UTC (permalink / raw)
To: xen-devel; +Cc: Jim Fehlig, Ian Jackson, Ian Campbell
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 <ian.jackson@eu.citrix.com>
---
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
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH 2/6] libxl: events: Deregister xenstore watch fd when not needed
2014-11-27 18:27 ` [PATCH 2/6] libxl: events: Deregister xenstore watch fd when not needed Ian Jackson
@ 2014-11-28 13:06 ` Ian Campbell
2014-11-28 14:56 ` Ian Jackson
2014-11-28 13:19 ` Ian Campbell
1 sibling, 1 reply; 43+ messages in thread
From: Ian Campbell @ 2014-11-28 13:06 UTC (permalink / raw)
To: Ian Jackson; +Cc: Jim Fehlig, xen-devel
On Thu, 2014-11-27 at 18:27 +0000, 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.
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 2/6] libxl: events: Deregister xenstore watch fd when not needed
2014-11-28 13:06 ` Ian Campbell
@ 2014-11-28 14:56 ` Ian Jackson
2014-11-28 15:00 ` Ian Campbell
0 siblings, 1 reply; 43+ messages in thread
From: Ian Jackson @ 2014-11-28 14:56 UTC (permalink / raw)
To: Ian Campbell; +Cc: Jim Fehlig, xen-devel
Ian Campbell writes ("Re: [PATCH 2/6] libxl: events: Deregister xenstore watch fd when not needed"):
> On Thu, 2014-11-27 at 18:27 +0000, 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.
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH 2/6] libxl: events: Deregister xenstore watch fd when not needed
2014-11-28 14:56 ` Ian Jackson
@ 2014-11-28 15:00 ` Ian Campbell
0 siblings, 0 replies; 43+ messages in thread
From: Ian Campbell @ 2014-11-28 15:00 UTC (permalink / raw)
To: Ian Jackson; +Cc: Jim Fehlig, xen-devel
On Fri, 2014-11-28 at 14:56 +0000, 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 +0000, 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.
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 2/6] libxl: events: Deregister xenstore watch fd when not needed
2014-11-27 18:27 ` [PATCH 2/6] libxl: events: Deregister xenstore watch fd when not needed Ian Jackson
2014-11-28 13:06 ` Ian Campbell
@ 2014-11-28 13:19 ` Ian Campbell
1 sibling, 0 replies; 43+ messages in thread
From: Ian Campbell @ 2014-11-28 13:19 UTC (permalink / raw)
To: Ian Jackson; +Cc: Jim Fehlig, xen-devel
On Thu, 2014-11-27 at 18:27 +0000, 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 <ian.jackson@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH 3/6] libxl: events: Deregister, don't just modify, sigchld pipe fd
2014-11-27 18:27 ` [PATCH for-4.5 0/6] libxl: events: Tear down fd interests when idle Ian Jackson
2014-11-27 18:27 ` [PATCH 1/6] libxl: events: Assert that libxl_ctx_free is not called from a hook Ian Jackson
2014-11-27 18:27 ` [PATCH 2/6] libxl: events: Deregister xenstore watch fd when not needed Ian Jackson
@ 2014-11-27 18:27 ` Ian Jackson
2014-11-28 12:48 ` Ian Campbell
2014-11-27 18:27 ` [PATCH 4/6] libxl: events: Tear down SIGCHLD machinery on ctx destruction Ian Jackson
` (4 subsequent siblings)
7 siblings, 1 reply; 43+ messages in thread
From: Ian Jackson @ 2014-11-27 18:27 UTC (permalink / raw)
To: xen-devel; +Cc: Jim Fehlig, Ian Jackson, Ian Campbell
We want to have no fd events registered when we are idle. This
implies that we must be able to deregister our interest in the sigchld
self-pipe fd, not just modify to request no events.
Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
tools/libxl/libxl_fork.c | 9 +--------
1 file changed, 1 insertion(+), 8 deletions(-)
diff --git a/tools/libxl/libxl_fork.c b/tools/libxl/libxl_fork.c
index fa15095..144208a 100644
--- a/tools/libxl/libxl_fork.c
+++ b/tools/libxl/libxl_fork.c
@@ -372,15 +372,8 @@ static void sigchld_user_remove(libxl_ctx *ctx) /* idempotent */
void libxl__sigchld_notneeded(libxl__gc *gc) /* non-reentrant, idempotent */
{
- int rc;
-
sigchld_user_remove(CTX);
-
- if (libxl__ev_fd_isregistered(&CTX->sigchld_selfpipe_efd)) {
- rc = libxl__ev_fd_modify(gc, &CTX->sigchld_selfpipe_efd, 0);
- if (rc)
- libxl__ev_fd_deregister(gc, &CTX->sigchld_selfpipe_efd);
- }
+ libxl__ev_fd_deregister(gc, &CTX->sigchld_selfpipe_efd);
}
int libxl__sigchld_needed(libxl__gc *gc) /* non-reentrant, idempotent */
--
1.7.10.4
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH 3/6] libxl: events: Deregister, don't just modify, sigchld pipe fd
2014-11-27 18:27 ` [PATCH 3/6] libxl: events: Deregister, don't just modify, sigchld pipe fd Ian Jackson
@ 2014-11-28 12:48 ` Ian Campbell
2014-11-28 14:42 ` Ian Jackson
0 siblings, 1 reply; 43+ messages in thread
From: Ian Campbell @ 2014-11-28 12:48 UTC (permalink / raw)
To: Ian Jackson; +Cc: Jim Fehlig, xen-devel
On Thu, 2014-11-27 at 18:27 +0000, Ian Jackson wrote:
> We want to have no fd events registered when we are idle. This
> implies that we must be able to deregister our interest in the sigchld
> self-pipe fd, not just modify to request no events.
>
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
I think in principal there is now some redundant code in
libxl__sigchld_needed which calls modify to set POLLIN if the fd is not
registered. I think it's redundant because now the fd is registered iff
it is looking for POLLIN.
> ---
> tools/libxl/libxl_fork.c | 9 +--------
> 1 file changed, 1 insertion(+), 8 deletions(-)
>
> diff --git a/tools/libxl/libxl_fork.c b/tools/libxl/libxl_fork.c
> index fa15095..144208a 100644
> --- a/tools/libxl/libxl_fork.c
> +++ b/tools/libxl/libxl_fork.c
> @@ -372,15 +372,8 @@ static void sigchld_user_remove(libxl_ctx *ctx) /* idempotent */
>
> void libxl__sigchld_notneeded(libxl__gc *gc) /* non-reentrant, idempotent */
> {
> - int rc;
> -
> sigchld_user_remove(CTX);
> -
> - if (libxl__ev_fd_isregistered(&CTX->sigchld_selfpipe_efd)) {
> - rc = libxl__ev_fd_modify(gc, &CTX->sigchld_selfpipe_efd, 0);
> - if (rc)
> - libxl__ev_fd_deregister(gc, &CTX->sigchld_selfpipe_efd);
> - }
> + libxl__ev_fd_deregister(gc, &CTX->sigchld_selfpipe_efd);
> }
>
> int libxl__sigchld_needed(libxl__gc *gc) /* non-reentrant, idempotent */
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH 3/6] libxl: events: Deregister, don't just modify, sigchld pipe fd
2014-11-28 12:48 ` Ian Campbell
@ 2014-11-28 14:42 ` Ian Jackson
2014-11-28 14:44 ` Ian Campbell
0 siblings, 1 reply; 43+ messages in thread
From: Ian Jackson @ 2014-11-28 14:42 UTC (permalink / raw)
To: Ian Campbell; +Cc: Jim Fehlig, xen-devel
Ian Campbell writes ("Re: [PATCH 3/6] libxl: events: Deregister, don't just modify, sigchld pipe fd"):
> On Thu, 2014-11-27 at 18:27 +0000, Ian Jackson wrote:
> > We want to have no fd events registered when we are idle. This
> > implies that we must be able to deregister our interest in the sigchld
> > self-pipe fd, not just modify to request no events.
> >
> > Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
>
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
>
> I think in principal there is now some redundant code in
> libxl__sigchld_needed which calls modify to set POLLIN if the fd is not
> registered. I think it's redundant because now the fd is registered iff
> it is looking for POLLIN.
You're right. (Although you mean `calls modify if the fd /is/
registered'.)
I wonder if it would be better to leave tidying that up until
post-4.5, in case we are wrong.
Ian.
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH 3/6] libxl: events: Deregister, don't just modify, sigchld pipe fd
2014-11-28 14:42 ` Ian Jackson
@ 2014-11-28 14:44 ` Ian Campbell
0 siblings, 0 replies; 43+ messages in thread
From: Ian Campbell @ 2014-11-28 14:44 UTC (permalink / raw)
To: Ian Jackson; +Cc: Jim Fehlig, xen-devel
On Fri, 2014-11-28 at 14:42 +0000, Ian Jackson wrote:
> Ian Campbell writes ("Re: [PATCH 3/6] libxl: events: Deregister, don't just modify, sigchld pipe fd"):
> > On Thu, 2014-11-27 at 18:27 +0000, Ian Jackson wrote:
> > > We want to have no fd events registered when we are idle. This
> > > implies that we must be able to deregister our interest in the sigchld
> > > self-pipe fd, not just modify to request no events.
> > >
> > > Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> >
> > Acked-by: Ian Campbell <ian.campbell@citrix.com>
> >
> > I think in principal there is now some redundant code in
> > libxl__sigchld_needed which calls modify to set POLLIN if the fd is not
> > registered. I think it's redundant because now the fd is registered iff
> > it is looking for POLLIN.
>
> You're right. (Although you mean `calls modify if the fd /is/
> registered'.)
Indeed.
> I wonder if it would be better to leave tidying that up until
> post-4.5, in case we are wrong.
Sure.
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH 4/6] libxl: events: Tear down SIGCHLD machinery on ctx destruction
2014-11-27 18:27 ` [PATCH for-4.5 0/6] libxl: events: Tear down fd interests when idle Ian Jackson
` (2 preceding siblings ...)
2014-11-27 18:27 ` [PATCH 3/6] libxl: events: Deregister, don't just modify, sigchld pipe fd Ian Jackson
@ 2014-11-27 18:27 ` Ian Jackson
2014-11-28 12:51 ` Ian Campbell
2014-11-27 18:27 ` [PATCH 5/6] libxl: events: Deregister evtchn fd when not needed Ian Jackson
` (3 subsequent siblings)
7 siblings, 1 reply; 43+ messages in thread
From: Ian Jackson @ 2014-11-27 18:27 UTC (permalink / raw)
To: xen-devel; +Cc: Jim Fehlig, Ian Jackson, Ian Campbell
We want to have no fd events registered when we are idle.
Also, we should put back the default SIGCHLD handler. So:
* In libxl_ctx_free, use libxl_childproc_setmode to set the mode to
the default, which is libxl_sigchld_owner_libxl (ie `libxl owns
SIGCHLD only when it has active children').
But of course there are no active children at libxl teardown so
this results in libxl__sigchld_notneeded: the ctx loses its
interest in SIGCHLD (unsetting the SIGCHLD handler if we were the
last ctx) and deregisters the per-ctx selfpipe fd.
* assert that this is the case: ie that we are no longer interested
in the selfpipe.
Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
tools/libxl/libxl.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 12a1013..785253d 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -162,11 +162,12 @@ int libxl_ctx_free(libxl_ctx *ctx)
while ((eject = LIBXL_LIST_FIRST(&CTX->disk_eject_evgens)))
libxl__evdisable_disk_eject(gc, eject);
+ libxl_childproc_setmode(CTX,0,0);
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);
- libxl__ev_fd_deregister(gc, &ctx->sigchld_selfpipe_efd);
+ assert(!libxl__ev_fd_isregistered(&ctx->sigchld_selfpipe_efd));
/* Now there should be no more events requested from the application: */
--
1.7.10.4
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH 4/6] libxl: events: Tear down SIGCHLD machinery on ctx destruction
2014-11-27 18:27 ` [PATCH 4/6] libxl: events: Tear down SIGCHLD machinery on ctx destruction Ian Jackson
@ 2014-11-28 12:51 ` Ian Campbell
0 siblings, 0 replies; 43+ messages in thread
From: Ian Campbell @ 2014-11-28 12:51 UTC (permalink / raw)
To: Ian Jackson; +Cc: Jim Fehlig, xen-devel
On Thu, 2014-11-27 at 18:27 +0000, Ian Jackson wrote:
> We want to have no fd events registered when we are idle.
> Also, we should put back the default SIGCHLD handler. So:
>
> * In libxl_ctx_free, use libxl_childproc_setmode to set the mode to
> the default, which is libxl_sigchld_owner_libxl (ie `libxl owns
> SIGCHLD only when it has active children').
>
> But of course there are no active children at libxl teardown so
> this results in libxl__sigchld_notneeded: the ctx loses its
> interest in SIGCHLD (unsetting the SIGCHLD handler if we were the
> last ctx) and deregisters the per-ctx selfpipe fd.
>
> * assert that this is the case: ie that we are no longer interested
> in the selfpipe.
>
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
> ---
> tools/libxl/libxl.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 12a1013..785253d 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -162,11 +162,12 @@ int libxl_ctx_free(libxl_ctx *ctx)
> while ((eject = LIBXL_LIST_FIRST(&CTX->disk_eject_evgens)))
> libxl__evdisable_disk_eject(gc, eject);
>
> + libxl_childproc_setmode(CTX,0,0);
> 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);
> - libxl__ev_fd_deregister(gc, &ctx->sigchld_selfpipe_efd);
> + assert(!libxl__ev_fd_isregistered(&ctx->sigchld_selfpipe_efd));
>
> /* Now there should be no more events requested from the application: */
>
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH 5/6] libxl: events: Deregister evtchn fd when not needed
2014-11-27 18:27 ` [PATCH for-4.5 0/6] libxl: events: Tear down fd interests when idle Ian Jackson
` (3 preceding siblings ...)
2014-11-27 18:27 ` [PATCH 4/6] libxl: events: Tear down SIGCHLD machinery on ctx destruction Ian Jackson
@ 2014-11-27 18:27 ` Ian Jackson
2014-11-28 13:04 ` Ian Campbell
2014-11-27 18:27 ` [PATCH 6/6] libxl: events: Document and enforce actual callbacks restriction Ian Jackson
` (2 subsequent siblings)
7 siblings, 1 reply; 43+ messages in thread
From: Ian Jackson @ 2014-11-27 18:27 UTC (permalink / raw)
To: xen-devel; +Cc: Jim Fehlig, Ian Jackson, Ian Campbell
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.jackson@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
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH 5/6] libxl: events: Deregister evtchn fd when not needed
2014-11-27 18:27 ` [PATCH 5/6] libxl: events: Deregister evtchn fd when not needed Ian Jackson
@ 2014-11-28 13:04 ` Ian Campbell
2014-11-28 14:47 ` Ian Jackson
0 siblings, 1 reply; 43+ messages in thread
From: Ian Campbell @ 2014-11-28 13:04 UTC (permalink / raw)
To: Ian Jackson; +Cc: Jim Fehlig, xen-devel
On Thu, 2014-11-27 at 18:27 +0000, 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.jackson@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.
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH 5/6] libxl: events: Deregister evtchn fd when not needed
2014-11-28 13:04 ` Ian Campbell
@ 2014-11-28 14:47 ` Ian Jackson
2014-11-28 14:52 ` Ian Campbell
0 siblings, 1 reply; 43+ messages in thread
From: Ian Jackson @ 2014-11-28 14:47 UTC (permalink / raw)
To: Ian Campbell; +Cc: Jim Fehlig, xen-devel
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:
> > 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.
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH 5/6] libxl: events: Deregister evtchn fd when not needed
2014-11-28 14:47 ` Ian Jackson
@ 2014-11-28 14:52 ` Ian Campbell
2014-12-09 11:22 ` Ian Jackson
0 siblings, 1 reply; 43+ messages in thread
From: Ian Campbell @ 2014-11-28 14:52 UTC (permalink / raw)
To: Ian Jackson; +Cc: Jim Fehlig, xen-devel
On Fri, 2014-11-28 at 14:47 +0000, 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 +0000, 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.
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH 5/6] libxl: events: Deregister evtchn fd when not needed
2014-11-28 14:52 ` Ian Campbell
@ 2014-12-09 11:22 ` Ian Jackson
2014-12-09 11:31 ` Ian Campbell
0 siblings, 1 reply; 43+ messages in thread
From: Ian Jackson @ 2014-12-09 11:22 UTC (permalink / raw)
To: Ian Campbell; +Cc: Jim Fehlig, xen-devel
Ian Campbell writes ("Re: [PATCH 5/6] libxl: events: Deregister evtchn fd when not needed"):
> On Fri, 2014-11-28 at 14:47 +0000, 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.
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH 5/6] libxl: events: Deregister evtchn fd when not needed
2014-12-09 11:22 ` Ian Jackson
@ 2014-12-09 11:31 ` Ian Campbell
2014-12-09 15:48 ` [PATCH 5/6] libxl: events: Deregister evtchn fd when not needed [and 1 more messages] Ian Jackson
0 siblings, 1 reply; 43+ messages in thread
From: Ian Campbell @ 2014-12-09 11:31 UTC (permalink / raw)
To: Ian Jackson; +Cc: Jim Fehlig, xen-devel
On Tue, 2014-12-09 at 11:22 +0000, 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 +0000, 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.
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH 5/6] libxl: events: Deregister evtchn fd when not needed [and 1 more messages]
2014-12-09 11:31 ` Ian Campbell
@ 2014-12-09 15:48 ` Ian Jackson
0 siblings, 0 replies; 43+ messages in thread
From: Ian Jackson @ 2014-12-09 15:48 UTC (permalink / raw)
To: Ian Campbell; +Cc: Jim Fehlig, xen-devel, Ian Jackson
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)
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH 6/6] libxl: events: Document and enforce actual callbacks restriction
2014-11-27 18:27 ` [PATCH for-4.5 0/6] libxl: events: Tear down fd interests when idle Ian Jackson
` (4 preceding siblings ...)
2014-11-27 18:27 ` [PATCH 5/6] libxl: events: Deregister evtchn fd when not needed Ian Jackson
@ 2014-11-27 18:27 ` Ian Jackson
2014-11-28 13:04 ` Ian Campbell
2014-11-27 18:30 ` [PATCH for-4.5 0/6] libxl: events: Tear down fd interests when idle Ian Jackson
2014-12-09 15:54 ` [PATCH for-4.5 v2 " Ian Jackson
7 siblings, 1 reply; 43+ messages in thread
From: Ian Jackson @ 2014-11-27 18:27 UTC (permalink / raw)
To: xen-devel; +Cc: Jim Fehlig, Ian Jackson, Ian Campbell
libxl_event_register_callbacks cannot reasonably be called while libxl
is busy (has outstanding operations and/or enabled events).
This is because the previous spec implied (although not entirely
clearly) that event hooks would not be called for existing fd and
timeout interests. There is thus no way to reliably ensure that libxl
would get told about fds and timeouts which it became interested in
beforehand.
So there have to be no such fds or timeouts, which means that the
callbacks must only be registered or changed when the ctx is idle.
Document this restriction, and enforce it with a pair of asserts.
(It would be nicer, perhaps, to say that the application may not call
libxl_osevent_register_hooks other than right after creating the ctx.
But there are existing callers, including libvirt, who do it later -
even after doing major operations such as domain creation.)
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
tools/libxl/libxl_event.c | 2 ++
tools/libxl/libxl_event.h | 6 ++----
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/tools/libxl/libxl_event.c b/tools/libxl/libxl_event.c
index 716f318..45f0871 100644
--- a/tools/libxl/libxl_event.c
+++ b/tools/libxl/libxl_event.c
@@ -1217,6 +1217,8 @@ void libxl_osevent_register_hooks(libxl_ctx *ctx,
{
GC_INIT(ctx);
CTX_LOCK;
+ assert(LIBXL_LIST_EMPTY(&ctx->efds));
+ assert(LIBXL_TAILQ_EMPTY(&ctx->etimes));
ctx->osevent_hooks = hooks;
ctx->osevent_user = user;
CTX_UNLOCK;
diff --git a/tools/libxl/libxl_event.h b/tools/libxl/libxl_event.h
index b5db83c..3c6fcfe 100644
--- a/tools/libxl/libxl_event.h
+++ b/tools/libxl/libxl_event.h
@@ -124,10 +124,8 @@ void libxl_event_register_callbacks(libxl_ctx *ctx,
* different parameters, as the application likes; the most recent
* call determines the libxl behaviour. However it is NOT safe to
* call _register_callbacks concurrently with, or reentrantly from,
- * any other libxl function.
- *
- * Calls to _register_callbacks do not affect events which have
- * already occurred.
+ * any other libxl function, nor while any event-generation
+ * facilities are enabled.
*/
--
1.7.10.4
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH 6/6] libxl: events: Document and enforce actual callbacks restriction
2014-11-27 18:27 ` [PATCH 6/6] libxl: events: Document and enforce actual callbacks restriction Ian Jackson
@ 2014-11-28 13:04 ` Ian Campbell
0 siblings, 0 replies; 43+ messages in thread
From: Ian Campbell @ 2014-11-28 13:04 UTC (permalink / raw)
To: Ian Jackson; +Cc: Jim Fehlig, xen-devel
On Thu, 2014-11-27 at 18:27 +0000, Ian Jackson wrote:
> libxl_event_register_callbacks cannot reasonably be called while libxl
> is busy (has outstanding operations and/or enabled events).
>
> This is because the previous spec implied (although not entirely
> clearly) that event hooks would not be called for existing fd and
> timeout interests. There is thus no way to reliably ensure that libxl
> would get told about fds and timeouts which it became interested in
> beforehand.
>
> So there have to be no such fds or timeouts, which means that the
> callbacks must only be registered or changed when the ctx is idle.
>
> Document this restriction, and enforce it with a pair of asserts.
>
> (It would be nicer, perhaps, to say that the application may not call
> libxl_osevent_register_hooks other than right after creating the ctx.
> But there are existing callers, including libvirt, who do it later -
> even after doing major operations such as domain creation.)
>
> Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH for-4.5 0/6] libxl: events: Tear down fd interests when idle
2014-11-27 18:27 ` [PATCH for-4.5 0/6] libxl: events: Tear down fd interests when idle Ian Jackson
` (5 preceding siblings ...)
2014-11-27 18:27 ` [PATCH 6/6] libxl: events: Document and enforce actual callbacks restriction Ian Jackson
@ 2014-11-27 18:30 ` Ian Jackson
2014-11-28 13:05 ` Ian Campbell
2014-12-08 10:33 ` Ian Campbell
2014-12-09 15:54 ` [PATCH for-4.5 v2 " Ian Jackson
7 siblings, 2 replies; 43+ messages in thread
From: Ian Jackson @ 2014-11-27 18:30 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk; +Cc: Jim Fehlig, xen-devel, Ian Campbell
Konrad: here is a set of fixes for libxl which ought IMO to be in 4.5.
See below, or the rest of the thread, for details. It's broken up
into 6 tiny patches for ease of review.
Ian Jackson writes ("[PATCH for-4.5 0/6] libxl: events: Tear down fd interests when idle"):
> If libxl_event_register_callbacks is called with nonzero hooks while
> any part of libxl has any fd interests registered internally, then:
>
> * There is no way for libxl to notify the application that it wants
> to be told about these fds, because the spec in the doc comment
> says that the new hooks are not called for existing interests.
>
> * When libxl becomes no longer interested, it will try to find the
> nexus for the deregistration hook. But such a nexus is only set up
> with nonzero hooks, so libxl will dereference NULL.
>
> * Specifically, since 66bff9fd492f, libxl would unconditionally
> become interested in its event channel fd during setup. So if the
> application sets nontrivial hooks it will always crash in
> libxl_ctx_free. (This case reported as a bug by Ian Campbell.)
>
> To fix this, it would be nice to simply forbid `late' registration of
> event hooks. But this would be an incompatible API changel. And
> indeed libvirt already registers event hooks after using the ctx to
> create a domain (!)
>
> So instead we add the minimum workable restriction: hooks can (only)
> be registered when libxl is idle.
>
> To do this we need to:
> * Defer registration of fds until they are needed.
> * Deregister fds again as they become idle.
> There is no need to do anything about timeouts because an idle libxl
> already never has any timeouts.
>
> In this series I add defensive assertions. This is a good idea
> because violations of the rules typically produce crashes anyway, but
> distant from the cause.
>
>
> This series should be included in Xen 4.5:
>
> The evtchn fd issue is new in 4.5 - that is, we have a regression
> since 4.4. It causes libvirt to segfault.
>
> But even in 4.4 there are potential bugs, with symptoms such as the
> libxl watch fd not being properly registered, so that operations are
> unreasonably delayed, or crashes on ctx teardown. So after these
> patches make it into 4.5 master the relevant subset should probably be
> backported.
Thanks,
Ian.
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH for-4.5 0/6] libxl: events: Tear down fd interests when idle
2014-11-27 18:30 ` [PATCH for-4.5 0/6] libxl: events: Tear down fd interests when idle Ian Jackson
@ 2014-11-28 13:05 ` Ian Campbell
2014-12-08 16:18 ` Konrad Rzeszutek Wilk
2014-12-08 10:33 ` Ian Campbell
1 sibling, 1 reply; 43+ messages in thread
From: Ian Campbell @ 2014-11-28 13:05 UTC (permalink / raw)
To: Ian Jackson; +Cc: Jim Fehlig, xen-devel
On Thu, 2014-11-27 at 18:30 +0000, Ian Jackson wrote:
> Konrad: here is a set of fixes for libxl which ought IMO to be in 4.5.
> See below, or the rest of the thread, for details. It's broken up
> into 6 tiny patches for ease of review.
I tested an identical version of this series, just without the commit
logs, with libvirt on ARM. So in addition to my various acks:
Tested-by: Ian Campbell <ian.campbell@citrix.com>
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH for-4.5 0/6] libxl: events: Tear down fd interests when idle
2014-11-28 13:05 ` Ian Campbell
@ 2014-12-08 16:18 ` Konrad Rzeszutek Wilk
0 siblings, 0 replies; 43+ messages in thread
From: Konrad Rzeszutek Wilk @ 2014-12-08 16:18 UTC (permalink / raw)
To: Ian Campbell; +Cc: Jim Fehlig, xen-devel, Ian Jackson
On Fri, Nov 28, 2014 at 01:05:39PM +0000, Ian Campbell wrote:
> On Thu, 2014-11-27 at 18:30 +0000, Ian Jackson wrote:
> > Konrad: here is a set of fixes for libxl which ought IMO to be in 4.5.
> > See below, or the rest of the thread, for details. It's broken up
> > into 6 tiny patches for ease of review.
>
> I tested an identical version of this series, just without the commit
> logs, with libvirt on ARM. So in addition to my various acks:
>
> Tested-by: Ian Campbell <ian.campbell@citrix.com>
Release-Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH for-4.5 0/6] libxl: events: Tear down fd interests when idle
2014-11-27 18:30 ` [PATCH for-4.5 0/6] libxl: events: Tear down fd interests when idle Ian Jackson
2014-11-28 13:05 ` Ian Campbell
@ 2014-12-08 10:33 ` Ian Campbell
1 sibling, 0 replies; 43+ messages in thread
From: Ian Campbell @ 2014-12-08 10:33 UTC (permalink / raw)
To: Ian Jackson; +Cc: Jim Fehlig, xen-devel
On Thu, 2014-11-27 at 18:30 +0000, Ian Jackson wrote:
> Konrad: here is a set of fixes for libxl which ought IMO to be in 4.5.
> See below, or the rest of the thread, for details. It's broken up
> into 6 tiny patches for ease of review.
Konrad did I miss you reply/ack here or has one yet to be given?
Ian.
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH for-4.5 v2 0/6] libxl: events: Tear down fd interests when idle
2014-11-27 18:27 ` [PATCH for-4.5 0/6] libxl: events: Tear down fd interests when idle Ian Jackson
` (6 preceding siblings ...)
2014-11-27 18:30 ` [PATCH for-4.5 0/6] libxl: events: Tear down fd interests when idle Ian Jackson
@ 2014-12-09 15:54 ` Ian Jackson
2014-12-09 15:54 ` [PATCH 1/6] libxl: events: Assert that libxl_ctx_free is not called from a hook Ian Jackson
` (6 more replies)
7 siblings, 7 replies; 43+ messages in thread
From: Ian Jackson @ 2014-12-09 15:54 UTC (permalink / raw)
To: xen-devel; +Cc: Jim Fehlig, Ian Campbell
If libxl_event_register_callbacks is called with nonzero hooks while
any part of libxl has any fd interests registered internally, then:
* There is no way for libxl to notify the application that it wants
to be told about these fds, because the spec in the doc comment
says that the new hooks are not called for existing interests.
* When libxl becomes no longer interested, it will try to find the
nexus for the deregistration hook. But such a nexus is only set up
with nonzero hooks, so libxl will dereference NULL.
* Specifically, since 66bff9fd492f, libxl would unconditionally
become interested in its event channel fd during setup. So if the
application sets nontrivial hooks it will always crash in
libxl_ctx_free. (This case reported as a bug by Ian Campbell.)
To fix this, it would be nice to simply forbid `late' registration of
event hooks. But this would be an incompatible API changel. And
indeed libvirt already registers event hooks after using the ctx to
create a domain (!)
So instead we add the minimum workable restriction: hooks can (only)
be registered when libxl is idle.
To do this we need to:
* Defer registration of fds until they are needed.
* Deregister fds again as they become idle.
There is no need to do anything about timeouts because an idle libxl
already never has any timeouts.
In this series I add defensive assertions. This is a good idea
because violations of the rules typically produce crashes anyway, but
distant from the cause.
The changes in version 2 of the series are:
* Fix bogus non-idempotent of evtchn_efd. (Bug in the patch.)
* Do not put evtchn_fd in the ctx. (Style improvement.)
This series should be included in Xen 4.5:
The evtchn fd issue is new in 4.5 - that is, we have a regression
since 4.4. It causes libvirt to segfault.
But even in 4.4 there are potential bugs, with symptoms such as the
libxl watch fd not being properly registered, so that operations are
unreasonably delayed, or crashes on ctx teardown. So after these
patches make it into 4.5 master the relevant subset should probably be
backported.
Version 1 of this series was:
Release-Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
which I have transferred into this version.
Version 1 had:
Tested-by: Ian Campbell <ian.campbell@citrix.com>
but that does not apply any more from patch 5 onwards since patch 5
has changed.
Ian C, do you still have the setup you used to test v1 ?
Thanks,
Ian.
^ permalink raw reply [flat|nested] 43+ messages in thread* [PATCH 1/6] libxl: events: Assert that libxl_ctx_free is not called from a hook
2014-12-09 15:54 ` [PATCH for-4.5 v2 " Ian Jackson
@ 2014-12-09 15:54 ` Ian Jackson
2014-12-09 15:54 ` [PATCH 2/6] libxl: events: Deregister xenstore watch fd when not needed Ian Jackson
` (5 subsequent siblings)
6 siblings, 0 replies; 43+ messages in thread
From: Ian Jackson @ 2014-12-09 15:54 UTC (permalink / raw)
To: xen-devel; +Cc: Jim Fehlig, Ian Jackson, Ian Campbell
No-one in their right mind would do this, and if they did everything
would definitely collapse. Arrange that if this happens, we crash
ASAP.
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
Tested-by: Ian Campbell <ian.campbell@citrix.com>
Release-Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
tools/libxl/libxl.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 74c00dc..55ef535 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -148,6 +148,8 @@ int libxl_ctx_free(libxl_ctx *ctx)
{
if (!ctx) return 0;
+ assert(!ctx->osevent_in_hook);
+
int i;
GC_INIT(ctx);
--
1.7.10.4
^ permalink raw reply related [flat|nested] 43+ messages in thread* [PATCH 2/6] libxl: events: Deregister xenstore watch fd when not needed
2014-12-09 15:54 ` [PATCH for-4.5 v2 " Ian Jackson
2014-12-09 15:54 ` [PATCH 1/6] libxl: events: Assert that libxl_ctx_free is not called from a hook Ian Jackson
@ 2014-12-09 15:54 ` Ian Jackson
2014-12-09 15:59 ` Ian Campbell
2014-12-09 15:54 ` [PATCH 3/6] libxl: events: Deregister, don't just modify, sigchld pipe fd Ian Jackson
` (4 subsequent siblings)
6 siblings, 1 reply; 43+ messages in thread
From: Ian Jackson @ 2014-12-09 15:54 UTC (permalink / raw)
To: xen-devel; +Cc: Jim Fehlig, Ian Jackson, Ian Campbell
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 <ian.jackson@eu.citrix.com>
---
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
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH 2/6] libxl: events: Deregister xenstore watch fd when not needed
2014-12-09 15:54 ` [PATCH 2/6] libxl: events: Deregister xenstore watch fd when not needed Ian Jackson
@ 2014-12-09 15:59 ` Ian Campbell
2014-12-09 16:13 ` Ian Jackson
0 siblings, 1 reply; 43+ messages in thread
From: Ian Campbell @ 2014-12-09 15:59 UTC (permalink / raw)
To: Ian Jackson; +Cc: Jim Fehlig, xen-devel
On Tue, 2014-12-09 at 15:54 +0000, 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 <ian.jackson@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 2/6] libxl: events: Deregister xenstore watch fd when not needed
2014-12-09 15:59 ` Ian Campbell
@ 2014-12-09 16:13 ` Ian Jackson
0 siblings, 0 replies; 43+ messages in thread
From: Ian Jackson @ 2014-12-09 16:13 UTC (permalink / raw)
To: Ian Campbell; +Cc: Jim Fehlig, xen-devel
Ian Campbell writes ("Re: [PATCH 2/6] libxl: events: Deregister xenstore watch fd when not needed"):
> On Tue, 2014-12-09 at 15:54 +0000, 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 <ian.jackson@eu.citrix.com>
>
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
Thanks. In fact you had acked this already but somehow I have dropped
that.
Ian.
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH 3/6] libxl: events: Deregister, don't just modify, sigchld pipe fd
2014-12-09 15:54 ` [PATCH for-4.5 v2 " Ian Jackson
2014-12-09 15:54 ` [PATCH 1/6] libxl: events: Assert that libxl_ctx_free is not called from a hook Ian Jackson
2014-12-09 15:54 ` [PATCH 2/6] libxl: events: Deregister xenstore watch fd when not needed Ian Jackson
@ 2014-12-09 15:54 ` Ian Jackson
2014-12-09 15:54 ` [PATCH 4/6] libxl: events: Tear down SIGCHLD machinery on ctx destruction Ian Jackson
` (3 subsequent siblings)
6 siblings, 0 replies; 43+ messages in thread
From: Ian Jackson @ 2014-12-09 15:54 UTC (permalink / raw)
To: xen-devel; +Cc: Jim Fehlig, Ian Jackson, Ian Campbell
We want to have no fd events registered when we are idle. This
implies that we must be able to deregister our interest in the sigchld
self-pipe fd, not just modify to request no events.
Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
Tested-by: Ian Campbell <ian.campbell@citrix.com>
Release-Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
tools/libxl/libxl_fork.c | 9 +--------
1 file changed, 1 insertion(+), 8 deletions(-)
diff --git a/tools/libxl/libxl_fork.c b/tools/libxl/libxl_fork.c
index fa15095..144208a 100644
--- a/tools/libxl/libxl_fork.c
+++ b/tools/libxl/libxl_fork.c
@@ -372,15 +372,8 @@ static void sigchld_user_remove(libxl_ctx *ctx) /* idempotent */
void libxl__sigchld_notneeded(libxl__gc *gc) /* non-reentrant, idempotent */
{
- int rc;
-
sigchld_user_remove(CTX);
-
- if (libxl__ev_fd_isregistered(&CTX->sigchld_selfpipe_efd)) {
- rc = libxl__ev_fd_modify(gc, &CTX->sigchld_selfpipe_efd, 0);
- if (rc)
- libxl__ev_fd_deregister(gc, &CTX->sigchld_selfpipe_efd);
- }
+ libxl__ev_fd_deregister(gc, &CTX->sigchld_selfpipe_efd);
}
int libxl__sigchld_needed(libxl__gc *gc) /* non-reentrant, idempotent */
--
1.7.10.4
^ permalink raw reply related [flat|nested] 43+ messages in thread* [PATCH 4/6] libxl: events: Tear down SIGCHLD machinery on ctx destruction
2014-12-09 15:54 ` [PATCH for-4.5 v2 " Ian Jackson
` (2 preceding siblings ...)
2014-12-09 15:54 ` [PATCH 3/6] libxl: events: Deregister, don't just modify, sigchld pipe fd Ian Jackson
@ 2014-12-09 15:54 ` Ian Jackson
2014-12-09 15:54 ` [PATCH 5/6] libxl: events: Deregister evtchn fd when not needed Ian Jackson
` (2 subsequent siblings)
6 siblings, 0 replies; 43+ messages in thread
From: Ian Jackson @ 2014-12-09 15:54 UTC (permalink / raw)
To: xen-devel; +Cc: Jim Fehlig, Ian Jackson, Ian Campbell
We want to have no fd events registered when we are idle.
Also, we should put back the default SIGCHLD handler. So:
* In libxl_ctx_free, use libxl_childproc_setmode to set the mode to
the default, which is libxl_sigchld_owner_libxl (ie `libxl owns
SIGCHLD only when it has active children').
But of course there are no active children at libxl teardown so
this results in libxl__sigchld_notneeded: the ctx loses its
interest in SIGCHLD (unsetting the SIGCHLD handler if we were the
last ctx) and deregisters the per-ctx selfpipe fd.
* assert that this is the case: ie that we are no longer interested
in the selfpipe.
Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
Tested-by: Ian Campbell <ian.campbell@citrix.com>
Release-Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
tools/libxl/libxl.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index a238621..8f06043 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -162,11 +162,12 @@ int libxl_ctx_free(libxl_ctx *ctx)
while ((eject = LIBXL_LIST_FIRST(&CTX->disk_eject_evgens)))
libxl__evdisable_disk_eject(gc, eject);
+ libxl_childproc_setmode(CTX,0,0);
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);
- libxl__ev_fd_deregister(gc, &ctx->sigchld_selfpipe_efd);
+ assert(!libxl__ev_fd_isregistered(&ctx->sigchld_selfpipe_efd));
/* Now there should be no more events requested from the application: */
--
1.7.10.4
^ permalink raw reply related [flat|nested] 43+ messages in thread* [PATCH 5/6] libxl: events: Deregister evtchn fd when not needed
2014-12-09 15:54 ` [PATCH for-4.5 v2 " Ian Jackson
` (3 preceding siblings ...)
2014-12-09 15:54 ` [PATCH 4/6] libxl: events: Tear down SIGCHLD machinery on ctx destruction Ian Jackson
@ 2014-12-09 15:54 ` Ian Jackson
2014-12-09 15:56 ` Ian Campbell
2014-12-09 15:54 ` [PATCH 6/6] libxl: events: Document and enforce actual callbacks restriction Ian Jackson
2014-12-10 10:45 ` [PATCH for-4.5 v2 0/6] libxl: events: Tear down fd interests when idle Ian Campbell
6 siblings, 1 reply; 43+ messages in thread
From: Ian Jackson @ 2014-12-09 15:54 UTC (permalink / raw)
To: xen-devel; +Cc: Jim Fehlig, Ian Jackson, Ian Campbell
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.jackson@eu.citrix.com>
Release-Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@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
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH 5/6] libxl: events: Deregister evtchn fd when not needed
2014-12-09 15:54 ` [PATCH 5/6] libxl: events: Deregister evtchn fd when not needed Ian Jackson
@ 2014-12-09 15:56 ` Ian Campbell
0 siblings, 0 replies; 43+ messages in thread
From: Ian Campbell @ 2014-12-09 15:56 UTC (permalink / raw)
To: Ian Jackson; +Cc: Jim Fehlig, xen-devel
On Tue, 2014-12-09 at 15:54 +0000, 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.jackson@eu.citrix.com>
> Release-Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH 6/6] libxl: events: Document and enforce actual callbacks restriction
2014-12-09 15:54 ` [PATCH for-4.5 v2 " Ian Jackson
` (4 preceding siblings ...)
2014-12-09 15:54 ` [PATCH 5/6] libxl: events: Deregister evtchn fd when not needed Ian Jackson
@ 2014-12-09 15:54 ` Ian Jackson
2014-12-10 10:45 ` [PATCH for-4.5 v2 0/6] libxl: events: Tear down fd interests when idle Ian Campbell
6 siblings, 0 replies; 43+ messages in thread
From: Ian Jackson @ 2014-12-09 15:54 UTC (permalink / raw)
To: xen-devel; +Cc: Jim Fehlig, Ian Jackson, Ian Campbell
libxl_event_register_callbacks cannot reasonably be called while libxl
is busy (has outstanding operations and/or enabled events).
This is because the previous spec implied (although not entirely
clearly) that event hooks would not be called for existing fd and
timeout interests. There is thus no way to reliably ensure that libxl
would get told about fds and timeouts which it became interested in
beforehand.
So there have to be no such fds or timeouts, which means that the
callbacks must only be registered or changed when the ctx is idle.
Document this restriction, and enforce it with a pair of asserts.
(It would be nicer, perhaps, to say that the application may not call
libxl_osevent_register_hooks other than right after creating the ctx.
But there are existing callers, including libvirt, who do it later -
even after doing major operations such as domain creation.)
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
Release-Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
tools/libxl/libxl_event.c | 2 ++
tools/libxl/libxl_event.h | 6 ++----
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/tools/libxl/libxl_event.c b/tools/libxl/libxl_event.c
index a36e6d9..0d874d9 100644
--- a/tools/libxl/libxl_event.c
+++ b/tools/libxl/libxl_event.c
@@ -1219,6 +1219,8 @@ void libxl_osevent_register_hooks(libxl_ctx *ctx,
{
GC_INIT(ctx);
CTX_LOCK;
+ assert(LIBXL_LIST_EMPTY(&ctx->efds));
+ assert(LIBXL_TAILQ_EMPTY(&ctx->etimes));
ctx->osevent_hooks = hooks;
ctx->osevent_user = user;
CTX_UNLOCK;
diff --git a/tools/libxl/libxl_event.h b/tools/libxl/libxl_event.h
index b5db83c..3c6fcfe 100644
--- a/tools/libxl/libxl_event.h
+++ b/tools/libxl/libxl_event.h
@@ -124,10 +124,8 @@ void libxl_event_register_callbacks(libxl_ctx *ctx,
* different parameters, as the application likes; the most recent
* call determines the libxl behaviour. However it is NOT safe to
* call _register_callbacks concurrently with, or reentrantly from,
- * any other libxl function.
- *
- * Calls to _register_callbacks do not affect events which have
- * already occurred.
+ * any other libxl function, nor while any event-generation
+ * facilities are enabled.
*/
--
1.7.10.4
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH for-4.5 v2 0/6] libxl: events: Tear down fd interests when idle
2014-12-09 15:54 ` [PATCH for-4.5 v2 " Ian Jackson
` (5 preceding siblings ...)
2014-12-09 15:54 ` [PATCH 6/6] libxl: events: Document and enforce actual callbacks restriction Ian Jackson
@ 2014-12-10 10:45 ` Ian Campbell
6 siblings, 0 replies; 43+ messages in thread
From: Ian Campbell @ 2014-12-10 10:45 UTC (permalink / raw)
To: Ian Jackson; +Cc: Jim Fehlig, xen-devel
On Tue, 2014-12-09 at 15:54 +0000, Ian Jackson wrote:
> Version 1 had:
> Tested-by: Ian Campbell <ian.campbell@citrix.com>
> but that does not apply any more from patch 5 onwards since patch 5
> has changed.
>
> Ian C, do you still have the setup you used to test v1 ?
Yes, and I've just rerun with this new version of the patches and it was
fine, so the tested-by can stand.
Ian.
^ permalink raw reply [flat|nested] 43+ messages in thread