On Mon, Jan 30, 2017 at 04:18:10PM -0500, Paolo Bonzini wrote: > On 30/01/2017 10:50, Stefan Hajnoczi wrote: > > On Fri, Jan 20, 2017 at 05:43:12PM +0100, Paolo Bonzini wrote: > >> + aio_co_wake(s->recv_coroutine[i]); > >> > >> - qemu_coroutine_enter(nbd_get_client_session(bs)->send_coroutine); > >> + /* We're woken up by the recv_coroutine itself. */ > >> + qemu_coroutine_yield(); > > > > This relies on recv_coroutine() entering us only after we've yielded - > > otherwise QEMU will crash. The code and comments don't make it obvious > > why this is guaranteed to be safe. > > It doesn't rely on that (that's the magic hidden behind aio_co_wake), > but you're right there is a documentation problem because I needed 10 > minutes to remind myself why it's correct. > > It works because neither coroutine is moving context: > > - if the two coroutines are in the same context, aio_co_wake queues the > coroutine for execution after the yield, which is obviously okay; > > - if they are in different context, the recv_coroutine's aio_co_wake > queues the read_reply_co with aio_co_schedule. It is then woken up > through a bottom half, which executes only after read_reply has yielded. > > It is implied by the aio_co_wake and aio_co_schedule documentation; all > requirements are satisfied: > > 1) aio_co_wake's comment says: > > * aio_co_wake may be executed either in coroutine or non-coroutine > * context. The coroutine must not be entered by anyone else while > * aio_co_wake() is active. > > read_reply_co is only woken by one recv_coroutine at a time > > 2) And for the case where read_reply_co and recv_coroutine are in > different contexts, aio_co_schedule's comment says: > > * In addition the coroutine must have yielded unless ctx > * is the context in which the coroutine is running (i.e. the value of > * qemu_get_current_aio_context() from the coroutine itself). > > which is okay because aio_co_wake always uses "the context in > which the coroutine is running" as the argument to aio_co_schedule. > > Any suggestions on how to document this properly? I'm not sure a > comment in the NBD driver is the best place, because similar logic will > appear soon in other networked block drivers.
Maybe add a new function that just does these two calls. I don't have a good name for it. Stefan
signature.asc
Description: PGP signature
