Re: [virtio-comment] [PATCH v10 2/8] transport-pci: Refer to the vq by its number

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

On Thu, 30 Mar 2023 00:23:35 +0300
Parav Pandit <parav@nvidia.com> wrote:

> 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 <cohuck@redhat.com>
> Reviewed-by: Max Gurtovoy <mgurtovoy@nvidia.com>
> Reviewed-by: Jiri Pirko <jiri@nvidia.com>
> Signed-off-by: Parav Pandit <parav@nvidia.com>
> ---

[..]

>  transport-pci.tex | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/transport-pci.tex b/transport-pci.tex
> index b07a822..0f3a48b 100644
> --- a/transport-pci.tex
> +++ b/transport-pci.tex
> @@ -390,13 +390,14 @@ \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.
> -        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... 

>          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 vq number. Some devices

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

> @@ -1053,7 +1054,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 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.

Regards,
Halil 


>  \end{itemize}