Re: [Qemu-devel] [RFC] block: Removed coroutine ownership assumption

[Peter Crosthwaite]

On Fri, Jun 22, 2012 at 6:53 PM, Kevin Wolf <address@hidden> wrote:
> Am 22.06.2012 10:20, schrieb Peter Crosthwaite:
>> On Fri, Jun 22, 2012 at 5:49 PM, Kevin Wolf <address@hidden> wrote:
>>> Am 22.06.2012 08:44, schrieb Peter A. G. Crosthwaite:
>>>> The block layer assumes that it is the only user of coroutines -
>>>> The qemu_in_coroutine() is used to determine if a function is in one of the
>>>> block layers coroutines, which is flawed. I.E. If a client (e.g. a device 
>>>> or
>>>> a machine model) of the block layer uses couroutine itself, the block layer
>>>> will identify the callers coroutines as its own, and may falsely yield the
>>>> calling coroutine (instead of creating its own to yield).
>>>>
>>>> AFAICT, there are no conflicts in the QEMU master here yet, but its kind 
>>>> of an
>>>> issue, as anyone who comes along and used coroutines and the block layer
>>>> together is going to run into some very obscure and hard to debug race
>>>> conditions.
>>>>
>>>> Signed-off-by: Peter A. G. Crosthwaite <address@hidden>
>>>
>>> What does your coroutine caller look like that this is a problem?
>>
>> Its a machine model that instantiated some block devices concurrently.
>> Theres some chicken-and-egg issues with the instantiation such that
>> they have the happen concurrently. One device instantiates a block
>> device (pflash_cfi_01) from coroutine context. block then check
>> qemu_in_coroutine() which returns true so it uses my coroutine for its
>> inner workings, whereas if it were a normal machine model it would
>> kick of its own coroutine to do its block stuff.
>>
>>  Does
>>> it make assumptions about the number of yields or anything like that?
>>
>> Yes it does, but I thought that was the point of coroutines vs
>> threads? coroutines you control yield behaviour explicitly whearas
>> thread you cant?
>
> Well, I can see your point, although today no code in qemu makes use of
> the property that the caller could in theory know where the coroutine
> yields. I think it's also dangerous because there is a non-obvious
> dependency of the caller on the internals of the coroutine. A simple
> innocent looking refactoring of the coroutine might break assumptions
> that are made here.
>
> Other code in qemu that uses coroutines only makes use of the fact that
> coroutines can only be interrupted at known points,

So within the block layer, will the block coroutines yield anywhere or
only at a qemu_coroutine_yield() site? Would the block layer break if
a couroutine could be pre-empted by the OS anywhere?

So lets continue this to an example of multiple clients of
qemu-coroutine. The block layer is one client. It defines three
possible yields points (qemu_co_yield() sites) in block.c. Lets call
them A,B and C. The co-routine policy is that your thread of control
will not be pre-empted until locations  A, B or C are reached (I.E.
coroutines can only be interrupted at known points). Alls fair and it
works.

Now here is where it breaks. I have a device or machine model or
whatever (lets call it foo) that uses co-routines. It decides that it
wants its coroutines to stop at only at points D,E and F for ease of
synchronization. If those coroutines however make calls into to the
block layer, the block layer will think its in its own co-routine and
potentially yield at any of points A,B and C. Thing is, my foo system
doesn't care about the block layer implementation, so it shouldnt have
awareness of the fact it used co-routines too. But because it talks to
block, it can get yielded as any call into the block API which is
awkward considering the point of coroutines is they can only be
interrupted at known points. You have essentially defeated the purpose
or coroutines in the first place. Foo's coroutines are behaving like
preemptible threads (while blocks are behaving like coroutines).

The problem is solved with a simple piece of policy - dont yield
someone elses co-routine. Thats this patch. Note that this is an RFC
intended to start discussion - it needs a real implementation.
Something along the lines of keeping track of the block layer
coroutines and checking if in one of those specifically, rather then a
blanket call to qemu_in_coroutine(). But I have this patch in my
workarounds branch until we figure out a real solution.

Regards,
Peter

 so synchronisation
> between multiple coroutines becomes easier.
>
> Kevin