Re: [PATCH v2 2/4] util/defer-call: move defer_call() to util/

[Stefan Hajnoczi <stefanha@redhat.com>]

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

On Fri, Aug 18, 2023 at 10:31:40AM +0200, Philippe Mathieu-Daudé wrote:
> Hi Stefan,
> 
> On 17/8/23 17:58, Stefan Hajnoczi wrote:
> > The networking subsystem may wish to use defer_call(), so move the code
> > to util/ where it can be reused.
> > 
> > As a reminder of what defer_call() does:
> > 
> > This API defers a function call within a defer_call_begin()/defer_call_end()
> > section, allowing multiple calls to batch up. This is a performance
> > optimization that is used in the block layer to submit several I/O requests
> > at once instead of individually:
> > 
> >    defer_call_begin(); <-- start of section
> >    ...
> >    defer_call(my_func, my_obj); <-- deferred my_func(my_obj) call
> >    defer_call(my_func, my_obj); <-- another
> >    defer_call(my_func, my_obj); <-- another
> >    ...
> >    defer_call_end(); <-- end of section, my_func(my_obj) is called once
> > 
> > Suggested-by: Ilya Maximets <i.maximets@ovn.org>
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >   MAINTAINERS                       |  3 ++-
> >   include/qemu/defer-call.h         | 15 +++++++++++++++
> >   include/sysemu/block-backend-io.h |  4 ----
> >   block/blkio.c                     |  1 +
> >   block/io_uring.c                  |  1 +
> >   block/linux-aio.c                 |  1 +
> >   block/nvme.c                      |  1 +
> >   hw/block/dataplane/xen-block.c    |  1 +
> >   hw/block/virtio-blk.c             |  1 +
> >   hw/scsi/virtio-scsi.c             |  1 +
> >   block/plug.c => util/defer-call.c |  2 +-
> >   block/meson.build                 |  1 -
> >   util/meson.build                  |  1 +
> >   13 files changed, 26 insertions(+), 7 deletions(-)
> >   create mode 100644 include/qemu/defer-call.h
> >   rename block/plug.c => util/defer-call.c (99%)
> > 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 6111b6b4d9..7cd7132ffc 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -2676,12 +2676,13 @@ S: Supported
> >   F: util/async.c
> >   F: util/aio-*.c
> >   F: util/aio-*.h
> > +F: util/defer-call.c
> 
> If used by network/other backends, maybe worth adding a
> brand new section instead, rather than "Block I/O path".

Changes to defer-call.c will go through my block tree. We don't split
out the event loop (async.c, aio-*.c, etc) either even though it's
shared by other subsystems. The important thing is that
scripts/get_maintainer.pl identifies the maintainers.

I'd rather not create lots of micro-subsystems in MAINTAINERS that
duplicate my email and block git repo URL.

> 
> >   F: util/fdmon-*.c
> >   F: block/io.c
> > -F: block/plug.c
> >   F: migration/block*
> >   F: include/block/aio.h
> >   F: include/block/aio-wait.h
> > +F: include/qemu/defer-call.h
> >   F: scripts/qemugdb/aio.py
> >   F: tests/unit/test-fdmon-epoll.c
> >   T: git https://github.com/stefanha/qemu.git block
> > diff --git a/include/qemu/defer-call.h b/include/qemu/defer-call.h
> > new file mode 100644
> > index 0000000000..291f86c987
> > --- /dev/null
> > +++ b/include/qemu/defer-call.h
> > @@ -0,0 +1,15 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +/*
> > + * Deferred calls
> > + *
> > + * Copyright Red Hat.
> > + */
> > +
> > +#ifndef QEMU_DEFER_CALL_H
> > +#define QEMU_DEFER_CALL_H
> > +
> 
> Please add smth like:
> 
>    /* See documentation in util/defer-call.c */

Sure, will fix.

> 
> > +void defer_call_begin(void);
> > +void defer_call_end(void);
> > +void defer_call(void (*fn)(void *), void *opaque);
> > +
> > +#endif /* QEMU_DEFER_CALL_H */
> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> 

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