Rename queue index to queue number

Introduce vq number and its range so that subsequent patches can refer to it.

Fixes https://github.com/oasis-tcs/virtio-spec/issues/163

Signed-off-by Parav Pandit <[email protected]>

---
changelog:
v9->v10:
- added braces around vq number wording
- added vqn as another term for vq number
v8->v9:
- added inclusive when describing the vq number range
- skipped comment to put virtqueue number wording first because we prefer to use shorter vq number as much as possible
v5->v6:
- moved description close to introduction, it was in middle of queue data transfer description
v2->v3:
- new patch

---
 content.tex | 4 ++++
 1 file changed, 4 insertions(+)

Currently specification uses virtqueue index and number interchangeably to refer to the virtqueue.

Instead refer to it by its number.

Fixes https://github.com/oasis-tcs/virtio-spec/issues/163

Reviewed-by Cornelia Huck <[email protected]>

Reviewed-by Max Gurtovoy <[email protected]>

Reviewed-by Jiri Pirko <[email protected]>

Signed-off-by Parav Pandit <[email protected]>

---

From: Halil Pasic <[email protected]>
Sent: Thursday, March 30, 2023 1:10 PM

transport-pci.tex \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport · line 393

Quoted 2 lines

transport-pci.tex +1 -1

-        The driver will use this value to put it in the 'virtqueue number' field
+        The driver uses this value in the field \field{vqn}

This is one of the problems with this approach...

Quoted 2 lines

in the available buffer notification structure.
See section \ref{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI-

specific Initialization And Device Operation / Available Buffer Notifications}.

Quoted 2 lines

\begin{note}
This field provides the device with flexibility to determine how

virtqueues

Quoted 2 lines

will be referred to in available buffer notifications.
- In a trivial case the device can set \field{queue_notify_data}=vqn. Some

devices

transport-pci.tex \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport · line 400

Quoted 2 lines

transport-pci.tex +2 -0

+        In a trivial case the device can set
+        \field{queue_notify_data} to the vq number. Some devices

... the 'vq' number here is not the same vqn above which renders the usage of vqn ambiguous.

Quoted 2 lines

(quoted diff) +0 -0

@@ -1053,7 +1054,7 @@ \subsubsection{Available Buffer

Notifications}\label{sec:Virtio Transport Option If

VIRTIO_F_NOTIF_CONFIG_DATA has been negotiated:

Quoted 3 lines

\begin{itemize}
\item If VIRTIO_F_NOTIFICATION_DATA has not been negotiated, the
driver MUST use the -\field{queue_notify_data} value instead of the virtqueue

index.

Quoted 3 lines

+\field{queue_notify_data} value instead of the vq number.
\item If VIRTIO_F_NOTIFICATION_DATA has been negotiated, the driver
MUST set the \field{vqn} field to the \field{queue_notify_data} value.

And here it is really obvious: if VIRTIO_F_NOTIF_CONFIG_DATA has been negotiated the field \field{vqn} does not hold a virtqueue number/vq nuber/vqn but a device supplied identifier which may or may not be same as the vqn.

So we went
from:
if !VIRTIO_F_NOTIF_CONFIG_DATA then vqn is the virtqueue index if !!VIRTIO_F_NOTIF_CONFIG_DATA then vqn is queue_notify_data which may or may not be the same as the virtqueue index to if !VIRTIO_F_NOTIF_CONFIG_DATA then vqn is the vq number if !!VIRTIO_F_NOTIF_CONFIG_DATA then vqn is queue_notify_data which may or may not be the same as the vq number.

Which is my opinion less clear that what we had before. And in my opinion calling the very same thing virtqueue und vq number interchangeably is not helpful either -- makes grepping harder.

I don't see the benefit of replacing virtqueue index with virtqueue number. AFAIR the supposed benefit was to:
a) harmonize the terminology in the notifications part with the rest of the spec
b) to resolve the RSS problematic with its receive virtqueue indexes.

For b) we are going down a different route calling those receive queue ids (AFAIR), and for the notifications part see my comments above.

I'm about to reply to the cover letter, and make my argument against standardizing virtqueue nuber (and in favor of standardizing on the on virtqueue index) along with a diff that is supposed to act as a counter proposal. If that doesn't work I will give up and make peace with vq number and vqn.

Currently specification uses virtqueue index and
number interchangeably to refer to the virtqueue.

Instead refer to it by its number.

Fixes https://github.com/oasis-tcs/virtio-spec/issues/163

Signed-off-by Parav Pandit <[email protected]>

---
changelog:
v9->v10:
- replaced queue_select with vqn
- used lower case letter for first word in comment v8->v9:
- replaced 'named' with 'known'
- replaced 'queue number' with 'vq number'
v3->v4:
- moved note to comment
v2->v3:
- added comment note for queue_select similar to max_queue_size v0->v1:
- new patch

---
 transport-ccw.tex | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

Receive queue number/index example is duplicate which is already defined in the Setting RSS parameters section.

Hence, avoid such duplicate example and prepare it for the subsequent patch to describe using receive queue handle.

Fixes https://github.com/oasis-tcs/virtio-spec/issues/163

Signed-off-by Parav Pandit <[email protected]>

---
 device-types/net/description.tex | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

device-types/net/description.tex +0 -0

index d7c8b1b..435c1fc 100644

The content of indirection table and unclassified_queue which are based on math calculation historically. To better describe this, to avoid intermixing array index with virtqueue index and to use virtqueue number

introduce a field rq_handle (receive queue handle) and refer them to describe unclassified_queue and indirection_table fields.

As part of it, have the example that uses non zero virtqueue

number which helps to have better mapping between receiveX object with virtqueue number and the actual value in the indirection table.

Fixes https://github.com/oasis-tcs/virtio-spec/issues/163

Signed-off-by Parav Pandit <[email protected]>

---
changelog:
v8->v9:
- reword rss_rq_id and unclassified_packets description
- use article
- use 'vq number' instead of 'virtqueue number'
v4->v5:
- change subject to reflect rss_rq_id
- fixed accidental removal of "unclassifed packets".
- simplfied text around indirection_table mask to use the term destination receive virtqueue. This aligns with next line about queue reset.
- removed rss_rq_id references as indirection table and unclassified_queue data type is self explanatory
v3->v4:
- renamed rq_handle to rss_rq_id
- moved rss_rq_id definition close to its usage in rss_config struct v2->v3:
- moved rq_handle definition before using it
- removed duplicate example as rq_handle is now described first v0->v1:
- new patch suggested by Michael Tsirkin

---
 device-types/net/description.tex | 27 ++++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)

device-types/net/description.tex +7 -4

index 435c1fc..c64335d 100644
@@ -1423,11 +1423,16 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
 
 Driver sends a VIRTIO_NET_CTRL_MQ_RSS_CONFIG command using the following format for \field{command-specific-data}:
 \begin{lstlisting}
+struct rss_rq_id {
+   le16 vqn_1_16: 15; /* Bits 1 to 16 of the vq number */
+   le16 reserved: 1; /* Set to zero */
+};
+
 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];

@@ -1442,10 +1447,15 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
 \field{indirection_table} array.
 Number of entries in \field{indirection_table} is (\field{indirection_table_mask} + 1).
 
-Field \field{unclassified_queue} contains the 0-based index of
-the receive virtqueue to place unclassified packets in. Index 0 corresponds to receiveq1.

From: Cornelia Huck <[email protected]>
Sent: Thursday, March 30, 2023 5:17 AM

Quoted 7 lines · Parav Pandit

The content of indirection table and unclassified_queue which are based on math calculation historically. To better describe this, to avoid intermixing array index with virtqueue index and to use virtqueue number

introduce a field rq_handle (receive queue handle) and refer them to describe unclassified_queue and indirection_table fields.

This description is a bit confusing (and you renamed rq_handle as well.) Does

"The content of the indirection table and unclassified_queue were originally described based on mathematical operations. In order to make it easier to understand and to avoid intermixing the array index with the virtqueue number, introduce a structure rss_rq_id (RSS receive queue
ID) and use it to describe the unclassified_queue and indirection_table fields."

capture the intent correctly?

Quoted 2 lines

As part of it, have the example that uses non zero virtqueue

"have the example use a non-zero..." ?

Quoted 2 lines

-Field \field{unclassified_queue} contains the 0-based index of -the receive virtqueue to place unclassified packets in. Index 0 corresponds to

receiveq1.

Quoted 8 lines

+\field{rss_rq_id} is a receive virtqueue id. \field{vqn_1_16}

(quoted diff) +6 -0

+consists of bits 1 to 16 of a vq number. For example, a
+\field{vqn_1_16} value of 3 corresponds to vq number 6, which maps to
+receiveq4.
+
+Field \field{unclassified_queue} contains the receive virtqueue in
+which to place unclassified packets.

Quoted 1 line

-Field \field{indirection_table} contains an array of 0-based indices of receive

virtqueues. Index 0 corresponds to receiveq1.

Quoted 1 line

+Field \field{indirection_table} is an array of receive virtqueues.

"an array of receive virtqueues identified via their rss_rq_id" ?

Quoted 3 lines

A driver sets \field{max_tx_vq} to inform a device how many transmit

virtqueues it may use (transmitq1\ldots transmitq \field{max_tx_vq}).

Quoted 5 lines

(quoted diff) +0 -0

@@ -1455,7 +1465,8 @@ \subsubsection{Control

Virtqueue}\label{sec:Device Types / Network Device / Devi

A driver MUST NOT send the VIRTIO_NET_CTRL_MQ_RSS_CONFIG command

if the feature VIRTIO_NET_F_RSS has not been negotiated.

Quoted 1 line

Quoted 1 line

-A driver MUST fill the \field{indirection_table} array only with indices of

enabled queues. Index 0 corresponds to receiveq1.

device-types/net/description.tex \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi · line 1469

Quoted 2 lines

device-types/net/description.tex +2 -0

+A driver MUST fill the \field{indirection_table} array only with
+enabled receive virtqueues.

"only with rss_rq_id references to enabled receive virtqueues" ?

Quoted 2 lines

The number of entries in \field{indirection_table}

(\field{indirection_table_mask} + 1) MUST be a power of two.

Quoted 4 lines

(quoted diff) +0 -0

@@ -1468,7 +1479,9 @@ \subsubsection{Control

Virtqueue}\label{sec:Device Types / Network Device / Devi \begin{itemize} \item Calculate the hash of the packet as defined in

\ref{sec:Device Types / Network Device / Device Operation / Processing of Incoming Packets / Hash calculation for incoming packets}.

Quoted 1 line

\item If the device did not calculate the hash for the specific packet, the

device directs the packet to the receiveq specified by \field{unclassified_queue} of virtio_net_rss_config structure.

Quoted 1 line

-\item Apply \field{indirection_table_mask} to the calculated hash and use

the result as the index in the indirection table to get 0-based number of destination receiveq.

Review

Quoted 3 lines

+\item Apply \field{indirection_table_mask} to the calculated hash and

(quoted diff) +2 -0

+use the result as the index in the indirection table to get the
+destination receive virtqueue.

Ok, you remove the line here anyway, so please just ignore my suggestion for the previous patch.

Quoted 1 line

\item If the destination receive queue is being reset (See \ref{sec:Basic

Facilities of a Virtio Device / Virtqueues / Virtqueue Reset}), the device MUST drop the packet.

Quoted 1 line

\end{itemize}

Quoted 5 lines

From: Cornelia Huck <[email protected]>
Sent: Thursday, March 30, 2023 5:17 AM

Quoted 1 line

+Field \field{indirection_table} is an array of receive virtqueues.

"an array of receive virtqueues identified via their rss_rq_id" ?

No need to make it this verbose. It is evident from the definition itself.

Quoted 12 lines

Quoted 3 lines

A driver sets \field{max_tx_vq} to inform a device how many transmit

virtqueues it may use (transmitq1\ldots transmitq \field{max_tx_vq}).

Quoted 5 lines

(quoted diff) +0 -0

@@ -1455,7 +1465,8 @@ \subsubsection{Control

Virtqueue}\label{sec:Device Types / Network Device / Devi

A driver MUST NOT send the VIRTIO_NET_CTRL_MQ_RSS_CONFIG command

if the feature VIRTIO_NET_F_RSS has not been negotiated.

Quoted 2 lines

-A driver MUST fill the \field{indirection_table} array only with indices of

Quoted 6 lines

enabled queues. Index 0 corresponds to receiveq1.

device-types/net/description.tex \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi · line 1469

Quoted 2 lines

device-types/net/description.tex +2 -0

+A driver MUST fill the \field{indirection_table} array only with
+enabled receive virtqueues.

"only with rss_rq_id references to enabled receive virtqueues" ?

In this field and other fields, we just refer to the receive virtqueues as rss_rq_id parent structure itself is describing what it is. Hence, we omit re-iterating rss_rq_id at multiple places.

From: Cornelia Huck <[email protected]>
Sent: Tuesday, April 4, 2023 4:15 AM

Quoted 1 line

In this field and other fields, we just refer to the receive virtqueues as

rss_rq_id parent structure itself is describing what it is.

Quoted 1 line

Hence, we omit re-iterating rss_rq_id at multiple places.

Personally, I'd prefer to spell it out, as it is a specific way to refer to a virtqueue.

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.

b. Replace virtqueue index with virtqueue number

c. RSS area of virtio net has inherited some logic, describe it using abstract rss_rq_id.

From: Halil Pasic <[email protected]>
Sent: Thursday, March 30, 2023 1:11 PM

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.

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

Quoted 2 lines

c. RSS area of virtio net has inherited some logic, describe it using abstract rss_rq_id.

-------------------------8<--------------------------------------
From: Halil Pasic <[email protected]>
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 <[email protected]>

---
 content.tex                      | 15 ++++++++++-----
 device-types/net/description.tex | 30 ++++++++++++++++++++++--------
 transport-ccw.tex                |  2 +-
 transport-pci.tex                |  7 ++++---
 4 files changed, 37 insertions(+), 17 deletions(-)

content.tex +0 -0

index cff548a..a376232 100644
@@ -298,6 +298,9 @@ \section{Virtqueues}\label{sec:Basic Facilities of a

Virtio Device / Virtqueues} virtqueues\footnote{For example, the simplest network device has one virtqueue for transmit and one for receive.}.

Driver makes requests available to device by adding an available buffer to the
queue, i.e., adding a buffer describing the request to a virtqueue, and optionally triggering @@ -402,7 +405,7 @@ \section{Driver Notifications} \label{sec:Basic Facilities of a Virtio Device /

When VIRTIO_F_NOTIFICATION_DATA has not been negotiated, this
notification involves sending the -virtqueue number to the device (method depending on the transport).
+virtqueue index to the device (method depending on the transport).

However, some devices benefit from the ability to find out the amount of
available data in the queue without accessing the virtqueue in memory:

content.tex +0 -0

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

content.tex +1 -0

+      (by transport specific means). Otherwise it is the virtqueue index.
 \item [next_off] Offset
       within the ring where the next available ring entry
       will be written.

@@ -786,13 +791,13 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved
 Feature Bits}
   buffer notifications.
   As mentioned in section \ref{sec:Basic Facilities of a Virtio Device / Driver

notifications}, when the
driver is required to send an available buffer notification to the device, it

content.tex +2 -1

-  sends the virtqueue number to be notified. The method of delivering
+  sends the virtqueue index of the virtqueue to be notified. The method
+ of delivering
   notifications is transport specific.
   With the PCI transport, the device can optionally provide a per-virtqueue

value

content.tex +2 -2

-  for the driver to use in driver notifications, instead of the virtqueue number.
+  for the driver to use in driver notifications, instead of the virtqueue index.
   Some devices may benefit from this flexibility by providing, for example,
   an internal virtqueue identifier, or an internal offset related to the
-  virtqueue number.
+  virtqueue index.
 
   This feature indicates the availability of such value. The definition of the
   data to be provided in driver notification and the delivery method is diff --git

a/device-types/net/description.tex b/device-types/net/description.tex index d7c8b1b..7678a57 100644

device-types/net/description.tex +0 -0

@@ -1421,18 +1421,33 @@ \subsubsection{Control

Virtqueue}\label{sec:Device Types / Network Device / Devi

\subparagraph{Setting RSS parameters}\label{sec:Device Types / Network
Device / Device Operation / Control Virtqueue / Receive-side scaling (RSS) / Setting RSS parameters}

+
Driver sends a VIRTIO_NET_CTRL_MQ_RSS_CONFIG command using the
following format for \field{command-specific-data}:
\begin{lstlisting}

 struct virtio_net_rss_config {
     le32 hash_types;
     le16 indirection_table_mask;

device-types/net/description.tex +11 -2

-    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.
+
 Field \field{hash_types} contains a bitmask of allowed hash types as  defined in

\ref{sec:Device Types / Network Device / Device Operation / Processing of Incoming Packets / Hash calculation for incoming packets / Supported/enabled hash types}.

device-types/net/description.tex +0 -0

@@ -1442,10 +1457,9 @@ \subsubsection{Control Virtqueue}\label{sec:Device

Types / Network Device / Devi \field{indirection_table} array.
Number of entries in \field{indirection_table} is
(\field{indirection_table_mask} + 1).

-Field \field{unclassified_queue} contains the 0-based index of -the receive virtqueue to place unclassified packets in. Index 0 corresponds to receiveq1. +Field \field{unclassified_queue} contains the receive queue id of the receive virtqueue to place unclassified packets in.

-Field \field{indirection_table} contains an array of 0-based indices of receive virtqueues. Index 0 corresponds to receiveq1.
+Field \field{indirection_table} contains an array of 0-based indices of receive queue ids.

A driver sets \field{max_tx_vq} to inform a device how many transmit
virtqueues it may use (transmitq1\ldots transmitq \field{max_tx_vq}).

device-types/net/description.tex +0 -0

@@ -1455,7 +1469,7 @@ \subsubsection{Control Virtqueue}\label{sec:Device

Types / Network Device / Devi

A driver MUST NOT send the VIRTIO_NET_CTRL_MQ_RSS_CONFIG command
if the feature VIRTIO_NET_F_RSS has not been negotiated.

-A driver MUST fill the \field{indirection_table} array only with indices of enabled queues. Index 0 corresponds to receiveq1.
+A driver MUST fill the \field{indirection_table} array only with receive queue ids of enabled queues.

The number of entries in \field{indirection_table}
(\field{indirection_table_mask} + 1) MUST be a power of two.

device-types/net/description.tex +0 -0

@@ -1467,8 +1481,8 @@ \subsubsection{Control Virtqueue}\label{sec:Device

Types / Network Device / Devi The device MUST determine the destination queue for a network packet as follows:
\begin{itemize}
\item Calculate the hash of the packet as defined in \ref{sec:Device Types /
Network Device / Device Operation / Processing of Incoming Packets / Hash calculation for incoming packets}.
-\item If the device did not calculate the hash for the specific packet, the device directs the packet to the receiveq specified by \field{unclassified_queue} of virtio_net_rss_config structure (value of 0 corresponds to receiveq1). -\item Apply \field{indirection_table_mask} to the calculated hash and use the result as the index in the indirection table to get 0-based number of destination receiveq (value of 0 corresponds to receiveq1).
+\item If the device did not calculate the hash for the specific packet, the device directs the packet to the receiveq specified by \field{unclassified_queue} of virtio_net_rss_config structure.
+\item Apply \field{indirection_table_mask} to the calculated hash and use the result as the index in the indirection table to receive queue id of the destination receiveq.
\item If the destination receive queue is being reset (See \ref{sec:Basic
Facilities of a Virtio Device / Virtqueues / Virtqueue Reset}), the device MUST drop the packet.
\end{itemize}

transport-ccw.tex index c492cb9..bc05639 renamed from transport-ccw.tex +0 -0

100644

transport-ccw.tex +0 -0

@@ -544,7 +544,7 @@ \subsubsection{Guest->Host
 Notification}\label{sec:Virtio Transport Options / Vi  \end{tabular}
 
 When VIRTIO_F_NOTIFICATION_DATA has not been negotiated, -the

\field{Notification data} contains the Virtqueue number. +the \field{Notification data} contains the virtque index.

When VIRTIO_F_NOTIFICATION_DATA has been negotiated, the value has the
following format:

transport-pci.tex +0 -0

@@ -390,15 +390,16 @@ \subsubsection{Common configuration structure

layout}\label{sec:Virtio Transport

\item[\field{queue_notify_data}]
This field exists only if VIRTIO_F_NOTIF_CONFIG_DATA has been
negotiated.

transport-pci.tex +2 -1

-        The driver will use this value to put it in the 'virtqueue number' field
+        The driver will use this value to put it in the \field{vqn}
+ field
         in the available buffer notification structure.
         See section \ref{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI-

specific Initialization And Device Operation / Available Buffer Notifications}.
\begin{note}
This field provides the device with flexibility to determine how virtqueues will be referred to in available buffer notifications.
- In a trivial case the device can set \field{queue_notify_data}=vqn. Some
devices
+ In a trivial case the device can set \field{queue_notify_data} to the
virtqueue index,
+ that is the value in \field{queue\_select}. Some devices
may benefit from providing another value, for example an internal
virtqueue

transport-pci.tex +1 -2

-        identifier, or an internal offset related to the virtqueue number.
+        identifier, or an internal offset related to the virtqueue index.
         \end{note}
 
 \item[\field{queue_reset}]
--

2.31.1

Quoted 9 lines

From: Halil Pasic <[email protected]>
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.

Quoted 5 lines

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

Quoted 6 lines

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.

Quoted 9 lines

(quoted diff) +0 -0

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

Quoted 9 lines · On Thu, 30 Mar 2023 19:13:47 +0000 Parav Pandit <[email protected]> wrote:

Quoted 9 lines

From: Halil Pasic <[email protected]>
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.

Quoted 2 lines

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.

Quoted 3 lines

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.

Quoted 7 lines · Parav Pandit

Quoted 3 lines

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.

Why, is there a deadline? Is this blocking some other feature? For something that is supposedly a cleanup we might as well get this right. If you guys both agree index is better, and at this point it sounds convincing, why not do it? It's more or less a machanical replacement. It's not like there's a lot of word smithing going on here except for 8/8 which really works with index just as well.

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.

Quoted 4 lines

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.

interrupt moderation voting is still ongoing, isn't it?

Quoted 5 lines · On Tue, 4 Apr 2023 03:07:26 -0400 "Michael S. Tsirkin" <[email protected]> wrote:

Quoted 4 lines

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.

Quoted 1 line

interrupt moderation voting is still ongoing, isn't it?

Yes it is. But not for very long. Close date is today. I won't change my vote because of the index vs number discussion. I believe we can include that patch in the cleanup if push comes to shove. But I wouldn't have anything against that ballot getting withdrawn either. I'm Cc-ing Heng Qi.

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.

Quoted 10 lines · On Thu, 30 Mar 2023 00:23:33 +0300 Parav Pandit <[email protected]> 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.

Quoted 1 line

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.

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

Quoted 2 lines

c. RSS area of virtio net has inherited some logic, describe it using abstract rss_rq_id.

-------------------------8<--------------------------------------
From: Halil Pasic <[email protected]>
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 <[email protected]>

---
 content.tex                      | 15 ++++++++++-----
 device-types/net/description.tex | 30 ++++++++++++++++++++++--------
 transport-ccw.tex                |  2 +-
 transport-pci.tex                |  7 ++++---
 4 files changed, 37 insertions(+), 17 deletions(-)

device-types/net/description.tex +6 -2

+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];

Quoted 38 lines · Halil Pasic

Quoted 10 lines · On Thu, 30 Mar 2023 00:23:33 +0300 Parav Pandit <[email protected]> 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.

Quoted 1 line

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.

Quoted 3 lines

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?

Quoted 26 lines

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

Quoted 2 lines

c. RSS area of virtio net has inherited some logic, describe it using abstract rss_rq_id.

-------------------------8<--------------------------------------
From: Halil Pasic <[email protected]>
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 <[email protected]>

---
 content.tex                      | 15 ++++++++++-----
 device-types/net/description.tex | 30 ++++++++++++++++++++++--------
 transport-ccw.tex                |  2 +-
 transport-pci.tex                |  7 ++++---
 4 files changed, 37 insertions(+), 17 deletions(-)

(...)

Quoted 18 lines

device-types/net/description.tex +8 -2

+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

device-types/net/description.tex \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi · line 1449

Quoted 6 lines

device-types/net/description.tex +6 -0

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