virtio-dev.lists.oasis-open.org archive mirror
 help / color / mirror / Atom feed
* [virtio-dev] [PATCH v11] virtio-net: support the virtqueue coalescing moderation
@ 2023-03-09 13:05 Heng Qi
  2023-03-09 18:53 ` [virtio-dev] " Parav Pandit
  0 siblings, 1 reply; 5+ messages in thread
From: Heng Qi @ 2023-03-09 13:05 UTC (permalink / raw)
  To: virtio-dev, virtio-comment
  Cc: Michael S . Tsirkin, Parav Pandit, Alvaro Karsz, David Edmondson,
	Jason Wang, Xuan Zhuo

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>
---
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 | 86 ++++++++++++++++++++++++++++----
 1 file changed, 75 insertions(+), 11 deletions(-)

diff --git a/device-types/net/description.tex b/device-types/net/description.tex
index e71e33b..c896485 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}
@@ -1505,13 +1508,13 @@ \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.
+If the VIRTIO_NET_F_NOTF_COAL feature is negotiated, a driver can 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, a 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,69 @@ \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 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.
+
 \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 +1632,31 @@ \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_SET 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.
 
+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


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


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [virtio-dev] Re: [PATCH v11] virtio-net: support the virtqueue coalescing moderation
  2023-03-09 13:05 [virtio-dev] [PATCH v11] virtio-net: support the virtqueue coalescing moderation Heng Qi
@ 2023-03-09 18:53 ` Parav Pandit
  2023-03-09 19:18   ` Michael S. Tsirkin
  2023-03-10  5:18   ` [virtio-dev] " Heng Qi
  0 siblings, 2 replies; 5+ messages in thread
From: Parav Pandit @ 2023-03-09 18:53 UTC (permalink / raw)
  To: Heng Qi, virtio-dev, virtio-comment
  Cc: Michael S . Tsirkin, Alvaro Karsz, David Edmondson, Jason Wang,
	Xuan Zhuo



On 3/9/2023 8:05 AM, Heng Qi 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>
[..]
>   device-types/net/description.tex | 86 ++++++++++++++++++++++++++++----
>   1 file changed, 75 insertions(+), 11 deletions(-)
> 
> diff --git a/device-types/net/description.tex b/device-types/net/description.tex
> index e71e33b..c896485 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}
> @@ -1505,13 +1508,13 @@ \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.
> +If the VIRTIO_NET_F_NOTF_COAL feature is negotiated, a driver can send
Above text had "the driver".
Better to not change it.

dynamically changing got dropped, better to keep it.

> +commands VIRTIO_NET_CTRL_NOTF_COAL_TX_SET and VIRTIO_NET_CTRL_NOTF_COAL_RX_SET
> +for notification coalescing.

>   

[..]

> +\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.
for a driver and the structure ..
drop extra ",".

> +\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 virtqueue0 and virtqueue2, and, virtqueue1 and virtqueue3 retain their previous parameter values.
above "and" is confusing. It implies set on vq0,vq2 and vq1, vq3.
Same for below Command2 as well.
It also introduces new terms as virtqueue0 and virtqueueN.
It is better to avoid.

Rewriting it as below avoids creating new terms and avoids confusion 
around "and".

Command1: VIRTIO_NET_CTRL_NOTF_COAL_RX_SET sets coalescing parameters 
for virtqueues having vqn 0 and vqn 2. Virtqueues having vqn 1 and 3 
retain their previous parameters.

> +\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.
Command2: VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET with \field{vqn} sets 
coalescing parameters for the virtuqueue 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 virtqueue0 set by command2.
parameters of vqn 0 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.for vqn 1. Virtqueue having vqn 3 retains its previous parameters.
> +\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.
> +
s/if the command/if the driver/

> +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.
> +
s/if the command/if the driver/
>   \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 +1632,31 @@ \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_SET command, or, if the command
s/if the command/if the driver/
> +did not configure TX/RX coalescing parameters, to 0.

