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

[Xen-devel] [RFC XEN PATCH for-4.13 0/4] 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-v1

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.

Alternatively to this craziness, it might be possible to increase the `backlog'
value by having libxl opening the QMP socket on behalf of QEMU. But this is
only possible with a recent version of QEMU (2.12 or newer, released in Apr
2018, or qemu-xen-4.12 or newer). It would involve to discover QEMU's
capability before we start the DM, which libxl isn't capable yet.

Cheers,

Anthony PERARD (4):
  libxl: Introduce libxl__ev_child_kill
  libxl: Introduce libxl__ev_qmplock
  libxl: libxl__ev_qmp_send now takes an egc
  libxl_qmp: Have a lock for QMP socket access

 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_event.c       |  3 +-
 tools/libxl/libxl_fork.c        | 55 ++++++++++++++++++++++++
 tools/libxl/libxl_internal.c    | 31 +++++++++++++-
 tools/libxl/libxl_internal.h    | 53 +++++++++++++++++------
 tools/libxl/libxl_pci.c         |  8 ++--
 tools/libxl/libxl_qmp.c         | 75 +++++++++++++++++++++++++--------
 tools/libxl/libxl_usb.c         | 28 ++++++------
 12 files changed, 219 insertions(+), 60 deletions(-)

-- 
Anthony PERARD

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

[Xen-devel] [RFC XEN PATCH for-4.13 1/4] libxl: Introduce libxl__ev_child_kill

[Anthony PERARD]

Allow to kill a child and deregister the callback associated with it.

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

libxl__ev_child_kill() 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>
---
 tools/libxl/libxl_event.c    |  3 +-
 tools/libxl/libxl_fork.c     | 55 ++++++++++++++++++++++++++++++++++++
 tools/libxl/libxl_internal.h |  8 ++++++
 3 files changed, 65 insertions(+), 1 deletion(-)

diff --git a/tools/libxl/libxl_event.c b/tools/libxl/libxl_event.c
index 0370b6acdd1c..f57c16da1fd9 100644
--- a/tools/libxl/libxl_event.c
+++ b/tools/libxl/libxl_event.c
@@ -1891,7 +1891,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..d99d40107f71 100644
--- a/tools/libxl/libxl_fork.c
+++ b/tools/libxl/libxl_fork.c
@@ -678,6 +678,61 @@ 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);
+
+/*
+ * Allow to send a SIGKILL signal to a child and deregister the death
+ * callback.
+ * state: Active/Idle -> Idle
+ */
+void libxl__ev_child_kill(libxl__ao *ao, libxl__ev_child *ch)
+{
+    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, SIGKILL);
+    if (r)
+        LOGE(ERROR, "failed to kill child [%ld]",
+             (unsigned long)pid);
+}
+
+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;
+
+    if (status) {
+        libxl_report_child_exitstatus(CTX, XTL_ERROR,
+                                      "killed fork (dying as expected)",
+                                      pid, status);
+    } else {
+        LOG(DEBUG, "killed child exit cleanly, unexpected");
+    }
+    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 d2d5af746b50..5823890703ad 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -727,6 +727,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{               \
@@ -1168,6 +1169,13 @@ static inline int libxl__ev_child_inuse(const libxl__ev_child *childw_out)
  * message>". */
 _hidden int libxl__ev_child_xenstore_reopen(libxl__gc *gc, const char *what);
 
+/*
+ * Allow to send a SIGKILL signal to a child and deregister the death
+ * callback.
+ * state: Active/Idle -> Idle
+ */
+_hidden void libxl__ev_child_kill(libxl__ao *ao, libxl__ev_child *ch);
+
 
 /*
  * Other event-handling support provided by the libxl event core to
-- 
Anthony PERARD

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

[Xen-devel] [RFC XEN PATCH for-4.13 2/4] libxl: Introduce libxl__ev_qmplock

[Anthony PERARD]

This lock will be used to prevent concurrent access the QEMU's QMP
socket. It is based on libxl__ev_devlock implementation and have the
same properties.

There is one addition to the ev_devlock API,
libxl__ev_qmplock_dispose, which allow to cancel the lock operation
while it is in Active state.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 tools/libxl/libxl_internal.c | 31 ++++++++++++++++++++++++++++++-
 tools/libxl/libxl_internal.h | 14 ++++++++++++++
 2 files changed, 44 insertions(+), 1 deletion(-)

diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index 0750b69cba61..afbb01c5722f 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -583,17 +583,25 @@ void libxl__ev_devlock_init(libxl__ev_devlock *lock)
     lock->held = false;
 }
 
+static void ev_lock_lock(libxl__egc *egc, libxl__ev_devlock *lock,
+                         const char *userdata_userid);
 static void ev_lock_prepare_fork(libxl__egc *egc, libxl__ev_devlock *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)
+{
+    ev_lock_lock(egc, lock, "libxl-device-changes-lock");
+}
+
+static void ev_lock_lock(libxl__egc *egc, libxl__ev_devlock *lock,
+                         const char *userdata_userid)
 {
     STATE_AO_GC(lock->ao);
     const char *lockfile;
 
     lockfile = libxl__userdata_path(gc, lock->domid,
-                                    "libxl-device-changes-lock", "l");
+                                    userdata_userid, "l");
     if (!lockfile) goto out;
     lock->path = libxl__strdup(NOGC, lockfile);
 
@@ -757,6 +765,27 @@ void libxl__ev_devlock_unlock(libxl__gc *gc, libxl__ev_devlock *lock)
     libxl__ev_devlock_init(lock);
 }
 
+void libxl__ev_qmplock_init(libxl__ev_qmplock *lock)
+{
+    libxl__ev_devlock_init(lock);
+}
+
+void libxl__ev_qmplock_lock(libxl__egc *egc, libxl__ev_qmplock *lock)
+{
+    ev_lock_lock(egc, lock, "qmp-socket-lock");
+}
+
+void libxl__ev_qmplock_unlock(libxl__gc *gc, libxl__ev_qmplock *lock)
+{
+    libxl__ev_devlock_unlock(gc, lock);
+}
+
+void libxl__ev_qmplock_dispose(libxl__gc *gc, libxl__ev_qmplock *lock)
+{
+    libxl__ev_child_kill(lock->ao, &lock->child);
+    libxl__ev_devlock_unlock(gc, lock);
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 5823890703ad..115c79d034d4 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -197,6 +197,7 @@ 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_devlock libxl__ev_qmplock;
 typedef struct libxl__dm_resume_state libxl__dm_resume_state;
 typedef struct libxl__ao_device libxl__ao_device;
 typedef struct libxl__multidev libxl__multidev;
@@ -4725,6 +4726,19 @@ _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 *);
 
+/* libxl__ev_qmplock
+ *
+ * See "Lock for device hotplug, qmp_lock." as it is similar but is used
+ * to regulate access the QEMU's QMP socket.
+ *
+ * libxl__ev_qmplock_dispose: Idle/Active/LockAcquired -> Idle
+ *   The callback will not be called anymore.
+ */
+_hidden void libxl__ev_qmplock_init(libxl__ev_qmplock *);
+_hidden void libxl__ev_qmplock_lock(libxl__egc *, libxl__ev_qmplock *);
+_hidden void libxl__ev_qmplock_unlock(libxl__gc *, libxl__ev_qmplock *);
+_hidden void libxl__ev_qmplock_dispose(libxl__gc *, libxl__ev_qmplock *);
+
 /* 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] [RFC XEN PATCH for-4.13 3/4] libxl: libxl__ev_qmp_send now takes an egc

[Anthony PERARD]

No functionnal changes.

Signed-off-by: Anthony PERARD <anthony.perard@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 733ad284c866..7cbee3953320 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 c00356a2f16a..47cf6dda7ec1 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -2650,7 +2650,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;
     }
 
@@ -2808,7 +2808,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;
     }
@@ -2880,7 +2880,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;
 
@@ -2940,7 +2940,7 @@ static void device_model_postconfig_vnc(libxl__egc *egc,
     if (vnc && vnc->passwd) {
         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 248dbc33e384..940ac334f4b1 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 9d0eb5aed11d..621df32c5dd8 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 115c79d034d4..ef6655587b79 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -421,7 +421,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 b5444d15523a..0dc4a83e166d 100644
--- a/tools/libxl/libxl_pci.c
+++ b/tools/libxl/libxl_pci.c
@@ -1163,7 +1163,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;
 
@@ -1182,7 +1182,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;
 
@@ -1997,7 +1997,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;
 
@@ -2036,7 +2036,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] [RFC XEN PATCH for-4.13 4/4] libxl_qmp: Have a lock for QMP socket access

[Anthony PERARD]

FIXME: The case where something failed when trying to acquired the
   lock isn't handled yet.

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.

To work around this, we use a new lock.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 tools/libxl/libxl_internal.h | 29 ++++++++++------
 tools/libxl/libxl_qmp.c      | 65 +++++++++++++++++++++++++++++-------
 2 files changed, 71 insertions(+), 23 deletions(-)

diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index ef6655587b79..d650188586e9 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -364,6 +364,18 @@ struct libxl__ev_child {
     LIBXL_LIST_ENTRY(struct libxl__ev_child) entry;
 };
 
+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;
+};
+
 /*
  * QMP asynchronous calls
  *
@@ -428,6 +440,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 */
@@ -461,6 +475,7 @@ struct libxl__ev_qmp {
     libxl__carefd *cfd;
     libxl__ev_fd efd;
     libxl__qmp_state state;
+    libxl__ev_qmplock lock;
     int id;
     int next_id;        /* next id to use */
     /* receive buffer */
@@ -4686,6 +4701,9 @@ static inline const char *libxl__qemu_qmp_path(libxl__gc *gc, int domid)
  * 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.
  *
@@ -4711,17 +4729,6 @@ static inline const char *libxl__qemu_qmp_path(libxl__gc *gc, int domid)
  *  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 *);
diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
index f0e0b50bd1c5..1ac50a95a42d 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -1084,6 +1084,7 @@ static void dm_state_saved(libxl__egc *egc, libxl__ev_qmp *ev,
  *
  * qmp_state     External   cfd    efd     id     rx_buf* tx_buf* msg*
  * disconnected   Idle       NULL   Idle    reset  free    free    free
+ * waiting_lock   Active     open   Idle    reset  used    free    set
  * 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
@@ -1153,6 +1154,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 +1173,13 @@ 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_disconnected ||
+               ev->state == qmp_state_waiting_lock);
+        break;
     case qmp_state_capability_negotiation:
         assert(ev->state == qmp_state_connecting);
         break;
@@ -1231,20 +1240,20 @@ 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)
+static void qmp_ev_lock_aquired(libxl__egc *, libxl__ev_qmplock *, int rc);
+
+static int qmp_ev_connect(libxl__egc *egc, libxl__ev_qmp *ev)
     /* disconnected -> 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_qmplock *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 +1267,35 @@ 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_qmplock_lock(egc, &ev->lock);
+
+    return 0;
+
+out:
+    return rc;
+}
+
+static void qmp_ev_lock_aquired(libxl__egc *egc, libxl__ev_qmplock *lock,
+                                int rc)
+{
+    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 +1317,10 @@ 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;
+    LOGD(ERROR, ev->domid, "connect failed");
 }
 
 /* QMP FD callbacks */
@@ -1779,6 +1817,8 @@ 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);
 }
 
 int libxl__ev_qmp_send(libxl__egc *egc, libxl__ev_qmp *ev,
@@ -1798,7 +1838,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 +1870,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_qmplock_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] [RFC XEN PATCH for-4.13 0/4] Fix: libxl workaround, multiple connection to single QMP socket

[Sander Eikelenboom]

On 25/10/2019 19:05, Anthony PERARD wrote:
> Patch series available in this git branch:
> https://xenbits.xen.org/git-http/people/aperard/xen-unstable.git br.fix-ev_qmp-multi-connect-v1
> 
> 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.

Hi Anthony,

Just tested with the patch series and it fixes my issue with starting a
guest with several PCI passthrough devices.

Thanks,

Sander


> 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.
> 
> Alternatively to this craziness, it might be possible to increase the `backlog'
> value by having libxl opening the QMP socket on behalf of QEMU. But this is
> only possible with a recent version of QEMU (2.12 or newer, released in Apr
> 2018, or qemu-xen-4.12 or newer). It would involve to discover QEMU's
> capability before we start the DM, which libxl isn't capable yet.
> 
> Cheers,
> 
> Anthony PERARD (4):
>   libxl: Introduce libxl__ev_child_kill
>   libxl: Introduce libxl__ev_qmplock
>   libxl: libxl__ev_qmp_send now takes an egc
>   libxl_qmp: Have a lock for QMP socket access
> 
>  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_event.c       |  3 +-
>  tools/libxl/libxl_fork.c        | 55 ++++++++++++++++++++++++
>  tools/libxl/libxl_internal.c    | 31 +++++++++++++-
>  tools/libxl/libxl_internal.h    | 53 +++++++++++++++++------
>  tools/libxl/libxl_pci.c         |  8 ++--
>  tools/libxl/libxl_qmp.c         | 75 +++++++++++++++++++++++++--------
>  tools/libxl/libxl_usb.c         | 28 ++++++------
>  12 files changed, 219 insertions(+), 60 deletions(-)
> 


