[virtio-comment] [PATCH v12] virtio-net: support the virtqueue coalescing moderation

[virtio-comment] [PATCH v12] virtio-net: support the virtqueue coalescing moderation

[Heng Qi]

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.

[1] https://docs.kernel.org/networking/net_dim.html

Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
Reviewed-by: Parav Pandit <parav@nvidia.com>
---
v11->v12:
       1. Use virtqueue having vqn N instead of virtqueueN. @Parav Pandit
       2. Some minor modifications. @Parav Pandit, @Michael S . Tsirkin
       3. Add Parav's Reviewed-by tag (Thanks!).

v10->v11:
       1. Describe read and write attributes for the structure. @Parav Pandit
       2. Some minor modifications. @Parav Pandit

v9->v10:
       1. Remove the "global values". @Parav Pandit
       2. Avoid multiple interpretations of command behavior. @Alvaro Karsz

v8->v9:
       1. Declare the commands that can be sent for each feature. @Alvaro Karsz
       2. Add information about "global values" in the command's explanation. @Alvaro Karsz

v7->v8:
       1. Use "best-effort" in Alvaro's patch instead of "the device may set the parameter to a value close to 2". @Michael S . Tsirkin, @David Edmondson

v6->v7:
       1. Clarify the relationship of VIRTIO_NET_CTRL_NOTF_COAL_TX/RX_SET and VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET. @Alvaro Karsz, @Michael S. Tsirkin
       2. Remove the formula for vqn range. @Parav Pandit
       3. Some expressions are clearer. @Parav Pandit, @Michael S. Tsirkin

v5->v6:
       1. Explain that the device may set a different value than the one passed in by the driver. @David Edmondson

v4->v5:
       1. Add the correspondence between virtio_net_ctrl_coal and virtio_net_ctrl_coal_vq and control commands. @Michael S. Tsirkin
       2. Add read and write attributes for each field. @Michael S. Tsirkin
       3. A clearer description of how to set coalescing parameters for vq reset. @Michael S. Tsirkin
       4. Fix some syntax errors. @Michael S. Tsirkin, @David Edmondson

v3->v4:
       1. Include virtio_net_ctrl_coal in the virtio_net_ctrl_coal_vq structure. @Alvaro Karsz
       2. Add consideration of vq reset. @Michael S. Tsirkin, @Parav Pandit, @Alvaro Karsz
       3. Avoid too many examples by giving a comprehensive example. @Michael S. Tsirkin
       4. Fix typos and streamline clarifications. @Michael S. Tsirkin, @Parav Pandit, @Alvaro Karsz

v2->v3:
       1. Add the netdim link. @Parav Pandit
       2. VIRTIO_NET_F_VQ_NOTF_COAL no longer depends on VIRTIO_NET_F_NOTF_COAL. @Michael S. Tsirkin, @Alvaro Karsz
       3. _VQ_GET is explained more. @Michael S. Tsirkin
       4. Add more examples to avoid misunderstandings. @Michael S. Tsirkin
       5. Clarify some statements. @Michael S. Tsirkin, @Parav Pandit, @Alvaro Karsz
       6. Adjust the virtio_net_ctrl_coal_vq structure. @Michael S. Tsirkin
       7. Fix some typos. @Michael S. Tsirkin

v1->v2:
       1. Rename VIRTIO_NET_F_PERQUEUE_NOTF_COAL to VIRTIO_NET_F_VQ_NOTF_COAL. @Michael S. Tsirkin
       2. Use the \field{vqn} instead of the qid. @Michael S. Tsirkin
       3. Unify tx and rx control structures into one structure virtio_net_ctrl_coal_vq. @Michael S. Tsirkin
       4. Add a new control command VIRTIO_NET_CTRL_NOTF_COAL_VQ. @Michael S. Tsirkin, @Parav Pandit, @Alvaro Karsz
       5. The special value 0xFFF is removed because VIRTIO_NET_CTRL_NOTF_COAL can be used. @Alvaro Karsz
       6. Clarify some special scenarios. @Michael S. Tsirkin, @Parav Pandit, @Alvaro Karsz

 device-types/net/description.tex | 80 ++++++++++++++++++++++++++++----
 1 file changed, 70 insertions(+), 10 deletions(-)

diff --git a/device-types/net/description.tex b/device-types/net/description.tex
index e71e33b..c6ab806 100644
--- a/device-types/net/description.tex
+++ b/device-types/net/description.tex
@@ -83,6 +83,8 @@ \subsection{Feature bits}\label{sec:Device Types / Network Device / Feature bits
 \item[VIRTIO_NET_F_CTRL_MAC_ADDR(23)] Set MAC address through control
     channel.
 
+\item[VIRTIO_NET_F_VQ_NOTF_COAL(52)] Device supports virtqueue notification coalescing.
+
 \item[VIRTIO_NET_F_NOTF_COAL(53)] Device supports notifications coalescing.
 
 \item[VIRTIO_NET_F_GUEST_USO4 (54)] Driver can receive USOv4 packets.
@@ -139,6 +141,7 @@ \subsubsection{Feature bit requirements}\label{sec:Device Types / Network Device
 \item[VIRTIO_NET_F_NOTF_COAL] Requires VIRTIO_NET_F_CTRL_VQ.
 \item[VIRTIO_NET_F_RSC_EXT] Requires VIRTIO_NET_F_HOST_TSO4 or VIRTIO_NET_F_HOST_TSO6.
 \item[VIRTIO_NET_F_RSS] Requires VIRTIO_NET_F_CTRL_VQ.
+\item[VIRTIO_NET_F_VQ_NOTF_COAL] Requires VIRTIO_NET_F_CTRL_VQ.
 \end{description}
 
 \subsubsection{Legacy Interface: Feature bits}\label{sec:Device Types / Network Device / Feature bits / Legacy Interface: Feature bits}
@@ -1506,12 +1509,12 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
 \paragraph{Notifications Coalescing}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Notifications Coalescing}
 
 If the VIRTIO_NET_F_NOTF_COAL feature is negotiated, the driver can
-send control commands for dynamically changing the coalescing parameters.
+send commands VIRTIO_NET_CTRL_NOTF_COAL_TX_SET and VIRTIO_NET_CTRL_NOTF_COAL_RX_SET
+for notification coalescing.
 
-\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}
+If the VIRTIO_NET_F_VQ_NOTF_COAL feature is negotiated, the driver can
+send commands VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET and VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET
+for virtqueue notification coalescing.
 
 \begin{lstlisting}
 struct virtio_net_ctrl_coal {
@@ -1519,25 +1522,63 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
     le32 max_usecs;
 };
 
