* [Xen-devel] [XEN PATCH for-4.13 v2 1/6] libxl: Introduce libxl__ev_child_kill_deregister
2019-10-30 18:06 [Xen-devel] [XEN PATCH for-4.13 v2 0/6] Fix: libxl workaround, multiple connection to single QMP socket Anthony PERARD
@ 2019-10-30 18:06 ` Anthony PERARD
2019-11-15 16:05 ` Ian Jackson
2019-10-30 18:07 ` [Xen-devel] [XEN PATCH for-4.13 v2 2/6] libxl: Move libxl__ev_devlock declaration Anthony PERARD
` (6 subsequent siblings)
7 siblings, 1 reply; 17+ messages in thread
From: Anthony PERARD @ 2019-10-30 18:06 UTC (permalink / raw)
To: xen-devel; +Cc: Anthony PERARD, Jürgen Groß, Ian Jackson, Wei Liu
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>
---
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
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [Xen-devel] [XEN PATCH for-4.13 v2 1/6] libxl: Introduce libxl__ev_child_kill_deregister
2019-10-30 18:06 ` [Xen-devel] [XEN PATCH for-4.13 v2 1/6] libxl: Introduce libxl__ev_child_kill_deregister Anthony PERARD
@ 2019-11-15 16:05 ` Ian Jackson
0 siblings, 0 replies; 17+ messages in thread
From: Ian Jackson @ 2019-11-15 16:05 UTC (permalink / raw)
To: Anthony PERARD; +Cc: Jürgen Groß, xen-devel, Wei Liu
Anthony PERARD writes ("[XEN PATCH for-4.13 v2 1/6] libxl: Introduce libxl__ev_child_kill_deregister"):
> 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.
Very good.
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
Thanks,
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 17+ messages in thread
* [Xen-devel] [XEN PATCH for-4.13 v2 2/6] libxl: Move libxl__ev_devlock declaration
2019-10-30 18:06 [Xen-devel] [XEN PATCH for-4.13 v2 0/6] Fix: libxl workaround, multiple connection to single QMP socket Anthony PERARD
2019-10-30 18:06 ` [Xen-devel] [XEN PATCH for-4.13 v2 1/6] libxl: Introduce libxl__ev_child_kill_deregister Anthony PERARD
@ 2019-10-30 18:07 ` Anthony PERARD
2019-11-15 16:06 ` Ian Jackson
2019-10-30 18:07 ` [Xen-devel] [XEN PATCH for-4.13 v2 3/6] libxl: Rename ev_devlock to ev_slowlock Anthony PERARD
` (5 subsequent siblings)
7 siblings, 1 reply; 17+ messages in thread
From: Anthony PERARD @ 2019-10-30 18:07 UTC (permalink / raw)
To: xen-devel; +Cc: Anthony PERARD, Jürgen Groß, Ian Jackson, Wei Liu
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>
---
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
^ permalink raw reply related [flat|nested] 17+ messages in thread* [Xen-devel] [XEN PATCH for-4.13 v2 3/6] libxl: Rename ev_devlock to ev_slowlock
2019-10-30 18:06 [Xen-devel] [XEN PATCH for-4.13 v2 0/6] Fix: libxl workaround, multiple connection to single QMP socket Anthony PERARD
2019-10-30 18:06 ` [Xen-devel] [XEN PATCH for-4.13 v2 1/6] libxl: Introduce libxl__ev_child_kill_deregister Anthony PERARD
2019-10-30 18:07 ` [Xen-devel] [XEN PATCH for-4.13 v2 2/6] libxl: Move libxl__ev_devlock declaration Anthony PERARD
@ 2019-10-30 18:07 ` Anthony PERARD
2019-11-15 16:08 ` Ian Jackson
2019-10-30 18:07 ` [Xen-devel] [XEN PATCH for-4.13 v2 4/6] libxl: Introduce libxl__ev_slowlock_dispose Anthony PERARD
` (4 subsequent siblings)
7 siblings, 1 reply; 17+ messages in thread
From: Anthony PERARD @ 2019-10-30 18:07 UTC (permalink / raw)
To: xen-devel; +Cc: Anthony PERARD, Jürgen Groß, Ian Jackson, Wei Liu
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>
---
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 9d0eb5aed11d..84ebfcceb83e 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);
@@ -2202,7 +2202,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
^ permalink raw reply related [flat|nested] 17+ messages in thread* [Xen-devel] [XEN PATCH for-4.13 v2 4/6] libxl: Introduce libxl__ev_slowlock_dispose
2019-10-30 18:06 [Xen-devel] [XEN PATCH for-4.13 v2 0/6] Fix: libxl workaround, multiple connection to single QMP socket Anthony PERARD
` (2 preceding siblings ...)
2019-10-30 18:07 ` [Xen-devel] [XEN PATCH for-4.13 v2 3/6] libxl: Rename ev_devlock to ev_slowlock Anthony PERARD
@ 2019-10-30 18:07 ` Anthony PERARD
2019-11-15 16:08 ` Ian Jackson
2019-10-30 18:07 ` [Xen-devel] [XEN PATCH for-4.13 v2 5/6] libxl: libxl__ev_qmp_send now takes an egc Anthony PERARD
` (3 subsequent siblings)
7 siblings, 1 reply; 17+ messages in thread
From: Anthony PERARD @ 2019-10-30 18:07 UTC (permalink / raw)
To: xen-devel; +Cc: Anthony PERARD, Jürgen Groß, Ian Jackson, Wei Liu
Which allow to cancel the lock operation while it is in Active state.
Signed-off-by: Anthony PERARD <anthony.perard@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
^ permalink raw reply related [flat|nested] 17+ messages in thread* [Xen-devel] [XEN PATCH for-4.13 v2 5/6] libxl: libxl__ev_qmp_send now takes an egc
2019-10-30 18:06 [Xen-devel] [XEN PATCH for-4.13 v2 0/6] Fix: libxl workaround, multiple connection to single QMP socket Anthony PERARD
` (3 preceding siblings ...)
2019-10-30 18:07 ` [Xen-devel] [XEN PATCH for-4.13 v2 4/6] libxl: Introduce libxl__ev_slowlock_dispose Anthony PERARD
@ 2019-10-30 18:07 ` Anthony PERARD
2019-10-30 18:07 ` [Xen-devel] [XEN PATCH for-4.13 v2 6/6] libxl_qmp: Have a lock for QMP socket access Anthony PERARD
` (2 subsequent siblings)
7 siblings, 0 replies; 17+ messages in thread
From: Anthony PERARD @ 2019-10-30 18:07 UTC (permalink / raw)
To: xen-devel; +Cc: Anthony PERARD, Jürgen Groß, Ian Jackson, Wei Liu
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 7e52f0973172..ebfe98ebc237 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) {
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 84ebfcceb83e..723ad3bf8edb 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 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
^ permalink raw reply related [flat|nested] 17+ messages in thread* [Xen-devel] [XEN PATCH for-4.13 v2 6/6] libxl_qmp: Have a lock for QMP socket access
2019-10-30 18:06 [Xen-devel] [XEN PATCH for-4.13 v2 0/6] Fix: libxl workaround, multiple connection to single QMP socket Anthony PERARD
` (4 preceding siblings ...)
2019-10-30 18:07 ` [Xen-devel] [XEN PATCH for-4.13 v2 5/6] libxl: libxl__ev_qmp_send now takes an egc Anthony PERARD
@ 2019-10-30 18:07 ` Anthony PERARD
2019-11-15 17:08 ` Ian Jackson
2019-10-30 22:27 ` [Xen-devel] [XEN PATCH for-4.13 v2 0/6] Fix: libxl workaround, multiple connection to single QMP socket Sander Eikelenboom
2019-11-08 6:06 ` Jürgen Groß
7 siblings, 1 reply; 17+ messages in thread
From: Anthony PERARD @ 2019-10-30 18:07 UTC (permalink / raw)
To: xen-devel
Cc: Anthony PERARD, Jürgen Groß,
Ian Jackson, Wei Liu, Sander Eikelenboom
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.
Error handling of connect() and lock() is a bit awkward as
libxl__ev_qmp_send() doesn't allow to call the callback synchronously.
So we setup a timer to have a callback that has been called
asynchronously. We use the _abs variant it does strictly less than
_rel, thus avoiding unnecessary code that could return an error
(unnecessary because we only need to have the callback been called
ASAP).
Reported-by: Sander Eikelenboom <linux@eikelenboom.it>
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
Notes:
v2:
- Handle error path
tools/libxl/libxl_internal.c | 5 ++
tools/libxl/libxl_internal.h | 9 +++
tools/libxl/libxl_qmp.c | 109 ++++++++++++++++++++++++++++++-----
3 files changed, 108 insertions(+), 15 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 f95895eae17d..b244b508f4c6 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -371,6 +371,9 @@ struct libxl__ev_child {
* 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.
*
@@ -411,6 +414,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 *);
@@ -479,6 +483,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 */
@@ -512,6 +518,9 @@ struct libxl__ev_qmp {
libxl__carefd *cfd;
libxl__ev_fd efd;
libxl__qmp_state state;
+ libxl__ev_slowlock lock;
+ libxl__ev_time etime;
+ 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..2087a85e321d 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
@@ -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,23 @@ 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_time *,
+ const struct timeval *, int rc);
+
+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 +1270,34 @@ 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)
+{
+ 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 +1319,43 @@ 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;
+ struct timeval now = { 0 };
+ rc = libxl__ev_time_register_abs(ev->ao, &ev->etime,
+ lock_error_callback, now);
+ /* If setting up the timer failed, there is no way to tell the caller
+ * of libxl__ev_qmp_send() that the connection to the QMP socket
+ * failed. But they are supposed to have a timer of their own. */
+ if (rc)
+ LOGD(ERROR, ev->domid,
+ "Failed to setup a callback call. rc=%d", rc);
+}
+
+static void lock_error_callback(libxl__egc *egc, libxl__ev_time *t,
+ const struct timeval *requested_abs,
+ int rc)
+{
+ EGC_GC;
+ libxl__ev_qmp *ev = CONTAINER_OF(t, *ev, etime);
+
+ /* We are only interested by the `rc' set during the setup of the
+ * connection to the QMP socket. */
+ 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 +1852,15 @@ 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;
+ libxl__ev_time_init(&ev->etime);
}
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 +1875,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 +1907,8 @@ 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_time_deregister(gc, &ev->etime);
+ 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
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [Xen-devel] [XEN PATCH for-4.13 v2 6/6] libxl_qmp: Have a lock for QMP socket access
2019-10-30 18:07 ` [Xen-devel] [XEN PATCH for-4.13 v2 6/6] libxl_qmp: Have a lock for QMP socket access Anthony PERARD
@ 2019-11-15 17:08 ` Ian Jackson
0 siblings, 0 replies; 17+ messages in thread
From: Ian Jackson @ 2019-11-15 17:08 UTC (permalink / raw)
To: Anthony PERARD
Cc: Jürgen Groß, xen-devel, Wei Liu, Sander Eikelenboom
Anthony PERARD writes ("[XEN PATCH for-4.13 v2 6/6] libxl_qmp: Have a lock for QMP socket access"):
> 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.
Thanks again for tackling this.
> Error handling of connect() and lock() is a bit awkward as
> libxl__ev_qmp_send() doesn't allow to call the callback synchronously.
> So we setup a timer to have a callback that has been called
> asynchronously. We use the _abs variant it does strictly less than
> _rel, thus avoiding unnecessary code that could return an error
> (unnecessary because we only need to have the callback been called
> ASAP).
I have some problems with the approach here, I'm afraid.
> 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.
...
> * 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
Don't `lock' and `time' need to be added to this table ?
The table may become rather wide :-/. Maybe it could be
compressed/abbreviated some more or maybe we'll just live with it
becoming wider.
> 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;
> + struct timeval now = { 0 };
> + rc = libxl__ev_time_register_abs(ev->ao, &ev->etime,
> + lock_error_callback, now);
> + /* If setting up the timer failed, there is no way to tell the caller
> + * of libxl__ev_qmp_send() that the connection to the QMP socket
> + * failed. But they are supposed to have a timer of their own. */
> + if (rc)
> + LOGD(ERROR, ev->domid,
> + "Failed to setup a callback call. rc=%d", rc);
I don't think this is right. I think this callback has to be set up
in a way that can't fail. But I think this is possible. We're free
to allocate memory (although this may not be needed) and the egc
(which we have) can contain callback pointers.
I can see two approaches:
1. Invent a new libxl_ev_immediate_but_not_reentrant_callback (can't
think of a good name right now). Add a new list head to the egc,
and when you want to register a callback, put it on that list.
Add the callback execution to egc_run_callbacks, probably at the
top in a loop (since immediate callbacks may add more immediate
callbacks).
2. Use libxl__ev_time; provide libxl__ev_time_register_now which
cannot fail: it sets the abs time to 0 and skips the abortable
stuff, and puts the libxl__ev_time on the front of the CTX etimes
list. Have egc_run_callbacks run immediate timeouts: ie, if the
head of the etimes list has abs time of 0, strip it, and call it.
I think this involves some special handling of this case because
you want to avoid messing about with the OSEVENT_HOOKs, so
you probably need a new bit in libxl__ev_time. Otherwise
time_deregister wouldn't know what to do.
I think 1 is probably less confusing and less risky. It doesn't
disturb, or get entangled in, the existing ev_time code; it doesn't
involve pointlessly allocating an unused abortable. And it doesn't
involve confusion over which error returns are possible where and
which rc values must be kept and which discarded.
Sorry,
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Xen-devel] [XEN PATCH for-4.13 v2 0/6] Fix: libxl workaround, multiple connection to single QMP socket
2019-10-30 18:06 [Xen-devel] [XEN PATCH for-4.13 v2 0/6] Fix: libxl workaround, multiple connection to single QMP socket Anthony PERARD
` (5 preceding siblings ...)
2019-10-30 18:07 ` [Xen-devel] [XEN PATCH for-4.13 v2 6/6] libxl_qmp: Have a lock for QMP socket access Anthony PERARD
@ 2019-10-30 22:27 ` Sander Eikelenboom
2019-11-08 6:06 ` Jürgen Groß
7 siblings, 0 replies; 17+ messages in thread
From: Sander Eikelenboom @ 2019-10-30 22:27 UTC (permalink / raw)
To: Anthony PERARD, xen-devel; +Cc: Jürgen Groß, Ian Jackson, Wei Liu
On 30/10/2019 19:06, 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-v2
>
> 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 (6):
Hi Anthony,
Re-tested, especially the pci-pt part, still works for me.
Thanks again (and thanks for providing a git branch)
--
Sander
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [Xen-devel] [XEN PATCH for-4.13 v2 0/6] Fix: libxl workaround, multiple connection to single QMP socket
2019-10-30 18:06 [Xen-devel] [XEN PATCH for-4.13 v2 0/6] Fix: libxl workaround, multiple connection to single QMP socket Anthony PERARD
` (6 preceding siblings ...)
2019-10-30 22:27 ` [Xen-devel] [XEN PATCH for-4.13 v2 0/6] Fix: libxl workaround, multiple connection to single QMP socket Sander Eikelenboom
@ 2019-11-08 6:06 ` Jürgen Groß
2019-11-15 13:51 ` Sander Eikelenboom
7 siblings, 1 reply; 17+ messages in thread
From: Jürgen Groß @ 2019-11-08 6:06 UTC (permalink / raw)
To: Anthony PERARD, xen-devel; +Cc: Ian Jackson, Wei Liu
On 30.10.19 19:06, 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-v2
>
> 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 (6):
> 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_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 | 6 +-
> tools/libxl/libxl_fork.c | 48 ++++++++++++
> tools/libxl/libxl_internal.c | 41 +++++++---
> tools/libxl/libxl_internal.h | 130 +++++++++++++++++++-------------
> tools/libxl/libxl_pci.c | 8 +-
> tools/libxl/libxl_qmp.c | 119 ++++++++++++++++++++++++-----
> tools/libxl/libxl_usb.c | 28 ++++---
> 12 files changed, 301 insertions(+), 125 deletions(-)
>
For the series:
Release-acked-by: Juergen Gross <jgross@suse.com>
Juergen
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [Xen-devel] [XEN PATCH for-4.13 v2 0/6] Fix: libxl workaround, multiple connection to single QMP socket
2019-11-08 6:06 ` Jürgen Groß
@ 2019-11-15 13:51 ` Sander Eikelenboom
2019-11-15 13:56 ` Jürgen Groß
0 siblings, 1 reply; 17+ messages in thread
From: Sander Eikelenboom @ 2019-11-15 13:51 UTC (permalink / raw)
To: Jürgen Groß, Anthony PERARD, xen-devel; +Cc: Ian Jackson, Wei Liu
On 08/11/2019 07:06, Jürgen Groß wrote:
> On 30.10.19 19:06, 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-v2
>>
>> 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 (6):
>> 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_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 | 6 +-
>> tools/libxl/libxl_fork.c | 48 ++++++++++++
>> tools/libxl/libxl_internal.c | 41 +++++++---
>> tools/libxl/libxl_internal.h | 130 +++++++++++++++++++-------------
>> tools/libxl/libxl_pci.c | 8 +-
>> tools/libxl/libxl_qmp.c | 119 ++++++++++++++++++++++++-----
>> tools/libxl/libxl_usb.c | 28 ++++---
>> 12 files changed, 301 insertions(+), 125 deletions(-)
>>
>
> For the series:
>
> Release-acked-by: Juergen Gross <jgross@suse.com>
>
>
> Juergen
Hi Juergen,
Since a lot more recent patches have been committed, but these don't seem to.
I was wondering if these fell through the cracks.
--
Sander
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Xen-devel] [XEN PATCH for-4.13 v2 0/6] Fix: libxl workaround, multiple connection to single QMP socket
2019-11-15 13:51 ` Sander Eikelenboom
@ 2019-11-15 13:56 ` Jürgen Groß
2019-11-15 14:01 ` Wei Liu
0 siblings, 1 reply; 17+ messages in thread
From: Jürgen Groß @ 2019-11-15 13:56 UTC (permalink / raw)
To: Sander Eikelenboom, Anthony PERARD, xen-devel; +Cc: Ian Jackson, Wei Liu
On 15.11.19 14:51, Sander Eikelenboom wrote:
> On 08/11/2019 07:06, Jürgen Groß wrote:
>> On 30.10.19 19:06, 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-v2
>>>
>>> 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 (6):
>>> 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_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 | 6 +-
>>> tools/libxl/libxl_fork.c | 48 ++++++++++++
>>> tools/libxl/libxl_internal.c | 41 +++++++---
>>> tools/libxl/libxl_internal.h | 130 +++++++++++++++++++-------------
>>> tools/libxl/libxl_pci.c | 8 +-
>>> tools/libxl/libxl_qmp.c | 119 ++++++++++++++++++++++++-----
>>> tools/libxl/libxl_usb.c | 28 ++++---
>>> 12 files changed, 301 insertions(+), 125 deletions(-)
>>>
>>
>> For the series:
>>
>> Release-acked-by: Juergen Gross <jgross@suse.com>
>>
>>
>> Juergen
>
> Hi Juergen,
>
> Since a lot more recent patches have been committed, but these don't seem to.
> I was wondering if these fell through the cracks.
Hi Sander,
I'm no committer, "just" the one who allows the committers to take a
patch or series at that phase of the release...
Juergen
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Xen-devel] [XEN PATCH for-4.13 v2 0/6] Fix: libxl workaround, multiple connection to single QMP socket
2019-11-15 13:56 ` Jürgen Groß
@ 2019-11-15 14:01 ` Wei Liu
0 siblings, 0 replies; 17+ messages in thread
From: Wei Liu @ 2019-11-15 14:01 UTC (permalink / raw)
To: Jürgen Groß
Cc: Anthony PERARD, Sander Eikelenboom, Ian Jackson, Wei Liu, xen-devel
On Fri, Nov 15, 2019 at 02:56:48PM +0100, Jürgen Groß wrote:
> > >
> > > Juergen
> >
> > Hi Juergen,
> >
> > Since a lot more recent patches have been committed, but these don't seem to.
> > I was wondering if these fell through the cracks.
>
> Hi Sander,
>
> I'm no committer, "just" the one who allows the committers to take a
> patch or series at that phase of the release...
This series hasn't been reviewed yet.
Unfortunately, -ETIME for me at the moment.
Wei.
>
> Juergen
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 17+ messages in thread