-- 

Met vriendelijke groet,

Sander Eikelenboom
mailto:Sander@Eikelenboom.IT

Eikelenboom IT Services
Kaapseweg 70
5642 HK Eindhoven
M: 06-14387484

PGP public key for sander@eikelenboom.it:
key id: 0xC4B99EEDECF2AE69
fingerprint: 07BB B819 FF93 E54D 5F5C  0BDE C4B9 9EED ECF2 AE69

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

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

[Ian Jackson]

Hi.  Thanks for tackling this swamp.  All very unfortunate.

Anthony PERARD writes ("[RFC XEN PATCH for-4.13 0/4] Fix: libxl workaround, multiple connection to single QMP socket"):
> Alternatively to this craziness, it might be possible to increase
> the `backlog' value by having libxl opening the QMP socket on behalf
> of QEMU. But this is only possible with a recent version of QEMU
> (2.12 or newer, released in Apr 2018, or qemu-xen-4.12 or newer). It
> would involve to discover QEMU's capability before we start the DM,
> which libxl isn't capable yet.

I have an ancient unapplied patch somewhere which runs qemu --help
and greps the output.  If you would like, I can dig it out.

But one problem with that approach is this: without that feature in
qemu, what would we do ?  Live with the bug where domain creation
fails ?  Bodge it by serialising within domain create (awkwardating
the code) ?

I have some other suggestions which ought to be considered:


1. Send a patch to qemu upstream to allow specifying the socket listen
queue.

1(a) Expect distros to apply that patch to older qemus, if they ship
older qemus.  Have libxl unconditionally specify that argument.

1(b) grep the help output (as I propose above) and if the patch is not
present, use LD_PRELOAD to wrap listen(2).


2. Send a patch to qemu upstream to change the fixed queue length from
1 to 10000.  Expect distros to apply that patch to older qemus (even,
perhaps, if it is not accepted upstream!)  Change libxl to detect
EAGAIN from qmp connect() and print a message explaining what patch is
missing.


Since you have provided an implementation of the fork/lock strategy,
I'll now go and do a detailed review of that.

Thanks,
Ian.

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

Re: [Xen-devel] [RFC XEN PATCH for-4.13 1/4] libxl: Introduce libxl__ev_child_kill

[Ian Jackson]

Hi.  Thanks.  The code here looks by and large good to me but I think
the docs and maybe the log messages need improvement.

Anthony PERARD writes ("[RFC XEN PATCH for-4.13 1/4] libxl: Introduce libxl__ev_child_kill"):
> Allow to kill a child and deregister the callback associated with it.

Did you read the doc comment above libxl__ev_child_fork, in
libxl_internal.h near line 1160 ?  The user of libxl__ev_child is
already permitted to kill the child.

In this patch are adding a layer to make this more convenient, and in
particular to let a libxl__ev_child user transfer responsibility for
reaping the child from its own application logic into the ao system.

Some more API documentation to make this much more explicit would be
good - ie the main doc comment the facility needs to discuss it:
 | * It is not possible to "deregister" the child death event source
^ this is no longer true after your patch; indeed that's the point.

So perhaps

> +void libxl__ev_child_kill(libxl__ao *ao, libxl__ev_child *ch)

should be called
   libxl__ev_child_reattach_to_ao
or
   libxl__ev_child_kill_deregister
or something, and maybe it should take a signal number ?

> +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;
> +
> +    if (status) {
> +        libxl_report_child_exitstatus(CTX, XTL_ERROR,
> +                                      "killed fork (dying as expected)",
> +                                      pid, status);
> +    } else {
> +        LOG(DEBUG, "killed child exit cleanly, unexpected");

I don't think this is entirely unexpected.  Maybe the child was just
exiting at the point where libxl__ev_child_kill was called.

And, please check log the actual whole exit status.  "status" is a
wait status.  We want to know what signal it died from, whether it
core dumped, the exit status, etc.  Probably, you should call
libxl_report_child_exitstatus.

> @@ -1891,7 +1891,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;
>  }

I wonder if this should gain a new debug message.  If the child gets
lost or stuck for some reason, it will otherwise require searching the
past log to find out why the ao doesn't return.

Ian.

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

Re: [Xen-devel] [RFC XEN PATCH for-4.13 3/4] libxl: libxl__ev_qmp_send now takes an egc

[Ian Jackson]

Anthony PERARD writes ("[RFC XEN PATCH for-4.13 3/4] libxl: libxl__ev_qmp_send now takes an egc"):
> No functionnal changes.

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

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

Re: [Xen-devel] [RFC XEN PATCH for-4.13 2/4] libxl: Introduce libxl__ev_qmplock

[Ian Jackson]

Thanks.  I approve of the general approach, and the code reuse, but I
have some qualms about the resulting layering structure and the
boilerplate wrappers.  I have some suggestions for how this might look
better.

Anthony PERARD writes ("[RFC XEN PATCH for-4.13 2/4] libxl: Introduce libxl__ev_qmplock"):
> This lock will be used to prevent concurrent access the QEMU's QMP
> socket. It is based on libxl__ev_devlock implementation and have the
> same properties.
...
> +void libxl__ev_qmplock_init(libxl__ev_qmplock *lock)
> +{
> +    libxl__ev_devlock_init(lock);
> +}
> +
> +void libxl__ev_qmplock_lock(libxl__egc *egc, libxl__ev_qmplock *lock)
> +{
> +    ev_lock_lock(egc, lock, "qmp-socket-lock");
> +}

This produces a lot of rather pointless functions.  Also the layering
is anomalous: one of these locks is primary and most of the calls for
the other are implemented in terms of the other.

One possible alternative approach would be as follows:

1. Rename devlock to slowlock everywhere.  Expect everyone including
   qmp to call libxl__ev_slowlock_*.

2. Perhaps, put const char *userdata_userid in the lock structure.
   Have it set by libxl__ev_slowlock_init rather than by _lock.  (This
   centralises things a bit and may reduce duplication or improve
   error messages or something.)

3. Perhaps wrap up libxl__ev_slowlock_init with two functions
   [libxl__ev_]devlock_init and libxl__ev_qmplock_init, and rename
   libxl__ev_slowlock_init to libxl__ev_slowlock_init_internal.

This avoids having to provide trivial wrappers for all the functions.
if you do all of this including (3) then the API is slightly anomalous
in that there are several distinct init functions but only one set of
operation functions but this seems OK to me.

What do you think ?

Thanks,
Ian.

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

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

[Ian Jackson]

Anthony PERARD writes ("[RFC XEN PATCH for-4.13 4/4] libxl_qmp: Have a lock for QMP socket access"):
> FIXME: The case where something failed when trying to acquired the
>    lock isn't handled yet.

This patch seems to contain roughly the kind of things I would expect.

Because of that FIXME I think it would not make sense for me to review
it in detail, as I would probably trip up over that too much.

Also we should decide if you like any of my alternative workaround
suggestions...

Thanks,
Ian.

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

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

[Anthony PERARD]

On Mon, Oct 28, 2019 at 11:25:26AM +0000, Ian Jackson wrote:
> Hi.  Thanks for tackling this swamp.  All very unfortunate.
> 
> Anthony PERARD writes ("[RFC XEN PATCH for-4.13 0/4] Fix: libxl workaround, multiple connection to single QMP socket"):
> > Alternatively to this craziness, it might be possible to increase
> > the `backlog' value by having libxl opening the QMP socket on behalf
> > of QEMU. But this is only possible with a recent version of QEMU
> > (2.12 or newer, released in Apr 2018, or qemu-xen-4.12 or newer). It
> > would involve to discover QEMU's capability before we start the DM,
> > which libxl isn't capable yet.
> 
> I have an ancient unapplied patch somewhere which runs qemu --help
> and greps the output.  If you would like, I can dig it out.
> 
> But one problem with that approach is this: without that feature in
> qemu, what would we do ?  Live with the bug where domain creation
> fails ?  Bodge it by serialising within domain create (awkwardating
> the code) ?
> 
> I have some other suggestions which ought to be considered:
> 
> 
> 1. Send a patch to qemu upstream to allow specifying the socket listen
> queue.
> 
> 1(a) Expect distros to apply that patch to older qemus, if they ship
> older qemus.  Have libxl unconditionally specify that argument.
> 
> 1(b) grep the help output (as I propose above) and if the patch is not
> present, use LD_PRELOAD to wrap listen(2).
> 
> 
> 2. Send a patch to qemu upstream to change the fixed queue length from
> 1 to 10000.  Expect distros to apply that patch to older qemus (even,
> perhaps, if it is not accepted upstream!)  Change libxl to detect
> EAGAIN from qmp connect() and print a message explaining what patch is
> missing.

