Re: [virtio-comment] Re: [PATCH 2/2] Add common configuration field "queue_indirect_size"

[Christian Schoenebeck <qemu_oss@crudebyte.com>]

On Montag, 24. Januar 2022 17:41:09 CET Stefan Hajnoczi wrote:
> On Mon, Jan 24, 2022 at 03:31:57PM +0100, Christian Schoenebeck wrote:
> > On Montag, 24. Januar 2022 14:39:28 CET Stefan Hajnoczi wrote:
> > > On Wed, Jan 05, 2022 at 02:52:46PM +0100, Christian Schoenebeck wrote:
> > > > On Montag, 3. Januar 2022 14:21:13 CET Cornelia Huck wrote:
> > > > > On Wed, Dec 29 2021, Christian Schoenebeck <qemu_oss@crudebyte.com>
> > 
> > wrote:
> > > > > > On Donnerstag, 23. Dezember 2021 12:03:50 CET Cornelia Huck wrote:
> > > > > >> On Wed, Dec 15 2021, Christian Schoenebeck
> > > > > >> <qemu_oss@crudebyte.com>
> > > > 
> > > > wrote:
> > > > > >> > On Dienstag, 14. Dezember 2021 18:20:28 CET Cornelia Huck 
wrote:
> > > > > >> >> Also, this is only for split ring; does packed ring need any
> > > > > >> >> updates?
> > > > > >> > 
> > > > > >> > I have not reviewed the packed ring as much as I did the split
> > > > > >> > ring, so
> > > > > >> > I
> > > > > >> > could not say reliably all the parts that shall be updated for
> > > > > >> > the
> > > > > >> > packed
> > > > > >> > ring. There are some obvious parts like:
> > > > > >> > 
> > > > > >> > 2.7.5 Scatter-Gather Support
> > > > > >> > 
> > > > > >> > "The device limits the number of descriptors in a list through
> > > > > >> > a
> > > > > >> > transport-
> > > > > >> > specific and/or device-specific value. If not limited, the
> > > > > >> > maximum
> > > > > >> > number
> > > > > >> > of descriptors in a list is the virt queue size."
> > > > > >> > 
> > > > > >> > However the question is, would anybody want large descriptor
> > > > > >> > chains
> > > > > >> > with
> > > > > >> > the packaged ring in the first place? If I understand it
> > > > > >> > correctly,
> > > > > >> > the
> > > > > >> > benefits of the packed ring over the split ring only manifest
> > > > > >> > for
> > > > > >> > devices
> > > > > >> > that interchange a very large number of rather small bulk data
> > > > > >> > (e.g.
> > > > > >> > network devices), no?
> > > > > >> 
> > > > > >> If we think that the feature does not make sense for packed ring,
> > > > > >> they
> > > > > >> should probably conflict with each other. Otherwise, I think we
> > > > > >> need
> > > > > >> at
> > > > > >> least a statement that the higher limit does not take effect for
> > > > > >> packed
> > > > > >> ring, or touch all the places where it would be relevant.
> > > > > >> 
> > > > > >> What do others think?
> > > > > > 
> > > > > > It would indeed be very useful if other people express their
> > > > > > opinion
> > > > > > about
> > > > > > this issue (packed ring scenario) as well before I continue on
> > > > > > this
> > > > > > issue.
> > > > > > 
> > > > > > Probably the fact that my patches never made it through to the
> > > > > > list
> > > > > > were
> > > > > > not necessarily supporting this. Should I contact somebody
> > > > > > regarding
> > > > > > this
> > > > > > ML issue? Do members of the other ML also read this virtio-comment
> > > > > > list?
> > > > > 
> > > > > Yes, this situation is very unsatisfactory :( (I have contacted the
> > > > > people running this list, but there have not yet been any fixes...)
> > > > 
> > > > Only my emails with patches are refused by the list. All my other
> > > > emails
> > > > are accepted. So not sure if the cause is really DKIM or something
> > > > else.
> > > > Maybe the admins can suggest a workaround for me.
> > > > 
> > > > > Not sure which other lists would be appropriate to cc: -- maybe
> > > > > virtualization@lists.linux-foundation.org, but that one also suffers
> > > > > from DKIM issues :(
> > > > 
> > > > What I thought was wether subscribers of virtio-dev would typically
> > > > read
> > > > virtio-comment as well. Because AFAICS people who more frequently deal
> > > > with
> > > > virtio for their companies rather seem to post to virtio-dev.
> > > > 
> > > > > > I tried to compensate the current situation by updating the
> > > > > > corresponding
> > > > > > issue description on Github in a very defailed and verbose way:
> > > > > > https://github.com/oasis-tcs/virtio-spec/issues/122
> > > > > 
> > > > > Thanks. Hopefully me quoting this makes it more visible (I tried to
> > > > > quote more than I usually would in my other replies already...)
> > > > > 
> > > > > Just to feature it more prominently for people who collapse quotes:
> > > > > 
> > > > > https://github.com/oasis-tcs/virtio-spec/issues/122
> > > > > 
> > > > > > If nobody replies early January, I would suggest to continue by
> > > > > > ignoring
> > > > > > the packed ring. Because if somebody wants this for packed ring in
> > > > > > future, this can still be added to the specs without breaking
> > > > > > things,
> > > > > > because this feature is negotiated per queue, not for the entire
> > > > > > device.
> > > > > 
> > > > > The problem is that we need to specify what is supposed to happen if
> > > > > packed ring *and* this feature are negotiated. If we do not want to
> > > > > add
> > > > > statements for the packed ring case, my suggestion would be
> > > > > - make packed ring and this feature mutually exclusive
> > > > > - add a new feature bit that works with packed ring later, if we
> > > > > think
> > > > > 
> > > > >   it is useful
> > > > 
> > > > Mja, I have to correct myself on that: I wrote in my previous email
> > > > that
> > > > this was negotiated per queue. That's false of course as all virtio
> > > > features are currently negotiated for the entire device.
> > > > 
> > > > So you are right, if this new indirect size feature was negotiated
> > > > then it
> > > > would apply to both a) split rings and b) packed rings a device might
> > > > have.
> > > > Which is unfortunate.
> > > > 
> > > > Stefan, you are aware about this circumstance as well, right? Because
> > > > I
> > > > remember we originally had a discussion on qemu-devel where you wanted
> > > > to
> > > > have this configurable per queue, not per device.
> > > 
> > > Regarding packed virtqueues, there are multiple reasons for using them.
> > > Recently someone asked about reducing complexity in VIRTIO
> > > implementations and one of the suggestions that Michael Tsirkin and I
> > > both made independently was to use the packed layout instead of split
> > > layout. I don't think we should assume packed virtqueues are only used
> > > in scenarios that don't need INDIRECT_SIZE.
> > > 
> > > Extending INDIRECT_SIZE to packed virtqueues looks straightforward and
> > > does not require many changes to the Packed Virtqueues section.
> > 
> > Yes I agree, it does make sense to apply this new INDIRECT_SIZE feature to
> > packed queues as well.
> > 
> > > The reason I pushed for per-virtqueue Indirect Size values is because
> > > multi-virtqueue devices have specific purposes for each virtqueue. The
> > > Queue Size field is per-virtqueue already across all transports. This
> > > way a device with control virtqueue can allocate fewer resources
> > > (smaller Queue Size and Indirect Size) to it than to the data path
> > > virtqueues that carry I/O data buffers.
> > > 
> > > I don't think it's necessary to support INDIRECT_SIZE on only a subset
> > > of a device's virtqueues. If the device doesn't want to "enable"
> > > INDIRECT_SIZE on a specific virtqueue it should just report Queue Size
> > > as the Indirect Size value?
> > 
> > Or another idea: what about adding this new indirect size field to the
> > struct that holds the queue size field already, then this would become
> > transport- independent (i.e. the discussed PCI config field would be
> > unnecessary), and it would be a feature configurable per queue. Or is
> > that struct inappropriate for feature negotiation purposes?
> 
> There is no transport-independent struct that contains Queue Size. It is
> handled by struct virtio_pci_common_cfg->queue_size, the MMIO QueueNum
> register, and the CCW struct vq_config_block->max_num and struct
> vq_info_block->num fields.

Aaah, I just realized I was completely confused about
struct virtio_pci_common_cfg: so far I assumed this were all entirely device 
global settings.

The respective queue is first selected via 'queue_select' field and then the 
subsequent fields reflect the selected queue. Now I got it.

Ok fine, then the current draft actually already negotiated the indirect size 
for each queue independently already, without me even being aware. ;-)

I guess I know all I need then for the next round of patches. Maybe I also 
come along for changes for MMIO and CCW side, we'll see.

Thanks!

Best regards,
Christian Schoenebeck