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

[Fam Zheng]

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.

> With that missing piece solved it would be possible to say:
> 
>   -device virtio-blk-pci,iothread.0=iothread0,iothread.1=iothread1
> 
> Or maybe:
> 
>   -device virtio-blk-pci,iothread[0]=iothread0,iothread[1]=iothread1

Understood. As I said in the other message, I'm not quite convinced with this
"arbitrary grouping" API made possible with this syntax. Probably the simplist
cases are okay, but I'm afraid as the number of virtio-blk-pci and iothread
grows, the configuration can get complicated to manage.

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

Fam