+struct virtio_net_ctrl_coal_vq {
+    le16 vqn;
+    le16 reserved;
+    struct virtio_net_ctrl_coal coal;
+};
+
 #define VIRTIO_NET_CTRL_NOTF_COAL 6
  #define VIRTIO_NET_CTRL_NOTF_COAL_TX_SET  0
  #define VIRTIO_NET_CTRL_NOTF_COAL_RX_SET 1
+ #define VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET 2
+ #define VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET 3
 \end{lstlisting}
 
 Coalescing parameters:
 \begin{itemize}
+\item \field{vqn}: The virtqueue number of an enabled transmit or receive virtqueue.
 \item \field{max_usecs} for RX: Maximum number of microseconds to delay a RX notification.
 \item \field{max_usecs} for TX: Maximum number of microseconds to delay a TX notification.
 \item \field{max_packets} for RX: Maximum number of packets to receive before a RX notification.
 \item \field{max_packets} for TX: Maximum number of packets to send before a TX notification.
 \end{itemize}
 
-The class VIRTIO_NET_CTRL_NOTF_COAL has 2 commands:
+\field{reserved} is reserved and it is ignored by a device.
+
+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, the structure virtio_net_ctrl_coal is write-only for a driver.
+\item For the command VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET, the structure virtio_net_ctrl_coal_vq is 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 the structure virtio_net_ctrl_coal is read-only for the driver.
+\end{itemize}
+
+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}
 
+The device may generate notifications more or less frequently than specified by set commands of the VIRTIO_NET_CTRL_NOTF_COAL class.
+
+If coalescing parameters are being set, the device applies the last coalescing parameters set for a
+virtqueue, regardless of the 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 virtqueues having vqn 0 and vqn 2. Virtqueues having vqn 1 and vqn 3 retain their previous parameters.
+\item Command2: VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET with \field{vqn} = 0 sets coalescing parameters for virtqueue having vqn 0. Virtqueue having vqn 2 retains the parameters from command1.
+\item Command3: VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET with \field{vqn} = 0, the device responds with coalescing parameters of vqn 0 set by command2.
+\item Command4: VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET with \field{vqn} = 1 sets coalescing parameters for virtqueue having vqn 1. Virtqueue having vqn 3 retains its previous parameters.
+\item Command5: VIRTIO_NET_CTRL_NOTF_COAL_TX_SET sets coalescing parameters for virtqueues having vqn 1 and vqn 3, and overrides the parameters set by command4.
+\item Command6: VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET with \field{vqn} = 1, the device responds with coalescing parameters of vqn 1 set by command5.
+\end{itemize}
+
 \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,14 +1626,33 @@ \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.
+
+Upon disabling and re-enabling the transmit virtqueue, the device MUST set the coalescing parameters of the virtqueue
+to those configured through the VIRTIO_NET_CTRL_NOTF_COAL_TX_SET command, or, if the driver did not set any TX coalescing parameters, to 0.
+
+Upon disabling and re-enabling the receive virtqueue, the device MUST set the coalescing parameters of the virtqueue
+to those configured through the VIRTIO_NET_CTRL_NOTF_COAL_RX_SET command, or, if the driver did not set any 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.
 
+The behavior of the device in response to set commands of the VIRTIO_NET_CTRL_NOTF_COAL class is best-effort:
+the device MAY generate notifications more or less frequently than specified.
+
 Upon reset, a device MUST initialize all coalescing parameters to 0.
 
 \subsubsection{Legacy Interface: Framing Requirements}\label{sec:Device
-- 
2.19.1.6.gb485710b


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] [PATCH v12] virtio-net: support the virtqueue coalescing moderation

[Heng Qi]

Hi all, is there any comment on this topic?
If not, can you please cast a vote, thank you very much!:)

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