Also the sentence structure should be:
set params to value A configured using method1, else to params B if not 
configured.

Instead currently it is,
set params to value A using method1, else not configured to param B.

So to keep if-else pattern:

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_SET 
command, or to 0 if the driver did not configure TX/RX coalescing 
parameters.

Just like how you wrote it in the non normative places. :)

I still think that disable,re-enable doesnt need to be documented twice.

Keeping it in the conformance section is good enough, specially when the 
text is exact same.

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

Other than above small wordings change, it looks good to me.
Reviewed-by: Parav Pandit <parav@nvidia.com>


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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* [virtio-dev] Re: [PATCH v11] virtio-net: support the virtqueue coalescing moderation
  2023-03-09 18:53 ` [virtio-dev] " Parav Pandit
@ 2023-03-09 19:18   ` Michael S. Tsirkin
  2023-03-09 20:08     ` [virtio-dev] " Parav Pandit
  2023-03-10  5:18   ` [virtio-dev] " Heng Qi
  1 sibling, 1 reply; 5+ messages in thread
From: Michael S. Tsirkin @ 2023-03-09 19:18 UTC (permalink / raw)
  To: Parav Pandit
  Cc: Heng Qi, virtio-dev, virtio-comment, Alvaro Karsz,
	David Edmondson, Jason Wang, Xuan Zhuo

On Thu, Mar 09, 2023 at 01:53:37PM -0500, Parav Pandit wrote:
> 
> 
> On 3/9/2023 8:05 AM, Heng Qi 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>
> [..]
> >   device-types/net/description.tex | 86 ++++++++++++++++++++++++++++----
> >   1 file changed, 75 insertions(+), 11 deletions(-)
> > 
> > diff --git a/device-types/net/description.tex b/device-types/net/description.tex
> > index e71e33b..c896485 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}
> > @@ -1505,13 +1508,13 @@ \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.
> > +If the VIRTIO_NET_F_NOTF_COAL feature is negotiated, a driver can send
> Above text had "the driver".
> Better to not change it.

I don't see why it matters. In most other cases we omit it.

> dynamically changing got dropped, better to keep it.

if you keep that you also need to mention parameters
this is what is changing.

I'm ok with the shorter wording - we have a full description
separate the point here is requirement of VIRTIO_NET_F_NOTF_COAL.


> > +commands VIRTIO_NET_CTRL_NOTF_COAL_TX_SET and VIRTIO_NET_CTRL_NOTF_COAL_RX_SET
> > +for notification coalescing.
> 
> 
> [..]
> 
> > +\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.
> for a driver and the structure ..
> drop extra ",".
> 
> > +\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 virtqueue0 and virtqueue2, and, virtqueue1 and virtqueue3 retain their previous parameter values.
> above "and" is confusing. It implies set on vq0,vq2 and vq1, vq3.
> Same for below Command2 as well.
> It also introduces new terms as virtqueue0 and virtqueueN.
> It is better to avoid.
> 
> Rewriting it as below avoids creating new terms and avoids confusion around
> "and".
> 
> Command1: VIRTIO_NET_CTRL_NOTF_COAL_RX_SET sets coalescing parameters for
> virtqueues having vqn 0 and vqn 2. Virtqueues having vqn 1 and 3 retain
> their previous parameters.
> 
> > +\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.
> Command2: VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET with \field{vqn} sets coalescing
> parameters for the virtuqueue 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 virtqueue0 set by command2.
> parameters of vqn 0 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.for vqn 1. Virtqueue having vqn 3 retains its previous parameters.
> > +\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.
> > +
> s/if the command/if the driver/

please don't just tell people what to do. provide reasoning.


> > +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.
> > +
> s/if the command/if the driver/
> >   \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 +1632,31 @@ \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_SET command, or, if the command
> s/if the command/if the driver/
> > +did not configure TX/RX coalescing parameters, to 0.
> 
> Also the sentence structure should be:
> set params to value A configured using method1, else to params B if not
> configured.
> 
> Instead currently it is,
> set params to value A using method1, else not configured to param B.

