[Xen-devel] [XEN PATCH for-4.13 v3 0/7] Fix: libxl workaround, multiple connection to single QMP socket

[Xen-devel] [XEN PATCH for-4.13 v3 0/7] Fix: libxl workaround, multiple connection to single QMP socket

[Anthony PERARD]

Patch series available in this git branch:
https://xenbits.xen.org/git-http/people/aperard/xen-unstable.git br.fix-ev_qmp-multi-connect-v3

v3:
Two patches left to review:
- libxl: Introduce libxl__ev_immediate (new)
- libxl_qmp: Have a lock for QMP socket access

And Jürgen already gave his ack on 8th of November:
Release-acked-by: Juergen Gross <jgross@suse.com>

Hi,

QEMU's QMP socket doesn't allow multiple concurrent connection. Also, it
listen() on the socket with a `backlog' of only 1. On Linux at least, once that
backlog is filled connect() will return EAGAIN if the socket fd is
non-blocking. libxl may attempt many concurrent connect() attempt if for
example a guest is started with several PCI passthrough devices, and a
connect() failure lead to a failure to start the guest.

Since we can't change the listen()'s `backlog' that QEMU use, we need other
ways to workaround the issue. This patch series introduce a lock to acquire
before attempting to connect() to the QMP socket. Since the lock might be held
for to long, the series also introduce a way to cancel the acquisition of the
lock, this means killing the process that tries to get the lock.

See thread[1] for discussed alternative.
[1] https://lists.xenproject.org/archives/html/xen-devel/2019-10/msg01815.html

Cheers,

Anthony PERARD (7):
  libxl: Introduce libxl__ev_child_kill_deregister
  libxl: Move libxl__ev_devlock declaration
  libxl: Rename ev_devlock to ev_slowlock
  libxl: Introduce libxl__ev_slowlock_dispose
  libxl: libxl__ev_qmp_send now takes an egc
  libxl: Introduce libxl__ev_immediate
  libxl_qmp: Have a lock for QMP socket access

 tools/libxl/libxl_disk.c        |  16 ++--
 tools/libxl/libxl_dm.c          |   8 +-
 tools/libxl/libxl_dom_save.c    |   2 +-
 tools/libxl/libxl_dom_suspend.c |   2 +-
 tools/libxl/libxl_domain.c      |  18 ++--
 tools/libxl/libxl_event.c       |  25 +++++-
 tools/libxl/libxl_fork.c        |  48 +++++++++++
 tools/libxl/libxl_internal.c    |  41 ++++++---
 tools/libxl/libxl_internal.h    | 147 ++++++++++++++++++++------------
 tools/libxl/libxl_pci.c         |   8 +-
 tools/libxl/libxl_qmp.c         | 128 ++++++++++++++++++++-------
 tools/libxl/libxl_usb.c         |  28 +++---
 12 files changed, 336 insertions(+), 135 deletions(-)

-- 
Anthony PERARD


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

[Xen-devel] [XEN PATCH for-4.13 v3 1/7] libxl: Introduce libxl__ev_child_kill_deregister

[Anthony PERARD]

Allow to deregister the callback associated with a child death event.

The death isn't immediate will need to be collected later, so the
ev_child machinery register its own callback.

libxl__ev_child_kill_deregister() might be called by an AO operation
that is finishing/cleaning up without a chance for libxl to be
notified of the child death (via SIGCHLD). So it is possible that the
application calls libxl_ctx_free() while there are still child around.
To avoid the application getting unexpected SIGCHLD, the libxl__ao
responsible for killing a child will have to wait until it has been
properly reaped.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
---

Notes:
    v2:
    - Rename new fn to libxl__ev_child_kill_deregister
    - Rework documentation of the new API and if ev_child
    - Add debug log in libxl__ao_complete
    - Always call libxl_report_child_exitstatus() in child callback.

 tools/libxl/libxl_event.c    |  6 ++++-
 tools/libxl/libxl_fork.c     | 48 ++++++++++++++++++++++++++++++++++++
 tools/libxl/libxl_internal.h | 15 ++++++++---
 3 files changed, 65 insertions(+), 4 deletions(-)

diff --git a/tools/libxl/libxl_event.c b/tools/libxl/libxl_event.c
index 0370b6acdd1c..43155368de76 100644
--- a/tools/libxl/libxl_event.c
+++ b/tools/libxl/libxl_event.c
@@ -1878,6 +1878,9 @@ void libxl__ao_complete(libxl__egc *egc, libxl__ao *ao, int rc)
     ao->complete = 1;
     ao->rc = rc;
     LIBXL_LIST_REMOVE(ao, inprogress_entry);
+    if (ao->outstanding_killed_child)
+        LOG(DEBUG, "ao %p: .. but waiting for %d fork to exit",
+            ao, ao->outstanding_killed_child);
     libxl__ao_complete_check_progress_reports(egc, ao);
 }
 
@@ -1891,7 +1894,8 @@ static bool ao_work_outstanding(libxl__ao *ao)
      * decrement progress_reports_outstanding, and call
      * libxl__ao_complete_check_progress_reports.
      */
-    return !ao->complete || ao->progress_reports_outstanding;
+    return !ao->complete || ao->progress_reports_outstanding
+        || ao->outstanding_killed_child;
 }
 
 void libxl__ao_complete_check_progress_reports(libxl__egc *egc, libxl__ao *ao)
diff --git a/tools/libxl/libxl_fork.c b/tools/libxl/libxl_fork.c
index eea3d5d4e68e..0f1b6b518c5c 100644
--- a/tools/libxl/libxl_fork.c
+++ b/tools/libxl/libxl_fork.c
@@ -678,6 +678,54 @@ int libxl__ev_child_xenstore_reopen(libxl__gc *gc, const char *what) {
     return rc;
 }
 
