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

[Stefan Hajnoczi]

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

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.

Stefan

signature.asc
Description: PGP signature