I don't see anything wrong with the proposed structure, and the
way you want it with if after else is IMHO weird and hard to read.



> So to keep if-else pattern:
> 
> 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_SET command,
> or to 0 if the driver did not configure TX/RX coalescing parameters.
> 
> Just like how you wrote it in the non normative places. :)
> 

maybe fix non-normative too. If  A then B is simple and clear.
B if A is understandable but is harder to read.

> I still think that disable,re-enable doesnt need to be documented twice.
> Keeping it in the conformance section is good enough, specially when the
> text is exact same.

We have this duplication in lots of other places.
But I agree the text should not be the same,
make the non normative shorter and informal,
with short sentences.

E.g.

	If VQ is disabled, its coalescing values reset to what is set
	by VIRTIO_NET_CTRL_NOTF_COAL_TX_SET and VIRTIO_NET_CTRL_NOTF_COAL_RX_SET
	or if not set then 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
> 
> Other than above small wordings change, it looks good to me.
> Reviewed-by: Parav Pandit <parav@nvidia.com>


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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* [virtio-dev] RE: [PATCH v11] virtio-net: support the virtqueue coalescing moderation
  2023-03-09 19:18   ` Michael S. Tsirkin
@ 2023-03-09 20:08     ` Parav Pandit
  0 siblings, 0 replies; 5+ messages in thread
From: Parav Pandit @ 2023-03-09 20:08 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: Heng Qi, virtio-dev, virtio-comment, Alvaro Karsz,
	David Edmondson, Jason Wang, Xuan Zhuo


> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Thursday, March 9, 2023 2:19 PM
> 
> On Thu, Mar 09, 2023 at 01:53:37PM -0500, Parav Pandit wrote:
> >
> > > 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.
> > > +If the VIRTIO_NET_F_NOTF_COAL feature is negotiated, a driver can
> > > +send
> > Above text had "the driver".
> > Better to not change it.
> 
> I don't see why it matters. In most other cases we omit it.

The hunk that changes article from "the" to "a" doesn't belong to this patch.

> > > +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.
> > > +
> > s/if the command/if the driver/
> 
> please don't just tell people what to do. provide reasoning.
> 
Reason I think is: 
The command is not the object or the actor. 
The device and driver are the object or actor setting the parameters through the command.

> +command, or, if the command
> > s/if the command/if the driver/
> > > +did not configure TX/RX coalescing parameters, to 0.
> >
> > Also the sentence structure should be:
> > set params to value A configured using method1, else to params B if
> > not configured.
> >
> > Instead currently it is,
> > set params to value A using method1, else not configured to param B.
> 
> I don't see anything wrong with the proposed structure, and the way you want it
> with if after else is IMHO weird and hard to read.
> 
> 
> 
> > So to keep if-else pattern:
> >
> > 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 to 0 if the driver did not configure TX/RX coalescing
> parameters.
> >
> > Just like how you wrote it in the non normative places. :)
> >
> 
> maybe fix non-normative too. If  A then B is simple and clear.
> B if A is understandable but is harder to read.
> 
Documenting one way is needed and without exact duplication at two places.

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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* [virtio-dev] Re: [PATCH v11] virtio-net: support the virtqueue coalescing moderation
  2023-03-09 18:53 ` [virtio-dev] " Parav Pandit
  2023-03-09 19:18   ` Michael S. Tsirkin
