Re: [PATCH 5/6] coroutine-sleep: replace QemuCoSleepState pointer with struct in the API

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

03.05.2021 14:25, Paolo Bonzini wrote:
> Right now, users of qemu_co_sleep_ns_wakeable are 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: the Coroutine* is
> cleared directly, rather than the pointer to it.
> 
> Since the usage is changed, take the occasion to rename the
> struct to QemuCoSleep.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   block/block-copy.c          |  8 +++----
>   block/nbd.c                 | 10 ++++-----
>   include/qemu/coroutine.h    | 22 +++++++++----------
>   util/qemu-coroutine-sleep.c | 43 ++++++++++++++++---------------------
>   4 files changed, 39 insertions(+), 44 deletions(-)
> 
> diff --git a/block/block-copy.c b/block/block-copy.c
> index f896dc56f2..c2e5090412 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 @@ out:
>   
>   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 1c6315b168..616f9ae6c4 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 c5d7742989..77cb8ce459 100644
> --- a/include/qemu/coroutine.h
> +++ b/include/qemu/coroutine.h
> @@ -291,21 +291,21 @@ 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. During this yield, @w
> + * can be used with qemu_co_sleep_wake() to terminate the sleep.

I'd add that function initializes @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);
> +
>   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 +314,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 68e9505b2e..89c3b758c5 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;

Don't you like g_steal_pointer()?

> +    if (co) {
>           /* 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);
> +        const char *scheduled = qatomic_cmpxchg(&co->scheduled,
> +                                                qemu_co_sleep_ns__scheduled, NULL);

indentation fixed, but not over-80 line )

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

indentation fixed..

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

with indentations and over-80 line fixed in appropriate previous patch:

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir