Re: [PATCH 2/5] Introduce VIRTIO_F_ADMIN_VQ_INDIRECT_DESC/VIRTIO_F_ADMIN_VQ_IN_ORDER

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

On Thu, Jan 13, 2022 at 07:07:53PM +0200, Max Gurtovoy wrote:
> 
> On 1/13/2022 5:33 PM, Michael S. Tsirkin wrote:
> > On Thu, Jan 13, 2022 at 04:51:00PM +0200, Max Gurtovoy wrote:
> > > These new features are parallel to VIRTIO_F_INDIRECT_DESC and
> > > VIRTIO_F_IN_ORDER. Some devices might support these features only for
> > > admin virtqueues and some might support them for both admin virtqueues
> > > and request virtqueues or only for non-admin virtqueues. Some
> > > optimization can be made for each type of virtqueue, thus we separate
> > > these features for the different virtqueue types.
> > > 
> > > Reviewed-by: Parav Pandit <parav@nvidia.com>
> > > Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
> > That seems vague as motivation.
> > Why do we need to optimize admin queues? Aren't they
> > fundamentally a control path feature?
> > Why would we want to special-case these features specifically?
> > Should we allow control of features per VQ generally?
> 
> We would like to allow executing admins commands out of order and IO
> requests in order for efficiency.

It's a control queue. Why do we worry?


> 
> And also the other way around.

what exactly does this mean?

> IO cmds and admin cmds have different considerations in many cases.

That's still pretty vague.  so do other types of VQ, such as RX/TX.

E.g. I can see how a hardware vendor might want to avoid supporting
indirect with RX for virtio net with mergeable buffers, but still
support it for TX.


I can vaguely see the usefulness of VIRTIO_F_ADMIN_VQ_IN_ORDER.
I think you want to reorder admin commands dealing with
unrelated VFs but keep io vqs in order for speed.
Just guessing, you should spell the real motivation out.
However, I think a better way to do that is with finalizing the
VIRTIO_F_PARTIAL_ORDER proposal from august.
Pls review and let me know. If there's finally a use for it
I'll prioritize finalizing that idea.
Don't see much point in tweaking INDIRECT at all.



