On Thu, Jun 1, 2017 at 3:15 PM, Stefan Hajnoczi <stefa...@redhat.com> wrote: > On Wed, May 31, 2017 at 03:23:25PM +0200, Roman Penyaev wrote: >> On Wed, May 31, 2017 at 3:06 PM, Stefan Hajnoczi <stefa...@gmail.com> 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.
Will resend v3 then. -- Roman