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

[Stefan Hajnoczi <stefanha@redhat.com>]

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

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

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