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

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

[Heng Qi]

Currently, the coalescing profile is directly applied to all queues.
This patch supports setting or getting the parameters for a specified queue,
and a typical application of this function is NetDIM.

When the traffic between queues is unbalanced, for example, one queue
is busy and another queue is idle, then it will be very useful to
control coalescing parameters at the queue granularity.

Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
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 structres 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

 content.tex | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 68 insertions(+), 1 deletion(-)

diff --git a/content.tex b/content.tex
index e863709..2c497e1 100644
--- a/content.tex
+++ b/content.tex
@@ -3084,6 +3084,9 @@ \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 the virtqueue
+    notifications coalescing.
+
 \item[VIRTIO_NET_F_NOTF_COAL(53)] Device supports notifications coalescing.
 
 \item[VIRTIO_NET_F_GUEST_USO4 (54)] Driver can receive USOv4 packets.
@@ -3140,6 +3143,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 and VIRTIO_NET_F_NOTF_COAL.
 \end{description}
 
 \subsubsection{Legacy Interface: Feature bits}\label{sec:Device Types / Network Device / Feature bits / Legacy Interface: Feature bits}
@@ -4520,6 +4524,49 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
 \item VIRTIO_NET_CTRL_NOTF_COAL_RX_SET: set the \field{rx_usecs} and \field{rx_max_packets} parameters.
 \end{enumerate}
 
+If additionally VIRTIO_NET_F_VQ_NOTF_COAL is negotiated, the driver can send
+control commands to set or get the coalescing parameters of a specified
+virtqueue (excluding the control virtqueue).
+
+\begin{lstlisting}
+struct virtio_net_ctrl_coal_vq {
+    le32 max_packets;
+    le32 usecs;
+    le16 vqn;
+};
+
+#define VIRTIO_NET_CTRL_NOTF_COAL_VQ 7
+ #define VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET 0
+ #define VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET 1
+\end{lstlisting}
+
+Virtqueue coalescing parameters:
+\begin{itemize}
+\item \field{max_packets}: The maximum number of packets sent/received by the
+    specified virtqueue before a TX/RX notification.
+\item \field{usecs}: The maximum number of TX/RX usecs that the specified
+    virtqueue delays a TX/RX notification.
+\item \field{vqn}: The virtqueue number of the specified virtqueue.
+\end{itemize}
+
+The range of \filed{vqn} is between 0 and 0xFFFF inclusive, $ \lfloor vqn / 2 \rfloor $
+is the index of the corresponding receiveq, and $\lfloor (vqn / 2) + 1 \rfloor $ is
+the corresponding tranmitq.
+
+The VIRTIO_NET_CTRL_NOTF_COAL_RX_SET command is the same as calling VIRTIO_NET_CTRL_NOTF_COAL_VQ
+for virtqueues corresponding to all receiveqs.
+
+The VIRTIO_NET_CTRL_NOTF_COAL_TX_SET command is the same as calling VIRTIO_NET_CTRL_NOTF_COAL_VQ
+for virtqueues corresponding to all transmitqs.
+
+Virtqueue coalescing will be disabled if all parameters are set to 0.
+
+The class VIRTIO_NET_CTRL_NOTF_COAL_VQ has 2 commands:
+\begin{enumerate}
+\item VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET: set the \field{max_packets}, \field{usecs} and \filed{vqn} parameters.
+\item VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET: get the \field{max_packets}, \field{usecs} and \field{vqn} parameters.
+\end{enumerate}
+
 \subparagraph{RX Notifications}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Notifications Coalescing / RX Notifications}
 
 If, for example:
@@ -4535,6 +4582,15 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
 \item If the notifications are not suppressed by the driver, the device will send an used buffer notification, otherwise, the device will not send an used buffer notification as long as the notifications are suppressed.
 \end{itemize}
 
+If, example of setting coalescing parameters for a receive virtqueue:
+\begin{itemize}
+\item \field{max_packets} = 15.
+\item \field{usecs} = 10.
+\item \field{vqn} = 0;
+\end{itemize}
+
+The device will only operate on recieveq1 as VIRTIO_NET_CTRL_NOTF_COAL_RX_SET.
+
 \subparagraph{TX Notifications}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Notifications Coalescing / TX Notifications}
 
 If, for example:
@@ -4550,13 +4606,24 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
 \item If the notifications are not suppressed by the driver, the device will send an used buffer notification, otherwise, the device will not send an used buffer notification as long as the notifications are suppressed.
 \end{itemize}
 
+If, example of setting coalescing parameters for a transmit virtqueue:
+\begin{itemize}
+\item \field{max_packets} = 15.
+\item \field{usecs} = 10.
+\item \field{vqn} = 1;
+\end{itemize}
+
+The device will only operate on transmitq1 as VIRTIO_NET_CTRL_NOTF_COAL_TX_SET.
+
 \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 the VIRTIO_NET_F_VQ_NOTF_COAL feature has not been negotiated, the driver MUST NOT issue VIRTIO_NET_CTRL_NOFT_COAL_VQ commands.
 
 \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 the VIRTIO_NET_CTRL_NOTF_COAL commands with VIRTIO_NET_ERR if it was not able to change the parameters or
+was not able to find a virtqueue using the \field{vqn}.
 
 A device SHOULD NOT send used buffer notifications to the driver, if the notifications are suppressed as explained in \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Used Buffer Notification Suppression}, even if the coalescing counters expired.
 
-- 
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/

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

[Michael S. Tsirkin]

On Fri, Feb 10, 2023 at 03:01:30PM +0800, Heng Qi wrote:
> Currently, the coalescing profile is directly applied to all queues.
> This patch supports setting or getting the parameters for a specified queue,
> and a typical application of this function is NetDIM.
> 
> When the traffic between queues is unbalanced, for example, one queue
> is busy and another queue is idle, then it will be very useful to
> control coalescing parameters at the queue granularity.
> 
> Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
> Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>


I like this generally, thanks! Language needs to be tightened a bit.
> ---
> 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 structres 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
> 
>  content.tex | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 68 insertions(+), 1 deletion(-)
> 
> diff --git a/content.tex b/content.tex
> index e863709..2c497e1 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -3084,6 +3084,9 @@ \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 the virtqueue
> +    notifications coalescing.
> +
>  \item[VIRTIO_NET_F_NOTF_COAL(53)] Device supports notifications coalescing.
>  
>  \item[VIRTIO_NET_F_GUEST_USO4 (54)] Driver can receive USOv4 packets.
> @@ -3140,6 +3143,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 and VIRTIO_NET_F_NOTF_COAL.
>  \end{description}
>  
>  \subsubsection{Legacy Interface: Feature bits}\label{sec:Device Types / Network Device / Feature bits / Legacy Interface: Feature bits}
> @@ -4520,6 +4524,49 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
>  \item VIRTIO_NET_CTRL_NOTF_COAL_RX_SET: set the \field{rx_usecs} and \field{rx_max_packets} parameters.
>  \end{enumerate}
>  
> +If additionally VIRTIO_NET_F_VQ_NOTF_COAL is negotiated, the driver can send
> +control commands to set or get the coalescing parameters of a specified
> +virtqueue (excluding the control virtqueue).
> +
> +\begin{lstlisting}
> +struct virtio_net_ctrl_coal_vq {
> +    le32 max_packets;
> +    le32 usecs;
> +    le16 vqn;

Add le16 reserved here, so keep things aligned naturally.
In fact if you want to support GET you need to re-order things
since write buffers come before read buffers:

 +    le16 vqn;
 +    le16 reserved;

 +    le32 max_packets;
 +    le32 usecs;

see below for more explanation.



> +};
> +
> +#define VIRTIO_NET_CTRL_NOTF_COAL_VQ 7
> + #define VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET 0
> + #define VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET 1
> +\end{lstlisting}
> +
> +Virtqueue coalescing parameters:
> +\begin{itemize}
> +\item \field{max_packets}: The maximum number of packets sent/received by the
> +    specified virtqueue before a TX/RX notification.
> +\item \field{usecs}: The maximum number of TX/RX usecs that the specified
> +    virtqueue delays a TX/RX notification.
> +\item \field{vqn}: The virtqueue number of the specified virtqueue.
> +\end{itemize}
> +
> +The range of \filed{vqn} is between 0 and 0xFFFF inclusive,


No really because last vq is a cvq. Maybe just drop this?

> $ \lfloor vqn / 2 \rfloor $
> +is the index of the corresponding receiveq, and $\lfloor (vqn / 2) + 1 \rfloor $ is
> +the corresponding tranmitq.
> +
> +The VIRTIO_NET_CTRL_NOTF_COAL_RX_SET command is the same as calling VIRTIO_NET_CTRL_NOTF_COAL_VQ

you don't "call" commands. driver sends them, device receives them.
But here you are talking about a command abstracty so I'd just drop 
a verb, or maybe "repeating".
And "same" is inexact in that it's not the same - takes more time - it
just has the same effect, or is equivalent to.


> +for virtqueues corresponding to all receiveqs.

receiveqs is confusing.

Also elsewhere we use the language receiveq1\ldots receiveqn
which seems clearer. Also you can not call VIRTIO_NET_CTRL_NOTF_COAL_VQ
for all vqs - it applies to one vq only. You mean each.
And virtqueues do not correspond to receiveqs - they
are receiveqs. If you like vqn corresponds to them.
Or better just "same as VIRTIO_NET_CTRL_NOTF_COAL_VQ repeated for
each of receiveq1\ldots receiveqn"

> +
> +The VIRTIO_NET_CTRL_NOTF_COAL_TX_SET command is the same as calling VIRTIO_NET_CTRL_NOTF_COAL_VQ
> +for virtqueues corresponding to all transmitqs.
> +
> +Virtqueue coalescing will be disabled if all parameters are set to 0.

In fact, either usecs 0 or max_packets disables coalescing, no?

> +
> +The class VIRTIO_NET_CTRL_NOTF_COAL_VQ has 2 commands:
> +\begin{enumerate}
> +\item VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET: set the \field{max_packets}, \field{usecs} and \filed{vqn} parameters.
> +\item VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET: get the \field{max_packets}, \field{usecs} and \field{vqn} parameters.

How does VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET work then? For which vq?
I think you mean vqn is specified and you get back max_packets and
usecs. All this needs to be documented fully for each command.

I would also think it's a good idea to mention that VQ_GET does not have
to return exactly the same parameters - since it's best effort anyway,
it's ok for device to round down and store a smaller value for either
max_packets or usecs, e.g. to save space.

This kind of formalizes the best effort thing we discussed
previously.


Maybe make VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET a separate patch -
in the series,
at this point you did not make any effort to document it,
it needs more work.


> +\end{enumerate}
> +
>  \subparagraph{RX Notifications}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Notifications Coalescing / RX Notifications}
>  
>  If, for example:
> @@ -4535,6 +4582,15 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
>  \item If the notifications are not suppressed by the driver, the device will send an used buffer notification, otherwise, the device will not send an used buffer notification as long as the notifications are suppressed.
>  \end{itemize}
>  
> +If, example of setting coalescing parameters for a receive virtqueue:

"If" without "then" is confusing. I'd just start with "Example".

> +\begin{itemize}
> +\item \field{max_packets} = 15.
> +\item \field{usecs} = 10.
> +\item \field{vqn} = 0;

why . above and ; here? I would just drop punctuation.

> +\end{itemize}
> +
> +The device will only operate on recieveq1 as VIRTIO_NET_CTRL_NOTF_COAL_RX_SET.

This really does not explain anything. Please just document exactly what
it does and does not do.


> +
>  \subparagraph{TX Notifications}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Notifications Coalescing / TX Notifications}
>  
>  If, for example:
> @@ -4550,13 +4606,24 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
>  \item If the notifications are not suppressed by the driver, the device will send an used buffer notification, otherwise, the device will not send an used buffer notification as long as the notifications are suppressed.
>  \end{itemize}
>  
> +If, example of setting coalescing parameters for a transmit virtqueue:
> +\begin{itemize}
> +\item \field{max_packets} = 15.
> +\item \field{usecs} = 10.
> +\item \field{vqn} = 1;
> +\end{itemize}
> +
> +The device will only operate on transmitq1 as VIRTIO_NET_CTRL_NOTF_COAL_TX_SET.
> +

I feel it's the other way around. Document 



>  \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 the VIRTIO_NET_F_VQ_NOTF_COAL feature has not been negotiated, the driver MUST NOT issue VIRTIO_NET_CTRL_NOFT_COAL_VQ commands.
>  

Don't we want to limit driver to legal values of vqn?


>  \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 the VIRTIO_NET_CTRL_NOTF_COAL commands with VIRTIO_NET_ERR if it was not able to change the parameters or
> +was not able to find a virtqueue using the \field{vqn}.

First please create multiple statements not a single long one.
Second vqn only exists for per vq commands so create a statement
just for that.
Also more explicit please. What does this mean? I suspect that vq was not
enabled?
Also, we MUST have vqn < max_virtqueue_pairs.


>  
>  A device SHOULD NOT send used buffer notifications to the driver, if the notifications are suppressed as explained in \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Used Buffer Notification Suppression}, even if the coalescing counters expired.
>  
> -- 
> 2.19.1.6.gb485710b


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

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

[Alvaro Karsz]

Thanks Heng.

> Currently, the coalescing profile is directly applied to all queues.
> This patch supports setting or getting the parameters for a specified queue,
> and a typical application of this function is NetDIM.
>
> When the traffic between queues is unbalanced, for example, one queue
> is busy and another queue is idle, then it will be very useful to
> control coalescing parameters at the queue granularity.
>
> Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
> Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> ---
> 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 structres 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
>
>  content.tex | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 68 insertions(+), 1 deletion(-)
>
> diff --git a/content.tex b/content.tex
> index e863709..2c497e1 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -3084,6 +3084,9 @@ \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 the virtqueue
> +    notifications coalescing.
> +
>  \item[VIRTIO_NET_F_NOTF_COAL(53)] Device supports notifications coalescing.
>
>  \item[VIRTIO_NET_F_GUEST_USO4 (54)] Driver can receive USOv4 packets.
> @@ -3140,6 +3143,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 and VIRTIO_NET_F_NOTF_COAL.
>  \end{description}
>

Do we need VIRTIO_NET_F_CTRL_VQ in this case?
VIRTIO_NET_F_VQ_NOTF_COAL requires VIRTIO_NET_F_NOTF_COAL, which
requires VIRTIO_NET_F_CTRL_VQ, so it's implied.

We have a similar example with VIRTIO_NET_F_HOST_ECN, which requires
VIRTIO_NET_F_HOST_TSO4 or VIRTIO_NET_F_HOST_TSO6.
VIRTIO_NET_F_CSUM is not mentioned here, although required.

So, should we remove VIRTIO_NET_F_CTRL_VQ here, or fix VIRTIO_NET_F_HOST_ECN?

>  \subsubsection{Legacy Interface: Feature bits}\label{sec:Device Types / Network Device / Feature bits / Legacy Interface: Feature bits}
> @@ -4520,6 +4524,49 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
>  \item VIRTIO_NET_CTRL_NOTF_COAL_RX_SET: set the \field{rx_usecs} and \field{rx_max_packets} parameters.
>  \end{enumerate}
>
> +If additionally VIRTIO_NET_F_VQ_NOTF_COAL is negotiated, the driver can send
> +control commands to set or get the coalescing parameters of a specified
> +virtqueue (excluding the control virtqueue).
> +
> +\begin{lstlisting}
> +struct virtio_net_ctrl_coal_vq {
> +    le32 max_packets;
> +    le32 usecs;
> +    le16 vqn;
> +};
> +
> +#define VIRTIO_NET_CTRL_NOTF_COAL_VQ 7
> + #define VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET 0
> + #define VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET 1
> +\end{lstlisting}
> +

We already have a notifications coalescing class, why not group all
coalescing commands under a single class?


> +Virtqueue coalescing parameters:
> +\begin{itemize}
> +\item \field{max_packets}: The maximum number of packets sent/received by the
> +    specified virtqueue before a TX/RX notification.
> +\item \field{usecs}: The maximum number of TX/RX usecs that the specified
> +    virtqueue delays a TX/RX notification.
> +\item \field{vqn}: The virtqueue number of the specified virtqueue.
> +\end{itemize}
> +
> +The range of \filed{vqn} is between 0 and 0xFFFF inclusive, $ \lfloor vqn / 2 \rfloor $
> +is the index of the corresponding receiveq, and $\lfloor (vqn / 2) + 1 \rfloor $ is
> +the corresponding tranmitq.
> +
> +The VIRTIO_NET_CTRL_NOTF_COAL_RX_SET command is the same as calling VIRTIO_NET_CTRL_NOTF_COAL_VQ
> +for virtqueues corresponding to all receiveqs.
> +
> +The VIRTIO_NET_CTRL_NOTF_COAL_TX_SET command is the same as calling VIRTIO_NET_CTRL_NOTF_COAL_VQ
> +for virtqueues corresponding to all transmitqs.
> +
> +Virtqueue coalescing will be disabled if all parameters are set to 0.
> +
> +The class VIRTIO_NET_CTRL_NOTF_COAL_VQ has 2 commands:
> +\begin{enumerate}
> +\item VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET: set the \field{max_packets}, \field{usecs} and \filed{vqn} parameters.
> +\item VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET: get the \field{max_packets}, \field{usecs} and \field{vqn} parameters.
> +\end{enumerate}
> +
>  \subparagraph{RX Notifications}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Notifications Coalescing / RX Notifications}
>
>  If, for example:
> @@ -4535,6 +4582,15 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
>  \item If the notifications are not suppressed by the driver, the device will send an used buffer notification, otherwise, the device will not send an used buffer notification as long as the notifications are suppressed.
>  \end{itemize}
>
> +If, example of setting coalescing parameters for a receive virtqueue:
> +\begin{itemize}
> +\item \field{max_packets} = 15.
> +\item \field{usecs} = 10.
> +\item \field{vqn} = 0;
> +\end{itemize}
> +
> +The device will only operate on recieveq1 as VIRTIO_NET_CTRL_NOTF_COAL_RX_SET.
> +
>  \subparagraph{TX Notifications}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Notifications Coalescing / TX Notifications}
>
>  If, for example:
> @@ -4550,13 +4606,24 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
>  \item If the notifications are not suppressed by the driver, the device will send an used buffer notification, otherwise, the device will not send an used buffer notification as long as the notifications are suppressed.
>  \end{itemize}
>
> +If, example of setting coalescing parameters for a transmit virtqueue:
> +\begin{itemize}
> +\item \field{max_packets} = 15.
> +\item \field{usecs} = 10.
> +\item \field{vqn} = 1;
> +\end{itemize}
> +
> +The device will only operate on transmitq1 as VIRTIO_NET_CTRL_NOTF_COAL_TX_SET.
> +
>  \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 the VIRTIO_NET_F_VQ_NOTF_COAL feature has not been negotiated, the driver MUST NOT issue VIRTIO_NET_CTRL_NOFT_COAL_VQ commands.
>
>  \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 the VIRTIO_NET_CTRL_NOTF_COAL commands with VIRTIO_NET_ERR if it was not able to change the parameters or
> +was not able to find a virtqueue using the \field{vqn}.
>
>  A device SHOULD NOT send used buffer notifications to the driver, if the notifications are suppressed as explained in \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Used Buffer Notification Suppression}, even if the coalescing counters expired.
>
> --
> 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/

[virtio-dev] Re: [PATCH v2] virtio-net: support the virtqueue coalescing moderation

[Alvaro Karsz]

> On Fri, Feb 10, 2023 at 03:01:30PM +0800, Heng Qi wrote:
> > Currently, the coalescing profile is directly applied to all queues.
> > This patch supports setting or getting the parameters for a specified queue,
> > and a typical application of this function is NetDIM.
> >
> > When the traffic between queues is unbalanced, for example, one queue
> > is busy and another queue is idle, then it will be very useful to
> > control coalescing parameters at the queue granularity.
> >
> > Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
> > Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
>
>
> I like this generally, thanks! Language needs to be tightened a bit.
> > ---
> > 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 structres 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
> >
> >  content.tex | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 68 insertions(+), 1 deletion(-)
> >
> > diff --git a/content.tex b/content.tex
> > index e863709..2c497e1 100644
> > --- a/content.tex
> > +++ b/content.tex
> > @@ -3084,6 +3084,9 @@ \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 the virtqueue
> > +    notifications coalescing.
> > +
> >  \item[VIRTIO_NET_F_NOTF_COAL(53)] Device supports notifications coalescing.
> >
> >  \item[VIRTIO_NET_F_GUEST_USO4 (54)] Driver can receive USOv4 packets.
> > @@ -3140,6 +3143,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 and VIRTIO_NET_F_NOTF_COAL.
> >  \end{description}
> >
> >  \subsubsection{Legacy Interface: Feature bits}\label{sec:Device Types / Network Device / Feature bits / Legacy Interface: Feature bits}
> > @@ -4520,6 +4524,49 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
> >  \item VIRTIO_NET_CTRL_NOTF_COAL_RX_SET: set the \field{rx_usecs} and \field{rx_max_packets} parameters.
> >  \end{enumerate}
> >
> > +If additionally VIRTIO_NET_F_VQ_NOTF_COAL is negotiated, the driver can send
> > +control commands to set or get the coalescing parameters of a specified
> > +virtqueue (excluding the control virtqueue).
> > +
> > +\begin{lstlisting}
> > +struct virtio_net_ctrl_coal_vq {
> > +    le32 max_packets;
> > +    le32 usecs;
> > +    le16 vqn;
>
> Add le16 reserved here, so keep things aligned naturally.
> In fact if you want to support GET you need to re-order things
> since write buffers come before read buffers:
>
>  +    le16 vqn;
>  +    le16 reserved;
>
>  +    le32 max_packets;
>  +    le32 usecs;
>
> see below for more explanation.
>
>
>
> > +};
> > +
> > +#define VIRTIO_NET_CTRL_NOTF_COAL_VQ 7
> > + #define VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET 0
> > + #define VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET 1
> > +\end{lstlisting}
> > +
> > +Virtqueue coalescing parameters:
> > +\begin{itemize}
> > +\item \field{max_packets}: The maximum number of packets sent/received by the
> > +    specified virtqueue before a TX/RX notification.
> > +\item \field{usecs}: The maximum number of TX/RX usecs that the specified
> > +    virtqueue delays a TX/RX notification.
> > +\item \field{vqn}: The virtqueue number of the specified virtqueue.
> > +\end{itemize}
> > +
> > +The range of \filed{vqn} is between 0 and 0xFFFF inclusive,
>
>
> No really because last vq is a cvq. Maybe just drop this?
>
> > $ \lfloor vqn / 2 \rfloor $
> > +is the index of the corresponding receiveq, and $\lfloor (vqn / 2) + 1 \rfloor $ is
> > +the corresponding tranmitq.
> > +
> > +The VIRTIO_NET_CTRL_NOTF_COAL_RX_SET command is the same as calling VIRTIO_NET_CTRL_NOTF_COAL_VQ
>
> you don't "call" commands. driver sends them, device receives them.
> But here you are talking about a command abstracty so I'd just drop
> a verb, or maybe "repeating".
> And "same" is inexact in that it's not the same - takes more time - it
> just has the same effect, or is equivalent to.
>
>
> > +for virtqueues corresponding to all receiveqs.
>
> receiveqs is confusing.
>
> Also elsewhere we use the language receiveq1\ldots receiveqn
> which seems clearer. Also you can not call VIRTIO_NET_CTRL_NOTF_COAL_VQ
> for all vqs - it applies to one vq only. You mean each.
> And virtqueues do not correspond to receiveqs - they
> are receiveqs. If you like vqn corresponds to them.
> Or better just "same as VIRTIO_NET_CTRL_NOTF_COAL_VQ repeated for
> each of receiveq1\ldots receiveqn"
>
> > +
> > +The VIRTIO_NET_CTRL_NOTF_COAL_TX_SET command is the same as calling VIRTIO_NET_CTRL_NOTF_COAL_VQ
> > +for virtqueues corresponding to all transmitqs.
> > +
> > +Virtqueue coalescing will be disabled if all parameters are set to 0.
>
> In fact, either usecs 0 or max_packets disables coalescing, no?
>
> > +
> > +The class VIRTIO_NET_CTRL_NOTF_COAL_VQ has 2 commands:
> > +\begin{enumerate}
> > +\item VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET: set the \field{max_packets}, \field{usecs} and \filed{vqn} parameters.
> > +\item VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET: get the \field{max_packets}, \field{usecs} and \field{vqn} parameters.
>
> How does VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET work then? For which vq?
> I think you mean vqn is specified and you get back max_packets and
> usecs. All this needs to be documented fully for each command.
>
> I would also think it's a good idea to mention that VQ_GET does not have
> to return exactly the same parameters - since it's best effort anyway,
> it's ok for device to round down and store a smaller value for either
> max_packets or usecs, e.g. to save space.
>
> This kind of formalizes the best effort thing we discussed
> previously.
>
>
> Maybe make VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET a separate patch -
> in the series,
> at this point you did not make any effort to document it,
> it needs more work.
>
>
> > +\end{enumerate}
> > +
> >  \subparagraph{RX Notifications}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Notifications Coalescing / RX Notifications}
> >
> >  If, for example:
> > @@ -4535,6 +4582,15 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
> >  \item If the notifications are not suppressed by the driver, the device will send an used buffer notification, otherwise, the device will not send an used buffer notification as long as the notifications are suppressed.
> >  \end{itemize}
> >
> > +If, example of setting coalescing parameters for a receive virtqueue:
>
> "If" without "then" is confusing. I'd just start with "Example".
>
> > +\begin{itemize}
> > +\item \field{max_packets} = 15.
> > +\item \field{usecs} = 10.
> > +\item \field{vqn} = 0;
>
> why . above and ; here? I would just drop punctuation.
>
> > +\end{itemize}
> > +
> > +The device will only operate on recieveq1 as VIRTIO_NET_CTRL_NOTF_COAL_RX_SET.
>
> This really does not explain anything. Please just document exactly what
> it does and does not do.
>
>
> > +
> >  \subparagraph{TX Notifications}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Notifications Coalescing / TX Notifications}
> >
> >  If, for example:
> > @@ -4550,13 +4606,24 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
> >  \item If the notifications are not suppressed by the driver, the device will send an used buffer notification, otherwise, the device will not send an used buffer notification as long as the notifications are suppressed.
> >  \end{itemize}
> >
> > +If, example of setting coalescing parameters for a transmit virtqueue:
> > +\begin{itemize}
> > +\item \field{max_packets} = 15.
> > +\item \field{usecs} = 10.
> > +\item \field{vqn} = 1;
> > +\end{itemize}
> > +
> > +The device will only operate on transmitq1 as VIRTIO_NET_CTRL_NOTF_COAL_TX_SET.
> > +
>
> I feel it's the other way around. Document
>
>
>
> >  \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 the VIRTIO_NET_F_VQ_NOTF_COAL feature has not been negotiated, the driver MUST NOT issue VIRTIO_NET_CTRL_NOFT_COAL_VQ commands.
> >
>
> Don't we want to limit driver to legal values of vqn?
>
>
> >  \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 the VIRTIO_NET_CTRL_NOTF_COAL commands with VIRTIO_NET_ERR if it was not able to change the parameters or
> > +was not able to find a virtqueue using the \field{vqn}.
>
> First please create multiple statements not a single long one.
> Second vqn only exists for per vq commands so create a statement
> just for that.
> Also more explicit please. What does this mean? I suspect that vq was not
> enabled?
> Also, we MUST have vqn < max_virtqueue_pairs.
>

