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

[Vladimir Sementsov-Ogievskiy]

10.05.2021 11:59, 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.

But it doesn't. It's called only from co_get_from_shres(). So, better make it a 
static function first.

> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>   util/qemu-co-shared-resource.c | 26 ++++++++++++++++++++++----
>   1 file changed, 22 insertions(+), 4 deletions(-)
>
> diff --git a/util/qemu-co-shared-resource.c b/util/qemu-co-shared-resource.c
> index 1c83cd9d29..c455d02a1e 100644
> --- a/util/qemu-co-shared-resource.c
> +++ b/util/qemu-co-shared-resource.c
> @@ -32,6 +32,7 @@ struct SharedResource {
>       uint64_t available;
>   
>       CoQueue queue;
> +    QemuMutex lock;
>   };
>   
>   SharedResource *shres_create(uint64_t total)
> @@ -40,17 +41,23 @@ SharedResource *shres_create(uint64_t total)
>   
>       s->total = s->available = total;
>       qemu_co_queue_init(&s->queue);
> +    qemu_mutex_init(&s->lock);
>   
>       return s;
>   }
>   
>   void shres_destroy(SharedResource *s)
>   {
> -    assert(s->available == s->total);
> +    WITH_QEMU_LOCK_GUARD(&s->lock) {
> +        assert(s->available == s->total);
> +    }

shres_destroy can't be thread-safe anyway, as it does qemu_mutex_destroy() and g_free. 
So, let's don't try to make part of shres_destroy() "thread safe".

> +
> +    qemu_mutex_destroy(&s->lock);
>       g_free(s);
>   }
>   
> -bool co_try_get_from_shres(SharedResource *s, uint64_t n)
> +/* Called with lock held */

it should be called _locked() than.


> +static bool co_try_get_from_shres_unlocked(SharedResource *s, uint64_t n)>   {
>       if (s->available >= n) {
>           s->available -= n;
> @@ -60,16 +67,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_unlocked(s, n);
> +
> +    return res;
> +}
> +
>   void coroutine_fn co_get_from_shres(SharedResource *s, uint64_t n)
>   {
> +    QEMU_LOCK_GUARD(&s->lock);
>       assert(n <= s->total);
> -    while (!co_try_get_from_shres(s, n)) {
> -        qemu_co_queue_wait(&s->queue, NULL);
> +    while (!co_try_get_from_shres_unlocked(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);


--
Best regards,
Vladimir