RE: [PATCH v10] virtio-net: support the virtqueue coalescing moderation

[Parav Pandit <parav@nvidia.com>]

> From: Heng Qi <hengqi@linux.alibaba.com>
> Sent: Wednesday, March 1, 2023 9:10 AM

> Currently, coalescing parameters are grouped for all transmit and receive
> virtqueues. This patch supports setting or getting the parameters for a specified
> virtqueue, and a typical application of this function is netdim[1].
> 
> When the traffic between virtqueues is unbalanced, for example, one
> virtqueue is busy and another virtqueue is idle, then it will be very useful to
> control coalescing parameters at the virtqueue granularity.
> 

[..]
> +Read/Write attributes for coalescing parameters:
> +\begin{itemize}
> +\item For commands VIRTIO_NET_CTRL_NOTF_COAL_TX_SET and
> VIRTIO_NET_CTRL_NOTF_COAL_RX_SET, \field{max_usecs}
> +      and \field{max_packets} are write-only for a driver.
> +\item For the command VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET, \field{vqn},
> \field{reserved}, \field{max_usecs}
> +      and \field{max_packets} are write-only for a driver.
> +\item For the command VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET, \field{vqn}
> and \field{reserved} are write-only
> +      for a driver, and, \field{max_usecs} and \field{max_packets} are read-only
> for the driver.
> +\end{itemize}
> +
Maybe I missed the conversation while I was sick.
I remember we discussed that instead of mentioning each individual field, better to describe the whole structure being read-only or write-only.

> +The class VIRTIO_NET_CTRL_NOTF_COAL has 4 commands:
>  \begin{enumerate}
> -\item VIRTIO_NET_CTRL_NOTF_COAL_TX_SET: set the \field{max_usecs} and
> \field{max_packets} parameters for all transmit virtqueues.
> -\item VIRTIO_NET_CTRL_NOTF_COAL_RX_SET: set the \field{max_usecs} and
> \field{max_packets} parameters for all receive virtqueues.
> +\item VIRTIO_NET_CTRL_NOTF_COAL_TX_SET: use the structure
> virtio_net_ctrl_coal to set the \field{max_usecs} and \field{max_packets}
> parameters for all transmit virtqueues.
> +\item VIRTIO_NET_CTRL_NOTF_COAL_RX_SET: use the structure
> virtio_net_ctrl_coal to set the \field{max_usecs} and \field{max_packets}
> parameters for all receive virtqueues.
> +\item VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET: use the structure
> virtio_net_ctrl_coal_vq to set the \field{max_usecs} and \field{max_packets}
> parameters
> +                                        for an enabled transmit/receive virtqueue whose
> number is \field{vqn}.
> +\item VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET: use the structure
> virtio_net_ctrl_coal_vq to get the \field{max_usecs} and \field{max_packets}
> parameters
> +                                        for an enabled transmit/receive virtqueue whose
> number is \field{vqn}.
>  \end{enumerate}
> 
> +\begin{note}
> +The behavior of the device in response to these commands is best-effort:
> +the device may generate notifications more or less frequently than specified.
> +\end{note}
> +
Michael,
Why this should be a note? Shouldn't it be a normative line in the device requirements?

> +If coalescing parameters are being set, the device applies the last
> +coalescing parameters received for a virtqueue, regardless of the
s/received/set to match the if condition wording and regardless sentence after.