Those suggestions are interesting idea, but I would prefer to have libxl
been able to deal with any version of QEMU, so without having to patch
QEMU. Beside serialising QMP access in the code, fork/lock strategy
might be the only other way. (Well there is also fork/connect with a
blocking fd, but we already have code for fork/lock.)

So I'll keep working on the fork/lock strategy.

> Since you have provided an implementation of the fork/lock strategy,
> I'll now go and do a detailed review of that.

Thanks,

-- 
Anthony PERARD

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

Re: [Xen-devel] [RFC XEN PATCH for-4.13 1/4] libxl: Introduce libxl__ev_child_kill

[Anthony PERARD]

On Mon, Oct 28, 2019 at 11:26:41AM +0000, Ian Jackson wrote:
> Hi.  Thanks.  The code here looks by and large good to me but I think
> the docs and maybe the log messages need improvement.
> 
> Anthony PERARD writes ("[RFC XEN PATCH for-4.13 1/4] libxl: Introduce libxl__ev_child_kill"):
> > Allow to kill a child and deregister the callback associated with it.
> 
> Did you read the doc comment above libxl__ev_child_fork, in
> libxl_internal.h near line 1160 ?  The user of libxl__ev_child is
> already permitted to kill the child.
> 
> In this patch are adding a layer to make this more convenient, and in
> particular to let a libxl__ev_child user transfer responsibility for
> reaping the child from its own application logic into the ao system.
> 
> Some more API documentation to make this much more explicit would be
> good - ie the main doc comment the facility needs to discuss it:
>  | * It is not possible to "deregister" the child death event source
> ^ this is no longer true after your patch; indeed that's the point.
> 
> So perhaps
> 
> > +void libxl__ev_child_kill(libxl__ao *ao, libxl__ev_child *ch)
> 
> should be called
>    libxl__ev_child_reattach_to_ao
> or
>    libxl__ev_child_kill_deregister
> or something, and maybe it should take a signal number ?