在 2023/3/12 下午8:46, Heng Qi 写道:
> 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.
>
> [1] https://docs.kernel.org/networking/net_dim.html
>
> Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
> Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> Reviewed-by: Parav Pandit <parav@nvidia.com>
> ---
> v11->v12:
>         1. Use virtqueue having vqn N instead of virtqueueN. @Parav Pandit
>         2. Some minor modifications. @Parav Pandit, @Michael S . Tsirkin
>         3. Add Parav's Reviewed-by tag (Thanks!).
>
> v10->v11:
>         1. Describe read and write attributes for the structure. @Parav Pandit
>         2. Some minor modifications. @Parav Pandit
>
> v9->v10:
>         1. Remove the "global values". @Parav Pandit
>         2. Avoid multiple interpretations of command behavior. @Alvaro Karsz
>
> v8->v9:
>         1. Declare the commands that can be sent for each feature. @Alvaro Karsz
>         2. Add information about "global values" in the command's explanation. @Alvaro Karsz
>
> v7->v8:
>         1. Use "best-effort" in Alvaro's patch instead of "the device may set the parameter to a value close to 2". @Michael S . Tsirkin, @David Edmondson
>
> v6->v7:
>         1. Clarify the relationship of VIRTIO_NET_CTRL_NOTF_COAL_TX/RX_SET and VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET. @Alvaro Karsz, @Michael S. Tsirkin
>         2. Remove the formula for vqn range. @Parav Pandit
>         3. Some expressions are clearer. @Parav Pandit, @Michael S. Tsirkin
>
> v5->v6:
>         1. Explain that the device may set a different value than the one passed in by the driver. @David Edmondson
>
> v4->v5:
>         1. Add the correspondence between virtio_net_ctrl_coal and virtio_net_ctrl_coal_vq and control commands. @Michael S. Tsirkin
>         2. Add read and write attributes for each field. @Michael S. Tsirkin
>         3. A clearer description of how to set coalescing parameters for vq reset. @Michael S. Tsirkin
>         4. Fix some syntax errors. @Michael S. Tsirkin, @David Edmondson
>
> v3->v4:
>         1. Include virtio_net_ctrl_coal in the virtio_net_ctrl_coal_vq structure. @Alvaro Karsz
>         2. Add consideration of vq reset. @Michael S. Tsirkin, @Parav Pandit, @Alvaro Karsz
>         3. Avoid too many examples by giving a comprehensive example. @Michael S. Tsirkin
>         4. Fix typos and streamline clarifications. @Michael S. Tsirkin, @Parav Pandit, @Alvaro Karsz
>
> v2->v3:
>         1. Add the netdim link. @Parav Pandit
>         2. VIRTIO_NET_F_VQ_NOTF_COAL no longer depends on VIRTIO_NET_F_NOTF_COAL. @Michael S. Tsirkin, @Alvaro Karsz
>         3. _VQ_GET is explained more. @Michael S. Tsirkin
>         4. Add more examples to avoid misunderstandings. @Michael S. Tsirkin
>         5. Clarify some statements. @Michael S. Tsirkin, @Parav Pandit, @Alvaro Karsz
>         6. Adjust the virtio_net_ctrl_coal_vq structure. @Michael S. Tsirkin
>         7. Fix some typos. @Michael S. Tsirkin
>
> v1->v2:
>         1. Rename VIRTIO_NET_F_PERQUEUE_NOTF_COAL to VIRTIO_NET_F_VQ_NOTF_COAL. @Michael S. Tsirkin
>         2. Use the \field{vqn} instead of the qid. @Michael S. Tsirkin
>         3. Unify tx and rx control structures into one structure virtio_net_ctrl_coal_vq. @Michael S. Tsirkin
>         4. Add a new control command VIRTIO_NET_CTRL_NOTF_COAL_VQ. @Michael S. Tsirkin, @Parav Pandit, @Alvaro Karsz
>         5. The special value 0xFFF is removed because VIRTIO_NET_CTRL_NOTF_COAL can be used. @Alvaro Karsz
>         6. Clarify some special scenarios. @Michael S. Tsirkin, @Parav Pandit, @Alvaro Karsz
>
>   device-types/net/description.tex | 80 ++++++++++++++++++++++++++++----
>   1 file changed, 70 insertions(+), 10 deletions(-)
>
> diff --git a/device-types/net/description.tex b/device-types/net/description.tex
> index e71e33b..c6ab806 100644
> --- a/device-types/net/description.tex
> +++ b/device-types/net/description.tex
> @@ -83,6 +83,8 @@ \subsection{Feature bits}\label{sec:Device Types / Network Device / Feature bits
>   \item[VIRTIO_NET_F_CTRL_MAC_ADDR(23)] Set MAC address through control
>       channel.
>   
> +\item[VIRTIO_NET_F_VQ_NOTF_COAL(52)] Device supports virtqueue notification coalescing.
> +
>   \item[VIRTIO_NET_F_NOTF_COAL(53)] Device supports notifications coalescing.
>   
>   \item[VIRTIO_NET_F_GUEST_USO4 (54)] Driver can receive USOv4 packets.
> @@ -139,6 +141,7 @@ \subsubsection{Feature bit requirements}\label{sec:Device Types / Network Device
>   \item[VIRTIO_NET_F_NOTF_COAL] Requires VIRTIO_NET_F_CTRL_VQ.
>   \item[VIRTIO_NET_F_RSC_EXT] Requires VIRTIO_NET_F_HOST_TSO4 or VIRTIO_NET_F_HOST_TSO6.
>   \item[VIRTIO_NET_F_RSS] Requires VIRTIO_NET_F_CTRL_VQ.
> +\item[VIRTIO_NET_F_VQ_NOTF_COAL] Requires VIRTIO_NET_F_CTRL_VQ.
>   \end{description}
>   
>   \subsubsection{Legacy Interface: Feature bits}\label{sec:Device Types / Network Device / Feature bits / Legacy Interface: Feature bits}
> @@ -1506,12 +1509,12 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
>   \paragraph{Notifications Coalescing}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Notifications Coalescing}
>   
>   If the VIRTIO_NET_F_NOTF_COAL feature is negotiated, the driver can
> -send control commands for dynamically changing the coalescing parameters.
> +send commands VIRTIO_NET_CTRL_NOTF_COAL_TX_SET and VIRTIO_NET_CTRL_NOTF_COAL_RX_SET
> +for notification coalescing.
>   
> -\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}
> +If the VIRTIO_NET_F_VQ_NOTF_COAL feature is negotiated, the driver can
> +send commands VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET and VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET
> +for virtqueue notification coalescing.
>   
>   \begin{lstlisting}
>   struct virtio_net_ctrl_coal {
> @@ -1519,25 +1522,63 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
>       le32 max_usecs;
>   };
>   
> +struct virtio_net_ctrl_coal_vq {
> +    le16 vqn;
> +    le16 reserved;
> +    struct virtio_net_ctrl_coal coal;
> +};
> +
>   #define VIRTIO_NET_CTRL_NOTF_COAL 6
>    #define VIRTIO_NET_CTRL_NOTF_COAL_TX_SET  0
>    #define VIRTIO_NET_CTRL_NOTF_COAL_RX_SET 1
> + #define VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET 2
> + #define VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET 3
>   \end{lstlisting}
>   
>   Coalescing parameters:
>   \begin{itemize}
> +\item \field{vqn}: The virtqueue number of an enabled transmit or receive virtqueue.
>   \item \field{max_usecs} for RX: Maximum number of microseconds to delay a RX notification.
>   \item \field{max_usecs} for TX: Maximum number of microseconds to delay a TX notification.
>   \item \field{max_packets} for RX: Maximum number of packets to receive before a RX notification.
>   \item \field{max_packets} for TX: Maximum number of packets to send before a TX notification.
>   \end{itemize}
>   
> -The class VIRTIO_NET_CTRL_NOTF_COAL has 2 commands:
> +\field{reserved} is reserved and it is ignored by a device.
> +
> +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, the structure virtio_net_ctrl_coal is write-only for a driver.
> +\item For the command VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET, the structure virtio_net_ctrl_coal_vq is 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 the structure virtio_net_ctrl_coal is read-only for the driver.
> +\end{itemize}
> +
> +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}
>   
> +The device may generate notifications more or less frequently than specified by set commands of the VIRTIO_NET_CTRL_NOTF_COAL class.
> +
> +If coalescing parameters are being set, the device applies the last coalescing parameters set for a
> +virtqueue, regardless of the 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 virtqueues having vqn 0 and vqn 2. Virtqueues having vqn 1 and vqn 3 retain their previous parameters.
> +\item Command2: VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET with \field{vqn} = 0 sets coalescing parameters for virtqueue having vqn 0. Virtqueue having vqn 2 retains the parameters from command1.
> +\item Command3: VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET with \field{vqn} = 0, the device responds with coalescing parameters of vqn 0 set by command2.
> +\item Command4: VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET with \field{vqn} = 1 sets coalescing parameters for virtqueue having vqn 1. Virtqueue having vqn 3 retains its previous parameters.
> +\item Command5: VIRTIO_NET_CTRL_NOTF_COAL_TX_SET sets coalescing parameters for virtqueues having vqn 1 and vqn 3, and overrides the parameters set by command4.
> +\item Command6: VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET with \field{vqn} = 1, the device responds with coalescing parameters of vqn 1 set by command5.
> +\end{itemize}
> +
>   \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,14 +1626,33 @@ \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.
> +
> +Upon disabling and re-enabling the transmit virtqueue, the device MUST set the coalescing parameters of the virtqueue
> +to those configured through the VIRTIO_NET_CTRL_NOTF_COAL_TX_SET command, or, if the driver did not set any TX coalescing parameters, to 0.
> +
> +Upon disabling and re-enabling the receive virtqueue, the device MUST set the coalescing parameters of the virtqueue
> +to those configured through the VIRTIO_NET_CTRL_NOTF_COAL_RX_SET command, or, if the driver did not set any 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.
>   
> +The behavior of the device in response to set commands of the VIRTIO_NET_CTRL_NOTF_COAL class is best-effort:
> +the device MAY generate notifications more or less frequently than specified.
> +
>   Upon reset, a device MUST initialize all coalescing parameters to 0.
>   
>   \subsubsection{Legacy Interface: Framing Requirements}\label{sec:Device


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] [PATCH v12] virtio-net: support the virtqueue coalescing moderation