+typedef struct ev_child_killed {
+    libxl__ao *ao;
+    libxl__ev_child ch;
+} ev_child_killed;
+static void deregistered_child_callback(libxl__egc *, libxl__ev_child *,
+                                        pid_t, int status);
+
+void libxl__ev_child_kill_deregister(libxl__ao *ao, libxl__ev_child *ch,
+                                     int sig)
+{
+    AO_GC;
+
+    if (!libxl__ev_child_inuse(ch))
+        return;
+
+    pid_t pid = ch->pid;
+
+    ev_child_killed *new_ch = GCNEW(new_ch);
+    new_ch->ao = ao;
+    new_ch->ch.pid = pid;
+    new_ch->ch.callback = deregistered_child_callback;
+    LIBXL_LIST_INSERT_HEAD(&CTX->children, &new_ch->ch, entry);
+    ao->outstanding_killed_child++;
+
+    LIBXL_LIST_REMOVE(ch, entry);
+    ch->pid = -1;
+    int r = kill(pid, sig);
+    if (r)
+        LOGED(ERROR, ao->domid,
+              "failed to kill child [%ld] with signal %d",
+             (unsigned long)pid, sig);
+}
+
+static void deregistered_child_callback(libxl__egc *egc,
+                                        libxl__ev_child *ch,
+                                        pid_t pid,
+                                        int status)
+{
+    ev_child_killed *ck = CONTAINER_OF(ch, *ck, ch);
+    EGC_GC;
+
+    libxl_report_child_exitstatus(CTX, XTL_ERROR,
+                                  "killed fork (dying as expected)",
+                                  pid, status);
+    ck->ao->outstanding_killed_child--;
+    libxl__ao_complete_check_progress_reports(egc, ck->ao);
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 6a614658c25d..4e433e110664 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -730,6 +730,7 @@ struct libxl__ao {
     libxl__poller *poller;
     uint32_t domid;
     LIBXL_TAILQ_ENTRY(libxl__ao) entry_for_callback;
+    int outstanding_killed_child;
 };
 
 #define LIBXL_INIT_GC(gc,ctx) do{               \
@@ -1155,9 +1156,14 @@ _hidden int libxl__ctx_evtchn_init(libxl__gc *gc); /* for libxl_ctx_alloc */
  * The parent may signal the child but it must not reap it.  That will
  * be done by the event machinery.
  *
- * It is not possible to "deregister" the child death event source.
- * It will generate exactly one event callback; until then the childw
- * is Active and may not be reused.
+ * The child death event will generate exactly one event callback; until
+ * then the childw is Active and may not be reused.
+ *
+ * libxl__ev_child_kill_deregister: Active -> Idle
+ *   This will transfer ownership of the child process death event from
+ *   `ch' to `ao', thus deregister the callback.
+ *   The `ao' completion will wait until the child have been reaped by the
+ *   event machinery.
  */
 _hidden pid_t libxl__ev_child_fork(libxl__gc *gc, libxl__ev_child *childw_out,
                                  libxl__ev_child_callback *death);
@@ -1165,6 +1171,9 @@ static inline void libxl__ev_child_init(libxl__ev_child *childw_out)
                 { childw_out->pid = -1; }
 static inline int libxl__ev_child_inuse(const libxl__ev_child *childw_out)
                 { return childw_out->pid >= 0; }
+_hidden void libxl__ev_child_kill_deregister(libxl__ao *ao,
+                                             libxl__ev_child *ch,
+                                             int sig);
 
 /* Useable (only) in the child to once more make the ctx useable for
  * xenstore operations.  logs failure in the form "what: <error
-- 
Anthony PERARD


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

[Xen-devel] [XEN PATCH for-4.13 v3 2/7] libxl: Move libxl__ev_devlock declaration

[Anthony PERARD]

We are going to want to include libxl__ev_devlock into libxl__ev_qmp.

No functional changes.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
---

Notes:
    New patch in v2:
        Move of the struct was done in "libxl_qmp: Have a lock for QMP
        socket access" before.

 tools/libxl/libxl_internal.h | 96 ++++++++++++++++++------------------
 1 file changed, 48 insertions(+), 48 deletions(-)

diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 4e433e110664..69d572c1866a 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -363,6 +363,54 @@ struct libxl__ev_child {
     LIBXL_LIST_ENTRY(struct libxl__ev_child) entry;
 };
 
+/*
+ * Lock for device hotplug, qmp_lock.
+ *
+ * libxl__ev_devlock implement a lock that is outside of CTX_LOCK in the
+ * lock hierarchy. It can be used when one want to make QMP calls to QEMU,
+ * which may take a significant amount time.
+ * It is to be acquired by an ao event callback.
+ *
+ * It is to be acquired when adding/removing devices or making changes
+ * to them when this is a slow operation and json_lock isn't appropriate.
+ *
+ * Possible states of libxl__ev_devlock:
+ *   Undefined
+ *    Might contain anything.
+ *  Idle
+ *    Struct contents are defined enough to pass to any
+ *    libxl__ev_devlock_* function.
+ *    The struct does not contain references to any allocated private
+ *    resources so can be thrown away.
+ *  Active
+ *    Waiting to get a lock.
+ *    Needs to wait until the callback is called.
+ *  LockAcquired
+ *    libxl__ev_devlock_unlock will need to be called to release the lock
+ *    and the resources of libxl__ev_devlock.
+ *
+ *  libxl__ev_devlock_init: Undefined/Idle -> Idle
+ *  libxl__ev_devlock_lock: Idle -> Active
+ *    May call callback synchronously.
+ *  libxl__ev_devlock_unlock: LockAcquired/Idle -> Idle
+ *  callback:     When called: Active -> LockAcquired (on error: Idle)
+ *    The callback is only called once.
+ */
+struct libxl__ev_devlock {
+    /* filled by user */
+    libxl__ao *ao;
+    libxl_domid domid;
+    void (*callback)(libxl__egc *, libxl__ev_devlock *, int rc);
+    /* private to libxl__ev_devlock* */
+    libxl__ev_child child;
+    char *path; /* path of the lock file itself */
+    int fd;
+    bool held;
+};
+_hidden void libxl__ev_devlock_init(libxl__ev_devlock *);
+_hidden void libxl__ev_devlock_lock(libxl__egc *, libxl__ev_devlock *);
+_hidden void libxl__ev_devlock_unlock(libxl__gc *, libxl__ev_devlock *);
+
 /*
  * QMP asynchronous calls
  *
@@ -4689,54 +4737,6 @@ static inline const char *libxl__qemu_qmp_path(libxl__gc *gc, int domid)
     return GCSPRINTF("%s/qmp-libxl-%d", libxl__run_dir_path(), domid);
 }
 
-/*
- * Lock for device hotplug, qmp_lock.
- *
- * libxl__ev_devlock implement a lock that is outside of CTX_LOCK in the
- * lock hierarchy. It can be used when one want to make QMP calls to QEMU,
- * which may take a significant amount time.
- * It is to be acquired by an ao event callback.
- *
- * It is to be acquired when adding/removing devices or making changes
- * to them when this is a slow operation and json_lock isn't appropriate.
- *
- * Possible states of libxl__ev_devlock:
- *   Undefined
- *    Might contain anything.
- *  Idle
- *    Struct contents are defined enough to pass to any
- *    libxl__ev_devlock_* function.
- *    The struct does not contain references to any allocated private
- *    resources so can be thrown away.
- *  Active
- *    Waiting to get a lock.
- *    Needs to wait until the callback is called.
- *  LockAcquired
- *    libxl__ev_devlock_unlock will need to be called to release the lock
- *    and the resources of libxl__ev_devlock.
- *
- *  libxl__ev_devlock_init: Undefined/Idle -> Idle
- *  libxl__ev_devlock_lock: Idle -> Active
- *    May call callback synchronously.
- *  libxl__ev_devlock_unlock: LockAcquired/Idle -> Idle
- *  callback:     When called: Active -> LockAcquired (on error: Idle)
- *    The callback is only called once.
- */
-struct libxl__ev_devlock {
-    /* filled by user */
-    libxl__ao *ao;
-    libxl_domid domid;
-    void (*callback)(libxl__egc *, libxl__ev_devlock *, int rc);
-    /* private to libxl__ev_devlock* */
-    libxl__ev_child child;
-    char *path; /* path of the lock file itself */
-    int fd;
-    bool held;
-};
-_hidden void libxl__ev_devlock_init(libxl__ev_devlock *);
-_hidden void libxl__ev_devlock_lock(libxl__egc *, libxl__ev_devlock *);
-_hidden void libxl__ev_devlock_unlock(libxl__gc *, libxl__ev_devlock *);
-
 /* Send control commands over xenstore and wait for an Ack. */
 _hidden int libxl__domain_pvcontrol(libxl__egc *egc,
                                     libxl__xswait_state *pvcontrol,
-- 
Anthony PERARD


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

[Xen-devel] [XEN PATCH for-4.13 v3 3/7] libxl: Rename ev_devlock to ev_slowlock

[Anthony PERARD]

We are going to introduce a different lock based on the same
implementation as the ev_devlock but with a different path. The
different slowlock will be differentiated by calling different _init()
functions.

So we rename libxl__ev_devlock to lib__ev_slowlock, but keep
libxl__ev_devlock_init().

Some log messages produced ev_slowlock are changed to print the
name of the lock file (userdata_userid).

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
---

Notes:
    New patch in v2.
        Instead of introducing many libxl__ev_qmplock*.

 tools/libxl/libxl_disk.c     | 10 +++++-----
 tools/libxl/libxl_domain.c   | 10 +++++-----
 tools/libxl/libxl_internal.c | 30 +++++++++++++++++++-----------
 tools/libxl/libxl_internal.h | 33 +++++++++++++++++----------------
 4 files changed, 46 insertions(+), 37 deletions(-)

diff --git a/tools/libxl/libxl_disk.c b/tools/libxl/libxl_disk.c
index 733ad284c866..77ae3a59bfb6 100644
--- a/tools/libxl/libxl_disk.c
+++ b/tools/libxl/libxl_disk.c
@@ -648,13 +648,13 @@ typedef struct {
     libxl_domid domid;
     libxl_device_disk *disk;
     libxl_device_disk disk_saved;
-    libxl__ev_devlock qmp_lock;
+    libxl__ev_slowlock qmp_lock;
     int dm_ver;
     libxl__ev_time time;
     libxl__ev_qmp qmp;
 } libxl__cdrom_insert_state;
 
-static void cdrom_insert_lock_acquired(libxl__egc *, libxl__ev_devlock *,
+static void cdrom_insert_lock_acquired(libxl__egc *, libxl__ev_slowlock *,
                                        int rc);
 static void cdrom_insert_ejected(libxl__egc *egc, libxl__ev_qmp *,
                                  const libxl__json_object *, int rc);
@@ -746,13 +746,13 @@ int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk,
         cdrom_insert_done(egc, cis, rc); /* must be last */
     } else {
         cis->qmp_lock.callback = cdrom_insert_lock_acquired;
-        libxl__ev_devlock_lock(egc, &cis->qmp_lock); /* must be last */
+        libxl__ev_slowlock_lock(egc, &cis->qmp_lock); /* must be last */
     }
     return AO_INPROGRESS;
 }
 
 static void cdrom_insert_lock_acquired(libxl__egc *egc,
-                                       libxl__ev_devlock *lock,
+                                       libxl__ev_slowlock *lock,
                                        int rc)
 {
     libxl__cdrom_insert_state *cis = CONTAINER_OF(lock, *cis, qmp_lock);
@@ -1052,7 +1052,7 @@ static void cdrom_insert_done(libxl__egc *egc,
     libxl__ev_time_deregister(gc, &cis->time);
     libxl__ev_qmp_dispose(gc, &cis->qmp);
     if (cis->qmp.payload_fd >= 0) close(cis->qmp.payload_fd);
-    libxl__ev_devlock_unlock(gc, &cis->qmp_lock);
+    libxl__ev_slowlock_unlock(gc, &cis->qmp_lock);
     libxl_device_disk_dispose(&cis->disk_saved);
     libxl__ao_complete(egc, cis->ao, rc);
 }
diff --git a/tools/libxl/libxl_domain.c b/tools/libxl/libxl_domain.c
index 33f9d9eaa481..7ca7a224f9a4 100644
--- a/tools/libxl/libxl_domain.c
+++ b/tools/libxl/libxl_domain.c
@@ -1874,12 +1874,12 @@ typedef struct {
     libxl__ev_qmp qmp;
     libxl__ev_time timeout;
     libxl_domain_config *d_config; /* user pointer */
-    libxl__ev_devlock devlock;
+    libxl__ev_slowlock devlock;
     libxl_bitmap qemuu_cpus;
 } retrieve_domain_configuration_state;
 
 static void retrieve_domain_configuration_lock_acquired(
-    libxl__egc *egc, libxl__ev_devlock *, int rc);
+    libxl__egc *egc, libxl__ev_slowlock *, int rc);
 static void retrieve_domain_configuration_cpu_queried(
     libxl__egc *egc, libxl__ev_qmp *qmp,
     const libxl__json_object *response, int rc);
@@ -1907,12 +1907,12 @@ int libxl_retrieve_domain_configuration(libxl_ctx *ctx, uint32_t domid,
     rdcs->devlock.ao = ao;
     rdcs->devlock.domid = domid;
     rdcs->devlock.callback = retrieve_domain_configuration_lock_acquired;
-    libxl__ev_devlock_lock(egc, &rdcs->devlock);
+    libxl__ev_slowlock_lock(egc, &rdcs->devlock);
     return AO_INPROGRESS;
 }
 
 static void retrieve_domain_configuration_lock_acquired(
-    libxl__egc *egc, libxl__ev_devlock *devlock, int rc)
+    libxl__egc *egc, libxl__ev_slowlock *devlock, int rc)
 {
     retrieve_domain_configuration_state *rdcs =
         CONTAINER_OF(devlock, *rdcs, devlock);
@@ -2204,7 +2204,7 @@ static void retrieve_domain_configuration_end(libxl__egc *egc,
     }
 
 out:
-    libxl__ev_devlock_unlock(gc, &rdcs->devlock);
+    libxl__ev_slowlock_unlock(gc, &rdcs->devlock);
     if (lock) libxl__unlock_domain_userdata(lock);
     libxl_bitmap_dispose(&rdcs->qemuu_cpus);
     libxl__ev_qmp_dispose(gc, &rdcs->qmp);
diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index 0750b69cba61..9520ac36149e 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -575,25 +575,32 @@ void libxl__update_domain_configuration(libxl__gc *gc,
     dst->b_info.video_memkb = src->b_info.video_memkb;
 }
 
-void libxl__ev_devlock_init(libxl__ev_devlock *lock)
+static void ev_slowlock_init_internal(libxl__ev_slowlock *lock,
+                                      const char *userdata_userid)
 {
     libxl__ev_child_init(&lock->child);
+    lock->userdata_userid = userdata_userid;
     lock->path = NULL;
     lock->fd = -1;
     lock->held = false;
 }
 
-static void ev_lock_prepare_fork(libxl__egc *egc, libxl__ev_devlock *lock);
+void libxl__ev_devlock_init(libxl__ev_slowlock *lock)
+{
+    ev_slowlock_init_internal(lock, "libxl-device-changes-lock");
+}
+
+static void ev_lock_prepare_fork(libxl__egc *egc, libxl__ev_slowlock *lock);
 static void ev_lock_child_callback(libxl__egc *egc, libxl__ev_child *child,
                                    pid_t pid, int status);
 
-void libxl__ev_devlock_lock(libxl__egc *egc, libxl__ev_devlock *lock)
+void libxl__ev_slowlock_lock(libxl__egc *egc, libxl__ev_slowlock *lock)
 {
     STATE_AO_GC(lock->ao);
     const char *lockfile;
 
     lockfile = libxl__userdata_path(gc, lock->domid,
-                                    "libxl-device-changes-lock", "l");
+                                    lock->userdata_userid, "l");
     if (!lockfile) goto out;
     lock->path = libxl__strdup(NOGC, lockfile);
 
@@ -603,7 +610,7 @@ void libxl__ev_devlock_lock(libxl__egc *egc, libxl__ev_devlock *lock)
     lock->callback(egc, lock, ERROR_LOCK_FAIL);
 }
 
-static void ev_lock_prepare_fork(libxl__egc *egc, libxl__ev_devlock *lock)
+static void ev_lock_prepare_fork(libxl__egc *egc, libxl__ev_slowlock *lock)
 {
     STATE_AO_GC(lock->ao);
     pid_t pid;
@@ -670,7 +677,7 @@ static void ev_lock_prepare_fork(libxl__egc *egc, libxl__ev_devlock *lock)
     libxl_fd_set_cloexec(CTX, fd, 1);
     return;
 out:
-    libxl__ev_devlock_unlock(gc, lock);
+    libxl__ev_slowlock_unlock(gc, lock);
     lock->callback(egc, lock, ERROR_LOCK_FAIL);
 }
 
@@ -678,7 +685,7 @@ static void ev_lock_child_callback(libxl__egc *egc, libxl__ev_child *child,
                                    pid_t pid, int status)
 {
     EGC_GC;
-    libxl__ev_devlock *lock = CONTAINER_OF(child, *lock, child);
+    libxl__ev_slowlock *lock = CONTAINER_OF(child, *lock, child);
     struct stat stab, fstab;
     int rc = ERROR_LOCK_FAIL;
 
@@ -726,13 +733,14 @@ static void ev_lock_child_callback(libxl__egc *egc, libxl__ev_child *child,
             rc = ERROR_LOCK_FAIL;
     }
     if (rc) {
-        LOGD(ERROR, domid, "Failed to grab qmp-lock");
-        libxl__ev_devlock_unlock(gc, lock);
+        LOGD(ERROR, domid, "Failed to grab lock for %s",
+             lock->userdata_userid);
+        libxl__ev_slowlock_unlock(gc, lock);
     }
     lock->callback(egc, lock, rc);
 }
 
-void libxl__ev_devlock_unlock(libxl__gc *gc, libxl__ev_devlock *lock)
+void libxl__ev_slowlock_unlock(libxl__gc *gc, libxl__ev_slowlock *lock)
 {
     int r;
 
@@ -754,7 +762,7 @@ void libxl__ev_devlock_unlock(libxl__gc *gc, libxl__ev_devlock *lock)
         close(lock->fd);
     }
     free(lock->path);
-    libxl__ev_devlock_init(lock);
+    ev_slowlock_init_internal(lock, lock->userdata_userid);
 }
 
 /*
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 69d572c1866a..a0f99252c39c 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -196,7 +196,7 @@ typedef struct libxl__osevent_hook_nexi libxl__osevent_hook_nexi;
 typedef struct libxl__device_type libxl__device_type;
 typedef struct libxl__json_object libxl__json_object;
 typedef struct libxl__carefd libxl__carefd;
-typedef struct libxl__ev_devlock libxl__ev_devlock;
+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;
@@ -366,7 +366,7 @@ struct libxl__ev_child {
 /*
  * Lock for device hotplug, qmp_lock.
  *
- * libxl__ev_devlock implement a lock that is outside of CTX_LOCK in the
+ * libxl__ev_slowlock implement a lock that is outside of CTX_LOCK in the
  * lock hierarchy. It can be used when one want to make QMP calls to QEMU,
  * which may take a significant amount time.
  * It is to be acquired by an ao event callback.
@@ -374,42 +374,43 @@ struct libxl__ev_child {
  * It is to be acquired when adding/removing devices or making changes
  * to them when this is a slow operation and json_lock isn't appropriate.
  *
- * Possible states of libxl__ev_devlock:
+ * Possible states of libxl__ev_slowlock:
  *   Undefined
  *    Might contain anything.
  *  Idle
  *    Struct contents are defined enough to pass to any
- *    libxl__ev_devlock_* function.
+ *    libxl__ev_slowlock_* function.
  *    The struct does not contain references to any allocated private
  *    resources so can be thrown away.
  *  Active
  *    Waiting to get a lock.
  *    Needs to wait until the callback is called.
  *  LockAcquired
- *    libxl__ev_devlock_unlock will need to be called to release the lock
- *    and the resources of libxl__ev_devlock.
+ *    libxl__ev_slowlock_unlock will need to be called to release the lock
+ *    and the resources of libxl__ev_slowlock.
  *
- *  libxl__ev_devlock_init: Undefined/Idle -> Idle
- *  libxl__ev_devlock_lock: Idle -> Active
+ *  libxl__ev_*lock_init: Undefined/Idle -> Idle
+ *  libxl__ev_slowlock_lock: Idle -> Active
  *    May call callback synchronously.
- *  libxl__ev_devlock_unlock: LockAcquired/Idle -> Idle
+ *  libxl__ev_slowlock_unlock: LockAcquired/Idle -> Idle
  *  callback:     When called: Active -> LockAcquired (on error: Idle)
  *    The callback is only called once.
  */
-struct libxl__ev_devlock {
+struct libxl__ev_slowlock {
     /* filled by user */
     libxl__ao *ao;
     libxl_domid domid;
-    void (*callback)(libxl__egc *, libxl__ev_devlock *, int rc);
-    /* private to libxl__ev_devlock* */
+    void (*callback)(libxl__egc *, libxl__ev_slowlock *, int rc);
+    /* private to libxl__ev_slowlock* */
     libxl__ev_child child;
+    const char *userdata_userid;
     char *path; /* path of the lock file itself */
     int fd;
     bool held;
 };
-_hidden void libxl__ev_devlock_init(libxl__ev_devlock *);
-_hidden void libxl__ev_devlock_lock(libxl__egc *, libxl__ev_devlock *);
-_hidden void libxl__ev_devlock_unlock(libxl__gc *, libxl__ev_devlock *);
+_hidden void libxl__ev_devlock_init(libxl__ev_slowlock *);
+_hidden void libxl__ev_slowlock_lock(libxl__egc *, libxl__ev_slowlock *);
+_hidden void libxl__ev_slowlock_unlock(libxl__gc *, libxl__ev_slowlock *);
 
 /*
  * QMP asynchronous calls
@@ -2853,7 +2854,7 @@ struct libxl__multidev {
  *   unlock json config
  *
  * Or in case QEMU is the primary config, this pattern can be use:
- *   qmp_lock (libxl__ev_devlock)
+ *   qmp_lock (libxl__ev_devlock_init)
  *      lock json config (json_lock)
  *          read json config
  *          update in-memory json config with new entry, replacing
-- 
Anthony PERARD


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

[Xen-devel] [XEN PATCH for-4.13 v3 4/7] libxl: Introduce libxl__ev_slowlock_dispose

[Anthony PERARD]

Which allow to cancel the lock operation while it is in Active state.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
---

Notes:
    v2:
    - Renamed libxl__ev_qmplock_dispose to libxl__ev_slowlock_dispose
    - This new API was part of the patch "Introduce libxl__ev_qmplock" in v1.

 tools/libxl/libxl_internal.c | 6 ++++++
 tools/libxl/libxl_internal.h | 3 +++
 2 files changed, 9 insertions(+)

diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index 9520ac36149e..b2084157e4cd 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -765,6 +765,12 @@ void libxl__ev_slowlock_unlock(libxl__gc *gc, libxl__ev_slowlock *lock)
     ev_slowlock_init_internal(lock, lock->userdata_userid);
 }
 
+void libxl__ev_slowlock_dispose(libxl__gc *gc, libxl__ev_slowlock *lock)
+{
+    libxl__ev_child_kill_deregister(lock->ao, &lock->child, SIGKILL);
+    libxl__ev_slowlock_unlock(gc, lock);
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index a0f99252c39c..9b84dddd3b7d 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -393,6 +393,8 @@ struct libxl__ev_child {
  *  libxl__ev_slowlock_lock: Idle -> Active
  *    May call callback synchronously.
  *  libxl__ev_slowlock_unlock: LockAcquired/Idle -> Idle
+ *  libxl__ev_slowlock_dispose: Idle/Active/LockAcquired -> Idle
+ *    The callback will not be called anymore.
  *  callback:     When called: Active -> LockAcquired (on error: Idle)
  *    The callback is only called once.
  */
@@ -411,6 +413,7 @@ struct libxl__ev_slowlock {
 _hidden void libxl__ev_devlock_init(libxl__ev_slowlock *);
 _hidden void libxl__ev_slowlock_lock(libxl__egc *, libxl__ev_slowlock *);
 _hidden void libxl__ev_slowlock_unlock(libxl__gc *, libxl__ev_slowlock *);
+_hidden void libxl__ev_slowlock_dispose(libxl__gc *, libxl__ev_slowlock *);
 
 /*
  * QMP asynchronous calls
-- 
Anthony PERARD


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

[Xen-devel] [XEN PATCH for-4.13 v3 5/7] libxl: libxl__ev_qmp_send now takes an egc

[Anthony PERARD]

No functionnal changes.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
 tools/libxl/libxl_disk.c        |  6 +++---
 tools/libxl/libxl_dm.c          |  8 ++++----
 tools/libxl/libxl_dom_save.c    |  2 +-
 tools/libxl/libxl_dom_suspend.c |  2 +-
 tools/libxl/libxl_domain.c      |  8 ++++----
 tools/libxl/libxl_internal.h    |  2 +-
 tools/libxl/libxl_pci.c         |  8 ++++----
 tools/libxl/libxl_qmp.c         | 10 +++++-----
 tools/libxl/libxl_usb.c         | 28 ++++++++++++++++------------
 9 files changed, 39 insertions(+), 35 deletions(-)

diff --git a/tools/libxl/libxl_disk.c b/tools/libxl/libxl_disk.c
index 77ae3a59bfb6..64a66914240a 100644
--- a/tools/libxl/libxl_disk.c
+++ b/tools/libxl/libxl_disk.c
@@ -776,7 +776,7 @@ static void cdrom_insert_lock_acquired(libxl__egc *egc,
 
         QMP_PARAMETERS_SPRINTF(&args, "device", "ide-%i", devid);
         cis->qmp.callback = cdrom_insert_ejected;
-        rc = libxl__ev_qmp_send(gc, &cis->qmp, "eject", args);
+        rc = libxl__ev_qmp_send(egc, &cis->qmp, "eject", args);
         if (rc) goto out;
     } else {
         cdrom_insert_ejected(egc, &cis->qmp, NULL, 0); /* must be last */
@@ -884,7 +884,7 @@ static void cdrom_insert_ejected(libxl__egc *egc,
                                libxl_disk_format_to_string(disk->format),
                                disk->pdev_path);
         qmp->callback = cdrom_insert_addfd_cb;
-        rc = libxl__ev_qmp_send(gc, qmp, "add-fd", args);
+        rc = libxl__ev_qmp_send(egc, qmp, "add-fd", args);
         if (rc) goto out;
         has_callback = true;
     } else {
@@ -938,7 +938,7 @@ static void cdrom_insert_addfd_cb(libxl__egc *egc,
     libxl__qmp_param_add_string(gc, &args, "arg",
         libxl__qemu_disk_format_string(disk->format));
     qmp->callback = cdrom_insert_inserted;
-    rc = libxl__ev_qmp_send(gc, qmp, "change", args);
+    rc = libxl__ev_qmp_send(egc, qmp, "change", args);
 out:
     if (rc)
         cdrom_insert_done(egc, cis, rc); /* must be last */
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 8e0fb78bd2f3..dac1b8ddb88a 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -2649,7 +2649,7 @@ void libxl__spawn_local_dm(libxl__egc *egc, libxl__dm_spawn_state *dmss)
         dmss->qmp.callback = device_model_qmp_cb;
         dmss->qmp.domid = domid;
         dmss->qmp.payload_fd = -1;
-        rc = libxl__ev_qmp_send(gc, &dmss->qmp, "query-status", NULL);
+        rc = libxl__ev_qmp_send(egc, &dmss->qmp, "query-status", NULL);
         if (rc) goto out_close;
     }
 
@@ -2807,7 +2807,7 @@ static void device_model_spawn_outcome(libxl__egc *egc,
         dmss->qmp.domid = dmss->guest_domid;
         dmss->qmp.payload_fd = -1;
         dmss->qmp.callback = device_model_postconfig_chardev;
-        rc = libxl__ev_qmp_send(gc, &dmss->qmp, "query-chardev", NULL);
+        rc = libxl__ev_qmp_send(egc, &dmss->qmp, "query-chardev", NULL);
         if (rc) goto out;
         return;
     }
@@ -2879,7 +2879,7 @@ static void device_model_postconfig_chardev(libxl__egc *egc,
     }
 
     qmp->callback = device_model_postconfig_vnc;
-    rc = libxl__ev_qmp_send(gc, qmp, "query-vnc", NULL);
+    rc = libxl__ev_qmp_send(egc, qmp, "query-vnc", NULL);
     if (rc) goto out;
     return;
 
@@ -2939,7 +2939,7 @@ static void device_model_postconfig_vnc(libxl__egc *egc,
     if (vnc && vnc->passwd && vnc->passwd[0]) {
         qmp->callback = device_model_postconfig_vnc_passwd;
         libxl__qmp_param_add_string(gc, &args, "password", vnc->passwd);
-        rc = libxl__ev_qmp_send(gc, qmp, "change-vnc-password", args);
+        rc = libxl__ev_qmp_send(egc, qmp, "change-vnc-password", args);
         if (rc) goto out;
         return;
     }
diff --git a/tools/libxl/libxl_dom_save.c b/tools/libxl/libxl_dom_save.c
index e70aa1585976..65610e6055a7 100644
--- a/tools/libxl/libxl_dom_save.c
+++ b/tools/libxl/libxl_dom_save.c
@@ -226,7 +226,7 @@ static void domain_suspend_switch_qemu_xen_logdirty
     qmp->payload_fd = -1;
     qmp->callback = switch_qemu_xen_logdirty_done;
     libxl__qmp_param_add_bool(gc, &args, "enable", enable);
-    rc = libxl__ev_qmp_send(gc, qmp, "xen-set-global-dirty-log", args);
+    rc = libxl__ev_qmp_send(egc, qmp, "xen-set-global-dirty-log", args);
     if (rc) goto out;
 
     return;
diff --git a/tools/libxl/libxl_dom_suspend.c b/tools/libxl/libxl_dom_suspend.c
index 35ae337261ba..25d1571895f8 100644
--- a/tools/libxl/libxl_dom_suspend.c
+++ b/tools/libxl/libxl_dom_suspend.c
@@ -545,7 +545,7 @@ void libxl__dm_resume(libxl__egc *egc,
         qmp->domid = domid;
         qmp->callback = dm_resume_qmp_done;
         qmp->payload_fd = -1;
-        rc = libxl__ev_qmp_send(gc, qmp, "cont", NULL);
+        rc = libxl__ev_qmp_send(egc, qmp, "cont", NULL);
         if (rc) goto out;
         break;
     default:
diff --git a/tools/libxl/libxl_domain.c b/tools/libxl/libxl_domain.c
index 7ca7a224f9a4..571450177858 100644
--- a/tools/libxl/libxl_domain.c
+++ b/tools/libxl/libxl_domain.c
@@ -1600,7 +1600,7 @@ int libxl_set_vcpuonline(libxl_ctx *ctx, uint32_t domid,
                                              LIBXL_QMP_CMD_TIMEOUT * 1000);
             if (rc) goto out;
             qmp->callback = set_vcpuonline_qmp_cpus_queried;
-            rc = libxl__ev_qmp_send(gc, qmp, "query-cpus", NULL);
+            rc = libxl__ev_qmp_send(egc, qmp, "query-cpus", NULL);
             if (rc) goto out;
             return AO_INPROGRESS;
         default:
@@ -1666,7 +1666,7 @@ static void set_vcpuonline_qmp_add_cpu(libxl__egc *egc,
         if (libxl_bitmap_test(map, svos->index)) {
             qmp->callback = set_vcpuonline_qmp_add_cpu;
             libxl__qmp_param_add_integer(gc, &args, "id", svos->index);
-            rc = libxl__ev_qmp_send(gc, qmp, "cpu-add", args);
+            rc = libxl__ev_qmp_send(egc, qmp, "cpu-add", args);
             if (rc) goto out;
             return;
         }
@@ -1740,7 +1740,7 @@ static void domain_s3_resume(libxl__ao *ao, libxl__egc *egc, int domid)
             }
             break;
         case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
-            rc = libxl__ev_qmp_send(gc, qmp, "system_wakeup", NULL);
+            rc = libxl__ev_qmp_send(egc, qmp, "system_wakeup", NULL);
             if (rc) goto out;
             return;
         default:
@@ -1958,7 +1958,7 @@ static void retrieve_domain_configuration_lock_acquired(
         libxl_bitmap_alloc(CTX, &rdcs->qemuu_cpus,
                            d_config->b_info.max_vcpus);
         rdcs->qmp.callback = retrieve_domain_configuration_cpu_queried;
-        rc = libxl__ev_qmp_send(gc, &rdcs->qmp, "query-cpus", NULL);
+        rc = libxl__ev_qmp_send(egc, &rdcs->qmp, "query-cpus", NULL);
         if (rc) goto out;
         has_callback = true;
     }
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 9b84dddd3b7d..f95895eae17d 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -472,7 +472,7 @@ typedef void libxl__ev_qmp_callback(libxl__egc *egc, libxl__ev_qmp *ev,
                                     int rc);
 
 _hidden void libxl__ev_qmp_init(libxl__ev_qmp *ev);
-_hidden int libxl__ev_qmp_send(libxl__gc *gc, libxl__ev_qmp *ev,
+_hidden int libxl__ev_qmp_send(libxl__egc *egc, libxl__ev_qmp *ev,
                                const char *cmd, libxl__json_object *args);
 _hidden void libxl__ev_qmp_dispose(libxl__gc *gc, libxl__ev_qmp *ev);
 
diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
index 2ccab033b460..a66915542b98 100644
--- a/tools/libxl/libxl_pci.c
+++ b/tools/libxl/libxl_pci.c
@@ -1186,7 +1186,7 @@ static void pci_add_qmp_device_add(libxl__egc *egc, pci_add_state *pas)
     qmp->domid = domid;
     qmp->payload_fd = -1;
     qmp->callback = pci_add_qmp_device_add_cb;
-    rc = libxl__ev_qmp_send(gc, qmp, "device_add", args);
+    rc = libxl__ev_qmp_send(egc, qmp, "device_add", args);
     if (rc) goto out;
     return;
 
@@ -1205,7 +1205,7 @@ static void pci_add_qmp_device_add_cb(libxl__egc *egc,
     if (rc) goto out;
 
     qmp->callback = pci_add_qmp_query_pci_cb;
-    rc = libxl__ev_qmp_send(gc, qmp, "query-pci", NULL);
+    rc = libxl__ev_qmp_send(egc, qmp, "query-pci", NULL);
     if (rc) goto out;
     return;
 
@@ -2020,7 +2020,7 @@ static void pci_remove_qmp_device_del(libxl__egc *egc,
     QMP_PARAMETERS_SPRINTF(&args, "id", PCI_PT_QDEV_ID,
                            pcidev->bus, pcidev->dev, pcidev->func);
     prs->qmp.callback = pci_remove_qmp_device_del_cb;
-    rc = libxl__ev_qmp_send(gc, &prs->qmp, "device_del", args);
+    rc = libxl__ev_qmp_send(egc, &prs->qmp, "device_del", args);
     if (rc) goto out;
     return;
 
@@ -2059,7 +2059,7 @@ static void pci_remove_qmp_retry_timer_cb(libxl__egc *egc, libxl__ev_time *ev,
     pci_remove_state *prs = CONTAINER_OF(ev, *prs, retry_timer);
 
     prs->qmp.callback = pci_remove_qmp_query_cb;
-    rc = libxl__ev_qmp_send(gc, &prs->qmp, "query-pci", NULL);
+    rc = libxl__ev_qmp_send(egc, &prs->qmp, "query-pci", NULL);
     if (rc) goto out;
     return;
 
diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
index 9aabad74fabd..f0e0b50bd1c5 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -924,7 +924,7 @@ int libxl_qemu_monitor_command(libxl_ctx *ctx, uint32_t domid,
     qmcs->qmp.callback = qemu_monitor_command_done;
     qmcs->output = output;
     libxl__qmp_param_add_string(gc, &args, "command-line", command_line);
-    rc = libxl__ev_qmp_send(gc, &qmcs->qmp, "human-monitor-command", args);
+    rc = libxl__ev_qmp_send(egc, &qmcs->qmp, "human-monitor-command", args);
 out:
     if (rc) return AO_CREATE_FAIL(rc);
     return AO_INPROGRESS;
@@ -978,7 +978,7 @@ void libxl__qmp_suspend_save(libxl__egc *egc,
     ev->callback = dm_stopped;
     ev->payload_fd = -1;
 
-    rc = libxl__ev_qmp_send(gc, ev, "stop", NULL);
+    rc = libxl__ev_qmp_send(egc, ev, "stop", NULL);
     if (rc)
         goto error;
 
@@ -1007,7 +1007,7 @@ static void dm_stopped(libxl__egc *egc, libxl__ev_qmp *ev,
     }
 
     ev->callback = dm_state_fd_ready;
-    rc = libxl__ev_qmp_send(gc, ev, "add-fd", NULL);
+    rc = libxl__ev_qmp_send(egc, ev, "add-fd", NULL);
     if (rc)
         goto error;
 
@@ -1052,7 +1052,7 @@ static void dm_state_fd_ready(libxl__egc *egc, libxl__ev_qmp *ev,
     if (qmp_ev_qemu_compare_version(ev, 2, 11, 0) >= 0)
         libxl__qmp_param_add_bool(gc, &args, "live", dsps->live);
     QMP_PARAMETERS_SPRINTF(&args, "filename", "/dev/fdset/%d", fdset);
-    rc = libxl__ev_qmp_send(gc, ev, "xen-save-devices-state", args);
+    rc = libxl__ev_qmp_send(egc, ev, "xen-save-devices-state", args);
     if (rc)
         goto error;
 
@@ -1781,7 +1781,7 @@ void libxl__ev_qmp_init(libxl__ev_qmp *ev)
     ev->qemu_version.micro = -1;
 }
 
-int libxl__ev_qmp_send(libxl__gc *unused_gc, libxl__ev_qmp *ev,
+int libxl__ev_qmp_send(libxl__egc *egc, libxl__ev_qmp *ev,
                        const char *cmd, libxl__json_object *args)
     /* disconnected -> connecting
      * connected -> waiting_reply (with msg set)
diff --git a/tools/libxl/libxl_usb.c b/tools/libxl/libxl_usb.c
index 1fc7ccf41f86..da5e3708e6cf 100644
--- a/tools/libxl/libxl_usb.c
+++ b/tools/libxl/libxl_usb.c
@@ -349,9 +349,10 @@ static char *pvusb_get_device_type(libxl_usbctrl_type type)
  * - usb-ehci       (version=2), always 6 ports
  * - nec-usb-xhci   (version=3), up to 15 ports
  */
-static int libxl__device_usbctrl_add_hvm(libxl__gc *gc, libxl__ev_qmp *qmp,
+static int libxl__device_usbctrl_add_hvm(libxl__egc *egc, libxl__ev_qmp *qmp,
                                          libxl_device_usbctrl *usbctrl)
 {
+    EGC_GC;
     libxl__json_object *qmp_args = NULL;
 
     switch (usbctrl->version) {
@@ -378,26 +379,28 @@ static int libxl__device_usbctrl_add_hvm(libxl__gc *gc, libxl__ev_qmp *qmp,
     libxl__qmp_param_add_string(gc, &qmp_args, "id",
                                 GCSPRINTF("xenusb-%d", usbctrl->devid));
 
-    return libxl__ev_qmp_send(gc, qmp, "device_add", qmp_args);
+    return libxl__ev_qmp_send(egc, qmp, "device_add", qmp_args);
 }
 
 /* Send qmp commands to delete a usb controller in qemu.  */
-static int libxl__device_usbctrl_del_hvm(libxl__gc *gc,
+static int libxl__device_usbctrl_del_hvm(libxl__egc *egc,
                                          libxl__ev_qmp *qmp,
                                          int devid)
 {
+    EGC_GC;
     libxl__json_object *qmp_args = NULL;
 
     libxl__qmp_param_add_string(gc, &qmp_args,
                                 "id", GCSPRINTF("xenusb-%d", devid));
 
-    return libxl__ev_qmp_send(gc, qmp, "device_del", qmp_args);
+    return libxl__ev_qmp_send(egc, qmp, "device_del", qmp_args);
 }
 
 /* Send qmp commands to create a usb device in qemu. */
-static int libxl__device_usbdev_add_hvm(libxl__gc *gc, libxl__ev_qmp *qmp,
+static int libxl__device_usbdev_add_hvm(libxl__egc *egc, libxl__ev_qmp *qmp,
                                         libxl_device_usbdev *usbdev)
 {
+    EGC_GC;
     libxl__json_object *qmp_args = NULL;
 
     libxl__qmp_param_add_string(gc, &qmp_args, "id",
@@ -413,20 +416,21 @@ static int libxl__device_usbdev_add_hvm(libxl__gc *gc, libxl__ev_qmp *qmp,
     libxl__qmp_param_add_string(gc, &qmp_args, "hostaddr",
         GCSPRINTF("%d", usbdev->u.hostdev.hostaddr));
 
-    return libxl__ev_qmp_send(gc, qmp, "device_add", qmp_args);
+    return libxl__ev_qmp_send(egc, qmp, "device_add", qmp_args);
 }
 
 /* Send qmp commands to delete a usb device in qemu. */
-static int libxl__device_usbdev_del_hvm(libxl__gc *gc, libxl__ev_qmp *qmp,
+static int libxl__device_usbdev_del_hvm(libxl__egc *egc, libxl__ev_qmp *qmp,
                                         libxl_device_usbdev *usbdev)
 {
+    EGC_GC;
     libxl__json_object *qmp_args = NULL;
 
     libxl__qmp_param_add_string(gc, &qmp_args, "id",
         GCSPRINTF("xenusb-%d-%d", usbdev->u.hostdev.hostbus,
                   usbdev->u.hostdev.hostaddr));
 
-    return libxl__ev_qmp_send(gc, qmp, "device_del", qmp_args);
+    return libxl__ev_qmp_send(egc, qmp, "device_del", qmp_args);
 }
 
 static LIBXL_DEFINE_UPDATE_DEVID(usbctrl)
@@ -490,7 +494,7 @@ static void libxl__device_usbctrl_add(libxl__egc *egc, uint32_t domid,
         qmp->domid = domid;
         qmp->payload_fd = -1;
         qmp->callback = device_usbctrl_add_qmp_cb;
-        rc = libxl__device_usbctrl_add_hvm(gc, qmp, usbctrl);
+        rc = libxl__device_usbctrl_add_hvm(egc, qmp, usbctrl);
         if (rc) goto outrm;
         return;
     }
@@ -647,7 +651,7 @@ static void device_usbctrl_usbdevs_removed(libxl__egc *egc,
         qmp->domid = aodev->dev->domid;
         qmp->callback = device_usbctrl_remove_qmp_cb;
         qmp->payload_fd = -1;
-        rc = libxl__device_usbctrl_del_hvm(gc, qmp, aodev->dev->devid);
+        rc = libxl__device_usbctrl_del_hvm(egc, qmp, aodev->dev->devid);
         if (rc) goto out;
         return;
     }
@@ -1797,7 +1801,7 @@ static void libxl__device_usbdev_add(libxl__egc *egc, uint32_t domid,
         aodev->qmp.domid = domid;
         aodev->qmp.callback = device_usbdev_add_qmp_cb;
         aodev->qmp.payload_fd = -1;
-        rc = libxl__device_usbdev_add_hvm(gc, &aodev->qmp, usbdev);
+        rc = libxl__device_usbdev_add_hvm(egc, &aodev->qmp, usbdev);
         if (rc) {
             libxl__device_usbdev_remove_xenstore(gc, domid, usbdev,
                                              LIBXL_USBCTRL_TYPE_DEVICEMODEL);
@@ -1979,7 +1983,7 @@ static void libxl__device_usbdev_remove(libxl__egc *egc, uint32_t domid,
         aodev->qmp.domid = domid;
         aodev->qmp.callback = device_usbdev_remove_qmp_cb;
         aodev->qmp.payload_fd = -1;
-        rc = libxl__device_usbdev_del_hvm(gc, &aodev->qmp, usbdev);
+        rc = libxl__device_usbdev_del_hvm(egc, &aodev->qmp, usbdev);
         if (rc) {
             libxl__device_usbdev_add_xenstore(gc, domid, usbdev,
                                               LIBXL_USBCTRL_TYPE_DEVICEMODEL,
-- 
Anthony PERARD


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

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

[Anthony PERARD]

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

[Xen-devel] [XEN PATCH for-4.13 v3 7/7] libxl_qmp: Have a lock for QMP socket access

[Anthony PERARD]

This patch workaround the fact that it's not possible to connect
multiple time to a single QMP socket. QEMU listen on the socket with
a backlog value of 1, which mean that on Linux when concurrent thread
call connect() on the socket, they get EAGAIN.

Background:
    This happens when attempting to create a guest with multiple
    pci devices passthrough, libxl creates one connection per device to
    attach and execute connect() on all at once before any single
    connection has finished.

To work around this, we use a new lock.

Reported-by: Sander Eikelenboom <linux@eikelenboom.it>
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---

Notes:
    v3:
    - Use new libxl_ev_immediate in qmp_ev_connect's callbacks error path.
    - Add `lock' state into the internal state table.
      And add comments about state changes to the new functions.
    
    v2:
    - Handle error path

 tools/libxl/libxl_internal.c |   5 ++
 tools/libxl/libxl_internal.h |   9 +++
 tools/libxl/libxl_qmp.c      | 118 +++++++++++++++++++++++++++--------
 3 files changed, 107 insertions(+), 25 deletions(-)

diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index b2084157e4cd..ba5637358e7c 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -590,6 +590,11 @@ void libxl__ev_devlock_init(libxl__ev_slowlock *lock)
     ev_slowlock_init_internal(lock, "libxl-device-changes-lock");
 }
 
