| Message ID | 20191118171309.1459302-7-anthony.perard@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix: libxl workaround, multiple connection to single QMP socket | expand |
Anthony PERARD writes ("[XEN PATCH for-4.13 v3 6/7] libxl: Introduce libxl__ev_immediate"):
> This new ev allows to arrange a non-reentrant callback to be called.
> This happen immediately after the current event is processed and after
> other ev_immediates that would have already been registered.
Thanks for doing this work.
> + LIBXL_TAILQ_FOREACH_SAFE(ei, &egc->ev_immediates, entry, ei_tmp) {
> + LIBXL_TAILQ_REMOVE(&egc->ev_immediates, ei, entry);
I think LIBXL_TAILQ_FOREACH_SAFE is not safe enough here.
ei->callback might *add* things to egc->ev_immediates. The manpage
just says
However, unlike their unsafe counterparts, TAILQ_FOREACH and
TAILQ_FOREACH_REVERSE permit to both remove var as well as free
it from within the loop safely without interfering with the
traversal.
I can't find an explicit statement about the allowable changes with
LIBXL_TAILQ_FOREACH but I expect they are "none". See the loop in
ao__abort for what I think is the correct pattern (albeit embedded in
something more complex).
The rest of this LGTM.
Thanks,
Ian.
On Mon, Nov 18, 2019 at 05:28:17PM +0000, Ian Jackson wrote: > Anthony PERARD writes ("[XEN PATCH for-4.13 v3 6/7] libxl: Introduce libxl__ev_immediate"): > > This new ev allows to arrange a non-reentrant callback to be called. > > This happen immediately after the current event is processed and after > > other ev_immediates that would have already been registered. > > Thanks for doing this work. > > > + LIBXL_TAILQ_FOREACH_SAFE(ei, &egc->ev_immediates, entry, ei_tmp) { > > + LIBXL_TAILQ_REMOVE(&egc->ev_immediates, ei, entry); > > I think LIBXL_TAILQ_FOREACH_SAFE is not safe enough here. > ei->callback might *add* things to egc->ev_immediates. The manpage > just says > > However, unlike their unsafe counterparts, TAILQ_FOREACH and > TAILQ_FOREACH_REVERSE permit to both remove var as well as free > it from within the loop safely without interfering with the > traversal. > > I can't find an explicit statement about the allowable changes with > LIBXL_TAILQ_FOREACH but I expect they are "none". See the loop in > ao__abort for what I think is the correct pattern (albeit embedded in > something more complex). Sound good. I'll also switch to STAILQ instead, single-link tail queue for a FIFO list. Thanks,
Anthony PERARD writes ("Re: [XEN PATCH for-4.13 v3 6/7] libxl: Introduce libxl__ev_immediate"):
> Sound good. I'll also switch to STAILQ instead, single-link tail queue
> for a FIFO list.
Err, yes, that would make sense. I should have considered whether you
chose the right kind of list but it doesn't really matter much in
these paths to have one that is too featureful...
Ian.
diff --git a/tools/libxl/libxl_event.c b/tools/libxl/libxl_event.c index 43155368de76..ceb775d8ca3f 100644 --- a/tools/libxl/libxl_event.c +++ b/tools/libxl/libxl_event.c @@ -914,6 +914,15 @@ int libxl__ev_devstate_wait(libxl__ao *ao, libxl__ev_devstate *ds, return rc; } +/* + * immediate non-reentrant callback + */ + +void libxl__ev_immediate_register(libxl__egc *egc, libxl__ev_immediate *ei) +{ + LIBXL_TAILQ_INSERT_TAIL(&egc->ev_immediates, ei, entry); +} + /* * domain death/destruction */ @@ -1395,6 +1404,16 @@ static void egc_run_callbacks(libxl__egc *egc) EGC_GC; libxl_event *ev, *ev_tmp; libxl__aop_occurred *aop, *aop_tmp; + libxl__ev_immediate *ei, *ei_tmp; + + LIBXL_TAILQ_FOREACH_SAFE(ei, &egc->ev_immediates, entry, ei_tmp) { + LIBXL_TAILQ_REMOVE(&egc->ev_immediates, ei, entry); + CTX_LOCK; + /* This callback is internal to libxl and expects CTX to be + * locked. */ + ei->callback(egc, ei); + CTX_UNLOCK; + } LIBXL_TAILQ_FOREACH_SAFE(ev, &egc->occurred_for_callback, link, ev_tmp) { LIBXL_TAILQ_REMOVE(&egc->occurred_for_callback, ev, link); diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h index f95895eae17d..400752a7f8fe 100644 --- a/tools/libxl/libxl_internal.h +++ b/tools/libxl/libxl_internal.h @@ -200,6 +200,7 @@ typedef struct libxl__ev_slowlock libxl__ev_slowlock; typedef struct libxl__dm_resume_state libxl__dm_resume_state; typedef struct libxl__ao_device libxl__ao_device; typedef struct libxl__multidev libxl__multidev; +typedef struct libxl__ev_immediate libxl__ev_immediate; typedef struct libxl__domain_create_state libxl__domain_create_state; typedef void libxl__domain_create_cb(struct libxl__egc *egc, @@ -363,6 +364,20 @@ struct libxl__ev_child { LIBXL_LIST_ENTRY(struct libxl__ev_child) entry; }; +/* libxl__ev_immediate + * + * Allow to call a non-reentrant callback. + * + * `callback' will be called immediately as a new event. + */ +struct libxl__ev_immediate { + /* filled by user */ + void (*callback)(libxl__egc *, libxl__ev_immediate *); + /* private to libxl__ev_immediate */ + LIBXL_TAILQ_ENTRY(libxl__ev_immediate) entry; +}; +void libxl__ev_immediate_register(libxl__egc *, libxl__ev_immediate *); + /* * Lock for device hotplug, qmp_lock. * @@ -733,6 +748,7 @@ struct libxl__egc { struct libxl__event_list occurred_for_callback; LIBXL_TAILQ_HEAD(, libxl__ao) aos_for_callback; LIBXL_TAILQ_HEAD(, libxl__aop_occurred) aops_for_callback; + LIBXL_TAILQ_HEAD(, libxl__ev_immediate) ev_immediates; }; struct libxl__aop_occurred { @@ -2322,6 +2338,7 @@ _hidden libxl_device_model_version libxl__default_device_model(libxl__gc *gc); LIBXL_TAILQ_INIT(&(egc).occurred_for_callback); \ LIBXL_TAILQ_INIT(&(egc).aos_for_callback); \ LIBXL_TAILQ_INIT(&(egc).aops_for_callback); \ + LIBXL_TAILQ_INIT(&(egc).ev_immediates); \ } while(0) _hidden void libxl__egc_cleanup(libxl__egc *egc);
This new ev allows to arrange a non-reentrant callback to be called. This happen immediately after the current event is processed and after other ev_immediates that would have already been registered. Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> --- Notes: v3: - new patch tools/libxl/libxl_event.c | 19 +++++++++++++++++++ tools/libxl/libxl_internal.h | 17 +++++++++++++++++ 2 files changed, 36 insertions(+)