[Cornelia Huck]

On Sun, Mar 12 2023, Heng Qi <hengqi@linux.alibaba.com> wrote:

> 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.
>
> [1] https://docs.kernel.org/networking/net_dim.html
>
> Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
> Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> Reviewed-by: Parav Pandit <parav@nvidia.com>

[Apologies for only reviewing this right now]

> @@ -1519,25 +1522,63 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
>      le32 max_usecs;
>  };
>  
> +struct virtio_net_ctrl_coal_vq {
> +    le16 vqn;
> +    le16 reserved;
> +    struct virtio_net_ctrl_coal coal;
> +};
> +
>  #define VIRTIO_NET_CTRL_NOTF_COAL 6
>   #define VIRTIO_NET_CTRL_NOTF_COAL_TX_SET  0
>   #define VIRTIO_NET_CTRL_NOTF_COAL_RX_SET 1
> + #define VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET 2
> + #define VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET 3
>  \end{lstlisting}
>  
>  Coalescing parameters:
>  \begin{itemize}
> +\item \field{vqn}: The virtqueue number of an enabled transmit or receive virtqueue.

As the commands obviously cannot target a control vq, I think we need a
statement on what the device is supposed to be doing if the driver puts
the number of the control q (or indeed an invalid number) here?

>  \item \field{max_usecs} for RX: Maximum number of microseconds to delay a RX notification.
>  \item \field{max_usecs} for TX: Maximum number of microseconds to delay a TX notification.
>  \item \field{max_packets} for RX: Maximum number of packets to receive before a RX notification.
>  \item \field{max_packets} for TX: Maximum number of packets to send before a TX notification.
>  \end{itemize}
>  
> -The class VIRTIO_NET_CTRL_NOTF_COAL has 2 commands:
> +\field{reserved} is reserved and it is ignored by a device.

s/a/the/

> +
> +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, the structure virtio_net_ctrl_coal is write-only for a driver.

s/a/the/

> +\item For the command VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET, the structure virtio_net_ctrl_coal_vq is write-only for a driver.

s/a/the/

> +\item For the command VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET, \field{vqn} and \field{reserved} are write-only
> +      for a driver, and the structure virtio_net_ctrl_coal is read-only for the driver.

s/a/the/ (otherwise, this is kind of inconsistent)

> +\end{itemize}
> +
> +The class VIRTIO_NET_CTRL_NOTF_COAL has 4 commands:

Instead of giving the exact number of commands, maybe use "the following
commands" instead?

>  \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}
>  
> +The device may generate notifications more or less frequently than specified by set commands of the VIRTIO_NET_CTRL_NOTF_COAL class.
> +
> +If coalescing parameters are being set, the device applies the last coalescing parameters set for a
> +virtqueue, regardless of the command used to set the parameters. Use the following command sequence
> +with 2 pairs of virtqueues as an example:

s/2/two/

> +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 virtqueues having vqn 0 and vqn 2. Virtqueues having vqn 1 and vqn 3 retain their previous parameters.
> +\item Command2: VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET with \field{vqn} = 0 sets coalescing parameters for virtqueue having vqn 0. Virtqueue having vqn 2 retains the parameters from command1.
> +\item Command3: VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET with \field{vqn} = 0, the device responds with coalescing parameters of vqn 0 set by command2.
> +\item Command4: VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET with \field{vqn} = 1 sets coalescing parameters for virtqueue having vqn 1. Virtqueue having vqn 3 retains its previous parameters.
> +\item Command5: VIRTIO_NET_CTRL_NOTF_COAL_TX_SET sets coalescing parameters for virtqueues having vqn 1 and vqn 3, and overrides the parameters set by command4.
> +\item Command6: VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET with \field{vqn} = 1, the device responds with coalescing parameters of vqn 1 set by command5.
> +\end{itemize}
> +
>  \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,14 +1626,33 @@ \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.

s/VIRIO/VIRTIO/

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

s/VIRIO/VIRTIO/

I'm not sure whether we should be expressing this via 'before' -- it's
not like the driver can negotiate the feature bit if it realizes later
that it wants to issue the command. IOW, I'd prefer the 'not negotiated'
-> 'MUST NOT issue' construct, but I'm not dead set on it.

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

Do we need to mandate here that the driver MUST NOT put anything else
but the number of an enabled transmit or receive virtqueue into the vqn
field?

(Generally, I'd prefer "The driver" instead of "A driver" as well, but
that might be a matter of taste.)

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

s/given/designated/ ?

What should the device do if the virtqueue is invalid (i.e. it is the
control vq?) I think we either need to add a statement explicitly
forbidding that to the driver conformance section, or specify that the
device MUST return an error here.

> +
> +Upon disabling and re-enabling the transmit virtqueue, the device MUST set the coalescing parameters of the virtqueue
> +to those configured through the VIRTIO_NET_CTRL_NOTF_COAL_TX_SET command, or, if the driver did not set any TX coalescing parameters, to 0.

"a transmit virtqueue" ?

> +
> +Upon disabling and re-enabling the receive virtqueue, the device MUST set the coalescing parameters of the virtqueue
> +to those configured through the VIRTIO_NET_CTRL_NOTF_COAL_RX_SET command, or, if the driver did not set any RX coalescing parameters, to 0.

