* [Qemu-devel] [PATCH v7 0/7] monitor: enable OOB by default
@ 2018-09-03 4:31 Peter Xu
2018-09-03 4:31 ` [Qemu-devel] [PATCH v7 1/7] qapi: Fix build_params() for empty parameter list Peter Xu
` (7 more replies)
0 siblings, 8 replies; 28+ messages in thread
From: Peter Xu @ 2018-09-03 4:31 UTC (permalink / raw)
To: qemu-devel
Cc: Eric Blake, Marc-André Lureau, Daniel P . Berrange,
Markus Armbruster, Dr . David Alan Gilbert, peterx
(this series is based on Markus's monitor-next tree so if patchew
spits something out with "apply failure" then it's expected)
v7:
- use Markus's commit message for patch "qapi: Drop
qapi_event_send_FOO()'s Error ** argument" [Markus]
- update commit message for "qapi: remove COMMAND_DROPPED event" since
QEMU 3.0 is released [Eric/Dave]
- rebase to Markus's monitor-next tree:
http://repo.or.cz/qemu/armbru.git/shortlog/refs/heads/monitor-next
the patch "monitor: suspend monitor instead of send CMD_DROP"
re-written since people prefer to drop need_resume flag so now I
hand-made it. Dropped a few patches since not appliable any more.
Please review. Thanks,
Markus Armbruster (1):
qapi: Fix build_params() for empty parameter list
Peter Xu (6):
qapi: Drop qapi_event_send_FOO()'s Error ** argument
monitor: suspend monitor instead of send CMD_DROP
qapi: remove COMMAND_DROPPED event
monitor: remove "x-oob", turn oob on by default
Revert "tests: Add parameter to qtest_init_without_qmp_handshake"
tests: add oob functional test for test-qmp-cmds
block/block-backend.c | 8 ++---
block/qcow2.c | 2 +-
block/quorum.c | 4 +--
block/write-threshold.c | 3 +-
blockjob.c | 13 ++++----
cpus.c | 8 ++---
docs/devel/qapi-code-gen.txt | 6 ++--
docs/interop/qmp-spec.txt | 5 ++--
dump.c | 3 +-
hw/acpi/core.c | 2 +-
hw/acpi/cpu.c | 2 +-
hw/acpi/memory_hotplug.c | 5 ++--
hw/char/virtio-console.c | 3 +-
hw/core/qdev.c | 3 +-
hw/net/virtio-net.c | 2 +-
hw/ppc/spapr_rtc.c | 2 +-
hw/timer/mc146818rtc.c | 2 +-
hw/virtio/virtio-balloon.c | 3 +-
hw/watchdog/watchdog.c | 15 +++++-----
include/monitor/monitor.h | 1 -
include/qapi/qmp-event.h | 3 +-
job.c | 2 +-
migration/migration.c | 4 +--
migration/ram.c | 2 +-
monitor.c | 58 ++++++++++++++----------------------
qapi/misc.json | 40 -------------------------
scripts/qapi/common.py | 10 +++----
scripts/qapi/events.py | 23 ++++----------
scsi/pr-manager-helper.c | 3 +-
tests/libqtest.c | 10 +++----
tests/libqtest.h | 4 +--
tests/qmp-test.c | 6 ++--
tests/test-qmp-cmds.c | 16 ++++++++++
tests/test-qmp-event.c | 11 ++++---
ui/spice-core.c | 10 +++----
ui/vnc.c | 7 ++---
vl.c | 21 +++++--------
37 files changed, 120 insertions(+), 202 deletions(-)
--
2.17.1
^ permalink raw reply [flat|nested] 28+ messages in thread
* [Qemu-devel] [PATCH v7 1/7] qapi: Fix build_params() for empty parameter list
2018-09-03 4:31 [Qemu-devel] [PATCH v7 0/7] monitor: enable OOB by default Peter Xu
@ 2018-09-03 4:31 ` Peter Xu
2018-09-03 4:31 ` [Qemu-devel] [PATCH v7 2/7] qapi: Drop qapi_event_send_FOO()'s Error ** argument Peter Xu
` (6 subsequent siblings)
7 siblings, 0 replies; 28+ messages in thread
From: Peter Xu @ 2018-09-03 4:31 UTC (permalink / raw)
To: qemu-devel
Cc: Eric Blake, Marc-André Lureau, Daniel P . Berrange,
Markus Armbruster, Dr . David Alan Gilbert, peterx
From: Markus Armbruster <armbru@redhat.com>
build_params() returns '' instead of 'void' when there are no
parameters. Can't happen now, but the next commit will change that.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
[peterx: compose the patch from email replies]
Signed-off-by: Peter Xu <peterx@redhat.com>
---
scripts/qapi/common.py | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
index 02c5c6767a..3b0d4bf9c0 100644
--- a/scripts/qapi/common.py
+++ b/scripts/qapi/common.py
@@ -2070,16 +2070,14 @@ extern const QEnumLookup %(c_name)s_lookup;
return ret
-def build_params(arg_type, boxed, extra):
- if not arg_type:
- assert not boxed
- return extra
+def build_params(arg_type, boxed, extra=None):
ret = ''
sep = ''
if boxed:
+ assert arg_type
ret += '%s arg' % arg_type.c_param_type()
sep = ', '
- else:
+ elif arg_type:
assert not arg_type.variants
for memb in arg_type.members:
ret += sep
@@ -2090,7 +2088,7 @@ def build_params(arg_type, boxed, extra):
c_name(memb.name))
if extra:
ret += sep + extra
- return ret
+ return ret if ret else 'void'
#
--
2.17.1
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [Qemu-devel] [PATCH v7 2/7] qapi: Drop qapi_event_send_FOO()'s Error ** argument
2018-09-03 4:31 [Qemu-devel] [PATCH v7 0/7] monitor: enable OOB by default Peter Xu
2018-09-03 4:31 ` [Qemu-devel] [PATCH v7 1/7] qapi: Fix build_params() for empty parameter list Peter Xu
@ 2018-09-03 4:31 ` Peter Xu
2018-09-03 4:31 ` [Qemu-devel] [PATCH v7 3/7] monitor: suspend monitor instead of send CMD_DROP Peter Xu
` (5 subsequent siblings)
7 siblings, 0 replies; 28+ messages in thread
From: Peter Xu @ 2018-09-03 4:31 UTC (permalink / raw)
To: qemu-devel
Cc: Eric Blake, Marc-André Lureau, Daniel P . Berrange,
Markus Armbruster, Dr . David Alan Gilbert, peterx
The generated qapi_event_send_FOO() take an Error ** argument. They
can't actually fail, because all they do with the argument is passing it
to functions that can't fail: the QObject output visitor, and the
@qmp_emit callback, which is either monitor_qapi_event_queue() or
event_test_emit().
Drop the argument, and pass &error_abort to the QObject output visitor
and @qmp_emit instead.
Suggested-by: Eric Blake <eblake@redhat.com>
Suggested-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
block/block-backend.c | 8 +++-----
block/qcow2.c | 2 +-
block/quorum.c | 4 ++--
block/write-threshold.c | 3 +--
blockjob.c | 13 +++++--------
cpus.c | 8 ++++----
docs/devel/qapi-code-gen.txt | 6 ++----
dump.c | 3 +--
hw/acpi/core.c | 2 +-
hw/acpi/cpu.c | 2 +-
hw/acpi/memory_hotplug.c | 5 ++---
hw/char/virtio-console.c | 3 +--
hw/core/qdev.c | 3 +--
hw/net/virtio-net.c | 2 +-
hw/ppc/spapr_rtc.c | 2 +-
hw/timer/mc146818rtc.c | 2 +-
hw/virtio/virtio-balloon.c | 3 +--
hw/watchdog/watchdog.c | 15 +++++++--------
include/qapi/qmp-event.h | 3 +--
job.c | 2 +-
migration/migration.c | 4 ++--
migration/ram.c | 2 +-
monitor.c | 5 ++---
scripts/qapi/events.py | 23 ++++++-----------------
scsi/pr-manager-helper.c | 3 +--
tests/test-qmp-event.c | 11 +++++------
ui/spice-core.c | 10 ++++------
ui/vnc.c | 7 +++----
vl.c | 16 +++++++---------
29 files changed, 69 insertions(+), 103 deletions(-)
diff --git a/block/block-backend.c b/block/block-backend.c
index fa120630be..14a1b7ac6a 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -980,8 +980,7 @@ void blk_dev_change_media_cb(BlockBackend *blk, bool load, Error **errp)
if (tray_was_open != tray_is_open) {
char *id = blk_get_attached_dev_id(blk);
- qapi_event_send_device_tray_moved(blk_name(blk), id, tray_is_open,
- &error_abort);
+ qapi_event_send_device_tray_moved(blk_name(blk), id, tray_is_open);
g_free(id);
}
}
@@ -1665,8 +1664,7 @@ static void send_qmp_error_event(BlockBackend *blk,
qapi_event_send_block_io_error(blk_name(blk), !!bs,
bs ? bdrv_get_node_name(bs) : NULL, optype,
action, blk_iostatus_is_enabled(blk),
- error == ENOSPC, strerror(error),
- &error_abort);
+ error == ENOSPC, strerror(error));
}
/* This is done by device models because, while the block layer knows
@@ -1782,7 +1780,7 @@ void blk_eject(BlockBackend *blk, bool eject_flag)
* the frontend experienced a tray event. */
id = blk_get_attached_dev_id(blk);
qapi_event_send_device_tray_moved(blk_name(blk), id,
- eject_flag, &error_abort);
+ eject_flag);
g_free(id);
}
diff --git a/block/qcow2.c b/block/qcow2.c
index ec9e6238a0..c13153735a 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -4659,7 +4659,7 @@ void qcow2_signal_corruption(BlockDriverState *bs, bool fatal, int64_t offset,
*node_name != '\0', node_name,
message, offset >= 0, offset,
size >= 0, size,
- fatal, &error_abort);
+ fatal);
g_free(message);
if (fatal) {
diff --git a/block/quorum.c b/block/quorum.c
index 9152da8c58..eb526cc0f1 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -199,7 +199,7 @@ static void quorum_report_bad(QuorumOpType type, uint64_t offset,
}
qapi_event_send_quorum_report_bad(type, !!msg, msg, node_name, start_sector,
- end_sector - start_sector, &error_abort);
+ end_sector - start_sector);
}
static void quorum_report_failure(QuorumAIOCB *acb)
@@ -210,7 +210,7 @@ static void quorum_report_failure(QuorumAIOCB *acb)
BDRV_SECTOR_SIZE);
qapi_event_send_quorum_failure(reference, start_sector,
- end_sector - start_sector, &error_abort);
+ end_sector - start_sector);
}
static int quorum_vote_error(QuorumAIOCB *acb);
diff --git a/block/write-threshold.c b/block/write-threshold.c
index 1d48fc2077..85b78dc2a9 100644
--- a/block/write-threshold.c
+++ b/block/write-threshold.c
@@ -63,8 +63,7 @@ static int coroutine_fn before_write_notify(NotifierWithReturn *notifier,
qapi_event_send_block_write_threshold(
bs->node_name,
amount,
- bs->write_threshold_offset,
- &error_abort);
+ bs->write_threshold_offset);
/* autodisable to avoid flooding the monitor */
write_threshold_disable(bs);
diff --git a/blockjob.c b/blockjob.c
index be5903aa96..bf7ef48f98 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -315,8 +315,7 @@ static void block_job_event_cancelled(Notifier *n, void *opaque)
job->job.id,
job->job.progress_total,
job->job.progress_current,
- job->speed,
- &error_abort);
+ job->speed);
}
static void block_job_event_completed(Notifier *n, void *opaque)
@@ -338,8 +337,7 @@ static void block_job_event_completed(Notifier *n, void *opaque)
job->job.progress_current,
job->speed,
!!msg,
- msg,
- &error_abort);
+ msg);
}
static void block_job_event_pending(Notifier *n, void *opaque)
@@ -351,8 +349,7 @@ static void block_job_event_pending(Notifier *n, void *opaque)
}
qapi_event_send_block_job_pending(job_type(&job->job),
- job->job.id,
- &error_abort);
+ job->job.id);
}
static void block_job_event_ready(Notifier *n, void *opaque)
@@ -367,7 +364,7 @@ static void block_job_event_ready(Notifier *n, void *opaque)
job->job.id,
job->job.progress_total,
job->job.progress_current,
- job->speed, &error_abort);
+ job->speed);
}
@@ -494,7 +491,7 @@ BlockErrorAction block_job_error_action(BlockJob *job, BlockdevOnError on_err,
qapi_event_send_block_job_error(job->job.id,
is_read ? IO_OPERATION_TYPE_READ :
IO_OPERATION_TYPE_WRITE,
- action, &error_abort);
+ action);
}
if (action == BLOCK_ERROR_ACTION_STOP) {
job_pause(&job->job);
diff --git a/cpus.c b/cpus.c
index 8ee6e5db93..412aebc8b0 100644
--- a/cpus.c
+++ b/cpus.c
@@ -1053,7 +1053,7 @@ static int do_vm_stop(RunState state, bool send_stop)
runstate_set(state);
vm_state_notify(0, state);
if (send_stop) {
- qapi_event_send_stop(&error_abort);
+ qapi_event_send_stop();
}
}
@@ -2107,13 +2107,13 @@ int vm_prepare_start(void)
* the STOP event.
*/
if (runstate_is_running()) {
- qapi_event_send_stop(&error_abort);
- qapi_event_send_resume(&error_abort);
+ qapi_event_send_stop();
+ qapi_event_send_resume();
return -1;
}
/* We are sending this now, but the CPUs will be resumed shortly later */
- qapi_event_send_resume(&error_abort);
+ qapi_event_send_resume();
replay_enable_events();
cpu_enable_ticks();
diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
index c2e11465f0..6d3cffd548 100644
--- a/docs/devel/qapi-code-gen.txt
+++ b/docs/devel/qapi-code-gen.txt
@@ -1356,10 +1356,9 @@ Example:
$ cat qapi-generated/example-qapi-events.c
[Uninteresting stuff omitted...]
- void qapi_event_send_my_event(Error **errp)
+ void qapi_event_send_my_event(void)
{
QDict *qmp;
- Error *err = NULL;
QMPEventFuncEmit emit;
emit = qmp_event_get_func_emit();
@@ -1369,9 +1368,8 @@ Example:
qmp = qmp_event_build_dict("MY_EVENT");
- emit(EXAMPLE_QAPI_EVENT_MY_EVENT, qmp, &err);
+ emit(EXAMPLE_QAPI_EVENT_MY_EVENT, qmp);
- error_propagate(errp, err);
qobject_unref(qmp);
}
diff --git a/dump.c b/dump.c
index 500b554523..4ec94c5e25 100644
--- a/dump.c
+++ b/dump.c
@@ -1890,8 +1890,7 @@ static void dump_process(DumpState *s, Error **errp)
/* should never fail */
assert(result);
qapi_event_send_dump_completed(result, !!local_err, (local_err ? \
- error_get_pretty(local_err) : NULL),
- &error_abort);
+ error_get_pretty(local_err) : NULL));
qapi_free_DumpQueryResult(result);
error_propagate(errp, local_err);
diff --git a/hw/acpi/core.c b/hw/acpi/core.c
index b8d39012cd..aafdc61648 100644
--- a/hw/acpi/core.c
+++ b/hw/acpi/core.c
@@ -570,7 +570,7 @@ static void acpi_pm1_cnt_write(ACPIREGS *ar, uint16_t val)
break;
default:
if (sus_typ == ar->pm1.cnt.s4_val) { /* S4 request */
- qapi_event_send_suspend_disk(&error_abort);
+ qapi_event_send_suspend_disk();
qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
}
break;
diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c
index 5ae595ecbe..d19b7722f0 100644
--- a/hw/acpi/cpu.c
+++ b/hw/acpi/cpu.c
@@ -160,7 +160,7 @@ static void cpu_hotplug_wr(void *opaque, hwaddr addr, uint64_t data,
cdev = &cpu_st->devs[cpu_st->selector];
cdev->ost_status = data;
info = acpi_cpu_device_status(cpu_st->selector, cdev);
- qapi_event_send_acpi_device_ost(info, &error_abort);
+ qapi_event_send_acpi_device_ost(info);
qapi_free_ACPIOSTInfo(info);
trace_cpuhp_acpi_write_ost_status(cpu_st->selector,
cdev->ost_status);
diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
index 0ff1712c4c..8c7c1013f3 100644
--- a/hw/acpi/memory_hotplug.c
+++ b/hw/acpi/memory_hotplug.c
@@ -161,7 +161,7 @@ static void acpi_memory_hotplug_write(void *opaque, hwaddr addr, uint64_t data,
/* TODO: implement memory removal on guest signal */
info = acpi_memory_device_status(mem_st->selector, mdev);
- qapi_event_send_acpi_device_ost(info, &error_abort);
+ qapi_event_send_acpi_device_ost(info);
qapi_free_ACPIOSTInfo(info);
break;
case 0x14: /* set is_* fields */
@@ -185,8 +185,7 @@ static void acpi_memory_hotplug_write(void *opaque, hwaddr addr, uint64_t data,
if (local_err) {
trace_mhp_acpi_pc_dimm_delete_failed(mem_st->selector);
qapi_event_send_mem_unplug_error(dev->id,
- error_get_pretty(local_err),
- &error_abort);
+ error_get_pretty(local_err));
error_free(local_err);
break;
}
diff --git a/hw/char/virtio-console.c b/hw/char/virtio-console.c
index 679a824888..2cbe1d4ed5 100644
--- a/hw/char/virtio-console.c
+++ b/hw/char/virtio-console.c
@@ -114,8 +114,7 @@ static void set_guest_connected(VirtIOSerialPort *port, int guest_connected)
}
if (dev->id) {
- qapi_event_send_vserport_change(dev->id, guest_connected,
- &error_abort);
+ qapi_event_send_vserport_change(dev->id, guest_connected);
}
}
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 529b82de18..36b788a66b 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -1000,8 +1000,7 @@ static void device_finalize(Object *obj)
if (dev->pending_deleted_event) {
g_assert(dev->canonical_path);
- qapi_event_send_device_deleted(!!dev->id, dev->id, dev->canonical_path,
- &error_abort);
+ qapi_event_send_device_deleted(!!dev->id, dev->id, dev->canonical_path);
g_free(dev->canonical_path);
dev->canonical_path = NULL;
}
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index f154756e85..4bdd5b8532 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -329,7 +329,7 @@ static void rxfilter_notify(NetClientState *nc)
if (nc->rxfilter_notify_enabled) {
gchar *path = object_get_canonical_path(OBJECT(n->qdev));
qapi_event_send_nic_rx_filter_changed(!!n->netclient_name,
- n->netclient_name, path, &error_abort);
+ n->netclient_name, path);
g_free(path);
/* disable event notification to avoid events flooding */
diff --git a/hw/ppc/spapr_rtc.c b/hw/ppc/spapr_rtc.c
index a37360537e..cd049f389d 100644
--- a/hw/ppc/spapr_rtc.c
+++ b/hw/ppc/spapr_rtc.c
@@ -118,7 +118,7 @@ static void rtas_set_time_of_day(PowerPCCPU *cpu, sPAPRMachineState *spapr,
}
/* Generate a monitor event for the change */
- qapi_event_send_rtc_change(qemu_timedate_diff(&tm), &error_abort);
+ qapi_event_send_rtc_change(qemu_timedate_diff(&tm));
host_ns = qemu_clock_get_ns(rtc_clock);
diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
index a504f0308d..acee47da0e 100644
--- a/hw/timer/mc146818rtc.c
+++ b/hw/timer/mc146818rtc.c
@@ -635,7 +635,7 @@ static void rtc_set_time(RTCState *s)
s->base_rtc = mktimegm(&tm);
s->last_update = qemu_clock_get_ns(rtc_clock);
- qapi_event_send_rtc_change(qemu_timedate_diff(&tm), &error_abort);
+ qapi_event_send_rtc_change(qemu_timedate_diff(&tm));
}
static void rtc_set_cmos(RTCState *s, const struct tm *tm)
diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index b5425080c5..1728e4f83a 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -365,8 +365,7 @@ static void virtio_balloon_set_config(VirtIODevice *vdev,
dev->actual = le32_to_cpu(config.actual);
if (dev->actual != oldactual) {
qapi_event_send_balloon_change(vm_ram_size -
- ((ram_addr_t) dev->actual << VIRTIO_BALLOON_PFN_SHIFT),
- &error_abort);
+ ((ram_addr_t) dev->actual << VIRTIO_BALLOON_PFN_SHIFT));
}
trace_virtio_balloon_set_config(dev->actual, oldactual);
}
diff --git a/hw/watchdog/watchdog.c b/hw/watchdog/watchdog.c
index 6e8ba061d8..33e6c20184 100644
--- a/hw/watchdog/watchdog.c
+++ b/hw/watchdog/watchdog.c
@@ -102,17 +102,17 @@ void watchdog_perform_action(void)
{
switch (watchdog_action) {
case WATCHDOG_ACTION_RESET: /* same as 'system_reset' in monitor */
- qapi_event_send_watchdog(WATCHDOG_ACTION_RESET, &error_abort);
+ qapi_event_send_watchdog(WATCHDOG_ACTION_RESET);
qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
break;
case WATCHDOG_ACTION_SHUTDOWN: /* same as 'system_powerdown' in monitor */
- qapi_event_send_watchdog(WATCHDOG_ACTION_SHUTDOWN, &error_abort);
+ qapi_event_send_watchdog(WATCHDOG_ACTION_SHUTDOWN);
qemu_system_powerdown_request();
break;
case WATCHDOG_ACTION_POWEROFF: /* same as 'quit' command in monitor */
- qapi_event_send_watchdog(WATCHDOG_ACTION_POWEROFF, &error_abort);
+ qapi_event_send_watchdog(WATCHDOG_ACTION_POWEROFF);
exit(0);
case WATCHDOG_ACTION_PAUSE: /* same as 'stop' command in monitor */
@@ -120,22 +120,21 @@ void watchdog_perform_action(void)
* you would get a deadlock. Bypass the problem.
*/
qemu_system_vmstop_request_prepare();
- qapi_event_send_watchdog(WATCHDOG_ACTION_PAUSE, &error_abort);
+ qapi_event_send_watchdog(WATCHDOG_ACTION_PAUSE);
qemu_system_vmstop_request(RUN_STATE_WATCHDOG);
break;
case WATCHDOG_ACTION_DEBUG:
- qapi_event_send_watchdog(WATCHDOG_ACTION_DEBUG, &error_abort);
+ qapi_event_send_watchdog(WATCHDOG_ACTION_DEBUG);
fprintf(stderr, "watchdog: timer fired\n");
break;
case WATCHDOG_ACTION_NONE:
- qapi_event_send_watchdog(WATCHDOG_ACTION_NONE, &error_abort);
+ qapi_event_send_watchdog(WATCHDOG_ACTION_NONE);
break;
case WATCHDOG_ACTION_INJECT_NMI:
- qapi_event_send_watchdog(WATCHDOG_ACTION_INJECT_NMI,
- &error_abort);
+ qapi_event_send_watchdog(WATCHDOG_ACTION_INJECT_NMI);
nmi_monitor_handle(0, NULL);
break;
diff --git a/include/qapi/qmp-event.h b/include/qapi/qmp-event.h
index 0c87ad833e..23e588ccf8 100644
--- a/include/qapi/qmp-event.h
+++ b/include/qapi/qmp-event.h
@@ -14,8 +14,7 @@
#ifndef QMP_EVENT_H
#define QMP_EVENT_H
-
-typedef void (*QMPEventFuncEmit)(unsigned event, QDict *dict, Error **errp);
+typedef void (*QMPEventFuncEmit)(unsigned event, QDict *dict);
void qmp_event_set_func_emit(QMPEventFuncEmit emit);
diff --git a/job.c b/job.c
index e36ebaafd8..b9ebd1c091 100644
--- a/job.c
+++ b/job.c
@@ -174,7 +174,7 @@ static void job_state_transition(Job *job, JobStatus s1)
job->status = s1;
if (!job_is_internal(job) && s1 != s0) {
- qapi_event_send_job_status_change(job->id, job->status, &error_abort);
+ qapi_event_send_job_status_change(job->id, job->status);
}
}
diff --git a/migration/migration.c b/migration/migration.c
index 4b316ec343..05d0a7296a 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -204,7 +204,7 @@ void migration_incoming_state_destroy(void)
static void migrate_generate_event(int new_state)
{
if (migrate_use_events()) {
- qapi_event_send_migration(new_state, &error_abort);
+ qapi_event_send_migration(new_state);
}
}
@@ -302,7 +302,7 @@ void qemu_start_incoming_migration(const char *uri, Error **errp)
{
const char *p;
- qapi_event_send_migration(MIGRATION_STATUS_SETUP, &error_abort);
+ qapi_event_send_migration(MIGRATION_STATUS_SETUP);
if (!strcmp(uri, "defer")) {
deferred_incoming_migration(errp);
} else if (strstart(uri, "tcp:", &p)) {
diff --git a/migration/ram.c b/migration/ram.c
index 79c89425a3..f6fd8e5e09 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1670,7 +1670,7 @@ static void migration_bitmap_sync(RAMState *rs)
rs->bytes_xfer_prev = bytes_xfer_now;
}
if (migrate_use_events()) {
- qapi_event_send_migration_pass(ram_counters.dirty_sync_count, NULL);
+ qapi_event_send_migration_pass(ram_counters.dirty_sync_count);
}
}
diff --git a/monitor.c b/monitor.c
index cf3b629cf7..3b90c9eb5f 100644
--- a/monitor.c
+++ b/monitor.c
@@ -589,7 +589,7 @@ monitor_qapi_event_queue_no_reenter(QAPIEvent event, QDict *qdict)
}
static void
-monitor_qapi_event_queue(QAPIEvent event, QDict *qdict, Error **errp)
+monitor_qapi_event_queue(QAPIEvent event, QDict *qdict)
{
/*
* monitor_qapi_event_queue_no_reenter() is not reentrant: it
@@ -4215,8 +4215,7 @@ static void handle_qmp_command(void *opaque, QObject *req, Error *err)
* that command was dropped.
*/
qapi_event_send_command_dropped(id,
- COMMAND_DROP_REASON_QUEUE_FULL,
- &error_abort);
+ COMMAND_DROP_REASON_QUEUE_FULL);
qmp_request_free(req_obj);
return;
}
diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py
index 764ef177ab..2ed7902424 100644
--- a/scripts/qapi/events.py
+++ b/scripts/qapi/events.py
@@ -18,7 +18,7 @@ from qapi.common import *
def build_event_send_proto(name, arg_type, boxed):
return 'void qapi_event_send_%(c_name)s(%(param)s)' % {
'c_name': c_name(name.lower()),
- 'param': build_params(arg_type, boxed, 'Error **errp')}
+ 'param': build_params(arg_type, boxed)}
def gen_event_send_decl(name, arg_type, boxed):
@@ -70,7 +70,6 @@ def gen_event_send(name, arg_type, boxed, event_enum_name):
%(proto)s
{
QDict *qmp;
- Error *err = NULL;
QMPEventFuncEmit emit;
''',
proto=build_event_send_proto(name, arg_type, boxed))
@@ -103,45 +102,35 @@ def gen_event_send(name, arg_type, boxed, event_enum_name):
''')
if not arg_type.is_implicit():
ret += mcgen('''
- visit_type_%(c_name)s(v, "%(name)s", &arg, &err);
+ visit_type_%(c_name)s(v, "%(name)s", &arg, &error_abort);
''',
name=name, c_name=arg_type.c_name())
else:
ret += mcgen('''
- visit_start_struct(v, "%(name)s", NULL, 0, &err);
- if (err) {
- goto out;
- }
- visit_type_%(c_name)s_members(v, ¶m, &err);
- if (!err) {
- visit_check_struct(v, &err);
- }
+ visit_start_struct(v, "%(name)s", NULL, 0, &error_abort);
+ visit_type_%(c_name)s_members(v, ¶m, &error_abort);
+ visit_check_struct(v, &error_abort);
visit_end_struct(v, NULL);
''',
name=name, c_name=arg_type.c_name())
ret += mcgen('''
- if (err) {
- goto out;
- }
visit_complete(v, &obj);
qdict_put_obj(qmp, "data", obj);
''')
ret += mcgen('''
- emit(%(c_enum)s, qmp, &err);
+ emit(%(c_enum)s, qmp);
''',
c_enum=c_enum_const(event_enum_name, name))
if arg_type and not arg_type.is_empty():
ret += mcgen('''
-out:
visit_free(v);
''')
ret += mcgen('''
- error_propagate(errp, err);
qobject_unref(qmp);
}
''')
diff --git a/scsi/pr-manager-helper.c b/scsi/pr-manager-helper.c
index 3027dde60d..438380fced 100644
--- a/scsi/pr-manager-helper.c
+++ b/scsi/pr-manager-helper.c
@@ -44,8 +44,7 @@ static void pr_manager_send_status_changed_event(PRManagerHelper *pr_mgr)
char *id = object_get_canonical_path_component(OBJECT(pr_mgr));
if (id) {
- qapi_event_send_pr_manager_status_changed(id, !!pr_mgr->ioc,
- &error_abort);
+ qapi_event_send_pr_manager_status_changed(id, !!pr_mgr->ioc);
g_free(id);
}
}
diff --git a/tests/test-qmp-event.c b/tests/test-qmp-event.c
index 8677094ad1..9cddd72adb 100644
--- a/tests/test-qmp-event.c
+++ b/tests/test-qmp-event.c
@@ -95,7 +95,7 @@ static bool qdict_cmp_simple(QDict *a, QDict *b)
/* This function is hooked as final emit function, which can verify the
correctness. */
-static void event_test_emit(test_QAPIEvent event, QDict *d, Error **errp)
+static void event_test_emit(test_QAPIEvent event, QDict *d)
{
QDict *t;
int64_t s, ms;
@@ -156,7 +156,7 @@ static void test_event_a(TestEventData *data,
QDict *d;
d = data->expect;
qdict_put_str(d, "event", "EVENT_A");
- qapi_event_send_event_a(&error_abort);
+ qapi_event_send_event_a();
}
static void test_event_b(TestEventData *data,
@@ -165,7 +165,7 @@ static void test_event_b(TestEventData *data,
QDict *d;
d = data->expect;
qdict_put_str(d, "event", "EVENT_B");
- qapi_event_send_event_b(&error_abort);
+ qapi_event_send_event_b();
}
static void test_event_c(TestEventData *data,
@@ -191,7 +191,7 @@ static void test_event_c(TestEventData *data,
qdict_put_str(d, "event", "EVENT_C");
qdict_put(d, "data", d_data);
- qapi_event_send_event_c(true, 1, true, &b, "test2", &error_abort);
+ qapi_event_send_event_c(true, 1, true, &b, "test2");
g_free(b.string);
}
@@ -233,8 +233,7 @@ static void test_event_d(TestEventData *data,
qdict_put_str(d, "event", "EVENT_D");
qdict_put(d, "data", d_data);
- qapi_event_send_event_d(&a, "test3", false, NULL, true, ENUM_ONE_VALUE3,
- &error_abort);
+ qapi_event_send_event_d(&a, "test3", false, NULL, true, ENUM_ONE_VALUE3);
g_free(struct1.string);
g_free(a.string);
diff --git a/ui/spice-core.c b/ui/spice-core.c
index f8c0878529..a4fbbc3898 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -218,8 +218,7 @@ static void channel_event(int event, SpiceChannelEventInfo *info)
switch (event) {
case SPICE_CHANNEL_EVENT_CONNECTED:
qapi_event_send_spice_connected(qapi_SpiceServerInfo_base(server),
- qapi_SpiceChannel_base(client),
- &error_abort);
+ qapi_SpiceChannel_base(client));
break;
case SPICE_CHANNEL_EVENT_INITIALIZED:
if (auth) {
@@ -228,13 +227,12 @@ static void channel_event(int event, SpiceChannelEventInfo *info)
}
add_channel_info(client, info);
channel_list_add(info);
- qapi_event_send_spice_initialized(server, client, &error_abort);
+ qapi_event_send_spice_initialized(server, client);
break;
case SPICE_CHANNEL_EVENT_DISCONNECTED:
channel_list_del(info);
qapi_event_send_spice_disconnected(qapi_SpiceServerInfo_base(server),
- qapi_SpiceChannel_base(client),
- &error_abort);
+ qapi_SpiceChannel_base(client));
break;
default:
break;
@@ -287,7 +285,7 @@ static void migrate_connect_complete_cb(SpiceMigrateInstance *sin)
static void migrate_end_complete_cb(SpiceMigrateInstance *sin)
{
- qapi_event_send_spice_migrate_completed(&error_abort);
+ qapi_event_send_spice_migrate_completed();
spice_migration_completed = true;
}
diff --git a/ui/vnc.c b/ui/vnc.c
index ccb1335d86..916a16d312 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -296,14 +296,13 @@ static void vnc_qmp_event(VncState *vs, QAPIEvent event)
switch (event) {
case QAPI_EVENT_VNC_CONNECTED:
- qapi_event_send_vnc_connected(si, qapi_VncClientInfo_base(vs->info),
- &error_abort);
+ qapi_event_send_vnc_connected(si, qapi_VncClientInfo_base(vs->info));
break;
case QAPI_EVENT_VNC_INITIALIZED:
- qapi_event_send_vnc_initialized(si, vs->info, &error_abort);
+ qapi_event_send_vnc_initialized(si, vs->info);
break;
case QAPI_EVENT_VNC_DISCONNECTED:
- qapi_event_send_vnc_disconnected(si, vs->info, &error_abort);
+ qapi_event_send_vnc_disconnected(si, vs->info);
break;
default:
break;
diff --git a/vl.c b/vl.c
index 5ba06adf78..10dd690e30 100644
--- a/vl.c
+++ b/vl.c
@@ -1647,8 +1647,7 @@ void qemu_system_reset(ShutdownCause reason)
qemu_devices_reset();
}
if (reason != SHUTDOWN_CAUSE_SUBSYSTEM_RESET) {
- qapi_event_send_reset(shutdown_caused_by_guest(reason),
- &error_abort);
+ qapi_event_send_reset(shutdown_caused_by_guest(reason));
}
cpu_synchronize_all_post_reset();
}
@@ -1661,11 +1660,11 @@ void qemu_system_guest_panicked(GuestPanicInformation *info)
current_cpu->crash_occurred = true;
}
qapi_event_send_guest_panicked(GUEST_PANIC_ACTION_PAUSE,
- !!info, info, &error_abort);
+ !!info, info);
vm_stop(RUN_STATE_GUEST_PANICKED);
if (!no_shutdown) {
qapi_event_send_guest_panicked(GUEST_PANIC_ACTION_POWEROFF,
- !!info, info, &error_abort);
+ !!info, info);
qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_PANIC);
}
@@ -1706,7 +1705,7 @@ static void qemu_system_suspend(void)
pause_all_vcpus();
notifier_list_notify(&suspend_notifiers, NULL);
runstate_set(RUN_STATE_SUSPENDED);
- qapi_event_send_suspend(&error_abort);
+ qapi_event_send_suspend();
}
void qemu_system_suspend_request(void)
@@ -1776,7 +1775,7 @@ void qemu_system_shutdown_request(ShutdownCause reason)
static void qemu_system_powerdown(void)
{
- qapi_event_send_powerdown(&error_abort);
+ qapi_event_send_powerdown();
notifier_list_notify(&powerdown_notifiers, NULL);
}
@@ -1819,8 +1818,7 @@ static bool main_loop_should_exit(void)
request = qemu_shutdown_requested();
if (request) {
qemu_kill_report();
- qapi_event_send_shutdown(shutdown_caused_by_guest(request),
- &error_abort);
+ qapi_event_send_shutdown(shutdown_caused_by_guest(request));
if (no_shutdown) {
vm_stop(RUN_STATE_SHUTDOWN);
} else {
@@ -1843,7 +1841,7 @@ static bool main_loop_should_exit(void)
notifier_list_notify(&wakeup_notifiers, &wakeup_reason);
wakeup_reason = QEMU_WAKEUP_REASON_NONE;
resume_all_vcpus();
- qapi_event_send_wakeup(&error_abort);
+ qapi_event_send_wakeup();
}
if (qemu_powerdown_requested()) {
qemu_system_powerdown();
--
2.17.1
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [Qemu-devel] [PATCH v7 3/7] monitor: suspend monitor instead of send CMD_DROP
2018-09-03 4:31 [Qemu-devel] [PATCH v7 0/7] monitor: enable OOB by default Peter Xu
2018-09-03 4:31 ` [Qemu-devel] [PATCH v7 1/7] qapi: Fix build_params() for empty parameter list Peter Xu
2018-09-03 4:31 ` [Qemu-devel] [PATCH v7 2/7] qapi: Drop qapi_event_send_FOO()'s Error ** argument Peter Xu
@ 2018-09-03 4:31 ` Peter Xu
2018-09-03 7:38 ` Markus Armbruster
2018-09-03 4:31 ` [Qemu-devel] [PATCH v7 4/7] qapi: remove COMMAND_DROPPED event Peter Xu
` (4 subsequent siblings)
7 siblings, 1 reply; 28+ messages in thread
From: Peter Xu @ 2018-09-03 4:31 UTC (permalink / raw)
To: qemu-devel
Cc: Eric Blake, Marc-André Lureau, Daniel P . Berrange,
Markus Armbruster, Dr . David Alan Gilbert, peterx
When we received too many qmp commands, previously we'll send
COMMAND_DROPPED events to monitors, then we'll drop the requests. Now
instead of dropping the command we stop reading from input when we
notice the queue is getting full. Note that here since we removed the
need_resume flag we need to be _very_ careful on the suspend/resume
paring on the conditions since unmatched condition checks will hang
death the monitor. Meanwhile, now we will need to read the queue length
to decide whether we'll need to resume the monitor so we need to take
the queue lock again even after popping from it.
Signed-off-by: Peter Xu <peterx@redhat.com>
---
monitor.c | 33 +++++++++++++++------------------
1 file changed, 15 insertions(+), 18 deletions(-)
diff --git a/monitor.c b/monitor.c
index 3b90c9eb5f..9e6cad2af6 100644
--- a/monitor.c
+++ b/monitor.c
@@ -89,6 +89,8 @@
#include "hw/s390x/storage-attributes.h"
#endif
+#define QMP_REQ_QUEUE_LEN_MAX (8)
+
/*
* Supported types:
*
@@ -4124,29 +4126,33 @@ static QMPRequest *monitor_qmp_requests_pop_any(void)
static void monitor_qmp_bh_dispatcher(void *data)
{
QMPRequest *req_obj = monitor_qmp_requests_pop_any();
+ Monitor *mon;
QDict *rsp;
bool need_resume;
if (!req_obj) {
return;
}
-
+ mon = req_obj->mon;
/* qmp_oob_enabled() might change after "qmp_capabilities" */
- need_resume = !qmp_oob_enabled(req_obj->mon);
+ qemu_mutex_lock(&mon->qmp.qmp_queue_lock);
+ need_resume = !qmp_oob_enabled(req_obj->mon) ||
+ mon->qmp.qmp_requests->length == QMP_REQ_QUEUE_LEN_MAX - 1;
+ qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);
if (req_obj->req) {
trace_monitor_qmp_cmd_in_band(qobject_get_try_str(req_obj->id) ?: "");
- monitor_qmp_dispatch(req_obj->mon, req_obj->req, req_obj->id);
+ monitor_qmp_dispatch(mon, req_obj->req, req_obj->id);
} else {
assert(req_obj->err);
rsp = qmp_error_response(req_obj->err);
req_obj->err = NULL;
- monitor_qmp_respond(req_obj->mon, rsp, NULL);
+ monitor_qmp_respond(mon, rsp, NULL);
qobject_unref(rsp);
}
if (need_resume) {
/* Pairs with the monitor_suspend() in handle_qmp_command() */
- monitor_resume(req_obj->mon);
+ monitor_resume(mon);
}
qmp_request_free(req_obj);
@@ -4154,8 +4160,6 @@ static void monitor_qmp_bh_dispatcher(void *data)
qemu_bh_schedule(qmp_dispatcher_bh);
}
-#define QMP_REQ_QUEUE_LEN_MAX (8)
-
static void handle_qmp_command(void *opaque, QObject *req, Error *err)
{
Monitor *mon = opaque;
@@ -4205,19 +4209,12 @@ static void handle_qmp_command(void *opaque, QObject *req, Error *err)
if (!qmp_oob_enabled(mon)) {
monitor_suspend(mon);
} else {
- /* Drop the request if queue is full. */
- if (mon->qmp.qmp_requests->length >= QMP_REQ_QUEUE_LEN_MAX) {
- qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);
+ if (mon->qmp.qmp_requests->length == QMP_REQ_QUEUE_LEN_MAX - 1) {
/*
- * FIXME @id's scope is just @mon, and broadcasting it is
- * wrong. If another monitor's client has a command with
- * the same ID in flight, the event will incorrectly claim
- * that command was dropped.
+ * If this is _exactly_ the last request that we can
+ * queue, we suspend the monitor right now.
*/
- qapi_event_send_command_dropped(id,
- COMMAND_DROP_REASON_QUEUE_FULL);
- qmp_request_free(req_obj);
- return;
+ monitor_suspend(mon);
}
}
--
2.17.1
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [Qemu-devel] [PATCH v7 4/7] qapi: remove COMMAND_DROPPED event
2018-09-03 4:31 [Qemu-devel] [PATCH v7 0/7] monitor: enable OOB by default Peter Xu
` (2 preceding siblings ...)
2018-09-03 4:31 ` [Qemu-devel] [PATCH v7 3/7] monitor: suspend monitor instead of send CMD_DROP Peter Xu
@ 2018-09-03 4:31 ` Peter Xu
2018-09-03 7:49 ` Markus Armbruster
2018-09-03 4:31 ` [Qemu-devel] [PATCH v7 5/7] monitor: remove "x-oob", turn oob on by default Peter Xu
` (3 subsequent siblings)
7 siblings, 1 reply; 28+ messages in thread
From: Peter Xu @ 2018-09-03 4:31 UTC (permalink / raw)
To: qemu-devel
Cc: Eric Blake, Marc-André Lureau, Daniel P . Berrange,
Markus Armbruster, Dr . David Alan Gilbert, peterx
Now it was not used any more, drop it. We can still do that since
out-of-band is still experimental, and this event is only used when
out-of-band is enabled.
Signed-off-by: Peter Xu <peterx@redhat.com>
---
docs/interop/qmp-spec.txt | 5 +++--
qapi/misc.json | 40 ---------------------------------------
2 files changed, 3 insertions(+), 42 deletions(-)
diff --git a/docs/interop/qmp-spec.txt b/docs/interop/qmp-spec.txt
index 8f7da0245d..67e44a8120 100644
--- a/docs/interop/qmp-spec.txt
+++ b/docs/interop/qmp-spec.txt
@@ -130,8 +130,9 @@ to pass "id" with out-of-band commands. Passing it with all commands
is recommended for clients that accept capability "oob".
If the client sends in-band commands faster than the server can
-execute them, the server will eventually drop commands to limit the
-queue length. The sever sends event COMMAND_DROPPED then.
+execute them, the server will stop reading the requests from the QMP
+channel until the request queue length is reduced to an acceptable
+range.
Only a few commands support out-of-band execution. The ones that do
have "allow-oob": true in output of query-qmp-schema.
diff --git a/qapi/misc.json b/qapi/misc.json
index d450cfef21..2c1a6119bf 100644
--- a/qapi/misc.json
+++ b/qapi/misc.json
@@ -3432,46 +3432,6 @@
##
{ 'command': 'query-sev-capabilities', 'returns': 'SevCapability' }
-##
-# @CommandDropReason:
-#
-# Reasons that caused one command to be dropped.
-#
-# @queue-full: the command queue is full. This can only occur when
-# the client sends a new non-oob command before the
-# response to the previous non-oob command has been
-# received.
-#
-# Since: 2.12
-##
-{ 'enum': 'CommandDropReason',
- 'data': [ 'queue-full' ] }
-
-##
-# @COMMAND_DROPPED:
-#
-# Emitted when a command is dropped due to some reason. Commands can
-# only be dropped when the oob capability is enabled.
-#
-# @id: The dropped command's "id" field.
-# FIXME Broken by design. Events are broadcast to all monitors. If
-# another monitor's client has a command with the same ID in flight,
-# the event will incorrectly claim that command was dropped.
-#
-# @reason: The reason why the command is dropped.
-#
-# Since: 2.12
-#
-# Example:
-#
-# { "event": "COMMAND_DROPPED",
-# "data": {"result": {"id": "libvirt-102",
-# "reason": "queue-full" } } }
-#
-##
-{ 'event': 'COMMAND_DROPPED' ,
- 'data': { 'id': 'any', 'reason': 'CommandDropReason' } }
-
##
# @set-numa-node:
#
--
2.17.1
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [Qemu-devel] [PATCH v7 5/7] monitor: remove "x-oob", turn oob on by default
2018-09-03 4:31 [Qemu-devel] [PATCH v7 0/7] monitor: enable OOB by default Peter Xu
` (3 preceding siblings ...)
2018-09-03 4:31 ` [Qemu-devel] [PATCH v7 4/7] qapi: remove COMMAND_DROPPED event Peter Xu
@ 2018-09-03 4:31 ` Peter Xu
2018-09-03 4:31 ` [Qemu-devel] [PATCH v7 6/7] Revert "tests: Add parameter to qtest_init_without_qmp_handshake" Peter Xu
` (2 subsequent siblings)
7 siblings, 0 replies; 28+ messages in thread
From: Peter Xu @ 2018-09-03 4:31 UTC (permalink / raw)
To: qemu-devel
Cc: Eric Blake, Marc-André Lureau, Daniel P . Berrange,
Markus Armbruster, Dr . David Alan Gilbert, peterx
OOB commands were introduced in commit cf869d53172. Unfortunately, we
ran into a regression, and had to disable them by default for 2.12
(commit be933ffc23).
http://lists.gnu.org/archive/html/qemu-devel/2018-03/msg06231.html
The regression has since been fixed (commit 951702f39c7 "monitor: bind
dispatch bh to iohandler context"). Time to re-enable OOB.
This patch partly reverts be933ffc23 (monitor: new parameter "x-oob"),
and turns it on again for non-MUX QMPs. Note that we can't enable
Out-Of-Band for monitors with MUX-typed chardev backends, because not
all the chardev frontends can run without main thread, or can run in
multiple threads.
Some trivial touch-up in the test code is required to make sure qmp-test
won't break.
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
include/monitor/monitor.h | 1 -
monitor.c | 22 ++++++----------------
tests/libqtest.c | 2 +-
tests/qmp-test.c | 2 +-
vl.c | 5 -----
5 files changed, 8 insertions(+), 24 deletions(-)
diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
index 2ef5e04b37..00ded7972c 100644
--- a/include/monitor/monitor.h
+++ b/include/monitor/monitor.h
@@ -13,7 +13,6 @@ extern __thread Monitor *cur_mon;
#define MONITOR_USE_READLINE 0x02
#define MONITOR_USE_CONTROL 0x04
#define MONITOR_USE_PRETTY 0x08
-#define MONITOR_USE_OOB 0x10
bool monitor_cur_is_qmp(void);
diff --git a/monitor.c b/monitor.c
index 9e6cad2af6..c2fd742f91 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4529,19 +4529,12 @@ void monitor_init(Chardev *chr, int flags)
{
Monitor *mon = g_malloc(sizeof(*mon));
bool use_readline = flags & MONITOR_USE_READLINE;
- bool use_oob = flags & MONITOR_USE_OOB;
-
- if (use_oob) {
- if (CHARDEV_IS_MUX(chr)) {
- error_report("Monitor out-of-band is not supported with "
- "MUX typed chardev backend");
- exit(1);
- }
- if (use_readline) {
- error_report("Monitor out-of-band is only supported by QMP");
- exit(1);
- }
- }
+ /*
+ * Note: we can't enable Out-Of-Band for monitors with MUX-typed
+ * chardev backends, because not all the chardev frontends can run
+ * without main thread, or can run in multiple threads.
+ */
+ bool use_oob = (flags & MONITOR_USE_CONTROL) && !CHARDEV_IS_MUX(chr);
monitor_data_init(mon, false, use_oob);
@@ -4631,9 +4624,6 @@ QemuOptsList qemu_mon_opts = {
},{
.name = "pretty",
.type = QEMU_OPT_BOOL,
- },{
- .name = "x-oob",
- .type = QEMU_OPT_BOOL,
},
{ /* end of list */ }
},
diff --git a/tests/libqtest.c b/tests/libqtest.c
index d635c5bea0..ebd92f22f6 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -231,7 +231,7 @@ QTestState *qtest_init_without_qmp_handshake(bool use_oob,
"-display none "
"%s", qemu_binary, socket_path,
getenv("QTEST_LOG") ? "/dev/fd/2" : "/dev/null",
- qmp_socket_path, use_oob ? ",x-oob=on" : "",
+ qmp_socket_path, "",
extra_args ?: "");
execlp("/bin/sh", "sh", "-c", command, NULL);
exit(1);
diff --git a/tests/qmp-test.c b/tests/qmp-test.c
index 4ae2245484..5302bd07b9 100644
--- a/tests/qmp-test.c
+++ b/tests/qmp-test.c
@@ -143,7 +143,7 @@ static void test_qmp_protocol(void)
g_assert(q);
test_version(qdict_get(q, "version"));
capabilities = qdict_get_qlist(q, "capabilities");
- g_assert(capabilities && qlist_empty(capabilities));
+ g_assert(capabilities);
qobject_unref(resp);
/* Test valid command before handshake */
diff --git a/vl.c b/vl.c
index 10dd690e30..0206f0c512 100644
--- a/vl.c
+++ b/vl.c
@@ -2323,11 +2323,6 @@ static int mon_init_func(void *opaque, QemuOpts *opts, Error **errp)
if (qemu_opt_get_bool(opts, "pretty", 0))
flags |= MONITOR_USE_PRETTY;
- /* OOB is off by default */
- if (qemu_opt_get_bool(opts, "x-oob", 0)) {
- flags |= MONITOR_USE_OOB;
- }
-
chardev = qemu_opt_get(opts, "chardev");
chr = qemu_chr_find(chardev);
if (chr == NULL) {
--
2.17.1
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [Qemu-devel] [PATCH v7 6/7] Revert "tests: Add parameter to qtest_init_without_qmp_handshake"
2018-09-03 4:31 [Qemu-devel] [PATCH v7 0/7] monitor: enable OOB by default Peter Xu
` (4 preceding siblings ...)
2018-09-03 4:31 ` [Qemu-devel] [PATCH v7 5/7] monitor: remove "x-oob", turn oob on by default Peter Xu
@ 2018-09-03 4:31 ` Peter Xu
2018-09-03 4:31 ` [Qemu-devel] [PATCH v7 7/7] tests: add oob functional test for test-qmp-cmds Peter Xu
2018-09-03 5:36 ` [Qemu-devel] [PATCH v7 0/7] monitor: enable OOB by default Markus Armbruster
7 siblings, 0 replies; 28+ messages in thread
From: Peter Xu @ 2018-09-03 4:31 UTC (permalink / raw)
To: qemu-devel
Cc: Eric Blake, Marc-André Lureau, Daniel P . Berrange,
Markus Armbruster, Dr . David Alan Gilbert, peterx
This reverts commit ddee57e0176f6ab53b13c6c97605b62737a8fd7a.
Meanwhile, revert one line from fa198ad9bdef to make sure
qtest_init_without_qmp_handshake() will only pass in one parameter.
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
tests/libqtest.c | 10 ++++------
tests/libqtest.h | 4 +---
tests/qmp-test.c | 4 ++--
3 files changed, 7 insertions(+), 11 deletions(-)
diff --git a/tests/libqtest.c b/tests/libqtest.c
index ebd92f22f6..3c594abbc2 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -191,8 +191,7 @@ static const char *qtest_qemu_binary(void)
return qemu_bin;
}
-QTestState *qtest_init_without_qmp_handshake(bool use_oob,
- const char *extra_args)
+QTestState *qtest_init_without_qmp_handshake(const char *extra_args)
{
QTestState *s;
int sock, qmpsock, i;
@@ -225,13 +224,12 @@ QTestState *qtest_init_without_qmp_handshake(bool use_oob,
command = g_strdup_printf("exec %s "
"-qtest unix:%s,nowait "
"-qtest-log %s "
- "-chardev socket,path=%s,nowait,id=char0 "
- "-mon chardev=char0,mode=control%s "
+ "-qmp unix:%s,nowait "
"-machine accel=qtest "
"-display none "
"%s", qemu_binary, socket_path,
getenv("QTEST_LOG") ? "/dev/fd/2" : "/dev/null",
- qmp_socket_path, "",
+ qmp_socket_path,
extra_args ?: "");
execlp("/bin/sh", "sh", "-c", command, NULL);
exit(1);
@@ -266,7 +264,7 @@ QTestState *qtest_init_without_qmp_handshake(bool use_oob,
QTestState *qtest_init(const char *extra_args)
{
- QTestState *s = qtest_init_without_qmp_handshake(false, extra_args);
+ QTestState *s = qtest_init_without_qmp_handshake(extra_args);
QDict *greeting;
/* Read the QMP greeting and then do the handshake */
diff --git a/tests/libqtest.h b/tests/libqtest.h
index 36d5caecd4..49ffc1ba9f 100644
--- a/tests/libqtest.h
+++ b/tests/libqtest.h
@@ -55,14 +55,12 @@ QTestState *qtest_init(const char *extra_args);
/**
* qtest_init_without_qmp_handshake:
- * @use_oob: true to have the server advertise OOB support
* @extra_args: other arguments to pass to QEMU. CAUTION: these
* arguments are subject to word splitting and shell evaluation.
*
* Returns: #QTestState instance.
*/
-QTestState *qtest_init_without_qmp_handshake(bool use_oob,
- const char *extra_args);
+QTestState *qtest_init_without_qmp_handshake(const char *extra_args);
/**
* qtest_quit:
diff --git a/tests/qmp-test.c b/tests/qmp-test.c
index 5302bd07b9..91a90d1c9d 100644
--- a/tests/qmp-test.c
+++ b/tests/qmp-test.c
@@ -135,7 +135,7 @@ static void test_qmp_protocol(void)
QList *capabilities;
QTestState *qts;
- qts = qtest_init_without_qmp_handshake(false, common_args);
+ qts = qtest_init_without_qmp_handshake(common_args);
/* Test greeting */
resp = qtest_qmp_receive(qts);
@@ -249,7 +249,7 @@ static void test_qmp_oob(void)
QList *capabilities;
QString *qstr;
- qts = qtest_init_without_qmp_handshake(true, common_args);
+ qts = qtest_init_without_qmp_handshake(common_args);
/* Check the greeting message. */
resp = qtest_qmp_receive(qts);
--
2.17.1
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [Qemu-devel] [PATCH v7 7/7] tests: add oob functional test for test-qmp-cmds
2018-09-03 4:31 [Qemu-devel] [PATCH v7 0/7] monitor: enable OOB by default Peter Xu
` (5 preceding siblings ...)
2018-09-03 4:31 ` [Qemu-devel] [PATCH v7 6/7] Revert "tests: Add parameter to qtest_init_without_qmp_handshake" Peter Xu
@ 2018-09-03 4:31 ` Peter Xu
2018-09-03 5:36 ` [Qemu-devel] [PATCH v7 0/7] monitor: enable OOB by default Markus Armbruster
7 siblings, 0 replies; 28+ messages in thread
From: Peter Xu @ 2018-09-03 4:31 UTC (permalink / raw)
To: qemu-devel
Cc: Eric Blake, Marc-André Lureau, Daniel P . Berrange,
Markus Armbruster, Dr . David Alan Gilbert, peterx
Straightforward test just to let the test-qmp-cmds be complete.
Signed-off-by: Peter Xu <peterx@redhat.com>
---
tests/test-qmp-cmds.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/tests/test-qmp-cmds.c b/tests/test-qmp-cmds.c
index ab414fa0c9..23c68c7944 100644
--- a/tests/test-qmp-cmds.c
+++ b/tests/test-qmp-cmds.c
@@ -122,6 +122,21 @@ static void test_dispatch_cmd(void)
qobject_unref(req);
}
+static void test_dispatch_cmd_oob(void)
+{
+ QDict *req = qdict_new();
+ QDict *resp;
+
+ qdict_put_str(req, "exec-oob", "test-flags-command");
+
+ resp = qmp_dispatch(&qmp_commands, QOBJECT(req), true);
+ assert(resp != NULL);
+ assert(!qdict_haskey(resp, "error"));
+
+ qobject_unref(resp);
+ qobject_unref(req);
+}
+
/* test commands that return an error due to invalid parameters */
static void test_dispatch_cmd_failure(void)
{
@@ -287,6 +302,7 @@ int main(int argc, char **argv)
g_test_init(&argc, &argv, NULL);
g_test_add_func("/qmp/dispatch_cmd", test_dispatch_cmd);
+ g_test_add_func("/qmp/dispatch_cmd_oob", test_dispatch_cmd_oob);
g_test_add_func("/qmp/dispatch_cmd_failure", test_dispatch_cmd_failure);
g_test_add_func("/qmp/dispatch_cmd_io", test_dispatch_cmd_io);
g_test_add_func("/qmp/dealloc_types", test_dealloc_types);
--
2.17.1
^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [PATCH v7 0/7] monitor: enable OOB by default
2018-09-03 4:31 [Qemu-devel] [PATCH v7 0/7] monitor: enable OOB by default Peter Xu
` (6 preceding siblings ...)
2018-09-03 4:31 ` [Qemu-devel] [PATCH v7 7/7] tests: add oob functional test for test-qmp-cmds Peter Xu
@ 2018-09-03 5:36 ` Markus Armbruster
7 siblings, 0 replies; 28+ messages in thread
From: Markus Armbruster @ 2018-09-03 5:36 UTC (permalink / raw)
To: Peter Xu; +Cc: qemu-devel, Dr . David Alan Gilbert, Marc-André Lureau
Peter Xu <peterx@redhat.com> writes:
> (this series is based on Markus's monitor-next tree so if patchew
> spits something out with "apply failure" then it's expected)
Easy to avoid with suitable Based: tags in the cover letter:
Based-on: <20180828191048.29806-1-armbru@redhat.com>
Based-on: <20180901111716.1675-1-armbru@redhat.com>
The first one gets you "[PULL 0/6] QAPI patches for 2018-08-28", which
includes your PATCH 1+2. The second one gets you "[PULL 0/6] Monitor
patches for 2018-09-01", which is monitor-next.
I pushed the result for reviewers' convenience:
http://repo.or.cz/qemu/armbru.git/shortlog/refs/heads/peterx-oob
>
> v7:
> - use Markus's commit message for patch "qapi: Drop
> qapi_event_send_FOO()'s Error ** argument" [Markus]
> - update commit message for "qapi: remove COMMAND_DROPPED event" since
> QEMU 3.0 is released [Eric/Dave]
> - rebase to Markus's monitor-next tree:
> http://repo.or.cz/qemu/armbru.git/shortlog/refs/heads/monitor-next
> the patch "monitor: suspend monitor instead of send CMD_DROP"
> re-written since people prefer to drop need_resume flag so now I
> hand-made it. Dropped a few patches since not appliable any more.
>
> Please review. Thanks,
>
> Markus Armbruster (1):
> qapi: Fix build_params() for empty parameter list
>
> Peter Xu (6):
> qapi: Drop qapi_event_send_FOO()'s Error ** argument
> monitor: suspend monitor instead of send CMD_DROP
> qapi: remove COMMAND_DROPPED event
> monitor: remove "x-oob", turn oob on by default
> Revert "tests: Add parameter to qtest_init_without_qmp_handshake"
> tests: add oob functional test for test-qmp-cmds
>
> block/block-backend.c | 8 ++---
> block/qcow2.c | 2 +-
> block/quorum.c | 4 +--
> block/write-threshold.c | 3 +-
> blockjob.c | 13 ++++----
> cpus.c | 8 ++---
> docs/devel/qapi-code-gen.txt | 6 ++--
> docs/interop/qmp-spec.txt | 5 ++--
> dump.c | 3 +-
> hw/acpi/core.c | 2 +-
> hw/acpi/cpu.c | 2 +-
> hw/acpi/memory_hotplug.c | 5 ++--
> hw/char/virtio-console.c | 3 +-
> hw/core/qdev.c | 3 +-
> hw/net/virtio-net.c | 2 +-
> hw/ppc/spapr_rtc.c | 2 +-
> hw/timer/mc146818rtc.c | 2 +-
> hw/virtio/virtio-balloon.c | 3 +-
> hw/watchdog/watchdog.c | 15 +++++-----
> include/monitor/monitor.h | 1 -
> include/qapi/qmp-event.h | 3 +-
> job.c | 2 +-
> migration/migration.c | 4 +--
> migration/ram.c | 2 +-
> monitor.c | 58 ++++++++++++++----------------------
> qapi/misc.json | 40 -------------------------
> scripts/qapi/common.py | 10 +++----
> scripts/qapi/events.py | 23 ++++----------
> scsi/pr-manager-helper.c | 3 +-
> tests/libqtest.c | 10 +++----
> tests/libqtest.h | 4 +--
> tests/qmp-test.c | 6 ++--
> tests/test-qmp-cmds.c | 16 ++++++++++
> tests/test-qmp-event.c | 11 ++++---
> ui/spice-core.c | 10 +++----
> ui/vnc.c | 7 ++---
> vl.c | 21 +++++--------
> 37 files changed, 120 insertions(+), 202 deletions(-)
Diffstat looks friendlier without PATCH 1+2:
docs/interop/qmp-spec.txt | 5 +++--
include/monitor/monitor.h | 1 -
monitor.c | 55 ++++++++++++++++++-----------------------------
qapi/misc.json | 40 ----------------------------------
tests/libqtest.c | 10 ++++-----
tests/libqtest.h | 4 +---
tests/qmp-test.c | 6 +++---
tests/test-qmp-cmds.c | 16 ++++++++++++++
vl.c | 5 -----
9 files changed, 48 insertions(+), 94 deletions(-)
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [PATCH v7 3/7] monitor: suspend monitor instead of send CMD_DROP
2018-09-03 4:31 ` [Qemu-devel] [PATCH v7 3/7] monitor: suspend monitor instead of send CMD_DROP Peter Xu
@ 2018-09-03 7:38 ` Markus Armbruster
2018-09-03 7:56 ` Markus Armbruster
2018-09-03 9:06 ` Peter Xu
0 siblings, 2 replies; 28+ messages in thread
From: Markus Armbruster @ 2018-09-03 7:38 UTC (permalink / raw)
To: Peter Xu; +Cc: qemu-devel, Dr . David Alan Gilbert, Marc-André Lureau
Peter Xu <peterx@redhat.com> writes:
> When we received too many qmp commands, previously we'll send
> COMMAND_DROPPED events to monitors, then we'll drop the requests. Now
> instead of dropping the command we stop reading from input when we
> notice the queue is getting full. Note that here since we removed the
> need_resume flag we need to be _very_ careful on the suspend/resume
> paring on the conditions since unmatched condition checks will hang
> death the monitor. Meanwhile, now we will need to read the queue length
> to decide whether we'll need to resume the monitor so we need to take
> the queue lock again even after popping from it.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
The subject's "send CMD_DROP" fails to highlight the important part:
we're no longer dropping commands. Moreover, the commit message could
do a better job explaining the tradeoffs. Here's my try:
monitor: Suspend monitor instead dropping commands
When a QMP client sends in-band commands more quickly that we can
process them, we can either queue them without limit (QUEUE), drop
commands when the queue is full (DROP), or suspend receiving commands
when the queue is full (SUSPEND). None of them is ideal:
* QUEUE lets a misbehaving client make QEMU eat memory without bounds.
Not such a hot idea.
* With DROP, the client has to cope with dropped in-band commands. To
inform the client, we send a COMMAND_DROPPED event then. The event is
flawed by design in two ways: it's ambiguous (see commit d621cfe0a17),
and it brings back the "eat memory without bounds" problem.
* With SUSPEND, the client has to manage the flow of in-band commands to
keep the monitor available for out-of-band commands.
We currently DROP. Switch to SUSPEND.
Managing the flow of in-band commands to keep the monitor available for
out-of-band commands isn't really hard: just count the number of
"outstanding" in-band commands (commands sent minus replies received),
and if it exceeds the limit, hold back additional ones until it drops
below the limit again.
Note that we need to be careful pairing the suspend with a resume, or
else the monitor will hang, possibly forever.
> ---
> monitor.c | 33 +++++++++++++++------------------
> 1 file changed, 15 insertions(+), 18 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index 3b90c9eb5f..9e6cad2af6 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -89,6 +89,8 @@
> #include "hw/s390x/storage-attributes.h"
> #endif
>
> +#define QMP_REQ_QUEUE_LEN_MAX (8)
> +
Let's drop the parenthesis.
> /*
> * Supported types:
> *
> @@ -4124,29 +4126,33 @@ static QMPRequest *monitor_qmp_requests_pop_any(void)
> static void monitor_qmp_bh_dispatcher(void *data)
> {
> QMPRequest *req_obj = monitor_qmp_requests_pop_any();
> + Monitor *mon;
> QDict *rsp;
> bool need_resume;
>
> if (!req_obj) {
> return;
> }
> -
Let's keep the blank line.
> + mon = req_obj->mon;
> /* qmp_oob_enabled() might change after "qmp_capabilities" */
> - need_resume = !qmp_oob_enabled(req_obj->mon);
> + qemu_mutex_lock(&mon->qmp.qmp_queue_lock);
> + need_resume = !qmp_oob_enabled(req_obj->mon) ||
> + mon->qmp.qmp_requests->length == QMP_REQ_QUEUE_LEN_MAX - 1;
Note for later: this is the condition guarding monitor_resume().
Is this race-free? We have two criticial sections, one in
monitor_qmp_requests_pop_any(), and one here. What if two threads
interleave like this
thread 1 thread 2
------------------------------------------------------------------
monitor_qmp_requests_pop_any()
lock
dequeue
unlock
monitor_qmp_requests_pop_any()
lock
dequeue
unlock
lock
check queue length
unlock
if queue length demands it
monitor_resume()
lock
check queue length
unlock
if queue length demands it
monitor_resume()
Looks racy to me: if we start with the queue full (and the monitor
suspended), both threads' check queue length see length
QMP_REQ_QUEUE_LEN_MAX - 2, and neither thread resumes the monitor.
Oops.
Simplest fix is probably moving the resume into
monitor_qmp_requests_pop_any()'s critical section.
> + qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);
> if (req_obj->req) {
> trace_monitor_qmp_cmd_in_band(qobject_get_try_str(req_obj->id) ?: "");
> - monitor_qmp_dispatch(req_obj->mon, req_obj->req, req_obj->id);
> + monitor_qmp_dispatch(mon, req_obj->req, req_obj->id);
> } else {
> assert(req_obj->err);
> rsp = qmp_error_response(req_obj->err);
> req_obj->err = NULL;
> - monitor_qmp_respond(req_obj->mon, rsp, NULL);
> + monitor_qmp_respond(mon, rsp, NULL);
> qobject_unref(rsp);
> }
>
> if (need_resume) {
> /* Pairs with the monitor_suspend() in handle_qmp_command() */
> - monitor_resume(req_obj->mon);
> + monitor_resume(mon);
> }
> qmp_request_free(req_obj);
The replacement of req_obj->mon by mon makes this change less clear than
it could be. Let's figure out the possible race, and then we'll see
whether we'll still want this replacement.
>
> @@ -4154,8 +4160,6 @@ static void monitor_qmp_bh_dispatcher(void *data)
> qemu_bh_schedule(qmp_dispatcher_bh);
> }
>
> -#define QMP_REQ_QUEUE_LEN_MAX (8)
> -
> static void handle_qmp_command(void *opaque, QObject *req, Error *err)
> {
> Monitor *mon = opaque;
> @@ -4205,19 +4209,12 @@ static void handle_qmp_command(void *opaque, QObject *req, Error *err)
/*
* If OOB is not enabled on the current monitor, we'll emulate the
* old behavior that we won't process the current monitor any more
* until it has responded. This helps make sure that as long as
* OOB is not enabled, the server will never drop any command.
*/
This comment is now stale, we don't drop commands anymore.
> if (!qmp_oob_enabled(mon)) {
> monitor_suspend(mon);
> } else {
> - /* Drop the request if queue is full. */
> - if (mon->qmp.qmp_requests->length >= QMP_REQ_QUEUE_LEN_MAX) {
> - qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);
> + if (mon->qmp.qmp_requests->length == QMP_REQ_QUEUE_LEN_MAX - 1) {
> /*
> - * FIXME @id's scope is just @mon, and broadcasting it is
> - * wrong. If another monitor's client has a command with
> - * the same ID in flight, the event will incorrectly claim
> - * that command was dropped.
> + * If this is _exactly_ the last request that we can
> + * queue, we suspend the monitor right now.
> */
> - qapi_event_send_command_dropped(id,
> - COMMAND_DROP_REASON_QUEUE_FULL);
> - qmp_request_free(req_obj);
> - return;
> + monitor_suspend(mon);
> }
> }
The conditional and its comments look unbalanced. Moreover, the
condition is visually dissimilar to the one guarding the matching
monitor_resume(). What about:
/*
* Suspend the monitor when we can't queue more requests after
* this one. Dequeuing in monitor_qmp_bh_dispatcher() will resume
* it.
* Note that when OOB is disabled, we queue at most one command,
* for backward compatibility.
*/
if (!qmp_oob_enabled(mon) ||
mon->qmp.qmp_requests->length == QMP_REQ_QUEUE_LEN_MAX - 1) {
monitor_suspend(mon);
}
You'll have to update the comment if you move the resume to
monitor_qmp_requests_pop_any().
Next:
/*
* Put the request to the end of queue so that requests will be
* handled in time order. Ownership for req_obj, req, id,
* etc. will be delivered to the handler side.
*/
Suggest asserting mon->qmp.qmp_requests->length < QMP_REQ_QUEUE_LEN_MAX
here.
g_queue_push_tail(mon->qmp.qmp_requests, req_obj);
qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [PATCH v7 4/7] qapi: remove COMMAND_DROPPED event
2018-09-03 4:31 ` [Qemu-devel] [PATCH v7 4/7] qapi: remove COMMAND_DROPPED event Peter Xu
@ 2018-09-03 7:49 ` Markus Armbruster
2018-09-03 10:16 ` Peter Xu
0 siblings, 1 reply; 28+ messages in thread
From: Markus Armbruster @ 2018-09-03 7:49 UTC (permalink / raw)
To: Peter Xu; +Cc: qemu-devel, Dr . David Alan Gilbert, Marc-André Lureau
Peter Xu <peterx@redhat.com> writes:
> Now it was not used any more, drop it. We can still do that since
> out-of-band is still experimental, and this event is only used when
> out-of-band is enabled.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
> docs/interop/qmp-spec.txt | 5 +++--
> qapi/misc.json | 40 ---------------------------------------
> 2 files changed, 3 insertions(+), 42 deletions(-)
>
> diff --git a/docs/interop/qmp-spec.txt b/docs/interop/qmp-spec.txt
> index 8f7da0245d..67e44a8120 100644
> --- a/docs/interop/qmp-spec.txt
> +++ b/docs/interop/qmp-spec.txt
> @@ -130,8 +130,9 @@ to pass "id" with out-of-band commands. Passing it with all commands
> is recommended for clients that accept capability "oob".
>
> If the client sends in-band commands faster than the server can
> -execute them, the server will eventually drop commands to limit the
> -queue length. The sever sends event COMMAND_DROPPED then.
> +execute them, the server will stop reading the requests from the QMP
> +channel until the request queue length is reduced to an acceptable
> +range.
The change described by this documentation update is in the previous
commit. This commit merely cleans up unused code. Instead of moving
the doc update to the previous commit, we can simply squash the two
commits.
Let's add a hint on managing flow of in-band commands to keep the
monitor available for out-of-band commands. Here's my try:
If the client sends in-band commands faster than the server can
execute them, the server will stop reading the requests from the QMP
channel until its request queue length falls below the limit. To
keep the monitor available for out-of-band commands, the client has
to manage the flow of in-band commands.
Of course, anyone trying to put this hint to use will need to know the
actual request queue limit. Options:
* Make it ABI by documenting it here
* Let the client configure it with a suitable command
* Let the introspect it with a suitable command
>
> Only a few commands support out-of-band execution. The ones that do
> have "allow-oob": true in output of query-qmp-schema.
> diff --git a/qapi/misc.json b/qapi/misc.json
> index d450cfef21..2c1a6119bf 100644
> --- a/qapi/misc.json
> +++ b/qapi/misc.json
> @@ -3432,46 +3432,6 @@
> ##
> { 'command': 'query-sev-capabilities', 'returns': 'SevCapability' }
>
> -##
> -# @CommandDropReason:
> -#
> -# Reasons that caused one command to be dropped.
> -#
> -# @queue-full: the command queue is full. This can only occur when
> -# the client sends a new non-oob command before the
> -# response to the previous non-oob command has been
> -# received.
> -#
> -# Since: 2.12
> -##
> -{ 'enum': 'CommandDropReason',
> - 'data': [ 'queue-full' ] }
> -
> -##
> -# @COMMAND_DROPPED:
> -#
> -# Emitted when a command is dropped due to some reason. Commands can
> -# only be dropped when the oob capability is enabled.
> -#
> -# @id: The dropped command's "id" field.
> -# FIXME Broken by design. Events are broadcast to all monitors. If
> -# another monitor's client has a command with the same ID in flight,
> -# the event will incorrectly claim that command was dropped.
> -#
> -# @reason: The reason why the command is dropped.
> -#
> -# Since: 2.12
> -#
> -# Example:
> -#
> -# { "event": "COMMAND_DROPPED",
> -# "data": {"result": {"id": "libvirt-102",
> -# "reason": "queue-full" } } }
> -#
> -##
> -{ 'event': 'COMMAND_DROPPED' ,
> - 'data': { 'id': 'any', 'reason': 'CommandDropReason' } }
> -
> ##
> # @set-numa-node:
> #
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [PATCH v7 3/7] monitor: suspend monitor instead of send CMD_DROP
2018-09-03 7:38 ` Markus Armbruster
@ 2018-09-03 7:56 ` Markus Armbruster
2018-09-03 9:06 ` Peter Xu
1 sibling, 0 replies; 28+ messages in thread
From: Markus Armbruster @ 2018-09-03 7:56 UTC (permalink / raw)
To: Peter Xu; +Cc: qemu-devel, Dr . David Alan Gilbert, Marc-André Lureau
One more thing: we need test coverage for "suspend on full queue, resume
when the logjam clears". qmp-test.c got the building blocks. Something
like this:
send_cmd_that_blocks() eight times
send_oob_cmd_that_fails()
unblock_blocked_cmd()
recv_cmd_id() for the 1st in-band command
recv_cmd_id() for the oob command
unblock_blocked_cmd()
recv_cmd_id() for the 2nd in-band command
... repeat for the remaining six in-band commands ...
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [PATCH v7 3/7] monitor: suspend monitor instead of send CMD_DROP
2018-09-03 7:38 ` Markus Armbruster
2018-09-03 7:56 ` Markus Armbruster
@ 2018-09-03 9:06 ` Peter Xu
2018-09-03 13:16 ` Markus Armbruster
1 sibling, 1 reply; 28+ messages in thread
From: Peter Xu @ 2018-09-03 9:06 UTC (permalink / raw)
To: Markus Armbruster
Cc: qemu-devel, Dr . David Alan Gilbert, Marc-André Lureau
On Mon, Sep 03, 2018 at 09:38:00AM +0200, Markus Armbruster wrote:
> Peter Xu <peterx@redhat.com> writes:
>
> > When we received too many qmp commands, previously we'll send
> > COMMAND_DROPPED events to monitors, then we'll drop the requests. Now
> > instead of dropping the command we stop reading from input when we
> > notice the queue is getting full. Note that here since we removed the
> > need_resume flag we need to be _very_ careful on the suspend/resume
> > paring on the conditions since unmatched condition checks will hang
> > death the monitor. Meanwhile, now we will need to read the queue length
> > to decide whether we'll need to resume the monitor so we need to take
> > the queue lock again even after popping from it.
> >
> > Signed-off-by: Peter Xu <peterx@redhat.com>
>
> The subject's "send CMD_DROP" fails to highlight the important part:
> we're no longer dropping commands. Moreover, the commit message could
> do a better job explaining the tradeoffs. Here's my try:
>
> monitor: Suspend monitor instead dropping commands
>
> When a QMP client sends in-band commands more quickly that we can
> process them, we can either queue them without limit (QUEUE), drop
> commands when the queue is full (DROP), or suspend receiving commands
> when the queue is full (SUSPEND). None of them is ideal:
>
> * QUEUE lets a misbehaving client make QEMU eat memory without bounds.
> Not such a hot idea.
>
> * With DROP, the client has to cope with dropped in-band commands. To
> inform the client, we send a COMMAND_DROPPED event then. The event is
> flawed by design in two ways: it's ambiguous (see commit d621cfe0a17),
> and it brings back the "eat memory without bounds" problem.
>
> * With SUSPEND, the client has to manage the flow of in-band commands to
> keep the monitor available for out-of-band commands.
>
> We currently DROP. Switch to SUSPEND.
>
> Managing the flow of in-band commands to keep the monitor available for
> out-of-band commands isn't really hard: just count the number of
> "outstanding" in-band commands (commands sent minus replies received),
> and if it exceeds the limit, hold back additional ones until it drops
> below the limit again.
(Will the simplest QMP client just block at the write() system call
without notice? Anyway, the SUSPEND is ideal enough to me... :)
>
> Note that we need to be careful pairing the suspend with a resume, or
> else the monitor will hang, possibly forever.
Will take your version, thanks.
>
>
> > ---
> > monitor.c | 33 +++++++++++++++------------------
> > 1 file changed, 15 insertions(+), 18 deletions(-)
> >
> > diff --git a/monitor.c b/monitor.c
> > index 3b90c9eb5f..9e6cad2af6 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -89,6 +89,8 @@
> > #include "hw/s390x/storage-attributes.h"
> > #endif
> >
> > +#define QMP_REQ_QUEUE_LEN_MAX (8)
> > +
>
> Let's drop the parenthesis.
Ok.
>
> > /*
> > * Supported types:
> > *
> > @@ -4124,29 +4126,33 @@ static QMPRequest *monitor_qmp_requests_pop_any(void)
> > static void monitor_qmp_bh_dispatcher(void *data)
> > {
> > QMPRequest *req_obj = monitor_qmp_requests_pop_any();
> > + Monitor *mon;
> > QDict *rsp;
> > bool need_resume;
> >
> > if (!req_obj) {
> > return;
> > }
> > -
>
> Let's keep the blank line.
Ok.
>
> > + mon = req_obj->mon;
> > /* qmp_oob_enabled() might change after "qmp_capabilities" */
> > - need_resume = !qmp_oob_enabled(req_obj->mon);
> > + qemu_mutex_lock(&mon->qmp.qmp_queue_lock);
> > + need_resume = !qmp_oob_enabled(req_obj->mon) ||
> > + mon->qmp.qmp_requests->length == QMP_REQ_QUEUE_LEN_MAX - 1;
>
> Note for later: this is the condition guarding monitor_resume().
>
> Is this race-free? We have two criticial sections, one in
> monitor_qmp_requests_pop_any(), and one here. What if two threads
> interleave like this
>
> thread 1 thread 2
> ------------------------------------------------------------------
> monitor_qmp_requests_pop_any()
> lock
> dequeue
> unlock
> monitor_qmp_requests_pop_any()
> lock
> dequeue
> unlock
> lock
> check queue length
> unlock
> if queue length demands it
> monitor_resume()
> lock
> check queue length
> unlock
> if queue length demands it
> monitor_resume()
>
> Looks racy to me: if we start with the queue full (and the monitor
> suspended), both threads' check queue length see length
> QMP_REQ_QUEUE_LEN_MAX - 2, and neither thread resumes the monitor.
> Oops.
>
> Simplest fix is probably moving the resume into
> monitor_qmp_requests_pop_any()'s critical section.
But we only have one QMP dispatcher, right? So IMHO we can't have two
threads doing monitor_qmp_requests_pop_any() at the same time.
But I fully agree that it'll be nicer to keep them together (e.g. in
case we allow to dispatch two commands some day), but then if you see
it'll be something like the old req_obj->need_resume flag, but we set
it in monitor_qmp_requests_pop_any() instead. If so, I really prefer
we just keep that need_resume flag for per-monitor by dropping
176160ce78 and keep my patch to move that flag into monitor struct
(which I dropped after the rebase of this version).
>
> > + qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);
> > if (req_obj->req) {
> > trace_monitor_qmp_cmd_in_band(qobject_get_try_str(req_obj->id) ?: "");
> > - monitor_qmp_dispatch(req_obj->mon, req_obj->req, req_obj->id);
> > + monitor_qmp_dispatch(mon, req_obj->req, req_obj->id);
> > } else {
> > assert(req_obj->err);
> > rsp = qmp_error_response(req_obj->err);
> > req_obj->err = NULL;
> > - monitor_qmp_respond(req_obj->mon, rsp, NULL);
> > + monitor_qmp_respond(mon, rsp, NULL);
> > qobject_unref(rsp);
> > }
> >
> > if (need_resume) {
> > /* Pairs with the monitor_suspend() in handle_qmp_command() */
> > - monitor_resume(req_obj->mon);
> > + monitor_resume(mon);
> > }
> > qmp_request_free(req_obj);
>
> The replacement of req_obj->mon by mon makes this change less clear than
> it could be. Let's figure out the possible race, and then we'll see
> whether we'll still want this replacement.
Sure.
>
> >
> > @@ -4154,8 +4160,6 @@ static void monitor_qmp_bh_dispatcher(void *data)
> > qemu_bh_schedule(qmp_dispatcher_bh);
> > }
> >
> > -#define QMP_REQ_QUEUE_LEN_MAX (8)
> > -
> > static void handle_qmp_command(void *opaque, QObject *req, Error *err)
> > {
> > Monitor *mon = opaque;
> > @@ -4205,19 +4209,12 @@ static void handle_qmp_command(void *opaque, QObject *req, Error *err)
> /*
> * If OOB is not enabled on the current monitor, we'll emulate the
> * old behavior that we won't process the current monitor any more
> * until it has responded. This helps make sure that as long as
> * OOB is not enabled, the server will never drop any command.
> */
>
> This comment is now stale, we don't drop commands anymore.
AFAIU it's describing [1] below but nothing related to COMMAND_DROP?
>
> > if (!qmp_oob_enabled(mon)) {
> > monitor_suspend(mon);
[1]
> > } else {
> > - /* Drop the request if queue is full. */
> > - if (mon->qmp.qmp_requests->length >= QMP_REQ_QUEUE_LEN_MAX) {
> > - qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);
> > + if (mon->qmp.qmp_requests->length == QMP_REQ_QUEUE_LEN_MAX - 1) {
> > /*
> > - * FIXME @id's scope is just @mon, and broadcasting it is
> > - * wrong. If another monitor's client has a command with
> > - * the same ID in flight, the event will incorrectly claim
> > - * that command was dropped.
> > + * If this is _exactly_ the last request that we can
> > + * queue, we suspend the monitor right now.
> > */
> > - qapi_event_send_command_dropped(id,
> > - COMMAND_DROP_REASON_QUEUE_FULL);
> > - qmp_request_free(req_obj);
> > - return;
> > + monitor_suspend(mon);
> > }
> > }
>
> The conditional and its comments look unbalanced. Moreover, the
> condition is visually dissimilar to the one guarding the matching
> monitor_resume(). What about:
>
> /*
> * Suspend the monitor when we can't queue more requests after
> * this one. Dequeuing in monitor_qmp_bh_dispatcher() will resume
> * it.
> * Note that when OOB is disabled, we queue at most one command,
> * for backward compatibility.
> */
> if (!qmp_oob_enabled(mon) ||
> mon->qmp.qmp_requests->length == QMP_REQ_QUEUE_LEN_MAX - 1) {
> monitor_suspend(mon);
> }
>
> You'll have to update the comment if you move the resume to
> monitor_qmp_requests_pop_any().
>
> Next:
>
> /*
> * Put the request to the end of queue so that requests will be
> * handled in time order. Ownership for req_obj, req, id,
> * etc. will be delivered to the handler side.
> */
>
> Suggest asserting mon->qmp.qmp_requests->length < QMP_REQ_QUEUE_LEN_MAX
> here.
Sure I can do this.
>
> g_queue_push_tail(mon->qmp.qmp_requests, req_obj);
> qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);
Thanks,
--
Peter Xu
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [PATCH v7 4/7] qapi: remove COMMAND_DROPPED event
2018-09-03 7:49 ` Markus Armbruster
@ 2018-09-03 10:16 ` Peter Xu
2018-09-03 13:31 ` Markus Armbruster
0 siblings, 1 reply; 28+ messages in thread
From: Peter Xu @ 2018-09-03 10:16 UTC (permalink / raw)
To: Markus Armbruster
Cc: qemu-devel, Dr . David Alan Gilbert, Marc-André Lureau
On Mon, Sep 03, 2018 at 09:49:13AM +0200, Markus Armbruster wrote:
> Peter Xu <peterx@redhat.com> writes:
>
> > Now it was not used any more, drop it. We can still do that since
> > out-of-band is still experimental, and this event is only used when
> > out-of-band is enabled.
> >
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> > docs/interop/qmp-spec.txt | 5 +++--
> > qapi/misc.json | 40 ---------------------------------------
> > 2 files changed, 3 insertions(+), 42 deletions(-)
> >
> > diff --git a/docs/interop/qmp-spec.txt b/docs/interop/qmp-spec.txt
> > index 8f7da0245d..67e44a8120 100644
> > --- a/docs/interop/qmp-spec.txt
> > +++ b/docs/interop/qmp-spec.txt
> > @@ -130,8 +130,9 @@ to pass "id" with out-of-band commands. Passing it with all commands
> > is recommended for clients that accept capability "oob".
> >
> > If the client sends in-band commands faster than the server can
> > -execute them, the server will eventually drop commands to limit the
> > -queue length. The sever sends event COMMAND_DROPPED then.
> > +execute them, the server will stop reading the requests from the QMP
> > +channel until the request queue length is reduced to an acceptable
> > +range.
>
> The change described by this documentation update is in the previous
> commit. This commit merely cleans up unused code. Instead of moving
> the doc update to the previous commit, we can simply squash the two
> commits.
Sure, I'll squash.
>
> Let's add a hint on managing flow of in-band commands to keep the
> monitor available for out-of-band commands. Here's my try:
>
> If the client sends in-band commands faster than the server can
> execute them, the server will stop reading the requests from the QMP
> channel until its request queue length falls below the limit. To
> keep the monitor available for out-of-band commands, the client has
> to manage the flow of in-band commands.
>
> Of course, anyone trying to put this hint to use will need to know the
> actual request queue limit. Options:
>
> * Make it ABI by documenting it here
>
> * Let the client configure it with a suitable command
>
> * Let the introspect it with a suitable command
I'm not sure whether we should expose this information to the client,
considering that the client can after all detect that "full" by
monitoring the status of its output buffer used (e.g., poll with
POLLOUT) and assuming that should be enough for a client. Or did I
miss anything that you were trying to emphasize but I didn't notice
(since after all you mentioned this too in the other thread)?
Regards,
--
Peter Xu
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [PATCH v7 3/7] monitor: suspend monitor instead of send CMD_DROP
2018-09-03 9:06 ` Peter Xu
@ 2018-09-03 13:16 ` Markus Armbruster
2018-09-04 3:33 ` Peter Xu
0 siblings, 1 reply; 28+ messages in thread
From: Markus Armbruster @ 2018-09-03 13:16 UTC (permalink / raw)
To: Peter Xu; +Cc: Marc-André Lureau, qemu-devel, Dr . David Alan Gilbert
Peter Xu <peterx@redhat.com> writes:
> On Mon, Sep 03, 2018 at 09:38:00AM +0200, Markus Armbruster wrote:
>> Peter Xu <peterx@redhat.com> writes:
>>
>> > When we received too many qmp commands, previously we'll send
>> > COMMAND_DROPPED events to monitors, then we'll drop the requests. Now
>> > instead of dropping the command we stop reading from input when we
>> > notice the queue is getting full. Note that here since we removed the
>> > need_resume flag we need to be _very_ careful on the suspend/resume
>> > paring on the conditions since unmatched condition checks will hang
>> > death the monitor. Meanwhile, now we will need to read the queue length
>> > to decide whether we'll need to resume the monitor so we need to take
>> > the queue lock again even after popping from it.
>> >
>> > Signed-off-by: Peter Xu <peterx@redhat.com>
>>
>> The subject's "send CMD_DROP" fails to highlight the important part:
>> we're no longer dropping commands. Moreover, the commit message could
>> do a better job explaining the tradeoffs. Here's my try:
>>
>> monitor: Suspend monitor instead dropping commands
>>
>> When a QMP client sends in-band commands more quickly that we can
>> process them, we can either queue them without limit (QUEUE), drop
>> commands when the queue is full (DROP), or suspend receiving commands
>> when the queue is full (SUSPEND). None of them is ideal:
>>
>> * QUEUE lets a misbehaving client make QEMU eat memory without bounds.
>> Not such a hot idea.
>>
>> * With DROP, the client has to cope with dropped in-band commands. To
>> inform the client, we send a COMMAND_DROPPED event then. The event is
>> flawed by design in two ways: it's ambiguous (see commit d621cfe0a17),
>> and it brings back the "eat memory without bounds" problem.
>>
>> * With SUSPEND, the client has to manage the flow of in-band commands to
>> keep the monitor available for out-of-band commands.
>>
>> We currently DROP. Switch to SUSPEND.
>>
>> Managing the flow of in-band commands to keep the monitor available for
>> out-of-band commands isn't really hard: just count the number of
>> "outstanding" in-band commands (commands sent minus replies received),
>> and if it exceeds the limit, hold back additional ones until it drops
>> below the limit again.
>
> (Will the simplest QMP client just block at the write() system call
> without notice?
Yes.
When you stop reading from a socket or pipe, the peer eventually can't
write more. "Eventually", because the TCP connection or pipe buffers
some.
A naive client using a blocking write() will block then.
A slightly more sophisticated client using a non-blocking write() has
its write() fail with EAGAIN or EWOULDBLOCK.
In both cases, OOB commands may be stuck in the TCP connection's /
pipe's buffer.
> Anyway, the SUSPEND is ideal enough to me... :)
>
>>
>> Note that we need to be careful pairing the suspend with a resume, or
>> else the monitor will hang, possibly forever.
>
> Will take your version, thanks.
>
>>
>>
>> > ---
>> > monitor.c | 33 +++++++++++++++------------------
>> > 1 file changed, 15 insertions(+), 18 deletions(-)
>> >
>> > diff --git a/monitor.c b/monitor.c
>> > index 3b90c9eb5f..9e6cad2af6 100644
>> > --- a/monitor.c
>> > +++ b/monitor.c
>> > @@ -89,6 +89,8 @@
>> > #include "hw/s390x/storage-attributes.h"
>> > #endif
>> >
>> > +#define QMP_REQ_QUEUE_LEN_MAX (8)
>> > +
>>
>> Let's drop the parenthesis.
>
> Ok.
>
>>
>> > /*
>> > * Supported types:
>> > *
>> > @@ -4124,29 +4126,33 @@ static QMPRequest *monitor_qmp_requests_pop_any(void)
>> > static void monitor_qmp_bh_dispatcher(void *data)
>> > {
>> > QMPRequest *req_obj = monitor_qmp_requests_pop_any();
>> > + Monitor *mon;
>> > QDict *rsp;
>> > bool need_resume;
>> >
>> > if (!req_obj) {
>> > return;
>> > }
>> > -
>>
>> Let's keep the blank line.
>
> Ok.
>
>>
>> > + mon = req_obj->mon;
>> > /* qmp_oob_enabled() might change after "qmp_capabilities" */
>> > - need_resume = !qmp_oob_enabled(req_obj->mon);
>> > + qemu_mutex_lock(&mon->qmp.qmp_queue_lock);
>> > + need_resume = !qmp_oob_enabled(req_obj->mon) ||
>> > + mon->qmp.qmp_requests->length == QMP_REQ_QUEUE_LEN_MAX - 1;
>>
>> Note for later: this is the condition guarding monitor_resume().
>>
>> Is this race-free? We have two criticial sections, one in
>> monitor_qmp_requests_pop_any(), and one here. What if two threads
>> interleave like this
>>
>> thread 1 thread 2
>> ------------------------------------------------------------------
>> monitor_qmp_requests_pop_any()
>> lock
>> dequeue
>> unlock
>> monitor_qmp_requests_pop_any()
>> lock
>> dequeue
>> unlock
>> lock
>> check queue length
>> unlock
>> if queue length demands it
>> monitor_resume()
>> lock
>> check queue length
>> unlock
>> if queue length demands it
>> monitor_resume()
>>
>> Looks racy to me: if we start with the queue full (and the monitor
>> suspended), both threads' check queue length see length
>> QMP_REQ_QUEUE_LEN_MAX - 2, and neither thread resumes the monitor.
>> Oops.
>>
>> Simplest fix is probably moving the resume into
>> monitor_qmp_requests_pop_any()'s critical section.
>
> But we only have one QMP dispatcher, right? So IMHO we can't have two
> threads doing monitor_qmp_requests_pop_any() at the same time.
You're right, we currently run monitor_qmp_bh_dispatcher() only in the
main thread, and a thread can't race with itself. But the main thread
can still race with handle_qmp_command() running in mon_iothread.
main thread mon_iothread
------------------------------------------------------------------
monitor_qmp_requests_pop_any()
lock
dequeue
unlock
lock
if queue length demands it
monitor_suspend()
push onto queue
unlock
lock
check queue length
unlock
if queue length demands it
monitor_resume()
If we start with the queue full (and the monitor suspended), the main
thread first determines it'll need to resume. mon_iothread then fills
the queue again, and suspends the suspended monitor some more. If I
read the code correctly, this increments mon->suspend_cnt from 1 to 2.
Finally, the main thread checks the queue length:
need_resume = !qmp_oob_enabled(req_obj->mon) ||
mon->qmp.qmp_requests->length == QMP_REQ_QUEUE_LEN_MAX - 1;
The yields false, because ->length is now QMP_REQ_QUEUE_LEN_MAX. The
main thread does *not* resume the monitor.
State after this: queue full, mon->suspend_cnt == 2. Bad, but we'll
recover on the dequeue after next: the next dequeue decrements
mon->suspend_cnt to 1 without resuming the monitor, the one after that
will decrement it to 0 and resume the monitor.
However, if handle_qmp_command() runs in this spot often enough to push
mon->suspend_cnt above QMP_REQ_QUEUE_LEN_MAX, the monitor will remain
suspended forever.
I'm teaching you multiprogramming 101 here. The thing that should make
the moderately experienced nose twitch is the anti-pattern
begin critical section
do something
end critical section
begin critical section
if we just did X, state must be Y, so we must now do Z
end critical section
The part "if we just did X, state must be Y" is *wrong*. You have no
idea what the state is, since code running between the two critical
sections may have changed it.
Here,
X = dequeued from a full queue"
Y = "mon->suspend_cnt == 1"
Z = monitor_resume() to resume the monitor
I showed that Y does not hold.
On a slightly more abstract level, this anti-pattern applies:
begin critical section
start messing with invariant
end critical section
// invariant does not hold here, oopsie
begin critical section
finish messing with invariant
end critical section
The invariant here is "monitor suspended exactly when the queue is
full". Your first critical section can make the queue non-full. The
second one resumes. The invariant doesn't hold in between, and all bets
are off.
To maintain the invariant, you *have* to enqueue and suspend in a single
critical section (which you do), *and* dequeue and resume in a single
critical section (which you don't).
> But I fully agree that it'll be nicer to keep them together (e.g. in
> case we allow to dispatch two commands some day), but then if you see
> it'll be something like the old req_obj->need_resume flag, but we set
> it in monitor_qmp_requests_pop_any() instead. If so, I really prefer
> we just keep that need_resume flag for per-monitor by dropping
> 176160ce78 and keep my patch to move that flag into monitor struct
> (which I dropped after the rebase of this version).
Please have another look.
>> > + qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);
>> > if (req_obj->req) {
>> > trace_monitor_qmp_cmd_in_band(qobject_get_try_str(req_obj->id) ?: "");
>> > - monitor_qmp_dispatch(req_obj->mon, req_obj->req, req_obj->id);
>> > + monitor_qmp_dispatch(mon, req_obj->req, req_obj->id);
>> > } else {
>> > assert(req_obj->err);
>> > rsp = qmp_error_response(req_obj->err);
>> > req_obj->err = NULL;
>> > - monitor_qmp_respond(req_obj->mon, rsp, NULL);
>> > + monitor_qmp_respond(mon, rsp, NULL);
>> > qobject_unref(rsp);
>> > }
>> >
>> > if (need_resume) {
>> > /* Pairs with the monitor_suspend() in handle_qmp_command() */
>> > - monitor_resume(req_obj->mon);
>> > + monitor_resume(mon);
>> > }
>> > qmp_request_free(req_obj);
>>
>> The replacement of req_obj->mon by mon makes this change less clear than
>> it could be. Let's figure out the possible race, and then we'll see
>> whether we'll still want this replacement.
>
> Sure.
>
>>
>> >
>> > @@ -4154,8 +4160,6 @@ static void monitor_qmp_bh_dispatcher(void *data)
>> > qemu_bh_schedule(qmp_dispatcher_bh);
>> > }
>> >
>> > -#define QMP_REQ_QUEUE_LEN_MAX (8)
>> > -
>> > static void handle_qmp_command(void *opaque, QObject *req, Error *err)
>> > {
>> > Monitor *mon = opaque;
>> > @@ -4205,19 +4209,12 @@ static void handle_qmp_command(void *opaque, QObject *req, Error *err)
>> /*
>> * If OOB is not enabled on the current monitor, we'll emulate the
>> * old behavior that we won't process the current monitor any more
>> * until it has responded. This helps make sure that as long as
>> * OOB is not enabled, the server will never drop any command.
>> */
>>
>> This comment is now stale, we don't drop commands anymore.
>
> AFAIU it's describing [1] below but nothing related to COMMAND_DROP?
The comment suggests the server can drop commands when OOB is enabled.
This is no longer correct after this patch.
>>
>> > if (!qmp_oob_enabled(mon)) {
>> > monitor_suspend(mon);
>
> [1]
>
>> > } else {
>> > - /* Drop the request if queue is full. */
>> > - if (mon->qmp.qmp_requests->length >= QMP_REQ_QUEUE_LEN_MAX) {
>> > - qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);
>> > + if (mon->qmp.qmp_requests->length == QMP_REQ_QUEUE_LEN_MAX - 1) {
>> > /*
>> > - * FIXME @id's scope is just @mon, and broadcasting it is
>> > - * wrong. If another monitor's client has a command with
>> > - * the same ID in flight, the event will incorrectly claim
>> > - * that command was dropped.
>> > + * If this is _exactly_ the last request that we can
>> > + * queue, we suspend the monitor right now.
>> > */
>> > - qapi_event_send_command_dropped(id,
>> > - COMMAND_DROP_REASON_QUEUE_FULL);
>> > - qmp_request_free(req_obj);
>> > - return;
>> > + monitor_suspend(mon);
>> > }
>> > }
>>
>> The conditional and its comments look unbalanced. Moreover, the
>> condition is visually dissimilar to the one guarding the matching
>> monitor_resume(). What about:
>>
>> /*
>> * Suspend the monitor when we can't queue more requests after
>> * this one. Dequeuing in monitor_qmp_bh_dispatcher() will resume
>> * it.
>> * Note that when OOB is disabled, we queue at most one command,
>> * for backward compatibility.
>> */
>> if (!qmp_oob_enabled(mon) ||
>> mon->qmp.qmp_requests->length == QMP_REQ_QUEUE_LEN_MAX - 1) {
>> monitor_suspend(mon);
>> }
>>
>> You'll have to update the comment if you move the resume to
>> monitor_qmp_requests_pop_any().
>>
>> Next:
>>
>> /*
>> * Put the request to the end of queue so that requests will be
>> * handled in time order. Ownership for req_obj, req, id,
>> * etc. will be delivered to the handler side.
>> */
>>
>> Suggest asserting mon->qmp.qmp_requests->length < QMP_REQ_QUEUE_LEN_MAX
>> here.
>
> Sure I can do this.
>
>>
>> g_queue_push_tail(mon->qmp.qmp_requests, req_obj);
>> qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);
>
> Thanks,
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [PATCH v7 4/7] qapi: remove COMMAND_DROPPED event
2018-09-03 10:16 ` Peter Xu
@ 2018-09-03 13:31 ` Markus Armbruster
2018-09-03 14:30 ` Eric Blake
0 siblings, 1 reply; 28+ messages in thread
From: Markus Armbruster @ 2018-09-03 13:31 UTC (permalink / raw)
To: Peter Xu
Cc: Marc-André Lureau, qemu-devel, Dr . David Alan Gilbert,
Eric Blake, Daniel P. Berrangé
Cc: Eric & Daniel for the libvirt point of view.
Peter Xu <peterx@redhat.com> writes:
> On Mon, Sep 03, 2018 at 09:49:13AM +0200, Markus Armbruster wrote:
>> Peter Xu <peterx@redhat.com> writes:
>>
>> > Now it was not used any more, drop it. We can still do that since
>> > out-of-band is still experimental, and this event is only used when
>> > out-of-band is enabled.
>> >
>> > Signed-off-by: Peter Xu <peterx@redhat.com>
>> > ---
>> > docs/interop/qmp-spec.txt | 5 +++--
>> > qapi/misc.json | 40 ---------------------------------------
>> > 2 files changed, 3 insertions(+), 42 deletions(-)
>> >
>> > diff --git a/docs/interop/qmp-spec.txt b/docs/interop/qmp-spec.txt
>> > index 8f7da0245d..67e44a8120 100644
>> > --- a/docs/interop/qmp-spec.txt
>> > +++ b/docs/interop/qmp-spec.txt
>> > @@ -130,8 +130,9 @@ to pass "id" with out-of-band commands. Passing it with all commands
>> > is recommended for clients that accept capability "oob".
>> >
>> > If the client sends in-band commands faster than the server can
>> > -execute them, the server will eventually drop commands to limit the
>> > -queue length. The sever sends event COMMAND_DROPPED then.
>> > +execute them, the server will stop reading the requests from the QMP
>> > +channel until the request queue length is reduced to an acceptable
>> > +range.
>>
>> The change described by this documentation update is in the previous
>> commit. This commit merely cleans up unused code. Instead of moving
>> the doc update to the previous commit, we can simply squash the two
>> commits.
>
> Sure, I'll squash.
>
>>
>> Let's add a hint on managing flow of in-band commands to keep the
>> monitor available for out-of-band commands. Here's my try:
>>
>> If the client sends in-band commands faster than the server can
>> execute them, the server will stop reading the requests from the QMP
>> channel until its request queue length falls below the limit. To
>> keep the monitor available for out-of-band commands, the client has
>> to manage the flow of in-band commands.
>>
>> Of course, anyone trying to put this hint to use will need to know the
>> actual request queue limit. Options:
>>
>> * Make it ABI by documenting it here
>>
>> * Let the client configure it with a suitable command
>>
>> * Let the introspect it with a suitable command
>
> I'm not sure whether we should expose this information to the client,
> considering that the client can after all detect that "full" by
> monitoring the status of its output buffer used (e.g., poll with
> POLLOUT) and assuming that should be enough for a client. Or did I
> miss anything that you were trying to emphasize but I didn't notice
> (since after all you mentioned this too in the other thread)?
By the time the client gets "write would block", OOB has already become
unavailable, and will remain so until the logjam clears sufficiently.
Example:
client sends in-band command #1
QEMU reads and queues
QEMU dequeues in-band command #1
in-band command #1 starts executing, but it's slooow
client sends in-band command #2
QEMU reads and queues
...
client sends in-band command #8
QEMU reads, queues and suspends the monitor
client sends out-of-band command
--> time passes...
in-band command #1 completes, QEMU sends reply
QEMU dequeues in-band command #2, resumes the monitor
in-band command #2 starts executing
QEMU reads and executes out-of-band command
out-of-band command completes, QEMU sends reply
in-band command #2 completes, QEMU sends reply
... same for remaining in-band commands ...
The out-of-band command gets stuck behind the in-band commands.
The client can avoid this by managing the flow of in-band commands: have
no more than 7 in flight, so the monitor never gets suspended.
This is a potentially useful thing to do for clients, isn't it?
Eric, Daniel, is it something libvirt would do?
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [PATCH v7 4/7] qapi: remove COMMAND_DROPPED event
2018-09-03 13:31 ` Markus Armbruster
@ 2018-09-03 14:30 ` Eric Blake
2018-09-03 14:41 ` Daniel P. Berrangé
0 siblings, 1 reply; 28+ messages in thread
From: Eric Blake @ 2018-09-03 14:30 UTC (permalink / raw)
To: Markus Armbruster, Peter Xu
Cc: Marc-André Lureau, qemu-devel, Dr . David Alan Gilbert,
Daniel P. Berrangé
On 09/03/2018 08:31 AM, Markus Armbruster wrote:
> Example:
>
> client sends in-band command #1
> QEMU reads and queues
> QEMU dequeues in-band command #1
> in-band command #1 starts executing, but it's slooow
> client sends in-band command #2
> QEMU reads and queues
> ...
> client sends in-band command #8
> QEMU reads, queues and suspends the monitor
> client sends out-of-band command
> --> time passes...
> in-band command #1 completes, QEMU sends reply
> QEMU dequeues in-band command #2, resumes the monitor
> in-band command #2 starts executing
> QEMU reads and executes out-of-band command
> out-of-band command completes, QEMU sends reply
> in-band command #2 completes, QEMU sends reply
> ... same for remaining in-band commands ...
>
> The out-of-band command gets stuck behind the in-band commands.
>
> The client can avoid this by managing the flow of in-band commands: have
> no more than 7 in flight, so the monitor never gets suspended.
>
> This is a potentially useful thing to do for clients, isn't it?
>
> Eric, Daniel, is it something libvirt would do?
Right now, libvirt serializes commands - it never sends a QMP command
until the previous command's response has been processed. But that may
not help much, since libvirt does not send OOB commands.
I guess when we are designing what libvirt should do, and deciding WHEN
it should send OOB commands, we have the luxury of designing libvirt to
enforce how many in-flight in-band commands it will ever have pending at
once (whether the current 'at most 1', or even if we make it more
parallel to 'at most 7'), so that we can still be ensured that the OOB
command will be processed without being stuck in the queue of suspended
in-band commands. If we never send more than one in-band at a time,
then it's not a concern how deep the qemu queue is; but if we do want
libvirt to start parallel in-band commands, then you are right that
having a way to learn the qemu queue depth is programmatically more
precise than just guessing the maximum depth. But it's also hard to
argue we need that complexity if we don't have an immediate use
envisioned for it.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [PATCH v7 4/7] qapi: remove COMMAND_DROPPED event
2018-09-03 14:30 ` Eric Blake
@ 2018-09-03 14:41 ` Daniel P. Berrangé
2018-09-04 5:30 ` Peter Xu
2018-09-04 6:39 ` Markus Armbruster
0 siblings, 2 replies; 28+ messages in thread
From: Daniel P. Berrangé @ 2018-09-03 14:41 UTC (permalink / raw)
To: Eric Blake
Cc: Markus Armbruster, Peter Xu, Marc-André Lureau, qemu-devel,
Dr . David Alan Gilbert
On Mon, Sep 03, 2018 at 09:30:52AM -0500, Eric Blake wrote:
> On 09/03/2018 08:31 AM, Markus Armbruster wrote:
>
> > Example:
> >
> > client sends in-band command #1
> > QEMU reads and queues
> > QEMU dequeues in-band command #1
> > in-band command #1 starts executing, but it's slooow
> > client sends in-band command #2
> > QEMU reads and queues
> > ...
> > client sends in-band command #8
> > QEMU reads, queues and suspends the monitor
> > client sends out-of-band command
> > --> time passes...
> > in-band command #1 completes, QEMU sends reply
> > QEMU dequeues in-band command #2, resumes the monitor
> > in-band command #2 starts executing
> > QEMU reads and executes out-of-band command
> > out-of-band command completes, QEMU sends reply
> > in-band command #2 completes, QEMU sends reply
> > ... same for remaining in-band commands ...
> >
> > The out-of-band command gets stuck behind the in-band commands.
> >
> > The client can avoid this by managing the flow of in-band commands: have
> > no more than 7 in flight, so the monitor never gets suspended.
> >
> > This is a potentially useful thing to do for clients, isn't it?
> >
> > Eric, Daniel, is it something libvirt would do?
>
> Right now, libvirt serializes commands - it never sends a QMP command until
> the previous command's response has been processed. But that may not help
> much, since libvirt does not send OOB commands.
Note that is not merely due to the QMP monitor restriction either.
Libvirt serializes all its public APIs that can change state of a running
domain. It usually aims to allow read-only APIs to be run in parallel with
APIs that change state.
The exception to the rule right now are some of the migration APIs which
we allow to be invoked to manage the migration process.
> I guess when we are designing what libvirt should do, and deciding WHEN it
> should send OOB commands, we have the luxury of designing libvirt to enforce
> how many in-flight in-band commands it will ever have pending at once
> (whether the current 'at most 1', or even if we make it more parallel to 'at
> most 7'), so that we can still be ensured that the OOB command will be
> processed without being stuck in the queue of suspended in-band commands.
> If we never send more than one in-band at a time, then it's not a concern
> how deep the qemu queue is; but if we do want libvirt to start parallel
> in-band commands, then you are right that having a way to learn the qemu
> queue depth is programmatically more precise than just guessing the maximum
> depth. But it's also hard to argue we need that complexity if we don't have
> an immediate use envisioned for it.
In terms of what libvirt would want to parallelize, I think it is reasonable
to consider any of the query-XXXX commands desirable. Other stuff is likely
to remain serialized from libvirt's side.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [PATCH v7 3/7] monitor: suspend monitor instead of send CMD_DROP
2018-09-03 13:16 ` Markus Armbruster
@ 2018-09-04 3:33 ` Peter Xu
2018-09-04 6:17 ` Markus Armbruster
0 siblings, 1 reply; 28+ messages in thread
From: Peter Xu @ 2018-09-04 3:33 UTC (permalink / raw)
To: Markus Armbruster
Cc: Marc-André Lureau, qemu-devel, Dr . David Alan Gilbert
On Mon, Sep 03, 2018 at 03:16:55PM +0200, Markus Armbruster wrote:
> Peter Xu <peterx@redhat.com> writes:
>
> > On Mon, Sep 03, 2018 at 09:38:00AM +0200, Markus Armbruster wrote:
> >> Peter Xu <peterx@redhat.com> writes:
> >>
> >> > When we received too many qmp commands, previously we'll send
> >> > COMMAND_DROPPED events to monitors, then we'll drop the requests. Now
> >> > instead of dropping the command we stop reading from input when we
> >> > notice the queue is getting full. Note that here since we removed the
> >> > need_resume flag we need to be _very_ careful on the suspend/resume
> >> > paring on the conditions since unmatched condition checks will hang
> >> > death the monitor. Meanwhile, now we will need to read the queue length
> >> > to decide whether we'll need to resume the monitor so we need to take
> >> > the queue lock again even after popping from it.
> >> >
> >> > Signed-off-by: Peter Xu <peterx@redhat.com>
> >>
> >> The subject's "send CMD_DROP" fails to highlight the important part:
> >> we're no longer dropping commands. Moreover, the commit message could
> >> do a better job explaining the tradeoffs. Here's my try:
> >>
> >> monitor: Suspend monitor instead dropping commands
> >>
> >> When a QMP client sends in-band commands more quickly that we can
> >> process them, we can either queue them without limit (QUEUE), drop
> >> commands when the queue is full (DROP), or suspend receiving commands
> >> when the queue is full (SUSPEND). None of them is ideal:
> >>
> >> * QUEUE lets a misbehaving client make QEMU eat memory without bounds.
> >> Not such a hot idea.
> >>
> >> * With DROP, the client has to cope with dropped in-band commands. To
> >> inform the client, we send a COMMAND_DROPPED event then. The event is
> >> flawed by design in two ways: it's ambiguous (see commit d621cfe0a17),
> >> and it brings back the "eat memory without bounds" problem.
> >>
> >> * With SUSPEND, the client has to manage the flow of in-band commands to
> >> keep the monitor available for out-of-band commands.
> >>
> >> We currently DROP. Switch to SUSPEND.
> >>
> >> Managing the flow of in-band commands to keep the monitor available for
> >> out-of-band commands isn't really hard: just count the number of
> >> "outstanding" in-band commands (commands sent minus replies received),
> >> and if it exceeds the limit, hold back additional ones until it drops
> >> below the limit again.
> >
> > (Will the simplest QMP client just block at the write() system call
> > without notice?
>
> Yes.
>
> When you stop reading from a socket or pipe, the peer eventually can't
> write more. "Eventually", because the TCP connection or pipe buffers
> some.
>
> A naive client using a blocking write() will block then.
>
> A slightly more sophisticated client using a non-blocking write() has
> its write() fail with EAGAIN or EWOULDBLOCK.
>
> In both cases, OOB commands may be stuck in the TCP connection's /
> pipe's buffer.
>
>
> > Anyway, the SUSPEND is ideal enough to me... :)
> >
> >>
> >> Note that we need to be careful pairing the suspend with a resume, or
> >> else the monitor will hang, possibly forever.
> >
> > Will take your version, thanks.
> >
> >>
> >>
> >> > ---
> >> > monitor.c | 33 +++++++++++++++------------------
> >> > 1 file changed, 15 insertions(+), 18 deletions(-)
> >> >
> >> > diff --git a/monitor.c b/monitor.c
> >> > index 3b90c9eb5f..9e6cad2af6 100644
> >> > --- a/monitor.c
> >> > +++ b/monitor.c
> >> > @@ -89,6 +89,8 @@
> >> > #include "hw/s390x/storage-attributes.h"
> >> > #endif
> >> >
> >> > +#define QMP_REQ_QUEUE_LEN_MAX (8)
> >> > +
> >>
> >> Let's drop the parenthesis.
> >
> > Ok.
> >
> >>
> >> > /*
> >> > * Supported types:
> >> > *
> >> > @@ -4124,29 +4126,33 @@ static QMPRequest *monitor_qmp_requests_pop_any(void)
> >> > static void monitor_qmp_bh_dispatcher(void *data)
> >> > {
> >> > QMPRequest *req_obj = monitor_qmp_requests_pop_any();
> >> > + Monitor *mon;
> >> > QDict *rsp;
> >> > bool need_resume;
> >> >
> >> > if (!req_obj) {
> >> > return;
> >> > }
> >> > -
> >>
> >> Let's keep the blank line.
> >
> > Ok.
> >
> >>
> >> > + mon = req_obj->mon;
> >> > /* qmp_oob_enabled() might change after "qmp_capabilities" */
> >> > - need_resume = !qmp_oob_enabled(req_obj->mon);
> >> > + qemu_mutex_lock(&mon->qmp.qmp_queue_lock);
> >> > + need_resume = !qmp_oob_enabled(req_obj->mon) ||
> >> > + mon->qmp.qmp_requests->length == QMP_REQ_QUEUE_LEN_MAX - 1;
> >>
> >> Note for later: this is the condition guarding monitor_resume().
> >>
> >> Is this race-free? We have two criticial sections, one in
> >> monitor_qmp_requests_pop_any(), and one here. What if two threads
> >> interleave like this
> >>
> >> thread 1 thread 2
> >> ------------------------------------------------------------------
> >> monitor_qmp_requests_pop_any()
> >> lock
> >> dequeue
> >> unlock
> >> monitor_qmp_requests_pop_any()
> >> lock
> >> dequeue
> >> unlock
> >> lock
> >> check queue length
> >> unlock
> >> if queue length demands it
> >> monitor_resume()
> >> lock
> >> check queue length
> >> unlock
> >> if queue length demands it
> >> monitor_resume()
> >>
> >> Looks racy to me: if we start with the queue full (and the monitor
> >> suspended), both threads' check queue length see length
> >> QMP_REQ_QUEUE_LEN_MAX - 2, and neither thread resumes the monitor.
> >> Oops.
> >>
> >> Simplest fix is probably moving the resume into
> >> monitor_qmp_requests_pop_any()'s critical section.
> >
> > But we only have one QMP dispatcher, right? So IMHO we can't have two
> > threads doing monitor_qmp_requests_pop_any() at the same time.
>
> You're right, we currently run monitor_qmp_bh_dispatcher() only in the
> main thread, and a thread can't race with itself. But the main thread
> can still race with handle_qmp_command() running in mon_iothread.
>
> main thread mon_iothread
> ------------------------------------------------------------------
> monitor_qmp_requests_pop_any()
> lock
> dequeue
> unlock
> lock
> if queue length demands it
> monitor_suspend()
> push onto queue
> unlock
> lock
> check queue length
> unlock
> if queue length demands it
> monitor_resume() <---------------------- [1]
>
> If we start with the queue full (and the monitor suspended), the main
> thread first determines it'll need to resume. mon_iothread then fills
> the queue again, and suspends the suspended monitor some more. If I
(Side note: here it's tricky that when we "determines to resume" we
didn't really resume, so we are still suspended, hence the
mon_iothread cannot fill the queue again until the resume at [1]
above. Hence IMHO we're safe here. But I agree that it's still racy
in other cases.)
> read the code correctly, this increments mon->suspend_cnt from 1 to 2.
> Finally, the main thread checks the queue length:
>
> need_resume = !qmp_oob_enabled(req_obj->mon) ||
> mon->qmp.qmp_requests->length == QMP_REQ_QUEUE_LEN_MAX - 1;
>
> The yields false, because ->length is now QMP_REQ_QUEUE_LEN_MAX. The
> main thread does *not* resume the monitor.
>
> State after this: queue full, mon->suspend_cnt == 2. Bad, but we'll
> recover on the dequeue after next: the next dequeue decrements
> mon->suspend_cnt to 1 without resuming the monitor, the one after that
> will decrement it to 0 and resume the monitor.
>
> However, if handle_qmp_command() runs in this spot often enough to push
> mon->suspend_cnt above QMP_REQ_QUEUE_LEN_MAX, the monitor will remain
> suspended forever.
>
> I'm teaching you multiprogramming 101 here. The thing that should make
> the moderately experienced nose twitch is the anti-pattern
>
> begin critical section
> do something
> end critical section
> begin critical section
> if we just did X, state must be Y, so we must now do Z
> end critical section
>
> The part "if we just did X, state must be Y" is *wrong*. You have no
> idea what the state is, since code running between the two critical
> sections may have changed it.
>
> Here,
>
> X = dequeued from a full queue"
> Y = "mon->suspend_cnt == 1"
> Z = monitor_resume() to resume the monitor
>
> I showed that Y does not hold.
>
> On a slightly more abstract level, this anti-pattern applies:
>
> begin critical section
> start messing with invariant
> end critical section
> // invariant does not hold here, oopsie
> begin critical section
> finish messing with invariant
> end critical section
>
> The invariant here is "monitor suspended exactly when the queue is
> full". Your first critical section can make the queue non-full. The
> second one resumes. The invariant doesn't hold in between, and all bets
> are off.
>
> To maintain the invariant, you *have* to enqueue and suspend in a single
> critical section (which you do), *and* dequeue and resume in a single
> critical section (which you don't).
Thank you for teaching the lesson for me.
>
> > But I fully agree that it'll be nicer to keep them together (e.g. in
> > case we allow to dispatch two commands some day), but then if you see
> > it'll be something like the old req_obj->need_resume flag, but we set
> > it in monitor_qmp_requests_pop_any() instead. If so, I really prefer
> > we just keep that need_resume flag for per-monitor by dropping
> > 176160ce78 and keep my patch to move that flag into monitor struct
> > (which I dropped after the rebase of this version).
>
> Please have another look.
Again, if we want to put pop+check into the same critical section,
then we possibly need to return something from the
monitor_qmp_requests_pop_any() to show the queue length information,
then I would prefer to keep the per-monitor need_resume flag.
What do you think?
>
> >> > + qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);
> >> > if (req_obj->req) {
> >> > trace_monitor_qmp_cmd_in_band(qobject_get_try_str(req_obj->id) ?: "");
> >> > - monitor_qmp_dispatch(req_obj->mon, req_obj->req, req_obj->id);
> >> > + monitor_qmp_dispatch(mon, req_obj->req, req_obj->id);
> >> > } else {
> >> > assert(req_obj->err);
> >> > rsp = qmp_error_response(req_obj->err);
> >> > req_obj->err = NULL;
> >> > - monitor_qmp_respond(req_obj->mon, rsp, NULL);
> >> > + monitor_qmp_respond(mon, rsp, NULL);
> >> > qobject_unref(rsp);
> >> > }
> >> >
> >> > if (need_resume) {
> >> > /* Pairs with the monitor_suspend() in handle_qmp_command() */
> >> > - monitor_resume(req_obj->mon);
> >> > + monitor_resume(mon);
> >> > }
> >> > qmp_request_free(req_obj);
> >>
> >> The replacement of req_obj->mon by mon makes this change less clear than
> >> it could be. Let's figure out the possible race, and then we'll see
> >> whether we'll still want this replacement.
> >
> > Sure.
> >
> >>
> >> >
> >> > @@ -4154,8 +4160,6 @@ static void monitor_qmp_bh_dispatcher(void *data)
> >> > qemu_bh_schedule(qmp_dispatcher_bh);
> >> > }
> >> >
> >> > -#define QMP_REQ_QUEUE_LEN_MAX (8)
> >> > -
> >> > static void handle_qmp_command(void *opaque, QObject *req, Error *err)
> >> > {
> >> > Monitor *mon = opaque;
> >> > @@ -4205,19 +4209,12 @@ static void handle_qmp_command(void *opaque, QObject *req, Error *err)
> >> /*
> >> * If OOB is not enabled on the current monitor, we'll emulate the
> >> * old behavior that we won't process the current monitor any more
> >> * until it has responded. This helps make sure that as long as
> >> * OOB is not enabled, the server will never drop any command.
> >> */
> >>
> >> This comment is now stale, we don't drop commands anymore.
> >
> > AFAIU it's describing [1] below but nothing related to COMMAND_DROP?
>
> The comment suggests the server can drop commands when OOB is enabled.
> This is no longer correct after this patch.
Ah yes the last sentense, sorry.
No matter what, I'm taking your suggestion below so the paragraph will
be dropped.
Thanks,
>
> >>
> >> > if (!qmp_oob_enabled(mon)) {
> >> > monitor_suspend(mon);
> >
> > [1]
> >
> >> > } else {
> >> > - /* Drop the request if queue is full. */
> >> > - if (mon->qmp.qmp_requests->length >= QMP_REQ_QUEUE_LEN_MAX) {
> >> > - qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);
> >> > + if (mon->qmp.qmp_requests->length == QMP_REQ_QUEUE_LEN_MAX - 1) {
> >> > /*
> >> > - * FIXME @id's scope is just @mon, and broadcasting it is
> >> > - * wrong. If another monitor's client has a command with
> >> > - * the same ID in flight, the event will incorrectly claim
> >> > - * that command was dropped.
> >> > + * If this is _exactly_ the last request that we can
> >> > + * queue, we suspend the monitor right now.
> >> > */
> >> > - qapi_event_send_command_dropped(id,
> >> > - COMMAND_DROP_REASON_QUEUE_FULL);
> >> > - qmp_request_free(req_obj);
> >> > - return;
> >> > + monitor_suspend(mon);
> >> > }
> >> > }
> >>
> >> The conditional and its comments look unbalanced. Moreover, the
> >> condition is visually dissimilar to the one guarding the matching
> >> monitor_resume(). What about:
> >>
> >> /*
> >> * Suspend the monitor when we can't queue more requests after
> >> * this one. Dequeuing in monitor_qmp_bh_dispatcher() will resume
> >> * it.
> >> * Note that when OOB is disabled, we queue at most one command,
> >> * for backward compatibility.
> >> */
> >> if (!qmp_oob_enabled(mon) ||
> >> mon->qmp.qmp_requests->length == QMP_REQ_QUEUE_LEN_MAX - 1) {
> >> monitor_suspend(mon);
> >> }
> >>
> >> You'll have to update the comment if you move the resume to
> >> monitor_qmp_requests_pop_any().
> >>
> >> Next:
> >>
> >> /*
> >> * Put the request to the end of queue so that requests will be
> >> * handled in time order. Ownership for req_obj, req, id,
> >> * etc. will be delivered to the handler side.
> >> */
> >>
> >> Suggest asserting mon->qmp.qmp_requests->length < QMP_REQ_QUEUE_LEN_MAX
> >> here.
> >
> > Sure I can do this.
> >
> >>
> >> g_queue_push_tail(mon->qmp.qmp_requests, req_obj);
> >> qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);
> >
> > Thanks,
Regards,
--
Peter Xu
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [PATCH v7 4/7] qapi: remove COMMAND_DROPPED event
2018-09-03 14:41 ` Daniel P. Berrangé
@ 2018-09-04 5:30 ` Peter Xu
2018-09-04 8:04 ` Markus Armbruster
2018-09-04 6:39 ` Markus Armbruster
1 sibling, 1 reply; 28+ messages in thread
From: Peter Xu @ 2018-09-04 5:30 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Eric Blake, Markus Armbruster, Marc-André Lureau,
qemu-devel, Dr . David Alan Gilbert
On Mon, Sep 03, 2018 at 03:41:16PM +0100, Daniel P. Berrangé wrote:
> On Mon, Sep 03, 2018 at 09:30:52AM -0500, Eric Blake wrote:
> > On 09/03/2018 08:31 AM, Markus Armbruster wrote:
> >
> > > Example:
> > >
> > > client sends in-band command #1
> > > QEMU reads and queues
> > > QEMU dequeues in-band command #1
> > > in-band command #1 starts executing, but it's slooow
> > > client sends in-band command #2
> > > QEMU reads and queues
> > > ...
> > > client sends in-band command #8
> > > QEMU reads, queues and suspends the monitor
> > > client sends out-of-band command
> > > --> time passes...
> > > in-band command #1 completes, QEMU sends reply
> > > QEMU dequeues in-band command #2, resumes the monitor
> > > in-band command #2 starts executing
> > > QEMU reads and executes out-of-band command
> > > out-of-band command completes, QEMU sends reply
> > > in-band command #2 completes, QEMU sends reply
> > > ... same for remaining in-band commands ...
> > >
> > > The out-of-band command gets stuck behind the in-band commands.
(It's a shame of me to have just noticed that the out-of-band command
will be stuck after we dropped the COMMAND_DROP event... so now I
agree it's not that ideal any more to drop the event but maybe still
preferable)
> > >
> > > The client can avoid this by managing the flow of in-band commands: have
> > > no more than 7 in flight, so the monitor never gets suspended.
> > >
> > > This is a potentially useful thing to do for clients, isn't it?
> > >
> > > Eric, Daniel, is it something libvirt would do?
> >
> > Right now, libvirt serializes commands - it never sends a QMP command until
> > the previous command's response has been processed. But that may not help
> > much, since libvirt does not send OOB commands.
>
> Note that is not merely due to the QMP monitor restriction either.
>
> Libvirt serializes all its public APIs that can change state of a running
> domain. It usually aims to allow read-only APIs to be run in parallel with
> APIs that change state.
>
> The exception to the rule right now are some of the migration APIs which
> we allow to be invoked to manage the migration process.
>
> > I guess when we are designing what libvirt should do, and deciding WHEN it
> > should send OOB commands, we have the luxury of designing libvirt to enforce
> > how many in-flight in-band commands it will ever have pending at once
> > (whether the current 'at most 1', or even if we make it more parallel to 'at
> > most 7'), so that we can still be ensured that the OOB command will be
> > processed without being stuck in the queue of suspended in-band commands.
> > If we never send more than one in-band at a time, then it's not a concern
> > how deep the qemu queue is; but if we do want libvirt to start parallel
> > in-band commands, then you are right that having a way to learn the qemu
> > queue depth is programmatically more precise than just guessing the maximum
> > depth. But it's also hard to argue we need that complexity if we don't have
> > an immediate use envisioned for it.
>
> In terms of what libvirt would want to parallelize, I think it is reasonable
> to consider any of the query-XXXX commands desirable. Other stuff is likely
> to remain serialized from libvirt's side.
IMHO concurrency won't help much now even for query commands, since
our current concurrency is still "partly" - the executions of query
commands (which is the most time consuming part) will still be done
sequentially, so even if we send multiple query commands in parallel
(without waiting for a response of any sent commands), the total time
used for the list of commands would be mostly the same.
My understanding for why we have such a queue length now is that it
came from a security concern: after we have a queue, we need that
queue length to limit the memory usages for the QMP server. Though
that might not help much for real users like Libvirt, it's majorly
serving as a way to protect QEMU QMP from being attacked or from being
turned down by a buggy QMP client.
But I agree now that the queue length information might still be
helpful some day. Maybe, we can hide that until we support executing
commands in parallel for some of them.
Regards,
--
Peter Xu
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [PATCH v7 3/7] monitor: suspend monitor instead of send CMD_DROP
2018-09-04 3:33 ` Peter Xu
@ 2018-09-04 6:17 ` Markus Armbruster
2018-09-04 7:01 ` Peter Xu
0 siblings, 1 reply; 28+ messages in thread
From: Markus Armbruster @ 2018-09-04 6:17 UTC (permalink / raw)
To: Peter Xu
Cc: Markus Armbruster, Marc-André Lureau, qemu-devel,
Dr . David Alan Gilbert
Peter Xu <peterx@redhat.com> writes:
> On Mon, Sep 03, 2018 at 03:16:55PM +0200, Markus Armbruster wrote:
>> Peter Xu <peterx@redhat.com> writes:
>>
>> > On Mon, Sep 03, 2018 at 09:38:00AM +0200, Markus Armbruster wrote:
>> >> Peter Xu <peterx@redhat.com> writes:
>> >>
>> >> > When we received too many qmp commands, previously we'll send
>> >> > COMMAND_DROPPED events to monitors, then we'll drop the requests. Now
>> >> > instead of dropping the command we stop reading from input when we
>> >> > notice the queue is getting full. Note that here since we removed the
>> >> > need_resume flag we need to be _very_ careful on the suspend/resume
>> >> > paring on the conditions since unmatched condition checks will hang
>> >> > death the monitor. Meanwhile, now we will need to read the queue length
>> >> > to decide whether we'll need to resume the monitor so we need to take
>> >> > the queue lock again even after popping from it.
>> >> >
>> >> > Signed-off-by: Peter Xu <peterx@redhat.com>
>> >>
>> >> The subject's "send CMD_DROP" fails to highlight the important part:
>> >> we're no longer dropping commands. Moreover, the commit message could
>> >> do a better job explaining the tradeoffs. Here's my try:
>> >>
>> >> monitor: Suspend monitor instead dropping commands
>> >>
>> >> When a QMP client sends in-band commands more quickly that we can
>> >> process them, we can either queue them without limit (QUEUE), drop
>> >> commands when the queue is full (DROP), or suspend receiving commands
>> >> when the queue is full (SUSPEND). None of them is ideal:
>> >>
>> >> * QUEUE lets a misbehaving client make QEMU eat memory without bounds.
>> >> Not such a hot idea.
>> >>
>> >> * With DROP, the client has to cope with dropped in-band commands. To
>> >> inform the client, we send a COMMAND_DROPPED event then. The event is
>> >> flawed by design in two ways: it's ambiguous (see commit d621cfe0a17),
>> >> and it brings back the "eat memory without bounds" problem.
>> >>
>> >> * With SUSPEND, the client has to manage the flow of in-band commands to
>> >> keep the monitor available for out-of-band commands.
>> >>
>> >> We currently DROP. Switch to SUSPEND.
>> >>
>> >> Managing the flow of in-band commands to keep the monitor available for
>> >> out-of-band commands isn't really hard: just count the number of
>> >> "outstanding" in-band commands (commands sent minus replies received),
>> >> and if it exceeds the limit, hold back additional ones until it drops
>> >> below the limit again.
>> >
>> > (Will the simplest QMP client just block at the write() system call
>> > without notice?
>>
>> Yes.
>>
>> When you stop reading from a socket or pipe, the peer eventually can't
>> write more. "Eventually", because the TCP connection or pipe buffers
>> some.
>>
>> A naive client using a blocking write() will block then.
>>
>> A slightly more sophisticated client using a non-blocking write() has
>> its write() fail with EAGAIN or EWOULDBLOCK.
>>
>> In both cases, OOB commands may be stuck in the TCP connection's /
>> pipe's buffer.
>>
>>
>> > Anyway, the SUSPEND is ideal enough to me... :)
>> >
>> >>
>> >> Note that we need to be careful pairing the suspend with a resume, or
>> >> else the monitor will hang, possibly forever.
>> >
>> > Will take your version, thanks.
>> >
>> >>
>> >>
>> >> > ---
>> >> > monitor.c | 33 +++++++++++++++------------------
>> >> > 1 file changed, 15 insertions(+), 18 deletions(-)
>> >> >
>> >> > diff --git a/monitor.c b/monitor.c
>> >> > index 3b90c9eb5f..9e6cad2af6 100644
>> >> > --- a/monitor.c
>> >> > +++ b/monitor.c
>> >> > @@ -89,6 +89,8 @@
>> >> > #include "hw/s390x/storage-attributes.h"
>> >> > #endif
>> >> >
>> >> > +#define QMP_REQ_QUEUE_LEN_MAX (8)
>> >> > +
>> >>
>> >> Let's drop the parenthesis.
>> >
>> > Ok.
>> >
>> >>
>> >> > /*
>> >> > * Supported types:
>> >> > *
>> >> > @@ -4124,29 +4126,33 @@ static QMPRequest *monitor_qmp_requests_pop_any(void)
>> >> > static void monitor_qmp_bh_dispatcher(void *data)
>> >> > {
>> >> > QMPRequest *req_obj = monitor_qmp_requests_pop_any();
>> >> > + Monitor *mon;
>> >> > QDict *rsp;
>> >> > bool need_resume;
>> >> >
>> >> > if (!req_obj) {
>> >> > return;
>> >> > }
>> >> > -
>> >>
>> >> Let's keep the blank line.
>> >
>> > Ok.
>> >
>> >>
>> >> > + mon = req_obj->mon;
>> >> > /* qmp_oob_enabled() might change after "qmp_capabilities" */
>> >> > - need_resume = !qmp_oob_enabled(req_obj->mon);
>> >> > + qemu_mutex_lock(&mon->qmp.qmp_queue_lock);
>> >> > + need_resume = !qmp_oob_enabled(req_obj->mon) ||
>> >> > + mon->qmp.qmp_requests->length == QMP_REQ_QUEUE_LEN_MAX - 1;
>> >>
>> >> Note for later: this is the condition guarding monitor_resume().
>> >>
>> >> Is this race-free? We have two criticial sections, one in
>> >> monitor_qmp_requests_pop_any(), and one here. What if two threads
>> >> interleave like this
>> >>
>> >> thread 1 thread 2
>> >> ------------------------------------------------------------------
>> >> monitor_qmp_requests_pop_any()
>> >> lock
>> >> dequeue
>> >> unlock
>> >> monitor_qmp_requests_pop_any()
>> >> lock
>> >> dequeue
>> >> unlock
>> >> lock
>> >> check queue length
>> >> unlock
>> >> if queue length demands it
>> >> monitor_resume()
>> >> lock
>> >> check queue length
>> >> unlock
>> >> if queue length demands it
>> >> monitor_resume()
>> >>
>> >> Looks racy to me: if we start with the queue full (and the monitor
>> >> suspended), both threads' check queue length see length
>> >> QMP_REQ_QUEUE_LEN_MAX - 2, and neither thread resumes the monitor.
>> >> Oops.
>> >>
>> >> Simplest fix is probably moving the resume into
>> >> monitor_qmp_requests_pop_any()'s critical section.
>> >
>> > But we only have one QMP dispatcher, right? So IMHO we can't have two
>> > threads doing monitor_qmp_requests_pop_any() at the same time.
>>
>> You're right, we currently run monitor_qmp_bh_dispatcher() only in the
>> main thread, and a thread can't race with itself. But the main thread
>> can still race with handle_qmp_command() running in mon_iothread.
>>
>> main thread mon_iothread
>> ------------------------------------------------------------------
>> monitor_qmp_requests_pop_any()
>> lock
>> dequeue
>> unlock
>> lock
>> if queue length demands it
>> monitor_suspend()
>> push onto queue
>> unlock
>> lock
>> check queue length
>> unlock
>> if queue length demands it
>> monitor_resume() <---------------------- [1]
>>
>> If we start with the queue full (and the monitor suspended), the main
>> thread first determines it'll need to resume. mon_iothread then fills
>> the queue again, and suspends the suspended monitor some more. If I
>
> (Side note: here it's tricky that when we "determines to resume" we
> didn't really resume, so we are still suspended, hence the
> mon_iothread cannot fill the queue again until the resume at [1]
You're right.
> above. Hence IMHO we're safe here. But I agree that it's still racy
> in other cases.)
Maybe.
Getting threads to cooperate safely is hard. Key to success is making
things as simple and obvious as we can. Suitable invariants help.
They help even more when they're documented and checked with assertions.
Perhaps you can think of ways to do that.
>> read the code correctly, this increments mon->suspend_cnt from 1 to 2.
>> Finally, the main thread checks the queue length:
>>
>> need_resume = !qmp_oob_enabled(req_obj->mon) ||
>> mon->qmp.qmp_requests->length == QMP_REQ_QUEUE_LEN_MAX - 1;
>>
>> The yields false, because ->length is now QMP_REQ_QUEUE_LEN_MAX. The
>> main thread does *not* resume the monitor.
>>
>> State after this: queue full, mon->suspend_cnt == 2. Bad, but we'll
>> recover on the dequeue after next: the next dequeue decrements
>> mon->suspend_cnt to 1 without resuming the monitor, the one after that
>> will decrement it to 0 and resume the monitor.
>>
>> However, if handle_qmp_command() runs in this spot often enough to push
>> mon->suspend_cnt above QMP_REQ_QUEUE_LEN_MAX, the monitor will remain
>> suspended forever.
>>
>> I'm teaching you multiprogramming 101 here. The thing that should make
>> the moderately experienced nose twitch is the anti-pattern
>>
>> begin critical section
>> do something
>> end critical section
>> begin critical section
>> if we just did X, state must be Y, so we must now do Z
>> end critical section
>>
>> The part "if we just did X, state must be Y" is *wrong*. You have no
>> idea what the state is, since code running between the two critical
>> sections may have changed it.
>>
>> Here,
>>
>> X = dequeued from a full queue"
>> Y = "mon->suspend_cnt == 1"
>> Z = monitor_resume() to resume the monitor
>>
>> I showed that Y does not hold.
>>
>> On a slightly more abstract level, this anti-pattern applies:
>>
>> begin critical section
>> start messing with invariant
>> end critical section
>> // invariant does not hold here, oopsie
>> begin critical section
>> finish messing with invariant
>> end critical section
>>
>> The invariant here is "monitor suspended exactly when the queue is
>> full". Your first critical section can make the queue non-full. The
>> second one resumes. The invariant doesn't hold in between, and all bets
>> are off.
>>
>> To maintain the invariant, you *have* to enqueue and suspend in a single
>> critical section (which you do), *and* dequeue and resume in a single
>> critical section (which you don't).
>
> Thank you for teaching the lesson for me.
>
>>
>> > But I fully agree that it'll be nicer to keep them together (e.g. in
>> > case we allow to dispatch two commands some day), but then if you see
>> > it'll be something like the old req_obj->need_resume flag, but we set
>> > it in monitor_qmp_requests_pop_any() instead. If so, I really prefer
>> > we just keep that need_resume flag for per-monitor by dropping
>> > 176160ce78 and keep my patch to move that flag into monitor struct
>> > (which I dropped after the rebase of this version).
>>
>> Please have another look.
>
> Again, if we want to put pop+check into the same critical section,
> then we possibly need to return something from the
> monitor_qmp_requests_pop_any() to show the queue length information,
> then I would prefer to keep the per-monitor need_resume flag.
>
> What do you think?
Options:
* Save rather than recompute @need_resume
This gets rid of the second critical section.
* Have monitor_qmp_requests_pop_any() return @need_resume
This merges the second critical section into the first.
* Have a single critical section in monitor_qmp_bh_dispatcher()
This replaces both critical sections by a single larger one.
Callers of monitor_qmp_bh_dispatcher() must hold monitor_lock.
* Other ideas?
The question to ask regardless of option: what invariant do the critical
sections maintain?
I proposed one: monitor suspended exactly when the queue is full.
handle_qmp_command()'s critical section maintains it: it suspends when
its enqueue fills up the queue.
monitor_qmp_bh_dispatcher()'s critical sections do not. Even if we
reduce them from two to one, the resulting single critical section can't
as long as we resume only after the dequeued command completed, because
that would require holding monitor_lock while the command executes. So,
to maintain this invariant, we need to rethink
monitor_qmp_bh_dispatcher(). Why can't we resume right after dequeuing?
You're of course welcome to propose a different invariant.
[...]
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [PATCH v7 4/7] qapi: remove COMMAND_DROPPED event
2018-09-03 14:41 ` Daniel P. Berrangé
2018-09-04 5:30 ` Peter Xu
@ 2018-09-04 6:39 ` Markus Armbruster
2018-09-04 8:23 ` Daniel P. Berrangé
1 sibling, 1 reply; 28+ messages in thread
From: Markus Armbruster @ 2018-09-04 6:39 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Eric Blake, Marc-André Lureau, Dr . David Alan Gilbert,
Markus Armbruster, Peter Xu, qemu-devel
Daniel P. Berrangé <berrange@redhat.com> writes:
> On Mon, Sep 03, 2018 at 09:30:52AM -0500, Eric Blake wrote:
>> On 09/03/2018 08:31 AM, Markus Armbruster wrote:
>>
>> > Example:
>> >
>> > client sends in-band command #1
>> > QEMU reads and queues
>> > QEMU dequeues in-band command #1
>> > in-band command #1 starts executing, but it's slooow
>> > client sends in-band command #2
>> > QEMU reads and queues
>> > ...
>> > client sends in-band command #8
>> > QEMU reads, queues and suspends the monitor
>> > client sends out-of-band command
>> > --> time passes...
>> > in-band command #1 completes, QEMU sends reply
>> > QEMU dequeues in-band command #2, resumes the monitor
>> > in-band command #2 starts executing
>> > QEMU reads and executes out-of-band command
>> > out-of-band command completes, QEMU sends reply
>> > in-band command #2 completes, QEMU sends reply
>> > ... same for remaining in-band commands ...
>> >
>> > The out-of-band command gets stuck behind the in-band commands.
>> >
>> > The client can avoid this by managing the flow of in-band commands: have
>> > no more than 7 in flight, so the monitor never gets suspended.
>> >
>> > This is a potentially useful thing to do for clients, isn't it?
>> >
>> > Eric, Daniel, is it something libvirt would do?
>>
>> Right now, libvirt serializes commands - it never sends a QMP command until
>> the previous command's response has been processed. But that may not help
>> much, since libvirt does not send OOB commands.
>
> Note that is not merely due to the QMP monitor restriction either.
>
> Libvirt serializes all its public APIs that can change state of a running
> domain. It usually aims to allow read-only APIs to be run in parallel with
> APIs that change state.
Makes sense. State changes are complex enough without concurrency.
Even permitting just concurrent queries can add non-trivial complexity.
However, pipelineing != concurrency. "Serializing" as I understand it
implies no concurrency, it doesn't imply no pipelining.
Mind, I'm not telling you to pipeline, I'm just trying to understand the
constraints.
> The exception to the rule right now are some of the migration APIs which
> we allow to be invoked to manage the migration process.
Can you give an example?
>> I guess when we are designing what libvirt should do, and deciding WHEN it
>> should send OOB commands, we have the luxury of designing libvirt to enforce
>> how many in-flight in-band commands it will ever have pending at once
>> (whether the current 'at most 1', or even if we make it more parallel to 'at
>> most 7'), so that we can still be ensured that the OOB command will be
>> processed without being stuck in the queue of suspended in-band commands.
>> If we never send more than one in-band at a time, then it's not a concern
>> how deep the qemu queue is; but if we do want libvirt to start parallel
>> in-band commands, then you are right that having a way to learn the qemu
>> queue depth is programmatically more precise than just guessing the maximum
>> depth. But it's also hard to argue we need that complexity if we don't have
>> an immediate use envisioned for it.
True.
Options for the initial interface:
(1) Provide means for the client to determine the queue length limit
(introspection or configuration). Clients that need the monitory to
remain available for out-of-band commands can keep limit - 1 in-band
commands in flight.
(2) Make the queue length limit part of the documented interface.
Clients that need the monitory to remain available for out-of-band
commands can keep limit - 1 in-band commands in flight. We can
increase the limit later, but not decrease it. We can also switch
to (1) as needed.
(3) Treat the queue length limit as implementation detail (but tacitly
assume its at least 2, since less makes no sense[*]). Clients that
need the monitory to remain available for out-of-band commands
cannot safely keep more than one in-band command in flight. We can
switch to (2) or (1) as needed.
Opinions?
> In terms of what libvirt would want to parallelize, I think it is reasonable
> to consider any of the query-XXXX commands desirable. Other stuff is likely
> to remain serialized from libvirt's side.
QEMU doesn't care whether it queues in-band query commands or other
in-band commands.
[*] With a queue length of 1, the monitor suspends on receipt of an
in-band command, and resumes after it completes. This renders
out-of-band commands useless: there is no way an out-of-band command can
overtake an in-band command.
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [PATCH v7 3/7] monitor: suspend monitor instead of send CMD_DROP
2018-09-04 6:17 ` Markus Armbruster
@ 2018-09-04 7:01 ` Peter Xu
0 siblings, 0 replies; 28+ messages in thread
From: Peter Xu @ 2018-09-04 7:01 UTC (permalink / raw)
To: Markus Armbruster
Cc: Marc-André Lureau, qemu-devel, Dr . David Alan Gilbert
On Tue, Sep 04, 2018 at 08:17:54AM +0200, Markus Armbruster wrote:
> Peter Xu <peterx@redhat.com> writes:
>
> > On Mon, Sep 03, 2018 at 03:16:55PM +0200, Markus Armbruster wrote:
> >> Peter Xu <peterx@redhat.com> writes:
> >>
> >> > On Mon, Sep 03, 2018 at 09:38:00AM +0200, Markus Armbruster wrote:
> >> >> Peter Xu <peterx@redhat.com> writes:
> >> >>
> >> >> > When we received too many qmp commands, previously we'll send
> >> >> > COMMAND_DROPPED events to monitors, then we'll drop the requests. Now
> >> >> > instead of dropping the command we stop reading from input when we
> >> >> > notice the queue is getting full. Note that here since we removed the
> >> >> > need_resume flag we need to be _very_ careful on the suspend/resume
> >> >> > paring on the conditions since unmatched condition checks will hang
> >> >> > death the monitor. Meanwhile, now we will need to read the queue length
> >> >> > to decide whether we'll need to resume the monitor so we need to take
> >> >> > the queue lock again even after popping from it.
> >> >> >
> >> >> > Signed-off-by: Peter Xu <peterx@redhat.com>
> >> >>
> >> >> The subject's "send CMD_DROP" fails to highlight the important part:
> >> >> we're no longer dropping commands. Moreover, the commit message could
> >> >> do a better job explaining the tradeoffs. Here's my try:
> >> >>
> >> >> monitor: Suspend monitor instead dropping commands
> >> >>
> >> >> When a QMP client sends in-band commands more quickly that we can
> >> >> process them, we can either queue them without limit (QUEUE), drop
> >> >> commands when the queue is full (DROP), or suspend receiving commands
> >> >> when the queue is full (SUSPEND). None of them is ideal:
> >> >>
> >> >> * QUEUE lets a misbehaving client make QEMU eat memory without bounds.
> >> >> Not such a hot idea.
> >> >>
> >> >> * With DROP, the client has to cope with dropped in-band commands. To
> >> >> inform the client, we send a COMMAND_DROPPED event then. The event is
> >> >> flawed by design in two ways: it's ambiguous (see commit d621cfe0a17),
> >> >> and it brings back the "eat memory without bounds" problem.
> >> >>
> >> >> * With SUSPEND, the client has to manage the flow of in-band commands to
> >> >> keep the monitor available for out-of-band commands.
> >> >>
> >> >> We currently DROP. Switch to SUSPEND.
> >> >>
> >> >> Managing the flow of in-band commands to keep the monitor available for
> >> >> out-of-band commands isn't really hard: just count the number of
> >> >> "outstanding" in-band commands (commands sent minus replies received),
> >> >> and if it exceeds the limit, hold back additional ones until it drops
> >> >> below the limit again.
> >> >
> >> > (Will the simplest QMP client just block at the write() system call
> >> > without notice?
> >>
> >> Yes.
> >>
> >> When you stop reading from a socket or pipe, the peer eventually can't
> >> write more. "Eventually", because the TCP connection or pipe buffers
> >> some.
> >>
> >> A naive client using a blocking write() will block then.
> >>
> >> A slightly more sophisticated client using a non-blocking write() has
> >> its write() fail with EAGAIN or EWOULDBLOCK.
> >>
> >> In both cases, OOB commands may be stuck in the TCP connection's /
> >> pipe's buffer.
> >>
> >>
> >> > Anyway, the SUSPEND is ideal enough to me... :)
> >> >
> >> >>
> >> >> Note that we need to be careful pairing the suspend with a resume, or
> >> >> else the monitor will hang, possibly forever.
> >> >
> >> > Will take your version, thanks.
> >> >
> >> >>
> >> >>
> >> >> > ---
> >> >> > monitor.c | 33 +++++++++++++++------------------
> >> >> > 1 file changed, 15 insertions(+), 18 deletions(-)
> >> >> >
> >> >> > diff --git a/monitor.c b/monitor.c
> >> >> > index 3b90c9eb5f..9e6cad2af6 100644
> >> >> > --- a/monitor.c
> >> >> > +++ b/monitor.c
> >> >> > @@ -89,6 +89,8 @@
> >> >> > #include "hw/s390x/storage-attributes.h"
> >> >> > #endif
> >> >> >
> >> >> > +#define QMP_REQ_QUEUE_LEN_MAX (8)
> >> >> > +
> >> >>
> >> >> Let's drop the parenthesis.
> >> >
> >> > Ok.
> >> >
> >> >>
> >> >> > /*
> >> >> > * Supported types:
> >> >> > *
> >> >> > @@ -4124,29 +4126,33 @@ static QMPRequest *monitor_qmp_requests_pop_any(void)
> >> >> > static void monitor_qmp_bh_dispatcher(void *data)
> >> >> > {
> >> >> > QMPRequest *req_obj = monitor_qmp_requests_pop_any();
> >> >> > + Monitor *mon;
> >> >> > QDict *rsp;
> >> >> > bool need_resume;
> >> >> >
> >> >> > if (!req_obj) {
> >> >> > return;
> >> >> > }
> >> >> > -
> >> >>
> >> >> Let's keep the blank line.
> >> >
> >> > Ok.
> >> >
> >> >>
> >> >> > + mon = req_obj->mon;
> >> >> > /* qmp_oob_enabled() might change after "qmp_capabilities" */
> >> >> > - need_resume = !qmp_oob_enabled(req_obj->mon);
> >> >> > + qemu_mutex_lock(&mon->qmp.qmp_queue_lock);
> >> >> > + need_resume = !qmp_oob_enabled(req_obj->mon) ||
> >> >> > + mon->qmp.qmp_requests->length == QMP_REQ_QUEUE_LEN_MAX - 1;
> >> >>
> >> >> Note for later: this is the condition guarding monitor_resume().
> >> >>
> >> >> Is this race-free? We have two criticial sections, one in
> >> >> monitor_qmp_requests_pop_any(), and one here. What if two threads
> >> >> interleave like this
> >> >>
> >> >> thread 1 thread 2
> >> >> ------------------------------------------------------------------
> >> >> monitor_qmp_requests_pop_any()
> >> >> lock
> >> >> dequeue
> >> >> unlock
> >> >> monitor_qmp_requests_pop_any()
> >> >> lock
> >> >> dequeue
> >> >> unlock
> >> >> lock
> >> >> check queue length
> >> >> unlock
> >> >> if queue length demands it
> >> >> monitor_resume()
> >> >> lock
> >> >> check queue length
> >> >> unlock
> >> >> if queue length demands it
> >> >> monitor_resume()
> >> >>
> >> >> Looks racy to me: if we start with the queue full (and the monitor
> >> >> suspended), both threads' check queue length see length
> >> >> QMP_REQ_QUEUE_LEN_MAX - 2, and neither thread resumes the monitor.
> >> >> Oops.
> >> >>
> >> >> Simplest fix is probably moving the resume into
> >> >> monitor_qmp_requests_pop_any()'s critical section.
> >> >
> >> > But we only have one QMP dispatcher, right? So IMHO we can't have two
> >> > threads doing monitor_qmp_requests_pop_any() at the same time.
> >>
> >> You're right, we currently run monitor_qmp_bh_dispatcher() only in the
> >> main thread, and a thread can't race with itself. But the main thread
> >> can still race with handle_qmp_command() running in mon_iothread.
> >>
> >> main thread mon_iothread
> >> ------------------------------------------------------------------
> >> monitor_qmp_requests_pop_any()
> >> lock
> >> dequeue
> >> unlock
> >> lock
> >> if queue length demands it
> >> monitor_suspend()
> >> push onto queue
> >> unlock
> >> lock
> >> check queue length
> >> unlock
> >> if queue length demands it
> >> monitor_resume() <---------------------- [1]
> >>
> >> If we start with the queue full (and the monitor suspended), the main
> >> thread first determines it'll need to resume. mon_iothread then fills
> >> the queue again, and suspends the suspended monitor some more. If I
> >
> > (Side note: here it's tricky that when we "determines to resume" we
> > didn't really resume, so we are still suspended, hence the
> > mon_iothread cannot fill the queue again until the resume at [1]
>
> You're right.
>
> > above. Hence IMHO we're safe here. But I agree that it's still racy
> > in other cases.)
>
> Maybe.
>
> Getting threads to cooperate safely is hard. Key to success is making
> things as simple and obvious as we can. Suitable invariants help.
>
> They help even more when they're documented and checked with assertions.
> Perhaps you can think of ways to do that.
>
> >> read the code correctly, this increments mon->suspend_cnt from 1 to 2.
> >> Finally, the main thread checks the queue length:
> >>
> >> need_resume = !qmp_oob_enabled(req_obj->mon) ||
> >> mon->qmp.qmp_requests->length == QMP_REQ_QUEUE_LEN_MAX - 1;
> >>
> >> The yields false, because ->length is now QMP_REQ_QUEUE_LEN_MAX. The
> >> main thread does *not* resume the monitor.
> >>
> >> State after this: queue full, mon->suspend_cnt == 2. Bad, but we'll
> >> recover on the dequeue after next: the next dequeue decrements
> >> mon->suspend_cnt to 1 without resuming the monitor, the one after that
> >> will decrement it to 0 and resume the monitor.
> >>
> >> However, if handle_qmp_command() runs in this spot often enough to push
> >> mon->suspend_cnt above QMP_REQ_QUEUE_LEN_MAX, the monitor will remain
> >> suspended forever.
> >>
> >> I'm teaching you multiprogramming 101 here. The thing that should make
> >> the moderately experienced nose twitch is the anti-pattern
> >>
> >> begin critical section
> >> do something
> >> end critical section
> >> begin critical section
> >> if we just did X, state must be Y, so we must now do Z
> >> end critical section
> >>
> >> The part "if we just did X, state must be Y" is *wrong*. You have no
> >> idea what the state is, since code running between the two critical
> >> sections may have changed it.
> >>
> >> Here,
> >>
> >> X = dequeued from a full queue"
> >> Y = "mon->suspend_cnt == 1"
> >> Z = monitor_resume() to resume the monitor
> >>
> >> I showed that Y does not hold.
> >>
> >> On a slightly more abstract level, this anti-pattern applies:
> >>
> >> begin critical section
> >> start messing with invariant
> >> end critical section
> >> // invariant does not hold here, oopsie
> >> begin critical section
> >> finish messing with invariant
> >> end critical section
> >>
> >> The invariant here is "monitor suspended exactly when the queue is
> >> full". Your first critical section can make the queue non-full. The
> >> second one resumes. The invariant doesn't hold in between, and all bets
> >> are off.
> >>
> >> To maintain the invariant, you *have* to enqueue and suspend in a single
> >> critical section (which you do), *and* dequeue and resume in a single
> >> critical section (which you don't).
> >
> > Thank you for teaching the lesson for me.
> >
> >>
> >> > But I fully agree that it'll be nicer to keep them together (e.g. in
> >> > case we allow to dispatch two commands some day), but then if you see
> >> > it'll be something like the old req_obj->need_resume flag, but we set
> >> > it in monitor_qmp_requests_pop_any() instead. If so, I really prefer
> >> > we just keep that need_resume flag for per-monitor by dropping
> >> > 176160ce78 and keep my patch to move that flag into monitor struct
> >> > (which I dropped after the rebase of this version).
> >>
> >> Please have another look.
> >
> > Again, if we want to put pop+check into the same critical section,
> > then we possibly need to return something from the
> > monitor_qmp_requests_pop_any() to show the queue length information,
> > then I would prefer to keep the per-monitor need_resume flag.
> >
> > What do you think?
>
> Options:
>
> * Save rather than recompute @need_resume
>
> This gets rid of the second critical section.
This one I always liked. Actually it will avoid potential risk when
the conditions become more complicated (now it only contains two
checks). Meanwhile...
>
> * Have monitor_qmp_requests_pop_any() return @need_resume
>
> This merges the second critical section into the first.
(I don't like this one much...)
>
> * Have a single critical section in monitor_qmp_bh_dispatcher()
>
> This replaces both critical sections by a single larger one.
> Callers of monitor_qmp_bh_dispatcher() must hold monitor_lock.
... I like this one too since it's clean enough at least for now and
it won't bother you to drop one queued patch in monitor-next.
I'll use this one if no one disagrees.
>
> * Other ideas?
>
> The question to ask regardless of option: what invariant do the critical
> sections maintain?
>
> I proposed one: monitor suspended exactly when the queue is full.
>
> handle_qmp_command()'s critical section maintains it: it suspends when
> its enqueue fills up the queue.
>
> monitor_qmp_bh_dispatcher()'s critical sections do not. Even if we
> reduce them from two to one, the resulting single critical section can't
> as long as we resume only after the dequeued command completed, because
> that would require holding monitor_lock while the command executes. So,
> to maintain this invariant, we need to rethink
> monitor_qmp_bh_dispatcher(). Why can't we resume right after dequeuing?
Makes sense. I can do that in my next post if you like.
>
> You're of course welcome to propose a different invariant.
>
> [...]
Thanks,
--
Peter Xu
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [PATCH v7 4/7] qapi: remove COMMAND_DROPPED event
2018-09-04 5:30 ` Peter Xu
@ 2018-09-04 8:04 ` Markus Armbruster
2018-09-05 3:53 ` Peter Xu
0 siblings, 1 reply; 28+ messages in thread
From: Markus Armbruster @ 2018-09-04 8:04 UTC (permalink / raw)
To: Peter Xu
Cc: Daniel P. Berrangé,
Marc-André Lureau, Dr . David Alan Gilbert, qemu-devel
Peter Xu <peterx@redhat.com> writes:
> On Mon, Sep 03, 2018 at 03:41:16PM +0100, Daniel P. Berrangé wrote:
>> On Mon, Sep 03, 2018 at 09:30:52AM -0500, Eric Blake wrote:
>> > On 09/03/2018 08:31 AM, Markus Armbruster wrote:
>> >
>> > > Example:
>> > >
>> > > client sends in-band command #1
>> > > QEMU reads and queues
>> > > QEMU dequeues in-band command #1
>> > > in-band command #1 starts executing, but it's slooow
>> > > client sends in-band command #2
>> > > QEMU reads and queues
>> > > ...
>> > > client sends in-band command #8
>> > > QEMU reads, queues and suspends the monitor
>> > > client sends out-of-band command
>> > > --> time passes...
>> > > in-band command #1 completes, QEMU sends reply
>> > > QEMU dequeues in-band command #2, resumes the monitor
>> > > in-band command #2 starts executing
>> > > QEMU reads and executes out-of-band command
>> > > out-of-band command completes, QEMU sends reply
>> > > in-band command #2 completes, QEMU sends reply
>> > > ... same for remaining in-band commands ...
>> > >
>> > > The out-of-band command gets stuck behind the in-band commands.
>
> (It's a shame of me to have just noticed that the out-of-band command
> will be stuck after we dropped the COMMAND_DROP event... so now I
> agree it's not that ideal any more to drop the event but maybe still
> preferable)
We can queue without limit, we can drop commands, or we can suspend
reading. Each of these has drawbacks:
* Queuing without limit is simple for the client, but unsafe for QEMU.
* Dropping commands requires the client to cope with dropped commands.
As currently designed, it's just as unsafe for QEMU: instead of
queuing commands without limit, we get to queue their COMMAND_DROPPED
events without limit. A better design could avoid this flaw.
* Suspending reading requires the client to manage the flow of commands
if it wants to keep the monitor available for out-of-band commands.
We decided that clients having to manage the flow of commands is no
worse than clients having to cope with dropped commands. There's still
time to challenge this decision.
This series acts upon the decision: it switches from dropping commands
to suspending reading. Makes the input direction safe. The output
direction remains as unsafe as it's always been. Fixing that is left
for later.
>> > >
>> > > The client can avoid this by managing the flow of in-band commands: have
>> > > no more than 7 in flight, so the monitor never gets suspended.
>> > >
>> > > This is a potentially useful thing to do for clients, isn't it?
>> > >
>> > > Eric, Daniel, is it something libvirt would do?
>> >
>> > Right now, libvirt serializes commands - it never sends a QMP command until
>> > the previous command's response has been processed. But that may not help
>> > much, since libvirt does not send OOB commands.
>>
>> Note that is not merely due to the QMP monitor restriction either.
>>
>> Libvirt serializes all its public APIs that can change state of a running
>> domain. It usually aims to allow read-only APIs to be run in parallel with
>> APIs that change state.
>>
>> The exception to the rule right now are some of the migration APIs which
>> we allow to be invoked to manage the migration process.
>>
>> > I guess when we are designing what libvirt should do, and deciding WHEN it
>> > should send OOB commands, we have the luxury of designing libvirt to enforce
>> > how many in-flight in-band commands it will ever have pending at once
>> > (whether the current 'at most 1', or even if we make it more parallel to 'at
>> > most 7'), so that we can still be ensured that the OOB command will be
>> > processed without being stuck in the queue of suspended in-band commands.
>> > If we never send more than one in-band at a time, then it's not a concern
>> > how deep the qemu queue is; but if we do want libvirt to start parallel
>> > in-band commands, then you are right that having a way to learn the qemu
>> > queue depth is programmatically more precise than just guessing the maximum
>> > depth. But it's also hard to argue we need that complexity if we don't have
>> > an immediate use envisioned for it.
>>
>> In terms of what libvirt would want to parallelize, I think it is reasonable
>> to consider any of the query-XXXX commands desirable. Other stuff is likely
>> to remain serialized from libvirt's side.
>
> IMHO concurrency won't help much now even for query commands, since
> our current concurrency is still "partly" - the executions of query
> commands (which is the most time consuming part) will still be done
> sequentially, so even if we send multiple query commands in parallel
> (without waiting for a response of any sent commands), the total time
> used for the list of commands would be mostly the same.
Yes. We execute all in-band commands (regardless of their monitor) in
the main thread. Out-of-band commands can execute in @mon_iothread,
which provides a modest degree of concurrency.
> My understanding for why we have such a queue length now is that it
> came from a security concern: after we have a queue, we need that
> queue length to limit the memory usages for the QMP server. Though
> that might not help much for real users like Libvirt, it's majorly
> serving as a way to protect QEMU QMP from being attacked or from being
> turned down by a buggy QMP client.
Yes.
QEMU has to trust its QMP clients, so malice is not a concern, but
accidents are. Robust software does not buffer without bounds.
> But I agree now that the queue length information might still be
> helpful some day. Maybe, we can hide that until we support executing
> commands in parallel for some of them.
Queue length can become interesting long before we get general
concurrency.
If you use QMP only synchronously (send command #1; receive reply #1;
send command #2; ...), then out-of-band does exactly nothing for you.
To make use of it, you have to send an out-of-band command *before* you
receive the previous command's reply. That's a form of pipelining.
Note there's still no general concurrency. There's a bit of pipelining,
and there's a bit of concurrency between one in-band command (executing
in main thread) and out-of-band command (executing in @mon_iothread).
Since we need to support a bit of pipelining anyway, why not support it
more generally? All it takes it raising the queue length limit above
the minimum required for the use of OOB I just sketched.
Note that "since we need to support a bit of concurrency anyway, why not
support it more generally?" would be ludicrously naive :)
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [PATCH v7 4/7] qapi: remove COMMAND_DROPPED event
2018-09-04 6:39 ` Markus Armbruster
@ 2018-09-04 8:23 ` Daniel P. Berrangé
2018-09-04 11:46 ` Markus Armbruster
0 siblings, 1 reply; 28+ messages in thread
From: Daniel P. Berrangé @ 2018-09-04 8:23 UTC (permalink / raw)
To: Markus Armbruster
Cc: Eric Blake, Marc-André Lureau, Dr . David Alan Gilbert,
Peter Xu, qemu-devel
On Tue, Sep 04, 2018 at 08:39:27AM +0200, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
>
> > On Mon, Sep 03, 2018 at 09:30:52AM -0500, Eric Blake wrote:
> >> On 09/03/2018 08:31 AM, Markus Armbruster wrote:
> >>
> >> > Example:
> >> >
> >> > client sends in-band command #1
> >> > QEMU reads and queues
> >> > QEMU dequeues in-band command #1
> >> > in-band command #1 starts executing, but it's slooow
> >> > client sends in-band command #2
> >> > QEMU reads and queues
> >> > ...
> >> > client sends in-band command #8
> >> > QEMU reads, queues and suspends the monitor
> >> > client sends out-of-band command
> >> > --> time passes...
> >> > in-band command #1 completes, QEMU sends reply
> >> > QEMU dequeues in-band command #2, resumes the monitor
> >> > in-band command #2 starts executing
> >> > QEMU reads and executes out-of-band command
> >> > out-of-band command completes, QEMU sends reply
> >> > in-band command #2 completes, QEMU sends reply
> >> > ... same for remaining in-band commands ...
> >> >
> >> > The out-of-band command gets stuck behind the in-band commands.
> >> >
> >> > The client can avoid this by managing the flow of in-band commands: have
> >> > no more than 7 in flight, so the monitor never gets suspended.
> >> >
> >> > This is a potentially useful thing to do for clients, isn't it?
> >> >
> >> > Eric, Daniel, is it something libvirt would do?
> >>
> >> Right now, libvirt serializes commands - it never sends a QMP command until
> >> the previous command's response has been processed. But that may not help
> >> much, since libvirt does not send OOB commands.
> >
> > Note that is not merely due to the QMP monitor restriction either.
> >
> > Libvirt serializes all its public APIs that can change state of a running
> > domain. It usually aims to allow read-only APIs to be run in parallel with
> > APIs that change state.
>
> Makes sense. State changes are complex enough without concurrency.
> Even permitting just concurrent queries can add non-trivial complexity.
>
> However, pipelineing != concurrency. "Serializing" as I understand it
> implies no concurrency, it doesn't imply no pipelining.
>
> Mind, I'm not telling you to pipeline, I'm just trying to understand the
> constraints.
I can only think of one place where libvirt would be likely to use
pipelining. We have an API that is used for collecting bulk stats
of many types, across al VMs. We do a couple of QMP queries per-VM,
so we could pipeline those queries. Even then, I'm not sure we would
go to the bother, as the bigger burden for performance is that we
round-robin across every VM. A bigger bang for our buck would be
to parallelize across VMs, but still serialize within VMs, as that
would have less complexity.
> > The exception to the rule right now are some of the migration APIs which
> > we allow to be invoked to manage the migration process.
>
> Can you give an example?
We have a job that triggers the migration and sits in a thread
monitoring its progress.
While this is happening we allow a few other API calls such as
"pause", and of course things like switching to post-copy mode,
changin max downtime, etc. None of this gets in to parallel
or pipelined monitor execution though.
> >> I guess when we are designing what libvirt should do, and deciding WHEN it
> >> should send OOB commands, we have the luxury of designing libvirt to enforce
> >> how many in-flight in-band commands it will ever have pending at once
> >> (whether the current 'at most 1', or even if we make it more parallel to 'at
> >> most 7'), so that we can still be ensured that the OOB command will be
> >> processed without being stuck in the queue of suspended in-band commands.
> >> If we never send more than one in-band at a time, then it's not a concern
> >> how deep the qemu queue is; but if we do want libvirt to start parallel
> >> in-band commands, then you are right that having a way to learn the qemu
> >> queue depth is programmatically more precise than just guessing the maximum
> >> depth. But it's also hard to argue we need that complexity if we don't have
> >> an immediate use envisioned for it.
>
> True.
>
> Options for the initial interface:
>
> (1) Provide means for the client to determine the queue length limit
> (introspection or configuration). Clients that need the monitory to
> remain available for out-of-band commands can keep limit - 1 in-band
> commands in flight.
>
> (2) Make the queue length limit part of the documented interface.
> Clients that need the monitory to remain available for out-of-band
> commands can keep limit - 1 in-band commands in flight. We can
> increase the limit later, but not decrease it. We can also switch
> to (1) as needed.
>
> (3) Treat the queue length limit as implementation detail (but tacitly
> assume its at least 2, since less makes no sense[*]). Clients that
> need the monitory to remain available for out-of-band commands
> cannot safely keep more than one in-band command in flight. We can
> switch to (2) or (1) as needed.
>
> Opinions?
If you did (3), effectively apps will be behaving as if you had done
(2) with a documented queue limit of 2, so why not just do (2) right
away.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [PATCH v7 4/7] qapi: remove COMMAND_DROPPED event
2018-09-04 8:23 ` Daniel P. Berrangé
@ 2018-09-04 11:46 ` Markus Armbruster
2018-09-05 11:45 ` Dr. David Alan Gilbert
0 siblings, 1 reply; 28+ messages in thread
From: Markus Armbruster @ 2018-09-04 11:46 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Marc-André Lureau, Dr . David Alan Gilbert, Peter Xu, qemu-devel
Daniel P. Berrangé <berrange@redhat.com> writes:
> On Tue, Sep 04, 2018 at 08:39:27AM +0200, Markus Armbruster wrote:
>> Daniel P. Berrangé <berrange@redhat.com> writes:
>>
>> > On Mon, Sep 03, 2018 at 09:30:52AM -0500, Eric Blake wrote:
>> >> On 09/03/2018 08:31 AM, Markus Armbruster wrote:
>> >>
>> >> > Example:
>> >> >
>> >> > client sends in-band command #1
>> >> > QEMU reads and queues
>> >> > QEMU dequeues in-band command #1
>> >> > in-band command #1 starts executing, but it's slooow
>> >> > client sends in-band command #2
>> >> > QEMU reads and queues
>> >> > ...
>> >> > client sends in-band command #8
>> >> > QEMU reads, queues and suspends the monitor
>> >> > client sends out-of-band command
>> >> > --> time passes...
>> >> > in-band command #1 completes, QEMU sends reply
>> >> > QEMU dequeues in-band command #2, resumes the monitor
>> >> > in-band command #2 starts executing
>> >> > QEMU reads and executes out-of-band command
>> >> > out-of-band command completes, QEMU sends reply
>> >> > in-band command #2 completes, QEMU sends reply
>> >> > ... same for remaining in-band commands ...
>> >> >
>> >> > The out-of-band command gets stuck behind the in-band commands.
>> >> >
>> >> > The client can avoid this by managing the flow of in-band commands: have
>> >> > no more than 7 in flight, so the monitor never gets suspended.
>> >> >
>> >> > This is a potentially useful thing to do for clients, isn't it?
>> >> >
>> >> > Eric, Daniel, is it something libvirt would do?
>> >>
>> >> Right now, libvirt serializes commands - it never sends a QMP command until
>> >> the previous command's response has been processed. But that may not help
>> >> much, since libvirt does not send OOB commands.
>> >
>> > Note that is not merely due to the QMP monitor restriction either.
>> >
>> > Libvirt serializes all its public APIs that can change state of a running
>> > domain. It usually aims to allow read-only APIs to be run in parallel with
>> > APIs that change state.
>>
>> Makes sense. State changes are complex enough without concurrency.
>> Even permitting just concurrent queries can add non-trivial complexity.
>>
>> However, pipelineing != concurrency. "Serializing" as I understand it
>> implies no concurrency, it doesn't imply no pipelining.
>>
>> Mind, I'm not telling you to pipeline, I'm just trying to understand the
>> constraints.
>
> I can only think of one place where libvirt would be likely to use
> pipelining. We have an API that is used for collecting bulk stats
> of many types, across al VMs. We do a couple of QMP queries per-VM,
> so we could pipeline those queries. Even then, I'm not sure we would
> go to the bother, as the bigger burden for performance is that we
> round-robin across every VM. A bigger bang for our buck would be
> to parallelize across VMs, but still serialize within VMs, as that
> would have less complexity.
>
>> > The exception to the rule right now are some of the migration APIs which
>> > we allow to be invoked to manage the migration process.
>>
>> Can you give an example?
>
> We have a job that triggers the migration and sits in a thread
> monitoring its progress.
>
> While this is happening we allow a few other API calls such as
> "pause", and of course things like switching to post-copy mode,
> changin max downtime, etc. None of this gets in to parallel
> or pipelined monitor execution though.
At the QMP command level, these are all in-band commands issued
synchronously, except for out-of-band migrate-recover and migrate-pause.
>> >> I guess when we are designing what libvirt should do, and deciding WHEN it
>> >> should send OOB commands, we have the luxury of designing libvirt to enforce
>> >> how many in-flight in-band commands it will ever have pending at once
>> >> (whether the current 'at most 1', or even if we make it more parallel to 'at
>> >> most 7'), so that we can still be ensured that the OOB command will be
>> >> processed without being stuck in the queue of suspended in-band commands.
>> >> If we never send more than one in-band at a time, then it's not a concern
>> >> how deep the qemu queue is; but if we do want libvirt to start parallel
>> >> in-band commands, then you are right that having a way to learn the qemu
>> >> queue depth is programmatically more precise than just guessing the maximum
>> >> depth. But it's also hard to argue we need that complexity if we don't have
>> >> an immediate use envisioned for it.
>>
>> True.
>>
>> Options for the initial interface:
>>
>> (1) Provide means for the client to determine the queue length limit
>> (introspection or configuration). Clients that need the monitory to
>> remain available for out-of-band commands can keep limit - 1 in-band
>> commands in flight.
>>
>> (2) Make the queue length limit part of the documented interface.
>> Clients that need the monitory to remain available for out-of-band
>> commands can keep limit - 1 in-band commands in flight. We can
>> increase the limit later, but not decrease it. We can also switch
>> to (1) as needed.
>>
>> (3) Treat the queue length limit as implementation detail (but tacitly
>> assume its at least 2, since less makes no sense[*]). Clients that
>> need the monitory to remain available for out-of-band commands
>> cannot safely keep more than one in-band command in flight. We can
>> switch to (2) or (1) as needed.
>>
>> Opinions?
>
> If you did (3), effectively apps will be behaving as if you had done
> (2) with a documented queue limit of 2, so why not just do (2) right
> away.
Yup, that's what I thought, too.
I append a proposed replacement for the patch to qmp-spec.txt. It
assumes the current queue size 8. That value is of course open to
debate.
diff --git a/docs/interop/qmp-spec.txt b/docs/interop/qmp-spec.txt
index 67e44a8120..1fc6a507e2 100644
--- a/docs/interop/qmp-spec.txt
+++ b/docs/interop/qmp-spec.txt
@@ -130,9 +130,11 @@ to pass "id" with out-of-band commands. Passing it with all commands
is recommended for clients that accept capability "oob".
If the client sends in-band commands faster than the server can
-execute them, the server will stop reading the requests from the QMP
-channel until the request queue length is reduced to an acceptable
-range.
+execute them, the server's queue will fill up, and the server will
+stop reading commands from the QMP channel until the queue has space
+again. Clients that need the server to remain responsive to
+out-of-band commands should avoid filling up the queue by limiting the
+number of in-band commands in flight to seven.
Only a few commands support out-of-band execution. The ones that do
have "allow-oob": true in output of query-qmp-schema.
^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [PATCH v7 4/7] qapi: remove COMMAND_DROPPED event
2018-09-04 8:04 ` Markus Armbruster
@ 2018-09-05 3:53 ` Peter Xu
0 siblings, 0 replies; 28+ messages in thread
From: Peter Xu @ 2018-09-05 3:53 UTC (permalink / raw)
To: Markus Armbruster
Cc: Daniel P. Berrangé,
Marc-André Lureau, Dr . David Alan Gilbert, qemu-devel
On Tue, Sep 04, 2018 at 10:04:00AM +0200, Markus Armbruster wrote:
> Peter Xu <peterx@redhat.com> writes:
>
> > On Mon, Sep 03, 2018 at 03:41:16PM +0100, Daniel P. Berrangé wrote:
> >> On Mon, Sep 03, 2018 at 09:30:52AM -0500, Eric Blake wrote:
> >> > On 09/03/2018 08:31 AM, Markus Armbruster wrote:
> >> >
> >> > > Example:
> >> > >
> >> > > client sends in-band command #1
> >> > > QEMU reads and queues
> >> > > QEMU dequeues in-band command #1
> >> > > in-band command #1 starts executing, but it's slooow
> >> > > client sends in-band command #2
> >> > > QEMU reads and queues
> >> > > ...
> >> > > client sends in-band command #8
> >> > > QEMU reads, queues and suspends the monitor
> >> > > client sends out-of-band command
> >> > > --> time passes...
> >> > > in-band command #1 completes, QEMU sends reply
> >> > > QEMU dequeues in-band command #2, resumes the monitor
> >> > > in-band command #2 starts executing
> >> > > QEMU reads and executes out-of-band command
> >> > > out-of-band command completes, QEMU sends reply
> >> > > in-band command #2 completes, QEMU sends reply
> >> > > ... same for remaining in-band commands ...
> >> > >
> >> > > The out-of-band command gets stuck behind the in-band commands.
> >
> > (It's a shame of me to have just noticed that the out-of-band command
> > will be stuck after we dropped the COMMAND_DROP event... so now I
> > agree it's not that ideal any more to drop the event but maybe still
> > preferable)
>
> We can queue without limit, we can drop commands, or we can suspend
> reading. Each of these has drawbacks:
>
> * Queuing without limit is simple for the client, but unsafe for QEMU.
>
> * Dropping commands requires the client to cope with dropped commands.
> As currently designed, it's just as unsafe for QEMU: instead of
> queuing commands without limit, we get to queue their COMMAND_DROPPED
> events without limit. A better design could avoid this flaw.
>
> * Suspending reading requires the client to manage the flow of commands
> if it wants to keep the monitor available for out-of-band commands.
>
> We decided that clients having to manage the flow of commands is no
> worse than clients having to cope with dropped commands. There's still
> time to challenge this decision.
>
> This series acts upon the decision: it switches from dropping commands
> to suspending reading. Makes the input direction safe. The output
> direction remains as unsafe as it's always been. Fixing that is left
> for later.
Yes. Options (1) and (2) seems not really acceptable for me, but my
conclusion is based on that I think QEMU should still protect itself
from the client. Take the example of QEMU & Libvirt: I think death or
bug of either of the program should not affect the other one. But
maybe I misunderstood somewhere since I saw that you emphasized it at
[1] below...
And for (3), I really think a proper client should never trigger that
queue full state. Hopefully with that then the client would never
lost the out-of-band feature due to a stuck input channel.
>
> >> > >
> >> > > The client can avoid this by managing the flow of in-band commands: have
> >> > > no more than 7 in flight, so the monitor never gets suspended.
> >> > >
> >> > > This is a potentially useful thing to do for clients, isn't it?
> >> > >
> >> > > Eric, Daniel, is it something libvirt would do?
> >> >
> >> > Right now, libvirt serializes commands - it never sends a QMP command until
> >> > the previous command's response has been processed. But that may not help
> >> > much, since libvirt does not send OOB commands.
> >>
> >> Note that is not merely due to the QMP monitor restriction either.
> >>
> >> Libvirt serializes all its public APIs that can change state of a running
> >> domain. It usually aims to allow read-only APIs to be run in parallel with
> >> APIs that change state.
> >>
> >> The exception to the rule right now are some of the migration APIs which
> >> we allow to be invoked to manage the migration process.
> >>
> >> > I guess when we are designing what libvirt should do, and deciding WHEN it
> >> > should send OOB commands, we have the luxury of designing libvirt to enforce
> >> > how many in-flight in-band commands it will ever have pending at once
> >> > (whether the current 'at most 1', or even if we make it more parallel to 'at
> >> > most 7'), so that we can still be ensured that the OOB command will be
> >> > processed without being stuck in the queue of suspended in-band commands.
> >> > If we never send more than one in-band at a time, then it's not a concern
> >> > how deep the qemu queue is; but if we do want libvirt to start parallel
> >> > in-band commands, then you are right that having a way to learn the qemu
> >> > queue depth is programmatically more precise than just guessing the maximum
> >> > depth. But it's also hard to argue we need that complexity if we don't have
> >> > an immediate use envisioned for it.
> >>
> >> In terms of what libvirt would want to parallelize, I think it is reasonable
> >> to consider any of the query-XXXX commands desirable. Other stuff is likely
> >> to remain serialized from libvirt's side.
> >
> > IMHO concurrency won't help much now even for query commands, since
> > our current concurrency is still "partly" - the executions of query
> > commands (which is the most time consuming part) will still be done
> > sequentially, so even if we send multiple query commands in parallel
> > (without waiting for a response of any sent commands), the total time
> > used for the list of commands would be mostly the same.
>
> Yes. We execute all in-band commands (regardless of their monitor) in
> the main thread. Out-of-band commands can execute in @mon_iothread,
> which provides a modest degree of concurrency.
>
> > My understanding for why we have such a queue length now is that it
> > came from a security concern: after we have a queue, we need that
> > queue length to limit the memory usages for the QMP server. Though
> > that might not help much for real users like Libvirt, it's majorly
> > serving as a way to protect QEMU QMP from being attacked or from being
> > turned down by a buggy QMP client.
>
> Yes.
>
> QEMU has to trust its QMP clients, so malice is not a concern, but
> accidents are. Robust software does not buffer without bounds.
[1]
>
> > But I agree now that the queue length information might still be
> > helpful some day. Maybe, we can hide that until we support executing
> > commands in parallel for some of them.
>
> Queue length can become interesting long before we get general
> concurrency.
>
> If you use QMP only synchronously (send command #1; receive reply #1;
> send command #2; ...), then out-of-band does exactly nothing for you.
> To make use of it, you have to send an out-of-band command *before* you
> receive the previous command's reply. That's a form of pipelining.
Yes, out-of-band should be special here, but as Dave has already
mentioned (possibly someone else too) that we may just need a length=1
queue for in-band command and length=1 queue for out-of-band command
and that should be enough at least for now (say, oob command will
never block, and oob commands will be executed once a time). By that
extra length=1 out-of-band queue we gain the ability to talk to QMP
any time we want when necessary (though with limited list of cmds).
>
> Note there's still no general concurrency. There's a bit of pipelining,
> and there's a bit of concurrency between one in-band command (executing
> in main thread) and out-of-band command (executing in @mon_iothread).
>
> Since we need to support a bit of pipelining anyway, why not support it
> more generally? All it takes it raising the queue length limit above
> the minimum required for the use of OOB I just sketched.
>
> Note that "since we need to support a bit of concurrency anyway, why not
> support it more generally?" would be ludicrously naive :)
Regards,
--
Peter Xu
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [PATCH v7 4/7] qapi: remove COMMAND_DROPPED event
2018-09-04 11:46 ` Markus Armbruster
@ 2018-09-05 11:45 ` Dr. David Alan Gilbert
0 siblings, 0 replies; 28+ messages in thread
From: Dr. David Alan Gilbert @ 2018-09-05 11:45 UTC (permalink / raw)
To: Markus Armbruster
Cc: Daniel P. Berrangé, Marc-André Lureau, Peter Xu, qemu-devel
* Markus Armbruster (armbru@redhat.com) wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
>
> > On Tue, Sep 04, 2018 at 08:39:27AM +0200, Markus Armbruster wrote:
> >> Daniel P. Berrangé <berrange@redhat.com> writes:
> >>
> >> > On Mon, Sep 03, 2018 at 09:30:52AM -0500, Eric Blake wrote:
> >> >> On 09/03/2018 08:31 AM, Markus Armbruster wrote:
> >> >> I guess when we are designing what libvirt should do, and deciding WHEN it
> >> >> should send OOB commands, we have the luxury of designing libvirt to enforce
> >> >> how many in-flight in-band commands it will ever have pending at once
> >> >> (whether the current 'at most 1', or even if we make it more parallel to 'at
> >> >> most 7'), so that we can still be ensured that the OOB command will be
> >> >> processed without being stuck in the queue of suspended in-band commands.
> >> >> If we never send more than one in-band at a time, then it's not a concern
> >> >> how deep the qemu queue is; but if we do want libvirt to start parallel
> >> >> in-band commands, then you are right that having a way to learn the qemu
> >> >> queue depth is programmatically more precise than just guessing the maximum
> >> >> depth. But it's also hard to argue we need that complexity if we don't have
> >> >> an immediate use envisioned for it.
> >>
> >> True.
> >>
> >> Options for the initial interface:
> >>
> >> (1) Provide means for the client to determine the queue length limit
> >> (introspection or configuration). Clients that need the monitory to
> >> remain available for out-of-band commands can keep limit - 1 in-band
> >> commands in flight.
> >>
> >> (2) Make the queue length limit part of the documented interface.
> >> Clients that need the monitory to remain available for out-of-band
> >> commands can keep limit - 1 in-band commands in flight. We can
> >> increase the limit later, but not decrease it. We can also switch
> >> to (1) as needed.
> >>
> >> (3) Treat the queue length limit as implementation detail (but tacitly
> >> assume its at least 2, since less makes no sense[*]). Clients that
> >> need the monitory to remain available for out-of-band commands
> >> cannot safely keep more than one in-band command in flight. We can
> >> switch to (2) or (1) as needed.
> >>
> >> Opinions?
> >
> > If you did (3), effectively apps will be behaving as if you had done
> > (2) with a documented queue limit of 2, so why not just do (2) right
> > away.
>
> Yup, that's what I thought, too.
>
> I append a proposed replacement for the patch to qmp-spec.txt. It
> assumes the current queue size 8. That value is of course open to
> debate.
Could you return that size in the response to qmp_capabilities
at the start of the connection?
>
> diff --git a/docs/interop/qmp-spec.txt b/docs/interop/qmp-spec.txt
> index 67e44a8120..1fc6a507e2 100644
> --- a/docs/interop/qmp-spec.txt
> +++ b/docs/interop/qmp-spec.txt
> @@ -130,9 +130,11 @@ to pass "id" with out-of-band commands. Passing it with all commands
> is recommended for clients that accept capability "oob".
>
> If the client sends in-band commands faster than the server can
> -execute them, the server will stop reading the requests from the QMP
> -channel until the request queue length is reduced to an acceptable
> -range.
> +execute them, the server's queue will fill up, and the server will
> +stop reading commands from the QMP channel until the queue has space
> +again. Clients that need the server to remain responsive to
> +out-of-band commands should avoid filling up the queue by limiting the
> +number of in-band commands in flight to seven.
If I understand what you're saying then this is a shared limit; i.e.
if you've got two QMP connections then you have to be aware of how many
the other connection is queuing, which is tricky.
Dave
> Only a few commands support out-of-band execution. The ones that do
> have "allow-oob": true in output of query-qmp-schema.
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 28+ messages in thread
end of thread, other threads:[~2018-09-05 11:45 UTC | newest]
Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-09-03 4:31 [Qemu-devel] [PATCH v7 0/7] monitor: enable OOB by default Peter Xu
2018-09-03 4:31 ` [Qemu-devel] [PATCH v7 1/7] qapi: Fix build_params() for empty parameter list Peter Xu
2018-09-03 4:31 ` [Qemu-devel] [PATCH v7 2/7] qapi: Drop qapi_event_send_FOO()'s Error ** argument Peter Xu
2018-09-03 4:31 ` [Qemu-devel] [PATCH v7 3/7] monitor: suspend monitor instead of send CMD_DROP Peter Xu
2018-09-03 7:38 ` Markus Armbruster
2018-09-03 7:56 ` Markus Armbruster
2018-09-03 9:06 ` Peter Xu
2018-09-03 13:16 ` Markus Armbruster
2018-09-04 3:33 ` Peter Xu
2018-09-04 6:17 ` Markus Armbruster
2018-09-04 7:01 ` Peter Xu
2018-09-03 4:31 ` [Qemu-devel] [PATCH v7 4/7] qapi: remove COMMAND_DROPPED event Peter Xu
2018-09-03 7:49 ` Markus Armbruster
2018-09-03 10:16 ` Peter Xu
2018-09-03 13:31 ` Markus Armbruster
2018-09-03 14:30 ` Eric Blake
2018-09-03 14:41 ` Daniel P. Berrangé
2018-09-04 5:30 ` Peter Xu
2018-09-04 8:04 ` Markus Armbruster
2018-09-05 3:53 ` Peter Xu
2018-09-04 6:39 ` Markus Armbruster
2018-09-04 8:23 ` Daniel P. Berrangé
2018-09-04 11:46 ` Markus Armbruster
2018-09-05 11:45 ` Dr. David Alan Gilbert
2018-09-03 4:31 ` [Qemu-devel] [PATCH v7 5/7] monitor: remove "x-oob", turn oob on by default Peter Xu
2018-09-03 4:31 ` [Qemu-devel] [PATCH v7 6/7] Revert "tests: Add parameter to qtest_init_without_qmp_handshake" Peter Xu
2018-09-03 4:31 ` [Qemu-devel] [PATCH v7 7/7] tests: add oob functional test for test-qmp-cmds Peter Xu
2018-09-03 5:36 ` [Qemu-devel] [PATCH v7 0/7] monitor: enable OOB by default Markus Armbruster
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.