I believe that you meant  vqn < max_virtqueue_pairs * 2

>
> >
> >  A device SHOULD NOT send used buffer notifications to the driver, if the notifications are suppressed as explained in \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Used Buffer Notification Suppression}, even if the coalescing counters expired.
> >
> > --
> > 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

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

[Michael S. Tsirkin]

On Fri, Feb 10, 2023 at 11:22:14AM +0200, Alvaro Karsz wrote:
> Thanks Heng.
> 
> > Currently, the coalescing profile is directly applied to all queues.
> > This patch supports setting or getting the parameters for a specified queue,
> > and a typical application of this function is NetDIM.
> >
> > When the traffic between queues is unbalanced, for example, one queue
> > is busy and another queue is idle, then it will be very useful to
> > control coalescing parameters at the queue granularity.
> >
> > Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
> > Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > ---
> > 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 structres 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
> >
> >  content.tex | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 68 insertions(+), 1 deletion(-)
> >
> > diff --git a/content.tex b/content.tex
> > index e863709..2c497e1 100644
> > --- a/content.tex
> > +++ b/content.tex
> > @@ -3084,6 +3084,9 @@ \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 the virtqueue
> > +    notifications coalescing.
> > +
> >  \item[VIRTIO_NET_F_NOTF_COAL(53)] Device supports notifications coalescing.
> >
> >  \item[VIRTIO_NET_F_GUEST_USO4 (54)] Driver can receive USOv4 packets.
> > @@ -3140,6 +3143,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 and VIRTIO_NET_F_NOTF_COAL.
> >  \end{description}
> >
> 
> Do we need VIRTIO_NET_F_CTRL_VQ in this case?
> VIRTIO_NET_F_VQ_NOTF_COAL requires VIRTIO_NET_F_NOTF_COAL, which
> requires VIRTIO_NET_F_CTRL_VQ, so it's implied.
> 
> We have a similar example with VIRTIO_NET_F_HOST_ECN, which requires
> VIRTIO_NET_F_HOST_TSO4 or VIRTIO_NET_F_HOST_TSO6.
> VIRTIO_NET_F_CSUM is not mentioned here, although required.
> 
> So, should we remove VIRTIO_NET_F_CTRL_VQ here, or fix VIRTIO_NET_F_HOST_ECN?

Ah good point.
But I think  VIRTIO_NET_F_VQ_NOTF_COAL should not depend on VIRTIO_NET_F_NOTF_COAL.
This way devices can drop the all-rx/all-tx commands if they want to.


> >  \subsubsection{Legacy Interface: Feature bits}\label{sec:Device Types / Network Device / Feature bits / Legacy Interface: Feature bits}
> > @@ -4520,6 +4524,49 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
> >  \item VIRTIO_NET_CTRL_NOTF_COAL_RX_SET: set the \field{rx_usecs} and \field{rx_max_packets} parameters.
> >  \end{enumerate}
> >
> > +If additionally VIRTIO_NET_F_VQ_NOTF_COAL is negotiated, the driver can send
> > +control commands to set or get the coalescing parameters of a specified
> > +virtqueue (excluding the control virtqueue).
> > +
> > +\begin{lstlisting}
> > +struct virtio_net_ctrl_coal_vq {
> > +    le32 max_packets;
> > +    le32 usecs;
> > +    le16 vqn;
> > +};
> > +
> > +#define VIRTIO_NET_CTRL_NOTF_COAL_VQ 7
> > + #define VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET 0
> > + #define VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET 1
> > +\end{lstlisting}
> > +
> 
> We already have a notifications coalescing class, why not group all
> coalescing commands under a single class?

Agree here.

> 
> > +Virtqueue coalescing parameters:
> > +\begin{itemize}
> > +\item \field{max_packets}: The maximum number of packets sent/received by the
> > +    specified virtqueue before a TX/RX notification.
> > +\item \field{usecs}: The maximum number of TX/RX usecs that the specified
> > +    virtqueue delays a TX/RX notification.
> > +\item \field{vqn}: The virtqueue number of the specified virtqueue.
> > +\end{itemize}
> > +
> > +The range of \filed{vqn} is between 0 and 0xFFFF inclusive, $ \lfloor vqn / 2 \rfloor $
> > +is the index of the corresponding receiveq, and $\lfloor (vqn / 2) + 1 \rfloor $ is
> > +the corresponding tranmitq.
> > +
> > +The VIRTIO_NET_CTRL_NOTF_COAL_RX_SET command is the same as calling VIRTIO_NET_CTRL_NOTF_COAL_VQ
> > +for virtqueues corresponding to all receiveqs.
> > +
> > +The VIRTIO_NET_CTRL_NOTF_COAL_TX_SET command is the same as calling VIRTIO_NET_CTRL_NOTF_COAL_VQ
> > +for virtqueues corresponding to all transmitqs.
> > +
> > +Virtqueue coalescing will be disabled if all parameters are set to 0.
> > +
> > +The class VIRTIO_NET_CTRL_NOTF_COAL_VQ has 2 commands:
> > +\begin{enumerate}
> > +\item VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET: set the \field{max_packets}, \field{usecs} and \filed{vqn} parameters.
> > +\item VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET: get the \field{max_packets}, \field{usecs} and \field{vqn} parameters.
> > +\end{enumerate}
> > +
> >  \subparagraph{RX Notifications}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Notifications Coalescing / RX Notifications}
> >
> >  If, for example:
> > @@ -4535,6 +4582,15 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
> >  \item If the notifications are not suppressed by the driver, the device will send an used buffer notification, otherwise, the device will not send an used buffer notification as long as the notifications are suppressed.
> >  \end{itemize}
> >
> > +If, example of setting coalescing parameters for a receive virtqueue:
> > +\begin{itemize}
> > +\item \field{max_packets} = 15.
> > +\item \field{usecs} = 10.
> > +\item \field{vqn} = 0;
> > +\end{itemize}
> > +
> > +The device will only operate on recieveq1 as VIRTIO_NET_CTRL_NOTF_COAL_RX_SET.
> > +
> >  \subparagraph{TX Notifications}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Notifications Coalescing / TX Notifications}
> >
> >  If, for example:
> > @@ -4550,13 +4606,24 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
> >  \item If the notifications are not suppressed by the driver, the device will send an used buffer notification, otherwise, the device will not send an used buffer notification as long as the notifications are suppressed.
> >  \end{itemize}
> >
> > +If, example of setting coalescing parameters for a transmit virtqueue:
> > +\begin{itemize}
> > +\item \field{max_packets} = 15.
> > +\item \field{usecs} = 10.
> > +\item \field{vqn} = 1;
> > +\end{itemize}
> > +
> > +The device will only operate on transmitq1 as VIRTIO_NET_CTRL_NOTF_COAL_TX_SET.
> > +
> >  \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 the VIRTIO_NET_F_VQ_NOTF_COAL feature has not been negotiated, the driver MUST NOT issue VIRTIO_NET_CTRL_NOFT_COAL_VQ commands.
> >
> >  \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 the VIRTIO_NET_CTRL_NOTF_COAL commands with VIRTIO_NET_ERR if it was not able to change the parameters or
> > +was not able to find a virtqueue using the \field{vqn}.
> >
> >  A device SHOULD NOT send used buffer notifications to the driver, if the notifications are suppressed as explained in \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Used Buffer Notification Suppression}, even if the coalescing counters expired.
> >
> > --
> > 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/

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

[Michael S. Tsirkin]

On Fri, Feb 10, 2023 at 11:30:40AM +0200, Alvaro Karsz wrote:
> > >  \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 the VIRTIO_NET_CTRL_NOTF_COAL commands with VIRTIO_NET_ERR if it was not able to change the parameters or
> > > +was not able to find a virtqueue using the \field{vqn}.
> >
> > First please create multiple statements not a single long one.
> > Second vqn only exists for per vq commands so create a statement
> > just for that.
> > Also more explicit please. What does this mean? I suspect that vq was not
> > enabled?
> > Also, we MUST have vqn < max_virtqueue_pairs.
> >
> 
> I believe that you meant  vqn < max_virtqueue_pairs * 2

Yes thanks!
Just pls snip to just the text you qoute to just relevant parts -
I had to scroll like 300 lines to get to the relevant one :)


-- 
MST


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

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

[Heng Qi]

在 2023/2/10 下午4:38, Michael S. Tsirkin 写道:
> On Fri, Feb 10, 2023 at 03:01:30PM +0800, Heng Qi wrote:
>> Currently, the coalescing profile is directly applied to all queues.
>> This patch supports setting or getting the parameters for a specified queue,
>> and a typical application of this function is NetDIM.
>>
>> When the traffic between queues is unbalanced, for example, one queue
>> is busy and another queue is idle, then it will be very useful to
>> control coalescing parameters at the queue granularity.
>>
>> Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
>> Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
>
> I like this generally, thanks! Language needs to be tightened a bit.
>> ---
>> 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 structres 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
>>
>>   content.tex | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 68 insertions(+), 1 deletion(-)
>>
>> diff --git a/content.tex b/content.tex
>> index e863709..2c497e1 100644
>> --- a/content.tex
>> +++ b/content.tex
>> @@ -3084,6 +3084,9 @@ \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 the virtqueue
>> +    notifications coalescing.
>> +
>>   \item[VIRTIO_NET_F_NOTF_COAL(53)] Device supports notifications coalescing.
>>   
>>   \item[VIRTIO_NET_F_GUEST_USO4 (54)] Driver can receive USOv4 packets.
>> @@ -3140,6 +3143,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 and VIRTIO_NET_F_NOTF_COAL.
>>   \end{description}
>>   
>>   \subsubsection{Legacy Interface: Feature bits}\label{sec:Device Types / Network Device / Feature bits / Legacy Interface: Feature bits}
>> @@ -4520,6 +4524,49 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
>>   \item VIRTIO_NET_CTRL_NOTF_COAL_RX_SET: set the \field{rx_usecs} and \field{rx_max_packets} parameters.
>>   \end{enumerate}
>>   
>> +If additionally VIRTIO_NET_F_VQ_NOTF_COAL is negotiated, the driver can send
>> +control commands to set or get the coalescing parameters of a specified
>> +virtqueue (excluding the control virtqueue).
>> +
>> +\begin{lstlisting}
>> +struct virtio_net_ctrl_coal_vq {
>> +    le32 max_packets;
>> +    le32 usecs;
>> +    le16 vqn;
> Add le16 reserved here, so keep things aligned naturally.
> In fact if you want to support GET you need to re-order things
> since write buffers come before read buffers:

I see, thanks for pointing it out.

>   +    le16 vqn;
>   +    le16 reserved;
>
>   +    le32 max_packets;
>   +    le32 usecs;
>
> see below for more explanation.
>
>
>
>> +};
>> +
>> +#define VIRTIO_NET_CTRL_NOTF_COAL_VQ 7
>> + #define VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET 0
>> + #define VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET 1
>> +\end{lstlisting}
>> +
>> +Virtqueue coalescing parameters:
>> +\begin{itemize}
>> +\item \field{max_packets}: The maximum number of packets sent/received by the
>> +    specified virtqueue before a TX/RX notification.
>> +\item \field{usecs}: The maximum number of TX/RX usecs that the specified
>> +    virtqueue delays a TX/RX notification.
>> +\item \field{vqn}: The virtqueue number of the specified virtqueue.
>> +\end{itemize}
>> +
>> +The range of \filed{vqn} is between 0 and 0xFFFF inclusive,
>
> No really because last vq is a cvq. Maybe just drop this?

In fact I have ruled out the control virtqueue.

Its virtqueue number should be 0x10000.

>
>> $ \lfloor vqn / 2 \rfloor $
>> +is the index of the corresponding receiveq, and $\lfloor (vqn / 2) + 1 \rfloor $ is
>> +the corresponding tranmitq.
>> +
>> +The VIRTIO_NET_CTRL_NOTF_COAL_RX_SET command is the same as calling VIRTIO_NET_CTRL_NOTF_COAL_VQ
> you don't "call" commands. driver sends them, device receives them.
> But here you are talking about a command abstracty so I'd just drop
> a verb, or maybe "repeating".
> And "same" is inexact in that it's not the same - takes more time - it
> just has the same effect, or is equivalent to.

There is indeed a gerund match here, I'll fix that.

>
>
>> +for virtqueues corresponding to all receiveqs.
> receiveqs is confusing.
>
> Also elsewhere we use the language receiveq1\ldots receiveqn
> which seems clearer. Also you can not call VIRTIO_NET_CTRL_NOTF_COAL_VQ
> for all vqs - it applies to one vq only. You mean each.

I express something wrong, but what I mean is to send for each virtqueue.

> And virtqueues do not correspond to receiveqs - they
> are receiveqs. If you like vqn corresponds to them.

This is ambiguous, the number of virtqueues is 2*N+1, the number of 
receive queues and transmit queues is N,
and there is also a control queue. Is this a problem?
But I know what you mean it's better to use "same as 
VIRTIO_NET_CTRL_NOTF_COAL_VQ repeated for each of receiveq1\ldots receiveqn"

> Or better just "same as VIRTIO_NET_CTRL_NOTF_COAL_VQ repeated for
> each of receiveq1\ldots receiveqn"

Sure.

>> +
>> +The VIRTIO_NET_CTRL_NOTF_COAL_TX_SET command is the same as calling VIRTIO_NET_CTRL_NOTF_COAL_VQ
>> +for virtqueues corresponding to all transmitqs.
>> +
>> +Virtqueue coalescing will be disabled if all parameters are set to 0.
> In fact, either usecs 0 or max_packets disables coalescing, no?

Yes. I'll fix this.

>
>> +
>> +The class VIRTIO_NET_CTRL_NOTF_COAL_VQ has 2 commands:
>> +\begin{enumerate}
>> +\item VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET: set the \field{max_packets}, \field{usecs} and \filed{vqn} parameters.
>> +\item VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET: get the \field{max_packets}, \field{usecs} and \field{vqn} parameters.
> How does VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET work then? For which vq?
> I think you mean vqn is specified and you get back max_packets and
> usecs. All this needs to be documented fully for each command.

Yes, VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET gets the parameters of the 
virtqueue corresponding to vqn each time.

>
> I would also think it's a good idea to mention that VQ_GET does not have
> to return exactly the same parameters - since it's best effort anyway,

This is confusing, I think the device does not have to set the same 
parameters for VQ_SET, but for VQ_GET, the device should return to the 
driver every time.

> it's ok for device to round down and store a smaller value for either
> max_packets or usecs, e.g. to save space.
>
> This kind of formalizes the best effort thing we discussed
> previously.
>
>
> Maybe make VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET a separate patch -
> in the series,

No problem, I'll try to describe it as best I can.

> at this point you did not make any effort to document it,
> it needs more work.
>
>
>> +\end{enumerate}
>> +
>>   \subparagraph{RX Notifications}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Notifications Coalescing / RX Notifications}
>>   
>>   If, for example:
>> @@ -4535,6 +4582,15 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
>>   \item If the notifications are not suppressed by the driver, the device will send an used buffer notification, otherwise, the device will not send an used buffer notification as long as the notifications are suppressed.
>>   \end{itemize}
>>   
>> +If, example of setting coalescing parameters for a receive virtqueue:
> "If" without "then" is confusing. I'd just start with "Example".

Ok.

>
>> +\begin{itemize}
>> +\item \field{max_packets} = 15.
>> +\item \field{usecs} = 10.
>> +\item \field{vqn} = 0;
> why . above and ; here? I would just drop punctuation.

It's a typo and I'll fix it.

>
>> +\end{itemize}
>> +
>> +The device will only operate on recieveq1 as VIRTIO_NET_CTRL_NOTF_COAL_RX_SET.
> This really does not explain anything. Please just document exactly what
> it does and does not do.

Ok.

>
>
>> +
>>   \subparagraph{TX Notifications}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Notifications Coalescing / TX Notifications}
>>   
>>   If, for example:
>> @@ -4550,13 +4606,24 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
>>   \item If the notifications are not suppressed by the driver, the device will send an used buffer notification, otherwise, the device will not send an used buffer notification as long as the notifications are suppressed.
>>   \end{itemize}
>>   
>> +If, example of setting coalescing parameters for a transmit virtqueue:
>> +\begin{itemize}
>> +\item \field{max_packets} = 15.
>> +\item \field{usecs} = 10.
>> +\item \field{vqn} = 1;
>> +\end{itemize}
>> +
>> +The device will only operate on transmitq1 as VIRTIO_NET_CTRL_NOTF_COAL_TX_SET.
>> +
> I feel it's the other way around. Document
>

Why? I'll add documentation, but read on below.

>
>>   \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 the VIRTIO_NET_F_VQ_NOTF_COAL feature has not been negotiated, the driver MUST NOT issue VIRTIO_NET_CTRL_NOFT_COAL_VQ commands.
>>   
> Don't we want to limit driver to legal values of vqn?
>

Yes, I will add.

>>   \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 the VIRTIO_NET_CTRL_NOTF_COAL commands with VIRTIO_NET_ERR if it was not able to change the parameters or
>> +was not able to find a virtqueue using the \field{vqn}.
> First please create multiple statements not a single long one.
> Second vqn only exists for per vq commands so create a statement
> just for that.

Ok, reasonable.

> Also more explicit please. What does this mean? I suspect that vq was not
> enabled?

Indicates that a vqn cannot be mapped to the corresponding virtqueue.

> Also, we MUST have vqn < max_virtqueue_pairs.

Here should be vq < max_virtqueue_pairs * 2?

Thanks.

>
>
>>   
>>   A device SHOULD NOT send used buffer notifications to the driver, if the notifications are suppressed as explained in \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Used Buffer Notification Suppression}, even if the coalescing counters expired.
>>   
>> -- 
>> 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/

[virtio-comment] Re: [virtio-dev] Re: [virtio-comment] [PATCH v2] virtio-net: support the virtqueue coalescing moderation

[Heng Qi]

在 2023/2/10 下午5:22, Alvaro Karsz 写道:
> Thanks Heng.
>
>> Currently, the coalescing profile is directly applied to all queues.
>> This patch supports setting or getting the parameters for a specified queue,
>> and a typical application of this function is NetDIM.
>>
>> When the traffic between queues is unbalanced, for example, one queue
>> is busy and another queue is idle, then it will be very useful to
>> control coalescing parameters at the queue granularity.
>>
>> Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
>> Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
>> ---
>> 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 structres 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
>>
>>   content.tex | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 68 insertions(+), 1 deletion(-)
>>
>> diff --git a/content.tex b/content.tex
>> index e863709..2c497e1 100644
>> --- a/content.tex
>> +++ b/content.tex
>> @@ -3084,6 +3084,9 @@ \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 the virtqueue
>> +    notifications coalescing.
>> +
>>   \item[VIRTIO_NET_F_NOTF_COAL(53)] Device supports notifications coalescing.
>>
>>   \item[VIRTIO_NET_F_GUEST_USO4 (54)] Driver can receive USOv4 packets.
>> @@ -3140,6 +3143,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 and VIRTIO_NET_F_NOTF_COAL.
>>   \end{description}
>>
> Do we need VIRTIO_NET_F_CTRL_VQ in this case?
> VIRTIO_NET_F_VQ_NOTF_COAL requires VIRTIO_NET_F_NOTF_COAL, which
> requires VIRTIO_NET_F_CTRL_VQ, so it's implied.
>
> We have a similar example with VIRTIO_NET_F_HOST_ECN, which requires
> VIRTIO_NET_F_HOST_TSO4 or VIRTIO_NET_F_HOST_TSO6.
> VIRTIO_NET_F_CSUM is not mentioned here, although required.
>
> So, should we remove VIRTIO_NET_F_CTRL_VQ here, or fix VIRTIO_NET_F_HOST_ECN?

Thanks for pointing out, I think following the previous for consistency, 
I will remove VIRTIO_NET_F_CTRL_VQ.

