qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v2 1/1] coroutine-lock: do not touch coroutine a


From: Roman Penyaev
Subject: Re: [Qemu-devel] [PATCH v2 1/1] coroutine-lock: do not touch coroutine after another one has been entered
Date: Wed, 31 May 2017 15:23:25 +0200

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?

--
Roman


>
> I think they used to be entered but not anymore after this patch.  Not
> sure if anything depends on this behavior...



reply via email to

[Prev in Thread] Current Thread [Next in Thread]