[v3] Fix: libxl workaround, multiple connection to single QMP socket

[Xen-devel] [XEN PATCH for-4.13 v3 6/7] libxl: Introduce libxl__ev_immediate

Posted by Anthony PERARD 6 years, 2 months ago

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(+)

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


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [XEN PATCH for-4.13 v3 6/7] libxl: Introduce libxl__ev_immediate

Posted by Ian Jackson 6 years, 2 months ago

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.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [XEN PATCH for-4.13 v3 6/7] libxl: Introduce libxl__ev_immediate

Posted by Anthony PERARD 6 years, 2 months ago

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [XEN PATCH for-4.13 v3 6/7] libxl: Introduce libxl__ev_immediate

Posted by Ian Jackson 6 years, 2 months ago

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.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [XEN PATCH for-4.13 v4 6/7] libxl: Introduce libxl__ev_immediate

Posted by Anthony PERARD 6 years, 2 months ago

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:
    v4:
    - rework foreach loop in egc_run_callbacks, to a safe alternative where
      the list is safe to be modified.
    - use STAILQ instead of TAILQ
    
    v3:
    - new patch

 tools/libxl/libxl_event.c    | 20 ++++++++++++++++++++
 tools/libxl/libxl_internal.h | 17 +++++++++++++++++
 2 files changed, 37 insertions(+)

diff --git a/tools/libxl/libxl_event.c b/tools/libxl/libxl_event.c
index 43155368de76..aa8b7d1945bd 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_STAILQ_INSERT_TAIL(&egc->ev_immediates, ei, entry);
+}
+
 /*
  * domain death/destruction
  */
@@ -1395,6 +1404,17 @@ 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;
+
+    while (!LIBXL_STAILQ_EMPTY(&egc->ev_immediates)) {
+        ei = LIBXL_STAILQ_FIRST(&egc->ev_immediates);
+        LIBXL_STAILQ_REMOVE_HEAD(&egc->ev_immediates, 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..0b75eef2a22f 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_STAILQ_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_STAILQ_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_STAILQ_INIT(&(egc).ev_immediates);        \
     } while(0)
 
 _hidden void libxl__egc_cleanup(libxl__egc *egc);
-- 
Anthony PERARD


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [XEN PATCH for-4.13 v4 6/7] libxl: Introduce libxl__ev_immediate

Posted by Ian Jackson 6 years, 2 months ago

Anthony PERARD writes ("[XEN PATCH for-4.13 v4 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.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

However:

> +    while (!LIBXL_STAILQ_EMPTY(&egc->ev_immediates)) {
> +        ei = LIBXL_STAILQ_FIRST(&egc->ev_immediates);
> +        LIBXL_STAILQ_REMOVE_HEAD(&egc->ev_immediates, entry);

I think maybe you were unaware that LIBXL_STAILQ_FIRST may be used on
an empty list and will return NULL.  This makes no difference (the
compiler will get rid of the duplication, surely) but might make the
code fractionally shorter.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel