| Message ID | 20220426085114.199647-3-eesposit@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Removal of AioContext lock, bs->parents and ->children: new rwlock | expand |
On 4/26/22 10:51, Emanuele Giuseppe Esposito wrote: > +#define qemu_co_queue_restart_all_lockable(queue, lock) \ > + qemu_co_queue_restart_all_impl(queue, QEMU_MAKE_LOCKABLE(lock)) I think this function should be named qemu_co_queue_enter_all, for consistency with qemu_co_queue_enter_next. Paolo
On Tue, Apr 26, 2022 at 04:51:08AM -0400, Emanuele Giuseppe Esposito wrote: > Current implementation of qemu_co_queue_do_restart > does not releases the lock before calling aio_co_wake. > Most of the time this is fine, but if the coroutine > acquires the lock again then we have a deadlock. > > Instead of duplicating code, use qemu_co_enter_next_impl, since > it implements exactly the same functionality that we > want. > > Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> > --- It's unclear whether this patch fixes a bug or introduces a new API that will be used in later patches. The commit message is a bit misleading: existing functions are not changed to release the lock when restarting all coroutines. I think what this commit is actually doing is adding a new API that will be used in later patches?
On 4/28/22 13:21, Stefan Hajnoczi wrote: > It's unclear whether this patch fixes a bug or introduces a new API that > will be used in later patches. > > The commit message is a bit misleading: existing functions are not > changed to release the lock when restarting all coroutines. > > I think what this commit is actually doing is adding a new API that will > be used in later patches? I think this patch overlaps and can be replaced by these (https://lore.kernel.org/qemu-devel/20220427130830.150180-1-pbonzini@redhat.com/T/#t). I wrote them after a discussion with Emanuele, and it looks like the same discussion begot this patch on his side. Paolo
Am 29/04/2022 um 00:14 schrieb Paolo Bonzini: > On 4/28/22 13:21, Stefan Hajnoczi wrote: >> It's unclear whether this patch fixes a bug or introduces a new API that >> will be used in later patches. >> >> The commit message is a bit misleading: existing functions are not >> changed to release the lock when restarting all coroutines. >> >> I think what this commit is actually doing is adding a new API that will >> be used in later patches? > > I think this patch overlaps and can be replaced by these > (https://lore.kernel.org/qemu-devel/20220427130830.150180-1-pbonzini@redhat.com/T/#t). > I wrote them after a discussion with Emanuele, and it looks like the > same discussion begot this patch on his side. > > Paolo > Makes sense, I will rebase on top of your serie. Emanuele
diff --git a/include/qemu/coroutine.h b/include/qemu/coroutine.h index c828a95ee0..c49cdc21b4 100644 --- a/include/qemu/coroutine.h +++ b/include/qemu/coroutine.h @@ -220,6 +220,16 @@ bool qemu_co_queue_next(CoQueue *queue); */ void qemu_co_queue_restart_all(CoQueue *queue); +/** + * Empties the CoQueue; all coroutines are woken up. + * OK to run from coroutine and non-coroutine context. + * Unlocks lock before waking up each coroutine takes it again + * when done. + */ +#define qemu_co_queue_restart_all_lockable(queue, lock) \ + qemu_co_queue_restart_all_impl(queue, QEMU_MAKE_LOCKABLE(lock)) +void qemu_co_queue_restart_all_impl(CoQueue *queue, QemuLockable *lock); + /** * Removes the next coroutine from the CoQueue, and wake it up. Unlike * qemu_co_queue_next, this function releases the lock during aio_co_wake diff --git a/util/qemu-coroutine-lock.c b/util/qemu-coroutine-lock.c index 2669403839..17bb0d0c95 100644 --- a/util/qemu-coroutine-lock.c +++ b/util/qemu-coroutine-lock.c @@ -67,32 +67,26 @@ void coroutine_fn qemu_co_queue_wait_impl(CoQueue *queue, QemuLockable *lock) } } -static bool qemu_co_queue_do_restart(CoQueue *queue, bool single) +static void qemu_co_queue_do_restart(CoQueue *queue, QemuLockable *lock) { - Coroutine *next; - - if (QSIMPLEQ_EMPTY(&queue->entries)) { - return false; + while (qemu_co_enter_next_impl(queue, lock)) { + /* nop */ } - - while ((next = QSIMPLEQ_FIRST(&queue->entries)) != NULL) { - QSIMPLEQ_REMOVE_HEAD(&queue->entries, co_queue_next); - aio_co_wake(next); - if (single) { - break; - } - } - return true; } bool qemu_co_queue_next(CoQueue *queue) { - return qemu_co_queue_do_restart(queue, true); + return qemu_co_enter_next_impl(queue, NULL); } void qemu_co_queue_restart_all(CoQueue *queue) { - qemu_co_queue_do_restart(queue, false); + qemu_co_queue_do_restart(queue, NULL); +} + +void qemu_co_queue_restart_all_impl(CoQueue *queue, QemuLockable *lock) +{ + qemu_co_queue_do_restart(queue, lock); } bool qemu_co_enter_next_impl(CoQueue *queue, QemuLockable *lock)
Current implementation of qemu_co_queue_do_restart does not releases the lock before calling aio_co_wake. Most of the time this is fine, but if the coroutine acquires the lock again then we have a deadlock. Instead of duplicating code, use qemu_co_enter_next_impl, since it implements exactly the same functionality that we want. Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> --- include/qemu/coroutine.h | 10 ++++++++++ util/qemu-coroutine-lock.c | 26 ++++++++++---------------- 2 files changed, 20 insertions(+), 16 deletions(-)