Re: [virtio-comment] [PATCH v3 1/4] Add VIRTIO_RING_F_INDIRECT_SIZE

["Michael S. Tsirkin" <mst@redhat.com>]

On Sun, Mar 20, 2022 at 02:32:23PM +0100, Christian Schoenebeck wrote:
> On Sonntag, 20. März 2022 13:31:51 CET Michael S. Tsirkin wrote:
> > On Sat, Mar 19, 2022 at 01:00:28PM +0100, Christian Schoenebeck wrote:
> > > On Samstag, 19. März 2022 10:33:49 CET Michael S. Tsirkin wrote:
> > > > On Wed, Mar 16, 2022, 15:47 Christian Schoenebeck
> > > > <qemu_oss@crudebyte.com>
> > > > 
> > > > wrote:
> > > > > This new feature flag allows to decouple the maximum amount of
> > > > > descriptors in indirect descriptor tables from the Queue Size.
> > > > 
> > > > if we are extending these limits, I suggest reusing the feature flag to
> > > > also add a limit on total s/g list size. making it separate from queue
> > > > size
> > > > was requested a while ago.
> > > 
> > > What do you mean with "total s/g list size"? The maximum bulk data size
> > > per
> > > message?
> > > Sum of both in and out s/g lists' bulk data or only for one of them?
> > > Or maximum size of exactly only one memory segment?
> > 
> > I don't really know what does "bulk data size" mean. Suggest we use
> > terminology from the spec. A buffer includes a group of direct and/or
> > indirect descriptors, in turn indirect descriptors point to direct
> > descriptors.
> 
> I already described why I think it makes sense not calling it "buffer" in this 
> particular context. So I am against changing this to "buffer".

Well spec just defines what buffers are.
If you are using a different term then you need to define it in the
spec.

> If other people 
> support your position then I'll change it though of course.
> 
> About "bulk data size": "Bulk data" is the user data actually being used on 
> application/driver level, i.e. above virtio level, and "Bulk data size" the 
> size of that data. See ASCII illustration here:
> https://github.com/oasis-tcs/virtio-spec/issues/122
> 
> The terminology "bulk data" is already used in the spec already BTW.

It does not refer to anything specific though, just generally
to vqs for passing lots of data as opposed to config space used
to pass small amount of data.

> > What has been requested a while is ability to limit per vq the
> > # of direct descriptors in a buffer.
> 
> So in cases where indirect descriptors are *not* used. This series is about 
> indirect descriptors only though.

No, in all cases.

> > Since IIUC what you want to do is allow more descriptors
> > than VQ size, then one way to achieve that is just to have
> > a per VQ limit on descriptor size and have that limit > VQ size.
> 
> Sorry, I can't follow you. What do you mean with "descriptor size"?
> For me a 
> descriptor has a predefined constant struct size. You mean the size of one 
> memory segment referenced by one indirect descriptor? And why would it be 
> better than what this series suggests?

My bad. What I meant is a "per VQ limit on # of direct descriptors
per buffer".

> 
> > Another thing related is that people wanted to block indirect
> > descriptors for some VQs. Not yet sure how to combine that
> > with this proposal, worth thinking about.
> 
> This series allows both, increasing *and* decreasing the number of indirect 
> descriptors per VQ already.

I don't see how you block indirect descritors for a queue with this.
Did I miss it?

Also I think you missed the fact that
a direct descriptor can point to an indirect one, the
result is that max # of descriptors in a buffer is then:

queue size - 1 + indirect table size 

I don't see how your proposal limits the # of descriptors
below queue size since guest is never forced to use
indirect.


> > > And are you suggesting this should become part of this series already?
> > 
> > yes since it's touching mostly same areas in the spec.
> 
> :/ Please note that I sent the first draft on this issue already in November 
> last year, and have not seen any response from your side so far. I actually 
> assumed we were already at a point where it was just about precise wording et 
> al., not restarting to redesign everything again from scratch now.

Sorry about that.