> > 
> > 
> > > ---
> > >   content.tex     | 47 +++++++++++++++++++++++++++++++++++++++--------
> > >   packed-ring.tex | 26 +++++++++++++-------------
> > >   split-ring.tex  | 35 +++++++++++++++++++++++------------
> > >   3 files changed, 75 insertions(+), 33 deletions(-)
> > > 
> > > diff --git a/content.tex b/content.tex
> > > index c524fab..cc3e648 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] Feature bits for the specific device type
> > > -\item[24 to 41] Feature bits reserved for extensions to the queue and
> > > +\item[24 to 43] Feature bits reserved for extensions to the queue and
> > >     feature negotiation mechanisms
> > > -\item[42 and above] Feature bits reserved for future extensions.
> > > +\item[44 and above] Feature bits reserved for future extensions.
> > >   \end{description}
> > >   \begin{note}
> > > @@ -318,8 +318,9 @@ \section{Virtqueues}\label{sec:Basic Facilities of a Virtio Device / Virtqueues}
> > >   Some devices always use descriptors in the same order in which
> > >   they have been made available. These devices can offer the
> > > -VIRTIO_F_IN_ORDER feature. If negotiated, this knowledge
> > > -might allow optimizations or simplify driver and/or device code.
> > > +VIRTIO_F_IN_ORDER and/or VIRTIO_F_ADMIN_VQ_IN_ORDER features.
> > > +If negotiated, this knowledge might allow optimizations or
> > > +simplify driver and/or device code.
> > >   Each virtqueue can consist of up to 3 parts:
> > >   \begin{itemize}
> > > @@ -6768,7 +6769,7 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
> > >   Device / Virtqueues / The Virtqueue Descriptor Table / Indirect
> > >   Descriptors}~\nameref{sec:Basic Facilities of a Virtio Device /
> > >   Virtqueues / The Virtqueue Descriptor Table / Indirect
> > > -Descriptors} and \ref{sec:Packed Virtqueues / Indirect Flag: Scatter-Gather Support}~\nameref{sec:Packed Virtqueues / Indirect Flag: Scatter-Gather Support}.
> > > +Descriptors} and \ref{sec:Packed Virtqueues / Indirect Flag: Scatter-Gather Support}~\nameref{sec:Packed Virtqueues / Indirect Flag: Scatter-Gather Support}. This excluding the descriptors sent via the admin virtqueue.
> > >     \item[VIRTIO_F_EVENT_IDX(29)] This feature enables the \field{used_event}
> > >     and the \field{avail_event} fields as described in
> > >   \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Used Buffer Notification Suppression}, \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Used Ring} and \ref{sec:Packed Virtqueues / Driver and Device Event Suppression}.
> > > @@ -6800,8 +6801,9 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
> > >     support for the packed virtqueue layout as described in
> > >     \ref{sec:Basic Facilities of a Virtio Device / Packed Virtqueues}~\nameref{sec:Basic Facilities of a Virtio Device / Packed Virtqueues}.
> > >     \item[VIRTIO_F_IN_ORDER(35)] This feature indicates
> > > -  that all buffers are used by the device in the same
> > > -  order in which they have been made available.
> > > +  that all buffers are used by the device, excluding buffers used by
> > > +  the admin virtqueue, in the same order in which they have been made
> > > +  available.
> > >     \item[VIRTIO_F_ORDER_PLATFORM(36)] This feature indicates
> > >     that memory accesses by the driver and the device are ordered
> > >     in a way described by the platform.
> > > @@ -6852,6 +6854,18 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
> > >     \item[VIRTIO_F_ADMIN_VQ (41)] This feature indicates that
> > >     the device supports administration virtqueue negotiation.
> > > +  \item[VIRTIO_F_ADMIN_VQ_INDIRECT_DESC (42)] Negotiating this feature
> > > +  indicates that the driver can use descriptors with the VIRTQ_DESC_F_INDIRECT
> > > +  flag set, as described in \ref{sec:Basic Facilities of a Virtio
> > > +Device / Virtqueues / The Virtqueue Descriptor Table / Indirect
> > > +Descriptors}~\nameref{sec:Basic Facilities of a Virtio Device /
> > > +Virtqueues / The Virtqueue Descriptor Table / Indirect
> > > +Descriptors} and \ref{sec:Packed Virtqueues / Indirect Flag: Scatter-Gather Support}~\nameref{sec:Packed Virtqueues / Indirect Flag: Scatter-Gather Support}. This refers to descriptors sent via the admin
> > > +  virtqueue and excluding the descriptors that sent via other virtqueues.
> > > +  \item[VIRTIO_F_ADMIN_VQ_IN_ORDER (43)] This feature indicates
> > > +  that all buffers are used by the admin virtqueue of the device in
> > > +  the same order in which they have been made available.
> > > +
> > >   \end{description}
> > >   \drivernormative{\section}{Reserved Feature Bits}{Reserved Feature Bits}
> > > @@ -6888,6 +6902,12 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
> > >   A driver SHOULD accept VIRTIO_F_NOTIF_CONFIG_DATA if it is offered.
> > > +A driver MAY accept VIRTIO_F_ADMIN_VQ_INDIRECT_DESC only if it accepts
> > > +VIRTIO_F_ADMIN_VQ.
> > > +
> > > +A driver MAY accept VIRTIO_F_ADMIN_VQ_IN_ORDER only if it accepts
> > > +VIRTIO_F_ADMIN_VQ.
> > > +
> > >   \devicenormative{\section}{Reserved Feature Bits}{Reserved Feature Bits}
> > >   A device MUST offer VIRTIO_F_VERSION_1.  A device MAY fail to operate further
> > > @@ -6902,7 +6922,12 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
> > >   accepted.
> > >   If VIRTIO_F_IN_ORDER has been negotiated, a device MUST use
> > > -buffers in the same order in which they have been available.
> > > +buffers in the same order in which they have been available. This refers
> > > +to buffers that are used by virtqueue that is not the admin virtqueue.
> > > +
> > > +If VIRTIO_F_ADMIN_VQ_IN_ORDER has been negotiated, a device MUST use
> > > +buffers in the same order in which they have been available. This refers
> > > +only for buffers that are used by the admin virtqueue.
> > >   A device MAY fail to operate further if
> > >   VIRTIO_F_ORDER_PLATFORM is offered but not accepted.
> > > @@ -6917,6 +6942,12 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
> > >   and presents a PCI SR-IOV capability structure, otherwise
> > >   it MUST NOT offer VIRTIO_F_SR_IOV.
> > > +A device MAY offer VIRTIO_F_ADMIN_VQ_INDIRECT_DESC only if it offers
> > > +VIRTIO_F_ADMIN_VQ.
> > > +
> > > +A device MAY offer VIRTIO_F_ADMIN_VQ_IN_ORDER only if it offers
> > > +VIRTIO_F_ADMIN_VQ.
> > > +
> > >   \section{Legacy Interface: Reserved Feature Bits}\label{sec:Reserved Feature Bits / Legacy Interface: Reserved Feature Bits}
> > >   Transitional devices MAY offer the following:
> > > diff --git a/packed-ring.tex b/packed-ring.tex
> > > index a9e6c16..ef1dbc2 100644
> > > --- a/packed-ring.tex
> > > +++ b/packed-ring.tex
> > > @@ -240,13 +240,12 @@ \subsection{Indirect Flag: Scatter-Gather Support}
> > >   \label{sec:Packed Virtqueues / Indirect Flag: Scatter-Gather Support}
> > >   Some devices benefit by concurrently dispatching a large number
> > > -of large requests. The VIRTIO_F_INDIRECT_DESC feature allows this. To increase
> > > -ring capacity the driver can store a (read-only by the device) table of indirect
> > > -descriptors anywhere in memory, and insert a descriptor in the main
> > > -virtqueue (with \field{Flags} bit VIRTQ_DESC_F_INDIRECT on) that refers to
> > > -a buffer element
> > > -containing this indirect descriptor table; \field{addr} and \field{len}
> > > -refer to the indirect table address and length in bytes,
> > > +of large requests. The VIRTIO_F_INDIRECT_DESC and VIRTIO_F_ADMIN_VQ_INDIRECT_DESC
> > > +features allows this. To increase ring capacity the driver can store a (read-only
> > > +by the device) table of indirect descriptors anywhere in memory, and insert a
> > > +descriptor in the main virtqueue (with \field{Flags} bit VIRTQ_DESC_F_INDIRECT on)
> > > +that refers to a buffer element containing this indirect descriptor table;
> > > +\field{addr} and \field{len} refer to the indirect table address and length in bytes,
> > >   respectively.
> > >   \begin{lstlisting}
> > >   /* This means the element contains a table of descriptors. */
> > > @@ -279,10 +278,11 @@ \subsection{In-order use of descriptors}
> > >   Some devices always use descriptors in the same order in which
> > >   they have been made available. These devices can offer the
> > > -VIRTIO_F_IN_ORDER feature. If negotiated, this knowledge allows
> > > -devices to notify the use of a batch of buffers to the driver by
> > > -only writing out a single used descriptor with the Buffer ID
> > > -corresponding to the last descriptor in the batch.
> > > +VIRTIO_F_IN_ORDER and/or VIRTIO_F_ADMIN_VQ_IN_ORDER features.
> > > +If negotiated, this knowledge allows devices to notify the use of
> > > +a batch of buffers to the driver by only writing out a single used
> > > +descriptor with the Buffer ID corresponding to the last descriptor
> > > +in the batch.
> > >   The device then skips forward in the ring according to the size of
> > >   the batch. The driver needs to look up the used Buffer ID and
> > > @@ -500,8 +500,8 @@ \subsection{Event Suppression Structure Format}\label{sec:Basic
> > >   \drivernormative{\subsection}{Indirect Descriptors}{Basic Facilities of a Virtio Device / Packed Virtqueues / The Virtqueue Descriptor Table / Indirect Descriptors}
> > >   The driver MUST NOT set the DESC_F_INDIRECT flag unless the
> > > -VIRTIO_F_INDIRECT_DESC feature was negotiated.   The driver MUST NOT
> > > -set any flags except DESC_F_WRITE within an indirect descriptor.
> > > +VIRTIO_F_INDIRECT_DESC or VIRTIO_F_ADMIN_VQ_INDIRECT_DESC features were negotiated.
> > > +The driver MUST NOT set any flags except DESC_F_WRITE within an indirect descriptor.
> > >   A driver MUST NOT create a descriptor chain longer than allowed
> > >   by the device.
> > > diff --git a/split-ring.tex b/split-ring.tex
> > > index de94038..cd53840 100644
> > > --- a/split-ring.tex
> > > +++ b/split-ring.tex
> > > @@ -208,6 +208,10 @@ \subsection{The Virtqueue Descriptor Table}\label{sec:Basic Facilities of a Virt
> > >   descriptors in ring order: starting from offset 0 in the table,
> > >   and wrapping around at the end of the table.
> > > +If VIRTIO_F_ADMIN_VQ_IN_ORDER has been negotiated, driver uses
> > > +descriptors in admin virtqueue ring order: starting from offset 0 in the
> > > +table, and wrapping around at the end of the table.
> > > +
> > >   \begin{note}
> > >   The legacy \hyperref[intro:Virtio PCI Draft]{[Virtio PCI Draft]}
> > >   referred to this structure as vring_desc, and the constants as
> > > @@ -223,16 +227,18 @@ \subsection{The Virtqueue Descriptor Table}\label{sec:Basic Facilities of a Virt
> > >   Drivers MUST NOT add a descriptor chain longer than $2^{32}$ bytes in total;
> > >   this implies that loops in the descriptor chain are forbidden!
> > > -If VIRTIO_F_IN_ORDER has been negotiated, and when making a
> > > -descriptor with VRING_DESC_F_NEXT set in \field{flags} at offset
> > > -$x$ in the table available to the device, driver MUST set
> > > +If VIRTIO_F_ADMIN_VQ_IN_ORDER and/or VIRTIO_F_IN_ORDER has been negotiated,
> > > +and when making a descriptor with VRING_DESC_F_NEXT set in \field{flags} at
> > > +offset $x$ in the table available to the device, driver MUST set
> > >   \field{next} to $0$ for the last descriptor in the table
> > >   (where $x = queue\_size - 1$) and to $x + 1$ for the rest of the descriptors.
> > > +This refers to admin virtqueue descriptors and rest other virtqueues types descriptors respectively.
> > >   \subsubsection{Indirect Descriptors}\label{sec:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Descriptor Table / Indirect Descriptors}
> > >   Some devices benefit by concurrently dispatching a large number
> > > -of large requests. The VIRTIO_F_INDIRECT_DESC feature allows this (see \ref{sec:virtio-queue.h}~\nameref{sec:virtio-queue.h}). To increase
> > > +of large requests. The VIRTIO_F_INDIRECT_DESC and VIRTIO_F_ADMIN_VQ_INDIRECT_DESC
> > > +features allows this (see \ref{sec:virtio-queue.h}~\nameref{sec:virtio-queue.h}). To increase
> > >   ring capacity the driver can store a table of indirect
> > >   descriptors anywhere in memory, and insert a descriptor in main
> > >   virtqueue (with \field{flags}\&VIRTQ_DESC_F_INDIRECT on) that refers to memory buffer
> > > @@ -258,15 +264,19 @@ \subsubsection{Indirect Descriptors}\label{sec:Basic Facilities of a Virtio Devi
> > >   A single indirect descriptor
> > >   table can include both device-readable and device-writable descriptors.
> > > -If VIRTIO_F_IN_ORDER has been negotiated, indirect descriptors
> > > -use sequential indices, in-order: index 0 followed by index 1
> > > +If VIRTIO_F_IN_ORDER has been negotiated, indirect descriptors,
> > > +for non admin virtqueue, use sequential indices, in-order: index 0 followed
> > > +by index 1 followed by index 2, etc.
> > > +
> > > +If VIRTIO_F_ADMIN_VQ_IN_ORDER has been negotiated, admin virtqueue indirect
> > > +descriptors use sequential indices, in-order: index 0 followed by index 1
> > >   followed by index 2, etc.
> > >   \drivernormative{\paragraph}{Indirect Descriptors}{Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Descriptor Table / Indirect Descriptors}
> > >   The driver MUST NOT set the VIRTQ_DESC_F_INDIRECT flag unless the
> > > -VIRTIO_F_INDIRECT_DESC feature was negotiated.   The driver MUST NOT
> > > -set the VIRTQ_DESC_F_INDIRECT flag within an indirect descriptor (ie. only
> > > -one table per descriptor).
> > > +VIRTIO_F_INDIRECT_DESC and/or VIRTIO_F_ADMIN_VQ_INDIRECT_DESC features were negotiated.
> > > +The driver MUST NOT 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.
> > > @@ -274,9 +284,10 @@ \subsubsection{Indirect Descriptors}\label{sec:Basic Facilities of a Virtio Devi
> > >   A driver MUST NOT set both VIRTQ_DESC_F_INDIRECT and VIRTQ_DESC_F_NEXT
> > >   in \field{flags}.
> > > -If VIRTIO_F_IN_ORDER has been negotiated, indirect descriptors
> > > -MUST appear sequentially, with \field{next} taking the value
> > > -of 1 for the 1st descriptor, 2 for the 2nd one, etc.
> > > +If VIRTIO_F_ADMIN_VQ_IN_ORDER and/or VIRTIO_F_IN_ORDER has been negotiated,
> > > +indirect descriptors MUST appear sequentially, with \field{next} taking the
> > > +value of 1 for the 1st descriptor, 2 for the 2nd one, etc for admin virtqueue
> > > +and rest other virtqueues types respectively.
> > >   \devicenormative{\paragraph}{Indirect Descriptors}{Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Descriptor Table / Indirect Descriptors}
> > >   The device MUST ignore the write-only flag (\field{flags}\&VIRTQ_DESC_F_WRITE) in the descriptor that refers to an indirect table.
> > > -- 
> > > 2.21.0