> +command used to set the parameters. Use the following command sequence
> with 2 pairs of virtqueues as an example:
> +Each of the following commands sets \field{max_usecs} and
> \field{max_packets} parameters for virtqueues.
> +\begin{itemize}
> +\item Command1: VIRTIO_NET_CTRL_NOTF_COAL_RX_SET sets coalescing
> parameters for virtqueue0 and virtqueue2, and, virtqueue1 and virtqueue3
> retain their previous parameter values.
> +\item Command2: VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET with \field{vqn} =
> 0 sets coalescing parameters for virtqueue0, and virtqueue2 retains the values
> from command1.
> +\item Command3: VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET with \field{vqn} =
> 0, the device responds with coalescing parameters of virtqueue0 set by
> command2.
> +\item Command4: VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET with \field{vqn} =
> 1 sets coalescing parameters for virtqueue1, and virtqueue3 retains its previous
> values.
> +\item Command5: VIRTIO_NET_CTRL_NOTF_COAL_TX_SET sets coalescing
> parameters for virtqueue1 and virtqueue3, and overrides the values set by
> command4.
> +\item Command6: VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET with \field{vqn} =
> 1, the device responds with coalescing parameters of virtqueue1 set by
> command5.
> +\end{itemize}
> +
> +Upon disabling and re-enabling the transmit virtqueue, the device will
> +set the coalescing parameters of the virtqueue to those configured through
> the VIRTIO_NET_CTRL_NOTF_COAL_TX_SET command, or to 0 if the command
> did not set any TX coalescing parameters.
> +
> +Upon disabling and re-enabling the receive virtqueue, the device will
> +set the coalescing parameters of the virtqueue to those configured through
> the VIRTIO_NET_CTRL_NOTF_COAL_RX_SET command, or to 0 if the command
> did not set any RX coalescing parameters.
> +
Looks good, however you have well covered in the device normative statements.
So possibly it can be removed from here.

>  \subparagraph{Operation}\label{sec:Device Types / Network Device / Device
> Operation / Control Virtqueue / Notifications Coalescing / Operation}
> 
>  The device sends a used buffer notification once the notification conditions are
> met and if the notifications are not suppressed as explained in \ref{sec:Basic
> Facilities of a Virtio Device / Virtqueues / Used Buffer Notification Suppression}.
> @@ -1585,11 +1637,25 @@ \subsubsection{Control
> Virtqueue}\label{sec:Device Types / Network Device / Devi
> 
>  \drivernormative{\subparagraph}{Notifications Coalescing}{Device Types /
> Network Device / Device Operation / Control Virtqueue / Notifications
> Coalescing}
> 
> -If the VIRTIO_NET_F_NOTF_COAL feature has not been negotiated, the driver
> MUST NOT issue VIRTIO_NET_CTRL_NOTF_COAL commands.
> +A driver MUST negotiate the VIRTIO_NET_F_NOTF_COAL feature before
> issuing commands VIRTIO_NET_CTRL_NOTF_COAL_TX_SET and
> VIRIO_NET_CTRL_NOTF_COAL_RX_SET.
> +
> +A driver MUST negotiate the VIRTIO_NET_F_VQ_NOTF_COAL feature before
> issuing commands VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET and
> VIRIO_NET_CTRL_NOTF_COAL_VQ_GET.
> +
> +A driver MUST ignore the values of coalescing parameters received from the
> VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET command if a device responds with
> VIRTIO_NET_ERR.
> 
>  \devicenormative{\subparagraph}{Notifications Coalescing}{Device Types /
> Network Device / Device Operation / Control Virtqueue / Notifications
> Coalescing}
> 
> -A device SHOULD respond to the VIRTIO_NET_CTRL_NOTF_COAL commands
> with VIRTIO_NET_ERR if it was not able to change the parameters.
> +A device MUST ignore \field{reserved}.
> +
> +A device SHOULD respond to VIRTIO_NET_CTRL_NOTF_COAL_TX_SET and
> VIRTIO_NET_CTRL_NOTF_COAL_RX_SET commands with VIRTIO_NET_ERR if it
> was not able to change the parameters.
> +
> +A device MUST respond to the VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET
> command with VIRTIO_NET_ERR if it was not able to change the parameters.
> +
> +A device MUST respond to VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET and
> VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET commands with VIRTIO_NET_ERR if
> the given virtqueue is disabled.
> +
> +After disabling and re-enabling a transmit/receive virtqueue, the
> +device MUST set coalescing parameters of the virtqueue to those
> +configured using the
> VIRTIO_NET_CTRL_NOTF_COAL_TX_SET/VIRTIO_NET_CTRL_NOTF_COAL_RX_S
> ET command, or, if the command did not configure TX/RX coalescing
> parameters, to 0.
> 
>  A device SHOULD NOT send used buffer notifications to the driver if the
> notifications are suppressed, even if the notification conditions are met.
> 
> --
> 2.19.1.6.gb485710b

Rest looks good to me.