[XEN,for-4.13,v3,6/7] libxl: Introduce libxl__ev_immediate
diff mbox series

Message ID 20191118171309.1459302-7-anthony.perard@citrix.com
State New
Headers show
Series
  • Fix: libxl workaround, multiple connection to single QMP socket
Related show

Commit Message

Anthony PERARD Nov. 18, 2019, 5:13 p.m. UTC
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(+)

Comments

Ian Jackson Nov. 18, 2019, 5:28 p.m. UTC | #1
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.
Anthony PERARD Nov. 18, 2019, 5:49 p.m. UTC | #2
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,
Ian Jackson Nov. 18, 2019, 5:57 p.m. UTC | #3
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.

Patch
diff mbox series

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);