"a receive virtqueue" ?

>  
>  A device SHOULD NOT send used buffer notifications to the driver if the notifications are suppressed, even if the notification conditions are met.

Generally, I'd prefer "The device" here as well.

>  
> +The behavior of the device in response to set commands of the VIRTIO_NET_CTRL_NOTF_COAL class is best-effort:
> +the device MAY generate notifications more or less frequently than specified.
> +
>  Upon reset, a device MUST initialize all coalescing parameters to 0.
>  
>  \subsubsection{Legacy Interface: Framing Requirements}\label{sec:Device

There are a couple of typos and some style issues here which we could
fix with an update on top, but I'd really like some clarity regarding
invalid virtqueues first.


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] [PATCH v12] virtio-net: support the virtqueue coalescing moderation

[Michael S. Tsirkin]

On Mon, Mar 20, 2023 at 05:35:38PM +0100, Cornelia Huck wrote:
> On Sun, Mar 12 2023, Heng Qi <hengqi@linux.alibaba.com> wrote:
> 
> > 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.
> >
> > [1] https://docs.kernel.org/networking/net_dim.html
> >
> > Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
> > Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > Reviewed-by: Parav Pandit <parav@nvidia.com>
> 
> [Apologies for only reviewing this right now]
> 
> > @@ -1519,25 +1522,63 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
> >      le32 max_usecs;
> >  };
> >  
> > +struct virtio_net_ctrl_coal_vq {
> > +    le16 vqn;
> > +    le16 reserved;
> > +    struct virtio_net_ctrl_coal coal;
> > +};
> > +
> >  #define VIRTIO_NET_CTRL_NOTF_COAL 6
> >   #define VIRTIO_NET_CTRL_NOTF_COAL_TX_SET  0
> >   #define VIRTIO_NET_CTRL_NOTF_COAL_RX_SET 1
> > + #define VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET 2
> > + #define VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET 3
> >  \end{lstlisting}
> >  
> >  Coalescing parameters:
> >  \begin{itemize}
> > +\item \field{vqn}: The virtqueue number of an enabled transmit or receive virtqueue.
> 
> As the commands obviously cannot target a control vq, I think we need a
> statement on what the device is supposed to be doing if the driver puts
> the number of the control q (or indeed an invalid number) here?


Not necessarily, we don't always require input validation. It's enough
to forbid driver from doing this (as you suggest below).

> >  \item \field{max_usecs} for RX: Maximum number of microseconds to delay a RX notification.
> >  \item \field{max_usecs} for TX: Maximum number of microseconds to delay a TX notification.
> >  \item \field{max_packets} for RX: Maximum number of packets to receive before a RX notification.
> >  \item \field{max_packets} for TX: Maximum number of packets to send before a TX notification.
> >  \end{itemize}
> >  
> > -The class VIRTIO_NET_CTRL_NOTF_COAL has 2 commands:
> > +\field{reserved} is reserved and it is ignored by a device.
> 
> s/a/the/
> 
> > +
> > +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, the structure virtio_net_ctrl_coal is write-only for a driver.
> 
> s/a/the/
> 
> > +\item For the command VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET, the structure virtio_net_ctrl_coal_vq is write-only for a driver.
> 
> s/a/the/
> 
> > +\item For the command VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET, \field{vqn} and \field{reserved} are write-only
> > +      for a driver, and the structure virtio_net_ctrl_coal is read-only for the driver.
> 
> s/a/the/ (otherwise, this is kind of inconsistent)
> 
> > +\end{itemize}
> > +
> > +The class VIRTIO_NET_CTRL_NOTF_COAL has 4 commands:
> 
> Instead of giving the exact number of commands, maybe use "the following
> commands" instead?
> 
> >  \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}
> >  
> > +The device may generate notifications more or less frequently than specified by set commands of the VIRTIO_NET_CTRL_NOTF_COAL class.
> > +
> > +If coalescing parameters are being set, the device applies the last coalescing parameters set for a
> > +virtqueue, regardless of the command used to set the parameters. Use the following command sequence
> > +with 2 pairs of virtqueues as an example:
> 
> s/2/two/
> 
> > +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 virtqueues having vqn 0 and vqn 2. Virtqueues having vqn 1 and vqn 3 retain their previous parameters.
> > +\item Command2: VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET with \field{vqn} = 0 sets coalescing parameters for virtqueue having vqn 0. Virtqueue having vqn 2 retains the parameters from command1.
> > +\item Command3: VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET with \field{vqn} = 0, the device responds with coalescing parameters of vqn 0 set by command2.
> > +\item Command4: VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET with \field{vqn} = 1 sets coalescing parameters for virtqueue having vqn 1. Virtqueue having vqn 3 retains its previous parameters.
> > +\item Command5: VIRTIO_NET_CTRL_NOTF_COAL_TX_SET sets coalescing parameters for virtqueues having vqn 1 and vqn 3, and overrides the parameters set by command4.
> > +\item Command6: VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET with \field{vqn} = 1, the device responds with coalescing parameters of vqn 1 set by command5.
> > +\end{itemize}
> > +
> >  \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,14 +1626,33 @@ \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.
> 
> s/VIRIO/VIRTIO/
> 
> > +
> > +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.
> 
> s/VIRIO/VIRTIO/
> 
> I'm not sure whether we should be expressing this via 'before' -- it's
> not like the driver can negotiate the feature bit if it realizes later
> that it wants to issue the command. IOW, I'd prefer the 'not negotiated'
> -> 'MUST NOT issue' construct, but I'm not dead set on it.

I don't like double negation myself. Don't do A if you don't do B is
less clear than "Do B before A".

> 
> > +
> > +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.
> 
> Do we need to mandate here that the driver MUST NOT put anything else
> but the number of an enabled transmit or receive virtqueue into the vqn
> field?


I think it's a good idea.

> (Generally, I'd prefer "The driver" instead of "A driver" as well, but
> that might be a matter of taste.)


Check the surrounding text, and follow that example.

> >  
> >  \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.
> 
> s/given/designated/ ?
> 
> What should the device do if the virtqueue is invalid (i.e. it is the
> control vq?) I think we either need to add a statement explicitly
> forbidding that to the driver conformance section, or specify that the
> device MUST return an error here.

I prefer forbidding this from driver. Device should be free to
handle this in a variety of ways.


