Re: [PATCH v3 8/8] virtio-net: Describe RSS using receive queue handle

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

On Wed, Mar 22, 2023 at 02:37:48AM +0000, Parav Pandit wrote:
> 
> 
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Tuesday, March 21, 2023 6:17 PM
> 
> > > -Field \field{unclassified_queue} contains the 0-based index of -the
> > > receive virtqueue to place unclassified packets in. Index 0 corresponds to
> > receiveq1.
> > > +\begin{lstlisting}
> > > +le16 rq_handle;
> > > +\end{lstlisting}
> > > +
> > > +\field{rq_handle} is a receive virtqueue handle. It is calculated as
> > > +virtqueue number divided by two. For example a receive virtqueue
> > > +handle value of 3 corresponds to virtqueue number 6 maps to receiveq4.
> > > +
> > > +Field \field{unclassified_queue} contains the receive queue handle
> > > +\field{rq_handle} described above.
> > 
> > You dropped *which* queue this refers to.
> >
> Will add back.
>  
> > And it's still kind of complex and non-standard. E.g. what does \begin{lstlisting}
> > le16 rq_handle;
> > \end{lstlisting}
> > mean exactly? Apparently nothing ...
> > 
> Why nothing, it is referenced further down.
> Did you suggest moving before using it?
> It was just fine to provide forward reference.

because it does not say anything about the contents or the format. just some kind
of integer.

> > I feel what we keep there is really the virtqueue number itself.
> > Just stored in this strage format.
> >
> > And all this talk about handles kind of seems to add yet another term to learn.
> > Where in fact all it is, is just a different way to store vqn.
> > 
> > So my idea was this: we say something like:
> > 
> > 
> > \field{unclassified_queue} contains the virtqueue number of the receive queue
> > to place unclassified packets in.
> > \field{indirection_table} contains an array of virtqueue numbers of receive
> > queues.
> >
> Above two lines are clearly confusing where virtqueue number describe in rest of the spec and above doesn't align to same notion.

That's true.

> So better to say field A contains the rq_handle and
> 
> struct rq_handle {
> 	le16 vqn_16_1: 15;
> 	le16 reserved : 1;
> };



>  
> > Both \field{unclassified_queue} and \field{indirection_table} use the following
> > format for the virtqueue numbers:
> > \begin{lstlisting}
> > struct rss_virtqueue_number {
> It is really not any superior in term of cost of learning.
> 
> > 	le16 vqn_16_1 : 15; /* Bits 16 to 1 of the virtqueue number */
> > 	le16 reserved : 1; /* Set to 0 */
> I like the structure and reserved bit that enables to claim one bit for some unknown future use.
> > }
> > \end{lstlisting}
> > for example, a value of 3 corresponds to virtqueue number 6 and maps to
> > receiveq4.
> > 
> > 
> > 
> > and then everywhere else we just say it keeps a vq number, we already
> > explained it is using this format once no need to repeat that.
> >
> I just prefer to rename it to rq_handle ( or at least other than virtqueue number) to distinguish it from rest of the virtqueue number.

Well first of all I really want to make it clear it's specific to
RSS at least for now. So let's prefix with rss_.
Maybe I'm wrong but I feel using up a completely
new term for something very specific to RSS is a waste.
We won't be able to use handle for something else without confusion.
So how about just

	struct rss_rq {
		le16 vqn_16_1 : 15; /* Bits 16 to 1 of the virtqueue number */
		le16 reserved : 1; /* Set to 0 */
	};

hmm?



> > WDYT?