Re: [PATCH v14 10/11] virtio-net: Describe RSS using rss rq id

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

On Mon, Apr 24, 2023 at 03:22:21PM +0200, Halil Pasic wrote:
> On Wed, 19 Apr 2023 04:46:38 +0300
> Parav Pandit <parav@nvidia.com> wrote:
> 
> > The content of the indirection table and unclassified_queue were
> > originally described based on mathematical operations. In order to
> > make it easier to understand and to avoid intermixing the array
> > index with the vq index, introduce a structure
> > rss_rq_id (RSS receive queue
> > ID) and use it to describe the unclassified_queue and
> > indirection_table fields.
> > 
> > As part of it, have the example that uses non-zero virtqueue
> > index which helps to have better mapping between receiveX
> > object with virtqueue index and the actual value in the
> > indirection table.
> > 
> > Fixes: https://github.com/oasis-tcs/virtio-spec/issues/163
> > Signed-off-by: Parav Pandit <parav@nvidia.com>
> > 
> > ---
> > changelog:
> > v12->v13:
> > - rename vqn to vq_index
> > - renamed vq index to virtqueue index
> > v11->v12:
> > - use 'virtqueue index' instead of 'virtqueue number'
> > v10->v11:
> > - commit log updated to drop old reference to rq_handle, replaced with
> >   rss_rq_id detail
> > v8->v9:
> > - reword rss_rq_id and unclassified_packets description
> > - use article
> > - use 'vq number' instead of 'virtqueue number'
> > v4->v5:
> > - change subject to reflect rss_rq_id
> > - fixed accidental removal of "unclassifed packets".
> > - simplfied text around indirection_table mask to use the term
> >   destination receive virtqueue. This aligns with next line about queue
> >   reset.
> > - removed rss_rq_id references as indirection table and
> >   unclassified_queue data type is self explanatory
> > v3->v4:
> > - renamed rq_handle to rss_rq_id
> > - moved rss_rq_id definition close to its usage in rss_config struct
> > v2->v3:
> > - moved rq_handle definition before using it
> > - removed duplicate example as rq_handle is now described first
> > v0->v1:
> > - new patch suggested by Michael Tsirkin
> > ---
> >  device-types/net/description.tex | 27 ++++++++++++++++++++-------
> >  1 file changed, 20 insertions(+), 7 deletions(-)
> > 
> > diff --git a/device-types/net/description.tex b/device-types/net/description.tex
> > index f3f9f1d..e4d236e 100644
> > --- a/device-types/net/description.tex
> > +++ b/device-types/net/description.tex
> > @@ -1434,11 +1434,16 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
> >  
> >  Driver sends a VIRTIO_NET_CTRL_MQ_RSS_CONFIG command

maybe add:
	including virtqueue index of relevant receive queues

> using the following format for \field{command-specific-data}:
> >  \begin{lstlisting}
> > +struct rss_rq_id {
> > +   le16 vq_index_1_16: 15; /* Bits 1 to 16 of the virtqueue index */
> > +   le16 reserved: 1; /* Set to zero */
> > +};
> > +
> 
> I'm not a fan of this being done with bitmaps, as I have stated before,
> but I can live with it.

Same.

> >  struct virtio_net_rss_config {
> >      le32 hash_types;
> >      le16 indirection_table_mask;
> > -    le16 unclassified_queue;
> > -    le16 indirection_table[indirection_table_length];
> > +    struct rss_rq_id unclassified_queue;
> > +    struct rss_rq_id indirection_table[indirection_table_length];
> >      le16 max_tx_vq;
> >      u8 hash_key_length;
> >      u8 hash_key_data[hash_key_length];
> > @@ -1453,10 +1458,15 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
> >  \field{indirection_table} array.
> >  Number of entries in \field{indirection_table} is (\field{indirection_table_mask} + 1).
> >  
> > -Field \field{unclassified_queue} contains the 0-based index of
> > -the receive virtqueue to place unclassified packets in. Index 0 corresponds to receiveq1.
> > +\field{rss_rq_id} is a receive virtqueue id. \field{vq_index_1_16}
> > +consists of bits 1 to 16 of a virtqueue index. For example, a
> > +\field{vq_index_1_16} value of 3 corresponds to virtqueue index 6,
> > +which maps to receiveq4.
> > +
> > +Field \field{unclassified_queue} contains the receive virtqueue
> > +in which to place unclassified packets.
> 
> It does not contain a receive virtqueue but its \field{rss_rq_id},
> i.e. it's "receive virtqueue id".

Yes all this last chunk is not an improvement.  
My whole point was that we do not need a name for this
thing that is struct rss_rq_id. It's just a weird way to store a vq
index.

Also \field{rss_rq_id} is confusing since it's actually
\field{struct rss_rq_id}. We are using C-like not C++ like syntax ;)

One way to address all this:

\field{struct rss_rq_id} contains a virtqueue index: \field{vq_index_1_16}
consists of bits 1 to 16 of a virtqueue index. For example, a
\field{vq_index_1_16} value of 3 corresponds to virtqueue index 6,
which maps to receiveq4.

Field \field{unclassified_queue} contains the virtqueue index
of the receive virtqueue to place unclassified packets in,
in \field{struct rss_rq_id} format.





> > +\field{vq_index_1_16} value of 3 corresponds to virtqueue index 6,
> > +which maps to receiveq4.
> > +
> > +Field \field{unclassified_queue} contains the receive virtqueue
> > +in which to place unclassified packets.




> >  
> > -Field \field{indirection_table} contains an array of 0-based indices of receive virtqueues. Index 0 corresponds to receiveq1.
> > +Field \field{indirection_table} is an array of receive virtqueues.
> 
> Same here.
> 
> >  
> >  A driver sets \field{max_tx_vq} to inform a device how many transmit virtqueues it may use (transmitq1\ldots transmitq \field{max_tx_vq}).
> >  
> > @@ -1466,7 +1476,8 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
> >  
> >  A driver MUST NOT send the VIRTIO_NET_CTRL_MQ_RSS_CONFIG command if the feature VIRTIO_NET_F_RSS has not been negotiated.
> >  
> > -A driver MUST fill the \field{indirection_table} array only with indices of enabled queues. Index 0 corresponds to receiveq1.
> > +A driver MUST fill the \field{indirection_table} array only with
> > +enabled receive virtqueues.
> 
> Same here.
> >  
> >  The number of entries in \field{indirection_table} (\field{indirection_table_mask} + 1) MUST be a power of two.
> >  
> > @@ -1479,7 +1490,9 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
> >  \begin{itemize}
> >  \item Calculate the hash of the packet as defined in \ref{sec:Device Types / Network Device / Device Operation / Processing of Incoming Packets / Hash calculation for incoming packets}.
> >  \item If the device did not calculate the hash for the specific packet, the device directs the packet to the receiveq specified by \field{unclassified_queue} of virtio_net_rss_config structure.
> > -\item Apply \field{indirection_table_mask} to the calculated hash and use the result as the index in the indirection table to get 0-based number of destination receiveq.
> > +\item Apply \field{indirection_table_mask} to the calculated hash
> > +and use the result as the index in the indirection table to get
> > +the destination receive virtqueue.
> 
> Same here.
> 
> >  \item If the destination receive queue is being reset (See \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Reset}), the device MUST drop the packet.
> >  \end{itemize}
> >