> > +
> > +Upon disabling and re-enabling the transmit virtqueue, the device MUST set the coalescing parameters of the virtqueue
> > +to those configured through the VIRTIO_NET_CTRL_NOTF_COAL_TX_SET command, or, if the driver did not set any TX coalescing parameters, to 0.
> 
> "a transmit virtqueue" ?
> 
> > +
> > +Upon disabling and re-enabling the receive virtqueue, the device MUST set the coalescing parameters of the virtqueue
> > +to those configured through the VIRTIO_NET_CTRL_NOTF_COAL_RX_SET command, or, if the driver did not set any RX coalescing parameters, to 0.
> 
> "a receive virtqueue" ?
> 
> >  
> >  A device SHOULD NOT send used buffer notifications to the driver if the notifications are suppressed, even if the notification conditions are met.
> 
> Generally, I'd prefer "The device" here as well.
> 
> >  
> > +The behavior of the device in response to set commands of the VIRTIO_NET_CTRL_NOTF_COAL class is best-effort:
> > +the device MAY generate notifications more or less frequently than specified.
> > +
> >  Upon reset, a device MUST initialize all coalescing parameters to 0.
> >  
> >  \subsubsection{Legacy Interface: Framing Requirements}\label{sec:Device
> 
> There are a couple of typos and some style issues here which we could
> fix with an update on top, but I'd really like some clarity regarding
> invalid virtqueues first.


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] [PATCH v12] virtio-net: support the virtqueue coalescing moderation

[Heng Qi]

在 2023/3/21 上午12:35, Cornelia Huck 写道:
> On Sun, Mar 12 2023, Heng Qi <hengqi@linux.alibaba.com> wrote:
>
>> 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.
>>
>> [1] https://docs.kernel.org/networking/net_dim.html
>>
>> Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
>> Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
>> Reviewed-by: Parav Pandit <parav@nvidia.com>
> [Apologies for only reviewing this right now]
>
>> @@ -1519,25 +1522,63 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
>>       le32 max_usecs;
>>   };
>>   
>> +struct virtio_net_ctrl_coal_vq {
>> +    le16 vqn;
>> +    le16 reserved;
>> +    struct virtio_net_ctrl_coal coal;
>> +};
>> +
>>   #define VIRTIO_NET_CTRL_NOTF_COAL 6
>>    #define VIRTIO_NET_CTRL_NOTF_COAL_TX_SET  0
>>    #define VIRTIO_NET_CTRL_NOTF_COAL_RX_SET 1
>> + #define VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET 2
>> + #define VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET 3
>>   \end{lstlisting}
>>   
>>   Coalescing parameters:
>>   \begin{itemize}
>> +\item \field{vqn}: The virtqueue number of an enabled transmit or receive virtqueue.
> As the commands obviously cannot target a control vq, I think we need a
> statement on what the device is supposed to be doing if the driver puts
> the number of the control q (or indeed an invalid number) here?

"of an enabled transmit or receive virtqueue" has clearly ruled out 
control vq.
As you said, we should make it clear in the normative statement that the 
driver must not send vq numbers other transmit vqs and receive vqs.

>
>>   \item \field{max_usecs} for RX: Maximum number of microseconds to delay a RX notification.
>>   \item \field{max_usecs} for TX: Maximum number of microseconds to delay a TX notification.
>>   \item \field{max_packets} for RX: Maximum number of packets to receive before a RX notification.
>>   \item \field{max_packets} for TX: Maximum number of packets to send before a TX notification.
>>   \end{itemize}
>>   
>> -The class VIRTIO_NET_CTRL_NOTF_COAL has 2 commands:
>> +\field{reserved} is reserved and it is ignored by a device.
> s/a/the/

I think "a" should be used for the first occurrence, and "the" is used 
to refer to the one that has already appeared, am I correct :) ?

>
>> +
>> +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, the structure virtio_net_ctrl_coal is write-only for a driver.
> s/a/the/
>
>> +\item For the command VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET, the structure virtio_net_ctrl_coal_vq is write-only for a driver.
> s/a/the/
>
>> +\item For the command VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET, \field{vqn} and \field{reserved} are write-only
>> +      for a driver, and the structure virtio_net_ctrl_coal is read-only for the driver.
> s/a/the/ (otherwise, this is kind of inconsistent)
>
>> +\end{itemize}
>> +
>> +The class VIRTIO_NET_CTRL_NOTF_COAL has 4 commands:
> Instead of giving the exact number of commands, maybe use "the following
> commands" instead?

Sure.

>
>>   \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}
>>   
>> +The device may generate notifications more or less frequently than specified by set commands of the VIRTIO_NET_CTRL_NOTF_COAL class.
>> +
>> +If coalescing parameters are being set, the device applies the last coalescing parameters set for a
>> +virtqueue, regardless of the command used to set the parameters. Use the following command sequence
>> +with 2 pairs of virtqueues as an example:
> s/2/two/

Yes, but I don't understand why this is important, it comes up elsewhere.

>
>> +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 virtqueues having vqn 0 and vqn 2. Virtqueues having vqn 1 and vqn 3 retain their previous parameters.
>> +\item Command2: VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET with \field{vqn} = 0 sets coalescing parameters for virtqueue having vqn 0. Virtqueue having vqn 2 retains the parameters from command1.
>> +\item Command3: VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET with \field{vqn} = 0, the device responds with coalescing parameters of vqn 0 set by command2.
>> +\item Command4: VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET with \field{vqn} = 1 sets coalescing parameters for virtqueue having vqn 1. Virtqueue having vqn 3 retains its previous parameters.
>> +\item Command5: VIRTIO_NET_CTRL_NOTF_COAL_TX_SET sets coalescing parameters for virtqueues having vqn 1 and vqn 3, and overrides the parameters set by command4.
>> +\item Command6: VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET with \field{vqn} = 1, the device responds with coalescing parameters of vqn 1 set by command5.
>> +\end{itemize}
>> +
>>   \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,14 +1626,33 @@ \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.
> s/VIRIO/VIRTIO/

I will fix it.

>
>> +
>> +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.
> s/VIRIO/VIRTIO/
>
> I'm not sure whether we should be expressing this via 'before' -- it's
> not like the driver can negotiate the feature bit if it realizes later
> that it wants to issue the command. IOW, I'd prefer the 'not negotiated'
> -> 'MUST NOT issue' construct, but I'm not dead set on it.

The previous version was a double negative, but I think that statement 
seems clearer now.
Because we know that double negation may cause confusion:
when A does not happen, B must not happen, but when A happens, does it 
mean that B can happen? Maybe B can't happen either.

