[PATCH 0/3] Rename queue index to queue number

[PATCH 0/3] Rename queue index to queue number

[Parav Pandit]

1. Currently, virtqueue is identified between driver and device
interchangeably using either number of 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 as Number.

Solution:
Use virtqueue number description, and rename MMIO register as QueueSize.

Patch summary:
patch-1 renames index to number for pci transport
patch-2 renames mmio register from Num to Size
patch-3 renames index to number for mmio transport

Please review.
This series fixes the issue [1].

This series is on top of [2].

[1] https://github.com/oasis-tcs/virtio-spec/issues/163
[2] https://lists.oasis-open.org/archives/virtio-dev/202302/msg00527.html

---
Cornelia:
I was not sure about ccw for vq_config_block and vq_info_block structures
index field refers to the queue number or not.
Can you please clarify?

If it vqn, I will send v1 by replacing index to vqn to be
consistent with other part of the spec which also uses vqn.

Parav Pandit (3):
  transport-pci: Refer to the vq by its number
  transport-mmio: Rename QueueNum register
  transport-mmio: Refer to the vq by its number

 transport-mmio.tex | 16 ++++++++--------
 transport-pci.tex  |  6 +++---
 2 files changed, 11 insertions(+), 11 deletions(-)

-- 
2.26.2

[PATCH 1/3] transport-pci: Refer to the vq by its number

[Parav Pandit]

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

Instead refer to it by its number.

This patch is on top of [1].

[1] https://lists.oasis-open.org/archives/virtio-dev/202302/msg00527.html

Fixes: https://github.com/oasis-tcs/virtio-spec/issues/163
Signed-off-by: Parav Pandit <parav@nvidia.com>
---
 transport-pci.tex | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/transport-pci.tex b/transport-pci.tex
