Re: [PATCH v2 5/5] co-shared-resource: protect with a mutex

[Vladimir Sementsov-Ogievskiy]

18.05.2021 12:40, Emanuele Giuseppe Esposito wrote:
> co-shared-resource is currently not thread-safe, as also reported
> in co-shared-resource.h. Add a QemuMutex because co_try_get_from_shres
> can also be invoked from non-coroutine context.
>
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>   include/qemu/co-shared-resource.h |  4 +---
>   util/qemu-co-shared-resource.c    | 27 ++++++++++++++++++++++-----
>   2 files changed, 23 insertions(+), 8 deletions(-)
>
> diff --git a/include/qemu/co-shared-resource.h 
> b/include/qemu/co-shared-resource.h
> index 4e4503004c..78ca5850f8 100644
> --- a/include/qemu/co-shared-resource.h
> +++ b/include/qemu/co-shared-resource.h
> @@ -26,15 +26,13 @@
>   #ifndef QEMU_CO_SHARED_RESOURCE_H
>   #define QEMU_CO_SHARED_RESOURCE_H
>   
> -
> +/* Accesses to co-shared-resource API are thread-safe */
>   typedef struct SharedResource SharedResource;
>   
>   /*
>    * Create SharedResource structure
>    *
>    * @total: total amount of some resource to be shared between clients
> - *
> - * Note: this API is not thread-safe.
>    */
>   SharedResource *shres_create(uint64_t total);
>   
> diff --git a/util/qemu-co-shared-resource.c b/util/qemu-co-shared-resource.c
> index 1c83cd9d29..bb875a86be 100644
> --- a/util/qemu-co-shared-resource.c
> +++ b/util/qemu-co-shared-resource.c
> @@ -28,10 +28,13 @@
>   #include "qemu/co-shared-resource.h"
>   
>   struct SharedResource {
> -    uint64_t total;
> -    uint64_t available;
> +    uint64_t total; /* Set in shres_create() and not changed anymore */
>   
> +    /* State fields protected by lock */
> +    uint64_t available;
>       CoQueue queue;
> +
> +    QemuMutex lock;
>   };
>   
>   SharedResource *shres_create(uint64_t total)
> @@ -40,6 +43,7 @@ SharedResource *shres_create(uint64_t total)
>   
>       s->total = s->available = total;
>       qemu_co_queue_init(&s->queue);
> +    qemu_mutex_init(&s->lock);
>   
>       return s;
>   }
> @@ -47,10 +51,12 @@ SharedResource *shres_create(uint64_t total)
>   void shres_destroy(SharedResource *s)
>   {
>       assert(s->available == s->total);
> +    qemu_mutex_destroy(&s->lock);
>       g_free(s);
>   }
>   
> -bool co_try_get_from_shres(SharedResource *s, uint64_t n)
> +/* Called with lock held. */
> +static bool co_try_get_from_shres_locked(SharedResource *s, uint64_t n)
>   {
>       if (s->available >= n) {
>           s->available -= n;
> @@ -60,16 +66,27 @@ bool co_try_get_from_shres(SharedResource *s, uint64_t n)
>       return false;
>   }
>   
> +bool co_try_get_from_shres(SharedResource *s, uint64_t n)
> +{
> +    bool res;
> +    QEMU_LOCK_GUARD(&s->lock);
> +    res = co_try_get_from_shres_locked(s, n);
> +
> +    return res;
> +}

it can be two lines:

QEMU_LOCK_GUARD(&s->lock);
return co_try_get_from_shres_locked(s, n);

> +
>   void coroutine_fn co_get_from_shres(SharedResource *s, uint64_t n)
>   {
>       assert(n <= s->total);
> -    while (!co_try_get_from_shres(s, n)) {
> -        qemu_co_queue_wait(&s->queue, NULL);
> +    QEMU_LOCK_GUARD(&s->lock);
> +    while (!co_try_get_from_shres_locked(s, n)) {
> +        qemu_co_queue_wait(&s->queue, &s->lock);
>       }
>   }
>   
>   void coroutine_fn co_put_to_shres(SharedResource *s, uint64_t n)
>   {
> +    QEMU_LOCK_GUARD(&s->lock);
>       assert(s->total - s->available >= n);
>       s->available += n;
>       qemu_co_queue_restart_all(&s->queue);

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


--
Best regards,
Vladimir