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. Stefan
signature.asc
Description: PGP signature