>
>>   \subsubsection{Legacy Interface: Feature bits}\label{sec:Device Types / Network Device / Feature bits / Legacy Interface: Feature bits}
>> @@ -4520,6 +4524,49 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
>>   \item VIRTIO_NET_CTRL_NOTF_COAL_RX_SET: set the \field{rx_usecs} and \field{rx_max_packets} parameters.
>>   \end{enumerate}
>>
>> +If additionally VIRTIO_NET_F_VQ_NOTF_COAL is negotiated, the driver can send
>> +control commands to set or get the coalescing parameters of a specified
>> +virtqueue (excluding the control virtqueue).
>> +
>> +\begin{lstlisting}
>> +struct virtio_net_ctrl_coal_vq {
>> +    le32 max_packets;
>> +    le32 usecs;
>> +    le16 vqn;
>> +};
>> +
>> +#define VIRTIO_NET_CTRL_NOTF_COAL_VQ 7
>> + #define VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET 0
>> + #define VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET 1
>> +\end{lstlisting}
>> +
> We already have a notifications coalescing class, why not group all
> coalescing commands under a single class?

Good idea, I overlooked this.

Thanks!


>
>
>> +Virtqueue coalescing parameters:
>> +\begin{itemize}
>> +\item \field{max_packets}: The maximum number of packets sent/received by the
>> +    specified virtqueue before a TX/RX notification.
>> +\item \field{usecs}: The maximum number of TX/RX usecs that the specified
>> +    virtqueue delays a TX/RX notification.
>> +\item \field{vqn}: The virtqueue number of the specified virtqueue.
>> +\end{itemize}
>> +
>> +The range of \filed{vqn} is between 0 and 0xFFFF inclusive, $ \lfloor vqn / 2 \rfloor $
>> +is the index of the corresponding receiveq, and $\lfloor (vqn / 2) + 1 \rfloor $ is
>> +the corresponding tranmitq.
>> +
>> +The VIRTIO_NET_CTRL_NOTF_COAL_RX_SET command is the same as calling VIRTIO_NET_CTRL_NOTF_COAL_VQ
>> +for virtqueues corresponding to all receiveqs.
>> +
>> +The VIRTIO_NET_CTRL_NOTF_COAL_TX_SET command is the same as calling VIRTIO_NET_CTRL_NOTF_COAL_VQ
>> +for virtqueues corresponding to all transmitqs.
>> +
>> +Virtqueue coalescing will be disabled if all parameters are set to 0.
>> +
>> +The class VIRTIO_NET_CTRL_NOTF_COAL_VQ has 2 commands:
>> +\begin{enumerate}
>> +\item VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET: set the \field{max_packets}, \field{usecs} and \filed{vqn} parameters.
>> +\item VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET: get the \field{max_packets}, \field{usecs} and \field{vqn} parameters.
>> +\end{enumerate}
>> +
>>   \subparagraph{RX Notifications}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Notifications Coalescing / RX Notifications}
>>
>>   If, for example:
>> @@ -4535,6 +4582,15 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
>>   \item If the notifications are not suppressed by the driver, the device will send an used buffer notification, otherwise, the device will not send an used buffer notification as long as the notifications are suppressed.
>>   \end{itemize}
>>
>> +If, example of setting coalescing parameters for a receive virtqueue:
>> +\begin{itemize}
>> +\item \field{max_packets} = 15.
>> +\item \field{usecs} = 10.
>> +\item \field{vqn} = 0;
>> +\end{itemize}
>> +
>> +The device will only operate on recieveq1 as VIRTIO_NET_CTRL_NOTF_COAL_RX_SET.
>> +
>>   \subparagraph{TX Notifications}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Notifications Coalescing / TX Notifications}
>>
>>   If, for example:
>> @@ -4550,13 +4606,24 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
>>   \item If the notifications are not suppressed by the driver, the device will send an used buffer notification, otherwise, the device will not send an used buffer notification as long as the notifications are suppressed.
>>   \end{itemize}
>>
>> +If, example of setting coalescing parameters for a transmit virtqueue:
>> +\begin{itemize}
>> +\item \field{max_packets} = 15.
>> +\item \field{usecs} = 10.
>> +\item \field{vqn} = 1;
>> +\end{itemize}
>> +
>> +The device will only operate on transmitq1 as VIRTIO_NET_CTRL_NOTF_COAL_TX_SET.
>> +
>>   \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 the VIRTIO_NET_F_VQ_NOTF_COAL feature has not been negotiated, the driver MUST NOT issue VIRTIO_NET_CTRL_NOFT_COAL_VQ commands.
>>
>>   \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 the VIRTIO_NET_CTRL_NOTF_COAL commands with VIRTIO_NET_ERR if it was not able to change the parameters or
>> +was not able to find a virtqueue using the \field{vqn}.
>>
>>   A device SHOULD NOT send used buffer notifications to the driver, if the notifications are suppressed as explained in \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Used Buffer Notification Suppression}, even if the coalescing counters expired.
>>
>> --
>> 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


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

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

[Heng Qi]

在 2023/2/10 下午5:38, Michael S. Tsirkin 写道:
> On Fri, Feb 10, 2023 at 11:22:14AM +0200, Alvaro Karsz wrote:
>> Thanks Heng.
>>
>>> Currently, the coalescing profile is directly applied to all queues.
>>> This patch supports setting or getting the parameters for a specified queue,
>>> and a typical application of this function is NetDIM.
>>>
>>> When the traffic between queues is unbalanced, for example, one queue
>>> is busy and another queue is idle, then it will be very useful to
>>> control coalescing parameters at the queue granularity.
>>>
>>> Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
>>> Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
>>> ---
>>> 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 structres 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
>>>
>>>   content.tex | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
>>>   1 file changed, 68 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/content.tex b/content.tex
>>> index e863709..2c497e1 100644
>>> --- a/content.tex
>>> +++ b/content.tex
>>> @@ -3084,6 +3084,9 @@ \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 the virtqueue
>>> +    notifications coalescing.
>>> +
>>>   \item[VIRTIO_NET_F_NOTF_COAL(53)] Device supports notifications coalescing.
>>>
>>>   \item[VIRTIO_NET_F_GUEST_USO4 (54)] Driver can receive USOv4 packets.
>>> @@ -3140,6 +3143,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 and VIRTIO_NET_F_NOTF_COAL.
>>>   \end{description}
>>>
>> Do we need VIRTIO_NET_F_CTRL_VQ in this case?
>> VIRTIO_NET_F_VQ_NOTF_COAL requires VIRTIO_NET_F_NOTF_COAL, which
>> requires VIRTIO_NET_F_CTRL_VQ, so it's implied.
>>
>> We have a similar example with VIRTIO_NET_F_HOST_ECN, which requires
>> VIRTIO_NET_F_HOST_TSO4 or VIRTIO_NET_F_HOST_TSO6.
>> VIRTIO_NET_F_CSUM is not mentioned here, although required.
>>
>> So, should we remove VIRTIO_NET_F_CTRL_VQ here, or fix VIRTIO_NET_F_HOST_ECN?
> Ah good point.
> But I think  VIRTIO_NET_F_VQ_NOTF_COAL should not depend on VIRTIO_NET_F_NOTF_COAL.
> This way devices can drop the all-rx/all-tx commands if they want to.

We need to confirm this. If we make VIRTIO_NET_F_VQ_NOTF_COAL 
independent of VIRTIO_NET_F_NOTF_COAL,
do we need to give vqn a special value so that the driver can also have 
the fast path of sending all queues with global settings via 
VIRTIO_NET_F_VQ_NOTF_COAL?

Thanks.

>
>
>>>   \subsubsection{Legacy Interface: Feature bits}\label{sec:Device Types / Network Device / Feature bits / Legacy Interface: Feature bits}
>>> @@ -4520,6 +4524,49 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
>>>   \item VIRTIO_NET_CTRL_NOTF_COAL_RX_SET: set the \field{rx_usecs} and \field{rx_max_packets} parameters.
>>>   \end{enumerate}
>>>
>>> +If additionally VIRTIO_NET_F_VQ_NOTF_COAL is negotiated, the driver can send
>>> +control commands to set or get the coalescing parameters of a specified
>>> +virtqueue (excluding the control virtqueue).
>>> +
>>> +\begin{lstlisting}
>>> +struct virtio_net_ctrl_coal_vq {
>>> +    le32 max_packets;
>>> +    le32 usecs;
>>> +    le16 vqn;
>>> +};
>>> +
>>> +#define VIRTIO_NET_CTRL_NOTF_COAL_VQ 7
>>> + #define VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET 0
>>> + #define VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET 1
>>> +\end{lstlisting}
>>> +
>> We already have a notifications coalescing class, why not group all
>> coalescing commands under a single class?
> Agree here.
>
>>> +Virtqueue coalescing parameters:
>>> +\begin{itemize}
>>> +\item \field{max_packets}: The maximum number of packets sent/received by the
>>> +    specified virtqueue before a TX/RX notification.
>>> +\item \field{usecs}: The maximum number of TX/RX usecs that the specified
>>> +    virtqueue delays a TX/RX notification.
>>> +\item \field{vqn}: The virtqueue number of the specified virtqueue.
>>> +\end{itemize}
>>> +
>>> +The range of \filed{vqn} is between 0 and 0xFFFF inclusive, $ \lfloor vqn / 2 \rfloor $
>>> +is the index of the corresponding receiveq, and $\lfloor (vqn / 2) + 1 \rfloor $ is
>>> +the corresponding tranmitq.
>>> +
>>> +The VIRTIO_NET_CTRL_NOTF_COAL_RX_SET command is the same as calling VIRTIO_NET_CTRL_NOTF_COAL_VQ
>>> +for virtqueues corresponding to all receiveqs.
>>> +
>>> +The VIRTIO_NET_CTRL_NOTF_COAL_TX_SET command is the same as calling VIRTIO_NET_CTRL_NOTF_COAL_VQ
>>> +for virtqueues corresponding to all transmitqs.
>>> +
>>> +Virtqueue coalescing will be disabled if all parameters are set to 0.
>>> +
>>> +The class VIRTIO_NET_CTRL_NOTF_COAL_VQ has 2 commands:
>>> +\begin{enumerate}
>>> +\item VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET: set the \field{max_packets}, \field{usecs} and \filed{vqn} parameters.
>>> +\item VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET: get the \field{max_packets}, \field{usecs} and \field{vqn} parameters.
>>> +\end{enumerate}
>>> +
>>>   \subparagraph{RX Notifications}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Notifications Coalescing / RX Notifications}
>>>
>>>   If, for example:
>>> @@ -4535,6 +4582,15 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
>>>   \item If the notifications are not suppressed by the driver, the device will send an used buffer notification, otherwise, the device will not send an used buffer notification as long as the notifications are suppressed.
>>>   \end{itemize}
>>>
>>> +If, example of setting coalescing parameters for a receive virtqueue:
>>> +\begin{itemize}
>>> +\item \field{max_packets} = 15.
>>> +\item \field{usecs} = 10.
>>> +\item \field{vqn} = 0;
>>> +\end{itemize}
>>> +
>>> +The device will only operate on recieveq1 as VIRTIO_NET_CTRL_NOTF_COAL_RX_SET.
>>> +
>>>   \subparagraph{TX Notifications}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Notifications Coalescing / TX Notifications}
>>>
>>>   If, for example:
>>> @@ -4550,13 +4606,24 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
>>>   \item If the notifications are not suppressed by the driver, the device will send an used buffer notification, otherwise, the device will not send an used buffer notification as long as the notifications are suppressed.
>>>   \end{itemize}
>>>
>>> +If, example of setting coalescing parameters for a transmit virtqueue:
>>> +\begin{itemize}
>>> +\item \field{max_packets} = 15.
>>> +\item \field{usecs} = 10.
>>> +\item \field{vqn} = 1;
>>> +\end{itemize}
>>> +
>>> +The device will only operate on transmitq1 as VIRTIO_NET_CTRL_NOTF_COAL_TX_SET.
>>> +
>>>   \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 the VIRTIO_NET_F_VQ_NOTF_COAL feature has not been negotiated, the driver MUST NOT issue VIRTIO_NET_CTRL_NOFT_COAL_VQ commands.
>>>
>>>   \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 the VIRTIO_NET_CTRL_NOTF_COAL commands with VIRTIO_NET_ERR if it was not able to change the parameters or
>>> +was not able to find a virtqueue using the \field{vqn}.
>>>
>>>   A device SHOULD NOT send used buffer notifications to the driver, if the notifications are suppressed as explained in \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Used Buffer Notification Suppression}, even if the coalescing counters expired.
>>>
>>> --
>>> 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/

[virtio-dev] Re: [virtio-comment] [PATCH v2] virtio-net: support the virtqueue coalescing moderation

[Alvaro Karsz]

> >> So, should we remove VIRTIO_NET_F_CTRL_VQ here, or fix VIRTIO_NET_F_HOST_ECN?
> > Ah good point.
> > But I think  VIRTIO_NET_F_VQ_NOTF_COAL should not depend on VIRTIO_NET_F_NOTF_COAL.
> > This way devices can drop the all-rx/all-tx commands if they want to.
>
> We need to confirm this. If we make VIRTIO_NET_F_VQ_NOTF_COAL
> independent of VIRTIO_NET_F_NOTF_COAL,
> do we need to give vqn a special value so that the driver can also have
> the fast path of sending all queues with global settings via
> VIRTIO_NET_F_VQ_NOTF_COAL?

IMO we don't need a special vqn value.
A device that can modify all the vqs should offer VIRTIO_NET_F_NOTF_COAL.

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

[virtio-dev] Re: [virtio-comment] Re: [PATCH v2] virtio-net: support the virtqueue coalescing moderation

[Michael S. Tsirkin]

On Fri, Feb 10, 2023 at 05:53:40PM +0800, Heng Qi wrote:
> 
> 
> 在 2023/2/10 下午4:38, Michael S. Tsirkin 写道:
> > On Fri, Feb 10, 2023 at 03:01:30PM +0800, Heng Qi wrote:
> > > Currently, the coalescing profile is directly applied to all queues.
> > > This patch supports setting or getting the parameters for a specified queue,
> > > and a typical application of this function is NetDIM.
> > > 
> > > When the traffic between queues is unbalanced, for example, one queue
> > > is busy and another queue is idle, then it will be very useful to
> > > control coalescing parameters at the queue granularity.
> > > 
> > > Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
> > > Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > 
> > I like this generally, thanks! Language needs to be tightened a bit.
> > > ---
> > > 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 structres 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
> > > 
> > >   content.tex | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
> > >   1 file changed, 68 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/content.tex b/content.tex
> > > index e863709..2c497e1 100644
> > > --- a/content.tex
> > > +++ b/content.tex
> > > @@ -3084,6 +3084,9 @@ \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 the virtqueue
> > > +    notifications coalescing.
> > > +
> > >   \item[VIRTIO_NET_F_NOTF_COAL(53)] Device supports notifications coalescing.
> > >   \item[VIRTIO_NET_F_GUEST_USO4 (54)] Driver can receive USOv4 packets.
> > > @@ -3140,6 +3143,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 and VIRTIO_NET_F_NOTF_COAL.
> > >   \end{description}
> > >   \subsubsection{Legacy Interface: Feature bits}\label{sec:Device Types / Network Device / Feature bits / Legacy Interface: Feature bits}
> > > @@ -4520,6 +4524,49 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
> > >   \item VIRTIO_NET_CTRL_NOTF_COAL_RX_SET: set the \field{rx_usecs} and \field{rx_max_packets} parameters.
> > >   \end{enumerate}
> > > +If additionally VIRTIO_NET_F_VQ_NOTF_COAL is negotiated, the driver can send
> > > +control commands to set or get the coalescing parameters of a specified
> > > +virtqueue (excluding the control virtqueue).
> > > +
> > > +\begin{lstlisting}
> > > +struct virtio_net_ctrl_coal_vq {
> > > +    le32 max_packets;
> > > +    le32 usecs;
> > > +    le16 vqn;
> > Add le16 reserved here, so keep things aligned naturally.
> > In fact if you want to support GET you need to re-order things
> > since write buffers come before read buffers:
> 
> I see, thanks for pointing it out.
> 
> >   +    le16 vqn;
> >   +    le16 reserved;
> > 
> >   +    le32 max_packets;
> >   +    le32 usecs;
> > 
> > see below for more explanation.
> > 
> > 
> > 
> > > +};
> > > +
> > > +#define VIRTIO_NET_CTRL_NOTF_COAL_VQ 7
> > > + #define VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET 0
> > > + #define VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET 1
> > > +\end{lstlisting}
> > > +
> > > +Virtqueue coalescing parameters:
> > > +\begin{itemize}
> > > +\item \field{max_packets}: The maximum number of packets sent/received by the
> > > +    specified virtqueue before a TX/RX notification.
> > > +\item \field{usecs}: The maximum number of TX/RX usecs that the specified
> > > +    virtqueue delays a TX/RX notification.
> > > +\item \field{vqn}: The virtqueue number of the specified virtqueue.
> > > +\end{itemize}
> > > +
> > > +The range of \filed{vqn} is between 0 and 0xFFFF inclusive,
> > 
> > No really because last vq is a cvq. Maybe just drop this?
> 
> In fact I have ruled out the control virtqueue.
> 
> Its virtqueue number should be 0x10000.

Nope, vq numberes are 16 bit.

> > 
> > > $ \lfloor vqn / 2 \rfloor $
> > > +is the index of the corresponding receiveq, and $\lfloor (vqn / 2) + 1 \rfloor $ is
> > > +the corresponding tranmitq.
> > > +
> > > +The VIRTIO_NET_CTRL_NOTF_COAL_RX_SET command is the same as calling VIRTIO_NET_CTRL_NOTF_COAL_VQ
> > you don't "call" commands. driver sends them, device receives them.
> > But here you are talking about a command abstracty so I'd just drop
> > a verb, or maybe "repeating".
> > And "same" is inexact in that it's not the same - takes more time - it
> > just has the same effect, or is equivalent to.
> 
> There is indeed a gerund match here, I'll fix that.
> 
> > 
> > 
> > > +for virtqueues corresponding to all receiveqs.
> > receiveqs is confusing.
> > 
> > Also elsewhere we use the language receiveq1\ldots receiveqn
> > which seems clearer. Also you can not call VIRTIO_NET_CTRL_NOTF_COAL_VQ
> > for all vqs - it applies to one vq only. You mean each.
> 
> I express something wrong, but what I mean is to send for each virtqueue.
> 
> > And virtqueues do not correspond to receiveqs - they
> > are receiveqs. If you like vqn corresponds to them.
> 
> This is ambiguous, the number of virtqueues is 2*N+1, the number of receive
> queues and transmit queues is N,
> and there is also a control queue. Is this a problem?
> But I know what you mean it's better to use "same as
> VIRTIO_NET_CTRL_NOTF_COAL_VQ repeated for each of receiveq1\ldots receiveqn"
> 
> > Or better just "same as VIRTIO_NET_CTRL_NOTF_COAL_VQ repeated for
> > each of receiveq1\ldots receiveqn"
> 
> Sure.
> 
> > > +
> > > +The VIRTIO_NET_CTRL_NOTF_COAL_TX_SET command is the same as calling VIRTIO_NET_CTRL_NOTF_COAL_VQ
> > > +for virtqueues corresponding to all transmitqs.
> > > +
> > > +Virtqueue coalescing will be disabled if all parameters are set to 0.
> > In fact, either usecs 0 or max_packets disables coalescing, no?
> 
> Yes. I'll fix this.
> 
> > 
> > > +
> > > +The class VIRTIO_NET_CTRL_NOTF_COAL_VQ has 2 commands:
> > > +\begin{enumerate}
> > > +\item VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET: set the \field{max_packets}, \field{usecs} and \filed{vqn} parameters.
> > > +\item VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET: get the \field{max_packets}, \field{usecs} and \field{vqn} parameters.
> > How does VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET work then? For which vq?
> > I think you mean vqn is specified and you get back max_packets and
> > usecs. All this needs to be documented fully for each command.
> 
> Yes, VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET gets the parameters of the virtqueue
> corresponding to vqn each time.
> 
> > 
> > I would also think it's a good idea to mention that VQ_GET does not have
> > to return exactly the same parameters - since it's best effort anyway,
> 
> This is confusing, I think the device does not have to set the same
> parameters for VQ_SET, but for VQ_GET, the device should return to the
> driver every time.

What I mean is that if you call VQ_SET with a value of 17,
device might decide to e.g. store just the power of 2


> > it's ok for device to round down and store a smaller value for either
> > max_packets or usecs, e.g. to save space.
> > 
> > This kind of formalizes the best effort thing we discussed
> > previously.
> > 
> > 
> > Maybe make VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET a separate patch -
> > in the series,
> 
> No problem, I'll try to describe it as best I can.
> 
> > at this point you did not make any effort to document it,
> > it needs more work.
> > 
> > 
> > > +\end{enumerate}
> > > +
> > >   \subparagraph{RX Notifications}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Notifications Coalescing / RX Notifications}
> > >   If, for example:
> > > @@ -4535,6 +4582,15 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
> > >   \item If the notifications are not suppressed by the driver, the device will send an used buffer notification, otherwise, the device will not send an used buffer notification as long as the notifications are suppressed.
> > >   \end{itemize}
> > > +If, example of setting coalescing parameters for a receive virtqueue:
> > "If" without "then" is confusing. I'd just start with "Example".
> 
> Ok.
> 
> > 
> > > +\begin{itemize}
> > > +\item \field{max_packets} = 15.
> > > +\item \field{usecs} = 10.
> > > +\item \field{vqn} = 0;
> > why . above and ; here? I would just drop punctuation.
> 
> It's a typo and I'll fix it.
> 
> > 
> > > +\end{itemize}
> > > +
> > > +The device will only operate on recieveq1 as VIRTIO_NET_CTRL_NOTF_COAL_RX_SET.
> > This really does not explain anything. Please just document exactly what
> > it does and does not do.
> 
> Ok.
> 
> > 
> > 
> > > +
> > >   \subparagraph{TX Notifications}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Notifications Coalescing / TX Notifications}
> > >   If, for example:
> > > @@ -4550,13 +4606,24 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
> > >   \item If the notifications are not suppressed by the driver, the device will send an used buffer notification, otherwise, the device will not send an used buffer notification as long as the notifications are suppressed.
> > >   \end{itemize}
> > > +If, example of setting coalescing parameters for a transmit virtqueue:
> > > +\begin{itemize}
> > > +\item \field{max_packets} = 15.
> > > +\item \field{usecs} = 10.
> > > +\item \field{vqn} = 1;
> > > +\end{itemize}
> > > +
> > > +The device will only operate on transmitq1 as VIRTIO_NET_CTRL_NOTF_COAL_TX_SET.
> > > +
> > I feel it's the other way around. Document
> > 
> 
> Why? I'll add documentation, but read on below.
> 
> > 
> > >   \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 the VIRTIO_NET_F_VQ_NOTF_COAL feature has not been negotiated, the driver MUST NOT issue VIRTIO_NET_CTRL_NOFT_COAL_VQ commands.
> > Don't we want to limit driver to legal values of vqn?
> > 
> 
> Yes, I will add.
> 
> > >   \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 the VIRTIO_NET_CTRL_NOTF_COAL commands with VIRTIO_NET_ERR if it was not able to change the parameters or
> > > +was not able to find a virtqueue using the \field{vqn}.
> > First please create multiple statements not a single long one.
> > Second vqn only exists for per vq commands so create a statement
> > just for that.
> 
> Ok, reasonable.
> 
> > Also more explicit please. What does this mean? I suspect that vq was not
> > enabled?
> 
> Indicates that a vqn cannot be mapped to the corresponding virtqueue.

Still no idea. what is this mapping you are talking about.
Why can't it be mapped? what is corresponding to what?