+void libxl__ev_qmplock_init(libxl__ev_slowlock *lock)
+{
+    ev_slowlock_init_internal(lock, "qmp-socket-lock");
+}
+
 static void ev_lock_prepare_fork(libxl__egc *egc, libxl__ev_slowlock *lock);
 static void ev_lock_child_callback(libxl__egc *egc, libxl__ev_child *child,
                                    pid_t pid, int status);
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 400752a7f8fe..8988831ae5f7 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -386,6 +386,9 @@ void libxl__ev_immediate_register(libxl__egc *, libxl__ev_immediate *);
  * which may take a significant amount time.
  * It is to be acquired by an ao event callback.
  *
+ * If libxl__ev_devlock is needed, it should be acquired while every
+ * libxl__ev_qmp are Idle for the current domain.
+ *
  * It is to be acquired when adding/removing devices or making changes
  * to them when this is a slow operation and json_lock isn't appropriate.
  *
@@ -426,6 +429,7 @@ struct libxl__ev_slowlock {
     bool held;
 };
 _hidden void libxl__ev_devlock_init(libxl__ev_slowlock *);
+_hidden void libxl__ev_qmplock_init(libxl__ev_slowlock *);
 _hidden void libxl__ev_slowlock_lock(libxl__egc *, libxl__ev_slowlock *);
 _hidden void libxl__ev_slowlock_unlock(libxl__gc *, libxl__ev_slowlock *);
 _hidden void libxl__ev_slowlock_dispose(libxl__gc *, libxl__ev_slowlock *);
