[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 07/17] nbd: convert to use qio_channel_yield
From: |
Stefan Hajnoczi |
Subject: |
Re: [Qemu-devel] [PATCH 07/17] nbd: convert to use qio_channel_yield |
Date: |
Tue, 31 Jan 2017 13:50:21 +0000 |
User-agent: |
Mutt/1.7.1 (2016-10-04) |
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
- Re: [Qemu-devel] [PATCH 06/17] io: make qio_channel_yield aware of AioContexts, (continued)
Re: [Qemu-devel] [PATCH 06/17] io: make qio_channel_yield aware of AioContexts, Stefan Hajnoczi, 2017/01/30
[Qemu-devel] [PATCH 08/17] coroutine-lock: reschedule coroutine on the AioContext it was running on, Paolo Bonzini, 2017/01/20
[Qemu-devel] [PATCH 04/17] block: move AioContext and QEMUTimer to libqemuutil, Paolo Bonzini, 2017/01/20
[Qemu-devel] [PATCH 07/17] nbd: convert to use qio_channel_yield, Paolo Bonzini, 2017/01/20
[Qemu-devel] [PATCH 09/17] qed: introduce qed_aio_start_io and qed_aio_next_io_cb, Paolo Bonzini, 2017/01/20
[Qemu-devel] [PATCH 10/17] aio: push aio_context_acquire/release down to dispatching, Paolo Bonzini, 2017/01/20
[Qemu-devel] [PATCH 11/17] block: explicitly acquire aiocontext in timers that need it, Paolo Bonzini, 2017/01/20
[Qemu-devel] [PATCH 15/17] aio-posix: partially inline aio_dispatch into aio_poll, Paolo Bonzini, 2017/01/20
[Qemu-devel] [PATCH 13/17] block: explicitly acquire aiocontext in bottom halves that need it, Paolo Bonzini, 2017/01/20
[Qemu-devel] [PATCH 14/17] block: explicitly acquire aiocontext in aio callbacks that need it, Paolo Bonzini, 2017/01/20
[Qemu-devel] [PATCH 17/17] block: document fields protected by AioContext lock, Paolo Bonzini, 2017/01/20
[Qemu-devel] [PATCH 16/17] async: remove unnecessary inc/dec pairs, Paolo Bonzini, 2017/01/20