I'll rework the documentation to explain that the AO won't complete
until the child has been reaped. Adding the signal number to the
parameter and renaming the function _kill_derigister sound good.

> > +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;
> > +
> > +    if (status) {
> > +        libxl_report_child_exitstatus(CTX, XTL_ERROR,
> > +                                      "killed fork (dying as expected)",
> > +                                      pid, status);
> > +    } else {
> > +        LOG(DEBUG, "killed child exit cleanly, unexpected");
> 
> I don't think this is entirely unexpected.  Maybe the child was just
> exiting at the point where libxl__ev_child_kill was called.
> 
> And, please check log the actual whole exit status.  "status" is a
> wait status.  We want to know what signal it died from, whether it
> core dumped, the exit status, etc.  Probably, you should call
> libxl_report_child_exitstatus.

It does ;-). But I guess I could call libxl_report_child_exitstatus()
unconditionally, so even if status=0.

> > @@ -1891,7 +1891,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;
> >  }
> 
> I wonder if this should gain a new debug message.  If the child gets
> lost or stuck for some reason, it will otherwise require searching the
> past log to find out why the ao doesn't return.

Do you mean adding a debug message in libxl__ev_child_kill_deregister()?
It's probably a good idea.

I'll add:
    LOG(DEBUG, "ao %p: Will wait process [%ld] death", ao, pid);