@@ -494,6 +498,8 @@ _hidden void libxl__ev_qmp_dispose(libxl__gc *gc, libxl__ev_qmp *ev);
 typedef enum {
     /* initial state */
     qmp_state_disconnected = 1,
+    /* waiting for lock */
+    qmp_state_waiting_lock,
     /* connected to QMP socket, waiting for greeting message */
     qmp_state_connecting,
     /* qmp_capabilities command sent, waiting for reply */
@@ -527,6 +533,9 @@ struct libxl__ev_qmp {
     libxl__carefd *cfd;
     libxl__ev_fd efd;
     libxl__qmp_state state;
+    libxl__ev_slowlock lock;
+    libxl__ev_immediate ei;
+    int rc;
     int id;
     int next_id;        /* next id to use */
     /* receive buffer */
diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
index f0e0b50bd1c5..c8eac2588a89 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -1082,16 +1082,17 @@ static void dm_state_saved(libxl__egc *egc, libxl__ev_qmp *ev,
 /*
  * Possible internal state compared to qmp_state:
  *
- * qmp_state     External   cfd    efd     id     rx_buf* tx_buf* msg*
- * disconnected   Idle       NULL   Idle    reset  free    free    free
- * connecting     Active     open   IN      reset  used    free    set
- * cap.neg        Active     open   IN|OUT  sent   used    cap_neg set
- * cap.neg        Active     open   IN      sent   used    free    set
- * connected      Connected  open   IN      any    used    free    free
- * waiting_reply  Active     open   IN|OUT  sent   used    free    set
- * waiting_reply  Active     open   IN|OUT  sent   used    user's  free
- * waiting_reply  Active     open   IN      sent   used    free    free
- * broken[1]      none[2]    any    Active  any    any     any     any
+ * qmp_state     External   cfd    efd     id     rx_buf* tx_buf* msg* lock
+ * disconnected   Idle       NULL   Idle    reset  free    free    free Idle
+ * waiting_lock   Active     open   Idle    reset  used    free    set  Active
+ * connecting     Active     open   IN      reset  used    free    set  Acquired
+ * cap.neg        Active     open   IN|OUT  sent   used    cap_neg set  Acquired
+ * cap.neg        Active     open   IN      sent   used    free    set  Acquired
+ * connected      Connected  open   IN      any    used    free    free Acquired
+ * waiting_reply  Active     open   IN|OUT  sent   used    free    set  Acquired
+ * waiting_reply  Active     open   IN|OUT  sent   used    user's  free Acquired
+ * waiting_reply  Active     open   IN      sent   used    free    free Acquired
+ * broken[1]      none[2]    any    Active  any    any     any     any  any
  *
  * [1] When an internal function return an error, it can leave ev_qmp in a
  * `broken` state but only if the caller is another internal function.
@@ -1118,7 +1119,8 @@ static void dm_state_saved(libxl__egc *egc, libxl__ev_qmp *ev,
  *     msg_id           0     id assoctiated with the command in `msg`
  *
  * - Allowed internal state transition:
- * disconnected                     -> connecting
+ * disconnected                     -> waiting_lock
+ * waiting_lock                     -> connecting
  * connection                       -> capability_negotiation
  * capability_negotiation/connected -> waiting_reply
  * waiting_reply                    -> connected
@@ -1153,6 +1155,10 @@ static void qmp_ev_ensure_reading_writing(libxl__gc *gc, libxl__ev_qmp *ev)
 {
     short events = POLLIN;
 
+    if (ev->state == qmp_state_waiting_lock)
+        /* We can't modifie the efd yet, as it isn't registered. */
+        return;
+
     if (ev->tx_buf)
         events |= POLLOUT;
     else if ((ev->state == qmp_state_waiting_reply) && ev->msg)
@@ -1168,9 +1174,12 @@ static void qmp_ev_set_state(libxl__gc *gc, libxl__ev_qmp *ev,
     switch (new_state) {
     case qmp_state_disconnected:
         break;
-    case qmp_state_connecting:
+    case qmp_state_waiting_lock:
         assert(ev->state == qmp_state_disconnected);
         break;
+    case qmp_state_connecting:
+        assert(ev->state == qmp_state_waiting_lock);
+        break;
     case qmp_state_capability_negotiation:
         assert(ev->state == qmp_state_connecting);
         break;
@@ -1231,20 +1240,22 @@ static int qmp_error_class_to_libxl_error_code(libxl__gc *gc,
 
 /* Setup connection */
 
-static int qmp_ev_connect(libxl__gc *gc, libxl__ev_qmp *ev)
-    /* disconnected -> connecting but with `msg` free
+static void qmp_ev_lock_aquired(libxl__egc *, libxl__ev_slowlock *,
+                                int rc);
+static void lock_error_callback(libxl__egc *, libxl__ev_immediate *);
+
+static int qmp_ev_connect(libxl__egc *egc, libxl__ev_qmp *ev)
+    /* disconnected -> waiting_lock/connecting but with `msg` free
      * on error: broken */
 {
+    EGC_GC;
     int fd;
-    int rc, r;
-    struct sockaddr_un un;
-    const char *qmp_socket_path;
-
-    assert(ev->state == qmp_state_disconnected);
+    int rc;
 
-    qmp_socket_path = libxl__qemu_qmp_path(gc, ev->domid);
+    /* Convenience aliases */
+    libxl__ev_slowlock *lock = &ev->lock;
 
-    LOGD(DEBUG, ev->domid, "Connecting to %s", qmp_socket_path);
+    assert(ev->state == qmp_state_disconnected);
 
     libxl__carefd_begin();
     fd = socket(AF_UNIX, SOCK_STREAM, 0);
@@ -1258,6 +1269,36 @@ static int qmp_ev_connect(libxl__gc *gc, libxl__ev_qmp *ev)
     if (rc)
         goto out;
 
+    qmp_ev_set_state(gc, ev, qmp_state_waiting_lock);
+
+    lock->ao = ev->ao;
+    lock->domid = ev->domid;
+    lock->callback = qmp_ev_lock_aquired;
+    libxl__ev_slowlock_lock(egc, &ev->lock);
+
+    return 0;
+
+out:
+    return rc;
+}
+
+static void qmp_ev_lock_aquired(libxl__egc *egc, libxl__ev_slowlock *lock,
+                                int rc)
+    /* waiting_lock (with `lock' Acquired) -> connecting
+     * on error: broken */
+{
+    libxl__ev_qmp *ev = CONTAINER_OF(lock, *ev, lock);
+    EGC_GC;
+    const char *qmp_socket_path;
+    struct sockaddr_un un;
+    int r;
+
+    if (rc) goto out;
+
+    qmp_socket_path = libxl__qemu_qmp_path(gc, ev->domid);
+
+    LOGD(DEBUG, ev->domid, "Connecting to %s", qmp_socket_path);
+
     rc = libxl__prepare_sockaddr_un(gc, &un, qmp_socket_path,
                                     "QMP socket");
     if (rc)
@@ -1279,10 +1320,33 @@ static int qmp_ev_connect(libxl__gc *gc, libxl__ev_qmp *ev)
 
     qmp_ev_set_state(gc, ev, qmp_state_connecting);
 
-    return 0;
+    return;
 
 out:
-    return rc;
+    /* An error occurred and we need to let the caller know.  At this
+     * point, we can only do so via the callback. Unfortunately, the
+     * callback of libxl__ev_slowlock_lock() might be called synchronously,
+     * but libxl__ev_qmp_send() promise that it will not call the callback
+     * synchronously. So we have to arrange to call the callback
+     * asynchronously. */
+    ev->rc = rc;
+    ev->ei.callback = lock_error_callback;
+    libxl__ev_immediate_register(egc, &ev->ei);
+}
+
+static void lock_error_callback(libxl__egc *egc, libxl__ev_immediate *ei)
+    /* broken -> disconnected */
+{
+    EGC_GC;
+    libxl__ev_qmp *ev = CONTAINER_OF(ei, *ev, ei);
+
+    int rc = ev->rc;
+
+    /* On error, deallocate all private resources */
+    libxl__ev_qmp_dispose(gc, ev);
+
+    /* And tell libxl__ev_qmp user about the error */
+    ev->callback(egc, ev, NULL, rc); /* must be last */
 }
 
 /* QMP FD callbacks */
@@ -1779,11 +1843,14 @@ void libxl__ev_qmp_init(libxl__ev_qmp *ev)
     ev->qemu_version.major = -1;
     ev->qemu_version.minor = -1;
     ev->qemu_version.micro = -1;
+
+    libxl__ev_qmplock_init(&ev->lock);
+    ev->rc = 0;
 }
 
 int libxl__ev_qmp_send(libxl__egc *egc, libxl__ev_qmp *ev,
                        const char *cmd, libxl__json_object *args)
-    /* disconnected -> connecting
+    /* disconnected -> waiting_lock/connecting
      * connected -> waiting_reply (with msg set)
      * on error: disconnected */
 {
@@ -1798,7 +1865,7 @@ int libxl__ev_qmp_send(libxl__egc *egc, libxl__ev_qmp *ev,
 
     /* Connect to QEMU if not already connected */
     if (ev->state == qmp_state_disconnected) {
-        rc = qmp_ev_connect(gc, ev);
+        rc = qmp_ev_connect(egc, ev);
         if (rc)
             goto error;
     }
@@ -1830,6 +1897,7 @@ void libxl__ev_qmp_dispose(libxl__gc *gc, libxl__ev_qmp *ev)
 
     libxl__ev_fd_deregister(gc, &ev->efd);
     libxl__carefd_close(ev->cfd);
+    libxl__ev_slowlock_dispose(gc, &ev->lock);
 
     libxl__ev_qmp_init(ev);
 }
-- 
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

[Ian Jackson]

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 7/7] libxl_qmp: Have a lock for QMP socket access

[Ian Jackson]

Anthony PERARD writes ("[XEN PATCH for-4.13 v3 7/7] libxl_qmp: Have a lock for QMP socket access"):
> This patch workaround the fact that it's not possible to connect
> multiple time to a single QMP socket. QEMU listen on the socket with
> a backlog value of 1, which mean that on Linux when concurrent thread
> call connect() on the socket, they get EAGAIN.
...
> +    if (ev->state == qmp_state_waiting_lock)
> +        /* We can't modifie the efd yet, as it isn't registered. */
                       ^
                       modify

Nevertheless,

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

:-).

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

[Anthony PERARD]

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

[Ian Jackson]

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

[Anthony PERARD]

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

[Ian Jackson]

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

Re: [Xen-devel] [XEN PATCH for-4.13 v3 7/7] libxl_qmp: Have a lock for QMP socket access

[Wei Liu]

On Mon, Nov 18, 2019 at 05:30:36PM +0000, Ian Jackson wrote:
> Anthony PERARD writes ("[XEN PATCH for-4.13 v3 7/7] libxl_qmp: Have a lock for QMP socket access"):
> > This patch workaround the fact that it's not possible to connect
> > multiple time to a single QMP socket. QEMU listen on the socket with
> > a backlog value of 1, which mean that on Linux when concurrent thread
> > call connect() on the socket, they get EAGAIN.
> ...
> > +    if (ev->state == qmp_state_waiting_lock)
> > +        /* We can't modifie the efd yet, as it isn't registered. */
>                        ^
>                        modify
> 

Fixed up this typo and whole series pushed.

Wei.

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