* [virtio-comment] [PATCH v9] virtio-net: support the virtqueue coalescing moderation
@ 2023-02-26 9:37 Heng Qi
2023-02-26 9:37 ` [virtio-dev] " Heng Qi
` (2 more replies)
0 siblings, 3 replies; 25+ messages in thread
From: Heng Qi @ 2023-02-26 9:37 UTC (permalink / raw)
To: virtio-dev, virtio-comment
Cc: Alvaro Karsz, Michael S . Tsirkin, Parav Pandit, 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>
---
This patch is on top of Alvaro's latest v7 patch: https://lists.oasis-open.org/archives/virtio-dev/202302/msg00431.html .
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 | 95 +++++++++++++++++++++++++++++---
1 file changed, 87 insertions(+), 8 deletions(-)
diff --git a/device-types/net/description.tex b/device-types/net/description.tex
index e71e33b..71f6827 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,8 +1508,17 @@ \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, the driver can send the VIRTIO_NET_CTRL_NOTF_COAL_TX_SET
+and VIRTIO_NET_CTRL_NOTF_COAL_RX_SET commands to dynamically change the coalescing parameters for all transmit
+and receive virtqueues, respectively.
+
+If the VIRTIO_NET_F_VQ_NOTF_COAL feature is negotiated:
+\begin{itemize}
+\item a driver can send a VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET command to set coalescing parameters of a given
+ enabled transmit/receive virtqueue.
+\item a driver can send a VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET command to a device, and the device responds with
+ coalescing parameters of a given enabled transmit/receive virtqueue.
+\end{itemize}
\begin{note}
The behavior of the device in response to these commands is best-effort:
@@ -1519,25 +1531,76 @@ \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}
+The VIRTIO_NET_CTRL_NOTF_COAL_TX_SET and VIRTIO_NET_CTRL_NOTF_COAL_RX_SET commands use the
+virtio_net_ctrl_coal structure to set \field{max_usecs} and \field{max_packets} for all
+transmit/receive virtqueues.
+
+The VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET command uses the virtio_net_ctrl_coal_vq structure
+to set \field{max_usecs} and \field{max_packets} for the supplied virtqueue number \field{vqn}.
+
+The VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET command gets the values of \field{max_usecs} and
+\field{max_packets} of the specified virtqueue from the device by setting \field{vqn}
+in the virtio_net_ctrl_coal_vq structure.
+
+# Read/Write attributes for coalescing parameters
+\begin{itemize}
+\item For commands VIRTIO_NET_CTRL_NOTF_COAL_TX_SET and VIRTIO_NET_CTRL_NOTF_COAL_RX_SET, \field{max_usecs}
+ and \field{max_packets} are write-only for a driver.
+\item For the command VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET, \field{vqn}, \field{reserved}, \field{max_usecs}
+ and \field{max_packets} are write-only for a driver.
+\item For the command VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET, \field{vqn} and \field{reserved} are write-only
+ for a driver, and, \field{max_usecs} and \field{max_packets} are read-only for the driver.
+\end{itemize}
+
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.
+
+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: set the \field{max_usecs} and \field{max_packets} parameters for all transmit virtqueues, and values of
+ coalescing parameters are recorded as TX global values by a device.
+\item VIRTIO_NET_CTRL_NOTF_COAL_RX_SET: set the \field{max_usecs} and \field{max_packets} parameters for all receive virtqueues, and values of
+ coalescing parameters are recorded as RX global values by a device.
+\item VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET: set the \field{max_usecs} and \field{max_packets} parameters for an enabled transmit/receive
+ virtqueue whose number is \field{vqn}, and do not change the TX/RX global values.
+\item VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET: the device returns the \field{max_usecs} and \field{max_packets} parameters for an enabled
+ transmit/receive virtqueue whose number is \field{vqn}.
\end{enumerate}
+If coalescing parameters are being set, the device applies the last coalescing parameters received for a
+virtqueue, regardless of the command used to set the parameters. For example with 2 pairs of virtqueues:
+# Command sequence
+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}
+
\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}.
@@ -1549,6 +1612,11 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
When the device has \field{max_usecs} = 0 or \field{max_packets} = 0, the notification conditions are met after every packet received/sent.
+When the device receives the VIRTIO_NET_CTRL_NOTF_COAL_TX_SET and VIRTIO_NET_CTRL_NOTF_COAL_RX_SET commands,
+it saves the values of coalescing parameters as global values, and the VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET command
+does not change the global values. If the device is reset, the global values will be set to 0.
+When a virtqueue is enabled after virtqueue reset, its coalescing parameters are set to global values.
+
\subparagraph{RX Example}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Notifications Coalescing / RX Example}
If, for example:
@@ -1585,15 +1653,26 @@ \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.
+If neither the VIRTIO_NET_F_NOTF_COAL nor the VIRTIO_NET_F_VQ_NOTF_COAL feature
+has been negotiated, the driver MUST NOT issue VIRTIO_NET_CTRL_NOTF_COAL commands.
+
+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 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.
+
+A device MUST ignore \field{reserved}.
A device SHOULD NOT send used buffer notifications to the driver if the notifications are suppressed, even if the notification conditions are met.
-Upon reset, a device MUST initialize all coalescing parameters to 0.
+After disabling and re-enabling a virtqueue, the device MUST revert coalescing parameters of the virtqueue to the global values.
+
+Upon reset, a device MUST initialize all coalescing parameters to 0 and MUST set the global values to 0.
\subsubsection{Legacy Interface: Framing Requirements}\label{sec:Device
Types / Network Device / Legacy Interface: Framing Requirements}
--
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/
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [virtio-dev] [PATCH v9] virtio-net: support the virtqueue coalescing moderation
2023-02-26 9:37 [virtio-comment] [PATCH v9] virtio-net: support the virtqueue coalescing moderation Heng Qi
@ 2023-02-26 9:37 ` Heng Qi
2023-02-26 19:33 ` [virtio-comment] " Alvaro Karsz
2023-02-28 1:40 ` [virtio-comment] " Parav Pandit
2 siblings, 0 replies; 25+ messages in thread
From: Heng Qi @ 2023-02-26 9:37 UTC (permalink / raw)
To: virtio-dev, virtio-comment
Cc: Alvaro Karsz, Michael S . Tsirkin, Parav Pandit, 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>
---
This patch is on top of Alvaro's latest v7 patch: https://lists.oasis-open.org/archives/virtio-dev/202302/msg00431.html .
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 | 95 +++++++++++++++++++++++++++++---
1 file changed, 87 insertions(+), 8 deletions(-)
diff --git a/device-types/net/description.tex b/device-types/net/description.tex
index e71e33b..71f6827 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,8 +1508,17 @@ \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, the driver can send the VIRTIO_NET_CTRL_NOTF_COAL_TX_SET
+and VIRTIO_NET_CTRL_NOTF_COAL_RX_SET commands to dynamically change the coalescing parameters for all transmit
+and receive virtqueues, respectively.
+
+If the VIRTIO_NET_F_VQ_NOTF_COAL feature is negotiated:
+\begin{itemize}
+\item a driver can send a VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET command to set coalescing parameters of a given
+ enabled transmit/receive virtqueue.
+\item a driver can send a VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET command to a device, and the device responds with
+ coalescing parameters of a given enabled transmit/receive virtqueue.
+\end{itemize}
\begin{note}
The behavior of the device in response to these commands is best-effort:
@@ -1519,25 +1531,76 @@ \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}
+The VIRTIO_NET_CTRL_NOTF_COAL_TX_SET and VIRTIO_NET_CTRL_NOTF_COAL_RX_SET commands use the
+virtio_net_ctrl_coal structure to set \field{max_usecs} and \field{max_packets} for all
+transmit/receive virtqueues.
+
+The VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET command uses the virtio_net_ctrl_coal_vq structure
+to set \field{max_usecs} and \field{max_packets} for the supplied virtqueue number \field{vqn}.
+
+The VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET command gets the values of \field{max_usecs} and
+\field{max_packets} of the specified virtqueue from the device by setting \field{vqn}
+in the virtio_net_ctrl_coal_vq structure.
+
+# Read/Write attributes for coalescing parameters
+\begin{itemize}
+\item For commands VIRTIO_NET_CTRL_NOTF_COAL_TX_SET and VIRTIO_NET_CTRL_NOTF_COAL_RX_SET, \field{max_usecs}
+ and \field{max_packets} are write-only for a driver.
+\item For the command VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET, \field{vqn}, \field{reserved}, \field{max_usecs}
+ and \field{max_packets} are write-only for a driver.
+\item For the command VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET, \field{vqn} and \field{reserved} are write-only
+ for a driver, and, \field{max_usecs} and \field{max_packets} are read-only for the driver.
+\end{itemize}
+
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.
+
+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: set the \field{max_usecs} and \field{max_packets} parameters for all transmit virtqueues, and values of
+ coalescing parameters are recorded as TX global values by a device.
+\item VIRTIO_NET_CTRL_NOTF_COAL_RX_SET: set the \field{max_usecs} and \field{max_packets} parameters for all receive virtqueues, and values of
+ coalescing parameters are recorded as RX global values by a device.
+\item VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET: set the \field{max_usecs} and \field{max_packets} parameters for an enabled transmit/receive
+ virtqueue whose number is \field{vqn}, and do not change the TX/RX global values.
+\item VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET: the device returns the \field{max_usecs} and \field{max_packets} parameters for an enabled
+ transmit/receive virtqueue whose number is \field{vqn}.
\end{enumerate}
+If coalescing parameters are being set, the device applies the last coalescing parameters received for a
+virtqueue, regardless of the command used to set the parameters. For example with 2 pairs of virtqueues:
+# Command sequence
+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}
+
\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}.
@@ -1549,6 +1612,11 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
When the device has \field{max_usecs} = 0 or \field{max_packets} = 0, the notification conditions are met after every packet received/sent.
+When the device receives the VIRTIO_NET_CTRL_NOTF_COAL_TX_SET and VIRTIO_NET_CTRL_NOTF_COAL_RX_SET commands,
+it saves the values of coalescing parameters as global values, and the VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET command
+does not change the global values. If the device is reset, the global values will be set to 0.
+When a virtqueue is enabled after virtqueue reset, its coalescing parameters are set to global values.
+
\subparagraph{RX Example}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Notifications Coalescing / RX Example}
If, for example:
@@ -1585,15 +1653,26 @@ \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.
+If neither the VIRTIO_NET_F_NOTF_COAL nor the VIRTIO_NET_F_VQ_NOTF_COAL feature
+has been negotiated, the driver MUST NOT issue VIRTIO_NET_CTRL_NOTF_COAL commands.
+
+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 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.
+
+A device MUST ignore \field{reserved}.
A device SHOULD NOT send used buffer notifications to the driver if the notifications are suppressed, even if the notification conditions are met.
-Upon reset, a device MUST initialize all coalescing parameters to 0.
+After disabling and re-enabling a virtqueue, the device MUST revert coalescing parameters of the virtqueue to the global values.
+
+Upon reset, a device MUST initialize all coalescing parameters to 0 and MUST set the global values to 0.
\subsubsection{Legacy Interface: Framing Requirements}\label{sec:Device
Types / Network Device / Legacy Interface: Framing Requirements}
--
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] 25+ messages in thread
* [virtio-comment] Re: [PATCH v9] virtio-net: support the virtqueue coalescing moderation
2023-02-26 9:37 [virtio-comment] [PATCH v9] virtio-net: support the virtqueue coalescing moderation Heng Qi
2023-02-26 9:37 ` [virtio-dev] " Heng Qi
@ 2023-02-26 19:33 ` Alvaro Karsz
2023-02-26 19:33 ` [virtio-dev] " Alvaro Karsz
2023-02-27 13:02 ` Heng Qi
2023-02-28 1:40 ` [virtio-comment] " Parav Pandit
2 siblings, 2 replies; 25+ messages in thread
From: Alvaro Karsz @ 2023-02-26 19:33 UTC (permalink / raw)
To: Heng Qi
Cc: virtio-dev, virtio-comment, Michael S . Tsirkin, Parav Pandit,
David Edmondson, Jason Wang, Xuan Zhuo
Hi,
> 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>
> ---
> This patch is on top of Alvaro's latest v7 patch: https://lists.oasis-open.org/archives/virtio-dev/202302/msg00431.html .
>
> 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 | 95 +++++++++++++++++++++++++++++---
> 1 file changed, 87 insertions(+), 8 deletions(-)
>
> diff --git a/device-types/net/description.tex b/device-types/net/description.tex
> index e71e33b..71f6827 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,8 +1508,17 @@ \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, the driver can send the VIRTIO_NET_CTRL_NOTF_COAL_TX_SET
> +and VIRTIO_NET_CTRL_NOTF_COAL_RX_SET commands to dynamically change the coalescing parameters for all transmit
> +and receive virtqueues, respectively.
> +
> +If the VIRTIO_NET_F_VQ_NOTF_COAL feature is negotiated:
> +\begin{itemize}
> +\item a driver can send a VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET command to set coalescing parameters of a given
> + enabled transmit/receive virtqueue.
> +\item a driver can send a VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET command to a device, and the device responds with
> + coalescing parameters of a given enabled transmit/receive virtqueue.
> +\end{itemize}
>
The VQ_NOTF_COAL commands are enclosed in an itemize and the NOTF_COAL
commands aren't.
I think that we should be consistent here.
I'm not sure that describing the commands in here is necessary, maybe
just mentioning which commands can be used with which feature bit is
enough (this is what I meant in v8, sorry if I wasn't clear).
I'm not saying that describing the commands in here is not good, but
notice that the commands are described in 3 different places.
This is #1.
> \begin{note}
> The behavior of the device in response to these commands is best-effort:
> @@ -1519,25 +1531,76 @@ \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}
>
> +The VIRTIO_NET_CTRL_NOTF_COAL_TX_SET and VIRTIO_NET_CTRL_NOTF_COAL_RX_SET commands use the
> +virtio_net_ctrl_coal structure to set \field{max_usecs} and \field{max_packets} for all
> +transmit/receive virtqueues.
> +
> +The VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET command uses the virtio_net_ctrl_coal_vq structure
> +to set \field{max_usecs} and \field{max_packets} for the supplied virtqueue number \field{vqn}.
> +
> +The VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET command gets the values of \field{max_usecs} and
> +\field{max_packets} of the specified virtqueue from the device by setting \field{vqn}
> +in the virtio_net_ctrl_coal_vq structure.
> +
This is #2.
> +# Read/Write attributes for coalescing parameters
> +\begin{itemize}
> +\item For commands VIRTIO_NET_CTRL_NOTF_COAL_TX_SET and VIRTIO_NET_CTRL_NOTF_COAL_RX_SET, \field{max_usecs}
> + and \field{max_packets} are write-only for a driver.
> +\item For the command VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET, \field{vqn}, \field{reserved}, \field{max_usecs}
> + and \field{max_packets} are write-only for a driver.
> +\item For the command VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET, \field{vqn} and \field{reserved} are write-only
> + for a driver, and, \field{max_usecs} and \field{max_packets} are read-only for the driver.
> +\end{itemize}
> +
> 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.
> +
> +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: set the \field{max_usecs} and \field{max_packets} parameters for all transmit virtqueues, and values of
> + coalescing parameters are recorded as TX global values by a device.
> +\item VIRTIO_NET_CTRL_NOTF_COAL_RX_SET: set the \field{max_usecs} and \field{max_packets} parameters for all receive virtqueues, and values of
> + coalescing parameters are recorded as RX global values by a device.
> +\item VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET: set the \field{max_usecs} and \field{max_packets} parameters for an enabled transmit/receive
> + virtqueue whose number is \field{vqn}, and do not change the TX/RX global values.
> +\item VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET: the device returns the \field{max_usecs} and \field{max_packets} parameters for an enabled
> + transmit/receive virtqueue whose number is \field{vqn}.
> \end{enumerate}
This is #3.
>
> +If coalescing parameters are being set, the device applies the last coalescing parameters received for a
> +virtqueue, regardless of the command used to set the parameters. For example with 2 pairs of virtqueues:
> +# Command sequence
> +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}
> +
> \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}.
> @@ -1549,6 +1612,11 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
>
> When the device has \field{max_usecs} = 0 or \field{max_packets} = 0, the notification conditions are met after every packet received/sent.
>
> +When the device receives the VIRTIO_NET_CTRL_NOTF_COAL_TX_SET and VIRTIO_NET_CTRL_NOTF_COAL_RX_SET commands,
> +it saves the values of coalescing parameters as global values, and the VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET command
> +does not change the global values. If the device is reset, the global values will be set to 0.
> +When a virtqueue is enabled after virtqueue reset, its coalescing parameters are set to global values.
> +
Maybe this explanation should be put closer to the commands
descriptions, where the global coalescing parameters are mentioned for
the first time.
Something like:
....
\item VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET: the device returns the
\field{max_usecs} and \field{max_packets} parameters for an enabled
transmit/receive virtqueue whose number
is \field{vqn}.
The device maintains global coalescing parameters for TX and RX....
And maybe we should use "global coalescing parameters" instead of
"global values" (in devicenormative as well).
> \subparagraph{RX Example}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Notifications Coalescing / RX Example}
>
> If, for example:
> @@ -1585,15 +1653,26 @@ \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.
> +If neither the VIRTIO_NET_F_NOTF_COAL nor the VIRTIO_NET_F_VQ_NOTF_COAL feature
> +has been negotiated, the driver MUST NOT issue VIRTIO_NET_CTRL_NOTF_COAL commands.
Maybe this part can be splitted to:
if the F1 feature has not been negotiated, the driver must not issue
commands C1,C2.
if the F2 feature has not been negotiated, the driver must not issue
commands C3,C4.
> +
> +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 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.
> +
> +A device MUST ignore \field{reserved}.
>
> A device SHOULD NOT send used buffer notifications to the driver if the notifications are suppressed, even if the notification conditions are met.
>
> -Upon reset, a device MUST initialize all coalescing parameters to 0.
> +After disabling and re-enabling a virtqueue, the device MUST revert coalescing parameters of the virtqueue to the global values.
> +
> +Upon reset, a device MUST initialize all coalescing parameters to 0 and MUST set the global values to 0.
>
> \subsubsection{Legacy Interface: Framing Requirements}\label{sec:Device
> Types / Network Device / Legacy Interface: Framing Requirements}
> --
Let's wait for more comments before the next version, I guess some may
not agree with my comments.
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/
^ permalink raw reply [flat|nested] 25+ messages in thread
* [virtio-dev] Re: [PATCH v9] virtio-net: support the virtqueue coalescing moderation
2023-02-26 19:33 ` [virtio-comment] " Alvaro Karsz
@ 2023-02-26 19:33 ` Alvaro Karsz
2023-02-27 13:02 ` Heng Qi
1 sibling, 0 replies; 25+ messages in thread
From: Alvaro Karsz @ 2023-02-26 19:33 UTC (permalink / raw)
To: Heng Qi
Cc: virtio-dev, virtio-comment, Michael S . Tsirkin, Parav Pandit,
David Edmondson, Jason Wang, Xuan Zhuo
Hi,
> 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>
> ---
> This patch is on top of Alvaro's latest v7 patch: https://lists.oasis-open.org/archives/virtio-dev/202302/msg00431.html .
>
> 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 | 95 +++++++++++++++++++++++++++++---
> 1 file changed, 87 insertions(+), 8 deletions(-)
>
> diff --git a/device-types/net/description.tex b/device-types/net/description.tex
> index e71e33b..71f6827 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,8 +1508,17 @@ \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, the driver can send the VIRTIO_NET_CTRL_NOTF_COAL_TX_SET
> +and VIRTIO_NET_CTRL_NOTF_COAL_RX_SET commands to dynamically change the coalescing parameters for all transmit
> +and receive virtqueues, respectively.
> +
> +If the VIRTIO_NET_F_VQ_NOTF_COAL feature is negotiated:
> +\begin{itemize}
> +\item a driver can send a VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET command to set coalescing parameters of a given
> + enabled transmit/receive virtqueue.
> +\item a driver can send a VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET command to a device, and the device responds with
> + coalescing parameters of a given enabled transmit/receive virtqueue.
> +\end{itemize}
>
The VQ_NOTF_COAL commands are enclosed in an itemize and the NOTF_COAL
commands aren't.
I think that we should be consistent here.
I'm not sure that describing the commands in here is necessary, maybe
just mentioning which commands can be used with which feature bit is
enough (this is what I meant in v8, sorry if I wasn't clear).
I'm not saying that describing the commands in here is not good, but
notice that the commands are described in 3 different places.
This is #1.
> \begin{note}
> The behavior of the device in response to these commands is best-effort:
> @@ -1519,25 +1531,76 @@ \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}
>
> +The VIRTIO_NET_CTRL_NOTF_COAL_TX_SET and VIRTIO_NET_CTRL_NOTF_COAL_RX_SET commands use the
> +virtio_net_ctrl_coal structure to set \field{max_usecs} and \field{max_packets} for all
> +transmit/receive virtqueues.
> +
> +The VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET command uses the virtio_net_ctrl_coal_vq structure
> +to set \field{max_usecs} and \field{max_packets} for the supplied virtqueue number \field{vqn}.
> +
> +The VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET command gets the values of \field{max_usecs} and
> +\field{max_packets} of the specified virtqueue from the device by setting \field{vqn}
> +in the virtio_net_ctrl_coal_vq structure.
> +
This is #2.
> +# Read/Write attributes for coalescing parameters
> +\begin{itemize}
> +\item For commands VIRTIO_NET_CTRL_NOTF_COAL_TX_SET and VIRTIO_NET_CTRL_NOTF_COAL_RX_SET, \field{max_usecs}
> + and \field{max_packets} are write-only for a driver.
> +\item For the command VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET, \field{vqn}, \field{reserved}, \field{max_usecs}
> + and \field{max_packets} are write-only for a driver.
> +\item For the command VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET, \field{vqn} and \field{reserved} are write-only
> + for a driver, and, \field{max_usecs} and \field{max_packets} are read-only for the driver.
> +\end{itemize}
> +
> 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.
> +
> +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: set the \field{max_usecs} and \field{max_packets} parameters for all transmit virtqueues, and values of
> + coalescing parameters are recorded as TX global values by a device.
> +\item VIRTIO_NET_CTRL_NOTF_COAL_RX_SET: set the \field{max_usecs} and \field{max_packets} parameters for all receive virtqueues, and values of
> + coalescing parameters are recorded as RX global values by a device.
> +\item VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET: set the \field{max_usecs} and \field{max_packets} parameters for an enabled transmit/receive
> + virtqueue whose number is \field{vqn}, and do not change the TX/RX global values.
> +\item VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET: the device returns the \field{max_usecs} and \field{max_packets} parameters for an enabled
> + transmit/receive virtqueue whose number is \field{vqn}.
> \end{enumerate}
This is #3.
>
> +If coalescing parameters are being set, the device applies the last coalescing parameters received for a
> +virtqueue, regardless of the command used to set the parameters. For example with 2 pairs of virtqueues:
> +# Command sequence
> +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}
> +
> \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}.
> @@ -1549,6 +1612,11 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
>
> When the device has \field{max_usecs} = 0 or \field{max_packets} = 0, the notification conditions are met after every packet received/sent.
>
> +When the device receives the VIRTIO_NET_CTRL_NOTF_COAL_TX_SET and VIRTIO_NET_CTRL_NOTF_COAL_RX_SET commands,
> +it saves the values of coalescing parameters as global values, and the VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET command
> +does not change the global values. If the device is reset, the global values will be set to 0.
> +When a virtqueue is enabled after virtqueue reset, its coalescing parameters are set to global values.
> +
Maybe this explanation should be put closer to the commands
descriptions, where the global coalescing parameters are mentioned for
the first time.
Something like:
....
\item VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET: the device returns the
\field{max_usecs} and \field{max_packets} parameters for an enabled
transmit/receive virtqueue whose number
is \field{vqn}.
The device maintains global coalescing parameters for TX and RX....
And maybe we should use "global coalescing parameters" instead of
"global values" (in devicenormative as well).
> \subparagraph{RX Example}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Notifications Coalescing / RX Example}
>
> If, for example:
> @@ -1585,15 +1653,26 @@ \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.
> +If neither the VIRTIO_NET_F_NOTF_COAL nor the VIRTIO_NET_F_VQ_NOTF_COAL feature
> +has been negotiated, the driver MUST NOT issue VIRTIO_NET_CTRL_NOTF_COAL commands.
Maybe this part can be splitted to:
if the F1 feature has not been negotiated, the driver must not issue
commands C1,C2.
if the F2 feature has not been negotiated, the driver must not issue
commands C3,C4.
> +
> +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 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.
> +
> +A device MUST ignore \field{reserved}.
>
> A device SHOULD NOT send used buffer notifications to the driver if the notifications are suppressed, even if the notification conditions are met.
>
> -Upon reset, a device MUST initialize all coalescing parameters to 0.
> +After disabling and re-enabling a virtqueue, the device MUST revert coalescing parameters of the virtqueue to the global values.
> +
> +Upon reset, a device MUST initialize all coalescing parameters to 0 and MUST set the global values to 0.
>
> \subsubsection{Legacy Interface: Framing Requirements}\label{sec:Device
> Types / Network Device / Legacy Interface: Framing Requirements}
> --
Let's wait for more comments before the next version, I guess some may
not agree with my comments.
---------------------------------------------------------------------
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] 25+ messages in thread
* Re: [virtio-dev] Re: [PATCH v9] virtio-net: support the virtqueue coalescing moderation
2023-02-26 19:33 ` [virtio-comment] " Alvaro Karsz
2023-02-26 19:33 ` [virtio-dev] " Alvaro Karsz
@ 2023-02-27 13:02 ` Heng Qi
2023-02-27 16:25 ` [virtio-comment] " Heng Qi
2023-02-27 18:45 ` Alvaro Karsz
1 sibling, 2 replies; 25+ messages in thread
From: Heng Qi @ 2023-02-27 13:02 UTC (permalink / raw)
To: Alvaro Karsz
Cc: virtio-dev, virtio-comment, Michael S . Tsirkin, Parav Pandit,
David Edmondson, Jason Wang, Xuan Zhuo
Hi, Alvaro.
在 2023/2/27 上午3:33, Alvaro Karsz 写道:
> Hi,
>
>> 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>
>> ---
>> This patch is on top of Alvaro's latest v7 patch: https://lists.oasis-open.org/archives/virtio-dev/202302/msg00431.html .
>>
>> 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 | 95 +++++++++++++++++++++++++++++---
>> 1 file changed, 87 insertions(+), 8 deletions(-)
>>
>> diff --git a/device-types/net/description.tex b/device-types/net/description.tex
>> index e71e33b..71f6827 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,8 +1508,17 @@ \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, the driver can send the VIRTIO_NET_CTRL_NOTF_COAL_TX_SET
>> +and VIRTIO_NET_CTRL_NOTF_COAL_RX_SET commands to dynamically change the coalescing parameters for all transmit
>> +and receive virtqueues, respectively.
>> +
>> +If the VIRTIO_NET_F_VQ_NOTF_COAL feature is negotiated:
>> +\begin{itemize}
>> +\item a driver can send a VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET command to set coalescing parameters of a given
>> + enabled transmit/receive virtqueue.
>> +\item a driver can send a VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET command to a device, and the device responds with
>> + coalescing parameters of a given enabled transmit/receive virtqueue.
>> +\end{itemize}
>>
> The VQ_NOTF_COAL commands are enclosed in an itemize and the NOTF_COAL
> commands aren't.
> I think that we should be consistent here.
The VQ commands are enclosed in an itemize because it has both GET and
SET operations.
I don't think VIRTIO_NET_CTRL_NOTF_COAL_RX_SET and
VIRTIO_NET_CTRL_NOTF_COAL_TX_SET
have to do this, because they are both settings, just in different
directions, and we have described them like this elsewhere.
>
> I'm not sure that describing the commands in here is necessary, maybe
> just mentioning which commands can be used with which feature bit is
Isn't that what this sentence does?
> enough (this is what I meant in v8, sorry if I wasn't clear).
>
> I'm not saying that describing the commands in here is not good, but
> notice that the commands are described in 3 different places.
Three places describe different effects.
#1 describes which commands are available for which feature.
#2 describes which command can use which structure.
#3 describes which parameters can be set for each command, and whether
they can affect "global values".
It is placed in the "Operation" section because it explains how the
device should behave.
Maybe #1 and #2 descriptions can be combined and described together.
> This is #1.
>
>> \begin{note}
>> The behavior of the device in response to these commands is best-effort:
>> @@ -1519,25 +1531,76 @@ \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}
>>
>> +The VIRTIO_NET_CTRL_NOTF_COAL_TX_SET and VIRTIO_NET_CTRL_NOTF_COAL_RX_SET commands use the
>> +virtio_net_ctrl_coal structure to set \field{max_usecs} and \field{max_packets} for all
>> +transmit/receive virtqueues.
>> +
>> +The VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET command uses the virtio_net_ctrl_coal_vq structure
>> +to set \field{max_usecs} and \field{max_packets} for the supplied virtqueue number \field{vqn}.
>> +
>> +The VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET command gets the values of \field{max_usecs} and
>> +\field{max_packets} of the specified virtqueue from the device by setting \field{vqn}
>> +in the virtio_net_ctrl_coal_vq structure.
>> +
> This is #2.
>
>> +# Read/Write attributes for coalescing parameters
>> +\begin{itemize}
>> +\item For commands VIRTIO_NET_CTRL_NOTF_COAL_TX_SET and VIRTIO_NET_CTRL_NOTF_COAL_RX_SET, \field{max_usecs}
>> + and \field{max_packets} are write-only for a driver.
>> +\item For the command VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET, \field{vqn}, \field{reserved}, \field{max_usecs}
>> + and \field{max_packets} are write-only for a driver.
>> +\item For the command VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET, \field{vqn} and \field{reserved} are write-only
>> + for a driver, and, \field{max_usecs} and \field{max_packets} are read-only for the driver.
>> +\end{itemize}
>> +
>> 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.
>> +
>> +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: set the \field{max_usecs} and \field{max_packets} parameters for all transmit virtqueues, and values of
>> + coalescing parameters are recorded as TX global values by a device.
>> +\item VIRTIO_NET_CTRL_NOTF_COAL_RX_SET: set the \field{max_usecs} and \field{max_packets} parameters for all receive virtqueues, and values of
>> + coalescing parameters are recorded as RX global values by a device.
>> +\item VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET: set the \field{max_usecs} and \field{max_packets} parameters for an enabled transmit/receive
>> + virtqueue whose number is \field{vqn}, and do not change the TX/RX global values.
>> +\item VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET: the device returns the \field{max_usecs} and \field{max_packets} parameters for an enabled
>> + transmit/receive virtqueue whose number is \field{vqn}.
>> \end{enumerate}
> This is #3.
>
>> +If coalescing parameters are being set, the device applies the last coalescing parameters received for a
>> +virtqueue, regardless of the command used to set the parameters. For example with 2 pairs of virtqueues:
>> +# Command sequence
>> +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}
>> +
>> \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}.
>> @@ -1549,6 +1612,11 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
>>
>> When the device has \field{max_usecs} = 0 or \field{max_packets} = 0, the notification conditions are met after every packet received/sent.
>>
>> +When the device receives the VIRTIO_NET_CTRL_NOTF_COAL_TX_SET and VIRTIO_NET_CTRL_NOTF_COAL_RX_SET commands,
>> +it saves the values of coalescing parameters as global values, and the VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET command
>> +does not change the global values. If the device is reset, the global values will be set to 0.
>> +When a virtqueue is enabled after virtqueue reset, its coalescing parameters are set to global values.
>> +
> Maybe this explanation should be put closer to the commands
> descriptions, where the global coalescing parameters are mentioned for
> the first time.
> Something like:
> ....
> \item VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET: the device returns the
> \field{max_usecs} and \field{max_packets} parameters for an enabled
> transmit/receive virtqueue whose number
> is \field{vqn}.
>
> The device maintains global coalescing parameters for TX and RX....
This is the "Operation" chapter, and the description of the operation is
more likely to be placed here, isn't it?
>
> And maybe we should use "global coalescing parameters" instead of
> "global values" (in devicenormative as well).
>
>> \subparagraph{RX Example}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Notifications Coalescing / RX Example}
>>
>> If, for example:
>> @@ -1585,15 +1653,26 @@ \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.
>> +If neither the VIRTIO_NET_F_NOTF_COAL nor the VIRTIO_NET_F_VQ_NOTF_COAL feature
>> +has been negotiated, the driver MUST NOT issue VIRTIO_NET_CTRL_NOTF_COAL commands.
> Maybe this part can be splitted to:
> if the F1 feature has not been negotiated, the driver must not issue
> commands C1,C2.
> if the F2 feature has not been negotiated, the driver must not issue
> commands C3,C4.
Ok.
Thanks.
>> +
>> +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 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.
>> +
>> +A device MUST ignore \field{reserved}.
>>
>> A device SHOULD NOT send used buffer notifications to the driver if the notifications are suppressed, even if the notification conditions are met.
>>
>> -Upon reset, a device MUST initialize all coalescing parameters to 0.
>> +After disabling and re-enabling a virtqueue, the device MUST revert coalescing parameters of the virtqueue to the global values.
>> +
>> +Upon reset, a device MUST initialize all coalescing parameters to 0 and MUST set the global values to 0.
>>
>> \subsubsection{Legacy Interface: Framing Requirements}\label{sec:Device
>> Types / Network Device / Legacy Interface: Framing Requirements}
>> --
> Let's wait for more comments before the next version, I guess some may
> not agree with my comments.
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
---------------------------------------------------------------------
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] 25+ messages in thread
* [virtio-comment] Re: [virtio-dev] Re: [PATCH v9] virtio-net: support the virtqueue coalescing moderation
2023-02-27 13:02 ` Heng Qi
@ 2023-02-27 16:25 ` Heng Qi
2023-02-27 16:25 ` Heng Qi
2023-02-27 16:27 ` [virtio-comment] " Parav Pandit
2023-02-27 18:45 ` Alvaro Karsz
1 sibling, 2 replies; 25+ messages in thread
From: Heng Qi @ 2023-02-27 16:25 UTC (permalink / raw)
To: Michael S . Tsirkin, Parav Pandit, Alvaro Karsz
Cc: virtio-dev, virtio-comment, David Edmondson, Jason Wang,
Xuan Zhuo
在 2023/2/27 下午9:02, Heng Qi 写道:
> Hi, Alvaro.
>
> 在 2023/2/27 上午3:33, Alvaro Karsz 写道:
>> Hi,
>>
>>> 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>
>>> ---
>>> This patch is on top of Alvaro's latest v7 patch:
>>> https://lists.oasis-open.org/archives/virtio-dev/202302/msg00431.html .
>>>
>>> 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 | 95
>>> +++++++++++++++++++++++++++++---
>>> 1 file changed, 87 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/device-types/net/description.tex
>>> b/device-types/net/description.tex
>>> index e71e33b..71f6827 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,8 +1508,17 @@ \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, the driver can
>>> send the VIRTIO_NET_CTRL_NOTF_COAL_TX_SET
>>> +and VIRTIO_NET_CTRL_NOTF_COAL_RX_SET commands to dynamically change
>>> the coalescing parameters for all transmit
>>> +and receive virtqueues, respectively.
>>> +
>>> +If the VIRTIO_NET_F_VQ_NOTF_COAL feature is negotiated:
>>> +\begin{itemize}
>>> +\item a driver can send a VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET command
>>> to set coalescing parameters of a given
>>> + enabled transmit/receive virtqueue.
>>> +\item a driver can send a VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET command
>>> to a device, and the device responds with
>>> + coalescing parameters of a given enabled transmit/receive
>>> virtqueue.
>>> +\end{itemize}
>>>
>> The VQ_NOTF_COAL commands are enclosed in an itemize and the NOTF_COAL
>> commands aren't.
>> I think that we should be consistent here.
>
> The VQ commands are enclosed in an itemize because it has both GET and
> SET operations.
>
> I don't think VIRTIO_NET_CTRL_NOTF_COAL_RX_SET and
> VIRTIO_NET_CTRL_NOTF_COAL_TX_SET
> have to do this, because they are both settings, just in different
> directions, and we have described them like this elsewhere.
>
>>
>> I'm not sure that describing the commands in here is necessary, maybe
>> just mentioning which commands can be used with which feature bit is
>
> Isn't that what this sentence does?
>
>> enough (this is what I meant in v8, sorry if I wasn't clear).
>>
>> I'm not saying that describing the commands in here is not good, but
>> notice that the commands are described in 3 different places.
>
> Three places describe different effects.
>
> #1 describes which commands are available for which feature.
> #2 describes which command can use which structure.
> #3 describes which parameters can be set for each command, and whether
> they can affect "global values".
> It is placed in the "Operation" section because it explains how the
> device should behave.
>
> Maybe #1 and #2 descriptions can be combined and described together.
>
>> This is #1.
>>
>>> \begin{note}
>>> The behavior of the device in response to these commands is
>>> best-effort:
>>> @@ -1519,25 +1531,76 @@ \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}
>>>
>>> +The VIRTIO_NET_CTRL_NOTF_COAL_TX_SET and
>>> VIRTIO_NET_CTRL_NOTF_COAL_RX_SET commands use the
>>> +virtio_net_ctrl_coal structure to set \field{max_usecs} and
>>> \field{max_packets} for all
>>> +transmit/receive virtqueues.
>>> +
>>> +The VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET command uses the
>>> virtio_net_ctrl_coal_vq structure
>>> +to set \field{max_usecs} and \field{max_packets} for the supplied
>>> virtqueue number \field{vqn}.
>>> +
>>> +The VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET command gets the values of
>>> \field{max_usecs} and
>>> +\field{max_packets} of the specified virtqueue from the device by
>>> setting \field{vqn}
>>> +in the virtio_net_ctrl_coal_vq structure.
>>> +
>> This is #2.
>>
>>> +# Read/Write attributes for coalescing parameters
>>> +\begin{itemize}
>>> +\item For commands VIRTIO_NET_CTRL_NOTF_COAL_TX_SET and
>>> VIRTIO_NET_CTRL_NOTF_COAL_RX_SET, \field{max_usecs}
>>> + and \field{max_packets} are write-only for a driver.
>>> +\item For the command VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET,
>>> \field{vqn}, \field{reserved}, \field{max_usecs}
>>> + and \field{max_packets} are write-only for a driver.
>>> +\item For the command VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET, \field{vqn}
>>> and \field{reserved} are write-only
>>> + for a driver, and, \field{max_usecs} and \field{max_packets}
>>> are read-only for the driver.
>>> +\end{itemize}
>>> +
>>> 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.
>>> +
>>> +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: set the \field{max_usecs}
>>> and \field{max_packets} parameters for all transmit virtqueues, and
>>> values of
>>> + coalescing parameters are
>>> recorded as TX global values by a device.
>>> +\item VIRTIO_NET_CTRL_NOTF_COAL_RX_SET: set the \field{max_usecs}
>>> and \field{max_packets} parameters for all receive virtqueues, and
>>> values of
>>> + coalescing parameters are
>>> recorded as RX global values by a device.
>>> +\item VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET: set the \field{max_usecs}
>>> and \field{max_packets} parameters for an enabled transmit/receive
>>> + virtqueue whose number is
>>> \field{vqn}, and do not change the TX/RX global values.
>>> +\item VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET: the device returns the
>>> \field{max_usecs} and \field{max_packets} parameters for an enabled
>>> + transmit/receive virtqueue
>>> whose number is \field{vqn}.
>>> \end{enumerate}
>> This is #3.
>>
>>> +If coalescing parameters are being set, the device applies the last
>>> coalescing parameters received for a
>>> +virtqueue, regardless of the command used to set the parameters.
>>> For example with 2 pairs of virtqueues:
>>> +# Command sequence
>>> +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}
>>> +
>>> \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}.
>>> @@ -1549,6 +1612,11 @@ \subsubsection{Control
>>> Virtqueue}\label{sec:Device Types / Network Device / Devi
>>>
>>> When the device has \field{max_usecs} = 0 or \field{max_packets} =
>>> 0, the notification conditions are met after every packet
>>> received/sent.
>>>
>>> +When the device receives the VIRTIO_NET_CTRL_NOTF_COAL_TX_SET and
>>> VIRTIO_NET_CTRL_NOTF_COAL_RX_SET commands,
>>> +it saves the values of coalescing parameters as global values, and
>>> the VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET command
>>> +does not change the global values. If the device is reset, the
>>> global values will be set to 0.
>>> +When a virtqueue is enabled after virtqueue reset, its coalescing
>>> parameters are set to global values.
>>> +
>> Maybe this explanation should be put closer to the commands
>> descriptions, where the global coalescing parameters are mentioned for
>> the first time.
>> Something like:
>> ....
>> \item VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET: the device returns the
>> \field{max_usecs} and \field{max_packets} parameters for an enabled
>> transmit/receive virtqueue whose number
>> is \field{vqn}.
>>
>> The device maintains global coalescing parameters for TX and RX....
>
> This is the "Operation" chapter, and the description of the operation
> is more likely to be placed here, isn't it?
>
>
>
>>
>> And maybe we should use "global coalescing parameters" instead of
>> "global values" (in devicenormative as well).
>>
>>> \subparagraph{RX Example}\label{sec:Device Types / Network Device
>>> / Device Operation / Control Virtqueue / Notifications Coalescing /
>>> RX Example}
>>>
>>> If, for example:
>>> @@ -1585,15 +1653,26 @@ \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.
>>> +If neither the VIRTIO_NET_F_NOTF_COAL nor the
>>> VIRTIO_NET_F_VQ_NOTF_COAL feature
>>> +has been negotiated, the driver MUST NOT issue
>>> VIRTIO_NET_CTRL_NOTF_COAL commands.
>> Maybe this part can be splitted to:
>> if the F1 feature has not been negotiated, the driver must not issue
>> commands C1,C2.
>> if the F2 feature has not been negotiated, the driver must not issue
>> commands C3,C4.
>
> Ok.
>
> Thanks.
>
>>> +
>>> +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 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.
>>> +
>>> +A device MUST ignore \field{reserved}.
>>>
>>> A device SHOULD NOT send used buffer notifications to the driver
>>> if the notifications are suppressed, even if the notification
>>> conditions are met.
>>>
>>> -Upon reset, a device MUST initialize all coalescing parameters to 0.
>>> +After disabling and re-enabling a virtqueue, the device MUST revert
>>> coalescing parameters of the virtqueue to the global values.
>>> +
>>> +Upon reset, a device MUST initialize all coalescing parameters to 0
>>> and MUST set the global values to 0.
>>>
>>> \subsubsection{Legacy Interface: Framing
>>> Requirements}\label{sec:Device
>>> Types / Network Device / Legacy Interface: Framing Requirements}
>>> --
>> Let's wait for more comments before the next version, I guess some may
>> not agree with my comments.
Hi, Michael and Parav, what do you think?
Thanks.
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
>> For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
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/
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [virtio-dev] Re: [PATCH v9] virtio-net: support the virtqueue coalescing moderation
2023-02-27 16:25 ` [virtio-comment] " Heng Qi
@ 2023-02-27 16:25 ` Heng Qi
2023-02-27 16:27 ` [virtio-comment] " Parav Pandit
1 sibling, 0 replies; 25+ messages in thread
From: Heng Qi @ 2023-02-27 16:25 UTC (permalink / raw)
To: Michael S . Tsirkin, Parav Pandit, Alvaro Karsz
Cc: virtio-dev, virtio-comment, David Edmondson, Jason Wang,
Xuan Zhuo
在 2023/2/27 下午9:02, Heng Qi 写道:
> Hi, Alvaro.
>
> 在 2023/2/27 上午3:33, Alvaro Karsz 写道:
>> Hi,
>>
>>> 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>
>>> ---
>>> This patch is on top of Alvaro's latest v7 patch:
>>> https://lists.oasis-open.org/archives/virtio-dev/202302/msg00431.html .
>>>
>>> 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 | 95
>>> +++++++++++++++++++++++++++++---
>>> 1 file changed, 87 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/device-types/net/description.tex
>>> b/device-types/net/description.tex
>>> index e71e33b..71f6827 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,8 +1508,17 @@ \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, the driver can
>>> send the VIRTIO_NET_CTRL_NOTF_COAL_TX_SET
>>> +and VIRTIO_NET_CTRL_NOTF_COAL_RX_SET commands to dynamically change
>>> the coalescing parameters for all transmit
>>> +and receive virtqueues, respectively.
>>> +
>>> +If the VIRTIO_NET_F_VQ_NOTF_COAL feature is negotiated:
>>> +\begin{itemize}
>>> +\item a driver can send a VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET command
>>> to set coalescing parameters of a given
>>> + enabled transmit/receive virtqueue.
>>> +\item a driver can send a VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET command
>>> to a device, and the device responds with
>>> + coalescing parameters of a given enabled transmit/receive
>>> virtqueue.
>>> +\end{itemize}
>>>
>> The VQ_NOTF_COAL commands are enclosed in an itemize and the NOTF_COAL
>> commands aren't.
>> I think that we should be consistent here.
>
> The VQ commands are enclosed in an itemize because it has both GET and
> SET operations.
>
> I don't think VIRTIO_NET_CTRL_NOTF_COAL_RX_SET and
> VIRTIO_NET_CTRL_NOTF_COAL_TX_SET
> have to do this, because they are both settings, just in different
> directions, and we have described them like this elsewhere.
>
>>
>> I'm not sure that describing the commands in here is necessary, maybe
>> just mentioning which commands can be used with which feature bit is
>
> Isn't that what this sentence does?
>
>> enough (this is what I meant in v8, sorry if I wasn't clear).
>>
>> I'm not saying that describing the commands in here is not good, but
>> notice that the commands are described in 3 different places.
>
> Three places describe different effects.
>
> #1 describes which commands are available for which feature.
> #2 describes which command can use which structure.
> #3 describes which parameters can be set for each command, and whether
> they can affect "global values".
> It is placed in the "Operation" section because it explains how the
> device should behave.
>
> Maybe #1 and #2 descriptions can be combined and described together.
>
>> This is #1.
>>
>>> \begin{note}
>>> The behavior of the device in response to these commands is
>>> best-effort:
>>> @@ -1519,25 +1531,76 @@ \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}
>>>
>>> +The VIRTIO_NET_CTRL_NOTF_COAL_TX_SET and
>>> VIRTIO_NET_CTRL_NOTF_COAL_RX_SET commands use the
>>> +virtio_net_ctrl_coal structure to set \field{max_usecs} and
>>> \field{max_packets} for all
>>> +transmit/receive virtqueues.
>>> +
>>> +The VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET command uses the
>>> virtio_net_ctrl_coal_vq structure
>>> +to set \field{max_usecs} and \field{max_packets} for the supplied
>>> virtqueue number \field{vqn}.
>>> +
>>> +The VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET command gets the values of
>>> \field{max_usecs} and
>>> +\field{max_packets} of the specified virtqueue from the device by
>>> setting \field{vqn}
>>> +in the virtio_net_ctrl_coal_vq structure.
>>> +
>> This is #2.
>>
>>> +# Read/Write attributes for coalescing parameters
>>> +\begin{itemize}
>>> +\item For commands VIRTIO_NET_CTRL_NOTF_COAL_TX_SET and
>>> VIRTIO_NET_CTRL_NOTF_COAL_RX_SET, \field{max_usecs}
>>> + and \field{max_packets} are write-only for a driver.
>>> +\item For the command VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET,
>>> \field{vqn}, \field{reserved}, \field{max_usecs}
>>> + and \field{max_packets} are write-only for a driver.
>>> +\item For the command VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET, \field{vqn}
>>> and \field{reserved} are write-only
>>> + for a driver, and, \field{max_usecs} and \field{max_packets}
>>> are read-only for the driver.
>>> +\end{itemize}
>>> +
>>> 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.
>>> +
>>> +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: set the \field{max_usecs}
>>> and \field{max_packets} parameters for all transmit virtqueues, and
>>> values of
>>> + coalescing parameters are
>>> recorded as TX global values by a device.
>>> +\item VIRTIO_NET_CTRL_NOTF_COAL_RX_SET: set the \field{max_usecs}
>>> and \field{max_packets} parameters for all receive virtqueues, and
>>> values of
>>> + coalescing parameters are
>>> recorded as RX global values by a device.
>>> +\item VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET: set the \field{max_usecs}
>>> and \field{max_packets} parameters for an enabled transmit/receive
>>> + virtqueue whose number is
>>> \field{vqn}, and do not change the TX/RX global values.
>>> +\item VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET: the device returns the
>>> \field{max_usecs} and \field{max_packets} parameters for an enabled
>>> + transmit/receive virtqueue
>>> whose number is \field{vqn}.
>>> \end{enumerate}
>> This is #3.
>>
>>> +If coalescing parameters are being set, the device applies the last
>>> coalescing parameters received for a
>>> +virtqueue, regardless of the command used to set the parameters.
>>> For example with 2 pairs of virtqueues:
>>> +# Command sequence
>>> +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}
>>> +
>>> \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}.
>>> @@ -1549,6 +1612,11 @@ \subsubsection{Control
>>> Virtqueue}\label{sec:Device Types / Network Device / Devi
>>>
>>> When the device has \field{max_usecs} = 0 or \field{max_packets} =
>>> 0, the notification conditions are met after every packet
>>> received/sent.
>>>
>>> +When the device receives the VIRTIO_NET_CTRL_NOTF_COAL_TX_SET and
>>> VIRTIO_NET_CTRL_NOTF_COAL_RX_SET commands,
>>> +it saves the values of coalescing parameters as global values, and
>>> the VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET command
>>> +does not change the global values. If the device is reset, the
>>> global values will be set to 0.
>>> +When a virtqueue is enabled after virtqueue reset, its coalescing
>>> parameters are set to global values.
>>> +
>> Maybe this explanation should be put closer to the commands
>> descriptions, where the global coalescing parameters are mentioned for
>> the first time.
>> Something like:
>> ....
>> \item VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET: the device returns the
>> \field{max_usecs} and \field{max_packets} parameters for an enabled
>> transmit/receive virtqueue whose number
>> is \field{vqn}.
>>
>> The device maintains global coalescing parameters for TX and RX....
>
> This is the "Operation" chapter, and the description of the operation
> is more likely to be placed here, isn't it?
>
>
>
>>
>> And maybe we should use "global coalescing parameters" instead of
>> "global values" (in devicenormative as well).
>>
>>> \subparagraph{RX Example}\label{sec:Device Types / Network Device
>>> / Device Operation / Control Virtqueue / Notifications Coalescing /
>>> RX Example}
>>>
>>> If, for example:
>>> @@ -1585,15 +1653,26 @@ \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.
>>> +If neither the VIRTIO_NET_F_NOTF_COAL nor the
>>> VIRTIO_NET_F_VQ_NOTF_COAL feature
>>> +has been negotiated, the driver MUST NOT issue
>>> VIRTIO_NET_CTRL_NOTF_COAL commands.
>> Maybe this part can be splitted to:
>> if the F1 feature has not been negotiated, the driver must not issue
>> commands C1,C2.
>> if the F2 feature has not been negotiated, the driver must not issue
>> commands C3,C4.
>
> Ok.
>
> Thanks.
>
>>> +
>>> +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 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.
>>> +
>>> +A device MUST ignore \field{reserved}.
>>>
>>> A device SHOULD NOT send used buffer notifications to the driver
>>> if the notifications are suppressed, even if the notification
>>> conditions are met.
>>>
>>> -Upon reset, a device MUST initialize all coalescing parameters to 0.
>>> +After disabling and re-enabling a virtqueue, the device MUST revert
>>> coalescing parameters of the virtqueue to the global values.
>>> +
>>> +Upon reset, a device MUST initialize all coalescing parameters to 0
>>> and MUST set the global values to 0.
>>>
>>> \subsubsection{Legacy Interface: Framing
>>> Requirements}\label{sec:Device
>>> Types / Network Device / Legacy Interface: Framing Requirements}
>>> --
>> Let's wait for more comments before the next version, I guess some may
>> not agree with my comments.
Hi, Michael and Parav, what do you think?
Thanks.
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
>> For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
---------------------------------------------------------------------
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] 25+ messages in thread
* RE: [virtio-comment] Re: [virtio-dev] Re: [PATCH v9] virtio-net: support the virtqueue coalescing moderation
2023-02-27 16:25 ` [virtio-comment] " Heng Qi
2023-02-27 16:25 ` Heng Qi
@ 2023-02-27 16:27 ` Parav Pandit
2023-02-27 16:27 ` [virtio-dev] " Parav Pandit
1 sibling, 1 reply; 25+ messages in thread
From: Parav Pandit @ 2023-02-27 16:27 UTC (permalink / raw)
To: Heng Qi, Michael S . Tsirkin, Alvaro Karsz
Cc: virtio-dev@lists.oasis-open.org,
virtio-comment@lists.oasis-open.org, David Edmondson, Jason Wang,
Xuan Zhuo
> From: virtio-comment@lists.oasis-open.org <virtio-comment@lists.oasis-
> open.org> On Behalf Of Heng Qi
> >>> \subsubsection{Legacy Interface: Framing
> >>> Requirements}\label{sec:Device
> >>> Types / Network Device / Legacy Interface: Framing Requirements}
> >>> --
> >> Let's wait for more comments before the next version, I guess some
> >> may not agree with my comments.
>
> Hi, Michael and Parav, what do you think?
I will review today.
^ permalink raw reply [flat|nested] 25+ messages in thread
* [virtio-dev] RE: [virtio-comment] Re: [virtio-dev] Re: [PATCH v9] virtio-net: support the virtqueue coalescing moderation
2023-02-27 16:27 ` [virtio-comment] " Parav Pandit
@ 2023-02-27 16:27 ` Parav Pandit
0 siblings, 0 replies; 25+ messages in thread
From: Parav Pandit @ 2023-02-27 16:27 UTC (permalink / raw)
To: Heng Qi, Michael S . Tsirkin, Alvaro Karsz
Cc: virtio-dev@lists.oasis-open.org,
virtio-comment@lists.oasis-open.org, David Edmondson, Jason Wang,
Xuan Zhuo
> From: virtio-comment@lists.oasis-open.org <virtio-comment@lists.oasis-
> open.org> On Behalf Of Heng Qi
> >>> \subsubsection{Legacy Interface: Framing
> >>> Requirements}\label{sec:Device
> >>> Types / Network Device / Legacy Interface: Framing Requirements}
> >>> --
> >> Let's wait for more comments before the next version, I guess some
> >> may not agree with my comments.
>
> Hi, Michael and Parav, what do you think?
I will review today.
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [virtio-dev] Re: [PATCH v9] virtio-net: support the virtqueue coalescing moderation
2023-02-27 13:02 ` Heng Qi
2023-02-27 16:25 ` [virtio-comment] " Heng Qi
@ 2023-02-27 18:45 ` Alvaro Karsz
2023-02-27 20:19 ` Michael S. Tsirkin
1 sibling, 1 reply; 25+ messages in thread
From: Alvaro Karsz @ 2023-02-27 18:45 UTC (permalink / raw)
To: Heng Qi
Cc: virtio-dev, virtio-comment, Michael S . Tsirkin, Parav Pandit,
David Edmondson, Jason Wang, Xuan Zhuo
> >> -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, the driver can send the VIRTIO_NET_CTRL_NOTF_COAL_TX_SET
> >> +and VIRTIO_NET_CTRL_NOTF_COAL_RX_SET commands to dynamically change the coalescing parameters for all transmit
> >> +and receive virtqueues, respectively.
> >> +
> >> +If the VIRTIO_NET_F_VQ_NOTF_COAL feature is negotiated:
> >> +\begin{itemize}
> >> +\item a driver can send a VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET command to set coalescing parameters of a given
> >> + enabled transmit/receive virtqueue.
> >> +\item a driver can send a VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET command to a device, and the device responds with
> >> + coalescing parameters of a given enabled transmit/receive virtqueue.
> >> +\end{itemize}
> >>
> > The VQ_NOTF_COAL commands are enclosed in an itemize and the NOTF_COAL
> > commands aren't.
> > I think that we should be consistent here.
>
> The VQ commands are enclosed in an itemize because it has both GET and
> SET operations.
>
> I don't think VIRTIO_NET_CTRL_NOTF_COAL_RX_SET and
> VIRTIO_NET_CTRL_NOTF_COAL_TX_SET
> have to do this, because they are both settings, just in different
> directions, and we have described them like this elsewhere.
>
> >
> > I'm not sure that describing the commands in here is necessary, maybe
> > just mentioning which commands can be used with which feature bit is
>
> Isn't that what this sentence does?
>
Yes, but it also describes what the command does
"to dynamically change the coalescing parameters for all transmit and
receive virtqueues"
Seems a bit repetitive to me.
> > enough (this is what I meant in v8, sorry if I wasn't clear).
> >
> > I'm not saying that describing the commands in here is not good, but
> > notice that the commands are described in 3 different places.
>
> Three places describe different effects.
>
> #1 describes which commands are available for which feature.
> #2 describes which command can use which structure.
> #3 describes which parameters can be set for each command, and whether
> they can affect "global values".
> It is placed in the "Operation" section because it explains how the
> device should behave.
>
You're right, but some parts seems repetitive to me:
#1: "commands to dynamically change the coalescing parameters for all
transmit and receive virtqueues"
#2: "commands use the virtio_net_ctrl_coal structure to set
\field{max_usecs} and \field{max_packets} for all transmit/receive
virtqueues."
#3: " set the \field{max_usecs} and \field{max_packets} parameters for
all transmit virtqueues, and values of..."
Feels like every time we re-explain the commands with more detail.
So, for example we read #2 and understand what the command does (set
usecs and packets), then we read #3 and understand that it does some
more stuff.
IMO we need to explain it once, with all the details.
> Maybe #1 and #2 descriptions can be combined and described together.
>
> > This is #1.
> >
> >> \begin{note}
> >> The behavior of the device in response to these commands is best-effort:
> >> @@ -1519,25 +1531,76 @@ \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}
> >>
> >> +The VIRTIO_NET_CTRL_NOTF_COAL_TX_SET and VIRTIO_NET_CTRL_NOTF_COAL_RX_SET commands use the
> >> +virtio_net_ctrl_coal structure to set \field{max_usecs} and \field{max_packets} for all
> >> +transmit/receive virtqueues.
> >> +
> >> +The VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET command uses the virtio_net_ctrl_coal_vq structure
> >> +to set \field{max_usecs} and \field{max_packets} for the supplied virtqueue number \field{vqn}.
> >> +
> >> +The VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET command gets the values of \field{max_usecs} and
> >> +\field{max_packets} of the specified virtqueue from the device by setting \field{vqn}
> >> +in the virtio_net_ctrl_coal_vq structure.
> >> +
> > This is #2.
> >
> >> +# Read/Write attributes for coalescing parameters
> >> +\begin{itemize}
> >> +\item For commands VIRTIO_NET_CTRL_NOTF_COAL_TX_SET and VIRTIO_NET_CTRL_NOTF_COAL_RX_SET, \field{max_usecs}
> >> + and \field{max_packets} are write-only for a driver.
> >> +\item For the command VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET, \field{vqn}, \field{reserved}, \field{max_usecs}
> >> + and \field{max_packets} are write-only for a driver.
> >> +\item For the command VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET, \field{vqn} and \field{reserved} are write-only
> >> + for a driver, and, \field{max_usecs} and \field{max_packets} are read-only for the driver.
> >> +\end{itemize}
> >> +
> >> 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.
> >> +
> >> +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: set the \field{max_usecs} and \field{max_packets} parameters for all transmit virtqueues, and values of
> >> + coalescing parameters are recorded as TX global values by a device.
> >> +\item VIRTIO_NET_CTRL_NOTF_COAL_RX_SET: set the \field{max_usecs} and \field{max_packets} parameters for all receive virtqueues, and values of
> >> + coalescing parameters are recorded as RX global values by a device.
> >> +\item VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET: set the \field{max_usecs} and \field{max_packets} parameters for an enabled transmit/receive
> >> + virtqueue whose number is \field{vqn}, and do not change the TX/RX global values.
> >> +\item VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET: the device returns the \field{max_usecs} and \field{max_packets} parameters for an enabled
> >> + transmit/receive virtqueue whose number is \field{vqn}.
> >> \end{enumerate}
> > This is #3.
> >
> >> +If coalescing parameters are being set, the device applies the last coalescing parameters received for a
> >> +virtqueue, regardless of the command used to set the parameters. For example with 2 pairs of virtqueues:
> >> +# Command sequence
> >> +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}
> >> +
> >> \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}.
> >> @@ -1549,6 +1612,11 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
> >>
> >> When the device has \field{max_usecs} = 0 or \field{max_packets} = 0, the notification conditions are met after every packet received/sent.
> >>
> >> +When the device receives the VIRTIO_NET_CTRL_NOTF_COAL_TX_SET and VIRTIO_NET_CTRL_NOTF_COAL_RX_SET commands,
> >> +it saves the values of coalescing parameters as global values, and the VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET command
> >> +does not change the global values. If the device is reset, the global values will be set to 0.
> >> +When a virtqueue is enabled after virtqueue reset, its coalescing parameters are set to global values.
> >> +
> > Maybe this explanation should be put closer to the commands
> > descriptions, where the global coalescing parameters are mentioned for
> > the first time.
> > Something like:
> > ....
> > \item VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET: the device returns the
> > \field{max_usecs} and \field{max_packets} parameters for an enabled
> > transmit/receive virtqueue whose number
> > is \field{vqn}.
> >
> > The device maintains global coalescing parameters for TX and RX....
>
> This is the "Operation" chapter, and the description of the operation is
> more likely to be placed here, isn't it?
>
I see your point, but a reader will see the "global notifications
coalescing parameter" concept, and won't know what it is until next
paragraph.
Maybe we can have a new list with the notification coalescing concepts
(like the notification coalescing parameters)? Just throwing an idea
here.
>
> >
> > And maybe we should use "global coalescing parameters" instead of
> > "global values" (in devicenormative as well).
> >
> >> \subparagraph{RX Example}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Notifications Coalescing / RX Example}
> >>
> >> If, for example:
> >> @@ -1585,15 +1653,26 @@ \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.
> >> +If neither the VIRTIO_NET_F_NOTF_COAL nor the VIRTIO_NET_F_VQ_NOTF_COAL feature
> >> +has been negotiated, the driver MUST NOT issue VIRTIO_NET_CTRL_NOTF_COAL commands.
> > Maybe this part can be splitted to:
> > if the F1 feature has not been negotiated, the driver must not issue
> > commands C1,C2.
> > if the F2 feature has not been negotiated, the driver must not issue
> > commands C3,C4.
>
> Ok.
>
> Thanks.
>
> >> +
> >> +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 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.
> >> +
> >> +A device MUST ignore \field{reserved}.
> >>
> >> A device SHOULD NOT send used buffer notifications to the driver if the notifications are suppressed, even if the notification conditions are met.
> >>
> >> -Upon reset, a device MUST initialize all coalescing parameters to 0.
> >> +After disabling and re-enabling a virtqueue, the device MUST revert coalescing parameters of the virtqueue to the global values.
> >> +
> >> +Upon reset, a device MUST initialize all coalescing parameters to 0 and MUST set the global values to 0.
> >>
> >> \subsubsection{Legacy Interface: Framing Requirements}\label{sec:Device
> >> Types / Network Device / Legacy Interface: Framing Requirements}
> >> --
> > Let's wait for more comments before the next version, I guess some may
> > not agree with my comments.
---------------------------------------------------------------------
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] 25+ messages in thread
* Re: [virtio-dev] Re: [PATCH v9] virtio-net: support the virtqueue coalescing moderation
2023-02-27 18:45 ` Alvaro Karsz
@ 2023-02-27 20:19 ` Michael S. Tsirkin
2023-02-27 20:41 ` [virtio-comment] " Alvaro Karsz
2023-02-28 2:41 ` [virtio-comment] " Heng Qi
0 siblings, 2 replies; 25+ messages in thread
From: Michael S. Tsirkin @ 2023-02-27 20:19 UTC (permalink / raw)
To: Alvaro Karsz
Cc: Heng Qi, virtio-dev, virtio-comment, Parav Pandit,
David Edmondson, Jason Wang, Xuan Zhuo
On Mon, Feb 27, 2023 at 08:45:43PM +0200, Alvaro Karsz wrote:
> > >> -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, the driver can send the VIRTIO_NET_CTRL_NOTF_COAL_TX_SET
> > >> +and VIRTIO_NET_CTRL_NOTF_COAL_RX_SET commands to dynamically change the coalescing parameters for all transmit
> > >> +and receive virtqueues, respectively.
> > >> +
> > >> +If the VIRTIO_NET_F_VQ_NOTF_COAL feature is negotiated:
> > >> +\begin{itemize}
> > >> +\item a driver can send a VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET command to set coalescing parameters of a given
> > >> + enabled transmit/receive virtqueue.
> > >> +\item a driver can send a VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET command to a device, and the device responds with
> > >> + coalescing parameters of a given enabled transmit/receive virtqueue.
> > >> +\end{itemize}
> > >>
> > > The VQ_NOTF_COAL commands are enclosed in an itemize and the NOTF_COAL
> > > commands aren't.
> > > I think that we should be consistent here.
> >
> > The VQ commands are enclosed in an itemize because it has both GET and
> > SET operations.
> >
> > I don't think VIRTIO_NET_CTRL_NOTF_COAL_RX_SET and
> > VIRTIO_NET_CTRL_NOTF_COAL_TX_SET
> > have to do this, because they are both settings, just in different
> > directions, and we have described them like this elsewhere.
> >
> > >
> > > I'm not sure that describing the commands in here is necessary, maybe
> > > just mentioning which commands can be used with which feature bit is
> >
> > Isn't that what this sentence does?
> >
>
> Yes, but it also describes what the command does
> "to dynamically change the coalescing parameters for all transmit and
> receive virtqueues"
>
> Seems a bit repetitive to me.
>
> > > enough (this is what I meant in v8, sorry if I wasn't clear).
> > >
> > > I'm not saying that describing the commands in here is not good, but
> > > notice that the commands are described in 3 different places.
> >
> > Three places describe different effects.
> >
> > #1 describes which commands are available for which feature.
> > #2 describes which command can use which structure.
> > #3 describes which parameters can be set for each command, and whether
> > they can affect "global values".
> > It is placed in the "Operation" section because it explains how the
> > device should behave.
> >
>
> You're right, but some parts seems repetitive to me:
> #1: "commands to dynamically change the coalescing parameters for all
> transmit and receive virtqueues"
>
> #2: "commands use the virtio_net_ctrl_coal structure to set
> \field{max_usecs} and \field{max_packets} for all transmit/receive
> virtqueues."
>
> #3: " set the \field{max_usecs} and \field{max_packets} parameters for
> all transmit virtqueues, and values of..."
>
> Feels like every time we re-explain the commands with more detail.
> So, for example we read #2 and understand what the command does (set
> usecs and packets), then we read #3 and understand that it does some
> more stuff.
>
> IMO we need to explain it once, with all the details.
This is how we've written it historically. The idea is that
reader can get everything on the first pass: 1st high level
picture then detailed description then the formalities.
Other specs do it differently so one has to re-read them
many times to understand. I don't like this personally and
I much prefer the virtio way.
> > Maybe #1 and #2 descriptions can be combined and described together.
> >
> > > This is #1.
> > >
> > >> \begin{note}
> > >> The behavior of the device in response to these commands is best-effort:
> > >> @@ -1519,25 +1531,76 @@ \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}
> > >>
> > >> +The VIRTIO_NET_CTRL_NOTF_COAL_TX_SET and VIRTIO_NET_CTRL_NOTF_COAL_RX_SET commands use the
> > >> +virtio_net_ctrl_coal structure to set \field{max_usecs} and \field{max_packets} for all
> > >> +transmit/receive virtqueues.
> > >> +
> > >> +The VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET command uses the virtio_net_ctrl_coal_vq structure
> > >> +to set \field{max_usecs} and \field{max_packets} for the supplied virtqueue number \field{vqn}.
> > >> +
> > >> +The VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET command gets the values of \field{max_usecs} and
> > >> +\field{max_packets} of the specified virtqueue from the device by setting \field{vqn}
> > >> +in the virtio_net_ctrl_coal_vq structure.
> > >> +
> > > This is #2.
> > >
> > >> +# Read/Write attributes for coalescing parameters
> > >> +\begin{itemize}
> > >> +\item For commands VIRTIO_NET_CTRL_NOTF_COAL_TX_SET and VIRTIO_NET_CTRL_NOTF_COAL_RX_SET, \field{max_usecs}
> > >> + and \field{max_packets} are write-only for a driver.
> > >> +\item For the command VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET, \field{vqn}, \field{reserved}, \field{max_usecs}
> > >> + and \field{max_packets} are write-only for a driver.
> > >> +\item For the command VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET, \field{vqn} and \field{reserved} are write-only
> > >> + for a driver, and, \field{max_usecs} and \field{max_packets} are read-only for the driver.
> > >> +\end{itemize}
> > >> +
> > >> 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.
> > >> +
> > >> +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: set the \field{max_usecs} and \field{max_packets} parameters for all transmit virtqueues, and values of
> > >> + coalescing parameters are recorded as TX global values by a device.
> > >> +\item VIRTIO_NET_CTRL_NOTF_COAL_RX_SET: set the \field{max_usecs} and \field{max_packets} parameters for all receive virtqueues, and values of
> > >> + coalescing parameters are recorded as RX global values by a device.
> > >> +\item VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET: set the \field{max_usecs} and \field{max_packets} parameters for an enabled transmit/receive
> > >> + virtqueue whose number is \field{vqn}, and do not change the TX/RX global values.
> > >> +\item VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET: the device returns the \field{max_usecs} and \field{max_packets} parameters for an enabled
> > >> + transmit/receive virtqueue whose number is \field{vqn}.
> > >> \end{enumerate}
> > > This is #3.
> > >
> > >> +If coalescing parameters are being set, the device applies the last coalescing parameters received for a
> > >> +virtqueue, regardless of the command used to set the parameters. For example with 2 pairs of virtqueues:
> > >> +# Command sequence
> > >> +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}
> > >> +
> > >> \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}.
> > >> @@ -1549,6 +1612,11 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
> > >>
> > >> When the device has \field{max_usecs} = 0 or \field{max_packets} = 0, the notification conditions are met after every packet received/sent.
> > >>
> > >> +When the device receives the VIRTIO_NET_CTRL_NOTF_COAL_TX_SET and VIRTIO_NET_CTRL_NOTF_COAL_RX_SET commands,
> > >> +it saves the values of coalescing parameters as global values, and the VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET command
> > >> +does not change the global values. If the device is reset, the global values will be set to 0.
> > >> +When a virtqueue is enabled after virtqueue reset, its coalescing parameters are set to global values.
> > >> +
> > > Maybe this explanation should be put closer to the commands
> > > descriptions, where the global coalescing parameters are mentioned for
> > > the first time.
> > > Something like:
> > > ....
> > > \item VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET: the device returns the
> > > \field{max_usecs} and \field{max_packets} parameters for an enabled
> > > transmit/receive virtqueue whose number
> > > is \field{vqn}.
> > >
> > > The device maintains global coalescing parameters for TX and RX....
> >
> > This is the "Operation" chapter, and the description of the operation is
> > more likely to be placed here, isn't it?
> >
>
> I see your point, but a reader will see the "global notifications
> coalescing parameter" concept, and won't know what it is until next
> paragraph.
> Maybe we can have a new list with the notification coalescing concepts
> (like the notification coalescing parameters)? Just throwing an idea
> here.
Actually, "global notification coalescing parameters" is confusing:
are these global notifications? global coalescings? global parameters?
The problem is the global qualifier. And it's not even global -
there are two sets for rx and for tx and does not apply to cvq at all.
How about "RX/TX coalescing parameters"?
> >
> > >
> > > And maybe we should use "global coalescing parameters" instead of
> > > "global values" (in devicenormative as well).
> > >
> > >> \subparagraph{RX Example}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Notifications Coalescing / RX Example}
> > >>
> > >> If, for example:
> > >> @@ -1585,15 +1653,26 @@ \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.
> > >> +If neither the VIRTIO_NET_F_NOTF_COAL nor the VIRTIO_NET_F_VQ_NOTF_COAL feature
> > >> +has been negotiated, the driver MUST NOT issue VIRTIO_NET_CTRL_NOTF_COAL commands.
> > > Maybe this part can be splitted to:
> > > if the F1 feature has not been negotiated, the driver must not issue
> > > commands C1,C2.
> > > if the F2 feature has not been negotiated, the driver must not issue
> > > commands C3,C4.
> >
> > Ok.
> >
> > Thanks.
> >
> > >> +
> > >> +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 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.
> > >> +
> > >> +A device MUST ignore \field{reserved}.
> > >>
> > >> A device SHOULD NOT send used buffer notifications to the driver if the notifications are suppressed, even if the notification conditions are met.
> > >>
> > >> -Upon reset, a device MUST initialize all coalescing parameters to 0.
> > >> +After disabling and re-enabling a virtqueue, the device MUST revert coalescing parameters of the virtqueue to the global values.
> > >> +
> > >> +Upon reset, a device MUST initialize all coalescing parameters to 0 and MUST set the global values to 0.
> > >>
> > >> \subsubsection{Legacy Interface: Framing Requirements}\label{sec:Device
> > >> Types / Network Device / Legacy Interface: Framing Requirements}
> > >> --
> > > Let's wait for more comments before the next version, I guess some may
> > > not agree with my comments.
---------------------------------------------------------------------
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] 25+ messages in thread
* [virtio-comment] Re: [virtio-dev] Re: [PATCH v9] virtio-net: support the virtqueue coalescing moderation
2023-02-27 20:19 ` Michael S. Tsirkin
@ 2023-02-27 20:41 ` Alvaro Karsz
2023-02-27 20:41 ` Alvaro Karsz
2023-02-28 2:41 ` [virtio-comment] " Heng Qi
1 sibling, 1 reply; 25+ messages in thread
From: Alvaro Karsz @ 2023-02-27 20:41 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Heng Qi, virtio-dev, virtio-comment, Parav Pandit,
David Edmondson, Jason Wang, Xuan Zhuo
> > I see your point, but a reader will see the "global notifications
> > coalescing parameter" concept, and won't know what it is until next
> > paragraph.
> > Maybe we can have a new list with the notification coalescing concepts
> > (like the notification coalescing parameters)? Just throwing an idea
> > here.
>
> Actually, "global notification coalescing parameters" is confusing:
> are these global notifications? global coalescings? global parameters?
>
> The problem is the global qualifier. And it's not even global -
> there are two sets for rx and for tx and does not apply to cvq at all.
> How about "RX/TX coalescing parameters"?
Seems fine to me.
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/
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [virtio-dev] Re: [PATCH v9] virtio-net: support the virtqueue coalescing moderation
2023-02-27 20:41 ` [virtio-comment] " Alvaro Karsz
@ 2023-02-27 20:41 ` Alvaro Karsz
0 siblings, 0 replies; 25+ messages in thread
From: Alvaro Karsz @ 2023-02-27 20:41 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Heng Qi, virtio-dev, virtio-comment, Parav Pandit,
David Edmondson, Jason Wang, Xuan Zhuo
> > I see your point, but a reader will see the "global notifications
> > coalescing parameter" concept, and won't know what it is until next
> > paragraph.
> > Maybe we can have a new list with the notification coalescing concepts
> > (like the notification coalescing parameters)? Just throwing an idea
> > here.
>
> Actually, "global notification coalescing parameters" is confusing:
> are these global notifications? global coalescings? global parameters?
>
> The problem is the global qualifier. And it's not even global -
> there are two sets for rx and for tx and does not apply to cvq at all.
> How about "RX/TX coalescing parameters"?
Seems fine to me.
---------------------------------------------------------------------
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] 25+ messages in thread
* [virtio-comment] RE: [PATCH v9] virtio-net: support the virtqueue coalescing moderation
2023-02-26 9:37 [virtio-comment] [PATCH v9] virtio-net: support the virtqueue coalescing moderation Heng Qi
2023-02-26 9:37 ` [virtio-dev] " Heng Qi
2023-02-26 19:33 ` [virtio-comment] " Alvaro Karsz
@ 2023-02-28 1:40 ` Parav Pandit
2023-02-28 1:40 ` [virtio-dev] " Parav Pandit
` (2 more replies)
2 siblings, 3 replies; 25+ messages in thread
From: Parav Pandit @ 2023-02-28 1:40 UTC (permalink / raw)
To: Heng Qi, virtio-dev@lists.oasis-open.org,
virtio-comment@lists.oasis-open.org
Cc: Alvaro Karsz, Michael S . Tsirkin, David Edmondson, Jason Wang,
Xuan Zhuo
> From: Heng Qi <hengqi@linux.alibaba.com>
> Sent: Sunday, February 26, 2023 4:37 AM
> +# Read/Write attributes for coalescing parameters \begin{itemize} \item
Hash # above is not needed. Please remove.
> +For commands VIRTIO_NET_CTRL_NOTF_COAL_TX_SET and
> VIRTIO_NET_CTRL_NOTF_COAL_RX_SET, \field{max_usecs}
> + and \field{max_packets} are write-only for a driver.
> +\item For the command VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET, \field{vqn},
> \field{reserved}, \field{max_usecs}
> + and \field{max_packets} are write-only for a driver.
> +\item For the command VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET, \field{vqn}
> and \field{reserved} are write-only
> + for a driver, and, \field{max_usecs} and \field{max_packets} are read-only
> for the driver.
> +\end{itemize}
Instead of describing each individual fields of the structure struct virtio_net_ctrl_coal coal as read-only or write-only, better to say the struct itself as read/write only.
This way when/if in the future, if struct virtio_net_ctrl_coal coal extends, no need to rewrite this section of the spec.
> +
> 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.
> +
> +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: set the \field{max_usecs} and
> \field{max_packets} parameters for all transmit virtqueues, and values of
> + coalescing parameters are recorded as TX global values
> by a device.
"recording TX global values" is not conveying anything more as "all transmit virtqueues" is already mentioned.
The original text without "recorded as Tx global values" is just fine. Same for the RX below.
> +\item VIRTIO_NET_CTRL_NOTF_COAL_RX_SET: set the \field{max_usecs} and
> \field{max_packets} parameters for all receive virtqueues, and values of
> + coalescing parameters are recorded as RX global values
> by a device.
> +\item VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET: set the \field{max_usecs} and
> \field{max_packets} parameters for an enabled transmit/receive
> + virtqueue whose number is \field{vqn}, and do not
> change the TX/RX global values.
> +\item VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET: the device returns the
> \field{max_usecs} and \field{max_packets} parameters for an enabled
> + transmit/receive virtqueue whose number is \field{vqn}.
> \end{enumerate}
>
> +If coalescing parameters are being set, the device applies the last
> +coalescing parameters received for a virtqueue, regardless of the command
> used to set the parameters. For example with 2 pairs of virtqueues:
> +# Command sequence
> +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}
> +
> \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}.
> @@ -1549,6 +1612,11 @@ \subsubsection{Control
> Virtqueue}\label{sec:Device Types / Network Device / Devi
>
> When the device has \field{max_usecs} = 0 or \field{max_packets} = 0, the
> notification conditions are met after every packet received/sent.
>
> +When the device receives the VIRTIO_NET_CTRL_NOTF_COAL_TX_SET and
> +VIRTIO_NET_CTRL_NOTF_COAL_RX_SET commands, it saves the values of
> +coalescing parameters as global values, and the
> VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET command does not change the global
> values. If the device is reset, the global values will be set to 0.
> +When a virtqueue is enabled after virtqueue reset, its coalescing parameters
> are set to global values.
> +
> \subparagraph{RX Example}\label{sec:Device Types / Network Device / Device
> Operation / Control Virtqueue / Notifications Coalescing / RX Example}
>
> If, for example:
> @@ -1585,15 +1653,26 @@ \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.
> +If neither the VIRTIO_NET_F_NOTF_COAL nor the
> VIRTIO_NET_F_VQ_NOTF_COAL
> +feature has been negotiated, the driver MUST NOT issue
> VIRTIO_NET_CTRL_NOTF_COAL commands.
> +
I agree to Alvaro's suggestion.
If VIRTIO_NET_F_NOTF_COAL is not negotiated, the driver must not issue commands A and B.
If VIRTIO_NET_F_VQ_NOTF_COAL is not negotiated, the driver must not issue commands C and D.
> +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 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.
> +
> +A device MUST ignore \field{reserved}.
>
> A device SHOULD NOT send used buffer notifications to the driver if the
> notifications are suppressed, even if the notification conditions are met.
>
> -Upon reset, a device MUST initialize all coalescing parameters to 0.
> +After disabling and re-enabling a virtqueue, the device MUST revert coalescing
revert to
> parameters of the virtqueue to the global values.
> +
Like others suggested, above can be reworded as,
After disabling and re-enabling a virtuqueue, the device MUST revert to coalescing parameters to the one configured using VIRTIO_NET_CTRL_NOTF_COAL.
> +Upon reset, a device MUST initialize all coalescing parameters to 0 and MUST
> set the global values to 0.
>
No need to mention global values here. "all coalescing parameters to 0" covers per vq and per group of tx and rx vqs.
> \subsubsection{Legacy Interface: Framing Requirements}\label{sec:Device
> Types / Network Device / Legacy Interface: Framing Requirements}
> --
> 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/
^ permalink raw reply [flat|nested] 25+ messages in thread
* [virtio-dev] RE: [PATCH v9] virtio-net: support the virtqueue coalescing moderation
2023-02-28 1:40 ` [virtio-comment] " Parav Pandit
@ 2023-02-28 1:40 ` Parav Pandit
2023-02-28 2:19 ` [virtio-comment] " Heng Qi
2023-02-28 7:47 ` [virtio-comment] " Michael S. Tsirkin
2 siblings, 0 replies; 25+ messages in thread
From: Parav Pandit @ 2023-02-28 1:40 UTC (permalink / raw)
To: Heng Qi, virtio-dev@lists.oasis-open.org,
virtio-comment@lists.oasis-open.org
Cc: Alvaro Karsz, Michael S . Tsirkin, David Edmondson, Jason Wang,
Xuan Zhuo
> From: Heng Qi <hengqi@linux.alibaba.com>
> Sent: Sunday, February 26, 2023 4:37 AM
> +# Read/Write attributes for coalescing parameters \begin{itemize} \item
Hash # above is not needed. Please remove.
> +For commands VIRTIO_NET_CTRL_NOTF_COAL_TX_SET and
> VIRTIO_NET_CTRL_NOTF_COAL_RX_SET, \field{max_usecs}
> + and \field{max_packets} are write-only for a driver.
> +\item For the command VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET, \field{vqn},
> \field{reserved}, \field{max_usecs}
> + and \field{max_packets} are write-only for a driver.
> +\item For the command VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET, \field{vqn}
> and \field{reserved} are write-only
> + for a driver, and, \field{max_usecs} and \field{max_packets} are read-only
> for the driver.
> +\end{itemize}
Instead of describing each individual fields of the structure struct virtio_net_ctrl_coal coal as read-only or write-only, better to say the struct itself as read/write only.
This way when/if in the future, if struct virtio_net_ctrl_coal coal extends, no need to rewrite this section of the spec.
> +
> 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.
> +
> +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: set the \field{max_usecs} and
> \field{max_packets} parameters for all transmit virtqueues, and values of
> + coalescing parameters are recorded as TX global values
> by a device.
"recording TX global values" is not conveying anything more as "all transmit virtqueues" is already mentioned.
The original text without "recorded as Tx global values" is just fine. Same for the RX below.
> +\item VIRTIO_NET_CTRL_NOTF_COAL_RX_SET: set the \field{max_usecs} and
> \field{max_packets} parameters for all receive virtqueues, and values of
> + coalescing parameters are recorded as RX global values
> by a device.
> +\item VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET: set the \field{max_usecs} and
> \field{max_packets} parameters for an enabled transmit/receive
> + virtqueue whose number is \field{vqn}, and do not
> change the TX/RX global values.
> +\item VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET: the device returns the
> \field{max_usecs} and \field{max_packets} parameters for an enabled
> + transmit/receive virtqueue whose number is \field{vqn}.
> \end{enumerate}
>
> +If coalescing parameters are being set, the device applies the last
> +coalescing parameters received for a virtqueue, regardless of the command
> used to set the parameters. For example with 2 pairs of virtqueues:
> +# Command sequence
> +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}
> +
> \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}.
> @@ -1549,6 +1612,11 @@ \subsubsection{Control
> Virtqueue}\label{sec:Device Types / Network Device / Devi
>
> When the device has \field{max_usecs} = 0 or \field{max_packets} = 0, the
> notification conditions are met after every packet received/sent.
>
> +When the device receives the VIRTIO_NET_CTRL_NOTF_COAL_TX_SET and
> +VIRTIO_NET_CTRL_NOTF_COAL_RX_SET commands, it saves the values of
> +coalescing parameters as global values, and the
> VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET command does not change the global
> values. If the device is reset, the global values will be set to 0.
> +When a virtqueue is enabled after virtqueue reset, its coalescing parameters
> are set to global values.
> +
> \subparagraph{RX Example}\label{sec:Device Types / Network Device / Device
> Operation / Control Virtqueue / Notifications Coalescing / RX Example}
>
> If, for example:
> @@ -1585,15 +1653,26 @@ \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.
> +If neither the VIRTIO_NET_F_NOTF_COAL nor the
> VIRTIO_NET_F_VQ_NOTF_COAL
> +feature has been negotiated, the driver MUST NOT issue
> VIRTIO_NET_CTRL_NOTF_COAL commands.
> +
I agree to Alvaro's suggestion.
If VIRTIO_NET_F_NOTF_COAL is not negotiated, the driver must not issue commands A and B.
If VIRTIO_NET_F_VQ_NOTF_COAL is not negotiated, the driver must not issue commands C and D.
> +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 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.
> +
> +A device MUST ignore \field{reserved}.
>
> A device SHOULD NOT send used buffer notifications to the driver if the
> notifications are suppressed, even if the notification conditions are met.
>
> -Upon reset, a device MUST initialize all coalescing parameters to 0.
> +After disabling and re-enabling a virtqueue, the device MUST revert coalescing
revert to
> parameters of the virtqueue to the global values.
> +
Like others suggested, above can be reworded as,
After disabling and re-enabling a virtuqueue, the device MUST revert to coalescing parameters to the one configured using VIRTIO_NET_CTRL_NOTF_COAL.
> +Upon reset, a device MUST initialize all coalescing parameters to 0 and MUST
> set the global values to 0.
>
No need to mention global values here. "all coalescing parameters to 0" covers per vq and per group of tx and rx vqs.
> \subsubsection{Legacy Interface: Framing Requirements}\label{sec:Device
> Types / Network Device / Legacy Interface: Framing Requirements}
> --
> 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 [flat|nested] 25+ messages in thread
* Re: [virtio-comment] RE: [PATCH v9] virtio-net: support the virtqueue coalescing moderation
2023-02-28 1:40 ` [virtio-comment] " Parav Pandit
2023-02-28 1:40 ` [virtio-dev] " Parav Pandit
@ 2023-02-28 2:19 ` Heng Qi
2023-02-28 2:19 ` [virtio-dev] " Heng Qi
2023-02-28 7:47 ` [virtio-comment] " Michael S. Tsirkin
2 siblings, 1 reply; 25+ messages in thread
From: Heng Qi @ 2023-02-28 2:19 UTC (permalink / raw)
To: Parav Pandit, virtio-dev@lists.oasis-open.org,
virtio-comment@lists.oasis-open.org
Cc: Alvaro Karsz, Michael S . Tsirkin, David Edmondson, Jason Wang,
Xuan Zhuo
在 2023/2/28 上午9:40, Parav Pandit 写道:
>> From: Heng Qi <hengqi@linux.alibaba.com>
>> Sent: Sunday, February 26, 2023 4:37 AM
>> +# Read/Write attributes for coalescing parameters \begin{itemize} \item
> Hash # above is not needed. Please remove.
Ok.
>
>> +For commands VIRTIO_NET_CTRL_NOTF_COAL_TX_SET and
>> VIRTIO_NET_CTRL_NOTF_COAL_RX_SET, \field{max_usecs}
>> + and \field{max_packets} are write-only for a driver.
>> +\item For the command VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET, \field{vqn},
>> \field{reserved}, \field{max_usecs}
>> + and \field{max_packets} are write-only for a driver.
>> +\item For the command VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET, \field{vqn}
>> and \field{reserved} are write-only
>> + for a driver, and, \field{max_usecs} and \field{max_packets} are read-only
>> for the driver.
>> +\end{itemize}
> Instead of describing each individual fields of the structure struct virtio_net_ctrl_coal coal as read-only or write-only, better to say the struct itself as read/write only.
> This way when/if in the future, if struct virtio_net_ctrl_coal coal extends, no need to rewrite this section of the spec.
Good idea, I will modify this.
>
>> +
>> 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.
>> +
>> +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: set the \field{max_usecs} and
>> \field{max_packets} parameters for all transmit virtqueues, and values of
>> + coalescing parameters are recorded as TX global values
>> by a device.
> "recording TX global values" is not conveying anything more as "all transmit virtqueues" is already mentioned.
> The original text without "recorded as Tx global values" is just fine. Same for the RX below.
>
>> +\item VIRTIO_NET_CTRL_NOTF_COAL_RX_SET: set the \field{max_usecs} and
>> \field{max_packets} parameters for all receive virtqueues, and values of
>> + coalescing parameters are recorded as RX global values
>> by a device.
>> +\item VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET: set the \field{max_usecs} and
>> \field{max_packets} parameters for an enabled transmit/receive
>> + virtqueue whose number is \field{vqn}, and do not
>> change the TX/RX global values.
>> +\item VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET: the device returns the
>> \field{max_usecs} and \field{max_packets} parameters for an enabled
>> + transmit/receive virtqueue whose number is \field{vqn}.
>> \end{enumerate}
>>
>> +If coalescing parameters are being set, the device applies the last
>> +coalescing parameters received for a virtqueue, regardless of the command
>> used to set the parameters. For example with 2 pairs of virtqueues:
>> +# Command sequence
>> +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}
>> +
>> \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}.
>> @@ -1549,6 +1612,11 @@ \subsubsection{Control
>> Virtqueue}\label{sec:Device Types / Network Device / Devi
>>
>> When the device has \field{max_usecs} = 0 or \field{max_packets} = 0, the
>> notification conditions are met after every packet received/sent.
>>
>> +When the device receives the VIRTIO_NET_CTRL_NOTF_COAL_TX_SET and
>> +VIRTIO_NET_CTRL_NOTF_COAL_RX_SET commands, it saves the values of
>> +coalescing parameters as global values, and the
>> VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET command does not change the global
>> values. If the device is reset, the global values will be set to 0.
>> +When a virtqueue is enabled after virtqueue reset, its coalescing parameters
>> are set to global values.
>> +
>> \subparagraph{RX Example}\label{sec:Device Types / Network Device / Device
>> Operation / Control Virtqueue / Notifications Coalescing / RX Example}
>>
>> If, for example:
>> @@ -1585,15 +1653,26 @@ \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.
>> +If neither the VIRTIO_NET_F_NOTF_COAL nor the
>> VIRTIO_NET_F_VQ_NOTF_COAL
>> +feature has been negotiated, the driver MUST NOT issue
>> VIRTIO_NET_CTRL_NOTF_COAL commands.
>> +
> I agree to Alvaro's suggestion.
> If VIRTIO_NET_F_NOTF_COAL is not negotiated, the driver must not issue commands A and B.
>
> If VIRTIO_NET_F_VQ_NOTF_COAL is not negotiated, the driver must not issue commands C and D.
>
>> +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 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.
>> +
>> +A device MUST ignore \field{reserved}.
>>
>> A device SHOULD NOT send used buffer notifications to the driver if the
>> notifications are suppressed, even if the notification conditions are met.
>>
>> -Upon reset, a device MUST initialize all coalescing parameters to 0.
>> +After disabling and re-enabling a virtqueue, the device MUST revert coalescing
> revert to
Thanks for pointing out.
>> parameters of the virtqueue to the global values.
>> +
> Like others suggested, above can be reworded as,
>
> After disabling and re-enabling a virtuqueue, the device MUST revert to coalescing parameters to the one configured using VIRTIO_NET_CTRL_NOTF_COAL.
I prefer this, I think it's better not to mention "global values".
>
>> +Upon reset, a device MUST initialize all coalescing parameters to 0 and MUST
>> set the global values to 0.
>>
> No need to mention global values here. "all coalescing parameters to 0" covers per vq and per group of tx and rx vqs.
Ok.
Thanks!:)
>
>> \subsubsection{Legacy Interface: Framing Requirements}\label{sec:Device
>> Types / Network Device / Legacy Interface: Framing Requirements}
>> --
>> 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/
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/
^ permalink raw reply [flat|nested] 25+ messages in thread
* [virtio-dev] Re: [virtio-comment] RE: [PATCH v9] virtio-net: support the virtqueue coalescing moderation
2023-02-28 2:19 ` [virtio-comment] " Heng Qi
@ 2023-02-28 2:19 ` Heng Qi
0 siblings, 0 replies; 25+ messages in thread
From: Heng Qi @ 2023-02-28 2:19 UTC (permalink / raw)
To: Parav Pandit, virtio-dev@lists.oasis-open.org,
virtio-comment@lists.oasis-open.org
Cc: Alvaro Karsz, Michael S . Tsirkin, David Edmondson, Jason Wang,
Xuan Zhuo
在 2023/2/28 上午9:40, Parav Pandit 写道:
>> From: Heng Qi <hengqi@linux.alibaba.com>
>> Sent: Sunday, February 26, 2023 4:37 AM
>> +# Read/Write attributes for coalescing parameters \begin{itemize} \item
> Hash # above is not needed. Please remove.
Ok.
>
>> +For commands VIRTIO_NET_CTRL_NOTF_COAL_TX_SET and
>> VIRTIO_NET_CTRL_NOTF_COAL_RX_SET, \field{max_usecs}
>> + and \field{max_packets} are write-only for a driver.
>> +\item For the command VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET, \field{vqn},
>> \field{reserved}, \field{max_usecs}
>> + and \field{max_packets} are write-only for a driver.
>> +\item For the command VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET, \field{vqn}
>> and \field{reserved} are write-only
>> + for a driver, and, \field{max_usecs} and \field{max_packets} are read-only
>> for the driver.
>> +\end{itemize}
> Instead of describing each individual fields of the structure struct virtio_net_ctrl_coal coal as read-only or write-only, better to say the struct itself as read/write only.
> This way when/if in the future, if struct virtio_net_ctrl_coal coal extends, no need to rewrite this section of the spec.
Good idea, I will modify this.
>
>> +
>> 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.
>> +
>> +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: set the \field{max_usecs} and
>> \field{max_packets} parameters for all transmit virtqueues, and values of
>> + coalescing parameters are recorded as TX global values
>> by a device.
> "recording TX global values" is not conveying anything more as "all transmit virtqueues" is already mentioned.
> The original text without "recorded as Tx global values" is just fine. Same for the RX below.
>
>> +\item VIRTIO_NET_CTRL_NOTF_COAL_RX_SET: set the \field{max_usecs} and
>> \field{max_packets} parameters for all receive virtqueues, and values of
>> + coalescing parameters are recorded as RX global values
>> by a device.
>> +\item VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET: set the \field{max_usecs} and
>> \field{max_packets} parameters for an enabled transmit/receive
>> + virtqueue whose number is \field{vqn}, and do not
>> change the TX/RX global values.
>> +\item VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET: the device returns the
>> \field{max_usecs} and \field{max_packets} parameters for an enabled
>> + transmit/receive virtqueue whose number is \field{vqn}.
>> \end{enumerate}
>>
>> +If coalescing parameters are being set, the device applies the last
>> +coalescing parameters received for a virtqueue, regardless of the command
>> used to set the parameters. For example with 2 pairs of virtqueues:
>> +# Command sequence
>> +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}
>> +
>> \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}.
>> @@ -1549,6 +1612,11 @@ \subsubsection{Control
>> Virtqueue}\label{sec:Device Types / Network Device / Devi
>>
>> When the device has \field{max_usecs} = 0 or \field{max_packets} = 0, the
>> notification conditions are met after every packet received/sent.
>>
>> +When the device receives the VIRTIO_NET_CTRL_NOTF_COAL_TX_SET and
>> +VIRTIO_NET_CTRL_NOTF_COAL_RX_SET commands, it saves the values of
>> +coalescing parameters as global values, and the
>> VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET command does not change the global
>> values. If the device is reset, the global values will be set to 0.
>> +When a virtqueue is enabled after virtqueue reset, its coalescing parameters
>> are set to global values.
>> +
>> \subparagraph{RX Example}\label{sec:Device Types / Network Device / Device
>> Operation / Control Virtqueue / Notifications Coalescing / RX Example}
>>
>> If, for example:
>> @@ -1585,15 +1653,26 @@ \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.
>> +If neither the VIRTIO_NET_F_NOTF_COAL nor the
>> VIRTIO_NET_F_VQ_NOTF_COAL
>> +feature has been negotiated, the driver MUST NOT issue
>> VIRTIO_NET_CTRL_NOTF_COAL commands.
>> +
> I agree to Alvaro's suggestion.
> If VIRTIO_NET_F_NOTF_COAL is not negotiated, the driver must not issue commands A and B.
>
> If VIRTIO_NET_F_VQ_NOTF_COAL is not negotiated, the driver must not issue commands C and D.
>
>> +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 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.
>> +
>> +A device MUST ignore \field{reserved}.
>>
>> A device SHOULD NOT send used buffer notifications to the driver if the
>> notifications are suppressed, even if the notification conditions are met.
>>
>> -Upon reset, a device MUST initialize all coalescing parameters to 0.
>> +After disabling and re-enabling a virtqueue, the device MUST revert coalescing
> revert to
Thanks for pointing out.
>> parameters of the virtqueue to the global values.
>> +
> Like others suggested, above can be reworded as,
>
> After disabling and re-enabling a virtuqueue, the device MUST revert to coalescing parameters to the one configured using VIRTIO_NET_CTRL_NOTF_COAL.
I prefer this, I think it's better not to mention "global values".
>
>> +Upon reset, a device MUST initialize all coalescing parameters to 0 and MUST
>> set the global values to 0.
>>
> No need to mention global values here. "all coalescing parameters to 0" covers per vq and per group of tx and rx vqs.
Ok.
Thanks!:)
>
>> \subsubsection{Legacy Interface: Framing Requirements}\label{sec:Device
>> Types / Network Device / Legacy Interface: Framing Requirements}
>> --
>> 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/
---------------------------------------------------------------------
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] 25+ messages in thread
* [virtio-comment] Re: [virtio-dev] Re: [PATCH v9] virtio-net: support the virtqueue coalescing moderation
2023-02-27 20:19 ` Michael S. Tsirkin
2023-02-27 20:41 ` [virtio-comment] " Alvaro Karsz
@ 2023-02-28 2:41 ` Heng Qi
2023-02-28 2:41 ` Heng Qi
2023-02-28 7:49 ` [virtio-comment] " Michael S. Tsirkin
1 sibling, 2 replies; 25+ messages in thread
From: Heng Qi @ 2023-02-28 2:41 UTC (permalink / raw)
To: Michael S. Tsirkin, Alvaro Karsz
Cc: virtio-dev, virtio-comment, Parav Pandit, David Edmondson,
Jason Wang, Xuan Zhuo
在 2023/2/28 上午4:19, Michael S. Tsirkin 写道:
> On Mon, Feb 27, 2023 at 08:45:43PM +0200, Alvaro Karsz wrote:
>>>>> -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, the driver can send the VIRTIO_NET_CTRL_NOTF_COAL_TX_SET
>>>>> +and VIRTIO_NET_CTRL_NOTF_COAL_RX_SET commands to dynamically change the coalescing parameters for all transmit
>>>>> +and receive virtqueues, respectively.
>>>>> +
>>>>> +If the VIRTIO_NET_F_VQ_NOTF_COAL feature is negotiated:
>>>>> +\begin{itemize}
>>>>> +\item a driver can send a VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET command to set coalescing parameters of a given
>>>>> + enabled transmit/receive virtqueue.
>>>>> +\item a driver can send a VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET command to a device, and the device responds with
>>>>> + coalescing parameters of a given enabled transmit/receive virtqueue.
>>>>> +\end{itemize}
>>>>>
>>>> The VQ_NOTF_COAL commands are enclosed in an itemize and the NOTF_COAL
>>>> commands aren't.
>>>> I think that we should be consistent here.
>>> The VQ commands are enclosed in an itemize because it has both GET and
>>> SET operations.
>>>
>>> I don't think VIRTIO_NET_CTRL_NOTF_COAL_RX_SET and
>>> VIRTIO_NET_CTRL_NOTF_COAL_TX_SET
>>> have to do this, because they are both settings, just in different
>>> directions, and we have described them like this elsewhere.
>>>
>>>> I'm not sure that describing the commands in here is necessary, maybe
>>>> just mentioning which commands can be used with which feature bit is
>>> Isn't that what this sentence does?
>>>
>> Yes, but it also describes what the command does
>> "to dynamically change the coalescing parameters for all transmit and
>> receive virtqueues"
>>
>> Seems a bit repetitive to me.
>>
>>>> enough (this is what I meant in v8, sorry if I wasn't clear).
>>>>
>>>> I'm not saying that describing the commands in here is not good, but
>>>> notice that the commands are described in 3 different places.
>>> Three places describe different effects.
>>>
>>> #1 describes which commands are available for which feature.
>>> #2 describes which command can use which structure.
>>> #3 describes which parameters can be set for each command, and whether
>>> they can affect "global values".
>>> It is placed in the "Operation" section because it explains how the
>>> device should behave.
>>>
>> You're right, but some parts seems repetitive to me:
>> #1: "commands to dynamically change the coalescing parameters for all
>> transmit and receive virtqueues"
>>
>> #2: "commands use the virtio_net_ctrl_coal structure to set
>> \field{max_usecs} and \field{max_packets} for all transmit/receive
>> virtqueues."
>>
>> #3: " set the \field{max_usecs} and \field{max_packets} parameters for
>> all transmit virtqueues, and values of..."
>>
>> Feels like every time we re-explain the commands with more detail.
>> So, for example we read #2 and understand what the command does (set
>> usecs and packets), then we read #3 and understand that it does some
>> more stuff.
>>
>> IMO we need to explain it once, with all the details.
>
> This is how we've written it historically. The idea is that
> reader can get everything on the first pass: 1st high level
> picture then detailed description then the formalities.
>
> Other specs do it differently so one has to re-read them
> many times to understand. I don't like this personally and
> I much prefer the virtio way.
>
>
>>> Maybe #1 and #2 descriptions can be combined and described together.
>>>
>>>> This is #1.
>>>>
>>>>> \begin{note}
>>>>> The behavior of the device in response to these commands is best-effort:
>>>>> @@ -1519,25 +1531,76 @@ \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}
>>>>>
>>>>> +The VIRTIO_NET_CTRL_NOTF_COAL_TX_SET and VIRTIO_NET_CTRL_NOTF_COAL_RX_SET commands use the
>>>>> +virtio_net_ctrl_coal structure to set \field{max_usecs} and \field{max_packets} for all
>>>>> +transmit/receive virtqueues.
>>>>> +
>>>>> +The VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET command uses the virtio_net_ctrl_coal_vq structure
>>>>> +to set \field{max_usecs} and \field{max_packets} for the supplied virtqueue number \field{vqn}.
>>>>> +
>>>>> +The VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET command gets the values of \field{max_usecs} and
>>>>> +\field{max_packets} of the specified virtqueue from the device by setting \field{vqn}
>>>>> +in the virtio_net_ctrl_coal_vq structure.
>>>>> +
>>>> This is #2.
>>>>
>>>>> +# Read/Write attributes for coalescing parameters
>>>>> +\begin{itemize}
>>>>> +\item For commands VIRTIO_NET_CTRL_NOTF_COAL_TX_SET and VIRTIO_NET_CTRL_NOTF_COAL_RX_SET, \field{max_usecs}
>>>>> + and \field{max_packets} are write-only for a driver.
>>>>> +\item For the command VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET, \field{vqn}, \field{reserved}, \field{max_usecs}
>>>>> + and \field{max_packets} are write-only for a driver.
>>>>> +\item For the command VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET, \field{vqn} and \field{reserved} are write-only
>>>>> + for a driver, and, \field{max_usecs} and \field{max_packets} are read-only for the driver.
>>>>> +\end{itemize}
>>>>> +
>>>>> 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.
>>>>> +
>>>>> +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: set the \field{max_usecs} and \field{max_packets} parameters for all transmit virtqueues, and values of
>>>>> + coalescing parameters are recorded as TX global values by a device.
>>>>> +\item VIRTIO_NET_CTRL_NOTF_COAL_RX_SET: set the \field{max_usecs} and \field{max_packets} parameters for all receive virtqueues, and values of
>>>>> + coalescing parameters are recorded as RX global values by a device.
>>>>> +\item VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET: set the \field{max_usecs} and \field{max_packets} parameters for an enabled transmit/receive
>>>>> + virtqueue whose number is \field{vqn}, and do not change the TX/RX global values.
>>>>> +\item VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET: the device returns the \field{max_usecs} and \field{max_packets} parameters for an enabled
>>>>> + transmit/receive virtqueue whose number is \field{vqn}.
>>>>> \end{enumerate}
>>>> This is #3.
>>>>
>>>>> +If coalescing parameters are being set, the device applies the last coalescing parameters received for a
>>>>> +virtqueue, regardless of the command used to set the parameters. For example with 2 pairs of virtqueues:
>>>>> +# Command sequence
>>>>> +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}
>>>>> +
>>>>> \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}.
>>>>> @@ -1549,6 +1612,11 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
>>>>>
>>>>> When the device has \field{max_usecs} = 0 or \field{max_packets} = 0, the notification conditions are met after every packet received/sent.
>>>>>
>>>>> +When the device receives the VIRTIO_NET_CTRL_NOTF_COAL_TX_SET and VIRTIO_NET_CTRL_NOTF_COAL_RX_SET commands,
>>>>> +it saves the values of coalescing parameters as global values, and the VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET command
>>>>> +does not change the global values. If the device is reset, the global values will be set to 0.
>>>>> +When a virtqueue is enabled after virtqueue reset, its coalescing parameters are set to global values.
>>>>> +
>>>> Maybe this explanation should be put closer to the commands
>>>> descriptions, where the global coalescing parameters are mentioned for
>>>> the first time.
>>>> Something like:
>>>> ....
>>>> \item VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET: the device returns the
>>>> \field{max_usecs} and \field{max_packets} parameters for an enabled
>>>> transmit/receive virtqueue whose number
>>>> is \field{vqn}.
>>>>
>>>> The device maintains global coalescing parameters for TX and RX....
>>> This is the "Operation" chapter, and the description of the operation is
>>> more likely to be placed here, isn't it?
>>>
>> I see your point, but a reader will see the "global notifications
>> coalescing parameter" concept, and won't know what it is until next
>> paragraph.
>> Maybe we can have a new list with the notification coalescing concepts
>> (like the notification coalescing parameters)? Just throwing an idea
>> here.
> Actually, "global notification coalescing parameters" is confusing:
> are these global notifications? global coalescings? global parameters?
>
> The problem is the global qualifier. And it's not even global -
> there are two sets for rx and for tx and does not apply to cvq at all.
> How about "RX/TX coalescing parameters"?
As Parav suggested, we probably don't need to mention "global
values/coalescing parameters" since this values/parameters are the ones
set by CTRL_NOTF_COAL_RX/TX_SET.
Just like this:
"
After disabling and re-enabling a virtuqueue, the device MUST revert to
coalescing parameters to the one configured using VIRTIO_NET_CTRL_NOTF_COAL.
"
Thanks.
>
>
>>>> And maybe we should use "global coalescing parameters" instead of
>>>> "global values" (in devicenormative as well).
>>>>
>>>>> \subparagraph{RX Example}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Notifications Coalescing / RX Example}
>>>>>
>>>>> If, for example:
>>>>> @@ -1585,15 +1653,26 @@ \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.
>>>>> +If neither the VIRTIO_NET_F_NOTF_COAL nor the VIRTIO_NET_F_VQ_NOTF_COAL feature
>>>>> +has been negotiated, the driver MUST NOT issue VIRTIO_NET_CTRL_NOTF_COAL commands.
>>>> Maybe this part can be splitted to:
>>>> if the F1 feature has not been negotiated, the driver must not issue
>>>> commands C1,C2.
>>>> if the F2 feature has not been negotiated, the driver must not issue
>>>> commands C3,C4.
>>> Ok.
>>>
>>> Thanks.
>>>
>>>>> +
>>>>> +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 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.
>>>>> +
>>>>> +A device MUST ignore \field{reserved}.
>>>>>
>>>>> A device SHOULD NOT send used buffer notifications to the driver if the notifications are suppressed, even if the notification conditions are met.
>>>>>
>>>>> -Upon reset, a device MUST initialize all coalescing parameters to 0.
>>>>> +After disabling and re-enabling a virtqueue, the device MUST revert coalescing parameters of the virtqueue to the global values.
>>>>> +
>>>>> +Upon reset, a device MUST initialize all coalescing parameters to 0 and MUST set the global values to 0.
>>>>>
>>>>> \subsubsection{Legacy Interface: Framing Requirements}\label{sec:Device
>>>>> Types / Network Device / Legacy Interface: Framing Requirements}
>>>>> --
>>>> Let's wait for more comments before the next version, I guess some may
>>>> not agree with my comments.
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/
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [virtio-dev] Re: [PATCH v9] virtio-net: support the virtqueue coalescing moderation
2023-02-28 2:41 ` [virtio-comment] " Heng Qi
@ 2023-02-28 2:41 ` Heng Qi
2023-02-28 7:49 ` [virtio-comment] " Michael S. Tsirkin
1 sibling, 0 replies; 25+ messages in thread
From: Heng Qi @ 2023-02-28 2:41 UTC (permalink / raw)
To: Michael S. Tsirkin, Alvaro Karsz
Cc: virtio-dev, virtio-comment, Parav Pandit, David Edmondson,
Jason Wang, Xuan Zhuo
在 2023/2/28 上午4:19, Michael S. Tsirkin 写道:
> On Mon, Feb 27, 2023 at 08:45:43PM +0200, Alvaro Karsz wrote:
>>>>> -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, the driver can send the VIRTIO_NET_CTRL_NOTF_COAL_TX_SET
>>>>> +and VIRTIO_NET_CTRL_NOTF_COAL_RX_SET commands to dynamically change the coalescing parameters for all transmit
>>>>> +and receive virtqueues, respectively.
>>>>> +
>>>>> +If the VIRTIO_NET_F_VQ_NOTF_COAL feature is negotiated:
>>>>> +\begin{itemize}
>>>>> +\item a driver can send a VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET command to set coalescing parameters of a given
>>>>> + enabled transmit/receive virtqueue.
>>>>> +\item a driver can send a VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET command to a device, and the device responds with
>>>>> + coalescing parameters of a given enabled transmit/receive virtqueue.
>>>>> +\end{itemize}
>>>>>
>>>> The VQ_NOTF_COAL commands are enclosed in an itemize and the NOTF_COAL
>>>> commands aren't.
>>>> I think that we should be consistent here.
>>> The VQ commands are enclosed in an itemize because it has both GET and
>>> SET operations.
>>>
>>> I don't think VIRTIO_NET_CTRL_NOTF_COAL_RX_SET and
>>> VIRTIO_NET_CTRL_NOTF_COAL_TX_SET
>>> have to do this, because they are both settings, just in different
>>> directions, and we have described them like this elsewhere.
>>>
>>>> I'm not sure that describing the commands in here is necessary, maybe
>>>> just mentioning which commands can be used with which feature bit is
>>> Isn't that what this sentence does?
>>>
>> Yes, but it also describes what the command does
>> "to dynamically change the coalescing parameters for all transmit and
>> receive virtqueues"
>>
>> Seems a bit repetitive to me.
>>
>>>> enough (this is what I meant in v8, sorry if I wasn't clear).
>>>>
>>>> I'm not saying that describing the commands in here is not good, but
>>>> notice that the commands are described in 3 different places.
>>> Three places describe different effects.
>>>
>>> #1 describes which commands are available for which feature.
>>> #2 describes which command can use which structure.
>>> #3 describes which parameters can be set for each command, and whether
>>> they can affect "global values".
>>> It is placed in the "Operation" section because it explains how the
>>> device should behave.
>>>
>> You're right, but some parts seems repetitive to me:
>> #1: "commands to dynamically change the coalescing parameters for all
>> transmit and receive virtqueues"
>>
>> #2: "commands use the virtio_net_ctrl_coal structure to set
>> \field{max_usecs} and \field{max_packets} for all transmit/receive
>> virtqueues."
>>
>> #3: " set the \field{max_usecs} and \field{max_packets} parameters for
>> all transmit virtqueues, and values of..."
>>
>> Feels like every time we re-explain the commands with more detail.
>> So, for example we read #2 and understand what the command does (set
>> usecs and packets), then we read #3 and understand that it does some
>> more stuff.
>>
>> IMO we need to explain it once, with all the details.
>
> This is how we've written it historically. The idea is that
> reader can get everything on the first pass: 1st high level
> picture then detailed description then the formalities.
>
> Other specs do it differently so one has to re-read them
> many times to understand. I don't like this personally and
> I much prefer the virtio way.
>
>
>>> Maybe #1 and #2 descriptions can be combined and described together.
>>>
>>>> This is #1.
>>>>
>>>>> \begin{note}
>>>>> The behavior of the device in response to these commands is best-effort:
>>>>> @@ -1519,25 +1531,76 @@ \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}
>>>>>
>>>>> +The VIRTIO_NET_CTRL_NOTF_COAL_TX_SET and VIRTIO_NET_CTRL_NOTF_COAL_RX_SET commands use the
>>>>> +virtio_net_ctrl_coal structure to set \field{max_usecs} and \field{max_packets} for all
>>>>> +transmit/receive virtqueues.
>>>>> +
>>>>> +The VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET command uses the virtio_net_ctrl_coal_vq structure
>>>>> +to set \field{max_usecs} and \field{max_packets} for the supplied virtqueue number \field{vqn}.
>>>>> +
>>>>> +The VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET command gets the values of \field{max_usecs} and
>>>>> +\field{max_packets} of the specified virtqueue from the device by setting \field{vqn}
>>>>> +in the virtio_net_ctrl_coal_vq structure.
>>>>> +
>>>> This is #2.
>>>>
>>>>> +# Read/Write attributes for coalescing parameters
>>>>> +\begin{itemize}
>>>>> +\item For commands VIRTIO_NET_CTRL_NOTF_COAL_TX_SET and VIRTIO_NET_CTRL_NOTF_COAL_RX_SET, \field{max_usecs}
>>>>> + and \field{max_packets} are write-only for a driver.
>>>>> +\item For the command VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET, \field{vqn}, \field{reserved}, \field{max_usecs}
>>>>> + and \field{max_packets} are write-only for a driver.
>>>>> +\item For the command VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET, \field{vqn} and \field{reserved} are write-only
>>>>> + for a driver, and, \field{max_usecs} and \field{max_packets} are read-only for the driver.
>>>>> +\end{itemize}
>>>>> +
>>>>> 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.
>>>>> +
>>>>> +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: set the \field{max_usecs} and \field{max_packets} parameters for all transmit virtqueues, and values of
>>>>> + coalescing parameters are recorded as TX global values by a device.
>>>>> +\item VIRTIO_NET_CTRL_NOTF_COAL_RX_SET: set the \field{max_usecs} and \field{max_packets} parameters for all receive virtqueues, and values of
>>>>> + coalescing parameters are recorded as RX global values by a device.
>>>>> +\item VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET: set the \field{max_usecs} and \field{max_packets} parameters for an enabled transmit/receive
>>>>> + virtqueue whose number is \field{vqn}, and do not change the TX/RX global values.
>>>>> +\item VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET: the device returns the \field{max_usecs} and \field{max_packets} parameters for an enabled
>>>>> + transmit/receive virtqueue whose number is \field{vqn}.
>>>>> \end{enumerate}
>>>> This is #3.
>>>>
>>>>> +If coalescing parameters are being set, the device applies the last coalescing parameters received for a
>>>>> +virtqueue, regardless of the command used to set the parameters. For example with 2 pairs of virtqueues:
>>>>> +# Command sequence
>>>>> +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}
>>>>> +
>>>>> \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}.
>>>>> @@ -1549,6 +1612,11 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
>>>>>
>>>>> When the device has \field{max_usecs} = 0 or \field{max_packets} = 0, the notification conditions are met after every packet received/sent.
>>>>>
>>>>> +When the device receives the VIRTIO_NET_CTRL_NOTF_COAL_TX_SET and VIRTIO_NET_CTRL_NOTF_COAL_RX_SET commands,
>>>>> +it saves the values of coalescing parameters as global values, and the VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET command
>>>>> +does not change the global values. If the device is reset, the global values will be set to 0.
>>>>> +When a virtqueue is enabled after virtqueue reset, its coalescing parameters are set to global values.
>>>>> +
>>>> Maybe this explanation should be put closer to the commands
>>>> descriptions, where the global coalescing parameters are mentioned for
>>>> the first time.
>>>> Something like:
>>>> ....
>>>> \item VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET: the device returns the
>>>> \field{max_usecs} and \field{max_packets} parameters for an enabled
>>>> transmit/receive virtqueue whose number
>>>> is \field{vqn}.
>>>>
>>>> The device maintains global coalescing parameters for TX and RX....
>>> This is the "Operation" chapter, and the description of the operation is
>>> more likely to be placed here, isn't it?
>>>
>> I see your point, but a reader will see the "global notifications
>> coalescing parameter" concept, and won't know what it is until next
>> paragraph.
>> Maybe we can have a new list with the notification coalescing concepts
>> (like the notification coalescing parameters)? Just throwing an idea
>> here.
> Actually, "global notification coalescing parameters" is confusing:
> are these global notifications? global coalescings? global parameters?
>
> The problem is the global qualifier. And it's not even global -
> there are two sets for rx and for tx and does not apply to cvq at all.
> How about "RX/TX coalescing parameters"?
As Parav suggested, we probably don't need to mention "global
values/coalescing parameters" since this values/parameters are the ones
set by CTRL_NOTF_COAL_RX/TX_SET.
Just like this:
"
After disabling and re-enabling a virtuqueue, the device MUST revert to
coalescing parameters to the one configured using VIRTIO_NET_CTRL_NOTF_COAL.
"
Thanks.
>
>
>>>> And maybe we should use "global coalescing parameters" instead of
>>>> "global values" (in devicenormative as well).
>>>>
>>>>> \subparagraph{RX Example}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Notifications Coalescing / RX Example}
>>>>>
>>>>> If, for example:
>>>>> @@ -1585,15 +1653,26 @@ \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.
>>>>> +If neither the VIRTIO_NET_F_NOTF_COAL nor the VIRTIO_NET_F_VQ_NOTF_COAL feature
>>>>> +has been negotiated, the driver MUST NOT issue VIRTIO_NET_CTRL_NOTF_COAL commands.
>>>> Maybe this part can be splitted to:
>>>> if the F1 feature has not been negotiated, the driver must not issue
>>>> commands C1,C2.
>>>> if the F2 feature has not been negotiated, the driver must not issue
>>>> commands C3,C4.
>>> Ok.
>>>
>>> Thanks.
>>>
>>>>> +
>>>>> +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 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.
>>>>> +
>>>>> +A device MUST ignore \field{reserved}.
>>>>>
>>>>> A device SHOULD NOT send used buffer notifications to the driver if the notifications are suppressed, even if the notification conditions are met.
>>>>>
>>>>> -Upon reset, a device MUST initialize all coalescing parameters to 0.
>>>>> +After disabling and re-enabling a virtqueue, the device MUST revert coalescing parameters of the virtqueue to the global values.
>>>>> +
>>>>> +Upon reset, a device MUST initialize all coalescing parameters to 0 and MUST set the global values to 0.
>>>>>
>>>>> \subsubsection{Legacy Interface: Framing Requirements}\label{sec:Device
>>>>> Types / Network Device / Legacy Interface: Framing Requirements}
>>>>> --
>>>> Let's wait for more comments before the next version, I guess some may
>>>> not agree with my comments.
---------------------------------------------------------------------
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] 25+ messages in thread
* [virtio-comment] Re: [PATCH v9] virtio-net: support the virtqueue coalescing moderation
2023-02-28 1:40 ` [virtio-comment] " Parav Pandit
2023-02-28 1:40 ` [virtio-dev] " Parav Pandit
2023-02-28 2:19 ` [virtio-comment] " Heng Qi
@ 2023-02-28 7:47 ` Michael S. Tsirkin
2023-02-28 7:47 ` [virtio-dev] " Michael S. Tsirkin
2 siblings, 1 reply; 25+ messages in thread
From: Michael S. Tsirkin @ 2023-02-28 7:47 UTC (permalink / raw)
To: Parav Pandit
Cc: Heng Qi, virtio-dev@lists.oasis-open.org,
virtio-comment@lists.oasis-open.org, Alvaro Karsz,
David Edmondson, Jason Wang, Xuan Zhuo
On Tue, Feb 28, 2023 at 01:40:29AM +0000, Parav Pandit wrote:
>
> > From: Heng Qi <hengqi@linux.alibaba.com>
> > Sent: Sunday, February 26, 2023 4:37 AM
>
> > +# Read/Write attributes for coalescing parameters \begin{itemize} \item
> Hash # above is not needed. Please remove.
>
> > +For commands VIRTIO_NET_CTRL_NOTF_COAL_TX_SET and
> > VIRTIO_NET_CTRL_NOTF_COAL_RX_SET, \field{max_usecs}
> > + and \field{max_packets} are write-only for a driver.
> > +\item For the command VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET, \field{vqn},
> > \field{reserved}, \field{max_usecs}
> > + and \field{max_packets} are write-only for a driver.
> > +\item For the command VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET, \field{vqn}
> > and \field{reserved} are write-only
> > + for a driver, and, \field{max_usecs} and \field{max_packets} are read-only
> > for the driver.
> > +\end{itemize}
> Instead of describing each individual fields of the structure struct virtio_net_ctrl_coal coal as read-only or write-only, better to say the struct itself as read/write only.
I don't see how you can reasonably do this. Which fields are RO and
which WO depends on the command.
> This way when/if in the future, if struct virtio_net_ctrl_coal coal extends, no need to rewrite this section of the spec.
Exactly the reverse.
If you specify RO/WO for each field then you don't need to touch
existing description when adding fields.
If you write in one place that all of struct is RO, when you add
WO fields you need to change it.
> > +
> > 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.
> > +
> > +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: set the \field{max_usecs} and
> > \field{max_packets} parameters for all transmit virtqueues, and values of
> > + coalescing parameters are recorded as TX global values
> > by a device.
> "recording TX global values" is not conveying anything more as "all transmit virtqueues" is already mentioned.
> The original text without "recorded as Tx global values" is just fine. Same for the RX below.
I agree.
> > +\item VIRTIO_NET_CTRL_NOTF_COAL_RX_SET: set the \field{max_usecs} and
> > \field{max_packets} parameters for all receive virtqueues, and values of
> > + coalescing parameters are recorded as RX global values
> > by a device.
> > +\item VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET: set the \field{max_usecs} and
> > \field{max_packets} parameters for an enabled transmit/receive
> > + virtqueue whose number is \field{vqn}, and do not
> > change the TX/RX global values.
> > +\item VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET: the device returns the
> > \field{max_usecs} and \field{max_packets} parameters for an enabled
> > + transmit/receive virtqueue whose number is \field{vqn}.
> > \end{enumerate}
> >
> > +If coalescing parameters are being set, the device applies the last
> > +coalescing parameters received for a virtqueue, regardless of the command
> > used to set the parameters. For example with 2 pairs of virtqueues:
> > +# Command sequence
> > +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}
> > +
> > \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}.
> > @@ -1549,6 +1612,11 @@ \subsubsection{Control
> > Virtqueue}\label{sec:Device Types / Network Device / Devi
> >
> > When the device has \field{max_usecs} = 0 or \field{max_packets} = 0, the
> > notification conditions are met after every packet received/sent.
> >
> > +When the device receives the VIRTIO_NET_CTRL_NOTF_COAL_TX_SET and
> > +VIRTIO_NET_CTRL_NOTF_COAL_RX_SET commands, it saves the values of
> > +coalescing parameters as global values, and the
> > VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET command does not change the global
> > values. If the device is reset, the global values will be set to 0.
> > +When a virtqueue is enabled after virtqueue reset, its coalescing parameters
> > are set to global values.
> > +
> > \subparagraph{RX Example}\label{sec:Device Types / Network Device / Device
> > Operation / Control Virtqueue / Notifications Coalescing / RX Example}
> >
> > If, for example:
> > @@ -1585,15 +1653,26 @@ \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.
> > +If neither the VIRTIO_NET_F_NOTF_COAL nor the
> > VIRTIO_NET_F_VQ_NOTF_COAL
> > +feature has been negotiated, the driver MUST NOT issue
> > VIRTIO_NET_CTRL_NOTF_COAL commands.
> > +
> I agree to Alvaro's suggestion.
> If VIRTIO_NET_F_NOTF_COAL is not negotiated, the driver must not issue commands A and B.
>
> If VIRTIO_NET_F_VQ_NOTF_COAL is not negotiated, the driver must not issue commands C and D.
Yea. Though double negatives are annoying. One way to avoid them is
The driver MUST negotiate VIRTIO_NET_F_NOTF_COAL before issuing commands A and B.
or
The driver is REQUIRED to negotiate VIRTIO_NET_F_NOTF_COAL before issuing commands A and B.
> > +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 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.
> > +
> > +A device MUST ignore \field{reserved}.
> >
> > A device SHOULD NOT send used buffer notifications to the driver if the
> > notifications are suppressed, even if the notification conditions are met.
> >
> > -Upon reset, a device MUST initialize all coalescing parameters to 0.
> > +After disabling and re-enabling a virtqueue, the device MUST revert coalescing
> revert to
> > parameters of the virtqueue to the global values.
> > +
> Like others suggested, above can be reworded as,
>
> After disabling and re-enabling a virtuqueue, the device MUST revert to coalescing parameters to the one configured using VIRTIO_NET_CTRL_NOTF_COAL.
meaning RX/TX I guess. be explicit.
... or 0 if RX/TX was not set?
> > +Upon reset, a device MUST initialize all coalescing parameters to 0 and MUST
> > set the global values to 0.
> >
> No need to mention global values here. "all coalescing parameters to 0" covers per vq and per group of tx and rx vqs.
Let's wait for new version of Alvaro's patch, rebase on top of that.
> > \subsubsection{Legacy Interface: Framing Requirements}\label{sec:Device
> > Types / Network Device / Legacy Interface: Framing Requirements}
> > --
> > 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/
^ permalink raw reply [flat|nested] 25+ messages in thread
* [virtio-dev] Re: [PATCH v9] virtio-net: support the virtqueue coalescing moderation
2023-02-28 7:47 ` [virtio-comment] " Michael S. Tsirkin
@ 2023-02-28 7:47 ` Michael S. Tsirkin
0 siblings, 0 replies; 25+ messages in thread
From: Michael S. Tsirkin @ 2023-02-28 7:47 UTC (permalink / raw)
To: Parav Pandit
Cc: Heng Qi, virtio-dev@lists.oasis-open.org,
virtio-comment@lists.oasis-open.org, Alvaro Karsz,
David Edmondson, Jason Wang, Xuan Zhuo
On Tue, Feb 28, 2023 at 01:40:29AM +0000, Parav Pandit wrote:
>
> > From: Heng Qi <hengqi@linux.alibaba.com>
> > Sent: Sunday, February 26, 2023 4:37 AM
>
> > +# Read/Write attributes for coalescing parameters \begin{itemize} \item
> Hash # above is not needed. Please remove.
>
> > +For commands VIRTIO_NET_CTRL_NOTF_COAL_TX_SET and
> > VIRTIO_NET_CTRL_NOTF_COAL_RX_SET, \field{max_usecs}
> > + and \field{max_packets} are write-only for a driver.
> > +\item For the command VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET, \field{vqn},
> > \field{reserved}, \field{max_usecs}
> > + and \field{max_packets} are write-only for a driver.
> > +\item For the command VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET, \field{vqn}
> > and \field{reserved} are write-only
> > + for a driver, and, \field{max_usecs} and \field{max_packets} are read-only
> > for the driver.
> > +\end{itemize}
> Instead of describing each individual fields of the structure struct virtio_net_ctrl_coal coal as read-only or write-only, better to say the struct itself as read/write only.
I don't see how you can reasonably do this. Which fields are RO and
which WO depends on the command.
> This way when/if in the future, if struct virtio_net_ctrl_coal coal extends, no need to rewrite this section of the spec.
Exactly the reverse.
If you specify RO/WO for each field then you don't need to touch
existing description when adding fields.
If you write in one place that all of struct is RO, when you add
WO fields you need to change it.
> > +
> > 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.
> > +
> > +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: set the \field{max_usecs} and
> > \field{max_packets} parameters for all transmit virtqueues, and values of
> > + coalescing parameters are recorded as TX global values
> > by a device.
> "recording TX global values" is not conveying anything more as "all transmit virtqueues" is already mentioned.
> The original text without "recorded as Tx global values" is just fine. Same for the RX below.
I agree.
> > +\item VIRTIO_NET_CTRL_NOTF_COAL_RX_SET: set the \field{max_usecs} and
> > \field{max_packets} parameters for all receive virtqueues, and values of
> > + coalescing parameters are recorded as RX global values
> > by a device.
> > +\item VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET: set the \field{max_usecs} and
> > \field{max_packets} parameters for an enabled transmit/receive
> > + virtqueue whose number is \field{vqn}, and do not
> > change the TX/RX global values.
> > +\item VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET: the device returns the
> > \field{max_usecs} and \field{max_packets} parameters for an enabled
> > + transmit/receive virtqueue whose number is \field{vqn}.
> > \end{enumerate}
> >
> > +If coalescing parameters are being set, the device applies the last
> > +coalescing parameters received for a virtqueue, regardless of the command
> > used to set the parameters. For example with 2 pairs of virtqueues:
> > +# Command sequence
> > +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}
> > +
> > \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}.
> > @@ -1549,6 +1612,11 @@ \subsubsection{Control
> > Virtqueue}\label{sec:Device Types / Network Device / Devi
> >
> > When the device has \field{max_usecs} = 0 or \field{max_packets} = 0, the
> > notification conditions are met after every packet received/sent.
> >
> > +When the device receives the VIRTIO_NET_CTRL_NOTF_COAL_TX_SET and
> > +VIRTIO_NET_CTRL_NOTF_COAL_RX_SET commands, it saves the values of
> > +coalescing parameters as global values, and the
> > VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET command does not change the global
> > values. If the device is reset, the global values will be set to 0.
> > +When a virtqueue is enabled after virtqueue reset, its coalescing parameters
> > are set to global values.
> > +
> > \subparagraph{RX Example}\label{sec:Device Types / Network Device / Device
> > Operation / Control Virtqueue / Notifications Coalescing / RX Example}
> >
> > If, for example:
> > @@ -1585,15 +1653,26 @@ \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.
> > +If neither the VIRTIO_NET_F_NOTF_COAL nor the
> > VIRTIO_NET_F_VQ_NOTF_COAL
> > +feature has been negotiated, the driver MUST NOT issue
> > VIRTIO_NET_CTRL_NOTF_COAL commands.
> > +
> I agree to Alvaro's suggestion.
> If VIRTIO_NET_F_NOTF_COAL is not negotiated, the driver must not issue commands A and B.
>
> If VIRTIO_NET_F_VQ_NOTF_COAL is not negotiated, the driver must not issue commands C and D.
Yea. Though double negatives are annoying. One way to avoid them is
The driver MUST negotiate VIRTIO_NET_F_NOTF_COAL before issuing commands A and B.
or
The driver is REQUIRED to negotiate VIRTIO_NET_F_NOTF_COAL before issuing commands A and B.
> > +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 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.
> > +
> > +A device MUST ignore \field{reserved}.
> >
> > A device SHOULD NOT send used buffer notifications to the driver if the
> > notifications are suppressed, even if the notification conditions are met.
> >
> > -Upon reset, a device MUST initialize all coalescing parameters to 0.
> > +After disabling and re-enabling a virtqueue, the device MUST revert coalescing
> revert to
> > parameters of the virtqueue to the global values.
> > +
> Like others suggested, above can be reworded as,
>
> After disabling and re-enabling a virtuqueue, the device MUST revert to coalescing parameters to the one configured using VIRTIO_NET_CTRL_NOTF_COAL.
meaning RX/TX I guess. be explicit.
... or 0 if RX/TX was not set?
> > +Upon reset, a device MUST initialize all coalescing parameters to 0 and MUST
> > set the global values to 0.
> >
> No need to mention global values here. "all coalescing parameters to 0" covers per vq and per group of tx and rx vqs.
Let's wait for new version of Alvaro's patch, rebase on top of that.
> > \subsubsection{Legacy Interface: Framing Requirements}\label{sec:Device
> > Types / Network Device / Legacy Interface: Framing Requirements}
> > --
> > 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 [flat|nested] 25+ messages in thread
* [virtio-comment] Re: [virtio-dev] Re: [PATCH v9] virtio-net: support the virtqueue coalescing moderation
2023-02-28 2:41 ` [virtio-comment] " Heng Qi
2023-02-28 2:41 ` Heng Qi
@ 2023-02-28 7:49 ` Michael S. Tsirkin
2023-02-28 7:49 ` Michael S. Tsirkin
2023-02-28 8:10 ` [virtio-comment] " Heng Qi
1 sibling, 2 replies; 25+ messages in thread
From: Michael S. Tsirkin @ 2023-02-28 7:49 UTC (permalink / raw)
To: Heng Qi
Cc: Alvaro Karsz, virtio-dev, virtio-comment, Parav Pandit,
David Edmondson, Jason Wang, Xuan Zhuo
On Tue, Feb 28, 2023 at 10:41:18AM +0800, Heng Qi wrote:
> > The problem is the global qualifier. And it's not even global -
> > there are two sets for rx and for tx and does not apply to cvq at all.
> > How about "RX/TX coalescing parameters"?
>
> As Parav suggested, we probably don't need to mention "global
> values/coalescing parameters" since this values/parameters are the ones set
> by CTRL_NOTF_COAL_RX/TX_SET.
>
> Just like this:
> "
> After disabling and re-enabling a virtuqueue, the device MUST revert to
> coalescing parameters to the one configured using VIRTIO_NET_CTRL_NOTF_COAL.
> "
>
> Thanks.
VIRTIO_NET_CTRL_NOTF_COAL is not a thing. Let's wait for Alvaro's new
patch to be merged though, see what teminology he comes up with and
reuse.
--
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/
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [virtio-dev] Re: [PATCH v9] virtio-net: support the virtqueue coalescing moderation
2023-02-28 7:49 ` [virtio-comment] " Michael S. Tsirkin
@ 2023-02-28 7:49 ` Michael S. Tsirkin
2023-02-28 8:10 ` [virtio-comment] " Heng Qi
1 sibling, 0 replies; 25+ messages in thread
From: Michael S. Tsirkin @ 2023-02-28 7:49 UTC (permalink / raw)
To: Heng Qi
Cc: Alvaro Karsz, virtio-dev, virtio-comment, Parav Pandit,
David Edmondson, Jason Wang, Xuan Zhuo
On Tue, Feb 28, 2023 at 10:41:18AM +0800, Heng Qi wrote:
> > The problem is the global qualifier. And it's not even global -
> > there are two sets for rx and for tx and does not apply to cvq at all.
> > How about "RX/TX coalescing parameters"?
>
> As Parav suggested, we probably don't need to mention "global
> values/coalescing parameters" since this values/parameters are the ones set
> by CTRL_NOTF_COAL_RX/TX_SET.
>
> Just like this:
> "
> After disabling and re-enabling a virtuqueue, the device MUST revert to
> coalescing parameters to the one configured using VIRTIO_NET_CTRL_NOTF_COAL.
> "
>
> Thanks.
VIRTIO_NET_CTRL_NOTF_COAL is not a thing. Let's wait for Alvaro's new
patch to be merged though, see what teminology he comes up with and
reuse.
--
MST
---------------------------------------------------------------------
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] 25+ messages in thread
* Re: [virtio-comment] Re: [virtio-dev] Re: [PATCH v9] virtio-net: support the virtqueue coalescing moderation
2023-02-28 7:49 ` [virtio-comment] " Michael S. Tsirkin
2023-02-28 7:49 ` Michael S. Tsirkin
@ 2023-02-28 8:10 ` Heng Qi
2023-02-28 8:10 ` [virtio-dev] " Heng Qi
1 sibling, 1 reply; 25+ messages in thread
From: Heng Qi @ 2023-02-28 8:10 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Alvaro Karsz, virtio-dev, virtio-comment, Parav Pandit,
David Edmondson, Jason Wang, Xuan Zhuo
在 2023/2/28 下午3:49, Michael S. Tsirkin 写道:
> On Tue, Feb 28, 2023 at 10:41:18AM +0800, Heng Qi wrote:
>>> The problem is the global qualifier. And it's not even global -
>>> there are two sets for rx and for tx and does not apply to cvq at all.
>>> How about "RX/TX coalescing parameters"?
>> As Parav suggested, we probably don't need to mention "global
>> values/coalescing parameters" since this values/parameters are the ones set
>> by CTRL_NOTF_COAL_RX/TX_SET.
>>
>> Just like this:
>> "
>> After disabling and re-enabling a virtuqueue, the device MUST revert to
>> coalescing parameters to the one configured using VIRTIO_NET_CTRL_NOTF_COAL.
>> "
>>
>> Thanks.
> VIRTIO_NET_CTRL_NOTF_COAL is not a thing. Let's wait for Alvaro's new
> patch to be merged though, see what teminology he comes up with and
> reuse.
Ok, are you referring to Alvaro's "virtio-net: Fix and update
VIRTIO_NET_F_NOTF_COAL feature",
if so I see that his v7 version is stable and has been voted in
https://github.com/oasis-tcs/ virtio-spec/issues/159 ,
this patch was made on top of his v7 version.
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/
^ permalink raw reply [flat|nested] 25+ messages in thread
* [virtio-dev] Re: [virtio-comment] Re: [virtio-dev] Re: [PATCH v9] virtio-net: support the virtqueue coalescing moderation
2023-02-28 8:10 ` [virtio-comment] " Heng Qi
@ 2023-02-28 8:10 ` Heng Qi
0 siblings, 0 replies; 25+ messages in thread
From: Heng Qi @ 2023-02-28 8:10 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Alvaro Karsz, virtio-dev, virtio-comment, Parav Pandit,
David Edmondson, Jason Wang, Xuan Zhuo
在 2023/2/28 下午3:49, Michael S. Tsirkin 写道:
> On Tue, Feb 28, 2023 at 10:41:18AM +0800, Heng Qi wrote:
>>> The problem is the global qualifier. And it's not even global -
>>> there are two sets for rx and for tx and does not apply to cvq at all.
>>> How about "RX/TX coalescing parameters"?
>> As Parav suggested, we probably don't need to mention "global
>> values/coalescing parameters" since this values/parameters are the ones set
>> by CTRL_NOTF_COAL_RX/TX_SET.
>>
>> Just like this:
>> "
>> After disabling and re-enabling a virtuqueue, the device MUST revert to
>> coalescing parameters to the one configured using VIRTIO_NET_CTRL_NOTF_COAL.
>> "
>>
>> Thanks.
> VIRTIO_NET_CTRL_NOTF_COAL is not a thing. Let's wait for Alvaro's new
> patch to be merged though, see what teminology he comes up with and
> reuse.
Ok, are you referring to Alvaro's "virtio-net: Fix and update
VIRTIO_NET_F_NOTF_COAL feature",
if so I see that his v7 version is stable and has been voted in
https://github.com/oasis-tcs/ virtio-spec/issues/159 ,
this patch was made on top of his v7 version.
Thanks.
---------------------------------------------------------------------
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] 25+ messages in thread
end of thread, other threads:[~2023-02-28 8:10 UTC | newest]
Thread overview: 25+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-02-26 9:37 [virtio-comment] [PATCH v9] virtio-net: support the virtqueue coalescing moderation Heng Qi
2023-02-26 9:37 ` [virtio-dev] " Heng Qi
2023-02-26 19:33 ` [virtio-comment] " Alvaro Karsz
2023-02-26 19:33 ` [virtio-dev] " Alvaro Karsz
2023-02-27 13:02 ` Heng Qi
2023-02-27 16:25 ` [virtio-comment] " Heng Qi
2023-02-27 16:25 ` Heng Qi
2023-02-27 16:27 ` [virtio-comment] " Parav Pandit
2023-02-27 16:27 ` [virtio-dev] " Parav Pandit
2023-02-27 18:45 ` Alvaro Karsz
2023-02-27 20:19 ` Michael S. Tsirkin
2023-02-27 20:41 ` [virtio-comment] " Alvaro Karsz
2023-02-27 20:41 ` Alvaro Karsz
2023-02-28 2:41 ` [virtio-comment] " Heng Qi
2023-02-28 2:41 ` Heng Qi
2023-02-28 7:49 ` [virtio-comment] " Michael S. Tsirkin
2023-02-28 7:49 ` Michael S. Tsirkin
2023-02-28 8:10 ` [virtio-comment] " Heng Qi
2023-02-28 8:10 ` [virtio-dev] " Heng Qi
2023-02-28 1:40 ` [virtio-comment] " Parav Pandit
2023-02-28 1:40 ` [virtio-dev] " Parav Pandit
2023-02-28 2:19 ` [virtio-comment] " Heng Qi
2023-02-28 2:19 ` [virtio-dev] " Heng Qi
2023-02-28 7:47 ` [virtio-comment] " Michael S. Tsirkin
2023-02-28 7:47 ` [virtio-dev] " Michael S. Tsirkin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox