Re: [virtio-dev] [PATCH v10 0/8] Rename queue index to queue number

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

On Fri, Mar 31, 2023 at 10:13:51AM +0200, Cornelia Huck wrote:
> On Thu, Mar 30 2023, Halil Pasic <pasic@linux.ibm.com> wrote:
> 
> > On Thu, 30 Mar 2023 00:23:33 +0300
> > Parav Pandit <parav@nvidia.com> wrote:
> >
> >> 1. Currently, virtqueue is identified between driver and device
> >> interchangeably using either number or index terminology.
> >> 
> >> 2. Between PCI and MMIO transport the queue size (depth) is
> >> defined as queue_size and QueueNum respectively.
> >> 
> >> To avoid confusion and to have consistency, unify them to use Number.
> >> 
> >> Solution:
> >> a. Use virtqueue number description, and rename MMIO register as QueueSize.
> >
> > I'm in favor of replacing number with size where appropriate.
> >
> >> b. Replace virtqueue index with virtqueue number
> >
> > I don't see the benefit of replacing virtqueue index with virtqueue
> > number.
> >
> > Currently virtqueue number is only used in the parts that describe
> > notifications (Guest->Host), the rest of the spec uses virtqueue index.
> >
> > I argue that using a different term in that context than in the rest
> > of the specification makes sense, because in the context of notifications
> > the virtqueue isn't always identified by its index.
> >
> > More precisely: if VIRTIO_F_NOTIF_CONFIG_DATA has been negotiated in the
> > context of notifications the virtqueue is identified by the
> > so called "queue_notify_data"; if VIRTIO_F_NOTIF_CONFIG_DATA has been
> > negotiated in the context of notifications the virtqueue is identified by
> > the virtqueue index (as usual, for example in queue_select, or in
> > the ccws).
> >
> > As I've pointed out in my comment to patch 2, I believe replacing
> > virtqueue index with virtqueue number is detrimental to clarity.
> >
> > Thus please find a counter-proposal below. If there is interest
> > I can make a series out of it, and prettify it. If I can't convince
> > you guys, then I will have to get used to vqn and virtqueue number.
> 
> I would generally prefer "index" as well, but there seemed to be a
> strong sentiment that we should go with "number"... so, what *is* the
> actual general sentiment? It's hard to say, but maybe most people are
> fine with either?

If we really can't decide one way or another then I can run a ballot,
it's not hard.


> >
> > AFAIR the other problem with index was the RSS for virtio-net. But there
> > we are currently heading down a direction of introducing a new
> > abstraction. This approach avoids confusion around the term 'virtqueue
> > index' as much as it avoids confusion around the term 'virtqueue nuber'.
> >
> >
> >> c. RSS area of virtio net has inherited some logic, describe it
> >> using abstract rss_rq_id.
> >
> > -------------------------8<--------------------------------------
> > From: Halil Pasic <pasic@linux.ibm.com>
> > Date: Thu, 30 Mar 2023 17:57:53 +0200
> > Subject: [PATCH 1/1] content: clarify how virtques are identified
> >
> > Clarify how virtqueues are identified in the context of
> > available notifications and in the context of RSS for
> > virtio-net .
> >
> > Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
> > ---
> >  content.tex                      | 15 ++++++++++-----
> >  device-types/net/description.tex | 30 ++++++++++++++++++++++--------
> >  transport-ccw.tex                |  2 +-
> >  transport-pci.tex                |  7 ++++---
> >  4 files changed, 37 insertions(+), 17 deletions(-)
> 
> (...)
> 
> > +struct rss_rq_id {
> > +   le16 value; /* virtqueue index divided by two */
> > +};
> > +
> >  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];
> >  };
> >  \end{lstlisting}
> > +
> > +The type struct rss\_rq\_id is introduced to better distinguish receive queue
> > +ids form other integral fields.
> > +
> > +A receive queue id is only defined for receive queues, as the virtqueue index
> > +of the receive virtqueue divided by two (the virtqueue index of a receive
> > +queue is always even). For example receiveq4 is identified by the virtqueue
> > +index 6 and the receive queue id 3.
> 
> FWIW, I think this is much easier to understand.