>
>> +
>> +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.
> Do we need to mandate here that the driver MUST NOT put anything else
> but the number of an enabled transmit or receive virtqueue into the vqn
> field?

Yes, you are right. I agree.

>
> (Generally, I'd prefer "The driver" instead of "A driver" as well, but
> that might be a matter of taste.)
>
>>   
>>   \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.
> s/given/designated/ ?

Ok.

>
> What should the device do if the virtqueue is invalid (i.e. it is the
> control vq?) I think we either need to add a statement explicitly
> forbidding that to the driver conformance section, or specify that the
> device MUST return an error here.

Yes, I would prefer to let the driver do this.

>
>> +
>> +Upon disabling and re-enabling the transmit virtqueue, the device MUST set the coalescing parameters of the virtqueue
>> +to those configured through the VIRTIO_NET_CTRL_NOTF_COAL_TX_SET command, or, if the driver did not set any TX coalescing parameters, to 0.
> "a transmit virtqueue" ?

I'm a bit confused, can you explain more? Because in the example below 
you say "s/a driver/the driver".

     +\item For the command VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET, \field{vqn} and \field{reserved} are write-only
     +      for a driver, and the structure virtio_net_ctrl_coal is read-only for the driver.


>
>> +
>> +Upon disabling and re-enabling the receive virtqueue, the device MUST set the coalescing parameters of the virtqueue
>> +to those configured through the VIRTIO_NET_CTRL_NOTF_COAL_RX_SET command, or, if the driver did not set any RX coalescing parameters, to 0.
> "a receive virtqueue" ?
>
>>   
>>   A device SHOULD NOT send used buffer notifications to the driver if the notifications are suppressed, even if the notification conditions are met.
> Generally, I'd prefer "The device" here as well.
>
>>   
>> +The behavior of the device in response to set commands of the VIRTIO_NET_CTRL_NOTF_COAL class is best-effort:
>> +the device MAY generate notifications more or less frequently than specified.
>> +
>>   Upon reset, a device MUST initialize all coalescing parameters to 0.
>>   
>>   \subsubsection{Legacy Interface: Framing Requirements}\label{sec:Device
> There are a couple of typos and some style issues here which we could
> fix with an update on top, but I'd really like some clarity regarding
> invalid virtqueues first.

Yes, I think you are right. I see that the voting has already started, 
do I need to wait for the voting to end and repost a new version based 
on the results?

Thanks.



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] [PATCH v12] virtio-net: support the virtqueue coalescing moderation

[Cornelia Huck]

On Tue, Mar 21 2023, Heng Qi <hengqi@linux.alibaba.com> wrote:

> 在 2023/3/21 上午12:35, Cornelia Huck 写道:
>> On Sun, Mar 12 2023, Heng Qi <hengqi@linux.alibaba.com> wrote:
>>>   \item \field{max_usecs} for RX: Maximum number of microseconds to delay a RX notification.
>>>   \item \field{max_usecs} for TX: Maximum number of microseconds to delay a TX notification.
>>>   \item \field{max_packets} for RX: Maximum number of packets to receive before a RX notification.
>>>   \item \field{max_packets} for TX: Maximum number of packets to send before a TX notification.
>>>   \end{itemize}
>>>   
>>> -The class VIRTIO_NET_CTRL_NOTF_COAL has 2 commands:
>>> +\field{reserved} is reserved and it is ignored by a device.
>> s/a/the/
>
> I think "a" should be used for the first occurrence, and "the" is used 
> to refer to the one that has already appeared, am I correct :) ?

IMHO, we are talking about a concrete device implementing this, so it
should be "the" :) Not a native speaker, but "it is ignored by a device"
sounds like it refers to a random device, and not the concrete one
which will actually ignore it.

>>> +If coalescing parameters are being set, the device applies the last coalescing parameters set for a
>>> +virtqueue, regardless of the command used to set the parameters. Use the following command sequence
>>> +with 2 pairs of virtqueues as an example:
>> s/2/two/
>
> Yes, but I don't understand why this is important, it comes up elsewhere.

I remember style guidance that you should spell out small numbers,
instead of using digits. Not really important, but might be worth
addressing if this needs a respin anyway.

>>> +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.
>> s/VIRIO/VIRTIO/
>>
>> I'm not sure whether we should be expressing this via 'before' -- it's
>> not like the driver can negotiate the feature bit if it realizes later
>> that it wants to issue the command. IOW, I'd prefer the 'not negotiated'
>> -> 'MUST NOT issue' construct, but I'm not dead set on it.
>
> The previous version was a double negative, but I think that statement 
> seems clearer now.
> Because we know that double negation may cause confusion:
> when A does not happen, B must not happen, but when A happens, does it 
> mean that B can happen? Maybe B can't happen either.

Maybe make this "MUST have negotiated (...) when issuing"?

>
>>
>>> +
>>> +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.
>> Do we need to mandate here that the driver MUST NOT put anything else
>> but the number of an enabled transmit or receive virtqueue into the vqn
>> field?
>
> Yes, you are right. I agree.
>
>>
>> (Generally, I'd prefer "The driver" instead of "A driver" as well, but
>> that might be a matter of taste.)
>>
>>>   
>>>   \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.
>> s/given/designated/ ?
>
> Ok.
>
>>
>> What should the device do if the virtqueue is invalid (i.e. it is the
>> control vq?) I think we either need to add a statement explicitly
>> forbidding that to the driver conformance section, or specify that the
>> device MUST return an error here.
>
> Yes, I would prefer to let the driver do this.

Ok, sounds good.

>
>>
>>> +
>>> +Upon disabling and re-enabling the transmit virtqueue, the device MUST set the coalescing parameters of the virtqueue
>>> +to those configured through the VIRTIO_NET_CTRL_NOTF_COAL_TX_SET command, or, if the driver did not set any TX coalescing parameters, to 0.
>> "a transmit virtqueue" ?
>
> I'm a bit confused, can you explain more? Because in the example below 
> you say "s/a driver/the driver".
>
>      +\item For the command VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET, \field{vqn} and \field{reserved} are write-only
>      +      for a driver, and the structure virtio_net_ctrl_coal is read-only for the driver.

It is one of the transmit virtqueues (i.e. a random one, not a concrete
one) that is getting disabled and re-enabled, but then it is that
concrete virtqueue that needs to be configured correctly.

In the second example, we're talking about a concrete driver
implementing this, so it should be "the".

(Native or close-to-native speakers, please correct me if I'm wrong! I
don't want to get into language lawyering, but I stumbled over this
while reading.)

