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

Best regards,
Christian Schoenebeck