Re: [PATCH v1] coroutine: avoid inserting duplicate coroutine to co_queue_wakeup

From: zhuyangyang via <qemu-devel@nongnu.org>
To: <eblake@redhat.com>
Cc: <chenxiaoyu48@huawei.com>, <fam@euphon.net>,
	<luolongmin@huawei.com>, <qemu-block@nongnu.org>,
	<qemu-devel@nongnu.org>, <qemu-stable@nongnu.org>,
	<stefanha@redhat.com>, <suxiaodong1@huawei.com>,
	<wangyan122@huawei.com>, <yebiaoxiang@huawei.com>,
	<zhuyangyang14@huawei.com>
Subject: Re: [PATCH v1] coroutine: avoid inserting duplicate coroutine to co_queue_wakeup
Date: Tue, 26 Mar 2024 20:25:59 +0800	[thread overview]
Message-ID: <20240326122559.2777462-1-zhuyangyang14@huawei.com> (raw)
In-Reply-To: <wac6lbzuvpdyttfpuoeac4ygps2elzktf5dcw22wruaupqy5r6@sfausqavuvil>

On Mon, 25 Mar 2024 11:00:31 -0500 Eric Blake wrote:
> >  util/async.c | 13 ++++++++++++-
> >  1 file changed, 12 insertions(+), 1 deletion(-)
> > 
> > diff --git a/util/async.c b/util/async.c
> > index 0467890052..25fc1e6083 100644
> > --- a/util/async.c
> > +++ b/util/async.c
> > @@ -705,7 +705,18 @@ void aio_co_enter(AioContext *ctx, Coroutine *co)
> >      if (qemu_in_coroutine()) {
> >          Coroutine *self = qemu_coroutine_self();
> >          assert(self != co);
> > -        QSIMPLEQ_INSERT_TAIL(&self->co_queue_wakeup, co, co_queue_next);
> > +        /*
> > +         * If the Coroutine *co is already in the co_queue_wakeup, this
> > +         * repeated insertion will causes the loss of other queue element
> 
> s/causes/cause/
> 
> > +         * or infinite loop.
> > +         * For examplex:
> 
> s/examplex/example/
> 
> > +         * Head->a->b->c->NULL, after insert_tail(head, b) => Head->a->b->NULL
> > +         * Head->a-b>->NULL, after insert_tail(head, b) => Head->a->b->b...
> 
> s/b>->/b->/
> 
> > +         */
> > +        if (!co->co_queue_next.sqe_next &&
> > +            self->co_queue_wakeup.sqh_last != &co->co_queue_next.sqe_next) {
> > +            QSIMPLEQ_INSERT_TAIL(&self->co_queue_wakeup, co, co_queue_next);
> > +        }
> >      } else {
> >          qemu_aio_coroutine_enter(ctx, co);
> >      }
> 
> Intuitively, attacking the symptoms (avoiding bogus list insertion
> when it is already on the list) makes sense; but I want to make sure
> we attack the root cause.

Repairing the nbd_negotiate_handle_starttls() can solve this problem, therefore,
I'm not sure if this commit is still needed.

--
Best Regards,
Zhu Yangyang