>> There are a couple of typos and some style issues here which we could
>> fix with an update on top, but I'd really like some clarity regarding
>> invalid virtqueues first.
>
> Yes, I think you are right. I see that the voting has already started, 
> do I need to wait for the voting to end and repost a new version based 
> on the results?

If you plan to respin this, you can request to withdraw the vote, and we
can restart voting on the updated version. If voting were to close with
the change being approved, we'd need to include it and then vote on an
incremental change on top. (I'd prefer to go with the first option;
incremental changes on top are better suited to simple spelling fixes
that don't need another round of voting.)


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] [PATCH v12] virtio-net: support the virtqueue coalescing moderation

[Michael S. Tsirkin]

On Tue, Mar 21, 2023 at 10:41:55AM +0800, Heng Qi wrote:
> Yes, I think you are right. I see that the voting has already started, do I
> need to wait for the voting to end and repost a new version based on the
> results?

We can always cancel the vote, just ask.

-- 
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] [PATCH v12] virtio-net: support the virtqueue coalescing moderation

[Heng Qi]

在 2023/3/21 下午5:29, Cornelia Huck 写道:
> On Tue, Mar 21 2023, Heng Qi <hengqi@linux.alibaba.com> wrote:
>
>> 在 2023/3/21 上午12:35, Cornelia Huck 写道:
>>> On Sun, Mar 12 2023, Heng Qi <hengqi@linux.alibaba.com> wrote:
>>>>    \item \field{max_usecs} for RX: Maximum number of microseconds to delay a RX notification.
>>>>    \item \field{max_usecs} for TX: Maximum number of microseconds to delay a TX notification.
>>>>    \item \field{max_packets} for RX: Maximum number of packets to receive before a RX notification.
>>>>    \item \field{max_packets} for TX: Maximum number of packets to send before a TX notification.
>>>>    \end{itemize}
>>>>    
>>>> -The class VIRTIO_NET_CTRL_NOTF_COAL has 2 commands:
>>>> +\field{reserved} is reserved and it is ignored by a device.
>>> s/a/the/
>> I think "a" should be used for the first occurrence, and "the" is used
>> to refer to the one that has already appeared, am I correct :) ?
> IMHO, we are talking about a concrete device implementing this, so it
> should be "the" :) Not a native speaker, but "it is ignored by a device"
> sounds like it refers to a random device, and not the concrete one
> which will actually ignore it.

Combined with your explanation of "a/the" queue, I believe I understand! 
Thanks :)

>
>>>> +If coalescing parameters are being set, the device applies the last coalescing parameters set for a
>>>> +virtqueue, regardless of the command used to set the parameters. Use the following command sequence
>>>> +with 2 pairs of virtqueues as an example:
>>> s/2/two/
>> Yes, but I don't understand why this is important, it comes up elsewhere.
> I remember style guidance that you should spell out small numbers,
> instead of using digits. Not really important, but might be worth
> addressing if this needs a respin anyway.

Will fix.

>
>>>> +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.
>>> s/VIRIO/VIRTIO/
>>>
>>> I'm not sure whether we should be expressing this via 'before' -- it's
>>> not like the driver can negotiate the feature bit if it realizes later
>>> that it wants to issue the command. IOW, I'd prefer the 'not negotiated'
>>> -> 'MUST NOT issue' construct, but I'm not dead set on it.
>> The previous version was a double negative, but I think that statement
>> seems clearer now.
>> Because we know that double negation may cause confusion:
>> when A does not happen, B must not happen, but when A happens, does it
>> mean that B can happen? Maybe B can't happen either.
> Maybe make this "MUST have negotiated (...) when issuing"?

Ok, I think it works.

>
>>>> +
>>>> +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.
>>> Do we need to mandate here that the driver MUST NOT put anything else
>>> but the number of an enabled transmit or receive virtqueue into the vqn
>>> field?
>> Yes, you are right. I agree.
>>
>>> (Generally, I'd prefer "The driver" instead of "A driver" as well, but
>>> that might be a matter of taste.)
>>>
>>>>    
>>>>    \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.
>>> s/given/designated/ ?
>> Ok.
>>
>>> What should the device do if the virtqueue is invalid (i.e. it is the
>>> control vq?) I think we either need to add a statement explicitly
>>> forbidding that to the driver conformance section, or specify that the
>>> device MUST return an error here.
>> Yes, I would prefer to let the driver do this.
> Ok, sounds good.
>
>>>> +
>>>> +Upon disabling and re-enabling the transmit virtqueue, the device MUST set the coalescing parameters of the virtqueue
>>>> +to those configured through the VIRTIO_NET_CTRL_NOTF_COAL_TX_SET command, or, if the driver did not set any TX coalescing parameters, to 0.
>>> "a transmit virtqueue" ?
>> I'm a bit confused, can you explain more? Because in the example below
>> you say "s/a driver/the driver".
>>
>>       +\item For the command VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET, \field{vqn} and \field{reserved} are write-only
>>       +      for a driver, and the structure virtio_net_ctrl_coal is read-only for the driver.
> It is one of the transmit virtqueues (i.e. a random one, not a concrete
> one) that is getting disabled and re-enabled, but then it is that
> concrete virtqueue that needs to be configured correctly.
>
> In the second example, we're talking about a concrete driver
> implementing this, so it should be "the".
>
> (Native or close-to-native speakers, please correct me if I'm wrong! I
> don't want to get into language lawyering, but I stumbled over this
> while reading.)
>
>>> There are a couple of typos and some style issues here which we could
>>> fix with an update on top, but I'd really like some clarity regarding
>>> invalid virtqueues first.
>> Yes, I think you are right. I see that the voting has already started,
>> do I need to wait for the voting to end and repost a new version based
>> on the results?
> If you plan to respin this, you can request to withdraw the vote, and we
> can restart voting on the updated version. If voting were to close with
> the change being approved, we'd need to include it and then vote on an
> incremental change on top. (I'd prefer to go with the first option;
> incremental changes on top are better suited to simple spelling fixes
> that don't need another round of voting.)

Ok, I got it.

May I bother you please to withdraw the vote, I will modify it according 
to your opinion and release a new version!:)

Thank you very much!



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] [PATCH v12] virtio-net: support the virtqueue coalescing moderation

[Cornelia Huck]

On Tue, Mar 21 2023, Heng Qi <hengqi@linux.alibaba.com> wrote:

> May I bother you please to withdraw the vote, I will modify it according 
> to your opinion and release a new version!:)

Sure, done.


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/