@ 2023-03-10  5:18   ` Heng Qi
  1 sibling, 0 replies; 5+ messages in thread
From: Heng Qi @ 2023-03-10  5:18 UTC (permalink / raw)
  To: Parav Pandit, virtio-dev, virtio-comment
  Cc: Michael S . Tsirkin, Alvaro Karsz, David Edmondson, Jason Wang,
	Xuan Zhuo


在 2023/3/10 上午2:53, Parav Pandit 写道:
>
>
> On 3/9/2023 8:05 AM, Heng Qi 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>
> [..]
>>   device-types/net/description.tex | 86 ++++++++++++++++++++++++++++----
>>   1 file changed, 75 insertions(+), 11 deletions(-)
>>
>> diff --git a/device-types/net/description.tex 
>> b/device-types/net/description.tex
>> index e71e33b..c896485 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}
>> @@ -1505,13 +1508,13 @@ \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.
>> +If the VIRTIO_NET_F_NOTF_COAL feature is negotiated, a driver can send
> Above text had "the driver".
> Better to not change it.

Ok.

>
> dynamically changing got dropped, better to keep it.

The motivation for this modification comes from Alvaro. We only need to 
say here that
feature A can send B, instead of repeatedly mentioning "action: 
dynamically changing the coalescing parameters."
and "object: for all tx/rx virtqueues or for the virtqueue with vqn" 
every time a command is mentioned.

>
>> +commands VIRTIO_NET_CTRL_NOTF_COAL_TX_SET and 
>> VIRTIO_NET_CTRL_NOTF_COAL_RX_SET
>> +for notification coalescing.
>
>
> [..]
>
>> +\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.
> for a driver and the structure ..
> drop extra ",".

I'll remove it.

>
>> +\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 virtqueue0 and virtqueue2, and, virtqueue1 and 
>> virtqueue3 retain their previous parameter values.
> above "and" is confusing. It implies set on vq0,vq2 and vq1, vq3.
> Same for below Command2 as well.
> It also introduces new terms as virtqueue0 and virtqueueN.
> It is better to avoid.

Indeed, I agree.

>
> Rewriting it as below avoids creating new terms and avoids confusion 
> around "and".
>
> Command1: VIRTIO_NET_CTRL_NOTF_COAL_RX_SET sets coalescing parameters 
> for virtqueues having vqn 0 and vqn 2. Virtqueues having vqn 1 and 3 
> retain their previous parameters.
>

I think it works, thanks for the example.

>> +\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.
> Command2: VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET with \field{vqn} sets 
> coalescing parameters for the virtuqueue 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 virtqueue0 set 
>> by command2.
> parameters of vqn 0 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.for vqn 1. Virtqueue having vqn 3 retains its 
>> previous parameters.
>> +\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.
>> +
> s/if the command/if the driver/
>
>> +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.
>> +
> s/if the command/if the driver/
>> \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 +1632,31 @@ \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_SET 
>> command, or, if the command
> s/if the command/if the driver/
>> +did not configure TX/RX coalescing parameters, to 0.
>
> Also the sentence structure should be:
> set params to value A configured using method1, else to params B if 
> not configured.
>
> Instead currently it is,
> set params to value A using method1, else not configured to param B.
>
> So to keep if-else pattern:
>
> 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_SET 
> command, or to 0 if the driver did not configure TX/RX coalescing 
> parameters.

Ok. Then I think we can move the description of the non-normative part 
here, and keep only one description to avoid repetition. As Michael 
suggested, we keep the 'if then' syntax for readability.

>
> Just like how you wrote it in the non normative places. :)
>
> I still think that disable,re-enable doesnt need to be documented twice.
>
> Keeping it in the conformance section is good enough, specially when 
> the text is exact same.

Sure.

>
>>     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
>
> Other than above small wordings change, it looks good to me.
> Reviewed-by: Parav Pandit <parav@nvidia.com>

Thank you very much! :)



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


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2023-03-10  5:18 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-03-09 13:05 [virtio-dev] [PATCH v11] virtio-net: support the virtqueue coalescing moderation Heng Qi
2023-03-09 18:53 ` [virtio-dev] " Parav Pandit
2023-03-09 19:18   ` Michael S. Tsirkin
2023-03-09 20:08     ` [virtio-dev] " Parav Pandit
2023-03-10  5:18   ` [virtio-dev] " Heng Qi

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).