vq with this number is not enabled or vqn >= 2max_virtqueue_pairs are
the only reasons i could come up with.  if that's it just say so. if not
list the actual reasons.

> > Also, we MUST have vqn < max_virtqueue_pairs.
> 
> Here should be vq < max_virtqueue_pairs * 2?
> 
> Thanks.

yea.

> > 
> > 
> > >   A device SHOULD NOT send used buffer notifications to the driver, if the notifications are suppressed as explained in \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Used Buffer Notification Suppression}, even if the coalescing counters expired.
> > > -- 
> > > 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

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

[Michael S. Tsirkin]

On Fri, Feb 10, 2023 at 06:00:42PM +0800, Heng Qi wrote:
> 
> 
> 在 2023/2/10 下午5:38, Michael S. Tsirkin 写道:
> > On Fri, Feb 10, 2023 at 11:22:14AM +0200, Alvaro Karsz wrote:
> > > Thanks Heng.
> > > 
> > > > Currently, the coalescing profile is directly applied to all queues.
> > > > This patch supports setting or getting the parameters for a specified queue,
> > > > and a typical application of this function is NetDIM.
> > > > 
> > > > When the traffic between queues is unbalanced, for example, one queue
> > > > is busy and another queue is idle, then it will be very useful to
> > > > control coalescing parameters at the queue granularity.
> > > > 
> > > > Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
> > > > Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > > > ---
> > > > 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 structres 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
> > > > 
> > > >   content.tex | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
> > > >   1 file changed, 68 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/content.tex b/content.tex
> > > > index e863709..2c497e1 100644
> > > > --- a/content.tex
> > > > +++ b/content.tex
> > > > @@ -3084,6 +3084,9 @@ \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 the virtqueue
> > > > +    notifications coalescing.
> > > > +
> > > >   \item[VIRTIO_NET_F_NOTF_COAL(53)] Device supports notifications coalescing.
> > > > 
> > > >   \item[VIRTIO_NET_F_GUEST_USO4 (54)] Driver can receive USOv4 packets.
> > > > @@ -3140,6 +3143,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 and VIRTIO_NET_F_NOTF_COAL.
> > > >   \end{description}
> > > > 
> > > Do we need VIRTIO_NET_F_CTRL_VQ in this case?
> > > VIRTIO_NET_F_VQ_NOTF_COAL requires VIRTIO_NET_F_NOTF_COAL, which
> > > requires VIRTIO_NET_F_CTRL_VQ, so it's implied.
> > > 
> > > We have a similar example with VIRTIO_NET_F_HOST_ECN, which requires
> > > VIRTIO_NET_F_HOST_TSO4 or VIRTIO_NET_F_HOST_TSO6.
> > > VIRTIO_NET_F_CSUM is not mentioned here, although required.
> > > 
> > > So, should we remove VIRTIO_NET_F_CTRL_VQ here, or fix VIRTIO_NET_F_HOST_ECN?
> > Ah good point.
> > But I think  VIRTIO_NET_F_VQ_NOTF_COAL should not depend on VIRTIO_NET_F_NOTF_COAL.
> > This way devices can drop the all-rx/all-tx commands if they want to.
> 
> We need to confirm this. If we make VIRTIO_NET_F_VQ_NOTF_COAL independent of
> VIRTIO_NET_F_NOTF_COAL,
> do we need to give vqn a special value so that the driver can also have the
> fast path of sending all queues with global settings via
> VIRTIO_NET_F_VQ_NOTF_COAL?
> 
> Thanks.

No - without VIRTIO_NET_F_NOTF_COAL driver has to iterate
over all vqs if it wants to do something with all of them.


> > 
> > 
> > > >   \subsubsection{Legacy Interface: Feature bits}\label{sec:Device Types / Network Device / Feature bits / Legacy Interface: Feature bits}
> > > > @@ -4520,6 +4524,49 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
> > > >   \item VIRTIO_NET_CTRL_NOTF_COAL_RX_SET: set the \field{rx_usecs} and \field{rx_max_packets} parameters.
> > > >   \end{enumerate}
> > > > 
> > > > +If additionally VIRTIO_NET_F_VQ_NOTF_COAL is negotiated, the driver can send
> > > > +control commands to set or get the coalescing parameters of a specified
> > > > +virtqueue (excluding the control virtqueue).
> > > > +
> > > > +\begin{lstlisting}
> > > > +struct virtio_net_ctrl_coal_vq {
> > > > +    le32 max_packets;
> > > > +    le32 usecs;
> > > > +    le16 vqn;
> > > > +};
> > > > +
> > > > +#define VIRTIO_NET_CTRL_NOTF_COAL_VQ 7
> > > > + #define VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET 0
> > > > + #define VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET 1
> > > > +\end{lstlisting}
> > > > +
> > > We already have a notifications coalescing class, why not group all
> > > coalescing commands under a single class?
> > Agree here.
> > 
> > > > +Virtqueue coalescing parameters:
> > > > +\begin{itemize}
> > > > +\item \field{max_packets}: The maximum number of packets sent/received by the
> > > > +    specified virtqueue before a TX/RX notification.
> > > > +\item \field{usecs}: The maximum number of TX/RX usecs that the specified
> > > > +    virtqueue delays a TX/RX notification.
> > > > +\item \field{vqn}: The virtqueue number of the specified virtqueue.
> > > > +\end{itemize}
> > > > +
> > > > +The range of \filed{vqn} is between 0 and 0xFFFF inclusive, $ \lfloor vqn / 2 \rfloor $
> > > > +is the index of the corresponding receiveq, and $\lfloor (vqn / 2) + 1 \rfloor $ is
> > > > +the corresponding tranmitq.
> > > > +
> > > > +The VIRTIO_NET_CTRL_NOTF_COAL_RX_SET command is the same as calling VIRTIO_NET_CTRL_NOTF_COAL_VQ
> > > > +for virtqueues corresponding to all receiveqs.
> > > > +
> > > > +The VIRTIO_NET_CTRL_NOTF_COAL_TX_SET command is the same as calling VIRTIO_NET_CTRL_NOTF_COAL_VQ
> > > > +for virtqueues corresponding to all transmitqs.
> > > > +
> > > > +Virtqueue coalescing will be disabled if all parameters are set to 0.
> > > > +
> > > > +The class VIRTIO_NET_CTRL_NOTF_COAL_VQ has 2 commands:
> > > > +\begin{enumerate}
> > > > +\item VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET: set the \field{max_packets}, \field{usecs} and \filed{vqn} parameters.
> > > > +\item VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET: get the \field{max_packets}, \field{usecs} and \field{vqn} parameters.
> > > > +\end{enumerate}
> > > > +
> > > >   \subparagraph{RX Notifications}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Notifications Coalescing / RX Notifications}
> > > > 
> > > >   If, for example:
> > > > @@ -4535,6 +4582,15 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
> > > >   \item If the notifications are not suppressed by the driver, the device will send an used buffer notification, otherwise, the device will not send an used buffer notification as long as the notifications are suppressed.
> > > >   \end{itemize}
> > > > 
> > > > +If, example of setting coalescing parameters for a receive virtqueue:
> > > > +\begin{itemize}
> > > > +\item \field{max_packets} = 15.
> > > > +\item \field{usecs} = 10.
> > > > +\item \field{vqn} = 0;
> > > > +\end{itemize}
> > > > +
> > > > +The device will only operate on recieveq1 as VIRTIO_NET_CTRL_NOTF_COAL_RX_SET.
> > > > +
> > > >   \subparagraph{TX Notifications}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Notifications Coalescing / TX Notifications}
> > > > 
> > > >   If, for example:
> > > > @@ -4550,13 +4606,24 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
> > > >   \item If the notifications are not suppressed by the driver, the device will send an used buffer notification, otherwise, the device will not send an used buffer notification as long as the notifications are suppressed.
> > > >   \end{itemize}
> > > > 
> > > > +If, example of setting coalescing parameters for a transmit virtqueue:
> > > > +\begin{itemize}
> > > > +\item \field{max_packets} = 15.
> > > > +\item \field{usecs} = 10.
> > > > +\item \field{vqn} = 1;
> > > > +\end{itemize}
> > > > +
> > > > +The device will only operate on transmitq1 as VIRTIO_NET_CTRL_NOTF_COAL_TX_SET.
> > > > +
> > > >   \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 the VIRTIO_NET_F_VQ_NOTF_COAL feature has not been negotiated, the driver MUST NOT issue VIRTIO_NET_CTRL_NOFT_COAL_VQ commands.
> > > > 
> > > >   \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 the VIRTIO_NET_CTRL_NOTF_COAL commands with VIRTIO_NET_ERR if it was not able to change the parameters or
> > > > +was not able to find a virtqueue using the \field{vqn}.
> > > > 
> > > >   A device SHOULD NOT send used buffer notifications to the driver, if the notifications are suppressed as explained in \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Used Buffer Notification Suppression}, even if the coalescing counters expired.
> > > > 
> > > > --
> > > > 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/

[virtio-comment] Re: [virtio-dev] Re: [virtio-comment] Re: [PATCH v2] virtio-net: support the virtqueue coalescing moderation

[Heng Qi]

在 2023/2/10 下午6:29, Michael S. Tsirkin 写道:
> On Fri, Feb 10, 2023 at 05:53:40PM +0800, Heng Qi wrote:
>>
>> 在 2023/2/10 下午4:38, Michael S. Tsirkin 写道:
>>> On Fri, Feb 10, 2023 at 03:01:30PM +0800, Heng Qi wrote:
>>>> Currently, the coalescing profile is directly applied to all queues.
>>>> This patch supports setting or getting the parameters for a specified queue,
>>>> and a typical application of this function is NetDIM.
>>>>
>>>> When the traffic between queues is unbalanced, for example, one queue
>>>> is busy and another queue is idle, then it will be very useful to
>>>> control coalescing parameters at the queue granularity.
>>>>
>>>> Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
>>>> Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
>>> I like this generally, thanks! Language needs to be tightened a bit.
>>>> ---
>>>> 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 structres 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
>>>>
>>>>    content.tex | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
>>>>    1 file changed, 68 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/content.tex b/content.tex
>>>> index e863709..2c497e1 100644
>>>> --- a/content.tex
>>>> +++ b/content.tex
>>>> @@ -3084,6 +3084,9 @@ \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 the virtqueue
>>>> +    notifications coalescing.
>>>> +
>>>>    \item[VIRTIO_NET_F_NOTF_COAL(53)] Device supports notifications coalescing.
>>>>    \item[VIRTIO_NET_F_GUEST_USO4 (54)] Driver can receive USOv4 packets.
>>>> @@ -3140,6 +3143,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 and VIRTIO_NET_F_NOTF_COAL.
>>>>    \end{description}
>>>>    \subsubsection{Legacy Interface: Feature bits}\label{sec:Device Types / Network Device / Feature bits / Legacy Interface: Feature bits}
>>>> @@ -4520,6 +4524,49 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
>>>>    \item VIRTIO_NET_CTRL_NOTF_COAL_RX_SET: set the \field{rx_usecs} and \field{rx_max_packets} parameters.
>>>>    \end{enumerate}
>>>> +If additionally VIRTIO_NET_F_VQ_NOTF_COAL is negotiated, the driver can send
>>>> +control commands to set or get the coalescing parameters of a specified
>>>> +virtqueue (excluding the control virtqueue).
>>>> +
>>>> +\begin{lstlisting}
>>>> +struct virtio_net_ctrl_coal_vq {
>>>> +    le32 max_packets;
>>>> +    le32 usecs;
>>>> +    le16 vqn;
>>> Add le16 reserved here, so keep things aligned naturally.
>>> In fact if you want to support GET you need to re-order things
>>> since write buffers come before read buffers:
>> I see, thanks for pointing it out.
>>
>>>    +    le16 vqn;
>>>    +    le16 reserved;
>>>
>>>    +    le32 max_packets;
>>>    +    le32 usecs;
>>>
>>> see below for more explanation.
>>>
>>>
>>>
>>>> +};
>>>> +
>>>> +#define VIRTIO_NET_CTRL_NOTF_COAL_VQ 7
>>>> + #define VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET 0
>>>> + #define VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET 1
>>>> +\end{lstlisting}
>>>> +
>>>> +Virtqueue coalescing parameters:
>>>> +\begin{itemize}
>>>> +\item \field{max_packets}: The maximum number of packets sent/received by the
>>>> +    specified virtqueue before a TX/RX notification.
>>>> +\item \field{usecs}: The maximum number of TX/RX usecs that the specified
>>>> +    virtqueue delays a TX/RX notification.
>>>> +\item \field{vqn}: The virtqueue number of the specified virtqueue.
>>>> +\end{itemize}
>>>> +
>>>> +The range of \filed{vqn} is between 0 and 0xFFFF inclusive,
>>> No really because last vq is a cvq. Maybe just drop this?
>> In fact I have ruled out the control virtqueue.
>>
>> Its virtqueue number should be 0x10000.
> Nope, vq numberes are 16 bit.

Yes I know. The [0, 0xFFFF] listed in v2 is the maximum range of 
virtqueue numbers (including all receiveqs and all transmitqs)
that can be set by the driver, because the current spec says
"The device MUST set \field{max_virtqueue_pairs} to between 1 and 0x8000 
inclusive, if it offers VIRTIO_NET_F_MQ.".
So assuming that the maximum number of receiveqs and transmitqs are 
0x8000 respectively,
then the total is 0x10000 (the queue number to the control queue is 
0x10001), that is, the vqn indexed from 0 is [0,0x10000-1]=[0,0xFFFF ],
which already excludes controlq.

But I know that you mean to emphasize in the specification that no 
contro queue is included.

>
>>>> $ \lfloor vqn / 2 \rfloor $
>>>> +is the index of the corresponding receiveq, and $\lfloor (vqn / 2) + 1 \rfloor $ is
>>>> +the corresponding tranmitq.
>>>> +
>>>> +The VIRTIO_NET_CTRL_NOTF_COAL_RX_SET command is the same as calling VIRTIO_NET_CTRL_NOTF_COAL_VQ
>>> you don't "call" commands. driver sends them, device receives them.
>>> But here you are talking about a command abstracty so I'd just drop
>>> a verb, or maybe "repeating".
>>> And "same" is inexact in that it's not the same - takes more time - it
>>> just has the same effect, or is equivalent to.
>> There is indeed a gerund match here, I'll fix that.
>>
>>>
>>>> +for virtqueues corresponding to all receiveqs.
>>> receiveqs is confusing.
>>>
>>> Also elsewhere we use the language receiveq1\ldots receiveqn
>>> which seems clearer. Also you can not call VIRTIO_NET_CTRL_NOTF_COAL_VQ
>>> for all vqs - it applies to one vq only. You mean each.
>> I express something wrong, but what I mean is to send for each virtqueue.
>>
>>> And virtqueues do not correspond to receiveqs - they
>>> are receiveqs. If you like vqn corresponds to them.
>> This is ambiguous, the number of virtqueues is 2*N+1, the number of receive
>> queues and transmit queues is N,
>> and there is also a control queue. Is this a problem?
>> But I know what you mean it's better to use "same as
>> VIRTIO_NET_CTRL_NOTF_COAL_VQ repeated for each of receiveq1\ldots receiveqn"
>>
>>> Or better just "same as VIRTIO_NET_CTRL_NOTF_COAL_VQ repeated for
>>> each of receiveq1\ldots receiveqn"
>> Sure.
>>
>>>> +
>>>> +The VIRTIO_NET_CTRL_NOTF_COAL_TX_SET command is the same as calling VIRTIO_NET_CTRL_NOTF_COAL_VQ
>>>> +for virtqueues corresponding to all transmitqs.
>>>> +
>>>> +Virtqueue coalescing will be disabled if all parameters are set to 0.
>>> In fact, either usecs 0 or max_packets disables coalescing, no?
>> Yes. I'll fix this.
>>
>>>> +
>>>> +The class VIRTIO_NET_CTRL_NOTF_COAL_VQ has 2 commands:
>>>> +\begin{enumerate}
>>>> +\item VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET: set the \field{max_packets}, \field{usecs} and \filed{vqn} parameters.
>>>> +\item VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET: get the \field{max_packets}, \field{usecs} and \field{vqn} parameters.
>>> How does VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET work then? For which vq?
>>> I think you mean vqn is specified and you get back max_packets and
>>> usecs. All this needs to be documented fully for each command.
>> Yes, VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET gets the parameters of the virtqueue
>> corresponding to vqn each time.
>>
>>> I would also think it's a good idea to mention that VQ_GET does not have
>>> to return exactly the same parameters - since it's best effort anyway,
>> This is confusing, I think the device does not have to set the same
>> parameters for VQ_SET, but for VQ_GET, the device should return to the
>> driver every time.
> What I mean is that if you call VQ_SET with a value of 17,
> device might decide to e.g. store just the power of 2

Oh! I misunderstood what you meant before, I will add an appropriate 
reminder!

>
>
>>> it's ok for device to round down and store a smaller value for either
>>> max_packets or usecs, e.g. to save space.
>>>
>>> This kind of formalizes the best effort thing we discussed
>>> previously.
>>>
>>>
>>> Maybe make VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET a separate patch -
>>> in the series,
>> No problem, I'll try to describe it as best I can.
>>
>>> at this point you did not make any effort to document it,
>>> it needs more work.
>>>
>>>
>>>> +\end{enumerate}
>>>> +
>>>>    \subparagraph{RX Notifications}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Notifications Coalescing / RX Notifications}
>>>>    If, for example:
>>>> @@ -4535,6 +4582,15 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
>>>>    \item If the notifications are not suppressed by the driver, the device will send an used buffer notification, otherwise, the device will not send an used buffer notification as long as the notifications are suppressed.
>>>>    \end{itemize}
>>>> +If, example of setting coalescing parameters for a receive virtqueue:
>>> "If" without "then" is confusing. I'd just start with "Example".
>> Ok.
>>
>>>> +\begin{itemize}
>>>> +\item \field{max_packets} = 15.
>>>> +\item \field{usecs} = 10.
>>>> +\item \field{vqn} = 0;
>>> why . above and ; here? I would just drop punctuation.
>> It's a typo and I'll fix it.
>>
>>>> +\end{itemize}
>>>> +
>>>> +The device will only operate on recieveq1 as VIRTIO_NET_CTRL_NOTF_COAL_RX_SET.
>>> This really does not explain anything. Please just document exactly what
>>> it does and does not do.
>> Ok.
>>
>>>
>>>> +
>>>>    \subparagraph{TX Notifications}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Notifications Coalescing / TX Notifications}
>>>>    If, for example:
>>>> @@ -4550,13 +4606,24 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
>>>>    \item If the notifications are not suppressed by the driver, the device will send an used buffer notification, otherwise, the device will not send an used buffer notification as long as the notifications are suppressed.
>>>>    \end{itemize}
>>>> +If, example of setting coalescing parameters for a transmit virtqueue:
>>>> +\begin{itemize}
>>>> +\item \field{max_packets} = 15.
>>>> +\item \field{usecs} = 10.
>>>> +\item \field{vqn} = 1;
>>>> +\end{itemize}
>>>> +
>>>> +The device will only operate on transmitq1 as VIRTIO_NET_CTRL_NOTF_COAL_TX_SET.
>>>> +
>>> I feel it's the other way around. Document
>>>
>> Why? I'll add documentation, but read on below.
>>
>>>>    \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 the VIRTIO_NET_F_VQ_NOTF_COAL feature has not been negotiated, the driver MUST NOT issue VIRTIO_NET_CTRL_NOFT_COAL_VQ commands.
>>> Don't we want to limit driver to legal values of vqn?
>>>
>> Yes, I will add.
>>
>>>>    \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 the VIRTIO_NET_CTRL_NOTF_COAL commands with VIRTIO_NET_ERR if it was not able to change the parameters or
>>>> +was not able to find a virtqueue using the \field{vqn}.
>>> First please create multiple statements not a single long one.
>>> Second vqn only exists for per vq commands so create a statement
>>> just for that.
>> Ok, reasonable.
>>
>>> Also more explicit please. What does this mean? I suspect that vq was not
>>> enabled?
>> Indicates that a vqn cannot be mapped to the corresponding virtqueue.
> Still no idea. what is this mapping you are talking about.
> Why can't it be mapped? what is corresponding to what?
>
> vq with this number is not enabled or vqn >= 2max_virtqueue_pairs are

I am referring to the former.

> the only reasons i could come up with.  if that's it just say so. if not
> list the actual reasons.

Sorry, I will explain more clearly later.

Thanks.

>
>>> Also, we MUST have vqn < max_virtqueue_pairs.
>> Here should be vq < max_virtqueue_pairs * 2?
>>
>> Thanks.
> yea.
>
>>>
>>>>    A device SHOULD NOT send used buffer notifications to the driver, if the notifications are suppressed as explained in \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Used Buffer Notification Suppression}, even if the coalescing counters expired.
>>>> -- 
>>>> 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


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

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

[Heng Qi]

在 2023/2/10 下午6:16, Alvaro Karsz 写道:
>>>> So, should we remove VIRTIO_NET_F_CTRL_VQ here, or fix VIRTIO_NET_F_HOST_ECN?
>>> Ah good point.
>>> But I think  VIRTIO_NET_F_VQ_NOTF_COAL should not depend on VIRTIO_NET_F_NOTF_COAL.
>>> This way devices can drop the all-rx/all-tx commands if they want to.
>> We need to confirm this. If we make VIRTIO_NET_F_VQ_NOTF_COAL
>> independent of VIRTIO_NET_F_NOTF_COAL,
>> do we need to give vqn a special value so that the driver can also have
>> the fast path of sending all queues with global settings via
>> VIRTIO_NET_F_VQ_NOTF_COAL?
> IMO we don't need a special vqn value.
> A device that can modify all the vqs should offer VIRTIO_NET_F_NOTF_COAL.

That's clear, thanks for the quick reply.
Have a great weekend!


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

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

[Heng Qi]

在 2023/2/10 下午6:31, Michael S. Tsirkin 写道:
> On Fri, Feb 10, 2023 at 06:00:42PM +0800, Heng Qi wrote:
>>
>> 在 2023/2/10 下午5:38, Michael S. Tsirkin 写道:
>>> On Fri, Feb 10, 2023 at 11:22:14AM +0200, Alvaro Karsz wrote:
>>>> Thanks Heng.
>>>>
>>>>> Currently, the coalescing profile is directly applied to all queues.
>>>>> This patch supports setting or getting the parameters for a specified queue,
>>>>> and a typical application of this function is NetDIM.
>>>>>
>>>>> When the traffic between queues is unbalanced, for example, one queue
>>>>> is busy and another queue is idle, then it will be very useful to
>>>>> control coalescing parameters at the queue granularity.
>>>>>
>>>>> Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
>>>>> Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
>>>>> ---
>>>>> 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 structres 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
>>>>>
>>>>>    content.tex | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
>>>>>    1 file changed, 68 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/content.tex b/content.tex
>>>>> index e863709..2c497e1 100644
>>>>> --- a/content.tex
>>>>> +++ b/content.tex
>>>>> @@ -3084,6 +3084,9 @@ \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 the virtqueue
>>>>> +    notifications coalescing.
>>>>> +
>>>>>    \item[VIRTIO_NET_F_NOTF_COAL(53)] Device supports notifications coalescing.
>>>>>
>>>>>    \item[VIRTIO_NET_F_GUEST_USO4 (54)] Driver can receive USOv4 packets.
>>>>> @@ -3140,6 +3143,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 and VIRTIO_NET_F_NOTF_COAL.
>>>>>    \end{description}
>>>>>
>>>> Do we need VIRTIO_NET_F_CTRL_VQ in this case?
>>>> VIRTIO_NET_F_VQ_NOTF_COAL requires VIRTIO_NET_F_NOTF_COAL, which
>>>> requires VIRTIO_NET_F_CTRL_VQ, so it's implied.
>>>>
>>>> We have a similar example with VIRTIO_NET_F_HOST_ECN, which requires
>>>> VIRTIO_NET_F_HOST_TSO4 or VIRTIO_NET_F_HOST_TSO6.
>>>> VIRTIO_NET_F_CSUM is not mentioned here, although required.
>>>>
>>>> So, should we remove VIRTIO_NET_F_CTRL_VQ here, or fix VIRTIO_NET_F_HOST_ECN?
>>> Ah good point.
>>> But I think  VIRTIO_NET_F_VQ_NOTF_COAL should not depend on VIRTIO_NET_F_NOTF_COAL.
>>> This way devices can drop the all-rx/all-tx commands if they want to.
>> We need to confirm this. If we make VIRTIO_NET_F_VQ_NOTF_COAL independent of
>> VIRTIO_NET_F_NOTF_COAL,
>> do we need to give vqn a special value so that the driver can also have the
>> fast path of sending all queues with global settings via
>> VIRTIO_NET_F_VQ_NOTF_COAL?
>>
>> Thanks.
> No - without VIRTIO_NET_F_NOTF_COAL driver has to iterate
> over all vqs if it wants to do something with all of them.

