Re: [Qemu-devel] [PATCH v2 1/1] coroutine-lock: do not touch coroutine after another one has been entered

[Stefan Hajnoczi]

On Wed, May 31, 2017 at 03:23:25PM +0200, Roman Penyaev wrote:
> On Wed, May 31, 2017 at 3:06 PM, Stefan Hajnoczi <address@hidden> wrote:
> > On Tue, May 30, 2017 at 12:07:36PM +0200, Roman Pen wrote:
> >> diff --git a/util/qemu-coroutine-lock.c b/util/qemu-coroutine-lock.c
> >> index 6328eed26bc6..d589d8c66d5e 100644
> >> --- a/util/qemu-coroutine-lock.c
> >> +++ b/util/qemu-coroutine-lock.c
> >> @@ -77,10 +77,20 @@ void coroutine_fn qemu_co_queue_wait(CoQueue *queue, 
> >> CoMutex *mutex)
> >>  void qemu_co_queue_run_restart(Coroutine *co)
> >>  {
> >>      Coroutine *next;
> >> +    QSIMPLEQ_HEAD(, Coroutine) tmp_queue_wakeup =
> >> +        QSIMPLEQ_HEAD_INITIALIZER(tmp_queue_wakeup);
> >>
> >>      trace_qemu_co_queue_run_restart(co);
> >> -    while ((next = QSIMPLEQ_FIRST(&co->co_queue_wakeup))) {
> >> -        QSIMPLEQ_REMOVE_HEAD(&co->co_queue_wakeup, co_queue_next);
> >> +
> >> +    /* Because "co" has yielded, any coroutine that we wakeup can resume 
> >> it.
> >> +     * If this happens and "co" terminates, co->co_queue_wakeup becomes
> >> +     * invalid memory.  Therefore, use a temporary queue and do not touch
> >> +     * the "co" coroutine as soon as you enter another one.
> >> +     */
> >> +    QSIMPLEQ_CONCAT(&tmp_queue_wakeup, &co->co_queue_wakeup);
> >> +
> >> +    while ((next = QSIMPLEQ_FIRST(&tmp_queue_wakeup))) {
> >> +        QSIMPLEQ_REMOVE_HEAD(&tmp_queue_wakeup, co_queue_next);
> >>          qemu_coroutine_enter(next);
> >>      }
> >>  }
> >
> > What happens if co remains alive and qemu_coroutine_enter(next) causes
> > additional coroutines to add themselves to co->co_queue_wakeup?
> 
> Yeah, I thought about it.  But according to my understanding the only
> path where you add something to the tail of a queue is:
> 
> void aio_co_enter(AioContext *ctx, struct 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); <<<< HERE
> 
> So you should be in *that* coroutine to chain other coroutines.
> That means that caller of your 'co' will be responsible to complete
> what it has in the list.  Something like that:
> 
> 
> co1 YIELDED,
> foreach co in co1.queue{co2}
>    enter(co) -------------->  co2 does something and
>                               eventually enter(co1):  -----> co1 does
> something and
>                                                              add co4
> to the queue
>                                                              terminates
>                                                       <-----
>                               co2 iterates over the queue of co1 and
>                                foreach co in co1.queue{co4}
> 
> 
> Sorry, the explanation is totally crap, but the key is:
> caller is responsible for cleaning the queue no matter what
> happens.  Sounds sane?

Yes, that makes sense.  A comment in the code would be helpful.

Stefan

signature.asc
Description: PGP signature