[PATCH qemu 0/1] Add IOURING_SETUP_SINGLE_ISSUER flag to improve iouring performance

[PATCH qemu 0/1] Add IOURING_SETUP_SINGLE_ISSUER flag to improve iouring performance

[~h0lyalg0rithm]

Suggestion by Stefan Hajnoczi to improve io_uring performance

Suraj Shirvankar (1):
  Add IOURING_SETUP_SINGLE_ISSUER flag to improve iouring performance

 util/fdmon-io_uring.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

-- 
2.45.3

[PATCH qemu 1/1] Add IOURING_SETUP_SINGLE_ISSUER flag to improve iouring performance

[~h0lyalg0rithm]

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(-)

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;
     }
-- 
2.45.3

Re: [PATCH qemu 1/1] Add IOURING_SETUP_SINGLE_ISSUER flag to improve iouring performance

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2344 bytes --]

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
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH qemu 1/1] Add IOURING_SETUP_SINGLE_ISSUER flag to improve iouring performance

[Kevin Wolf]

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

Re: [PATCH qemu 1/1] Add IOURING_SETUP_SINGLE_ISSUER flag to improve iouring performance

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1031 bytes --]

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
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH qemu 1/1] Add IOURING_SETUP_SINGLE_ISSUER flag to improve iouring performance

[Kevin Wolf]

[-- Attachment #1: Type: text/plain, Size: 1161 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH qemu 1/1] Add IOURING_SETUP_SINGLE_ISSUER flag to improve iouring performance

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1953 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]