From d90226808b5b1b30b07968e94d8f74bf8804fc89 Mon Sep 17 00:00:00 2001 From: Zenghui Yu Date: Fri, 12 Mar 2021 19:21:43 +0800 Subject: [PATCH 1/8] multi-process: Initialize variables declared with g_auto* MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Quote docs/devel/style.rst (section "Automatic memory deallocation"): * Variables declared with g_auto* MUST always be initialized, otherwise the cleanup function will use uninitialized stack memory Initialize @name properly to get rid of the compilation error (using gcc-7.3.0 on CentOS): ../hw/remote/proxy.c: In function 'pci_proxy_dev_realize': /usr/include/glib-2.0/glib/glib-autocleanups.h:28:3: error: 'name' may be used uninitialized in this function [-Werror=maybe-uninitialized] g_free (*pp); ^~~~~~~~~~~~ ../hw/remote/proxy.c:350:30: note: 'name' was declared here g_autofree char *name; ^~~~ Signed-off-by: Zenghui Yu Reviewed-by: Jagannathan Raman Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Miroslav Rezanina Message-id: 20210312112143.1369-1-yuzenghui@huawei.com Signed-off-by: Stefan Hajnoczi --- hw/remote/memory.c | 5 ++--- hw/remote/proxy.c | 3 +-- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/hw/remote/memory.c b/hw/remote/memory.c index 2d4174614af6..472ed2a272e3 100644 --- a/hw/remote/memory.c +++ b/hw/remote/memory.c @@ -41,10 +41,9 @@ void remote_sysmem_reconfig(MPQemuMsg *msg, Error **errp) remote_sysmem_reset(); - for (region = 0; region < msg->num_fds; region++) { - g_autofree char *name; + for (region = 0; region < msg->num_fds; region++, suffix++) { + g_autofree char *name = g_strdup_printf("remote-mem-%u", suffix); subregion = g_new(MemoryRegion, 1); - name = g_strdup_printf("remote-mem-%u", suffix++); memory_region_init_ram_from_fd(subregion, NULL, name, sysmem_info->sizes[region], true, msg->fds[region], diff --git a/hw/remote/proxy.c b/hw/remote/proxy.c index 4fa4be079d76..6dda705fc24c 100644 --- a/hw/remote/proxy.c +++ b/hw/remote/proxy.c @@ -347,13 +347,12 @@ static void probe_pci_info(PCIDevice *dev, Error **errp) PCI_BASE_ADDRESS_SPACE_IO : PCI_BASE_ADDRESS_SPACE_MEMORY; if (size) { - g_autofree char *name; + g_autofree char *name = g_strdup_printf("bar-region-%d", i); pdev->region[i].dev = pdev; pdev->region[i].present = true; if (type == PCI_BASE_ADDRESS_SPACE_MEMORY) { pdev->region[i].memory = true; } - name = g_strdup_printf("bar-region-%d", i); memory_region_init_io(&pdev->region[i].mr, OBJECT(pdev), &proxy_mr_ops, &pdev->region[i], name, size); From 5c6ae58d4bdca71f0b63e854ceb6a8ee67cbddd8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philippe=20Mathieu-Daud=C3=A9?= Date: Mon, 10 May 2021 22:07:57 +0200 Subject: [PATCH 2/8] bitops.h: Improve find_xxx_bit() documentation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Document the following functions return the bitmap size if no matching bit is found: - find_first_bit - find_next_bit - find_last_bit - find_first_zero_bit - find_next_zero_bit Reviewed-by: Richard Henderson Signed-off-by: Philippe Mathieu-Daudé Reviewed-by: Stefan Hajnoczi Message-id: 20210510200758.2623154-2-philmd@redhat.com Signed-off-by: Stefan Hajnoczi --- include/qemu/bitops.h | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/include/qemu/bitops.h b/include/qemu/bitops.h index 3acbf3384c63..a72f69fea85f 100644 --- a/include/qemu/bitops.h +++ b/include/qemu/bitops.h @@ -140,7 +140,8 @@ static inline int test_bit(long nr, const unsigned long *addr) * @addr: The address to start the search at * @size: The maximum size to search * - * Returns the bit number of the first set bit, or size. + * Returns the bit number of the last set bit, + * or @size if there is no set bit in the bitmap. */ unsigned long find_last_bit(const unsigned long *addr, unsigned long size); @@ -150,6 +151,9 @@ unsigned long find_last_bit(const unsigned long *addr, * @addr: The address to base the search on * @offset: The bitnumber to start searching at * @size: The bitmap size in bits + * + * Returns the bit number of the next set bit, + * or @size if there are no further set bits in the bitmap. */ unsigned long find_next_bit(const unsigned long *addr, unsigned long size, @@ -160,6 +164,9 @@ unsigned long find_next_bit(const unsigned long *addr, * @addr: The address to base the search on * @offset: The bitnumber to start searching at * @size: The bitmap size in bits + * + * Returns the bit number of the next cleared bit, + * or @size if there are no further clear bits in the bitmap. */ unsigned long find_next_zero_bit(const unsigned long *addr, @@ -171,7 +178,8 @@ unsigned long find_next_zero_bit(const unsigned long *addr, * @addr: The address to start the search at * @size: The maximum size to search * - * Returns the bit number of the first set bit. + * Returns the bit number of the first set bit, + * or @size if there is no set bit in the bitmap. */ static inline unsigned long find_first_bit(const unsigned long *addr, unsigned long size) @@ -194,7 +202,8 @@ static inline unsigned long find_first_bit(const unsigned long *addr, * @addr: The address to start the search at * @size: The maximum size to search * - * Returns the bit number of the first cleared bit. + * Returns the bit number of the first cleared bit, + * or @size if there is no clear bit in the bitmap. */ static inline unsigned long find_first_zero_bit(const unsigned long *addr, unsigned long size) From 5b33e015d38acb00340b8310a24c9998138afbe6 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Mon, 17 May 2021 12:05:43 +0200 Subject: [PATCH 3/8] coroutine-sleep: use a stack-allocated timer The lifetime of the timer is well-known (it cannot outlive qemu_co_sleep_ns_wakeable, because it's deleted by the time the coroutine resumes), so it is not necessary to place it on the heap. Reviewed-by: Vladimir Sementsov-Ogievskiy Signed-off-by: Paolo Bonzini Message-id: 20210517100548.28806-2-pbonzini@redhat.com Signed-off-by: Stefan Hajnoczi --- util/qemu-coroutine-sleep.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/util/qemu-coroutine-sleep.c b/util/qemu-coroutine-sleep.c index 8c4dac4fd75e..eec6e81f3f38 100644 --- a/util/qemu-coroutine-sleep.c +++ b/util/qemu-coroutine-sleep.c @@ -21,7 +21,7 @@ static const char *qemu_co_sleep_ns__scheduled = "qemu_co_sleep_ns"; struct QemuCoSleepState { Coroutine *co; - QEMUTimer *ts; + QEMUTimer ts; QemuCoSleepState **user_state_pointer; }; @@ -35,7 +35,7 @@ void qemu_co_sleep_wake(QemuCoSleepState *sleep_state) if (sleep_state->user_state_pointer) { *sleep_state->user_state_pointer = NULL; } - timer_del(sleep_state->ts); + timer_del(&sleep_state->ts); aio_co_wake(sleep_state->co); } @@ -50,7 +50,6 @@ void coroutine_fn qemu_co_sleep_ns_wakeable(QEMUClockType type, int64_t ns, AioContext *ctx = qemu_get_current_aio_context(); QemuCoSleepState state = { .co = qemu_coroutine_self(), - .ts = aio_timer_new(ctx, type, SCALE_NS, co_sleep_cb, &state), .user_state_pointer = sleep_state, }; @@ -63,10 +62,11 @@ void coroutine_fn qemu_co_sleep_ns_wakeable(QEMUClockType type, int64_t ns, abort(); } + aio_timer_init(ctx, &state.ts, type, SCALE_NS, co_sleep_cb, &state); if (sleep_state) { *sleep_state = &state; } - timer_mod(state.ts, qemu_clock_get_ns(type) + ns); + timer_mod(&state.ts, qemu_clock_get_ns(type) + ns); qemu_coroutine_yield(); if (sleep_state) { /* @@ -75,5 +75,4 @@ void coroutine_fn qemu_co_sleep_ns_wakeable(QEMUClockType type, int64_t ns, */ assert(*sleep_state == NULL); } - timer_free(state.ts); } From fb74a286feaa4ec2cdcda61ba570244464581ca7 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Mon, 17 May 2021 12:05:44 +0200 Subject: [PATCH 4/8] coroutine-sleep: disallow NULL QemuCoSleepState** argument Simplify the code by removing conditionals. qemu_co_sleep_ns can simply point the argument to an on-stack temporary. Reviewed-by: Vladimir Sementsov-Ogievskiy Signed-off-by: Paolo Bonzini Message-id: 20210517100548.28806-3-pbonzini@redhat.com Signed-off-by: Stefan Hajnoczi --- include/qemu/coroutine.h | 5 +++-- util/qemu-coroutine-sleep.c | 18 +++++------------- 2 files changed, 8 insertions(+), 15 deletions(-) diff --git a/include/qemu/coroutine.h b/include/qemu/coroutine.h index ce5b9c6851a3..c5d77429896d 100644 --- a/include/qemu/coroutine.h +++ b/include/qemu/coroutine.h @@ -295,7 +295,7 @@ typedef struct QemuCoSleepState QemuCoSleepState; /** * Yield the coroutine for a given duration. During this yield, @sleep_state - * (if not NULL) is set to an opaque pointer, which may be used for + * is set to an opaque pointer, which may be used for * qemu_co_sleep_wake(). Be careful, the pointer is set back to zero when the * timer fires. Don't save the obtained value to other variables and don't call * qemu_co_sleep_wake from another aio context. @@ -304,7 +304,8 @@ void coroutine_fn qemu_co_sleep_ns_wakeable(QEMUClockType type, int64_t ns, QemuCoSleepState **sleep_state); static inline void coroutine_fn qemu_co_sleep_ns(QEMUClockType type, int64_t ns) { - qemu_co_sleep_ns_wakeable(type, ns, NULL); + QemuCoSleepState *unused = NULL; + qemu_co_sleep_ns_wakeable(type, ns, &unused); } /** diff --git a/util/qemu-coroutine-sleep.c b/util/qemu-coroutine-sleep.c index eec6e81f3f38..3f6f637e8183 100644 --- a/util/qemu-coroutine-sleep.c +++ b/util/qemu-coroutine-sleep.c @@ -32,9 +32,7 @@ void qemu_co_sleep_wake(QemuCoSleepState *sleep_state) qemu_co_sleep_ns__scheduled, NULL); assert(scheduled == qemu_co_sleep_ns__scheduled); - if (sleep_state->user_state_pointer) { - *sleep_state->user_state_pointer = NULL; - } + *sleep_state->user_state_pointer = NULL; timer_del(&sleep_state->ts); aio_co_wake(sleep_state->co); } @@ -63,16 +61,10 @@ void coroutine_fn qemu_co_sleep_ns_wakeable(QEMUClockType type, int64_t ns, } aio_timer_init(ctx, &state.ts, type, SCALE_NS, co_sleep_cb, &state); - if (sleep_state) { - *sleep_state = &state; - } + *sleep_state = &state; timer_mod(&state.ts, qemu_clock_get_ns(type) + ns); qemu_coroutine_yield(); - if (sleep_state) { - /* - * Note that *sleep_state is cleared during qemu_co_sleep_wake - * before resuming this coroutine. - */ - assert(*sleep_state == NULL); - } + + /* qemu_co_sleep_wake clears *sleep_state before resuming this coroutine. */ + assert(*sleep_state == NULL); } From eaee0720851f22b273099eaf4c02722d9f24340a Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Mon, 17 May 2021 12:05:45 +0200 Subject: [PATCH 5/8] coroutine-sleep: allow qemu_co_sleep_wake that wakes nothing All callers of qemu_co_sleep_wake are checking whether they are passing a NULL argument inside the pointer-to-pointer: do the check in qemu_co_sleep_wake itself. As a side effect, qemu_co_sleep_wake can be called more than once and it will only wake the coroutine once; after the first time, the argument will be set to NULL via *sleep_state->user_state_pointer. However, this would not be safe unless co_sleep_cb keeps using the QemuCoSleepState* directly, so make it go through the pointer-to-pointer instead. Reviewed-by: Vladimir Sementsov-Ogievskiy Signed-off-by: Paolo Bonzini Message-id: 20210517100548.28806-4-pbonzini@redhat.com Signed-off-by: Stefan Hajnoczi --- block/block-copy.c | 4 +--- block/nbd.c | 8 ++------ util/qemu-coroutine-sleep.c | 21 ++++++++++++--------- 3 files changed, 15 insertions(+), 18 deletions(-) diff --git a/block/block-copy.c b/block/block-copy.c index 9b4af00614cc..f896dc56f274 100644 --- a/block/block-copy.c +++ b/block/block-copy.c @@ -674,9 +674,7 @@ block_copy_dirty_clusters(BlockCopyCallState *call_state) void block_copy_kick(BlockCopyCallState *call_state) { - if (call_state->sleep_state) { - qemu_co_sleep_wake(call_state->sleep_state); - } + qemu_co_sleep_wake(call_state->sleep_state); } /* diff --git a/block/nbd.c b/block/nbd.c index 1d4668d42d9c..1c6315b168f7 100644 --- a/block/nbd.c +++ b/block/nbd.c @@ -289,9 +289,7 @@ static void coroutine_fn nbd_client_co_drain_begin(BlockDriverState *bs) BDRVNBDState *s = (BDRVNBDState *)bs->opaque; s->drained = true; - if (s->connection_co_sleep_ns_state) { - qemu_co_sleep_wake(s->connection_co_sleep_ns_state); - } + qemu_co_sleep_wake(s->connection_co_sleep_ns_state); nbd_co_establish_connection_cancel(bs, false); @@ -330,9 +328,7 @@ static void nbd_teardown_connection(BlockDriverState *bs) s->state = NBD_CLIENT_QUIT; if (s->connection_co) { - if (s->connection_co_sleep_ns_state) { - qemu_co_sleep_wake(s->connection_co_sleep_ns_state); - } + qemu_co_sleep_wake(s->connection_co_sleep_ns_state); nbd_co_establish_connection_cancel(bs, true); } if (qemu_in_coroutine()) { diff --git a/util/qemu-coroutine-sleep.c b/util/qemu-coroutine-sleep.c index 3f6f637e8183..3ae2b5399aa5 100644 --- a/util/qemu-coroutine-sleep.c +++ b/util/qemu-coroutine-sleep.c @@ -27,19 +27,22 @@ struct QemuCoSleepState { void qemu_co_sleep_wake(QemuCoSleepState *sleep_state) { - /* Write of schedule protected by barrier write in aio_co_schedule */ - const char *scheduled = qatomic_cmpxchg(&sleep_state->co->scheduled, - qemu_co_sleep_ns__scheduled, NULL); + if (sleep_state) { + /* Write of schedule protected by barrier write in aio_co_schedule */ + const char *scheduled = qatomic_cmpxchg(&sleep_state->co->scheduled, + qemu_co_sleep_ns__scheduled, NULL); - assert(scheduled == qemu_co_sleep_ns__scheduled); - *sleep_state->user_state_pointer = NULL; - timer_del(&sleep_state->ts); - aio_co_wake(sleep_state->co); + assert(scheduled == qemu_co_sleep_ns__scheduled); + *sleep_state->user_state_pointer = NULL; + timer_del(&sleep_state->ts); + aio_co_wake(sleep_state->co); + } } static void co_sleep_cb(void *opaque) { - qemu_co_sleep_wake(opaque); + QemuCoSleepState **sleep_state = opaque; + qemu_co_sleep_wake(*sleep_state); } void coroutine_fn qemu_co_sleep_ns_wakeable(QEMUClockType type, int64_t ns, @@ -60,7 +63,7 @@ void coroutine_fn qemu_co_sleep_ns_wakeable(QEMUClockType type, int64_t ns, abort(); } - aio_timer_init(ctx, &state.ts, type, SCALE_NS, co_sleep_cb, &state); + aio_timer_init(ctx, &state.ts, type, SCALE_NS, co_sleep_cb, sleep_state); *sleep_state = &state; timer_mod(&state.ts, qemu_clock_get_ns(type) + ns); qemu_coroutine_yield(); From 1485f0c24cfbef7a542ce13d49e9f68e285c57d8 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Mon, 17 May 2021 12:05:46 +0200 Subject: [PATCH 6/8] coroutine-sleep: move timer out of QemuCoSleepState This simplification is enabled by the previous patch. Now aio_co_wake will only be called once, therefore we do not care about a spurious firing of the timer after a qemu_co_sleep_wake. Reviewed-by: Vladimir Sementsov-Ogievskiy Signed-off-by: Paolo Bonzini Message-id: 20210517100548.28806-5-pbonzini@redhat.com Signed-off-by: Stefan Hajnoczi --- util/qemu-coroutine-sleep.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/util/qemu-coroutine-sleep.c b/util/qemu-coroutine-sleep.c index 3ae2b5399aa5..1d2501962015 100644 --- a/util/qemu-coroutine-sleep.c +++ b/util/qemu-coroutine-sleep.c @@ -21,7 +21,6 @@ static const char *qemu_co_sleep_ns__scheduled = "qemu_co_sleep_ns"; struct QemuCoSleepState { Coroutine *co; - QEMUTimer ts; QemuCoSleepState **user_state_pointer; }; @@ -34,7 +33,6 @@ void qemu_co_sleep_wake(QemuCoSleepState *sleep_state) assert(scheduled == qemu_co_sleep_ns__scheduled); *sleep_state->user_state_pointer = NULL; - timer_del(&sleep_state->ts); aio_co_wake(sleep_state->co); } } @@ -49,6 +47,7 @@ void coroutine_fn qemu_co_sleep_ns_wakeable(QEMUClockType type, int64_t ns, QemuCoSleepState **sleep_state) { AioContext *ctx = qemu_get_current_aio_context(); + QEMUTimer ts; QemuCoSleepState state = { .co = qemu_coroutine_self(), .user_state_pointer = sleep_state, @@ -63,10 +62,11 @@ void coroutine_fn qemu_co_sleep_ns_wakeable(QEMUClockType type, int64_t ns, abort(); } - aio_timer_init(ctx, &state.ts, type, SCALE_NS, co_sleep_cb, sleep_state); + aio_timer_init(ctx, &ts, type, SCALE_NS, co_sleep_cb, sleep_state); *sleep_state = &state; - timer_mod(&state.ts, qemu_clock_get_ns(type) + ns); + timer_mod(&ts, qemu_clock_get_ns(type) + ns); qemu_coroutine_yield(); + timer_del(&ts); /* qemu_co_sleep_wake clears *sleep_state before resuming this coroutine. */ assert(*sleep_state == NULL); From 29a6ea24eb85d8400df1607a9e11d0ef9ec5e88d Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Mon, 17 May 2021 12:05:47 +0200 Subject: [PATCH 7/8] coroutine-sleep: replace QemuCoSleepState pointer with struct in the API Right now, users of qemu_co_sleep_ns_wakeable are simply passing a pointer to QemuCoSleepState by reference to the function. But QemuCoSleepState really is just a Coroutine*; making the content of the struct public is just as efficient and lets us skip the user_state_pointer indirection. Since the usage is changed, take the occasion to rename the struct to QemuCoSleep. Reviewed-by: Vladimir Sementsov-Ogievskiy Signed-off-by: Paolo Bonzini Message-id: 20210517100548.28806-6-pbonzini@redhat.com Signed-off-by: Stefan Hajnoczi --- block/block-copy.c | 8 ++++---- block/nbd.c | 10 ++++----- include/qemu/coroutine.h | 23 +++++++++++---------- util/qemu-coroutine-sleep.c | 41 ++++++++++++++++--------------------- 4 files changed, 39 insertions(+), 43 deletions(-) diff --git a/block/block-copy.c b/block/block-copy.c index f896dc56f274..c2e509041232 100644 --- a/block/block-copy.c +++ b/block/block-copy.c @@ -50,7 +50,7 @@ typedef struct BlockCopyCallState { /* State */ int ret; bool finished; - QemuCoSleepState *sleep_state; + QemuCoSleep sleep; bool cancelled; /* OUT parameters */ @@ -625,8 +625,8 @@ block_copy_dirty_clusters(BlockCopyCallState *call_state) if (ns > 0) { block_copy_task_end(task, -EAGAIN); g_free(task); - qemu_co_sleep_ns_wakeable(QEMU_CLOCK_REALTIME, ns, - &call_state->sleep_state); + qemu_co_sleep_ns_wakeable(&call_state->sleep, + QEMU_CLOCK_REALTIME, ns); continue; } } @@ -674,7 +674,7 @@ block_copy_dirty_clusters(BlockCopyCallState *call_state) void block_copy_kick(BlockCopyCallState *call_state) { - qemu_co_sleep_wake(call_state->sleep_state); + qemu_co_sleep_wake(&call_state->sleep); } /* diff --git a/block/nbd.c b/block/nbd.c index 1c6315b168f7..616f9ae6c4da 100644 --- a/block/nbd.c +++ b/block/nbd.c @@ -116,7 +116,7 @@ typedef struct BDRVNBDState { CoQueue free_sema; Coroutine *connection_co; Coroutine *teardown_co; - QemuCoSleepState *connection_co_sleep_ns_state; + QemuCoSleep reconnect_sleep; bool drained; bool wait_drained_end; int in_flight; @@ -289,7 +289,7 @@ static void coroutine_fn nbd_client_co_drain_begin(BlockDriverState *bs) BDRVNBDState *s = (BDRVNBDState *)bs->opaque; s->drained = true; - qemu_co_sleep_wake(s->connection_co_sleep_ns_state); + qemu_co_sleep_wake(&s->reconnect_sleep); nbd_co_establish_connection_cancel(bs, false); @@ -328,7 +328,7 @@ static void nbd_teardown_connection(BlockDriverState *bs) s->state = NBD_CLIENT_QUIT; if (s->connection_co) { - qemu_co_sleep_wake(s->connection_co_sleep_ns_state); + qemu_co_sleep_wake(&s->reconnect_sleep); nbd_co_establish_connection_cancel(bs, true); } if (qemu_in_coroutine()) { @@ -685,8 +685,8 @@ static coroutine_fn void nbd_co_reconnect_loop(BDRVNBDState *s) } bdrv_inc_in_flight(s->bs); } else { - qemu_co_sleep_ns_wakeable(QEMU_CLOCK_REALTIME, timeout, - &s->connection_co_sleep_ns_state); + qemu_co_sleep_ns_wakeable(&s->reconnect_sleep, + QEMU_CLOCK_REALTIME, timeout); if (s->drained) { continue; } diff --git a/include/qemu/coroutine.h b/include/qemu/coroutine.h index c5d77429896d..82c0671f80a7 100644 --- a/include/qemu/coroutine.h +++ b/include/qemu/coroutine.h @@ -291,21 +291,22 @@ void qemu_co_rwlock_wrlock(CoRwlock *lock); */ void qemu_co_rwlock_unlock(CoRwlock *lock); -typedef struct QemuCoSleepState QemuCoSleepState; +typedef struct QemuCoSleep { + Coroutine *to_wake; +} QemuCoSleep; /** - * Yield the coroutine for a given duration. During this yield, @sleep_state - * is set to an opaque pointer, which may be used for - * qemu_co_sleep_wake(). Be careful, the pointer is set back to zero when the - * timer fires. Don't save the obtained value to other variables and don't call - * qemu_co_sleep_wake from another aio context. + * Yield the coroutine for a given duration. Initializes @w so that, + * during this yield, it can be passed to qemu_co_sleep_wake() to + * terminate the sleep. */ -void coroutine_fn qemu_co_sleep_ns_wakeable(QEMUClockType type, int64_t ns, - QemuCoSleepState **sleep_state); +void coroutine_fn qemu_co_sleep_ns_wakeable(QemuCoSleep *w, + QEMUClockType type, int64_t ns); + static inline void coroutine_fn qemu_co_sleep_ns(QEMUClockType type, int64_t ns) { - QemuCoSleepState *unused = NULL; - qemu_co_sleep_ns_wakeable(type, ns, &unused); + QemuCoSleep w = { 0 }; + qemu_co_sleep_ns_wakeable(&w, type, ns); } /** @@ -314,7 +315,7 @@ static inline void coroutine_fn qemu_co_sleep_ns(QEMUClockType type, int64_t ns) * qemu_co_sleep_ns() and should be checked to be non-NULL before calling * qemu_co_sleep_wake(). */ -void qemu_co_sleep_wake(QemuCoSleepState *sleep_state); +void qemu_co_sleep_wake(QemuCoSleep *w); /** * Yield until a file descriptor becomes readable diff --git a/util/qemu-coroutine-sleep.c b/util/qemu-coroutine-sleep.c index 1d2501962015..89c3b758c55f 100644 --- a/util/qemu-coroutine-sleep.c +++ b/util/qemu-coroutine-sleep.c @@ -19,42 +19,37 @@ static const char *qemu_co_sleep_ns__scheduled = "qemu_co_sleep_ns"; -struct QemuCoSleepState { +void qemu_co_sleep_wake(QemuCoSleep *w) +{ Coroutine *co; - QemuCoSleepState **user_state_pointer; -}; -void qemu_co_sleep_wake(QemuCoSleepState *sleep_state) -{ - if (sleep_state) { + co = w->to_wake; + w->to_wake = NULL; + if (co) { /* Write of schedule protected by barrier write in aio_co_schedule */ - const char *scheduled = qatomic_cmpxchg(&sleep_state->co->scheduled, + const char *scheduled = qatomic_cmpxchg(&co->scheduled, qemu_co_sleep_ns__scheduled, NULL); assert(scheduled == qemu_co_sleep_ns__scheduled); - *sleep_state->user_state_pointer = NULL; - aio_co_wake(sleep_state->co); + aio_co_wake(co); } } static void co_sleep_cb(void *opaque) { - QemuCoSleepState **sleep_state = opaque; - qemu_co_sleep_wake(*sleep_state); + QemuCoSleep *w = opaque; + qemu_co_sleep_wake(w); } -void coroutine_fn qemu_co_sleep_ns_wakeable(QEMUClockType type, int64_t ns, - QemuCoSleepState **sleep_state) +void coroutine_fn qemu_co_sleep_ns_wakeable(QemuCoSleep *w, + QEMUClockType type, int64_t ns) { + Coroutine *co = qemu_coroutine_self(); AioContext *ctx = qemu_get_current_aio_context(); QEMUTimer ts; - QemuCoSleepState state = { - .co = qemu_coroutine_self(), - .user_state_pointer = sleep_state, - }; - const char *scheduled = qatomic_cmpxchg(&state.co->scheduled, NULL, - qemu_co_sleep_ns__scheduled); + const char *scheduled = qatomic_cmpxchg(&co->scheduled, NULL, + qemu_co_sleep_ns__scheduled); if (scheduled) { fprintf(stderr, "%s: Co-routine was already scheduled in '%s'\n", @@ -62,12 +57,12 @@ void coroutine_fn qemu_co_sleep_ns_wakeable(QEMUClockType type, int64_t ns, abort(); } - aio_timer_init(ctx, &ts, type, SCALE_NS, co_sleep_cb, sleep_state); - *sleep_state = &state; + w->to_wake = co; + aio_timer_init(ctx, &ts, type, SCALE_NS, co_sleep_cb, w), timer_mod(&ts, qemu_clock_get_ns(type) + ns); qemu_coroutine_yield(); timer_del(&ts); - /* qemu_co_sleep_wake clears *sleep_state before resuming this coroutine. */ - assert(*sleep_state == NULL); + /* w->to_wake is cleared before resuming this coroutine. */ + assert(w->to_wake == NULL); } From 0a6f0c76a030710780ce10d6347a70f098024d21 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Mon, 17 May 2021 12:05:48 +0200 Subject: [PATCH 8/8] coroutine-sleep: introduce qemu_co_sleep Allow using QemuCoSleep to sleep forever until woken by qemu_co_sleep_wake. This makes the logic of qemu_co_sleep_ns_wakeable easy to understand. In the future we will introduce an API that can work even if the sleep and wake happen from different threads. For now, initializing w->to_wake after timer_mod is fine because the timer can only fire in the same AioContext. Reviewed-by: Vladimir Sementsov-Ogievskiy Signed-off-by: Paolo Bonzini Message-id: 20210517100548.28806-7-pbonzini@redhat.com Signed-off-by: Stefan Hajnoczi --- include/qemu/coroutine.h | 5 +++++ util/qemu-coroutine-sleep.c | 26 +++++++++++++++++++------- 2 files changed, 24 insertions(+), 7 deletions(-) diff --git a/include/qemu/coroutine.h b/include/qemu/coroutine.h index 82c0671f80a7..292e61aef0c2 100644 --- a/include/qemu/coroutine.h +++ b/include/qemu/coroutine.h @@ -303,6 +303,11 @@ typedef struct QemuCoSleep { void coroutine_fn qemu_co_sleep_ns_wakeable(QemuCoSleep *w, QEMUClockType type, int64_t ns); +/** + * Yield the coroutine until the next call to qemu_co_sleep_wake. + */ +void coroutine_fn qemu_co_sleep(QemuCoSleep *w); + static inline void coroutine_fn qemu_co_sleep_ns(QEMUClockType type, int64_t ns) { QemuCoSleep w = { 0 }; diff --git a/util/qemu-coroutine-sleep.c b/util/qemu-coroutine-sleep.c index 89c3b758c55f..571ab521ff0b 100644 --- a/util/qemu-coroutine-sleep.c +++ b/util/qemu-coroutine-sleep.c @@ -41,12 +41,9 @@ static void co_sleep_cb(void *opaque) qemu_co_sleep_wake(w); } -void coroutine_fn qemu_co_sleep_ns_wakeable(QemuCoSleep *w, - QEMUClockType type, int64_t ns) +void coroutine_fn qemu_co_sleep(QemuCoSleep *w) { Coroutine *co = qemu_coroutine_self(); - AioContext *ctx = qemu_get_current_aio_context(); - QEMUTimer ts; const char *scheduled = qatomic_cmpxchg(&co->scheduled, NULL, qemu_co_sleep_ns__scheduled); @@ -58,11 +55,26 @@ void coroutine_fn qemu_co_sleep_ns_wakeable(QemuCoSleep *w, } w->to_wake = co; - aio_timer_init(ctx, &ts, type, SCALE_NS, co_sleep_cb, w), - timer_mod(&ts, qemu_clock_get_ns(type) + ns); qemu_coroutine_yield(); - timer_del(&ts); /* w->to_wake is cleared before resuming this coroutine. */ assert(w->to_wake == NULL); } + +void coroutine_fn qemu_co_sleep_ns_wakeable(QemuCoSleep *w, + QEMUClockType type, int64_t ns) +{ + AioContext *ctx = qemu_get_current_aio_context(); + QEMUTimer ts; + + aio_timer_init(ctx, &ts, type, SCALE_NS, co_sleep_cb, w); + timer_mod(&ts, qemu_clock_get_ns(type) + ns); + + /* + * The timer will fire in the current AiOContext, so the callback + * must happen after qemu_co_sleep yields and there is no race + * between timer_mod and qemu_co_sleep. + */ + qemu_co_sleep(w); + timer_del(&ts); +}