index da1486a..c9e112a 100644
--- a/transport-pci.tex
+++ b/transport-pci.tex
@@ -1005,7 +1005,7 @@ \subsubsection{Device Initialization}\label{sec:Virtio Transport Options / Virti
 The driver typically does this as follows, for each virtqueue a device has:
 
 \begin{enumerate}
-\item Write the virtqueue index (first queue is 0) to \field{queue_select}.
+\item Write the virtqueue number (first queue is 0) to \field{queue_select}.
 
 \item Read the virtqueue size from \field{queue_size}. This controls how big the virtqueue is
   (see \ref{sec:Basic Facilities of a Virtio Device / Virtqueues}~\nameref{sec:Basic Facilities of a Virtio Device / Virtqueues}). If this field is 0, the virtqueue does not exist.
@@ -1035,7 +1035,7 @@ \subsubsection{Available Buffer Notifications}\label{sec:Virtio Transport Option
 
 When VIRTIO_F_NOTIFICATION_DATA has not been negotiated,
 the driver sends an available buffer notification to the device by writing
-the 16-bit virtqueue index
+the 16-bit virtqueue number
 of this virtqueue to the Queue Notify address.
 
 When VIRTIO_F_NOTIFICATION_DATA has been negotiated,
@@ -1053,7 +1053,7 @@ \subsubsection{Available Buffer Notifications}\label{sec:Virtio Transport Option
 If VIRTIO_F_NOTIF_CONFIG_DATA has been negotiated:
 \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.
+\field{queue_notify_data} value instead of the virtqueue 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.
 \end{itemize}
-- 
2.26.2

Re: [virtio-dev] [PATCH 1/3] transport-pci: Refer to the vq by its number

[Jiri Pirko]

Thu, Feb 23, 2023 at 06:46:22AM CET, parav@nvidia.com wrote:
>Currently specification uses virtqueue index and
>number interchangeably to refer to the virtqueue.
>
>Instead refer to it by its number.
>
>This patch is on top of [1].
>
>[1] https://lists.oasis-open.org/archives/virtio-dev/202302/msg00527.html
>
>Fixes: https://github.com/oasis-tcs/virtio-spec/issues/163
>Signed-off-by: Parav Pandit <parav@nvidia.com>

Reviewed-by: Jiri Pirko <jiri@nvidia.com>

[PATCH 2/3] transport-mmio: Rename QueueNum register

[Parav Pandit]

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

It is better to identify it using one terminology.

Two registers QueueNumMax and QueueNum actually reflect the queue size
or queue depth indicating max and actual number of entries in the queue.
Equivalent register in PCI transport is named differently as queue_size.

To bring consistency between pci and mmio transport, and to avoid
confusion between number and index, rename the QueueNumMax and QueueNum
registers to QueueSizeMax and QueueSize respectively.

[1] https://lists.oasis-open.org/archives/virtio-dev/202302/msg00527.html

Fixes: https://github.com/oasis-tcs/virtio-spec/issues/163
Signed-off-by: Parav Pandit <parav@nvidia.com>
---
 transport-mmio.tex | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/transport-mmio.tex b/transport-mmio.tex
index 65bae54..c59975e 100644
--- a/transport-mmio.tex
+++ b/transport-mmio.tex
@@ -104,14 +104,14 @@ \subsection{MMIO Device Register Layout}\label{sec:Virtio Transport Options / Vi
     number of the first queue is zero (0x0).
   }
   \hline
-  \mmioreg{QueueNumMax}{Maximum virtual queue size}{0x034}{R}{%
+  \mmioreg{QueueSizeMax}{Maximum virtual queue size}{0x034}{R}{%
     Reading from the register returns the maximum size (number of
     elements) of the queue the device is ready to process or
     zero (0x0) if the queue is not available. This applies to the
     queue selected by writing to \field{QueueSel}.
   }
   \hline
-  \mmioreg{QueueNum}{Virtual queue size}{0x038}{W}{%
+  \mmioreg{QueueSize}{Virtual queue size}{0x038}{W}{%
     Queue size is the number of elements in the queue.
     Writing to this register notifies the device what size of the
     queue the driver will use. This applies to the queue selected by
@@ -459,7 +459,7 @@ \subsection{Legacy interface}\label{sec:Virtio Transport Options / Virtio Over M
 .
   }
   \hline
-  \mmioreg{QueueNumMax}{Maximum virtual queue size}{0x034}{R}{%
+  \mmioreg{QueueSizeMax}{Maximum virtual queue size}{0x034}{R}{%
     Reading from the register returns the maximum size of the queue
     the device is ready to process or zero (0x0) if the queue is not
     available. This applies to the queue selected by writing to
@@ -467,7 +467,7 @@ \subsection{Legacy interface}\label{sec:Virtio Transport Options / Virtio Over M
     (0x0), so when the queue is not actively used.
   }
   \hline
-  \mmioreg{QueueNum}{Virtual queue size}{0x038}{W}{%
+  \mmioreg{QueueSize}{Virtual queue size}{0x038}{W}{%
     Queue size is the number of elements in the queue.
     Writing to this register notifies the device what size of the
     queue the driver will use. This applies to the queue selected by
-- 
2.26.2

Re: [virtio-dev] [PATCH 2/3] transport-mmio: Rename QueueNum register

[Jiri Pirko]

Thu, Feb 23, 2023 at 06:46:23AM CET, parav@nvidia.com wrote:
>Currently, the specification uses virtqueue index and number
>interchangeably to refer to the virtqueue.
>
>It is better to identify it using one terminology.
>
>Two registers QueueNumMax and QueueNum actually reflect the queue size
>or queue depth indicating max and actual number of entries in the queue.
>Equivalent register in PCI transport is named differently as queue_size.
>
>To bring consistency between pci and mmio transport, and to avoid
>confusion between number and index, rename the QueueNumMax and QueueNum
>registers to QueueSizeMax and QueueSize respectively.
>
>[1] https://lists.oasis-open.org/archives/virtio-dev/202302/msg00527.html
>
>Fixes: https://github.com/oasis-tcs/virtio-spec/issues/163
>Signed-off-by: Parav Pandit <parav@nvidia.com>

Reviewed-by: Jiri Pirko <jiri@nvidia.com>

Re: [PATCH 2/3] transport-mmio: Rename QueueNum register

[Michael S. Tsirkin]

On Thu, Feb 23, 2023 at 07:46:23AM +0200, Parav Pandit wrote:
> Currently, the specification uses virtqueue index and number
> interchangeably to refer to the virtqueue.
> 
> It is better to identify it using one terminology.
> 
> Two registers QueueNumMax and QueueNum actually reflect the queue size
> or queue depth indicating max and actual number of entries in the queue.
> Equivalent register in PCI transport is named differently as queue_size.
> 
> To bring consistency between pci and mmio transport, and to avoid
> confusion between number and index, rename the QueueNumMax and QueueNum
> registers to QueueSizeMax and QueueSize respectively.
> 
> [1] https://lists.oasis-open.org/archives/virtio-dev/202302/msg00527.html
> 
> Fixes: https://github.com/oasis-tcs/virtio-spec/issues/163
> Signed-off-by: Parav Pandit <parav@nvidia.com>

I think this is a good change but it will confuse people
who have e.g. a driver source and are trying to match
it to te spec. Suggestions below


> ---
>  transport-mmio.tex | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/transport-mmio.tex b/transport-mmio.tex
> index 65bae54..c59975e 100644
> --- a/transport-mmio.tex
> +++ b/transport-mmio.tex
> @@ -104,14 +104,14 @@ \subsection{MMIO Device Register Layout}\label{sec:Virtio Transport Options / Vi
>      number of the first queue is zero (0x0).
>    }
>    \hline
> -  \mmioreg{QueueNumMax}{Maximum virtual queue size}{0x034}{R}{%
> +  \mmioreg{QueueSizeMax}{Maximum virtual queue size}{0x034}{R}{%

I would add something like "Note: this was previously known as QueueNumMax"
and same elsewhere.

>      Reading from the register returns the maximum size (number of
>      elements) of the queue the device is ready to process or
>      zero (0x0) if the queue is not available. This applies to the
>      queue selected by writing to \field{QueueSel}.

>    }
>    \hline
> -  \mmioreg{QueueNum}{Virtual queue size}{0x038}{W}{%
> +  \mmioreg{QueueSize}{Virtual queue size}{0x038}{W}{%
>      Queue size is the number of elements in the queue.
>      Writing to this register notifies the device what size of the
>      queue the driver will use. This applies to the queue selected by
> @@ -459,7 +459,7 @@ \subsection{Legacy interface}\label{sec:Virtio Transport Options / Virtio Over M
>  .
>    }
>    \hline
> -  \mmioreg{QueueNumMax}{Maximum virtual queue size}{0x034}{R}{%
> +  \mmioreg{QueueSizeMax}{Maximum virtual queue size}{0x034}{R}{%
>      Reading from the register returns the maximum size of the queue
>      the device is ready to process or zero (0x0) if the queue is not
>      available. This applies to the queue selected by writing to

Same here.

> @@ -467,7 +467,7 @@ \subsection{Legacy interface}\label{sec:Virtio Transport Options / Virtio Over M
>      (0x0), so when the queue is not actively used.
>    }
>    \hline
> -  \mmioreg{QueueNum}{Virtual queue size}{0x038}{W}{%
> +  \mmioreg{QueueSize}{Virtual queue size}{0x038}{W}{%
>      Queue size is the number of elements in the queue.
>      Writing to this register notifies the device what size of the
>      queue the driver will use. This applies to the queue selected by

And here.

> -- 
> 2.26.2

[PATCH 3/3] transport-mmio: Refer to the vq by its number

[Parav Pandit]

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

Instead refer to it by its number.

This patch is on top of [1].

[1] https://lists.oasis-open.org/archives/virtio-dev/202302/msg00527.html

Fixes: https://github.com/oasis-tcs/virtio-spec/issues/163
Signed-off-by: Parav Pandit <parav@nvidia.com>
---
 transport-mmio.tex | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/transport-mmio.tex b/transport-mmio.tex
index c59975e..324cecf 100644
--- a/transport-mmio.tex
+++ b/transport-mmio.tex
@@ -96,7 +96,7 @@ \subsection{MMIO Device Register Layout}\label{sec:Virtio Transport Options / Vi
     bits accessible by writing to \field{DriverFeatures}.
   }
   \hline
-  \mmioreg{QueueSel}{Virtual queue index}{0x030}{W}{%
+  \mmioreg{QueueSel}{Virtual queue number}{0x030}{W}{%
     Writing to this register selects the virtual queue that the
     following operations on \field{QueueNumMax}, \field{QueueNum}, \field{QueueReady},
     \field{QueueDescLow}, \field{QueueDescHigh}, \field{QueueDriverlLow}, \field{QueueDriverHigh},
@@ -130,7 +130,7 @@ \subsection{MMIO Device Register Layout}\label{sec:Virtio Transport Options / Vi
     there are new buffers to process in a queue.
 
     When VIRTIO_F_NOTIFICATION_DATA has not been negotiated,
-    the value written is the queue index.
+    the value written is the queue number.
 
     When VIRTIO_F_NOTIFICATION_DATA has been negotiated,
     the \field{Notification data} value has the following format:
@@ -373,7 +373,7 @@ \subsubsection{Available Buffer Notifications}\label{sec:Virtio Transport Option
 
 When VIRTIO_F_NOTIFICATION_DATA has not been negotiated,
 the driver sends an available buffer notification to the device by writing
-the 16-bit virtqueue index
+the 16-bit virtqueue number
 of the queue to be notified to \field{QueueNotify}.
 
 When VIRTIO_F_NOTIFICATION_DATA has been negotiated,
@@ -451,7 +451,7 @@ \subsection{Legacy interface}\label{sec:Virtio Transport Options / Virtio Over M
     (see QueuePFN).
   }
   \hline
-  \mmioreg{QueueSel}{Virtual queue index}{0x030}{W}{%
+  \mmioreg{QueueSel}{Virtual queue number}{0x030}{W}{%
     Writing to this register selects the virtual queue that the
     following operations on the \field{QueueNumMax}, \field{QueueNum}, \field{QueueAlign}
     and \field{QueuePFN} registers apply to. The index
-- 
2.26.2

Re: [virtio-dev] [PATCH 3/3] transport-mmio: Refer to the vq by its number

[Jiri Pirko]

Thu, Feb 23, 2023 at 06:46:24AM CET, parav@nvidia.com wrote:
>Currently specification uses virtqueue index and
>number interchangeably to refer to the virtqueue.
>
>Instead refer to it by its number.
>
>This patch is on top of [1].
>
>[1] https://lists.oasis-open.org/archives/virtio-dev/202302/msg00527.html
>
>Fixes: https://github.com/oasis-tcs/virtio-spec/issues/163
>Signed-off-by: Parav Pandit <parav@nvidia.com>

Reviewed-by: Jiri Pirko <jiri@nvidia.com>

[virtio-comment] Re: [PATCH 0/3] Rename queue index to queue number

[Cornelia Huck]

On Thu, Feb 23 2023, Parav Pandit <parav@nvidia.com> wrote:

> 1. Currently, virtqueue is identified between driver and device
> interchangeably using either number of 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 as Number.
>
> Solution:
> Use virtqueue number description, and rename MMIO register as QueueSize.
>
> Patch summary:
> patch-1 renames index to number for pci transport
> patch-2 renames mmio register from Num to Size
> patch-3 renames index to number for mmio transport
>
> Please review.
> This series fixes the issue [1].
>
> This series is on top of [2].
>
> [1] https://github.com/oasis-tcs/virtio-spec/issues/163
> [2] https://lists.oasis-open.org/archives/virtio-dev/202302/msg00527.html
>
> ---
> Cornelia:
> I was not sure about ccw for vq_config_block and vq_info_block structures
> index field refers to the queue number or not.
> Can you please clarify?
>
> If it vqn, I will send v1 by replacing index to vqn to be
> consistent with other part of the spec which also uses vqn.

The vq_*_block structures use "index" for the vq index/number and "num"
for the number of buffers (queue size).

I'm wondering what terminology we should standardize on. For the size of
the queue, we have queue_size, QueueNum, and num. Calling it some
variation of "queue size" and mentioning that it refers to the number of
buffers makes sense.

For the vq index/number, I'm not that sure that "virtqueue number" is
better that "virtqueue index" -- actually, I'd prefer the latter. We'd
need some renaming either way.


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

RE: [PATCH 0/3] Rename queue index to queue number

[Parav Pandit]

> From: Cornelia Huck <cohuck@redhat.com>
> Sent: Monday, February 27, 2023 3:46 AM
> 
> On Thu, Feb 23 2023, Parav Pandit <parav@nvidia.com> wrote:
> 
> > 1. Currently, virtqueue is identified between driver and device
> > interchangeably using either number of 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 as Number.
> >
> > Solution:
> > Use virtqueue number description, and rename MMIO register as QueueSize.
> >
> > Patch summary:
> > patch-1 renames index to number for pci transport
> > patch-2 renames mmio register from Num to Size
> > patch-3 renames index to number for mmio transport
> >
> > Please review.
> > This series fixes the issue [1].
> >
> > This series is on top of [2].
> >
> > [1] https://github.com/oasis-tcs/virtio-spec/issues/163
> > [2]
> > https://lists.oasis-open.org/archives/virtio-dev/202302/msg00527.html
> >
> > ---
> > Cornelia:
> > I was not sure about ccw for vq_config_block and vq_info_block
> > structures index field refers to the queue number or not.
> > Can you please clarify?
> >
> > If it vqn, I will send v1 by replacing index to vqn to be consistent
> > with other part of the spec which also uses vqn.
> 
> The vq_*_block structures use "index" for the vq index/number and "num"
> for the number of buffers (queue size).
> 
Shall I change it too given the below response?
Or ccw sync is not very interesting at this point?

> I'm wondering what terminology we should standardize on. For the size of the
> queue, we have queue_size, QueueNum, and num. Calling it some variation of
> "queue size" and mentioning that it refers to the number of buffers makes
> sense.
> 
> For the vq index/number, I'm not that sure that "virtqueue number" is better
> that "virtqueue index" -- actually, I'd prefer the latter. We'd need some
> renaming either way.

In other thread, we discussed with Michael to use vq number to align to the following ongoing work.
1. existing spec 
2. future patches that he wrote for AQ (v10)
3. VQ level interrupt moderation patches (v9)
#2 and #3 uses vq number terminology.

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

[Michael S. Tsirkin]

On Mon, Feb 27, 2023 at 04:00:24PM +0000, Parav Pandit wrote:
> 
> > From: Cornelia Huck <cohuck@redhat.com>
> > Sent: Monday, February 27, 2023 3:46 AM
> > 
> > On Thu, Feb 23 2023, Parav Pandit <parav@nvidia.com> wrote:
> > 
> > > 1. Currently, virtqueue is identified between driver and device
> > > interchangeably using either number of 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 as Number.
> > >
> > > Solution:
> > > Use virtqueue number description, and rename MMIO register as QueueSize.
> > >
> > > Patch summary:
> > > patch-1 renames index to number for pci transport
> > > patch-2 renames mmio register from Num to Size
> > > patch-3 renames index to number for mmio transport
> > >
> > > Please review.
> > > This series fixes the issue [1].
> > >
> > > This series is on top of [2].
> > >
> > > [1] https://github.com/oasis-tcs/virtio-spec/issues/163
> > > [2]
> > > https://lists.oasis-open.org/archives/virtio-dev/202302/msg00527.html
> > >
> > > ---
> > > Cornelia:
> > > I was not sure about ccw for vq_config_block and vq_info_block
> > > structures index field refers to the queue number or not.
> > > Can you please clarify?
> > >
> > > If it vqn, I will send v1 by replacing index to vqn to be consistent
> > > with other part of the spec which also uses vqn.
> > 
> > The vq_*_block structures use "index" for the vq index/number and "num"
> > for the number of buffers (queue size).
> > 
> Shall I change it too given the below response?
> Or ccw sync is not very interesting at this point?

I'm for changing this. I am guessing Cornelia didn't being it up
just to move some electrons around either...

> > I'm wondering what terminology we should standardize on. For the size of the
> > queue, we have queue_size, QueueNum, and num. Calling it some variation of
> > "queue size" and mentioning that it refers to the number of buffers makes
> > sense.
> > 
> > For the vq index/number, I'm not that sure that "virtqueue number" is better
> > that "virtqueue index" -- actually, I'd prefer the latter. We'd need some
> > renaming either way.
> 
> In other thread, we discussed with Michael to use vq number to align to the following ongoing work.
> 1. existing spec 
> 2. future patches that he wrote for AQ (v10)
> 3. VQ level interrupt moderation patches (v9)
> #2 and #3 uses vq number terminology.


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[virtio-comment] Re: [virtio-dev] Re: [virtio-comment] Re: [PATCH 0/3] Rename queue index to queue number

[Halil Pasic]

On Fri, 3 Mar 2023 02:42:49 -0500
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> > >
> > > And then for extra fun, in the RSS section we say "0 based index" where we
> > > seem to mean "this number in the queue name, but subtract 1 in your head".
> > > Why subtract 1? I guess we wanted these 5 year olds to practice math ...  
> > 
> > Heh :)
> > 
> > Yeah, that looks like a mess... I don't think we should use a weird
> > substraction scheme. I haven't looked at the RSS stuff much, would it be
> > hard to fix it up?  
> 
> We can't change the ABI, I guess we can say that it's bits 1 to 15
> of the VQ number or equivalently VQ number divided
> by 2 (it's always an even number for any receiveq).

I don't think it would require an ABI change. We could just change the
queue names. AFAIK those are not part of the ABI. I don't think it would
be hard.

BTW what speaks for "VQ number" over "VQ index"?

Regards,
Halil



This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

[virtio-comment] Re: [virtio-dev] Re: [virtio-comment] Re: [PATCH 0/3] Rename queue index to queue number

[Michael S. Tsirkin]

On Fri, Mar 03, 2023 at 10:49:37PM +0100, Halil Pasic wrote:
> On Fri, 3 Mar 2023 02:42:49 -0500
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > > >
> > > > And then for extra fun, in the RSS section we say "0 based index" where we
> > > > seem to mean "this number in the queue name, but subtract 1 in your head".
> > > > Why subtract 1? I guess we wanted these 5 year olds to practice math ...  
> > > 
> > > Heh :)
> > > 
> > > Yeah, that looks like a mess... I don't think we should use a weird
> > > substraction scheme. I haven't looked at the RSS stuff much, would it be
> > > hard to fix it up?  
> > 
> > We can't change the ABI, I guess we can say that it's bits 1 to 15
> > of the VQ number or equivalently VQ number divided
> > by 2 (it's always an even number for any receiveq).
> 
> I don't think it would require an ABI change. We could just change the
> queue names. AFAIK those are not part of the ABI. I don't think it would
> be hard.

Well at the moment this is the mapping:

RSS index -  queue name - virtio pci vqn

0		receiveq1	0
1		receiveq2	2
2		receiveq3	4
3		receiveq4	6




> BTW what speaks for "VQ number" over "VQ index"?
> 
> Regards,
> Halil

We use "vq index" when referring to queue_select.

But, we use "vq number" when talking about notifications.

For fun MMIO calls the queue size field QueueNum


So both number and index are taken by things other than the number,
changing the meaning can confuse existing users.  Ideally we'd use some
other new term to avoid confusion but I could not come up with one so
far.  I feel there's less of a chance of a confusion between VQ size and
its number. But it's not a strong prefrence, RSS is relatively young
and it's the only incompatible user of index so far.


-- 
MST


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

[virtio-comment] Re: [virtio-dev] Re: [virtio-comment] Re: [PATCH 0/3] Rename queue index to queue number

[Halil Pasic]

On Sun, 5 Mar 2023 04:51:54 -0500
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> > I don't think it would require an ABI change. We could just change the
> > queue names. AFAIK those are not part of the ABI. I don't think it would
> > be hard.  
> 
> Well at the moment this is the mapping:
> 
> RSS index -  queue name - virtio pci vqn
> 
> 0		receiveq1	0
> 1		receiveq2	2
> 2		receiveq3	4
> 3		receiveq4	6
> 

Agreed. My point was that the names receiveq1, ... , receiveqN are not
part of any ABI. There is no virtio-pci/mmio/ccw transport nor
virtio-net interface where one would get or put something like
"receiveq1". I.e by changing those names we would not break ABI.
I don't say we should. I agree it could confuse people. I just
say it is possible.


> 
> 
> 
> > BTW what speaks for "VQ number" over "VQ index"?
> > 
> > Regards,
> > Halil  
> 
> We use "vq index" when referring to queue_select.

Right!

> 
> But, we use "vq number" when talking about notifications.
> 

Yes and no!

"""
4.1.5.2 Available Buffer Notifications

When VIRTIO_F_NOTIFICATION_DATA has not been negotiated, the driver sends an available buffer notification to the device by writing the 16-bit virtqueue index of this virtqueue to the Queue Notify address.
"""

Here we say *index of this virtqueue*.

"""
QueueNotify
0x050
W
	

Queue notifier
Writing a value to this register notifies the device that there are new buffers to process in a queue.
"""
When VIRTIO_F_NOTIFICATION_DATA has not been negotiated, the value written is the queue index. 

as well uses "index", but

"""
2.7.23 Driver notifications

The driver is sometimes required to send an available buffer notification to the 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).
"""

here we have that "virqueue number".

> For fun MMIO calls the queue size field QueueNum
> 
> 
> So both number and index are taken by things other than the number,
> changing the meaning can confuse existing users.  Ideally we'd use some
> other new term to avoid confusion but I could not come up with one so
> far.  

Connie is usually pretty good at coming up with good names. Out of the
top of my head, I guess I could live with "identifier/id" or "key". I
would still prefer sticking to "index" but always spelling out the index
of what. I.e. "the index of the virtqueue", "index of the receive
queue" (RSS), "index specified by avail_event into the available ring",
etc. 

Please notice that for RSS I changed "index of the receive
virtqueue" to "index of the receive queue", because "index of the receive
virtqueue" is ambiguous. On one hand one read this as. I want the
unclassified packets placed into receiveq3. So receiveq3 is actually the
5-th virtqueue of the virtio-net device, i.e. the virtqueue with the
index (and vqn) 4. By following that reasoning (first identify the
virtqueue than take it's index) one would write 4 into the
unclassified_queue field, which would be wrong according to your
table. On the other hand, one could also reason like this. Since I
want the  unclassified packets in receiveq3, I have to write 2 into
unclassified_queue, because the "receive-virtqueue index" of
 * receiveq1 is 0, 
 * receiveq2 is 1, and of
 * receiveq3 is 3.
I.e. we are not talking about the index of virtqueue-index the virtqueue
but about the receivequeue-index of the receive queue. In other words
the conceptual array/table we are indexing into is not the array of
virtqueues, but the array of the receive queues.

To uniquely identify an element we need both the index (a value) and
the array/table that is being indexed. And I believe this is what we
need to improve on. I don't think calling indexes into the conceptual
array of receive queues (or receive virtqueues) "index of the receive
virtqueue", indexes into the conceptual array of virtqueues that belong
to a certain device a "vq number", indexes into the descriptor table
"descriptor keys", and indexes into the used rings "used ring element
ids" would really help -- to make an attempt at using reduction ad
absurdum ;)



> I feel there's less of a chance of a confusion between VQ size and
> its number. But it's not a strong prefrence, RSS is relatively young
> and it's the only incompatible user of index so far.

I don't understand why is RSS incompatible with "index".

Regards,
Halil

This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

[virtio-comment] Re: [virtio-dev] Re: [virtio-comment] Re: [PATCH 0/3] Rename queue index to queue number

[Michael S. Tsirkin]

On Thu, Mar 09, 2023 at 05:46:50PM +0100, Halil Pasic wrote:
> To uniquely identify an element we need both the index (a value) and
> the array/table that is being indexed. And I believe this is what we
> need to improve on. I don't think calling indexes into the conceptual
> array of receive queues (or receive virtqueues) "index of the receive
> virtqueue", indexes into the conceptual array of virtqueues that belong
> to a certain device a "vq number", indexes into the descriptor table
> "descriptor keys", and indexes into the used rings "used ring element
> ids" would really help -- to make an attempt at using reduction ad
> absurdum ;)

Right.  I think Parav said he's converting everything to just refer
to "VQ number".


> 
> 
> > I feel there's less of a chance of a confusion between VQ size and
> > its number. But it's not a strong prefrence, RSS is relatively young
> > and it's the only incompatible user of index so far.
> 
> I don't understand why is RSS incompatible with "index".
> 
> Regards,
> Halil

It uses the word "index" in a way that is different from what you
suggest.

Anyway, both "number" and "index" require cleanup work, whoever
is going to do it gets to decide which way it will go.

-- 
MST


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

[virtio-comment] Re: [virtio-dev] Re: [virtio-comment] Re: [PATCH 0/3] Rename queue index to queue number

[Halil Pasic]

On Thu, 9 Mar 2023 11:53:34 -0500
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> Anyway, both "number" and "index" require cleanup work, whoever
> is going to do it gets to decide which way it will go

Makes sense! Thank you!

Regards,
Halil

This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

Re: [PATCH 0/3] Rename queue index to queue number

[Michael S. Tsirkin]

On Thu, Feb 23, 2023 at 07:46:21AM +0200, Parav Pandit wrote:
> 1. Currently, virtqueue is identified between driver and device
> interchangeably using either number of 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 as Number.
> 
> Solution:
> Use virtqueue number description, and rename MMIO register as QueueSize.
> 
> Patch summary:
> patch-1 renames index to number for pci transport
> patch-2 renames mmio register from Num to Size
> patch-3 renames index to number for mmio transport
> 
> Please review.
> This series fixes the issue [1].
> 
> This series is on top of [2].
> 
> [1] https://github.com/oasis-tcs/virtio-spec/issues/163
> [2] https://lists.oasis-open.org/archives/virtio-dev/202302/msg00527.html

What about RSS description in net? That says:

description.tex:Field \field{unclassified_queue} contains the 0-based index of

is the index same as vq number? or something different?





> ---
> Cornelia:
> I was not sure about ccw for vq_config_block and vq_info_block structures
> index field refers to the queue number or not.
> Can you please clarify?
> 
> If it vqn, I will send v1 by replacing index to vqn to be
> consistent with other part of the spec which also uses vqn.
> 
> Parav Pandit (3):
>   transport-pci: Refer to the vq by its number
>   transport-mmio: Rename QueueNum register
>   transport-mmio: Refer to the vq by its number
> 
>  transport-mmio.tex | 16 ++++++++--------
>  transport-pci.tex  |  6 +++---
>  2 files changed, 11 insertions(+), 11 deletions(-)
> 
> -- 
> 2.26.2

Re: [virtio-comment] Re: [PATCH 0/3] Rename queue index to queue number

[Halil Pasic]

On Thu, 2 Mar 2023 16:12:48 +0000
Parav Pandit <parav@nvidia.com> wrote:

> > >
> > > is the index same as vq number? or something different?  
> > 
> > It is something different. The full paragraph sound like this
> > 
> > """
> > Field \field{unclassified_queue} contains the 0-based index of the receive
> > virtqueue to place unclassified packets in. Index 0 corresponds to receiveq1.
> > """  
> With vqn it can be as simply written as.
> Field \field{unclassified_queue} contains the receive virtqueue number to use for unclassified incoming packets.
> 
> Similarly rss description can be simplified.

But that would change the semantics of the interface (and the ABI). And
we really should refrain from that.

We could also say 

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

Provided that we decide to change the naming of the receive queues so
that receiveqN becomes receiveqN-1, and provided that we go with the
virtqueue index and not with the virtqueue number wording.

We could also just drop the name and just say that it corresponds to the
virtqueue index 2N like Michael has pointed out. But I would prefer the
former.

Regards,
Halil

This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

Re: [virtio-comment] Re: [PATCH 0/3] Rename queue index to queue number

[Michael S. Tsirkin]

On Fri, Mar 03, 2023 at 04:38:02PM +0100, Halil Pasic wrote:
> On Thu, 2 Mar 2023 16:12:48 +0000
> Parav Pandit <parav@nvidia.com> wrote:
> 
> > > >
> > > > is the index same as vq number? or something different?  
> > > 
> > > It is something different. The full paragraph sound like this
> > > 
> > > """
> > > Field \field{unclassified_queue} contains the 0-based index of the receive
> > > virtqueue to place unclassified packets in. Index 0 corresponds to receiveq1.
> > > """  
> > With vqn it can be as simply written as.
> > Field \field{unclassified_queue} contains the receive virtqueue number to use for unclassified incoming packets.
> > 
> > Similarly rss description can be simplified.
> 
> But that would change the semantics of the interface (and the ABI). And
> we really should refrain from that.
> 
> We could also say 
> 
> Field \field{unclassified_queue} contains the 0-based index of the receive
> virtqueue to place unclassified packets in. Index N corresponds to receiveqN
> and thus to virtqueue index 2N.
> Provided that we decide to change the naming of the receive queues so
> that receiveqN becomes receiveqN-1, and provided that we go with the
> virtqueue index and not with the virtqueue number wording.

This has a decent chance to confuse a lot of people.
E.g. if someone wrote "receiveq1" in their code comments,
now this suddenly refers to a different queue.

> We could also just drop the name and just say that it corresponds to the
> virtqueue index 2N like Michael has pointed out. But I would prefer the
> former.
> 
> Regards,
> Halil

I proposed virtqueue number not virtqueue index because we
said index in the past to mean N+1.
Or even better some new term.

-- 
MST


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

RE: [virtio-comment] Re: [PATCH 0/3] Rename queue index to queue number

[Parav Pandit]

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Thursday, March 2, 2023 6:39 PM
> >
> > Similarly rss description can be simplified.
> 
> Unfortunately this is not what it is. It's actually vqn / 2.
> 
:(
I see it now.
It is vqn/2.

So yes, need to document it this way that it is vqn/2.

> > > Here follows the explanation.
> > >
> > > Disclaimer: For a brief math background see [1] and [2].
> > >
> > > Here the index set the set of natural numbers (which includes 0, the
> > > neutral element for the addition operation).
> > >
> > > The indexed set is the set of receive virtqueues, each member of
> > > that set is associated with an unique member of the index set.
> > >
> > > """
> > > \subsection{Virtqueues}\label{sec:Device Types / Network Device /
> > > Virtqueues}
> > >
> > > \begin{description}
> > > \item[0] receiveq1
> > > \item[1] transmitq1
> > > \item[\ldots]
> > > \item[2(N-1)] receiveqN
> > > \item[2(N-1)+1] transmitqN
> > > \item[2N] controlq
> > > \end{description}
> > > """
> > >
> > > But, as seen above, we happen to also use an other index set for the
> > > receive queues of an virtio-net device. I guess the intention is to
> > > use these as names, in any case the set is { "receiveq1", "receiveq2", ... }
> (yes, I'm a little sloppy here).
> > >
> > > Thus we can say that:
> > > * the virtqueue index 2*(N-1)
> > > * the recieve virtqueue index N-1, and the
> > > * virtqueue name receiveqN
> > > refer to the very same virtqueue (for each N > 0)
> > >
> > Receiveq1 is a name to virtuque number (currently index) 0.
> > And naming this way doesn't seem to be a problem in the description.
> >
> > Section 5.1.2 is liste them already.
Bad auto-correction.
I was saying the existing section "Virtqueues" in Network device already lists the mapping between vqn(vq index) and logical receive and transmit q.
An additional short line telling about this mapping is enough.

> > A short line tell about this mapping will suffice.
> 
> can't parse this.

My point was, there isn't a lot of change needed around current examples where receiveN is talked.
Better to see in the actual patch.

This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/