Re: VIRTIO_RING_F_INDIRECT_SIZE status

[Stefan Hajnoczi <stefanha@redhat.com>]

On Mon, Mar 06, 2023 at 04:50:53PM -0500, Michael S. Tsirkin wrote:
> On Mon, Mar 06, 2023 at 03:46:01PM -0500, Stefan Hajnoczi wrote:
> > On Mon, Mar 06, 2023 at 12:41:25PM -0500, Michael S. Tsirkin wrote:
> > > On Mon, Mar 06, 2023 at 04:00:37PM +0100, Christian Schoenebeck wrote:
> > > > On Wednesday, March 1, 2023 3:55:57 PM CET Michael S. Tsirkin wrote:
> > > > > On Wed, Mar 01, 2023 at 01:55:14PM +0100, Christian Schoenebeck wrote:
> > > > > >   2.8 Packed Virtqueues
> > > > > >   ...
> > > > > >   2.8.5 Scatter-Gather Support [1]
> > > > > >   ...
> > > > > >   While unusual (most implementations either create all lists solely using   
> > > > > >   non-indirect descriptors, or always use a single indirect element), if both 
> > > > > >   features have been negotiated, mixing indirect and non-indirect descriptors 
> > > > > >   in a ring is valid, as long as each list only contains descriptors of a 
> > > > > >   given type.
> > > > > > 
> > > > > >   [1] https://docs.oasis-open.org/virtio/virtio/v1.2/cs01/virtio-v1.2-cs01.html#x1-770005
> > > > > > 
> > > > > > To avoid misapprehensions: the way I understand it, same restrictions apply to
> > > > > > packed queues as split queues, in the sense that you may neither chain several
> > > > > > tables in a single message, nor multi-level nest tables, nor mix a list of
> > > > > > direct descriptors and indirect descriptors on the same level within one
> > > > > > message. So the explicit exception described here, only means you may use
> > > > > > *one* indirect table in one message, while using chained direct descriptors in
> > > > > > another message. But that's it, right?
> > > > > 
> > > > > 
> > > > > That's my understanding.
> > > > > 
> > > > > > > 2. Given this is a lot of work I am trying to find a way to
> > > > > > > make the impact bigger. In particular to cover the use-case
> > > > > > > of limiting s/g to 1k while making queues deeper (with
> > > > > > > or without indirect). For this I proposed:
> > > > > > > 
> > > > > > > 	So I think that given this, we can limit the total number
> > > > > > > 	of non-indirect descriptors, including non-indirect ones
> > > > > > > 	in a chain + all the ones in indirect pointer table if any,
> > > > > > > 	and excluding the indirect descriptor itself, and this
> > > > > > > 	will address the issue you are describing here, right?
> > > > > > > 
> > > > > > > people seemed to be ok with this idea?
> > > > > > 
> > > > > > IIUIC it would not make a difference from design perspective from what I
> > > > > > proposed, as virtio currently neither allows to mix, chain or mult-level nest
> > > > > > indirect descriptor tables within a single message), and hence it would just
> > > > > > boil down to adjusting the wording. So yes, it would therefore cover my
> > > > > > intended use case.
> > > > > > 
> > > > > > Best regards,
> > > > > > Christian Schoenebeck
> > > > > 
> > > > > 
> > > > > Sounds good to me. One interesting case is scsi and blk which have
> > > > > a seg_max field. This is defined as
> > > > > 
> > > > > \item[\field{seg_max}] is the maximum number of segments that can be in a
> > > > >     command. A bidirectional command can include \field{seg_max} input
> > > > >     segments and \field{seg_max} output segments.
> > > > > 
> > > > > it is never explained what *are* the segments, or how does it
> > > > > interact with VQ depth. Current drivers interpret this
> > > > > strictly and assume that this limits the s/g length but does not
> > > > > allow you to exceed vq size.
> > > > > 
> > > > > Do we thus want two limits (for read and write descriptors)?
> > > > 
> > > > No opinion on that, as my intended use case was just extending the buffer size
> > > > beyond queue size, not limiting it below queue size. Either way is fine with
> > > > me.
> > > > 
> > > > Anyhow, as this now gets broader scope, that also means the suggested flag
> > > > VIRTIO_RING_F_INDIRECT_SIZE needs to be renamed. VIRTIO_RING_F_BUFFER_SIZE?
> > > > 
> > > > Best regards,
> > > > Christian Schoenebeck
> > > 
> > > 
> > > Hmm that's unclear in that it might be in bytes too.
> > > Given blk and scsi call these "segments" how about
> > > VIRTIO_RING_F_SEG_MAX?
> > 
> > The VIRTIO equivalent of a "segment" is an "element".
> 
> Hmm true:
> 	A buffer consists of zero or more device-readable physically-contiguous
> 	elements followed by zero or more physically-contiguous
> 	device-writable elements (each buffer has at least one element).
> 
> However we then need to clean this up, since
> 
> - At least in one place we say
> 
> indirect elements to mean indirect descriptors.
> 
> - we also say "queue elements" to mean "avail/desc/used"
> - We also say "descriptor elements" - not 100% sure it's the same.
> 
> so we need to clean this up a bit first and maybe add
> text about indirect descriptors not counting as elements.

Haha, yes. I also remembered that QEMU's type for a virtqueue buffer is
called VirtQueueElement :).

My impression from the spec is that when talking about virtqueues an
element is a data blob that's part of a buffer and when talking about
vrings an element descriptor is the ring entry that points to the data
blob. Often the terms are used interchangeably (just "descriptors" or
"elements").

I'm not sure if the distinction is necessary. It might be simpler to
always talk about descriptors and remove the term "element", since there
is no way to avoid talking about descriptors eventually.

Stefan

Attachment: signature.asc
Description: PGP signature