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 05:07:51PM +0000, Parav Pandit wrote:
> 
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Tuesday, March 21, 2023 11:46 PM
> > >
> > > > 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 a forward reference.
> > 
> > because it does not say anything about the contents or the format. just some
> > kind of integer.
> > 
> Because it is an integer there is no need of a special format.
> It does say about the content very clearly = vqn / 2;
> But more below.
> 
> > > > 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_.
> Yes, rss_ prefix is good.
> 
> > 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 */
> > 	};
> Rq is usually an object and here we want to just refer to its id/vqn/handle.
> 
> Hence, I prefer rss_rq_handle {} or rss_rq_id{} for the structure name.
> WDYT?

_id is ok. But don't define it separately as a special object, it's just
the format of indirection_table and unclassified_queue.
IOW define at point of 1st use.

-- 
MST