Or should we also add a debug log in libxl__ao_complete() ?


-- 
Anthony PERARD

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

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

[Ian Jackson]

Anthony PERARD writes ("Re: [RFC XEN PATCH for-4.13 0/4] Fix: libxl workaround, multiple connection to single QMP socket"):
> Those suggestions are interesting idea, but I would prefer to have libxl
> been able to deal with any version of QEMU, so without having to patch
> QEMU. Beside serialising QMP access in the code, fork/lock strategy
> might be the only other way. (Well there is also fork/connect with a
> blocking fd, but we already have code for fork/lock.)
> 
> So I'll keep working on the fork/lock strategy.

OK.  Thanks for the detailed reply, which makes sense to me.

Ian.

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

Re: [Xen-devel] [RFC XEN PATCH for-4.13 1/4] libxl: Introduce libxl__ev_child_kill

[Ian Jackson]

Anthony PERARD writes ("Re: [RFC XEN PATCH for-4.13 1/4] libxl: Introduce libxl__ev_child_kill"):
...
> > > +    if (status) {
> > > +        libxl_report_child_exitstatus(CTX, XTL_ERROR,
> > > +                                      "killed fork (dying as expected)",
> > > +                                      pid, status);
> > > +    } else {
> > > +        LOG(DEBUG, "killed child exit cleanly, unexpected");
> > 
> > I don't think this is entirely unexpected.  Maybe the child was just
> > exiting at the point where libxl__ev_child_kill was called.
> > 
> > And, please check log the actual whole exit status.  "status" is a
> > wait status.  We want to know what signal it died from, whether it
> > core dumped, the exit status, etc.  Probably, you should call
> > libxl_report_child_exitstatus.
> 
> It does ;-).

Oh.  Err.  Yes.

>   But I guess I could call libxl_report_child_exitstatus()
> unconditionally, so even if status=0.

I think that would be fine.  I'm not sure this code knows what set of
exit statuses are plausible so I think calling all kinds of exit
`expected' seems best to me.

> > > @@ -1891,7 +1891,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;
> > >  }
> > 
> > I wonder if this should gain a new debug message.  If the child gets
> > lost or stuck for some reason, it will otherwise require searching the
> > past log to find out why the ao doesn't return.
> 
> Do you mean adding a debug message in libxl__ev_child_kill_deregister()?
> It's probably a good idea.
...
> Or should we also add a debug log in libxl__ao_complete() ?

The latter, yes.  Because that happens at the point where the AO is
otherwise complete.  So if one is reading the log to try to find out
why the thing hasn't completed the debug log will actually say
something about it where you're looking, rather than miles away
somewhere in the scroll.

Thanks,
Ian.

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