Re: [Qemu-devel] [PATCH RFC 0/5] Introduce "-object iothread-group"

[Stefan Hajnoczi <stefanha@redhat.com>]

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

On Wed, Jul 12, 2017 at 07:06:33PM +0800, Fam Zheng wrote:
> On Tue, 07/11 15:58, Stefan Hajnoczi wrote:
> > On Mon, Jul 10, 2017 at 03:20:22PM +0800, Fam Zheng wrote:
> > > Last time we've looked at "-object iothread,spawns=N" but it was a bit abusive.
> > > A dedicated "iothread-group" class is cleaner from the interface point of view.
> > > This series does that.
> > > 
> > > It has the same set of poll parameters as the existing "iothread" object, plus
> > > a "size" option to specify how many threads to start. Using iothread-group
> > > doesn't require the user to explicitly create the contained IOThreads. The
> > > IOThreads are created by the group object.
> > > 
> > > Internally, IOThreads share one AioContext.  This is to make it easier to adapt
> > > this to the current data plane code (see the last patch). But it is an
> > > implementation detail, and will change depending on the block layer multiqueue
> > > needs.
> > > 
> > > TODO:
> > > 
> > > - qmp_query_iothread_groups, in addition to proper QOM @child property from
> > >   IOThreadGroup to its IOThread instances.
> > > - Add virtio-scsi.
> > > - Variant of iothread_stop_all().
> > > 
> > > Fam Zheng (5):
> > >   aio: Wrap poll parameters into AioContextPollParams
> > >   iothread: Don't error on windows
> > >   iothread: Extract iothread_start
> > >   Introduce iothread-group
> > >   virtio-blk: Add iothread-group property
> > > 
> > >  Makefile.objs                   |   2 +-
> > >  hw/block/dataplane/virtio-blk.c |  18 ++--
> > >  hw/block/virtio-blk.c           |   6 ++
> > >  include/block/aio.h             |  18 ++--
> > >  include/hw/virtio/virtio-blk.h  |   2 +
> > >  include/sysemu/iothread.h       |  35 ++++++-
> > >  iothread-group.c                | 210 ++++++++++++++++++++++++++++++++++++++++
> > >  iothread.c                      |  97 +++++++++----------
> > >  util/aio-posix.c                |  10 +-
> > >  util/aio-win32.c                |   8 +-
> > >  10 files changed, 328 insertions(+), 78 deletions(-)
> > >  create mode 100644 iothread-group.c
> > 
> > I reviewed QOM "foo[*]" array syntax but it's very limited.  Basically
> > all it does it append the new property to foo[0], foo[1], ... (i.e. it
> > allocates an index).  AFAICT there is no way to specify an array
> > property and really arrays are just individual properties.
> > 
> > Daniel Berrange has a patch for non-scalar properties:
> > "[PATCH v14 15/21] qom: support non-scalar properties with -object"
> > https://patchwork.kernel.org/patch/9358503/
> > 
> > I think the challenge with existing "[*]" or pre-allocated "foo[0]",
> > "foo[1]", ... is that they don't support variable-sized arrays via QAPI.
> 
> Plus I'm not a fan of such a "spelling out indexes explicitly" syntax. It makes
> me feel like a slave of a poorly coded programme. With it we'll avoid open
> coding an iothread group class in QEMU, but will ask the user to "open type"
> group configurations.

Indexes are necessary if the user wants to remove, swap, etc elements
later, so they do serve a purpose.  Sometimes they might not be
necessary.

> > When the virtio-blk-pci object is realized it can loop over iothread[*]
> > properties to set them up (if necessary).
> > 
> > Your current RFC series uses a single AioContext in IOThreadGroup.  I
> > think something similar can be achieved with an iothread property:
> > 
> >   -object iothread,id=iothread1,share-event-loop-with=iothread0
> > 
> > The reason I am suggesting this approach is to keep IOThread as the
> > first-class object.  Existing QMP APIs continue to apply to all objects.
> > We avoid introducing an ad-hoc group object just for IOThread.
> 
> Another possibility is to introduce 1) a "TYPE_GROUPABLE" InterfaceInfo that
> will add a ".group" str property to IOThread, and build a generic group object
> out of the objects sharing the same .group name; 2) a qdev prop
> DEFINE_PROP_LINK_TO_GROUP() that can be used to reference a QOM object group
> from virtio-blk/scsi. That way it is not specific to IOThread. (Since I am not
> QOM expert, this is basically brainstorming.)

What you describe sounds like qom/container.c except QOM links are used
instead of a QOM child relationship.

No new QOM interface is necessary in order to do this - the objects in a
group don't have to be aware that they are being grouped.

Stefan

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