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

[Parav Pandit <parav@nvidia.com>]

On 4/3/2023 9:36 PM, Halil Pasic wrote:
> 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.
Your below description clarifies that we both are talking same.
so I will pause to comment for above point.

> 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".
There is nothing like a zero based index. A VQ can be any u16 _number_ 
in range of 0 to 65534.
Number can also start from zero. So zero based index = zero based number.

> 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.
Yes. I proposed Y = vq_notify_identifier.
(like how we well named rss_rq_id).
Y = vqn when NOTIF_DATA is not negotiated (usually default).
Y = device supplied vq_notify_identifier when NOTIF_DATA is negotiated.

We should better rename the NOTIF_DATA to NOTIF_ID too. But I park that 
proposal for later.

> 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

Once we rename the field vqn to vq_notify_id in the notification 
structure, we are out of the mess.

> 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")
vq_notify_id is a good name that takes us in sane place.

> Both solutions are equally consistent in themselves, but I
> argue the latter is better because:
> * it is more consistent with historic usage
Well index/vqn has mixed up anyway now.
For historic reason, I agree that index is right.
But it is too late now.
Comments have not come on time.

> * in my subjective opinion "queue index" is a better
> name for "X" compared to "queue number"
> * it requires fewer changes.
Unfortunately not.
Based on past agreements more patches have progress to use vqn.
It includes this patch, other patch you voted yes for vqn.
(interrupt moderation), more work that Michael did with AQ in the series 
v11.

If you sum up, changing all of it requires more 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.
I was really fine either way for index and number, one month back when I 
asked to reach consensus.

Most agreed with vqn, and engineers wrote v11 of this series, and 
multiple people reviewed.
Followed by other two patches.

I see few options.

Option_1:
1. I will send v12 of this series to use index all the places instead of 
number.
2. driver notification structure vqn to rename to vq_notify_id.
(after merging this series as follow up cleanup).
3. Once interrupt moderation series is merged, rename vqn to vq_index. 
(really vqn reads better even though history say vq index).
Because votes is completed and voting period ended for the important 
feature.
4. Michael send v12 with index.

Option_2:
1. Continue with the v11 of this series as vq number.
2. Continue with interrupt moderation series voted
3. Continue with Michael's work of AQ
4. Will do #2 of option_1.

Please decide at earliest.