[PATCH v3] Use io_uring_register_ring_fd() to skip fd operations

[PATCH v3] Use io_uring_register_ring_fd() to skip fd operations

[Sam Li]

Linux recently added a new io_uring(7) optimization API that QEMU
doesn't take advantage of yet. The liburing library that QEMU uses
has added a corresponding new API calling io_uring_register_ring_fd().
When this API is called after creating the ring, the io_uring_submit()
library function passes a flag to the io_uring_enter(2) syscall
allowing it to skip the ring file descriptor fdget()/fdput()
operations. This saves some CPU cycles.

Signed-off-by: Sam Li <faithilikerun@gmail.com>
---
 block/io_uring.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/block/io_uring.c b/block/io_uring.c
index 782afdb433..51f4834b69 100644
--- a/block/io_uring.c
+++ b/block/io_uring.c
@@ -435,8 +435,16 @@ LuringState *luring_init(Error **errp)
     }
 
     ioq_init(&s->io_q);
-    return s;
+    if (io_uring_register_ring_fd(&s->ring) < 0) {
+        /*
+         * Only warn about this error: we will fallback to the non-optimized
+         * io_uring operations.
+         */
+        error_setg_errno(errp, errno,
+                         "failed to register linux io_uring ring file descriptor");
+    }
 
+    return s;
 }
 
 void luring_cleanup(LuringState *s)
-- 
2.35.1

Re: [PATCH v3] Use io_uring_register_ring_fd() to skip fd operations

[Fam Zheng]

On 2022-04-19 07:33, Sam Li wrote:
> Linux recently added a new io_uring(7) optimization API that QEMU
> doesn't take advantage of yet. The liburing library that QEMU uses
> has added a corresponding new API calling io_uring_register_ring_fd().
> When this API is called after creating the ring, the io_uring_submit()
> library function passes a flag to the io_uring_enter(2) syscall
> allowing it to skip the ring file descriptor fdget()/fdput()
> operations. This saves some CPU cycles.
> 
> Signed-off-by: Sam Li <faithilikerun@gmail.com>
> ---
>  block/io_uring.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/block/io_uring.c b/block/io_uring.c
> index 782afdb433..51f4834b69 100644
> --- a/block/io_uring.c
> +++ b/block/io_uring.c
> @@ -435,8 +435,16 @@ LuringState *luring_init(Error **errp)
>      }
>  
>      ioq_init(&s->io_q);
> -    return s;
> +    if (io_uring_register_ring_fd(&s->ring) < 0) {
> +        /*
> +         * Only warn about this error: we will fallback to the non-optimized
> +         * io_uring operations.
> +         */
> +        error_setg_errno(errp, errno,
> +                         "failed to register linux io_uring ring file descriptor");
> +    }
>  
> +    return s;

As a general convention, I don't think the errp is going to get proper handling
by the callers, if non-NULL is returned like here. IOW a matching error_free is
never called and this is memory leak?

I guess error_report is better?

Fam

>  }
>  
>  void luring_cleanup(LuringState *s)
> -- 
> 2.35.1
> 
> 

Re: [PATCH v3] Use io_uring_register_ring_fd() to skip fd operations

[olc]

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

Hi Fam,
I've missed out freeing error object and error_report would be a better
case indeed.
Thanks for pointing out the problem.  I'll fix that in no time.

Best regards,
Sam



Fam Zheng <fam@euphon.net> 于2022年4月21日周四 21:36写道：

> On 2022-04-19 07:33, Sam Li wrote:
> > Linux recently added a new io_uring(7) optimization API that QEMU
> > doesn't take advantage of yet. The liburing library that QEMU uses
> > has added a corresponding new API calling io_uring_register_ring_fd().
> > When this API is called after creating the ring, the io_uring_submit()
> > library function passes a flag to the io_uring_enter(2) syscall
> > allowing it to skip the ring file descriptor fdget()/fdput()
> > operations. This saves some CPU cycles.
> >
> > Signed-off-by: Sam Li <faithilikerun@gmail.com>
> > ---
> >  block/io_uring.c | 10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/block/io_uring.c b/block/io_uring.c
> > index 782afdb433..51f4834b69 100644
> > --- a/block/io_uring.c
> > +++ b/block/io_uring.c
> > @@ -435,8 +435,16 @@ LuringState *luring_init(Error **errp)
> >      }
> >
> >      ioq_init(&s->io_q);
> > -    return s;
> > +    if (io_uring_register_ring_fd(&s->ring) < 0) {
> > +        /*
> > +         * Only warn about this error: we will fallback to the
> non-optimized
> > +         * io_uring operations.
> > +         */
> > +        error_setg_errno(errp, errno,
> > +                         "failed to register linux io_uring ring file
> descriptor");
> > +    }
> >
> > +    return s;
>
> As a general convention, I don't think the errp is going to get proper
> handling
> by the callers, if non-NULL is returned like here. IOW a matching
> error_free is
> never called and this is memory leak?
>
> I guess error_report is better?
>
> Fam
>
> >  }
> >
> >  void luring_cleanup(LuringState *s)
> > --
> > 2.35.1
> >
> >
>

[-- Attachment #2: Type: text/html, Size: 2698 bytes --]