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

[Halil Pasic <pasic@linux.ibm.com>]

On Thu, 30 Mar 2023 19:13:47 +0000
Parav Pandit <parav@nvidia.com> wrote:

> > From: Halil Pasic <pasic@linux.ibm.com>
> > Sent: Thursday, March 30, 2023 1:11 PM  
> 
> > 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.  
> Using single term as number is possible, so lets use single term. 
> 

I don't understand. In my opinion we should use a single term for
a particular thing, but we should avoid using the same term for
two different things.

One thing is the zero based index of the virtqueue, for that
unfortunately we currently have multiple terms: virtqueue index
and virtqueue nubmer. To remove ourselves from the index vs number
discussion let us call it "X".

Another thing is the what you below propose to call vq_identifier, but
for the same reason as above let us briefly call it Y. "Y" may hold
an "X" or a "vq_notification_data" to use your term -- we both agree
on that so, I believe we should both agree that "X" and "Y" are distinct
things an need distinct and easy to distinguish names.

What we currently have is:
* when we mean "Y" we write "virtqueue number" or "vqn", 
* and when we mean "X" we write either "virtqueue index" or "virtqueue
  number"

Now let me pull the historic argument for consolidation on "virtqueue
index" rather than "virtqueue number".

Most of the occurrences of "virtqueue number" were introduced by
commit 4ca1311 ("VIRTIO_F_NOTIFICATION_DATA: extra data to devices"
and commit 2ff0d5c ("virtio-net: Add support for the flexible driver
notification structure.")

And I argue most of them stand where we mean "Y". Some of them
don't and that is bad.

Please have a look at the v1.0 version of the specification:
http://docs.oasis-open.org/virtio/virtio/v1.0/cs04/virtio-v1.0-cs04.html#x1-370007
and search for "queue number" and "queue index". For "queue number"
you should get 1 hit and for "queue index" 8. All of them mean "X",
because "Y" was introduced later.

I argue, it is more sane to assume that the single "queue number"
occurrence is a back then quite harmless mistake (all we had is "X", so
we didn't have the "queue number" means "Y"), that to say the 8
occurrences of "queue index" are all mistakes.

Your approach to resolving the problem is: 
* change all concurrences of "queue index" (all stand for "X") to
  "queue number" 
* change the occurrences of "queue number" that stand for "Y" (that
  is most of them) to "vq_identifier"
* keep the occurrences of "queue number" that stand for "X" as is

Yes that way we can reach an in itself consistent state.

I argue that the following approach is better:
* keep all occurrences of "queue index" as is (all stand for "X")
* change he occurrences of "queue number" that stand for "X"
  to "virtqueue index"
* if we can find a name for "Y" better than "queue number"
  replace all remaining occurrences of "queue number" with
  that new name (I agree "queue number" is not a very good
  name for "Y")

Both solutions are equally consistent in themselves, but I
argue the latter is better because:
* it is more consistent with historic usage
* in my subjective opinion "queue index" is a better
name for "X" compared to "queue number"
* it requires fewer changes.

@Michael: Do you agree? Disagree?

Parav, your work is very much appreciated. I know, the messy
current state is not your fault at all. In fact it is to a certain
degree my fault. 

> > 
> > 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  
> 
> You missed "not" before negotiation. :)

Right.

Regard,
Halil

> 
> > 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.

[..]

> > @@ -413,7 +416,9 @@ \section{Driver Notifications} \label{sec:Basic Facilities
> > of a Virtio Device /  the following information:
> > 
> >  \begin{description}
> > -\item [vqn] VQ number to be notified.
> > +\item [vqn] Identifies the virtqueueue to be notified. If
> > VIRTIO_F_NOTIF_CONFIG_DATA has been
> > +      negotiateted, the value of \field{vqn} is the notify data suplied by the
> > device  
> There is still vqn here.  So no better than vq number.
> To be truly clear, it should be renamed to vq_identifier, that either contains vqn or vq_notification_data.
> 
> As I replied I can take up that cleanup at later point if we find some motivation for CONFIG_DATA use.