Re: [Qemu-devel] [PATCH] qemu-coroutine-lock: fix co_queue multi-adding bug

[Fam Zheng]

On Sat, 02/07 17:51, w00214312 wrote:
> From: Bin Wu <address@hidden>
> 
> When a coroutine holds a lock, other coroutines who want to get
> the lock must wait on a co_queue by adding themselves to the
> CoQueue. However, if a waiting coroutine is woken up with the
> lock still be holding by other coroutine, this waiting coroutine

Could you explain who wakes up the waiting coroutine? Maybe the bug is that
it shouldn't be awaken in the first place.

> will add itself to the co_queue again. Latter, when the lock
> is released, a coroutine re-enter will occur.
> 
> We need to determine whether a coroutine is alread in the co_queue

s/alread/already/

Fam

> before adding it to the waiting queue.
> 
> Signed-off-by: Bin Wu <address@hidden>
> ---
>  include/block/coroutine_int.h | 1 +
>  qemu-coroutine-lock.c         | 6 +++++-
>  qemu-coroutine.c              | 1 +
>  3 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/include/block/coroutine_int.h b/include/block/coroutine_int.h
> index f133d65..c524990 100644
> --- a/include/block/coroutine_int.h
> +++ b/include/block/coroutine_int.h
> @@ -42,6 +42,7 @@ struct Coroutine {
>      /* Coroutines that should be woken up when we yield or terminate */
>      QTAILQ_HEAD(, Coroutine) co_queue_wakeup;
>      QTAILQ_ENTRY(Coroutine) co_queue_next;
> +    bool in_co_queue;
>  };
>  
>  Coroutine *qemu_coroutine_new(void);
> diff --git a/qemu-coroutine-lock.c b/qemu-coroutine-lock.c
> index e4860ae..d256f53 100644
> --- a/qemu-coroutine-lock.c
> +++ b/qemu-coroutine-lock.c
> @@ -36,7 +36,10 @@ void qemu_co_queue_init(CoQueue *queue)
>  void coroutine_fn qemu_co_queue_wait(CoQueue *queue)
>  {
>      Coroutine *self = qemu_coroutine_self();
> -    QTAILQ_INSERT_TAIL(&queue->entries, self, co_queue_next);
> +    if (!self->in_co_queue) {
> +        QTAILQ_INSERT_TAIL(&queue->entries, self, co_queue_next);
> +        self->in_co_queue = true;
> +    }
>      qemu_coroutine_yield();
>      assert(qemu_in_coroutine());
>  }
> @@ -71,6 +74,7 @@ static bool qemu_co_queue_do_restart(CoQueue *queue, bool 
> single)
>  
>      while ((next = QTAILQ_FIRST(&queue->entries)) != NULL) {
>          QTAILQ_REMOVE(&queue->entries, next, co_queue_next);
> +        next->in_co_queue = false;
>          QTAILQ_INSERT_TAIL(&self->co_queue_wakeup, next, co_queue_next);
>          trace_qemu_co_queue_next(next);
>          if (single) {
> diff --git a/qemu-coroutine.c b/qemu-coroutine.c
> index 525247b..a103721 100644
> --- a/qemu-coroutine.c
> +++ b/qemu-coroutine.c
> @@ -75,6 +75,7 @@ Coroutine *qemu_coroutine_create(CoroutineEntry *entry)
>      }
>  
>      co->entry = entry;
> +    co->in_co_queue = false;
>      QTAILQ_INIT(&co->co_queue_wakeup);
>      return co;
>  }
> -- 
> 1.7.12.4
> 
>