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:30:33PM +0000, Parav Pandit wrote:
> 
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Monday, April 24, 2023 9:59 AM
> 
> > > >  Driver sends a VIRTIO_NET_CTRL_MQ_RSS_CONFIG command
> > 
> > maybe add:
> > 	including virtqueue index of relevant receive queues
> > 
> Structure struct virtio_net_rss_config has many fields for RSS_CONFIG command.
> It is not much help to say including receive queues, including indirection table etc.
> The structure is evident and all those fields are described in the struct including the receive virtqueue.

ok

> 
> > > >  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".
> > 
> receive virtqueue != virtqueue index.
> The receive virtqueue is description. The structure rss_rq_id is clear enough to indicate that how an receive virtqueue is communicated with the device.

the text originally was pretty clear though, the only problem
was format of index was unclear. your change me and Halil
both feel obscured things.

> > 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.
> > 
> The data type of the unclassified_queue is struct rss_rq id, so we don't need to emphasis it again.

oh, you can drop that:
	\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.

but in any case, there is no point in saying "\field{rss_rq_id} is a
receive virtqueue id." This concept of "receive virtqueue id"
is not really useful.



> For example, we don't say, max_tx_vq is in le16 format.
> 
> I am going to keep the current version as it is better than then extra verbosity.

Maybe "specifies" instead of "contains" then?

+Field \field{unclassified_queue} specifies the receive virtqueue in
+which to place unclassified packets.