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

[Roman Penyaev <roman.penyaev@profitbricks.com>]

On Thu, Jun 1, 2017 at 3:15 PM, Stefan Hajnoczi <stefanha@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 <stefanha@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