Re: [PATCH 3/6] coroutine-sleep: allow qemu_co_sleep_wake that wakes nothing

[Vladimir Sementsov-Ogievskiy]

03.05.2021 14:25, Paolo Bonzini wrote:
> 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.

commit message say nothing about change in co_sleep_cb: it now passes the user 
pointer too.

> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   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 9b4af00614..f896dc56f2 100644
> --- a/block/block-copy.c
> +++ b/block/block-copy.c
> @@ -674,9 +674,7 @@ out:
>   
>   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 1d4668d42d..1c6315b168 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 3f6f637e81..096eea3444 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);

over-80 line (and indentation is broken)

>   
> -    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();

Patch looks OK itself:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

(still, please improve commit message).

--
Best regards,
Vladimir