* [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
* 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 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 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 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 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 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
* [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
* 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 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 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 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 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 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-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 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 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
* [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
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.