[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 2/2] io: follow coroutine AioContext in qio_channel_yield()
From: |
Stefan Hajnoczi |
Subject: |
Re: [PATCH 2/2] io: follow coroutine AioContext in qio_channel_yield() |
Date: |
Thu, 24 Aug 2023 14:26:42 -0400 |
On Thu, Aug 24, 2023 at 12:26:05PM +0100, Daniel P. Berrangé wrote:
> On Wed, Aug 23, 2023 at 07:45:04PM -0400, Stefan Hajnoczi wrote:
> > The ongoing QEMU multi-queue block layer effort makes it possible for
> > multiple
> > threads to process I/O in parallel. The nbd block driver is not compatible
> > with
> > the multi-queue block layer yet because QIOChannel cannot be used easily
> > from
> > coroutines running in multiple threads. This series changes the QIOChannel
> > API
> > to make that possible.
> >
> > In the current API, calling qio_channel_attach_aio_context() sets the
> > AioContext where qio_channel_yield() installs an fd handler prior to
> > yielding:
> >
> > qio_channel_attach_aio_context(ioc, my_ctx);
> > ...
> > qio_channel_yield(ioc); // my_ctx is used here
> > ...
> > qio_channel_detach_aio_context(ioc);
> >
> > This API design has limitations: reading and writing must be done in the
> > same
> > AioContext and moving between AioContexts involves a cumbersome sequence of
> > API
> > calls that is not suitable for doing on a per-request basis.
> >
> > There is no fundamental reason why a QIOChannel needs to run within the
> > same AioContext every time qio_channel_yield() is called. QIOChannel
> > only uses the AioContext while inside qio_channel_yield(). The rest of
> > the time, QIOChannel is independent of any AioContext.
> >
> > In the new API, qio_channel_yield() queries the AioContext from the current
> > coroutine using qemu_coroutine_get_aio_context(). There is no need to
> > explicitly attach/detach AioContexts anymore and
> > qio_channel_attach_aio_context() and qio_channel_detach_aio_context() are
> > gone.
> > One coroutine can read from the QIOChannel while another coroutine writes
> > from
> > a different AioContext.
> >
> > This API change allows the nbd block driver to use QIOChannel from any
> > thread.
> > It's important to keep in mind that the block driver already synchronizes
> > QIOChannel access and ensures that two coroutines never read simultaneously
> > or
> > write simultaneously.
> >
> > This patch updates all users of qio_channel_attach_aio_context() to the
> > new API. Most conversions are simple, but vhost-user-server requires a
> > new qemu_coroutine_yield() call to quiesce the vu_client_trip()
> > coroutine when not attached to any AioContext.
> >
> > While the API is has become simpler, there is one wart: QIOChannel has a
> > special case for the iohandler AioContext (used for handlers that must not
> > run
> > in nested event loops). I didn't find an elegant way preserve that
> > behavior, so
> > I added a new API called qio_channel_set_follow_coroutine_ctx(ioc,
> > true|false)
> > for opting in to the new AioContext model. By default QIOChannel uses the
> > iohandler AioHandler. Code that formerly called
> > qio_channel_attach_aio_context() now calls
> > qio_channel_set_follow_coroutine_ctx(ioc, true) once after the QIOChannel is
> > created.
>
> I wonder if it is better to just pass the AioContext object into
> qio_channel_yield explicitly eg have
>
> qio_channel_yield(QIOChannel *ioc,
> AioContext *ctx,
> GIOCondition cond);
>
> With semantics that if 'ctx == NULL', then we assume the default
> global iohandler context, and for non-default context it must
> be non-NULL ?
>
> That could nicely de-couple the API from relying on global
> coroutine/thread state for querying an AioContext, which
> makes it easier to reason about IMHO.
Hi Dan,
I've done most of the audit necessary to understand which AioContext is
used where. The call graph is large because qio_channel_yield() is used
internally by qio_channel_readv_full_all_eof(),
qio_channel_writev_full_all(), and their variants. They would all need
a new AioContext argument.
I think it's not worth passing AioContext explicitly everywhere since
this involves a lot of code changes and more verbosity to achieve what
we already have.
However, If you think the QIOChannel API should pass AioContext
explicitly then I'll go ahead and make the changes.
Here is what I've explored so far:
qio_channel_readv_full_all_eof
mpqemu_read - should be doable
qio_channel_readv_all_eof
qio_channel_read_all_eof
multifd_recv_thread - NULL non-coroutine
vu_message_read - coroutine AioContext
qio_channel_readv_full_all
hw/virtio/vhost-user.c:backend_read() - NULL non-coroutine
qio_channel_readv_all
nbd_co_receive_offset_data_payload - coroutine AioContext
nbd_co_do_receive_one_chunk - coroutine AioContext
qio_channel_read_all
hw/virtio/vhost-user.c:backend_read() - NULL non-coroutine
tpm_emulator_unix_tx_bufs - NULL non-coroutine
nbd_read - ?
zlib_recv_pages - NULL non-coroutine
zstd_recv_pages - NULL non-coroutine
multifd_initial_recv_packet - NULL non-coroutine
nbd_opt_read - iohandler
pr_manager_helper_read - NULL non-coroutine
prh_read_request - coroutine AioContext
prh_co_entry - coroutine AioContext
char_socket_ping_pong - NULL non-coroutine
nocomp_recv_pages - NULL non-coroutine
test_io_thread_reader - NULL non-coroutine
qio_channel_writev_full_all
mpqemu_msg_send - should be doable
qio_channel_writev_all
nbd_co_send_request - coroutine AioContext
hw/virtio/vhost-user.c:backend_read() - NULL non-coroutine
qio_channel_write_all
tpm_emulator_unix_tx_bufs - NULL non-coroutine
multifd_send_initial_packet - NULL non-coroutine
multifd_send_thread - NULL non-coroutine
nbd_write - ?
prh_write_response - coroutine AioContext
prh_co_entry - coroutine AioContext
char_socket_ping_pong - NULL non-coroutine
ppm_save - iohandler
qemu_fflush - ?
nbd_negotiate_send_meta_context - iohandler
nbd/server.c:nbd_co_send_iov - cooroutine AioContext
test_io_thread_writer - NULL non-coroutine
multifd_send_thread - NULL non-coroutine
qemu_fill_buffer - ?
What do you think?
Stefan
signature.asc
Description: PGP signature