Ok, this allows the driver to use both features more explicitly.
Thanks for the quick reply. 😁

>
>
>>>
>>>>>    \subsubsection{Legacy Interface: Feature bits}\label{sec:Device Types / Network Device / Feature bits / Legacy Interface: Feature bits}
>>>>> @@ -4520,6 +4524,49 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
>>>>>    \item VIRTIO_NET_CTRL_NOTF_COAL_RX_SET: set the \field{rx_usecs} and \field{rx_max_packets} parameters.
>>>>>    \end{enumerate}
>>>>>
>>>>> +If additionally VIRTIO_NET_F_VQ_NOTF_COAL is negotiated, the driver can send
>>>>> +control commands to set or get the coalescing parameters of a specified
>>>>> +virtqueue (excluding the control virtqueue).
>>>>> +
>>>>> +\begin{lstlisting}
>>>>> +struct virtio_net_ctrl_coal_vq {
>>>>> +    le32 max_packets;
>>>>> +    le32 usecs;
>>>>> +    le16 vqn;
>>>>> +};
>>>>> +
>>>>> +#define VIRTIO_NET_CTRL_NOTF_COAL_VQ 7
>>>>> + #define VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET 0
>>>>> + #define VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET 1
>>>>> +\end{lstlisting}
>>>>> +
>>>> We already have a notifications coalescing class, why not group all
>>>> coalescing commands under a single class?
>>> Agree here.
>>>
>>>>> +Virtqueue coalescing parameters:
>>>>> +\begin{itemize}
>>>>> +\item \field{max_packets}: The maximum number of packets sent/received by the
>>>>> +    specified virtqueue before a TX/RX notification.
>>>>> +\item \field{usecs}: The maximum number of TX/RX usecs that the specified
>>>>> +    virtqueue delays a TX/RX notification.
>>>>> +\item \field{vqn}: The virtqueue number of the specified virtqueue.
>>>>> +\end{itemize}
>>>>> +
>>>>> +The range of \filed{vqn} is between 0 and 0xFFFF inclusive, $ \lfloor vqn / 2 \rfloor $
>>>>> +is the index of the corresponding receiveq, and $\lfloor (vqn / 2) + 1 \rfloor $ is
>>>>> +the corresponding tranmitq.
>>>>> +
>>>>> +The VIRTIO_NET_CTRL_NOTF_COAL_RX_SET command is the same as calling VIRTIO_NET_CTRL_NOTF_COAL_VQ
>>>>> +for virtqueues corresponding to all receiveqs.
>>>>> +
>>>>> +The VIRTIO_NET_CTRL_NOTF_COAL_TX_SET command is the same as calling VIRTIO_NET_CTRL_NOTF_COAL_VQ
>>>>> +for virtqueues corresponding to all transmitqs.
>>>>> +
>>>>> +Virtqueue coalescing will be disabled if all parameters are set to 0.
>>>>> +
>>>>> +The class VIRTIO_NET_CTRL_NOTF_COAL_VQ has 2 commands:
>>>>> +\begin{enumerate}
>>>>> +\item VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET: set the \field{max_packets}, \field{usecs} and \filed{vqn} parameters.
>>>>> +\item VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET: get the \field{max_packets}, \field{usecs} and \field{vqn} parameters.
>>>>> +\end{enumerate}
>>>>> +
>>>>>    \subparagraph{RX Notifications}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Notifications Coalescing / RX Notifications}
>>>>>
>>>>>    If, for example:
>>>>> @@ -4535,6 +4582,15 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
>>>>>    \item If the notifications are not suppressed by the driver, the device will send an used buffer notification, otherwise, the device will not send an used buffer notification as long as the notifications are suppressed.
>>>>>    \end{itemize}
>>>>>
>>>>> +If, example of setting coalescing parameters for a receive virtqueue:
>>>>> +\begin{itemize}
>>>>> +\item \field{max_packets} = 15.
>>>>> +\item \field{usecs} = 10.
>>>>> +\item \field{vqn} = 0;
>>>>> +\end{itemize}
>>>>> +
>>>>> +The device will only operate on recieveq1 as VIRTIO_NET_CTRL_NOTF_COAL_RX_SET.
>>>>> +
>>>>>    \subparagraph{TX Notifications}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Notifications Coalescing / TX Notifications}
>>>>>
>>>>>    If, for example:
>>>>> @@ -4550,13 +4606,24 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
>>>>>    \item If the notifications are not suppressed by the driver, the device will send an used buffer notification, otherwise, the device will not send an used buffer notification as long as the notifications are suppressed.
>>>>>    \end{itemize}
>>>>>
>>>>> +If, example of setting coalescing parameters for a transmit virtqueue:
>>>>> +\begin{itemize}
>>>>> +\item \field{max_packets} = 15.
>>>>> +\item \field{usecs} = 10.
>>>>> +\item \field{vqn} = 1;
>>>>> +\end{itemize}
>>>>> +
>>>>> +The device will only operate on transmitq1 as VIRTIO_NET_CTRL_NOTF_COAL_TX_SET.
>>>>> +
>>>>>    \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 the VIRTIO_NET_F_VQ_NOTF_COAL feature has not been negotiated, the driver MUST NOT issue VIRTIO_NET_CTRL_NOFT_COAL_VQ commands.
>>>>>
>>>>>    \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 the VIRTIO_NET_CTRL_NOTF_COAL commands with VIRTIO_NET_ERR if it was not able to change the parameters or
>>>>> +was not able to find a virtqueue using the \field{vqn}.
>>>>>
>>>>>    A device SHOULD NOT send used buffer notifications to the driver, if the notifications are suppressed as explained in \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Used Buffer Notification Suppression}, even if the coalescing counters expired.
>>>>>
>>>>> --
>>>>> 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/

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

[Parav Pandit]

> From: Heng Qi <hengqi@linux.alibaba.com>
> Sent: Friday, February 10, 2023 2:02 AM
> 
> Currently, the coalescing profile is directly applied to all queues.

Say it,
Currently coalescing parameters are grouped for all transmit and receive virtqueues.

> This patch supports setting or getting the parameters for a specified queue, and
> a typical application of this function is NetDIM.
Many of us know the net dim.
But if you prefer to mention it here, better to have the link for it.

Please add pointer to it.
[1] https://docs.kernel.org/networking/net_dim.html

> 
> When the traffic between queues is unbalanced, for example, one queue is
> busy and another queue is idle, then it will be very useful to control coalescing
> parameters at the queue granularity.
> 
> 
> +If additionally VIRTIO_NET_F_VQ_NOTF_COAL is negotiated, the driver can
> +send control commands to set or get the coalescing parameters of a
Control command singular?

> +specified virtqueue (excluding the control virtqueue).
> +
> +\begin{lstlisting}
> +struct virtio_net_ctrl_coal_vq {
> +    le32 max_packets;
> +    le32 usecs;
> +    le16 vqn;
> +};
> +
Change that Michael suggest restructuring and under same class looks good to me.

> +#define VIRTIO_NET_CTRL_NOTF_COAL_VQ 7
> + #define VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET 0  #define
> +VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET 1 \end{lstlisting}
> +
> +Virtqueue coalescing parameters:
> +\begin{itemize}
> +\item \field{max_packets}: The maximum number of packets sent/received by
> the
> +    specified virtqueue before a TX/RX notification.
> +\item \field{usecs}: The maximum number of TX/RX usecs that the specified
> +    virtqueue delays a TX/RX notification.
> +\item \field{vqn}: The virtqueue number of the specified virtqueue.
> +\end{itemize}
> +
The virtqueue number of the enabled transmit or receive virtuqueue.
This will simplify below description.

> +The range of \filed{vqn} is between 0 and 0xFFFF inclusive, $ \lfloor
> +vqn / 2 \rfloor $ is the index of the corresponding receiveq, and
> +$\lfloor (vqn / 2) + 1 \rfloor $ is the corresponding tranmitq.
> +

Please add short description something like,

When the driver prefers to use per virtqueue notifications coalescing, and if queue group (transmit or receive) level notification coalescing is enabled, driver SHOULD first disable device level notification coalescing.
Or it should be,

Virtqueue level notifications coalescing, and device level notifications can be enabled together.
When both of them are enabled, per virtqueue notifications coalescing take priority over queue group level.

With rests of the comments from Michael and Alvaro in progress, looks good.

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/

[virtio-comment] Re: [virtio-dev] RE: [PATCH v2] virtio-net: support the virtqueue coalescing moderation

[Heng Qi]

在 2023/2/11 上午3:36, Parav Pandit 写道:
>
>> From: Heng Qi <hengqi@linux.alibaba.com>
>> Sent: Friday, February 10, 2023 2:02 AM
>>
>> Currently, the coalescing profile is directly applied to all queues.
> Say it,
> Currently coalescing parameters are grouped for all transmit and receive virtqueues.

Sure.

>
>> This patch supports setting or getting the parameters for a specified queue, and
>> a typical application of this function is NetDIM.
> Many of us know the net dim.
> But if you prefer to mention it here, better to have the link for it.

Yes, but this sentence is just a reminder for those who may not know the 
current usage scenarios of it.

>
> Please add pointer to it.
> [1] https://docs.kernel.org/networking/net_dim.html

Good opinion.

>
>> When the traffic between queues is unbalanced, for example, one queue is
>> busy and another queue is idle, then it will be very useful to control coalescing
>> parameters at the queue granularity.
>>
>>
>> +If additionally VIRTIO_NET_F_VQ_NOTF_COAL is negotiated, the driver can
>> +send control commands to set or get the coalescing parameters of a
> Control command singular?

Yes.

>> +specified virtqueue (excluding the control virtqueue).
>> +
>> +\begin{lstlisting}
>> +struct virtio_net_ctrl_coal_vq {
>> +    le32 max_packets;
>> +    le32 usecs;
>> +    le16 vqn;
>> +};
>> +
> Change that Michael suggest restructuring and under same class looks good to me.
>
>> +#define VIRTIO_NET_CTRL_NOTF_COAL_VQ 7
>> + #define VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET 0  #define
>> +VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET 1 \end{lstlisting}
>> +
>> +Virtqueue coalescing parameters:
>> +\begin{itemize}
>> +\item \field{max_packets}: The maximum number of packets sent/received by
>> the
>> +    specified virtqueue before a TX/RX notification.
>> +\item \field{usecs}: The maximum number of TX/RX usecs that the specified
>> +    virtqueue delays a TX/RX notification.
>> +\item \field{vqn}: The virtqueue number of the specified virtqueue.
>> +\end{itemize}
>> +
> The virtqueue number of the enabled transmit or receive virtuqueue.
> This will simplify below description.

Good opinion, I like this better.

>> +The range of \filed{vqn} is between 0 and 0xFFFF inclusive, $ \lfloor
>> +vqn / 2 \rfloor $ is the index of the corresponding receiveq, and
>> +$\lfloor (vqn / 2) + 1 \rfloor $ is the corresponding tranmitq.
>> +
> Please add short description something like,
>
> When the driver prefers to use per virtqueue notifications coalescing, and if queue group (transmit or receive) level notification coalescing is enabled, driver SHOULD first disable device level notification coalescing.

I think this is more in line with what netdim does, I'll add this.

Thanks.

> Or it should be,
>
> Virtqueue level notifications coalescing, and device level notifications can be enabled together.
> When both of them are enabled, per virtqueue notifications coalescing take priority over queue group level.
>
> With rests of the comments from Michael and Alvaro in progress, looks good.
>
> ---------------------------------------------------------------------
> 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/

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

[Alvaro Karsz]

> Please add short description something like,
>
> When the driver prefers to use per virtqueue notifications coalescing, and if queue group (transmit or receive) level notification coalescing is enabled, driver SHOULD first disable device level notification coalescing.
> Or it should be,
>

I disagree here.
IMO "queue group level notification coalescing" is not something to
enable or disable, but a shortcut to set all TX/RX queues at once.
Why should the spec force a driver to "disable device level
notification coalescing" (I assume you mean send a
VIRTIO_NET_CTRL_NOTF_COAL_[T/R]X_SET command with zeros)?
What if the driver sends a VIRTIO_NET_CTRL_NOTF_COAL_[T/R]X_SET
command, and then a single queue traffic increases? why should it zero
the parameters to all other queues?
I think that this should be discussed in the driver implementation
stage, not in the spec.

> Virtqueue level notifications coalescing, and device level notifications can be enabled together.
> When both of them are enabled, per virtqueue notifications coalescing take priority over queue group level.

How do you enable  Virtqueue level notifications coalescing? Why are
they different entities?
I don't think that we should have priorities, but the last command
should be the one that dictates the coalescing parameters.

For example, let's look at vq0 (RX):
Device receives VIRTIO_NET_CTRL_NOTF_COAL_RX_SET, vq0 should change
the parameters accordingly (all RX vqs should do the same).
Then device receives VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET with vqn = 0,
vq0 changes the parameters accordingly (all RX vqs are still using the
"old" parameters)
Then device receives VIRTIO_NET_CTRL_NOTF_COAL_RX_SET, vq0 changes the
parameters accordingly (all RX vqs should do the same).

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/

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

[Alvaro Karsz]

> > Please add short description something like,
> >
> > When the driver prefers to use per virtqueue notifications coalescing, and if queue group (transmit or receive) level notification coalescing is enabled, driver SHOULD first disable device level notification coalescing.
> > Or it should be,
> >
>
> I disagree here.
> IMO "queue group level notification coalescing" is not something to
> enable or disable, but a shortcut to set all TX/RX queues at once.
> Why should the spec force a driver to "disable device level
> notification coalescing" (I assume you mean send a
> VIRTIO_NET_CTRL_NOTF_COAL_[T/R]X_SET command with zeros)?
> What if the driver sends a VIRTIO_NET_CTRL_NOTF_COAL_[T/R]X_SET
> command, and then a single queue traffic increases? why should it zero
> the parameters to all other queues?
> I think that this should be discussed in the driver implementation
> stage, not in the spec.
>
> > Virtqueue level notifications coalescing, and device level notifications can be enabled together.
> > When both of them are enabled, per virtqueue notifications coalescing take priority over queue group level.
>
> How do you enable  Virtqueue level notifications coalescing? Why are
> they different entities?
> I don't think that we should have priorities, but the last command
> should be the one that dictates the coalescing parameters.
>
> For example, let's look at vq0 (RX):
> Device receives VIRTIO_NET_CTRL_NOTF_COAL_RX_SET, vq0 should change
> the parameters accordingly (all RX vqs should do the same).
> Then device receives VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET with vqn = 0,
> vq0 changes the parameters accordingly (all RX vqs are still using the
> "old" parameters)

Opps, I missed a word in here, it should have been:
Then device receives VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET with vqn = 0,
vq0 changes the parameters accordingly (all RX vqs EXCEPT FOR vq0
still use the previous parameters)

This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

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

[Heng Qi]

在 2023/2/11 下午4:45, Alvaro Karsz 写道:
>> Please add short description something like,
>>
>> When the driver prefers to use per virtqueue notifications coalescing, and if queue group (transmit or receive) level notification coalescing is enabled, driver SHOULD first disable device level notification coalescing.
>> Or it should be,
>>
> I disagree here.
> IMO "queue group level notification coalescing" is not something to
> enable or disable, but a shortcut to set all TX/RX queues at once.
> Why should the spec force a driver to "disable device level
> notification coalescing" (I assume you mean send a
> VIRTIO_NET_CTRL_NOTF_COAL_[T/R]X_SET command with zeros)?
> What if the driver sends a VIRTIO_NET_CTRL_NOTF_COAL_[T/R]X_SET
> command, and then a single queue traffic increases? why should it zero
> the parameters to all other queues?

Hi, Alvaro! Thanks for your reply!

I think Parav refers more to the scene where ethool sets parameters at 
the queue group level and
the scene where netdim sets parameters for a single queue. In this 
scenario, netdim should really
determine the coalescing parameters of the device, and the parameters at 
the queue group level set
by ethtool should be ignored (many drivers are designed this way, such 
as mlx), that is, we need to give netdim the right,
because it It has the ability to dynamically adjust parameters. 
(However, I think this friendly constraint is also possible in driver 
implementation.)

Of course, if we consider setting coalescing parameters at the queue 
group level and single queue level separately through ethtool,
then as you said, we should not set any priority for them.

Back to reality, I think the function of ethtool to set single queue 
parameters may come later, which is thankless for users because of netdim.

Therefore, if our specification tends to be practical, we can add 
Parav's proposal, and if our specification tends to be more general, 
then hand over
the constraints to the driver implementation. what do you think?

> I think that this should be discussed in the driver implementation
> stage, not in the spec.
>
>> Virtqueue level notifications coalescing, and device level notifications can be enabled together.
>> When both of them are enabled, per virtqueue notifications coalescing take priority over queue group level.
> How do you enable  Virtqueue level notifications coalescing? Why are
> they different entities?
> I don't think that we should have priorities, but the last command
> should be the one that dictates the coalescing parameters.
>
> For example, let's look at vq0 (RX):
> Device receives VIRTIO_NET_CTRL_NOTF_COAL_RX_SET, vq0 should change
> the parameters accordingly (all RX vqs should do the same).
> Then device receives VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET with vqn = 0,
> vq0 changes the parameters accordingly (all RX vqs are still using the
> "old" parameters)
> Then device receives VIRTIO_NET_CTRL_NOTF_COAL_RX_SET, vq0 changes the
> parameters accordingly (all RX vqs should do the same).

Yes I see what you mean, thanks for the clear example. This should be 
the second scenario I described above,
let's discuss how to solve it in the above reply.

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/


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

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

[Alvaro Karsz]

> >> Please add short description something like,
> >>
> >> When the driver prefers to use per virtqueue notifications coalescing, and if queue group (transmit or receive) level notification coalescing is enabled, driver SHOULD first disable device level notification coalescing.
> >> Or it should be,
> >>
> > I disagree here.
> > IMO "queue group level notification coalescing" is not something to
> > enable or disable, but a shortcut to set all TX/RX queues at once.
> > Why should the spec force a driver to "disable device level
> > notification coalescing" (I assume you mean send a
> > VIRTIO_NET_CTRL_NOTF_COAL_[T/R]X_SET command with zeros)?
> > What if the driver sends a VIRTIO_NET_CTRL_NOTF_COAL_[T/R]X_SET
> > command, and then a single queue traffic increases? why should it zero
> > the parameters to all other queues?
>
> Hi, Alvaro! Thanks for your reply!
>
> I think Parav refers more to the scene where ethool sets parameters at
> the queue group level and
> the scene where netdim sets parameters for a single queue. In this
> scenario, netdim should really
> determine the coalescing parameters of the device, and the parameters at
> the queue group level set
> by ethtool should be ignored (many drivers are designed this way, such
> as mlx), that is, we need to give netdim the right,
> because it It has the ability to dynamically adjust parameters.
> (However, I think this friendly constraint is also possible in driver
> implementation.)
>
> Of course, if we consider setting coalescing parameters at the queue
> group level and single queue level separately through ethtool,
> then as you said, we should not set any priority for them.
>
> Back to reality, I think the function of ethtool to set single queue
> parameters may come later, which is thankless for users because of netdim.
>
> Therefore, if our specification tends to be practical, we can add
> Parav's proposal, and if our specification tends to be more general,
> then hand over
> the constraints to the driver implementation. what do you think?

Hi,
I understand your and Parav's point, and that this is needed for netdim.
I just think that the spec should not dictate this, and this should be
implementation specific.
What if a OS with a different adaptive functionality wants to support
these features?

This is just my opinion though.

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/

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

[Parav Pandit]

> From: Alvaro Karsz <alvaro.karsz@solid-run.com>
> Sent: Saturday, February 11, 2023 3:45 AM
> 
> > Please add short description something like,
> >
> > When the driver prefers to use per virtqueue notifications coalescing, and if
> queue group (transmit or receive) level notification coalescing is enabled, driver
> SHOULD first disable device level notification coalescing.
> > Or it should be,
> >
> 
> I disagree here.
> IMO "queue group level notification coalescing" is not something to enable or
> disable, but a shortcut to set all TX/RX queues at once.
That short cut is the enable/disablement.

> Why should the spec force a driver to "disable device level notification
> coalescing" (I assume you mean send a
> VIRTIO_NET_CTRL_NOTF_COAL_[T/R]X_SET command with zeros)?
Yes. Because to have well defined behavior when sw configured both one after the another.

> What if the driver sends a VIRTIO_NET_CTRL_NOTF_COAL_[T/R]X_SET
> command, and then a single queue traffic increases? why should it zero the
> parameters to all other queues?
That is short transition when driver is switching over to per queue mode.
This is fine to have short glitch.

> I think that this should be discussed in the driver implementation stage, not in
> the spec.
> 
There should be a clear guidance on how device should behave when both per q and per device are configured.

> > Virtqueue level notifications coalescing, and device level notifications can be
> enabled together.
> > When both of them are enabled, per virtqueue notifications coalescing take
> priority over queue group level.
> 
> How do you enable  Virtqueue level notifications coalescing? Why are they
> different entities?
Using the new command that has vqn in it.

> I don't think that we should have priorities, but the last command should be the
> one that dictates the coalescing parameters.
> 
Priority is applicable when driver has issued both the commands. Per tx/rx, and per vqn.

> For example, let's look at vq0 (RX):
> Device receives VIRTIO_NET_CTRL_NOTF_COAL_RX_SET, vq0 should change
> the parameters accordingly (all RX vqs should do the same).
> Then device receives VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET with vqn = 0,
> vq0 changes the parameters accordingly (all RX vqs are still using the "old"
> parameters) Then device receives VIRTIO_NET_CTRL_NOTF_COAL_RX_SET, vq0
> changes the parameters accordingly (all RX vqs should do the same).
In this example, per VQ were overridden with per device.
Yes, so the last one is applicable, so priority of last one applies.

We continue to refuse to add the mode, and hence need to supply these description of both the sequence on how device should behave.

Sequence_1:
1. tx/rx group level
2. per vqn level
When #2 is done, VQ's whose per vq level is configured, follows vqn, rest of the VQs follow #1.

Sequence_2:
1. per vqn
2. tx/rx group level
When #2 is done, group level overrides the per vqn parameters. 

RE: [virtio-comment] Re: [PATCH v2] virtio-net: support the virtqueue coalescing moderation

[Parav Pandit]

> From: Alvaro Karsz <alvaro.karsz@solid-run.com>
> Sent: Saturday, February 11, 2023 5:19 AM

> > Therefore, if our specification tends to be practical, we can add
> > Parav's proposal, and if our specification tends to be more general,
> > then hand over the constraints to the driver implementation. what do
> > you think?
> 
> Hi,
> I understand your and Parav's point, and that this is needed for netdim.
> I just think that the spec should not dictate this, and this should be
> implementation specific.
> What if a OS with a different adaptive functionality wants to support these
> features?
> 
> This is just my opinion though.

We don’t dictate it in the spec. in the spec just defines 
a. the behavior for device should do when both are configured
One way to say, it doesn’t try to configure both.
Driver should disable one and switch to other.
Simple for driver, device implementation and spec as this is more common scenario.

Other way to say, spec wants the door to remain open for mixed mode here.
b. In this case the priority is defined as (last configured for a given object). So device is clear how to implement mixed mode.

#a is well known case. #b is more flexible.
Both spec definitions look good to me, we just got to describe it.

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

[Alvaro Karsz]

You write "both are configured", "try to configure both", "disable one
and switch to other" "mixed mode" etc..

But no modes/levels are mentioned in the patch, not once.

I totally agree that *if* we define 2 modes, we should have strict
rules on how to switch between modes and how to operate with both
modes.

But if we decide to have modes, your suggestion is not enough IMO, we
should explicitly define the modes.

In my POV, we should have no modes at all, everything is per queue,
and VIRTIO_NET_CTRL_NOTF_COAL_RX_SET is exactly the same as iterating
through the RX queues and issuing a VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET
command.

Every queue will use the last coalescing parameters it received from
the driver, regardless to the command used to deliver the parameters.

No modes at all.

I really don't see what we gain from having different operation modes.

I thought that we agreed not to have operation modes in v1.

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/

[virtio-dev] RE: [virtio-comment] Re: [PATCH v2] virtio-net: support the virtqueue coalescing moderation

[Parav Pandit]

> From: Alvaro Karsz <alvaro.karsz@solid-run.com>
> Sent: Saturday, February 11, 2023 10:47 AM
> 
> You write "both are configured", "try to configure both", "disable one and
> switch to other" "mixed mode" etc..
> 
> But no modes/levels are mentioned in the patch, not once.

using RX_SET and VQ_SET simultaneously implies "both, mixed mode, configured both".
Spec cannot be silent about it.
Did I miss the requirements section that describes the behavior of above two commands in combination?

> 
> I totally agree that *if* we define 2 modes, we should have strict rules on how
> to switch between modes and how to operate with both modes.
>
Even without any explicit mode, we need to define behavior of two commands invoked one after the other.
 
> But if we decide to have modes, your suggestion is not enough IMO, we should
> explicitly define the modes.
> 
> In my POV, we should have no modes at all, everything is per queue, and
> VIRTIO_NET_CTRL_NOTF_COAL_RX_SET is exactly the same as iterating
> through the RX queues and issuing a VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET
> command.
> 
> Every queue will use the last coalescing parameters it received from the driver,
> regardless to the command used to deliver the parameters.
>
This to be part of the spec. I don’t see it or I missed it?
 
> No modes at all.
> 
> I really don't see what we gain from having different operation modes.
> 
> I thought that we agreed not to have operation modes in v1.
We agreed to not have explicit mode, at condition to define the clear behavior on what should happen when both commands are used one after other.

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

[Alvaro Karsz]

I think that I wasn't clear enough.

I'm not saying that we should not define in the spec how to handle a
situation when a device receives both  RX_SET and VQ_SET (or a driver
sends both).
I'm saying that I don't think that the driver should handle the
situation the way you described it:

> When the driver prefers to use per virtqueue notifications coalescing, and if queue group (transmit or receive) level notification coalescing is enabled, driver SHOULD first disable device level notification coalescing.
> Or it should be,
>
> Virtqueue level notifications coalescing, and device level notifications can be enabled together.
> When both of them are enabled, per virtqueue notifications coalescing take priority over queue group level.

This implies that we have 2 modes and have priorities.

I think that if we want to refer to this situation in the spec, it
should be something like:
"A Device should use the last coalescing parameters received for a
virtqueue, regardless of the command used to deliver the parameters."
(just an example to make the point).

This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

RE: [virtio-comment] Re: [PATCH v2] virtio-net: support the virtqueue coalescing moderation

[Parav Pandit]

> From: Alvaro Karsz <alvaro.karsz@solid-run.com>
> Sent: Saturday, February 11, 2023 11:14 AM
> 
> I think that I wasn't clear enough.
> 
> I'm not saying that we should not define in the spec how to handle a situation
> when a device receives both  RX_SET and VQ_SET (or a driver sends both).
> I'm saying that I don't think that the driver should handle the situation the way
> you described it:
>

> > When the driver prefers to use per virtqueue notifications coalescing, and if
> queue group (transmit or receive) level notification coalescing is enabled, driver
> SHOULD first disable device level notification coalescing.
> > Or it should be,
> >
> > Virtqueue level notifications coalescing, and device level notifications can be
> enabled together.
> > When both of them are enabled, per virtqueue notifications coalescing take
> priority over queue group level.
> 
These are the two options I proposed.
In second option missed the case of per vq followed by group level.
Which I further acked in [1] to have it as last configured.

> I think that if we want to refer to this situation in the spec, it should be
> something like:
> "A Device should use the last coalescing parameters received for a virtqueue,
> regardless of the command used to deliver the parameters."
> (just an example to make the point).

Yes, as acked in [1]. :)
We both agree on #b which you described in above example.

So things looks good. Lets wait for Heng to generate v3 covering above case.

[1] https://lists.oasis-open.org/archives/virtio-dev/202302/msg00227.html

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

[Michael S. Tsirkin]

On Sat, Feb 11, 2023 at 01:47:16PM +0000, Parav Pandit wrote:
> 
> 
> > From: Alvaro Karsz <alvaro.karsz@solid-run.com>
> > Sent: Saturday, February 11, 2023 3:45 AM
> > 
> > > Please add short description something like,
> > >
> > > When the driver prefers to use per virtqueue notifications coalescing, and if
> > queue group (transmit or receive) level notification coalescing is enabled, driver
> > SHOULD first disable device level notification coalescing.
> > > Or it should be,
> > >
> > 
> > I disagree here.
> > IMO "queue group level notification coalescing" is not something to enable or
> > disable, but a shortcut to set all TX/RX queues at once.
> That short cut is the enable/disablement.
> 
> > Why should the spec force a driver to "disable device level notification
> > coalescing" (I assume you mean send a
> > VIRTIO_NET_CTRL_NOTF_COAL_[T/R]X_SET command with zeros)?
> Yes. Because to have well defined behavior when sw configured both one after the another.
> 
> > What if the driver sends a VIRTIO_NET_CTRL_NOTF_COAL_[T/R]X_SET
> > command, and then a single queue traffic increases? why should it zero the
> > parameters to all other queues?
> That is short transition when driver is switching over to per queue mode.
> This is fine to have short glitch.
> 
> > I think that this should be discussed in the driver implementation stage, not in
> > the spec.
> > 
> There should be a clear guidance on how device should behave when both per q and per device are configured.
> 
> > > Virtqueue level notifications coalescing, and device level notifications can be
> > enabled together.
> > > When both of them are enabled, per virtqueue notifications coalescing take
> > priority over queue group level.
> > 
> > How do you enable  Virtqueue level notifications coalescing? Why are they
> > different entities?
> Using the new command that has vqn in it.
> 
> > I don't think that we should have priorities, but the last command should be the
> > one that dictates the coalescing parameters.
> > 
> Priority is applicable when driver has issued both the commands. Per tx/rx, and per vqn.
> 
> > For example, let's look at vq0 (RX):
> > Device receives VIRTIO_NET_CTRL_NOTF_COAL_RX_SET, vq0 should change
> > the parameters accordingly (all RX vqs should do the same).
> > Then device receives VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET with vqn = 0,
> > vq0 changes the parameters accordingly (all RX vqs are still using the "old"
> > parameters) Then device receives VIRTIO_NET_CTRL_NOTF_COAL_RX_SET, vq0
> > changes the parameters accordingly (all RX vqs should do the same).
> In this example, per VQ were overridden with per device.
> Yes, so the last one is applicable, so priority of last one applies.
> 
> We continue to refuse to add the mode, and hence need to supply these description of both the sequence on how device should behave.
> 
> Sequence_1:
> 1. tx/rx group level
> 2. per vqn level
> When #2 is done, VQ's whose per vq level is configured, follows vqn, rest of the VQs follow #1.
> 
> Sequence_2:
> 1. per vqn
> 2. tx/rx group level
> When #2 is done, group level overrides the per vqn parameters. 

Since there's apparently some room for misunderstanding, I think adding these examples can't hurt.
I would also be more specific and just use specific numbers in the
example, to avoid any ambiguity.

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

[virtio-dev] Re: [virtio-comment] Re: [PATCH v2] virtio-net: support the virtqueue coalescing moderation

[Michael S. Tsirkin]

On Sat, Feb 11, 2023 at 12:18:45PM +0200, Alvaro Karsz wrote:
> > >> Please add short description something like,
> > >>
> > >> When the driver prefers to use per virtqueue notifications coalescing, and if queue group (transmit or receive) level notification coalescing is enabled, driver SHOULD first disable device level notification coalescing.
> > >> Or it should be,
> > >>
> > > I disagree here.
> > > IMO "queue group level notification coalescing" is not something to
> > > enable or disable, but a shortcut to set all TX/RX queues at once.
> > > Why should the spec force a driver to "disable device level
> > > notification coalescing" (I assume you mean send a
> > > VIRTIO_NET_CTRL_NOTF_COAL_[T/R]X_SET command with zeros)?
> > > What if the driver sends a VIRTIO_NET_CTRL_NOTF_COAL_[T/R]X_SET
> > > command, and then a single queue traffic increases? why should it zero
> > > the parameters to all other queues?
> >
> > Hi, Alvaro! Thanks for your reply!
> >
> > I think Parav refers more to the scene where ethool sets parameters at
> > the queue group level and
> > the scene where netdim sets parameters for a single queue. In this
> > scenario, netdim should really
> > determine the coalescing parameters of the device, and the parameters at
> > the queue group level set
> > by ethtool should be ignored (many drivers are designed this way, such
> > as mlx), that is, we need to give netdim the right,
> > because it It has the ability to dynamically adjust parameters.
> > (However, I think this friendly constraint is also possible in driver
> > implementation.)
> >
> > Of course, if we consider setting coalescing parameters at the queue
> > group level and single queue level separately through ethtool,
> > then as you said, we should not set any priority for them.
> >
> > Back to reality, I think the function of ethtool to set single queue
> > parameters may come later, which is thankless for users because of netdim.
> >
> > Therefore, if our specification tends to be practical, we can add
> > Parav's proposal, and if our specification tends to be more general,
> > then hand over
> > the constraints to the driver implementation. what do you think?
> 
> Hi,
> I understand your and Parav's point, and that this is needed for netdim.
> I just think that the spec should not dictate this, and this should be
> implementation specific.
> What if a OS with a different adaptive functionality wants to support
> these features?
> 
> This is just my opinion though.

I'm not sure the specific property is even a feature and not a bug.
One can argue e.g. that ethtool should fail with EBUSY rather
than being silently overwritten.
Thats why I feel this kind of policy is best set by driver.

-- 
MST


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

[virtio-dev] Re: [PATCH v2] virtio-net: support the virtqueue coalescing moderation

[Michael S. Tsirkin]

On Fri, Feb 10, 2023 at 07:36:03PM +0000, Parav Pandit wrote:
> 
> 
> > From: Heng Qi <hengqi@linux.alibaba.com>
> > Sent: Friday, February 10, 2023 2:02 AM
> > 
> > Currently, the coalescing profile is directly applied to all queues.
> 
> Say it,
> Currently coalescing parameters are grouped for all transmit and receive virtqueues.
> 
> > This patch supports setting or getting the parameters for a specified queue, and
> > a typical application of this function is NetDIM.
> Many of us know the net dim.
> But if you prefer to mention it here, better to have the link for it.
> 
> Please add pointer to it.
> [1] https://docs.kernel.org/networking/net_dim.html
> 
> > 
> > When the traffic between queues is unbalanced, for example, one queue is
> > busy and another queue is idle, then it will be very useful to control coalescing
> > parameters at the queue granularity.
> > 
> > 
> > +If additionally VIRTIO_NET_F_VQ_NOTF_COAL is negotiated, the driver can
> > +send control commands to set or get the coalescing parameters of a
> Control command singular?

why? driver can send any number of commands e.g. to different vqs.

> > +specified virtqueue (excluding the control virtqueue).
> > +
> > +\begin{lstlisting}
> > +struct virtio_net_ctrl_coal_vq {
> > +    le32 max_packets;
> > +    le32 usecs;
> > +    le16 vqn;
> > +};
> > +
> Change that Michael suggest restructuring and under same class looks good to me.
> 
> > +#define VIRTIO_NET_CTRL_NOTF_COAL_VQ 7
> > + #define VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET 0  #define
> > +VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET 1 \end{lstlisting}
> > +
> > +Virtqueue coalescing parameters:
> > +\begin{itemize}
> > +\item \field{max_packets}: The maximum number of packets sent/received by
> > the
> > +    specified virtqueue before a TX/RX notification.
> > +\item \field{usecs}: The maximum number of TX/RX usecs that the specified
> > +    virtqueue delays a TX/RX notification.
> > +\item \field{vqn}: The virtqueue number of the specified virtqueue.
> > +\end{itemize}
> > +
> The virtqueue number of the enabled transmit or receive virtuqueue.
> This will simplify below description.
> 
> > +The range of \filed{vqn} is between 0 and 0xFFFF inclusive, $ \lfloor
> > +vqn / 2 \rfloor $ is the index of the corresponding receiveq, and
> > +$\lfloor (vqn / 2) + 1 \rfloor $ is the corresponding tranmitq.
> > +
> 
> Please add short description something like,
> 
> When the driver prefers to use per virtqueue notifications coalescing, and if queue group (transmit or receive) level notification coalescing is enabled, driver SHOULD first disable device level notification coalescing.
> Or it should be,
> 
> Virtqueue level notifications coalescing, and device level notifications can be enabled together.
> When both of them are enabled, per virtqueue notifications coalescing take priority over queue group level.

Note that neither of these reflects what I proposed.
I proposed explaining that VIRTIO_NET_CTRL_NOTF_COAL_TX_SET and
VIRTIO_NET_CTRL_NOTF_COAL_RX_SET have the same effect as
repeatedly calling VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET for all TX/RX vqs.

> With rests of the comments from Michael and Alvaro in progress, looks good.


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

[virtio-comment] Re: [virtio-dev] Re: [virtio-comment] Re: [PATCH v2] virtio-net: support the virtqueue coalescing moderation

[Michael S. Tsirkin]

On Fri, Feb 10, 2023 at 07:19:49PM +0800, Heng Qi wrote:
> 
> 
> 在 2023/2/10 下午6:29, Michael S. Tsirkin 写道:
> > On Fri, Feb 10, 2023 at 05:53:40PM +0800, Heng Qi wrote:
> > > 
> > > 在 2023/2/10 下午4:38, Michael S. Tsirkin 写道:
> > > > On Fri, Feb 10, 2023 at 03:01:30PM +0800, Heng Qi wrote:
> > > > > Currently, the coalescing profile is directly applied to all queues.
> > > > > This patch supports setting or getting the parameters for a specified queue,
> > > > > and a typical application of this function is NetDIM.
> > > > > 
> > > > > When the traffic between queues is unbalanced, for example, one queue
> > > > > is busy and another queue is idle, then it will be very useful to
> > > > > control coalescing parameters at the queue granularity.
> > > > > 
> > > > > Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
> > > > > Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > > > I like this generally, thanks! Language needs to be tightened a bit.
> > > > > ---
> > > > > 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 structres 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
> > > > > 
> > > > >    content.tex | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
> > > > >    1 file changed, 68 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/content.tex b/content.tex
> > > > > index e863709..2c497e1 100644
> > > > > --- a/content.tex
> > > > > +++ b/content.tex
> > > > > @@ -3084,6 +3084,9 @@ \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 the virtqueue
> > > > > +    notifications coalescing.
> > > > > +
> > > > >    \item[VIRTIO_NET_F_NOTF_COAL(53)] Device supports notifications coalescing.
> > > > >    \item[VIRTIO_NET_F_GUEST_USO4 (54)] Driver can receive USOv4 packets.
> > > > > @@ -3140,6 +3143,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 and VIRTIO_NET_F_NOTF_COAL.
> > > > >    \end{description}
> > > > >    \subsubsection{Legacy Interface: Feature bits}\label{sec:Device Types / Network Device / Feature bits / Legacy Interface: Feature bits}
> > > > > @@ -4520,6 +4524,49 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
> > > > >    \item VIRTIO_NET_CTRL_NOTF_COAL_RX_SET: set the \field{rx_usecs} and \field{rx_max_packets} parameters.
> > > > >    \end{enumerate}
> > > > > +If additionally VIRTIO_NET_F_VQ_NOTF_COAL is negotiated, the driver can send
> > > > > +control commands to set or get the coalescing parameters of a specified
> > > > > +virtqueue (excluding the control virtqueue).
> > > > > +
> > > > > +\begin{lstlisting}
> > > > > +struct virtio_net_ctrl_coal_vq {
> > > > > +    le32 max_packets;
> > > > > +    le32 usecs;
> > > > > +    le16 vqn;
> > > > Add le16 reserved here, so keep things aligned naturally.
> > > > In fact if you want to support GET you need to re-order things
> > > > since write buffers come before read buffers:
> > > I see, thanks for pointing it out.
> > > 
> > > >    +    le16 vqn;
> > > >    +    le16 reserved;
> > > > 
> > > >    +    le32 max_packets;
> > > >    +    le32 usecs;
> > > > 
> > > > see below for more explanation.
> > > > 
> > > > 
> > > > 
> > > > > +};
> > > > > +
> > > > > +#define VIRTIO_NET_CTRL_NOTF_COAL_VQ 7
> > > > > + #define VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET 0
> > > > > + #define VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET 1
> > > > > +\end{lstlisting}
> > > > > +
> > > > > +Virtqueue coalescing parameters:
> > > > > +\begin{itemize}
> > > > > +\item \field{max_packets}: The maximum number of packets sent/received by the
> > > > > +    specified virtqueue before a TX/RX notification.
> > > > > +\item \field{usecs}: The maximum number of TX/RX usecs that the specified
> > > > > +    virtqueue delays a TX/RX notification.
> > > > > +\item \field{vqn}: The virtqueue number of the specified virtqueue.
> > > > > +\end{itemize}
> > > > > +
> > > > > +The range of \filed{vqn} is between 0 and 0xFFFF inclusive,
> > > > No really because last vq is a cvq. Maybe just drop this?
> > > In fact I have ruled out the control virtqueue.
> > > 
> > > Its virtqueue number should be 0x10000.
> > Nope, vq numberes are 16 bit.
> 
> Yes I know. The [0, 0xFFFF] listed in v2 is the maximum range of virtqueue
> numbers (including all receiveqs and all transmitqs)
> that can be set by the driver, because the current spec says
> "The device MUST set \field{max_virtqueue_pairs} to between 1 and 0x8000
> inclusive, if it offers VIRTIO_NET_F_MQ.".

Oh that looks like a bug since

	 \item[VIRTIO_NET_F_MQ] Requires VIRTIO_NET_F_CTRL_VQ. 

so max_virtqueue_pairs 0x8000 is not legal.


> So assuming that the maximum number of receiveqs and transmitqs are 0x8000
> respectively,
> then the total is 0x10000 (the queue number to the control queue is
> 0x10001), that is, the vqn indexed from 0 is [0,0x10000-1]=[0,0xFFFF ],
> which already excludes controlq.
> 
> But I know that you mean to emphasize in the specification that no contro
> queue is included.

I would just drop The range of \filed{vqn} is between 0 and 0xFFFF
inclusive - it is a 16 bit value there is really no point to
say any more.

> > 
> > > > > $ \lfloor vqn / 2 \rfloor $
> > > > > +is the index of the corresponding receiveq, and $\lfloor (vqn / 2) + 1 \rfloor $ is
> > > > > +the corresponding tranmitq.
> > > > > +
> > > > > +The VIRTIO_NET_CTRL_NOTF_COAL_RX_SET command is the same as calling VIRTIO_NET_CTRL_NOTF_COAL_VQ
> > > > you don't "call" commands. driver sends them, device receives them.
> > > > But here you are talking about a command abstracty so I'd just drop
> > > > a verb, or maybe "repeating".
> > > > And "same" is inexact in that it's not the same - takes more time - it
> > > > just has the same effect, or is equivalent to.
> > > There is indeed a gerund match here, I'll fix that.
> > > 
> > > > 
> > > > > +for virtqueues corresponding to all receiveqs.
> > > > receiveqs is confusing.
> > > > 
> > > > Also elsewhere we use the language receiveq1\ldots receiveqn
> > > > which seems clearer. Also you can not call VIRTIO_NET_CTRL_NOTF_COAL_VQ
> > > > for all vqs - it applies to one vq only. You mean each.
> > > I express something wrong, but what I mean is to send for each virtqueue.
> > > 
> > > > And virtqueues do not correspond to receiveqs - they
> > > > are receiveqs. If you like vqn corresponds to them.
> > > This is ambiguous, the number of virtqueues is 2*N+1, the number of receive
> > > queues and transmit queues is N,
> > > and there is also a control queue. Is this a problem?
> > > But I know what you mean it's better to use "same as
> > > VIRTIO_NET_CTRL_NOTF_COAL_VQ repeated for each of receiveq1\ldots receiveqn"
> > > 
> > > > Or better just "same as VIRTIO_NET_CTRL_NOTF_COAL_VQ repeated for
> > > > each of receiveq1\ldots receiveqn"
> > > Sure.
> > > 
> > > > > +
> > > > > +The VIRTIO_NET_CTRL_NOTF_COAL_TX_SET command is the same as calling VIRTIO_NET_CTRL_NOTF_COAL_VQ
> > > > > +for virtqueues corresponding to all transmitqs.
> > > > > +
> > > > > +Virtqueue coalescing will be disabled if all parameters are set to 0.
> > > > In fact, either usecs 0 or max_packets disables coalescing, no?
> > > Yes. I'll fix this.
> > > 
> > > > > +
> > > > > +The class VIRTIO_NET_CTRL_NOTF_COAL_VQ has 2 commands:
> > > > > +\begin{enumerate}
> > > > > +\item VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET: set the \field{max_packets}, \field{usecs} and \filed{vqn} parameters.
> > > > > +\item VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET: get the \field{max_packets}, \field{usecs} and \field{vqn} parameters.
> > > > How does VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET work then? For which vq?
> > > > I think you mean vqn is specified and you get back max_packets and
> > > > usecs. All this needs to be documented fully for each command.
> > > Yes, VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET gets the parameters of the virtqueue
> > > corresponding to vqn each time.
> > > 
> > > > I would also think it's a good idea to mention that VQ_GET does not have
> > > > to return exactly the same parameters - since it's best effort anyway,
> > > This is confusing, I think the device does not have to set the same
> > > parameters for VQ_SET, but for VQ_GET, the device should return to the
> > > driver every time.
> > What I mean is that if you call VQ_SET with a value of 17,
> > device might decide to e.g. store just the power of 2
> 
> Oh! I misunderstood what you meant before, I will add an appropriate
> reminder!
> 
> > 
> > 
> > > > it's ok for device to round down and store a smaller value for either
> > > > max_packets or usecs, e.g. to save space.
> > > > 
> > > > This kind of formalizes the best effort thing we discussed
> > > > previously.
> > > > 
> > > > 
> > > > Maybe make VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET a separate patch -
> > > > in the series,
> > > No problem, I'll try to describe it as best I can.
> > > 
> > > > at this point you did not make any effort to document it,
> > > > it needs more work.
> > > > 
> > > > 
> > > > > +\end{enumerate}
> > > > > +
> > > > >    \subparagraph{RX Notifications}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Notifications Coalescing / RX Notifications}
> > > > >    If, for example:
> > > > > @@ -4535,6 +4582,15 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
> > > > >    \item If the notifications are not suppressed by the driver, the device will send an used buffer notification, otherwise, the device will not send an used buffer notification as long as the notifications are suppressed.
> > > > >    \end{itemize}
> > > > > +If, example of setting coalescing parameters for a receive virtqueue:
> > > > "If" without "then" is confusing. I'd just start with "Example".
> > > Ok.
> > > 
> > > > > +\begin{itemize}
> > > > > +\item \field{max_packets} = 15.
> > > > > +\item \field{usecs} = 10.
> > > > > +\item \field{vqn} = 0;
> > > > why . above and ; here? I would just drop punctuation.
> > > It's a typo and I'll fix it.
> > > 
> > > > > +\end{itemize}
> > > > > +
> > > > > +The device will only operate on recieveq1 as VIRTIO_NET_CTRL_NOTF_COAL_RX_SET.
> > > > This really does not explain anything. Please just document exactly what
> > > > it does and does not do.
> > > Ok.
> > > 
> > > > 
> > > > > +
> > > > >    \subparagraph{TX Notifications}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Notifications Coalescing / TX Notifications}
> > > > >    If, for example:
> > > > > @@ -4550,13 +4606,24 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
> > > > >    \item If the notifications are not suppressed by the driver, the device will send an used buffer notification, otherwise, the device will not send an used buffer notification as long as the notifications are suppressed.
> > > > >    \end{itemize}
> > > > > +If, example of setting coalescing parameters for a transmit virtqueue:
> > > > > +\begin{itemize}
> > > > > +\item \field{max_packets} = 15.
> > > > > +\item \field{usecs} = 10.
> > > > > +\item \field{vqn} = 1;
> > > > > +\end{itemize}
> > > > > +
> > > > > +The device will only operate on transmitq1 as VIRTIO_NET_CTRL_NOTF_COAL_TX_SET.
> > > > > +
> > > > I feel it's the other way around. Document
> > > > 
> > > Why? I'll add documentation, but read on below.
> > > 
> > > > >    \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 the VIRTIO_NET_F_VQ_NOTF_COAL feature has not been negotiated, the driver MUST NOT issue VIRTIO_NET_CTRL_NOFT_COAL_VQ commands.
> > > > Don't we want to limit driver to legal values of vqn?
> > > > 
> > > Yes, I will add.
> > > 
> > > > >    \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 the VIRTIO_NET_CTRL_NOTF_COAL commands with VIRTIO_NET_ERR if it was not able to change the parameters or
> > > > > +was not able to find a virtqueue using the \field{vqn}.
> > > > First please create multiple statements not a single long one.
> > > > Second vqn only exists for per vq commands so create a statement
> > > > just for that.
> > > Ok, reasonable.
> > > 
> > > > Also more explicit please. What does this mean? I suspect that vq was not
> > > > enabled?
> > > Indicates that a vqn cannot be mapped to the corresponding virtqueue.
> > Still no idea. what is this mapping you are talking about.
> > Why can't it be mapped? what is corresponding to what?
> > 
> > vq with this number is not enabled or vqn >= 2max_virtqueue_pairs are
> 
> I am referring to the former.
> 
> > the only reasons i could come up with.  if that's it just say so. if not
> > list the actual reasons.
> 
> Sorry, I will explain more clearly later.
> 
> Thanks.
> 
> > 
> > > > Also, we MUST have vqn < max_virtqueue_pairs.
> > > Here should be vq < max_virtqueue_pairs * 2?
> > > 
> > > Thanks.
> > yea.
> > 
> > > > 
> > > > >    A device SHOULD NOT send used buffer notifications to the driver, if the notifications are suppressed as explained in \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Used Buffer Notification Suppression}, even if the coalescing counters expired.
> > > > > -- 
> > > > > 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


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

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

[Alvaro Karsz]

> I'm not sure the specific property is even a feature and not a bug.
> One can argue e.g. that ethtool should fail with EBUSY rather
> than being silently overwritten.
> Thats why I feel this kind of policy is best set by driver.

When you write "set by driver", you mean that [X] the spec needs to
define the driver behavior in this case, or that [Y] every driver
should decide what's best?

I'll answer to [X], if you meant [Y] the following part is not relevant.

If we talk implementation here, I would probably implement it like that:

net_dim_start_tx:
        set_ethtool_blocking_bool
        save_current_coalescing_params  # Maybe?
        clear_all_tx_coalescing_params     # NOTF_COAL_TX_SET 0
        do_net_dim

net_dim_stop_tx:
      restore_prev_ethtool_coalesing_params
      clear_ethtool_blocking_bool


It's reasonable to assume that sending a ethtool set coalesce when
netdim is operating will affect the performance.

But, windows' virtio_net driver may want to use _CTRL_NOTF_COAL_TX_SET
in its adaptive algorithm, and may have no userspace tool to configure
these parameters.
If we force the driver to clear/block some commands while using
others, we may do more harm than good in windows' case.

This is just my personal perspective.

I think that this is an important feature either way.

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/

[virtio-dev] Re: [virtio-comment] Re: [PATCH v2] virtio-net: support the virtqueue coalescing moderation

[Michael S. Tsirkin]

On Sun, Feb 12, 2023 at 10:53:50PM +0200, Alvaro Karsz wrote:
> > I'm not sure the specific property is even a feature and not a bug.
> > One can argue e.g. that ethtool should fail with EBUSY rather
> > than being silently overwritten.
> > Thats why I feel this kind of policy is best set by driver.
> 
> When you write "set by driver", you mean that [X] the spec needs to
> define the driver behavior in this case, or that [Y] every driver
> should decide what's best?
> I'll answer to [X], if you meant [Y] the following part is not relevant.

Y.

> If we talk implementation here, I would probably implement it like that:
> 
> net_dim_start_tx:
>         set_ethtool_blocking_bool
>         save_current_coalescing_params  # Maybe?
>         clear_all_tx_coalescing_params     # NOTF_COAL_TX_SET 0
>         do_net_dim
> 
> net_dim_stop_tx:
>       restore_prev_ethtool_coalesing_params
>       clear_ethtool_blocking_bool
> 
> It's reasonable to assume that sending a ethtool set coalesce when
> netdim is operating will affect the performance.
> 
> But, windows' virtio_net driver may want to use _CTRL_NOTF_COAL_TX_SET
> in its adaptive algorithm, and may have no userspace tool to configure
> these parameters.
> If we force the driver to clear/block some commands while using
> others, we may do more harm than good in windows' case.
> 
> This is just my personal perspective.
> 
> I think that this is an important feature either way.


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

[virtio-comment] Re: [virtio-dev] Re: [virtio-comment] Re: [PATCH v2] virtio-net: support the virtqueue coalescing moderation

[Heng Qi]

在 2023/2/13 上午4:47, Michael S. Tsirkin 写道:
> On Fri, Feb 10, 2023 at 07:19:49PM +0800, Heng Qi wrote:
>>
>> 在 2023/2/10 下午6:29, Michael S. Tsirkin 写道:
>>> On Fri, Feb 10, 2023 at 05:53:40PM +0800, Heng Qi wrote:
>>>> 在 2023/2/10 下午4:38, Michael S. Tsirkin 写道:
>>>>> On Fri, Feb 10, 2023 at 03:01:30PM +0800, Heng Qi wrote:
>>>>>> Currently, the coalescing profile is directly applied to all queues.
>>>>>> This patch supports setting or getting the parameters for a specified queue,
>>>>>> and a typical application of this function is NetDIM.
>>>>>>
>>>>>> When the traffic between queues is unbalanced, for example, one queue
>>>>>> is busy and another queue is idle, then it will be very useful to
>>>>>> control coalescing parameters at the queue granularity.
>>>>>>
>>>>>> Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
>>>>>> Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
>>>>> I like this generally, thanks! Language needs to be tightened a bit.
>>>>>> ---
>>>>>> 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 structres 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
>>>>>>
>>>>>>     content.tex | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
>>>>>>     1 file changed, 68 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/content.tex b/content.tex
>>>>>> index e863709..2c497e1 100644
>>>>>> --- a/content.tex
>>>>>> +++ b/content.tex
>>>>>> @@ -3084,6 +3084,9 @@ \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 the virtqueue
>>>>>> +    notifications coalescing.
>>>>>> +
>>>>>>     \item[VIRTIO_NET_F_NOTF_COAL(53)] Device supports notifications coalescing.
>>>>>>     \item[VIRTIO_NET_F_GUEST_USO4 (54)] Driver can receive USOv4 packets.
>>>>>> @@ -3140,6 +3143,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 and VIRTIO_NET_F_NOTF_COAL.
>>>>>>     \end{description}
>>>>>>     \subsubsection{Legacy Interface: Feature bits}\label{sec:Device Types / Network Device / Feature bits / Legacy Interface: Feature bits}
>>>>>> @@ -4520,6 +4524,49 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
>>>>>>     \item VIRTIO_NET_CTRL_NOTF_COAL_RX_SET: set the \field{rx_usecs} and \field{rx_max_packets} parameters.
>>>>>>     \end{enumerate}
>>>>>> +If additionally VIRTIO_NET_F_VQ_NOTF_COAL is negotiated, the driver can send
>>>>>> +control commands to set or get the coalescing parameters of a specified
>>>>>> +virtqueue (excluding the control virtqueue).
>>>>>> +
>>>>>> +\begin{lstlisting}
>>>>>> +struct virtio_net_ctrl_coal_vq {
>>>>>> +    le32 max_packets;
>>>>>> +    le32 usecs;
>>>>>> +    le16 vqn;
>>>>> Add le16 reserved here, so keep things aligned naturally.
>>>>> In fact if you want to support GET you need to re-order things
>>>>> since write buffers come before read buffers:
>>>> I see, thanks for pointing it out.
>>>>
>>>>>     +    le16 vqn;
>>>>>     +    le16 reserved;
>>>>>
>>>>>     +    le32 max_packets;
>>>>>     +    le32 usecs;
>>>>>
>>>>> see below for more explanation.
>>>>>
>>>>>
>>>>>
>>>>>> +};
>>>>>> +
>>>>>> +#define VIRTIO_NET_CTRL_NOTF_COAL_VQ 7
>>>>>> + #define VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET 0
>>>>>> + #define VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET 1
>>>>>> +\end{lstlisting}
>>>>>> +
>>>>>> +Virtqueue coalescing parameters:
>>>>>> +\begin{itemize}
>>>>>> +\item \field{max_packets}: The maximum number of packets sent/received by the
>>>>>> +    specified virtqueue before a TX/RX notification.
>>>>>> +\item \field{usecs}: The maximum number of TX/RX usecs that the specified
>>>>>> +    virtqueue delays a TX/RX notification.
>>>>>> +\item \field{vqn}: The virtqueue number of the specified virtqueue.
>>>>>> +\end{itemize}
>>>>>> +
>>>>>> +The range of \filed{vqn} is between 0 and 0xFFFF inclusive,
>>>>> No really because last vq is a cvq. Maybe just drop this?
>>>> In fact I have ruled out the control virtqueue.
>>>>
>>>> Its virtqueue number should be 0x10000.
>>> Nope, vq numberes are 16 bit.
>> Yes I know. The [0, 0xFFFF] listed in v2 is the maximum range of virtqueue
>> numbers (including all receiveqs and all transmitqs)
>> that can be set by the driver, because the current spec says
>> "The device MUST set \field{max_virtqueue_pairs} to between 1 and 0x8000
>> inclusive, if it offers VIRTIO_NET_F_MQ.".
> Oh that looks like a bug since
>
> 	 \item[VIRTIO_NET_F_MQ] Requires VIRTIO_NET_F_CTRL_VQ.
>
> so max_virtqueue_pairs 0x8000 is not legal.

No, it's not a bug.
As described in the specification:
"This field specifies the maximum number of each of transmit and receive 
virtqueues (receiveq1\ldots receiveqN and transmitq1\ldots transmitqN 
respectively)
that can be configured once at least one of these features is negotiated."
\field{max_virtqueue_pairs} does not include control queue.

>
>
>> So assuming that the maximum number of receiveqs and transmitqs are 0x8000
>> respectively,
>> then the total is 0x10000 (the queue number to the control queue is
>> 0x10001), that is, the vqn indexed from 0 is [0,0x10000-1]=[0,0xFFFF ],
>> which already excludes controlq.
>>
>> But I know that you mean to emphasize in the specification that no contro
>> queue is included.
> I would just drop The range of \filed{vqn} is between 0 and 0xFFFF
> inclusive - it is a 16 bit value there is really no point to
> say any more.

Sure. :)

Thanks.

>
>>>>>> $ \lfloor vqn / 2 \rfloor $
>>>>>> +is the index of the corresponding receiveq, and $\lfloor (vqn / 2) + 1 \rfloor $ is
>>>>>> +the corresponding tranmitq.
>>>>>> +
>>>>>> +The VIRTIO_NET_CTRL_NOTF_COAL_RX_SET command is the same as calling VIRTIO_NET_CTRL_NOTF_COAL_VQ
>>>>> you don't "call" commands. driver sends them, device receives them.
>>>>> But here you are talking about a command abstracty so I'd just drop
>>>>> a verb, or maybe "repeating".
>>>>> And "same" is inexact in that it's not the same - takes more time - it
>>>>> just has the same effect, or is equivalent to.
>>>> There is indeed a gerund match here, I'll fix that.
>>>>
>>>>>> +for virtqueues corresponding to all receiveqs.
>>>>> receiveqs is confusing.
>>>>>
>>>>> Also elsewhere we use the language receiveq1\ldots receiveqn
>>>>> which seems clearer. Also you can not call VIRTIO_NET_CTRL_NOTF_COAL_VQ
>>>>> for all vqs - it applies to one vq only. You mean each.
>>>> I express something wrong, but what I mean is to send for each virtqueue.
>>>>
>>>>> And virtqueues do not correspond to receiveqs - they
>>>>> are receiveqs. If you like vqn corresponds to them.
>>>> This is ambiguous, the number of virtqueues is 2*N+1, the number of receive
>>>> queues and transmit queues is N,
>>>> and there is also a control queue. Is this a problem?
>>>> But I know what you mean it's better to use "same as
>>>> VIRTIO_NET_CTRL_NOTF_COAL_VQ repeated for each of receiveq1\ldots receiveqn"
>>>>
>>>>> Or better just "same as VIRTIO_NET_CTRL_NOTF_COAL_VQ repeated for
>>>>> each of receiveq1\ldots receiveqn"
>>>> Sure.
>>>>
>>>>>> +
>>>>>> +The VIRTIO_NET_CTRL_NOTF_COAL_TX_SET command is the same as calling VIRTIO_NET_CTRL_NOTF_COAL_VQ
>>>>>> +for virtqueues corresponding to all transmitqs.
>>>>>> +
>>>>>> +Virtqueue coalescing will be disabled if all parameters are set to 0.
>>>>> In fact, either usecs 0 or max_packets disables coalescing, no?
>>>> Yes. I'll fix this.
>>>>
>>>>>> +
>>>>>> +The class VIRTIO_NET_CTRL_NOTF_COAL_VQ has 2 commands:
>>>>>> +\begin{enumerate}
>>>>>> +\item VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET: set the \field{max_packets}, \field{usecs} and \filed{vqn} parameters.
>>>>>> +\item VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET: get the \field{max_packets}, \field{usecs} and \field{vqn} parameters.
>>>>> How does VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET work then? For which vq?
>>>>> I think you mean vqn is specified and you get back max_packets and
>>>>> usecs. All this needs to be documented fully for each command.
>>>> Yes, VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET gets the parameters of the virtqueue
>>>> corresponding to vqn each time.
>>>>
>>>>> I would also think it's a good idea to mention that VQ_GET does not have
>>>>> to return exactly the same parameters - since it's best effort anyway,
>>>> This is confusing, I think the device does not have to set the same
>>>> parameters for VQ_SET, but for VQ_GET, the device should return to the
>>>> driver every time.
>>> What I mean is that if you call VQ_SET with a value of 17,
>>> device might decide to e.g. store just the power of 2
>> Oh! I misunderstood what you meant before, I will add an appropriate
>> reminder!
>>
>>>
>>>>> it's ok for device to round down and store a smaller value for either
>>>>> max_packets or usecs, e.g. to save space.
>>>>>
>>>>> This kind of formalizes the best effort thing we discussed
>>>>> previously.
>>>>>
>>>>>
>>>>> Maybe make VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET a separate patch -
>>>>> in the series,
>>>> No problem, I'll try to describe it as best I can.
>>>>
>>>>> at this point you did not make any effort to document it,
>>>>> it needs more work.
>>>>>
>>>>>
>>>>>> +\end{enumerate}
>>>>>> +
>>>>>>     \subparagraph{RX Notifications}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Notifications Coalescing / RX Notifications}
>>>>>>     If, for example:
>>>>>> @@ -4535,6 +4582,15 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
>>>>>>     \item If the notifications are not suppressed by the driver, the device will send an used buffer notification, otherwise, the device will not send an used buffer notification as long as the notifications are suppressed.
>>>>>>     \end{itemize}
>>>>>> +If, example of setting coalescing parameters for a receive virtqueue:
>>>>> "If" without "then" is confusing. I'd just start with "Example".
>>>> Ok.
>>>>
>>>>>> +\begin{itemize}
>>>>>> +\item \field{max_packets} = 15.
>>>>>> +\item \field{usecs} = 10.
>>>>>> +\item \field{vqn} = 0;
>>>>> why . above and ; here? I would just drop punctuation.
>>>> It's a typo and I'll fix it.
>>>>
>>>>>> +\end{itemize}
>>>>>> +
>>>>>> +The device will only operate on recieveq1 as VIRTIO_NET_CTRL_NOTF_COAL_RX_SET.
>>>>> This really does not explain anything. Please just document exactly what
>>>>> it does and does not do.
>>>> Ok.
>>>>
>>>>>> +
>>>>>>     \subparagraph{TX Notifications}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Notifications Coalescing / TX Notifications}
>>>>>>     If, for example:
>>>>>> @@ -4550,13 +4606,24 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
>>>>>>     \item If the notifications are not suppressed by the driver, the device will send an used buffer notification, otherwise, the device will not send an used buffer notification as long as the notifications are suppressed.
>>>>>>     \end{itemize}
>>>>>> +If, example of setting coalescing parameters for a transmit virtqueue:
>>>>>> +\begin{itemize}
>>>>>> +\item \field{max_packets} = 15.
>>>>>> +\item \field{usecs} = 10.
>>>>>> +\item \field{vqn} = 1;
>>>>>> +\end{itemize}
>>>>>> +
>>>>>> +The device will only operate on transmitq1 as VIRTIO_NET_CTRL_NOTF_COAL_TX_SET.
>>>>>> +
>>>>> I feel it's the other way around. Document
>>>>>
>>>> Why? I'll add documentation, but read on below.
>>>>
>>>>>>     \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 the VIRTIO_NET_F_VQ_NOTF_COAL feature has not been negotiated, the driver MUST NOT issue VIRTIO_NET_CTRL_NOFT_COAL_VQ commands.
>>>>> Don't we want to limit driver to legal values of vqn?
>>>>>
>>>> Yes, I will add.
>>>>
>>>>>>     \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 the VIRTIO_NET_CTRL_NOTF_COAL commands with VIRTIO_NET_ERR if it was not able to change the parameters or
>>>>>> +was not able to find a virtqueue using the \field{vqn}.
>>>>> First please create multiple statements not a single long one.
>>>>> Second vqn only exists for per vq commands so create a statement
>>>>> just for that.
>>>> Ok, reasonable.
>>>>
>>>>> Also more explicit please. What does this mean? I suspect that vq was not
>>>>> enabled?
>>>> Indicates that a vqn cannot be mapped to the corresponding virtqueue.
>>> Still no idea. what is this mapping you are talking about.
>>> Why can't it be mapped? what is corresponding to what?
>>>
>>> vq with this number is not enabled or vqn >= 2max_virtqueue_pairs are
>> I am referring to the former.
>>
>>> the only reasons i could come up with.  if that's it just say so. if not
>>> list the actual reasons.
>> Sorry, I will explain more clearly later.
>>
>> Thanks.
>>
>>>>> Also, we MUST have vqn < max_virtqueue_pairs.
>>>> Here should be vq < max_virtqueue_pairs * 2?
>>>>
>>>> Thanks.
>>> yea.
>>>
>>>>>>     A device SHOULD NOT send used buffer notifications to the driver, if the notifications are suppressed as explained in \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Used Buffer Notification Suppression}, even if the coalescing counters expired.
>>>>>> -- 
>>>>>> 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


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/

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

[Heng Qi]

On Sat, Feb 11, 2023 at 01:47:16PM +0000, Parav Pandit wrote:
> 
> 
> > From: Alvaro Karsz <alvaro.karsz@solid-run.com>
> > Sent: Saturday, February 11, 2023 3:45 AM
> > 
> > > Please add short description something like,
> > >
> > > When the driver prefers to use per virtqueue notifications coalescing, and if
> > queue group (transmit or receive) level notification coalescing is enabled, driver
> > SHOULD first disable device level notification coalescing.
> > > Or it should be,
> > >
> > 
> > I disagree here.
> > IMO "queue group level notification coalescing" is not something to enable or
> > disable, but a shortcut to set all TX/RX queues at once.
> That short cut is the enable/disablement.
> 
> > Why should the spec force a driver to "disable device level notification
> > coalescing" (I assume you mean send a
> > VIRTIO_NET_CTRL_NOTF_COAL_[T/R]X_SET command with zeros)?
> Yes. Because to have well defined behavior when sw configured both one after the another.
> 
> > What if the driver sends a VIRTIO_NET_CTRL_NOTF_COAL_[T/R]X_SET
> > command, and then a single queue traffic increases? why should it zero the
> > parameters to all other queues?
> That is short transition when driver is switching over to per queue mode.
> This is fine to have short glitch.
> 
> > I think that this should be discussed in the driver implementation stage, not in
> > the spec.
> > 
> There should be a clear guidance on how device should behave when both per q and per device are configured.
> 
> > > Virtqueue level notifications coalescing, and device level notifications can be
> > enabled together.
> > > When both of them are enabled, per virtqueue notifications coalescing take
> > priority over queue group level.
> > 
> > How do you enable  Virtqueue level notifications coalescing? Why are they
> > different entities?
> Using the new command that has vqn in it.
> 
> > I don't think that we should have priorities, but the last command should be the
> > one that dictates the coalescing parameters.
> > 
> Priority is applicable when driver has issued both the commands. Per tx/rx, and per vqn.
> 
> > For example, let's look at vq0 (RX):
> > Device receives VIRTIO_NET_CTRL_NOTF_COAL_RX_SET, vq0 should change
> > the parameters accordingly (all RX vqs should do the same).
> > Then device receives VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET with vqn = 0,
> > vq0 changes the parameters accordingly (all RX vqs are still using the "old"
> > parameters) Then device receives VIRTIO_NET_CTRL_NOTF_COAL_RX_SET, vq0
> > changes the parameters accordingly (all RX vqs should do the same).
> In this example, per VQ were overridden with per device.
> Yes, so the last one is applicable, so priority of last one applies.
> 
> We continue to refuse to add the mode, and hence need to supply these description of both the sequence on how device should behave.
> 
> Sequence_1:
> 1. tx/rx group level
> 2. per vqn level
> When #2 is done, VQ's whose per vq level is configured, follows vqn, rest of the VQs follow #1.
> 
> Sequence_2:
> 1. per vqn
> 2. tx/rx group level
> When #2 is done, group level overrides the per vqn parameters. 

Adding examples of the two command sequences is due, I will do so in the next release. :)

Thanks.


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

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

[Heng Qi]

On Sat, Feb 11, 2023 at 06:13:55PM +0200, Alvaro Karsz wrote:
> I think that I wasn't clear enough.
> 
> I'm not saying that we should not define in the spec how to handle a
> situation when a device receives both  RX_SET and VQ_SET (or a driver
> sends both).
> I'm saying that I don't think that the driver should handle the
> situation the way you described it:
> 
> > When the driver prefers to use per virtqueue notifications coalescing, and if queue group (transmit or receive) level notification coalescing is enabled, driver SHOULD first disable device level notification coalescing.
> > Or it should be,
> >
> > Virtqueue level notifications coalescing, and device level notifications can be enabled together.
> > When both of them are enabled, per virtqueue notifications coalescing take priority over queue group level.
> 
> This implies that we have 2 modes and have priorities.
> 
> I think that if we want to refer to this situation in the spec, it
> should be something like:
> "A Device should use the last coalescing parameters received for a
> virtqueue, regardless of the command used to deliver the parameters."

Your suggestion is good, the per-device command and the per-queue command need some examples and behavior definitions,
I will add them to avoid some misunderstandings.

Thanks.

> (just an example to make the point).

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/

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

[Heng Qi]

On Sun, Feb 12, 2023 at 04:35:37AM -0500, Michael S. Tsirkin wrote:
> On Sat, Feb 11, 2023 at 01:47:16PM +0000, Parav Pandit wrote:
> > 
> > 
> > > From: Alvaro Karsz <alvaro.karsz@solid-run.com>
> > > Sent: Saturday, February 11, 2023 3:45 AM
> > > 
> > > > Please add short description something like,
> > > >
> > > > When the driver prefers to use per virtqueue notifications coalescing, and if
> > > queue group (transmit or receive) level notification coalescing is enabled, driver
> > > SHOULD first disable device level notification coalescing.
> > > > Or it should be,
> > > >
> > > 
> > > I disagree here.
> > > IMO "queue group level notification coalescing" is not something to enable or
> > > disable, but a shortcut to set all TX/RX queues at once.
> > That short cut is the enable/disablement.
> > 
> > > Why should the spec force a driver to "disable device level notification
> > > coalescing" (I assume you mean send a
> > > VIRTIO_NET_CTRL_NOTF_COAL_[T/R]X_SET command with zeros)?
> > Yes. Because to have well defined behavior when sw configured both one after the another.
> > 
> > > What if the driver sends a VIRTIO_NET_CTRL_NOTF_COAL_[T/R]X_SET
> > > command, and then a single queue traffic increases? why should it zero the
> > > parameters to all other queues?
> > That is short transition when driver is switching over to per queue mode.
> > This is fine to have short glitch.
> > 
> > > I think that this should be discussed in the driver implementation stage, not in
> > > the spec.
> > > 
> > There should be a clear guidance on how device should behave when both per q and per device are configured.
> > 
> > > > Virtqueue level notifications coalescing, and device level notifications can be
> > > enabled together.
> > > > When both of them are enabled, per virtqueue notifications coalescing take
> > > priority over queue group level.
> > > 
> > > How do you enable  Virtqueue level notifications coalescing? Why are they
> > > different entities?
> > Using the new command that has vqn in it.
> > 
> > > I don't think that we should have priorities, but the last command should be the
> > > one that dictates the coalescing parameters.
> > > 
> > Priority is applicable when driver has issued both the commands. Per tx/rx, and per vqn.
> > 
> > > For example, let's look at vq0 (RX):
> > > Device receives VIRTIO_NET_CTRL_NOTF_COAL_RX_SET, vq0 should change
> > > the parameters accordingly (all RX vqs should do the same).
> > > Then device receives VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET with vqn = 0,
> > > vq0 changes the parameters accordingly (all RX vqs are still using the "old"
> > > parameters) Then device receives VIRTIO_NET_CTRL_NOTF_COAL_RX_SET, vq0
> > > changes the parameters accordingly (all RX vqs should do the same).
> > In this example, per VQ were overridden with per device.
> > Yes, so the last one is applicable, so priority of last one applies.
> > 
> > We continue to refuse to add the mode, and hence need to supply these description of both the sequence on how device should behave.
> > 
> > Sequence_1:
> > 1. tx/rx group level
> > 2. per vqn level
> > When #2 is done, VQ's whose per vq level is configured, follows vqn, rest of the VQs follow #1.
> > 
> > Sequence_2:
> > 1. per vqn
> > 2. tx/rx group level
> > When #2 is done, group level overrides the per vqn parameters. 
> 
> Since there's apparently some room for misunderstanding, I think adding these examples can't hurt.

Ok, I'll handle this.

> I would also be more specific and just use specific numbers in the
> example, to avoid any ambiguity.

Agree.

Thanks.

> 
> -- 
> MST

This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

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

[Heng Qi]

On Sun, Feb 12, 2023 at 03:43:45PM -0500, Michael S. Tsirkin wrote:
> On Fri, Feb 10, 2023 at 07:36:03PM +0000, Parav Pandit wrote:
> > 
> > 
> > > From: Heng Qi <hengqi@linux.alibaba.com>
> > > Sent: Friday, February 10, 2023 2:02 AM
> > > 
> > > Currently, the coalescing profile is directly applied to all queues.
> > 
> > Say it,
> > Currently coalescing parameters are grouped for all transmit and receive virtqueues.
> > 
> > > This patch supports setting or getting the parameters for a specified queue, and
> > > a typical application of this function is NetDIM.
> > Many of us know the net dim.
> > But if you prefer to mention it here, better to have the link for it.
> > 
> > Please add pointer to it.
> > [1] https://docs.kernel.org/networking/net_dim.html
> > 
> > > 
> > > When the traffic between queues is unbalanced, for example, one queue is
> > > busy and another queue is idle, then it will be very useful to control coalescing
> > > parameters at the queue granularity.
> > > 
> > > 
> > > +If additionally VIRTIO_NET_F_VQ_NOTF_COAL is negotiated, the driver can
> > > +send control commands to set or get the coalescing parameters of a
> > Control command singular?
> 
> why? driver can send any number of commands e.g. to different vqs.

I think Parav pointed out a syntax error on my part.

a singular command --> a specified virtqueue.

Thanks.

> 
> > > +specified virtqueue (excluding the control virtqueue).
> > > +
> > > +\begin{lstlisting}
> > > +struct virtio_net_ctrl_coal_vq {
> > > +    le32 max_packets;
> > > +    le32 usecs;
> > > +    le16 vqn;
> > > +};
> > > +
> > Change that Michael suggest restructuring and under same class looks good to me.
> > 
> > > +#define VIRTIO_NET_CTRL_NOTF_COAL_VQ 7
> > > + #define VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET 0  #define
> > > +VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET 1 \end{lstlisting}
> > > +
> > > +Virtqueue coalescing parameters:
> > > +\begin{itemize}
> > > +\item \field{max_packets}: The maximum number of packets sent/received by
> > > the
> > > +    specified virtqueue before a TX/RX notification.
> > > +\item \field{usecs}: The maximum number of TX/RX usecs that the specified
> > > +    virtqueue delays a TX/RX notification.
> > > +\item \field{vqn}: The virtqueue number of the specified virtqueue.
> > > +\end{itemize}
> > > +
> > The virtqueue number of the enabled transmit or receive virtuqueue.
> > This will simplify below description.
> > 
> > > +The range of \filed{vqn} is between 0 and 0xFFFF inclusive, $ \lfloor
> > > +vqn / 2 \rfloor $ is the index of the corresponding receiveq, and
> > > +$\lfloor (vqn / 2) + 1 \rfloor $ is the corresponding tranmitq.
> > > +
> > 
> > Please add short description something like,
> > 
> > When the driver prefers to use per virtqueue notifications coalescing, and if queue group (transmit or receive) level notification coalescing is enabled, driver SHOULD first disable device level notification coalescing.
> > Or it should be,
> > 
> > Virtqueue level notifications coalescing, and device level notifications can be enabled together.
> > When both of them are enabled, per virtqueue notifications coalescing take priority over queue group level.
> 
> Note that neither of these reflects what I proposed.
> I proposed explaining that VIRTIO_NET_CTRL_NOTF_COAL_TX_SET and
> VIRTIO_NET_CTRL_NOTF_COAL_RX_SET have the same effect as
> repeatedly calling VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET for all TX/RX vqs.
> 
> > With rests of the comments from Michael and Alvaro in progress, looks good.
> 
> 
> 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/

[virtio-comment] Re: [virtio-dev] Re: [virtio-comment] Re: [PATCH v2] virtio-net: support the virtqueue coalescing moderation

[Michael S. Tsirkin]

On Mon, Feb 13, 2023 at 10:36:13AM +0800, Heng Qi wrote:
> 
> 
> 在 2023/2/13 上午4:47, Michael S. Tsirkin 写道:
> > On Fri, Feb 10, 2023 at 07:19:49PM +0800, Heng Qi wrote:
> > > 
> > > 在 2023/2/10 下午6:29, Michael S. Tsirkin 写道:
> > > > On Fri, Feb 10, 2023 at 05:53:40PM +0800, Heng Qi wrote:
> > > > > 在 2023/2/10 下午4:38, Michael S. Tsirkin 写道:
> > > > > > On Fri, Feb 10, 2023 at 03:01:30PM +0800, Heng Qi wrote:
> > > > > > > Currently, the coalescing profile is directly applied to all queues.
> > > > > > > This patch supports setting or getting the parameters for a specified queue,
> > > > > > > and a typical application of this function is NetDIM.
> > > > > > > 
> > > > > > > When the traffic between queues is unbalanced, for example, one queue
> > > > > > > is busy and another queue is idle, then it will be very useful to
> > > > > > > control coalescing parameters at the queue granularity.
> > > > > > > 
> > > > > > > Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
> > > > > > > Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > > > > > I like this generally, thanks! Language needs to be tightened a bit.
> > > > > > > ---
> > > > > > > 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 structres 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
> > > > > > > 
> > > > > > >     content.tex | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
> > > > > > >     1 file changed, 68 insertions(+), 1 deletion(-)
> > > > > > > 
> > > > > > > diff --git a/content.tex b/content.tex
> > > > > > > index e863709..2c497e1 100644
> > > > > > > --- a/content.tex
> > > > > > > +++ b/content.tex
> > > > > > > @@ -3084,6 +3084,9 @@ \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 the virtqueue
> > > > > > > +    notifications coalescing.
> > > > > > > +
> > > > > > >     \item[VIRTIO_NET_F_NOTF_COAL(53)] Device supports notifications coalescing.
> > > > > > >     \item[VIRTIO_NET_F_GUEST_USO4 (54)] Driver can receive USOv4 packets.
> > > > > > > @@ -3140,6 +3143,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 and VIRTIO_NET_F_NOTF_COAL.
> > > > > > >     \end{description}
> > > > > > >     \subsubsection{Legacy Interface: Feature bits}\label{sec:Device Types / Network Device / Feature bits / Legacy Interface: Feature bits}
> > > > > > > @@ -4520,6 +4524,49 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
> > > > > > >     \item VIRTIO_NET_CTRL_NOTF_COAL_RX_SET: set the \field{rx_usecs} and \field{rx_max_packets} parameters.
> > > > > > >     \end{enumerate}
> > > > > > > +If additionally VIRTIO_NET_F_VQ_NOTF_COAL is negotiated, the driver can send
> > > > > > > +control commands to set or get the coalescing parameters of a specified
> > > > > > > +virtqueue (excluding the control virtqueue).
> > > > > > > +
> > > > > > > +\begin{lstlisting}
> > > > > > > +struct virtio_net_ctrl_coal_vq {
> > > > > > > +    le32 max_packets;
> > > > > > > +    le32 usecs;
> > > > > > > +    le16 vqn;
> > > > > > Add le16 reserved here, so keep things aligned naturally.
> > > > > > In fact if you want to support GET you need to re-order things
> > > > > > since write buffers come before read buffers:
> > > > > I see, thanks for pointing it out.
> > > > > 
> > > > > >     +    le16 vqn;
> > > > > >     +    le16 reserved;
> > > > > > 
> > > > > >     +    le32 max_packets;
> > > > > >     +    le32 usecs;
> > > > > > 
> > > > > > see below for more explanation.
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > > +};
> > > > > > > +
> > > > > > > +#define VIRTIO_NET_CTRL_NOTF_COAL_VQ 7
> > > > > > > + #define VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET 0
> > > > > > > + #define VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET 1
> > > > > > > +\end{lstlisting}
> > > > > > > +
> > > > > > > +Virtqueue coalescing parameters:
> > > > > > > +\begin{itemize}
> > > > > > > +\item \field{max_packets}: The maximum number of packets sent/received by the
> > > > > > > +    specified virtqueue before a TX/RX notification.
> > > > > > > +\item \field{usecs}: The maximum number of TX/RX usecs that the specified
> > > > > > > +    virtqueue delays a TX/RX notification.
> > > > > > > +\item \field{vqn}: The virtqueue number of the specified virtqueue.
> > > > > > > +\end{itemize}
> > > > > > > +
> > > > > > > +The range of \filed{vqn} is between 0 and 0xFFFF inclusive,
> > > > > > No really because last vq is a cvq. Maybe just drop this?
> > > > > In fact I have ruled out the control virtqueue.
> > > > > 
> > > > > Its virtqueue number should be 0x10000.
> > > > Nope, vq numberes are 16 bit.
> > > Yes I know. The [0, 0xFFFF] listed in v2 is the maximum range of virtqueue
> > > numbers (including all receiveqs and all transmitqs)
> > > that can be set by the driver, because the current spec says
> > > "The device MUST set \field{max_virtqueue_pairs} to between 1 and 0x8000
> > > inclusive, if it offers VIRTIO_NET_F_MQ.".
> > Oh that looks like a bug since
> > 
> > 	 \item[VIRTIO_NET_F_MQ] Requires VIRTIO_NET_F_CTRL_VQ.
> > 
> > so max_virtqueue_pairs 0x8000 is not legal.
> 
> No, it's not a bug.
> As described in the specification:
> "This field specifies the maximum number of each of transmit and receive
> virtqueues (receiveq1\ldots receiveqN and transmitq1\ldots transmitqN
> respectively)
> that can be configured once at least one of these features is negotiated."
> \field{max_virtqueue_pairs} does not include control queue.

there is no way to have 0x8000 of these pairs though.

> > 
> > 
> > > So assuming that the maximum number of receiveqs and transmitqs are 0x8000
> > > respectively,
> > > then the total is 0x10000 (the queue number to the control queue is
> > > 0x10001), that is, the vqn indexed from 0 is [0,0x10000-1]=[0,0xFFFF ],
> > > which already excludes controlq.
> > > 
> > > But I know that you mean to emphasize in the specification that no contro
> > > queue is included.
> > I would just drop The range of \filed{vqn} is between 0 and 0xFFFF
> > inclusive - it is a 16 bit value there is really no point to
> > say any more.
> 
> Sure. :)
> 
> Thanks.
> 
> > 
> > > > > > > $ \lfloor vqn / 2 \rfloor $
> > > > > > > +is the index of the corresponding receiveq, and $\lfloor (vqn / 2) + 1 \rfloor $ is
> > > > > > > +the corresponding tranmitq.
> > > > > > > +
> > > > > > > +The VIRTIO_NET_CTRL_NOTF_COAL_RX_SET command is the same as calling VIRTIO_NET_CTRL_NOTF_COAL_VQ
> > > > > > you don't "call" commands. driver sends them, device receives them.
> > > > > > But here you are talking about a command abstracty so I'd just drop
> > > > > > a verb, or maybe "repeating".
> > > > > > And "same" is inexact in that it's not the same - takes more time - it
> > > > > > just has the same effect, or is equivalent to.
> > > > > There is indeed a gerund match here, I'll fix that.
> > > > > 
> > > > > > > +for virtqueues corresponding to all receiveqs.
> > > > > > receiveqs is confusing.
> > > > > > 
> > > > > > Also elsewhere we use the language receiveq1\ldots receiveqn
> > > > > > which seems clearer. Also you can not call VIRTIO_NET_CTRL_NOTF_COAL_VQ
> > > > > > for all vqs - it applies to one vq only. You mean each.
> > > > > I express something wrong, but what I mean is to send for each virtqueue.
> > > > > 
> > > > > > And virtqueues do not correspond to receiveqs - they
> > > > > > are receiveqs. If you like vqn corresponds to them.
> > > > > This is ambiguous, the number of virtqueues is 2*N+1, the number of receive
> > > > > queues and transmit queues is N,
> > > > > and there is also a control queue. Is this a problem?
> > > > > But I know what you mean it's better to use "same as
> > > > > VIRTIO_NET_CTRL_NOTF_COAL_VQ repeated for each of receiveq1\ldots receiveqn"
> > > > > 
> > > > > > Or better just "same as VIRTIO_NET_CTRL_NOTF_COAL_VQ repeated for
> > > > > > each of receiveq1\ldots receiveqn"
> > > > > Sure.
> > > > > 
> > > > > > > +
> > > > > > > +The VIRTIO_NET_CTRL_NOTF_COAL_TX_SET command is the same as calling VIRTIO_NET_CTRL_NOTF_COAL_VQ
> > > > > > > +for virtqueues corresponding to all transmitqs.
> > > > > > > +
> > > > > > > +Virtqueue coalescing will be disabled if all parameters are set to 0.
> > > > > > In fact, either usecs 0 or max_packets disables coalescing, no?
> > > > > Yes. I'll fix this.
> > > > > 
> > > > > > > +
> > > > > > > +The class VIRTIO_NET_CTRL_NOTF_COAL_VQ has 2 commands:
> > > > > > > +\begin{enumerate}
> > > > > > > +\item VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET: set the \field{max_packets}, \field{usecs} and \filed{vqn} parameters.
> > > > > > > +\item VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET: get the \field{max_packets}, \field{usecs} and \field{vqn} parameters.
> > > > > > How does VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET work then? For which vq?
> > > > > > I think you mean vqn is specified and you get back max_packets and
> > > > > > usecs. All this needs to be documented fully for each command.
> > > > > Yes, VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET gets the parameters of the virtqueue
> > > > > corresponding to vqn each time.
> > > > > 
> > > > > > I would also think it's a good idea to mention that VQ_GET does not have
> > > > > > to return exactly the same parameters - since it's best effort anyway,
> > > > > This is confusing, I think the device does not have to set the same
> > > > > parameters for VQ_SET, but for VQ_GET, the device should return to the
> > > > > driver every time.
> > > > What I mean is that if you call VQ_SET with a value of 17,
> > > > device might decide to e.g. store just the power of 2
> > > Oh! I misunderstood what you meant before, I will add an appropriate
> > > reminder!
> > > 
> > > > 
> > > > > > it's ok for device to round down and store a smaller value for either
> > > > > > max_packets or usecs, e.g. to save space.
> > > > > > 
> > > > > > This kind of formalizes the best effort thing we discussed
> > > > > > previously.
> > > > > > 
> > > > > > 
> > > > > > Maybe make VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET a separate patch -
> > > > > > in the series,
> > > > > No problem, I'll try to describe it as best I can.
> > > > > 
> > > > > > at this point you did not make any effort to document it,
> > > > > > it needs more work.
> > > > > > 
> > > > > > 
> > > > > > > +\end{enumerate}
> > > > > > > +
> > > > > > >     \subparagraph{RX Notifications}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Notifications Coalescing / RX Notifications}
> > > > > > >     If, for example:
> > > > > > > @@ -4535,6 +4582,15 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
> > > > > > >     \item If the notifications are not suppressed by the driver, the device will send an used buffer notification, otherwise, the device will not send an used buffer notification as long as the notifications are suppressed.
> > > > > > >     \end{itemize}
> > > > > > > +If, example of setting coalescing parameters for a receive virtqueue:
> > > > > > "If" without "then" is confusing. I'd just start with "Example".
> > > > > Ok.
> > > > > 
> > > > > > > +\begin{itemize}
> > > > > > > +\item \field{max_packets} = 15.
> > > > > > > +\item \field{usecs} = 10.
> > > > > > > +\item \field{vqn} = 0;
> > > > > > why . above and ; here? I would just drop punctuation.
> > > > > It's a typo and I'll fix it.
> > > > > 
> > > > > > > +\end{itemize}
> > > > > > > +
> > > > > > > +The device will only operate on recieveq1 as VIRTIO_NET_CTRL_NOTF_COAL_RX_SET.
> > > > > > This really does not explain anything. Please just document exactly what
> > > > > > it does and does not do.
> > > > > Ok.
> > > > > 
> > > > > > > +
> > > > > > >     \subparagraph{TX Notifications}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Notifications Coalescing / TX Notifications}
> > > > > > >     If, for example:
> > > > > > > @@ -4550,13 +4606,24 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
> > > > > > >     \item If the notifications are not suppressed by the driver, the device will send an used buffer notification, otherwise, the device will not send an used buffer notification as long as the notifications are suppressed.
> > > > > > >     \end{itemize}
> > > > > > > +If, example of setting coalescing parameters for a transmit virtqueue:
> > > > > > > +\begin{itemize}
> > > > > > > +\item \field{max_packets} = 15.
> > > > > > > +\item \field{usecs} = 10.
> > > > > > > +\item \field{vqn} = 1;
> > > > > > > +\end{itemize}
> > > > > > > +
> > > > > > > +The device will only operate on transmitq1 as VIRTIO_NET_CTRL_NOTF_COAL_TX_SET.
> > > > > > > +
> > > > > > I feel it's the other way around. Document
> > > > > > 
> > > > > Why? I'll add documentation, but read on below.
> > > > > 
> > > > > > >     \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 the VIRTIO_NET_F_VQ_NOTF_COAL feature has not been negotiated, the driver MUST NOT issue VIRTIO_NET_CTRL_NOFT_COAL_VQ commands.
> > > > > > Don't we want to limit driver to legal values of vqn?
> > > > > > 
> > > > > Yes, I will add.
> > > > > 
> > > > > > >     \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 the VIRTIO_NET_CTRL_NOTF_COAL commands with VIRTIO_NET_ERR if it was not able to change the parameters or
> > > > > > > +was not able to find a virtqueue using the \field{vqn}.
> > > > > > First please create multiple statements not a single long one.
> > > > > > Second vqn only exists for per vq commands so create a statement
> > > > > > just for that.
> > > > > Ok, reasonable.
> > > > > 
> > > > > > Also more explicit please. What does this mean? I suspect that vq was not
> > > > > > enabled?
> > > > > Indicates that a vqn cannot be mapped to the corresponding virtqueue.
> > > > Still no idea. what is this mapping you are talking about.
> > > > Why can't it be mapped? what is corresponding to what?
> > > > 
> > > > vq with this number is not enabled or vqn >= 2max_virtqueue_pairs are
> > > I am referring to the former.
> > > 
> > > > the only reasons i could come up with.  if that's it just say so. if not
> > > > list the actual reasons.
> > > Sorry, I will explain more clearly later.
> > > 
> > > Thanks.
> > > 
> > > > > > Also, we MUST have vqn < max_virtqueue_pairs.
> > > > > Here should be vq < max_virtqueue_pairs * 2?
> > > > > 
> > > > > Thanks.
> > > > yea.
> > > > 
> > > > > > >     A device SHOULD NOT send used buffer notifications to the driver, if the notifications are suppressed as explained in \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Used Buffer Notification Suppression}, even if the coalescing counters expired.
> > > > > > > -- 
> > > > > > > 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


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/