| Message ID | 174293621917.22751.11381319865102029969-1@git.sr.ht (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add IOURING_SETUP_SINGLE_ISSUER flag to improve iouring performance | expand |
On Tue, Mar 25, 2025 at 09:49:38PM +0100, ~h0lyalg0rithm wrote: > From: Suraj Shirvankar <surajshirvankar@gmail.com> > Please include the rationale for this change in the commit description. This way anyone reading the git log will be able to understand the intent behind this change. Something like: IORING_SETUP_SINGLE_ISSUER enables optimizations in the kernel for applications that only access the io_uring context from one thread. QEMU calls io_uring_enter(2) from one AioContext, so it is safe to enable this flag. > Signed-off-by: Suraj Shirvankar <surajshirvankar@gmail.com> > --- > util/fdmon-io_uring.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) `make check` fails with this patch applied because aio_context_new() (which calls fdmon_io_uring_setup()) is called before the thread is created. When fdmon_io_uring_wait() is called the io_uring context is now being used by another thread: qemu-system-x86_64: ../util/fdmon-io_uring.c:330: fdmon_io_uring_wait: Assertion `ret >= 0' failed. Once this hurdle is overcome it should be possible to use IORING_SETUP_SINGLE_ISSUER. Two ideas: 1. Modify aio_context_new() callers so they create the AioContext inside the thread. 2. Defer io_uring context creation until it is needed. It's probably still a good idea to create a temporary io-uring context early during startup to check that io_uring is available (and then destroy it right away). I slightly prefer the first option. > > diff --git a/util/fdmon-io_uring.c b/util/fdmon-io_uring.c > index b0d68bdc44..235837abcb 100644 > --- a/util/fdmon-io_uring.c > +++ b/util/fdmon-io_uring.c > @@ -324,8 +324,14 @@ static const FDMonOps fdmon_io_uring_ops = { > bool fdmon_io_uring_setup(AioContext *ctx) > { > int ret; > + unsigned int flags = 0; > > - ret = io_uring_queue_init(FDMON_IO_URING_ENTRIES, &ctx->fdmon_io_uring, 0); > + /* This improves performance but can be skipped on old hosts */ > +#ifdef IORING_SETUP_SINGLE_ISSUER > + flags |= IORING_SETUP_SINGLE_ISSUER The semicolon is missing at the end of the line. > +#endif > + > + ret = io_uring_queue_init(FDMON_IO_URING_ENTRIES, &ctx->fdmon_io_uring, flags); > if (ret != 0) { > return false; > } > -- > 2.45.3 >
Am 25.03.2025 um 21:49 hat ~h0lyalg0rithm geschrieben: > From: Suraj Shirvankar <surajshirvankar@gmail.com> > > Signed-off-by: Suraj Shirvankar <surajshirvankar@gmail.com> > --- > util/fdmon-io_uring.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) As Stefan already mentioned, the commit message should say why we want to set this flag and why it is correct to do so. Is there a reason why you change the io_uring_queue_init() call in util/fdmon-io_uring.c, but not the one in block/io_uring.c? If so, please document it in the commit message, too. Kevin
On Wed, Mar 26, 2025 at 06:13:44PM +0100, Kevin Wolf wrote: > Am 25.03.2025 um 21:49 hat ~h0lyalg0rithm geschrieben: > > From: Suraj Shirvankar <surajshirvankar@gmail.com> > > > > Signed-off-by: Suraj Shirvankar <surajshirvankar@gmail.com> > > --- > > util/fdmon-io_uring.c | 8 +++++++- > > 1 file changed, 7 insertions(+), 1 deletion(-) > > As Stefan already mentioned, the commit message should say why we want > to set this flag and why it is correct to do so. > > Is there a reason why you change the io_uring_queue_init() call in > util/fdmon-io_uring.c, but not the one in block/io_uring.c? I only asked Suraj to look at util/fdmon-io_uring.c because I expect block/io_uring.c's io_uring context to go away soon. In my local io_uring branches I have prepared commits that replace the io_uring context in block/io_uring.c with aio_add_sqe() calls that use the AioContext's fdmon-io_uring.c io_uring context. Stefan > > If so, please document it in the commit message, too. > > Kevin >
Am 26.03.2025 um 18:46 hat Stefan Hajnoczi geschrieben: > On Wed, Mar 26, 2025 at 06:13:44PM +0100, Kevin Wolf wrote: > > Am 25.03.2025 um 21:49 hat ~h0lyalg0rithm geschrieben: > > > From: Suraj Shirvankar <surajshirvankar@gmail.com> > > > > > > Signed-off-by: Suraj Shirvankar <surajshirvankar@gmail.com> > > > --- > > > util/fdmon-io_uring.c | 8 +++++++- > > > 1 file changed, 7 insertions(+), 1 deletion(-) > > > > As Stefan already mentioned, the commit message should say why we want > > to set this flag and why it is correct to do so. > > > > Is there a reason why you change the io_uring_queue_init() call in > > util/fdmon-io_uring.c, but not the one in block/io_uring.c? > > I only asked Suraj to look at util/fdmon-io_uring.c because I expect > block/io_uring.c's io_uring context to go away soon. > > In my local io_uring branches I have prepared commits that replace the > io_uring context in block/io_uring.c with aio_add_sqe() calls that use > the AioContext's fdmon-io_uring.c io_uring context. Then we should either document this intention in the commit message or make this one Based-on your changes. Kevin
On Thu, Mar 27, 2025 at 02:14:25PM +0100, Kevin Wolf wrote: > Am 26.03.2025 um 18:46 hat Stefan Hajnoczi geschrieben: > > On Wed, Mar 26, 2025 at 06:13:44PM +0100, Kevin Wolf wrote: > > > Am 25.03.2025 um 21:49 hat ~h0lyalg0rithm geschrieben: > > > > From: Suraj Shirvankar <surajshirvankar@gmail.com> > > > > > > > > Signed-off-by: Suraj Shirvankar <surajshirvankar@gmail.com> > > > > --- > > > > util/fdmon-io_uring.c | 8 +++++++- > > > > 1 file changed, 7 insertions(+), 1 deletion(-) > > > > > > As Stefan already mentioned, the commit message should say why we want > > > to set this flag and why it is correct to do so. > > > > > > Is there a reason why you change the io_uring_queue_init() call in > > > util/fdmon-io_uring.c, but not the one in block/io_uring.c? > > > > I only asked Suraj to look at util/fdmon-io_uring.c because I expect > > block/io_uring.c's io_uring context to go away soon. > > > > In my local io_uring branches I have prepared commits that replace the > > io_uring context in block/io_uring.c with aio_add_sqe() calls that use > > the AioContext's fdmon-io_uring.c io_uring context. > > Then we should either document this intention in the commit message or > make this one Based-on your changes. Hi Suraj, Please rebase your patch on my branch here: https://gitlab.com/stefanha/qemu/-/tree/aio_add_sqe My series removes the io_uring context from block/io_uring.c, unifying it with util/fdmon-io_uring.c. That way there's no need to duplicate io_uring context setup and your SINGLE_ISSUER change only needs to be done in util/fdmon-io_uring.c. The email thread for my series is here: https://lore.kernel.org/qemu-devel/20250401142721.280287-1-stefanha@redhat.com/T/#t You can add "Based-on: 20250401142721.280287-1-stefanha@redhat.com" to the cover letter of your patch to indicate that this does not apply to qemu.git/master but on top of my patch series. Stefan
diff --git a/util/fdmon-io_uring.c b/util/fdmon-io_uring.c index b0d68bdc44..235837abcb 100644 --- a/util/fdmon-io_uring.c +++ b/util/fdmon-io_uring.c @@ -324,8 +324,14 @@ static const FDMonOps fdmon_io_uring_ops = { bool fdmon_io_uring_setup(AioContext *ctx) { int ret; + unsigned int flags = 0; - ret = io_uring_queue_init(FDMON_IO_URING_ENTRIES, &ctx->fdmon_io_uring, 0); + /* This improves performance but can be skipped on old hosts */ +#ifdef IORING_SETUP_SINGLE_ISSUER + flags |= IORING_SETUP_SINGLE_ISSUER +#endif + + ret = io_uring_queue_init(FDMON_IO_URING_ENTRIES, &ctx->fdmon_io_uring, flags); if (ret != 0) { return false; }