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

-- 
2.9.4

On Mon, 07/10 15:20, 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.

I forgot to mention that the last patch depends on

    [PATCH v3 00/20] qdev: Introduce DEFINE_PROP_LINK

Fam

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

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

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

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

From your TODO note above it looks like you plan to duplicate IOThread
interfaces for IOThreadGroup?  This means existing query-iothreads users
no longer see the full view of all IOThreads.

I think it would be cleaner to define and query IOThreads like they are
today but change virtio-blk/virtio-scsi to accept a list of IOThreads.
That way existing management tool functionality can be used and the only
tweak is that devices can now be added to multiple IOThreads.

It would be nice to express the group relationship in QOM instead of
open coding a new group object.  The majority of the RFC code creates a
child/link list relationship that QOM should support for any type, not
just IOThread.

Adding Eric Blake so we can discuss QMP requirements further.

On Tue, 07/11 15:15, 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
> 
> From your TODO note above it looks like you plan to duplicate IOThread
> interfaces for IOThreadGroup?  This means existing query-iothreads users
> no longer see the full view of all IOThreads.
> 
> I think it would be cleaner to define and query IOThreads like they are
> today but change virtio-blk/virtio-scsi to accept a list of IOThreads.

That way the groups are formed passively and I'm not sure if it is better for
users/tools to manage in the long run. Consider this syntax:

    -object iothread,id=iot0 \
    -object iothread,id=iot1 \
    -object iothread,id=iot2 \
    -device virtio-blk-pci,id=vblk0,iothread.0=iot0,iothread.1=iot1 \
    -device virtio-blk-pci,id=vblk1,iothread.0=iot1,iothread.1=iot2

where vblk0 uses iot0 and iot1 and vblk1 uses iot1 and iot2. There is a
intersection between the two groups. IMO it is less clean compared to the
rule set by an explicit syntax:

    -object iothread-group,id=iotg0,size=4 \
    -object iothread-group,id=iotg1,size=4 \
    -device virtio-blk-pci,id=vblk0,iothread-group=iotg0 \
    -device virtio-blk-pci,id=vblk1,iothread-group=iotg1 \


Also I have not idea how easy it is to add a "list of links" qdev property. I
remember there was some related work in progress, but I've lost the pointers.

> That way existing management tool functionality can be used and the only
> tweak is that devices can now be added to multiple IOThreads.

Another way could be to still include any IOThreads created by IOThreadGroup in
"query-iothreads" output, and add a "group name" property so users know the
groupings.

> 
> It would be nice to express the group relationship in QOM instead of
> open coding a new group object.  The majority of the RFC code creates a
> child/link list relationship that QOM should support for any type, not
> just IOThread.

Sounds fine, but I'm not sure what exactly you have in mind (I think it is
extending QOM). Can you elaborate?

Fam

On Tue, Jul 11, 2017 at 11:14:21PM +0800, Fam Zheng wrote:
> On Tue, 07/11 15:15, 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
> > 
> > From your TODO note above it looks like you plan to duplicate IOThread
> > interfaces for IOThreadGroup?  This means existing query-iothreads users
> > no longer see the full view of all IOThreads.
> > 
> > I think it would be cleaner to define and query IOThreads like they are
> > today but change virtio-blk/virtio-scsi to accept a list of IOThreads.
> 
> That way the groups are formed passively and I'm not sure if it is better for
> users/tools to manage in the long run. Consider this syntax:
> 
>     -object iothread,id=iot0 \
>     -object iothread,id=iot1 \
>     -object iothread,id=iot2 \
>     -device virtio-blk-pci,id=vblk0,iothread.0=iot0,iothread.1=iot1 \
>     -device virtio-blk-pci,id=vblk1,iothread.0=iot1,iothread.1=iot2
> 
> where vblk0 uses iot0 and iot1 and vblk1 uses iot1 and iot2. There is a
> intersection between the two groups. IMO it is less clean compared to the
> rule set by an explicit syntax:
> 
>     -object iothread-group,id=iotg0,size=4 \
>     -object iothread-group,id=iotg1,size=4 \
>     -device virtio-blk-pci,id=vblk0,iothread-group=iotg0 \
>     -device virtio-blk-pci,id=vblk1,iothread-group=iotg1 \
> 
> 
> Also I have not idea how easy it is to add a "list of links" qdev property. I
> remember there was some related work in progress, but I've lost the pointers.
> 
> > That way existing management tool functionality can be used and the only
> > tweak is that devices can now be added to multiple IOThreads.
> 
> Another way could be to still include any IOThreads created by IOThreadGroup in
> "query-iothreads" output, and add a "group name" property so users know the
> groupings.
> 
> > 
> > It would be nice to express the group relationship in QOM instead of
> > open coding a new group object.  The majority of the RFC code creates a
> > child/link list relationship that QOM should support for any type, not
> > just IOThread.
> 
> Sounds fine, but I'm not sure what exactly you have in mind (I think it is
> extending QOM). Can you elaborate?

I thought about it after sending this email.  Here is a follow-up that
gets into the QOM details:

https://lists.gnu.org/archive/html/qemu-devel/2017-07/msg02799.html

Stefan