> > > > > The new term "Queue Indirect Size" is introduced for this purpose,
> > > > > which is a transport specific configuration whose negotiation is
> > > > > further specified for each transport with subsequent patches.
> > > > > 
> > > > > Fixes: https://github.com/oasis-tcs/virtio-spec/issues/122
> > > > > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> > > > > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> > > > > ---
> > > > > 
> > > > >  content.tex     | 32 ++++++++++++++++++++++++++++++--
> > > > >  packed-ring.tex |  2 +-
> > > > >  split-ring.tex  |  8 ++++++--
> > > > >  3 files changed, 37 insertions(+), 5 deletions(-)
> > > > > 
> > > > > diff --git a/content.tex b/content.tex
> > > > > index c6f116c..685525d 100644
> > > > > --- a/content.tex
> > > > > +++ b/content.tex
> > > > > @@ -99,10 +99,10 @@ \section{Feature Bits}\label{sec:Basic Facilities
> > > > > of a
> > > > > Virtio Device / Feature B
> > > > > 
> > > > >  \begin{description}
> > > > >  \item[0 to 23, and 50 to 127] Feature bits for the specific device
> > > > >  type
> > > > > 
> > > > > -\item[24 to 40] Feature bits reserved for extensions to the queue and
> > > > > +\item[24 to 41] Feature bits reserved for extensions to the queue and
> > > > > 
> > > > >    feature negotiation mechanisms
> > > > > 
> > > > > -\item[41 to 49, and 128 and above] Feature bits reserved for future
> > > > > extensions.
> > > > > +\item[42 to 49, and 128 and above] Feature bits reserved for future
> > > > > extensions.
> > > > > 
> > > > >  \end{description}
> > > > >  
> > > > >  \begin{note}
> > > > > 
> > > > > @@ -1051,6 +1051,10 @@ \subsubsection{Common configuration structure
> > > > > layout}\label{sec:Virtio Transport
> > > > > 
> > > > >  present either a value of 0 or a power of 2 in
> > > > >  \field{queue_size}.
> > > > > 
> > > > > +If VIRTIO_RING_F_INDIRECT_SIZE has been negotiated, the device MUST
> > > > > provide the
> > > > > +Queue Indirect Size supported by device, which is a transport
> > > > > specific
> > > > > +configuration. It MUST allow the driver to set a lower value.
> > > > > +
> > > > > 
> > > > >  \drivernormative{\paragraph}{Common configuration structure
> > > > > 
> > > > > layout}{Virtio Transport Options / Virtio Over PCI Bus / PCI Device
> > > > > Layout
> > > > > / Common configuration structure layout}
> > > > > 
> > > > >  The driver MUST NOT write to \field{device_feature},
> > > > >  \field{num_queues},
> > > > > 
> > > > > \field{config_generation}, \field{queue_notify_off} or
> > > > > \field{queue_notify_data}.
> > > > > @@ -6847,6 +6851,30 @@ \chapter{Reserved Feature
> > > > > Bits}\label{sec:Reserved
> > > > > Feature Bits}
> > > > > 
> > > > >    that the driver can reset a queue individually.
> > > > >    See \ref{sec:Basic Facilities of a Virtio Device / Virtqueues /
> > > > > 
> > > > > Virtqueue Reset}.
> > > > > 
> > > > > +  \item[VIRTIO_RING_F_INDIRECT_SIZE(41)] This feature indicates that
> > > > > the
> > > > > +  Queue Indirect Size, i.e. the maximum amount of descriptors in
> > > > > indirect
> > > > > +  descriptor tables, is independent from the Queue Size.
> > > > > +
> > > > > +  Without this feature, the Queue Size limits the length of the
> > > > > descriptor
> > > > > +  chain, including indirect descriptor tables as in \ref{sec:Basic
> > > > > Facilities of
> > > > > +  a Virtio Device / Virtqueues / The Virtqueue Descriptor Table /
> > > > > Indirect
> > > > > +  Descriptors}, i.e. both the maximum amount of slots in the vring
> > > > > and
> > > > > the
> > > > > +  actual bulk data size transmitted per vring slot.
> > > > 
> > > > spect does not call these  slots elsewhere.
> > > 
> > > Yes, I intentionally used "vring slot" instead of "buffer" as I find the
> > > latter too vague in this context. A "buffer" can be a memory segment, a
> > > set of memory segments and what not. "vring slot" OTOH makes it clear
> > > that it is about exactly one, atomic pointer (hence with fixed size) in a
> > > Ring Buffer, as depicted in the ASCII illustration here:
> > > 
> > > https://github.com/oasis-tcs/virtio-spec/issues/122
> > > 
> > > The maximum amount of vring slots is therefore the maximum amount of
> > > messages that can be emplaced into a Ring Buffer, independent of any
> > > "bulk data buffer size".
> > > 
> > > > +
> > > > 
> > > > > +  With this feature enabled, the Queue Size only limits the maximum
> > > > > amount
> > > > > +  of slots in the vring, but does not limit the actual bulk data size
> > > > > +  being transmitted when indirect descriptors are used. Decoupling
> > > > > these
> > > > > +  two configuration parameters this way not only allows much larger
> > > > > bulk
> > > > > data
> > > > > +  being transferred per vring slot, but also avoids complicated
> > > > > synchronization
> > > > > +  mechanisms if the device only supports a very small amount of vring
> > > > > slots. Due
> > > > > +  to the 16-bit size of a descriptor's "next" field there is still an
> > > > > absolute
> > > > > +  limit of $2^{16}$ descriptors per indirect descriptor table.
> > > > > However
> > > > > the
> > > > > +  actual maximum amount supported by either device or driver might be
> > > > > less,
> > > > > +  and therefore the bus specific Queue Indirect Size value MUST
> > > > > additionally
> > > > > +  be negotiated if VIRTIO_RING_F_INDIRECT_SIZE was negotiated to
> > > > > subsequently
> > > > > +  negotiate the actual amount of maximum indirect descriptors
> > > > > supported
> > > > > +  by both sides.
> > > > 
> > > > still not sure what exactly is the value. e.g. in a buffer including
> > > > indirect and direct descriptors.
> > > > 
> > > > +
> > > > 
> > > > >  \end{description}
> > > > >  
> > > > >  \drivernormative{\section}{Reserved Feature Bits}{Reserved Feature
> > > > >  Bits}
> > > > > 
> > > > > diff --git a/packed-ring.tex b/packed-ring.tex
> > > > > index a9e6c16..e26d112 100644
> > > > > --- a/packed-ring.tex
> > > > > +++ b/packed-ring.tex
> > > > > @@ -195,7 +195,7 @@ \subsection{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.
> > > > > +size unless the VIRTIO_RING_F_INDIRECT_SIZE feature has been
> > > > > negotiated.
> > > > > 
> > > > >  \subsection{Next Flag: Descriptor Chaining}
> > > > >  \label{sec:Packed Virtqueues / Next Flag: Descriptor Chaining}
> > > > > 
> > > > > diff --git a/split-ring.tex b/split-ring.tex
> > > > > index de94038..eaa90c3 100644
> > > > > --- a/split-ring.tex
> > > > > +++ b/split-ring.tex
> > > > > @@ -268,8 +268,12 @@ \subsubsection{Indirect
> > > > > Descriptors}\label{sec:Basic
> > > > > Facilities of a Virtio Devi
> > > > > 
> > > > >  set the VIRTQ_DESC_F_INDIRECT flag within an indirect descriptor (ie.
> > > > >  only
> > > > >  one table per descriptor).
> > > > > 
> > > > > -A driver MUST NOT create a descriptor chain longer than the Queue
> > > > > Size of
> > > > > -the device.
> > > > 
> > > > +If VIRTIO_RING_F_INDIRECT_SIZE has not been negotiated, the driver MUST
> > > > 
> > > > > +NOT create a descriptor chain longer than the Queue Size of the
> > > > > device.
> > > > > +
> > > > > +If VIRTIO_RING_F_INDIRECT_SIZE has been negotiated, the number of
> > > > > +descriptors per indirect descriptor table MUST NOT exceed the
> > > > > negotiated
> > > > > +Queue Indirect Size.
> > > > 
> > > > it is not negotiated is it?
> > > 
> > > What makes you think it is not negotiated?
> 
> Also see my previous question here ^


Sorry, what I mean is that you don't define what does negotiation
involve. I think you mean this:

	The driver SHOULD write to \field{queue_indirect_size} if its maximum
	number of descriptors per vring slot is lower than that reported by the
	device.

but driver can just read the value and that's it - and then the value
that is set by device applies, right?

If you are going to use terms such as negotiated you need to define what
they mean. In this case I would just say something like
"the value of Queue Indirect Size".



> > > > >  A driver MUST NOT set both VIRTQ_DESC_F_INDIRECT and
> > > > >  VIRTQ_DESC_F_NEXT
> > > > >  in \field{flags}.
> > > > > 
> > > > > --
> > > > > 2.30.2
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > This publicly archived list offers a means to provide input to the
> > > > > OASIS Virtual I/O Device (VIRTIO) TC.
> > > > > 
> > > > > In order to verify user consent to the Feedback License terms and
> > > > > to minimize spam in the list archive, subscription is required
> > > > > before posting.
> > > > > 
> > > > > Subscribe: virtio-comment-subscribe@lists.oasis-open.org
> > > > > Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
> > > > > List help: virtio-comment-help@lists.oasis-open.org
> > > > > List archive: https://lists.oasis-open.org/archives/virtio-comment/
> > > > > Feedback License:
> > > > > https://www.oasis-open.org/who/ipr/feedback_license.pdf
> > > > > List Guidelines:
> > > > > https://www.oasis-open.org/policies-guidelines/mailing-lists
> > > > > Committee: https://www.oasis-open.org/committees/virtio/
> > > > > Join OASIS: https://www.oasis-open.org/join/
>