Re: [Qemu-devel] [Qemu-block] [PATCH 00/29] qed: Convert to coroutines

[Paolo Bonzini]

On 01/06/2017 18:28, Kevin Wolf wrote:
>> - qed_acquire/qed_release can be removed as you inline stuff, but this
>> should not cause bugs so you can either do it as a final patch or let
>> me remove it later.
> To be honest, I don't completely understand what they are protecting in
> the first place. The places where they are look quite strange to me. So
> I tried to simply leave them alone.
> 
> What is the reason that we can remove them when we inline stuff?
> Shouldn't the inlining be semantically equivalent?

You're right, they can be removed when going from callback to direct
call.  Callbacks are invoked without the AioContext acquire (aio_co_wake
does it for the callbacks).

>> - coroutine_fn annotations are missing.  Also can be done as a final
>> patch.  It's a bit ugly that L1/L2 table access can happen both in a
>> coroutine (for regular I/O) and outside (for create/check).
>
> Create is coroutine-based these days. Maybe we should convert check,
> too.

Good idea, separate series. :)

>> - qed_is_allocated_cb can be inlined and QEDIsAllocatedCB removed.  Of
>> course a lot more cleanup could be done (I will do a couple more such
>> changes when adding a CoMutex to make the driver thread-safe) but this
>> one is pretty low-hanging fruit and seems to be in line with the rest
>> of the patches.
>>
>> - qed_co_request doesn't need g_new for the QEDAIOCB.
>
> I stopped intentionally when the thing was fully coroutine based
> because I thought the series is already long enough. There are many more
> cleanups that could be done (e.g. QEDAIOCB should be completely
> avoided), but let's do that on top.

Agreed about qed_is_allocated_cb.  Regarding qed_co_request you're
already doing an almost complete rewrite of it in patch 27, and making
QEDAIOCB not heap allocated would be a very small change removing half a
dozen lines of code.

Paolo