On Fri, Apr 22, 2022 at 11:08:39AM +0100, Daniel P. Berrangé wrote: > On Fri, Apr 22, 2022 at 11:00:47AM +0100, Fam Zheng wrote: > > On 2022-04-22 09:52, Daniel P. Berrangé wrote: > > > On Fri, Apr 22, 2022 at 09:34:28AM +0100, Fam Zheng wrote: > > > > On 2022-04-22 00:36, 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 <faithilike...@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..5247fb79e2 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_reportf_err(*errp, > > > > > + "failed to register linux io_uring ring > > > > > file descriptor"); > > > > > > > > IIUC errp can be NULL, so let's not dereference it without checking. > > > > So, just > > > > use error_report? > > > > > > Plenty of people will be running kernels that lack the new feature, > > > so this "failure" will be an expected scenario. We shouldn't be > > > spamming the logs with any error or warning message. Assuming QEMU > > > remains fully functional, merely not as optimized, we should be > > > totally silent. > > > > Functionally, that's a very valid point. But performance wise, is it good to > > have some visibility of this? Since people use io_uring instead of other > > options almost certainly for performance, and here the issue does matter > > quite > > a bit. > > IMHO what you describe is largely a documentation issue, and/or something > for OS vendors to worry about if they want to maximise their users' > performance. As long as io_uring is fully functional we shouldn't print > errors on every QEMU startup, as it leads to pointless bug reports/support > escalations about something that is operating normally, wasting users and > vendors' time.
Also, this is a minor optimization. It's nice to save a CPU cycles when possible, but it's probably not significant enough that users would bother to upgrade their kernel. I think no warning is necessary. Stefan
signature.asc
Description: PGP signature