virtio-comment

Currently, the coalescing profile is directly applied to all queues.
This patch supports configuring the parameters for a specified queue.

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>
---
content.tex | 49 ++++++++++++++++++++++++++++++++++++++++++-------
1 file changed, 42 insertions(+), 7 deletions(-)

diff --git a/content.tex b/content.tex
index e863709..049c0e4 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_PERQUEUE_NOTF_COAL(52)] Device supports per-queue
+ 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_PERQUEUE_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}
@@ -4488,16 +4492,21 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi

If the VIRTIO_NET_F_NOTF_COAL feature is negotiated, the driver can
send control commands for dynamically changing the coalescing parameters.
+If additionally VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated, the driver
+can send control commands to configure the coalescing parameters of a
+specified receiveq or transmitq.

\begin{lstlisting}
struct virtio_net_ctrl_coal_rx {
le32 rx_max_packets;
le32 rx_usecs;
+ le16 rx_qid; (Only if VIRTIO_NET_F_PERQUEUE_NOTF_COAL negotiated)
};

struct virtio_net_ctrl_coal_tx {
le32 tx_max_packets;
le32 tx_usecs;
+ le16 tx_qid; (Only if VIRTIO_NET_F_PERQUEUE_NOTF_COAL negotiated)
};

#define VIRTIO_NET_CTRL_NOTF_COAL 6
@@ -4507,17 +4516,34 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi

Coalescing parameters:
\begin{itemize}
-\item \field{rx_usecs}: Maximum number of usecs to delay a RX notification.
-\item \field{tx_usecs}: Maximum number of usecs to delay a TX notification.
+\item \field{rx_qid}: Index of which receiveq to change the coalescing parameters.
+ If the value is between 0 and 0x7FFF, it represents the index of the specified
+ receiveq. Otherwise, if the value is 0xFFFF, it indicates to change the
+ coalescing parameters for all receiveqs.
+\item \field{tx_qid}: Index of which transmitq to change the coalescing parameters.
+ If the value is between 0 and 0x7FFF, it represents the index of the specified
+ transmitq. Otherwise, if the value is 0xFFFF, it indicates to change the
+ coalescing parameters for all transmitqs.
+\item \field{rx_usecs}: Maximum number of usecs to delay a RX notification. If additionally
+ VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated, it works for the specified recieveq
+ or all receieveqs.
+\item \field{tx_usecs}: Maximum number of usecs to delay a TX notification. If additionally
+ VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated, it works for the specified transmitq
+ or all transmitqs.
\item \field{rx_max_packets}: Maximum number of packets to receive before a RX notification.
+ If additionally VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated, it works for the
+ specified receiveq or all receiveqs.
\item \field{tx_max_packets}: Maximum number of packets to send before a TX notification.
+ If additionally VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated, it works for the
+ specified transmitq or all transmitqs.
\end{itemize}

-
The class VIRTIO_NET_CTRL_NOTF_COAL has 2 commands:
\begin{enumerate}
-\item VIRTIO_NET_CTRL_NOTF_COAL_TX_SET: set the \field{tx_usecs} and \field{tx_max_packets} parameters.
-\item VIRTIO_NET_CTRL_NOTF_COAL_RX_SET: set the \field{rx_usecs} and \field{rx_max_packets} parameters.
+\item VIRTIO_NET_CTRL_NOTF_COAL_TX_SET: set the \field{tx_usecs} and \field{tx_max_packets} parameters,
+ and set the \field{tx_qid} if VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated.
+\item VIRTIO_NET_CTRL_NOTF_COAL_RX_SET: set the \field{rx_usecs} and \field{rx_max_packets} parameters,
+ and set the \field{rx_qid} if VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated.
\end{enumerate}

\subparagraph{RX Notifications}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Notifications Coalescing / RX Notifications}
@@ -4531,7 +4557,11 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
The device will operate as follows:

\begin{itemize}
-\item The device will count received packets until it accumulates 15, or until 10 usecs elapsed since the first one was received.
+\item The device will count received packets until it accumulates 15, or until
+ 10 usecs elapsed since the first one was received. If additionally
+ VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated: when \field{rx_qid} is set
+ to 0, the accumulation was only from the receiveq1; when \field{rx_qid} is
+ set to 0xFFFF, the accumulation was from all receiveqs.
\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}

@@ -4546,7 +4576,12 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
The device will operate as follows:

\begin{itemize}
-\item The device will count sent packets until it accumulates 15, or until 10 usecs elapsed since the first one was sent.
+\item The device will count sent packets on the transmitq1 until it accumulates 15,
+ or until 10 usecs elapsed since the first one was sent. If additionally
+ VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated: when \field{tx_qid} is set
+ to 0, the accumulation was only from the transmitq1; when \field{tx_qid} is
+ set to 0xFFFF, the accumulation was from all transmitqs.
+
\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}

--
2.19.1.6.gb485710b

Hi Heng,

> Currently, the coalescing profile is directly applied to all queues.
> This patch supports configuring the parameters for a specified queue.
>
> 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>
> ---
> content.tex | 49 ++++++++++++++++++++++++++++++++++++++++++-------
> 1 file changed, 42 insertions(+), 7 deletions(-)
>
> diff --git a/content.tex b/content.tex
> index e863709..049c0e4 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_PERQUEUE_NOTF_COAL(52)] Device supports per-queue
> + notifications coalescing.
> +

Since this feature allows us to change the coalescing parameters for
all the queues when rx/tx_qid = 0xFFFF, and since version 1.3 wasn't
released yet,
maybe the "per-vq" functionality can be added to
VIRTIO_NET_F_NOTF_COAL instead of adding a new feature?


> \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_PERQUEUE_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}
> @@ -4488,16 +4492,21 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
>
> If the VIRTIO_NET_F_NOTF_COAL feature is negotiated, the driver can
> send control commands for dynamically changing the coalescing parameters.
> +If additionally VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated, the driver
> +can send control commands to configure the coalescing parameters of a
> +specified receiveq or transmitq.
>
> \begin{lstlisting}
> struct virtio_net_ctrl_coal_rx {
> le32 rx_max_packets;
> le32 rx_usecs;
> + le16 rx_qid; (Only if VIRTIO_NET_F_PERQUEUE_NOTF_COAL negotiated)
> };
>
> struct virtio_net_ctrl_coal_tx {
> le32 tx_max_packets;
> le32 tx_usecs;
> + le16 tx_qid; (Only if VIRTIO_NET_F_PERQUEUE_NOTF_COAL negotiated)
> };
>
> #define VIRTIO_NET_CTRL_NOTF_COAL 6
> @@ -4507,17 +4516,34 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
>
> Coalescing parameters:
> \begin{itemize}
> -\item \field{rx_usecs}: Maximum number of usecs to delay a RX notification.
> -\item \field{tx_usecs}: Maximum number of usecs to delay a TX notification.
> +\item \field{rx_qid}: Index of which receiveq to change the coalescing parameters.
> + If the value is between 0 and 0x7FFF, it represents the index of the specified
> + receiveq. Otherwise, if the value is 0xFFFF, it indicates to change the
> + coalescing parameters for all receiveqs.
> +\item \field{tx_qid}: Index of which transmitq to change the coalescing parameters.
> + If the value is between 0 and 0x7FFF, it represents the index of the specified
> + transmitq. Otherwise, if the value is 0xFFFF, it indicates to change the
> + coalescing parameters for all transmitqs.
> +\item \field{rx_usecs}: Maximum number of usecs to delay a RX notification. If additionally
> + VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated, it works for the specified recieveq
> + or all receieveqs.
> +\item \field{tx_usecs}: Maximum number of usecs to delay a TX notification. If additionally
> + VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated, it works for the specified transmitq
> + or all transmitqs.
> \item \field{rx_max_packets}: Maximum number of packets to receive before a RX notification.
> + If additionally VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated, it works for the
> + specified receiveq or all receiveqs.
> \item \field{tx_max_packets}: Maximum number of packets to send before a TX notification.
> + If additionally VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated, it works for the
> + specified transmitq or all transmitqs.
> \end{itemize}
>
> -
> The class VIRTIO_NET_CTRL_NOTF_COAL has 2 commands:
> \begin{enumerate}
> -\item VIRTIO_NET_CTRL_NOTF_COAL_TX_SET: set the \field{tx_usecs} and \field{tx_max_packets} parameters.
> -\item VIRTIO_NET_CTRL_NOTF_COAL_RX_SET: set the \field{rx_usecs} and \field{rx_max_packets} parameters.
> +\item VIRTIO_NET_CTRL_NOTF_COAL_TX_SET: set the \field{tx_usecs} and \field{tx_max_packets} parameters,
> + and set the \field{tx_qid} if VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated.
> +\item VIRTIO_NET_CTRL_NOTF_COAL_RX_SET: set the \field{rx_usecs} and \field{rx_max_packets} parameters,
> + and set the \field{rx_qid} if VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated.
> \end{enumerate}
>
> \subparagraph{RX Notifications}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Notifications Coalescing / RX Notifications}
> @@ -4531,7 +4557,11 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
> The device will operate as follows:
>
> \begin{itemize}
> -\item The device will count received packets until it accumulates 15, or until 10 usecs elapsed since the first one was received.
> +\item The device will count received packets until it accumulates 15, or until
> + 10 usecs elapsed since the first one was received. If additionally
> + VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated: when \field{rx_qid} is set
> + to 0, the accumulation was only from the receiveq1; when \field{rx_qid} is
> + set to 0xFFFF, the accumulation was from all receiveqs.
> \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}
>
> @@ -4546,7 +4576,12 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
> The device will operate as follows:
>
> \begin{itemize}
> -\item The device will count sent packets until it accumulates 15, or until 10 usecs elapsed since the first one was sent.
> +\item The device will count sent packets on the transmitq1 until it accumulates 15,
> + or until 10 usecs elapsed since the first one was sent. If additionally
> + VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated: when \field{tx_qid} is set
> + to 0, the accumulation was only from the transmitq1; when \field{tx_qid} is
> + set to 0xFFFF, the accumulation was from all transmitqs.
> +
> \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}
>
> --
> 2.19.1.6.gb485710b
>

Hi Heng,

> Currently, the coalescing profile is directly applied to all queues.
> This patch supports configuring the parameters for a specified queue.
>
> 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>
> ---
> content.tex | 49 ++++++++++++++++++++++++++++++++++++++++++-------
> 1 file changed, 42 insertions(+), 7 deletions(-)
>
> diff --git a/content.tex b/content.tex
> index e863709..049c0e4 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_PERQUEUE_NOTF_COAL(52)] Device supports per-queue
> + notifications coalescing.
> +

Since this feature allows us to change the coalescing parameters for
all the queues when rx/tx_qid = 0xFFFF, and since version 1.3 wasn't
released yet,
maybe the "per-vq" functionality can be added to
VIRTIO_NET_F_NOTF_COAL instead of adding a new feature?


> \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_PERQUEUE_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}
> @@ -4488,16 +4492,21 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
>
> If the VIRTIO_NET_F_NOTF_COAL feature is negotiated, the driver can
> send control commands for dynamically changing the coalescing parameters.
> +If additionally VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated, the driver
> +can send control commands to configure the coalescing parameters of a
> +specified receiveq or transmitq.
>
> \begin{lstlisting}
> struct virtio_net_ctrl_coal_rx {
> le32 rx_max_packets;
> le32 rx_usecs;
> + le16 rx_qid; (Only if VIRTIO_NET_F_PERQUEUE_NOTF_COAL negotiated)
> };
>
> struct virtio_net_ctrl_coal_tx {
> le32 tx_max_packets;
> le32 tx_usecs;
> + le16 tx_qid; (Only if VIRTIO_NET_F_PERQUEUE_NOTF_COAL negotiated)
> };
>
> #define VIRTIO_NET_CTRL_NOTF_COAL 6
> @@ -4507,17 +4516,34 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
>
> Coalescing parameters:
> \begin{itemize}
> -\item \field{rx_usecs}: Maximum number of usecs to delay a RX notification.
> -\item \field{tx_usecs}: Maximum number of usecs to delay a TX notification.
> +\item \field{rx_qid}: Index of which receiveq to change the coalescing parameters.
> + If the value is between 0 and 0x7FFF, it represents the index of the specified
> + receiveq. Otherwise, if the value is 0xFFFF, it indicates to change the
> + coalescing parameters for all receiveqs.
> +\item \field{tx_qid}: Index of which transmitq to change the coalescing parameters.
> + If the value is between 0 and 0x7FFF, it represents the index of the specified
> + transmitq. Otherwise, if the value is 0xFFFF, it indicates to change the
> + coalescing parameters for all transmitqs.
> +\item \field{rx_usecs}: Maximum number of usecs to delay a RX notification. If additionally
> + VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated, it works for the specified recieveq
> + or all receieveqs.
> +\item \field{tx_usecs}: Maximum number of usecs to delay a TX notification. If additionally
> + VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated, it works for the specified transmitq
> + or all transmitqs.
> \item \field{rx_max_packets}: Maximum number of packets to receive before a RX notification.
> + If additionally VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated, it works for the
> + specified receiveq or all receiveqs.
> \item \field{tx_max_packets}: Maximum number of packets to send before a TX notification.
> + If additionally VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated, it works for the
> + specified transmitq or all transmitqs.
> \end{itemize}
>
> -
> The class VIRTIO_NET_CTRL_NOTF_COAL has 2 commands:
> \begin{enumerate}
> -\item VIRTIO_NET_CTRL_NOTF_COAL_TX_SET: set the \field{tx_usecs} and \field{tx_max_packets} parameters.
> -\item VIRTIO_NET_CTRL_NOTF_COAL_RX_SET: set the \field{rx_usecs} and \field{rx_max_packets} parameters.
> +\item VIRTIO_NET_CTRL_NOTF_COAL_TX_SET: set the \field{tx_usecs} and \field{tx_max_packets} parameters,
> + and set the \field{tx_qid} if VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated.
> +\item VIRTIO_NET_CTRL_NOTF_COAL_RX_SET: set the \field{rx_usecs} and \field{rx_max_packets} parameters,
> + and set the \field{rx_qid} if VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated.
> \end{enumerate}
>
> \subparagraph{RX Notifications}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Notifications Coalescing / RX Notifications}
> @@ -4531,7 +4557,11 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
> The device will operate as follows:
>
> \begin{itemize}
> -\item The device will count received packets until it accumulates 15, or until 10 usecs elapsed since the first one was received.
> +\item The device will count received packets until it accumulates 15, or until
> + 10 usecs elapsed since the first one was received. If additionally
> + VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated: when \field{rx_qid} is set
> + to 0, the accumulation was only from the receiveq1; when \field{rx_qid} is
> + set to 0xFFFF, the accumulation was from all receiveqs.
> \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}
>
> @@ -4546,7 +4576,12 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
> The device will operate as follows:
>
> \begin{itemize}
> -\item The device will count sent packets until it accumulates 15, or until 10 usecs elapsed since the first one was sent.
> +\item The device will count sent packets on the transmitq1 until it accumulates 15,
> + or until 10 usecs elapsed since the first one was sent. If additionally
> + VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated: when \field{tx_qid} is set
> + to 0, the accumulation was only from the transmitq1; when \field{tx_qid} is
> + set to 0xFFFF, the accumulation was from all transmitqs.
> +
> \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}
>
> --
> 2.19.1.6.gb485710b
>

On Tue, 7 Feb 2023 13:25:13 +0200, Alvaro Karsz <alvaro.karsz@solid-run.com> wrote:
> Hi Heng,
>
> > Currently, the coalescing profile is directly applied to all queues.
> > This patch supports configuring the parameters for a specified queue.
> >
> > 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>
> > ---
> > content.tex | 49 ++++++++++++++++++++++++++++++++++++++++++-------
> > 1 file changed, 42 insertions(+), 7 deletions(-)
> >
> > diff --git a/content.tex b/content.tex
> > index e863709..049c0e4 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_PERQUEUE_NOTF_COAL(52)] Device supports per-queue
> > + notifications coalescing.
> > +
>
> Since this feature allows us to change the coalescing parameters for
> all the queues when rx/tx_qid = 0xFFFF, and since version 1.3 wasn't
> released yet,
> maybe the "per-vq" functionality can be added to
> VIRTIO_NET_F_NOTF_COAL instead of adding a new feature?


According to my understanding, all the features of voting are formal. It
can be used by the manufacturer.

Of course, as far as I know, no manufacturer has used this feature for the
time being. But I think we should add a new feature.

Or other people have other ideas.

Thanks.



>
>
> > \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_PERQUEUE_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}
> > @@ -4488,16 +4492,21 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
> >
> > If the VIRTIO_NET_F_NOTF_COAL feature is negotiated, the driver can
> > send control commands for dynamically changing the coalescing parameters.
> > +If additionally VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated, the driver
> > +can send control commands to configure the coalescing parameters of a
> > +specified receiveq or transmitq.
> >
> > \begin{lstlisting}
> > struct virtio_net_ctrl_coal_rx {
> > le32 rx_max_packets;
> > le32 rx_usecs;
> > + le16 rx_qid; (Only if VIRTIO_NET_F_PERQUEUE_NOTF_COAL negotiated)
> > };
> >
> > struct virtio_net_ctrl_coal_tx {
> > le32 tx_max_packets;
> > le32 tx_usecs;
> > + le16 tx_qid; (Only if VIRTIO_NET_F_PERQUEUE_NOTF_COAL negotiated)
> > };
> >
> > #define VIRTIO_NET_CTRL_NOTF_COAL 6
> > @@ -4507,17 +4516,34 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
> >
> > Coalescing parameters:
> > \begin{itemize}
> > -\item \field{rx_usecs}: Maximum number of usecs to delay a RX notification.
> > -\item \field{tx_usecs}: Maximum number of usecs to delay a TX notification.
> > +\item \field{rx_qid}: Index of which receiveq to change the coalescing parameters.
> > + If the value is between 0 and 0x7FFF, it represents the index of the specified
> > + receiveq. Otherwise, if the value is 0xFFFF, it indicates to change the
> > + coalescing parameters for all receiveqs.
> > +\item \field{tx_qid}: Index of which transmitq to change the coalescing parameters.
> > + If the value is between 0 and 0x7FFF, it represents the index of the specified
> > + transmitq. Otherwise, if the value is 0xFFFF, it indicates to change the
> > + coalescing parameters for all transmitqs.
> > +\item \field{rx_usecs}: Maximum number of usecs to delay a RX notification. If additionally
> > + VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated, it works for the specified recieveq
> > + or all receieveqs.
> > +\item \field{tx_usecs}: Maximum number of usecs to delay a TX notification. If additionally
> > + VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated, it works for the specified transmitq
> > + or all transmitqs.
> > \item \field{rx_max_packets}: Maximum number of packets to receive before a RX notification.
> > + If additionally VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated, it works for the
> > + specified receiveq or all receiveqs.
> > \item \field{tx_max_packets}: Maximum number of packets to send before a TX notification.
> > + If additionally VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated, it works for the
> > + specified transmitq or all transmitqs.
> > \end{itemize}
> >
> > -
> > The class VIRTIO_NET_CTRL_NOTF_COAL has 2 commands:
> > \begin{enumerate}
> > -\item VIRTIO_NET_CTRL_NOTF_COAL_TX_SET: set the \field{tx_usecs} and \field{tx_max_packets} parameters.
> > -\item VIRTIO_NET_CTRL_NOTF_COAL_RX_SET: set the \field{rx_usecs} and \field{rx_max_packets} parameters.
> > +\item VIRTIO_NET_CTRL_NOTF_COAL_TX_SET: set the \field{tx_usecs} and \field{tx_max_packets} parameters,
> > + and set the \field{tx_qid} if VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated.
> > +\item VIRTIO_NET_CTRL_NOTF_COAL_RX_SET: set the \field{rx_usecs} and \field{rx_max_packets} parameters,
> > + and set the \field{rx_qid} if VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated.
> > \end{enumerate}
> >
> > \subparagraph{RX Notifications}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Notifications Coalescing / RX Notifications}
> > @@ -4531,7 +4557,11 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
> > The device will operate as follows:
> >
> > \begin{itemize}
> > -\item The device will count received packets until it accumulates 15, or until 10 usecs elapsed since the first one was received.
> > +\item The device will count received packets until it accumulates 15, or until
> > + 10 usecs elapsed since the first one was received. If additionally
> > + VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated: when \field{rx_qid} is set
> > + to 0, the accumulation was only from the receiveq1; when \field{rx_qid} is
> > + set to 0xFFFF, the accumulation was from all receiveqs.
> > \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}
> >
> > @@ -4546,7 +4576,12 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
> > The device will operate as follows:
> >
> > \begin{itemize}
> > -\item The device will count sent packets until it accumulates 15, or until 10 usecs elapsed since the first one was sent.
> > +\item The device will count sent packets on the transmitq1 until it accumulates 15,
> > + or until 10 usecs elapsed since the first one was sent. If additionally
> > + VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated: when \field{tx_qid} is set
> > + to 0, the accumulation was only from the transmitq1; when \field{tx_qid} is
> > + set to 0xFFFF, the accumulation was from all transmitqs.
> > +
> > \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}
> >
> > --
> > 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/
>

On Tue, 7 Feb 2023 13:25:13 +0200, Alvaro Karsz <alvaro.karsz@solid-run.com> wrote:
> Hi Heng,
>
> > Currently, the coalescing profile is directly applied to all queues.
> > This patch supports configuring the parameters for a specified queue.
> >
> > 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>
> > ---
> > content.tex | 49 ++++++++++++++++++++++++++++++++++++++++++-------
> > 1 file changed, 42 insertions(+), 7 deletions(-)
> >
> > diff --git a/content.tex b/content.tex
> > index e863709..049c0e4 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_PERQUEUE_NOTF_COAL(52)] Device supports per-queue
> > + notifications coalescing.
> > +
>
> Since this feature allows us to change the coalescing parameters for
> all the queues when rx/tx_qid = 0xFFFF, and since version 1.3 wasn't
> released yet,
> maybe the "per-vq" functionality can be added to
> VIRTIO_NET_F_NOTF_COAL instead of adding a new feature?


According to my understanding, all the features of voting are formal. It
can be used by the manufacturer.

Of course, as far as I know, no manufacturer has used this feature for the
time being. But I think we should add a new feature.

Or other people have other ideas.

Thanks.



>
>
> > \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_PERQUEUE_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}
> > @@ -4488,16 +4492,21 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
> >
> > If the VIRTIO_NET_F_NOTF_COAL feature is negotiated, the driver can
> > send control commands for dynamically changing the coalescing parameters.
> > +If additionally VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated, the driver
> > +can send control commands to configure the coalescing parameters of a
> > +specified receiveq or transmitq.
> >
> > \begin{lstlisting}
> > struct virtio_net_ctrl_coal_rx {
> > le32 rx_max_packets;
> > le32 rx_usecs;
> > + le16 rx_qid; (Only if VIRTIO_NET_F_PERQUEUE_NOTF_COAL negotiated)
> > };
> >
> > struct virtio_net_ctrl_coal_tx {
> > le32 tx_max_packets;
> > le32 tx_usecs;
> > + le16 tx_qid; (Only if VIRTIO_NET_F_PERQUEUE_NOTF_COAL negotiated)
> > };
> >
> > #define VIRTIO_NET_CTRL_NOTF_COAL 6
> > @@ -4507,17 +4516,34 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
> >
> > Coalescing parameters:
> > \begin{itemize}
> > -\item \field{rx_usecs}: Maximum number of usecs to delay a RX notification.
> > -\item \field{tx_usecs}: Maximum number of usecs to delay a TX notification.
> > +\item \field{rx_qid}: Index of which receiveq to change the coalescing parameters.
> > + If the value is between 0 and 0x7FFF, it represents the index of the specified
> > + receiveq. Otherwise, if the value is 0xFFFF, it indicates to change the
> > + coalescing parameters for all receiveqs.
> > +\item \field{tx_qid}: Index of which transmitq to change the coalescing parameters.
> > + If the value is between 0 and 0x7FFF, it represents the index of the specified
> > + transmitq. Otherwise, if the value is 0xFFFF, it indicates to change the
> > + coalescing parameters for all transmitqs.
> > +\item \field{rx_usecs}: Maximum number of usecs to delay a RX notification. If additionally
> > + VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated, it works for the specified recieveq
> > + or all receieveqs.
> > +\item \field{tx_usecs}: Maximum number of usecs to delay a TX notification. If additionally
> > + VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated, it works for the specified transmitq
> > + or all transmitqs.
> > \item \field{rx_max_packets}: Maximum number of packets to receive before a RX notification.
> > + If additionally VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated, it works for the
> > + specified receiveq or all receiveqs.
> > \item \field{tx_max_packets}: Maximum number of packets to send before a TX notification.
> > + If additionally VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated, it works for the
> > + specified transmitq or all transmitqs.
> > \end{itemize}
> >
> > -
> > The class VIRTIO_NET_CTRL_NOTF_COAL has 2 commands:
> > \begin{enumerate}
> > -\item VIRTIO_NET_CTRL_NOTF_COAL_TX_SET: set the \field{tx_usecs} and \field{tx_max_packets} parameters.
> > -\item VIRTIO_NET_CTRL_NOTF_COAL_RX_SET: set the \field{rx_usecs} and \field{rx_max_packets} parameters.
> > +\item VIRTIO_NET_CTRL_NOTF_COAL_TX_SET: set the \field{tx_usecs} and \field{tx_max_packets} parameters,
> > + and set the \field{tx_qid} if VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated.
> > +\item VIRTIO_NET_CTRL_NOTF_COAL_RX_SET: set the \field{rx_usecs} and \field{rx_max_packets} parameters,
> > + and set the \field{rx_qid} if VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated.
> > \end{enumerate}
> >
> > \subparagraph{RX Notifications}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Notifications Coalescing / RX Notifications}
> > @@ -4531,7 +4557,11 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
> > The device will operate as follows:
> >
> > \begin{itemize}
> > -\item The device will count received packets until it accumulates 15, or until 10 usecs elapsed since the first one was received.
> > +\item The device will count received packets until it accumulates 15, or until
> > + 10 usecs elapsed since the first one was received. If additionally
> > + VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated: when \field{rx_qid} is set
> > + to 0, the accumulation was only from the receiveq1; when \field{rx_qid} is
> > + set to 0xFFFF, the accumulation was from all receiveqs.
> > \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}
> >
> > @@ -4546,7 +4576,12 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
> > The device will operate as follows:
> >
> > \begin{itemize}
> > -\item The device will count sent packets until it accumulates 15, or until 10 usecs elapsed since the first one was sent.
> > +\item The device will count sent packets on the transmitq1 until it accumulates 15,
> > + or until 10 usecs elapsed since the first one was sent. If additionally
> > + VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated: when \field{tx_qid} is set
> > + to 0, the accumulation was only from the transmitq1; when \field{tx_qid} is
> > + set to 0xFFFF, the accumulation was from all transmitqs.
> > +
> > \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}
> >
> > --
> > 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/
>

> From: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> Sent: Tuesday, February 7, 2023 6:50 AM
>
> On Tue, 7 Feb 2023 13:25:13 +0200, Alvaro Karsz <alvaro.karsz@solid-run.com>
> wrote:
> > Hi Heng,
> >
> > > Currently, the coalescing profile is directly applied to all queues.
> > > This patch supports configuring the parameters for a specified queue.
> > >
> > > 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>
> > > ---
> > > content.tex | 49 ++++++++++++++++++++++++++++++++++++++++++-------
> > > 1 file changed, 42 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/content.tex b/content.tex index e863709..049c0e4 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_PERQUEUE_NOTF_COAL(52)] Device supports per-
> queue
> > > + notifications coalescing.
> > > +
> >
> > Since this feature allows us to change the coalescing parameters for
> > all the queues when rx/tx_qid = 0xFFFF, and since version 1.3 wasn't
> > released yet, maybe the "per-vq" functionality can be added to
> > VIRTIO_NET_F_NOTF_COAL instead of adding a new feature?
>
>
> According to my understanding, all the features of voting are formal. It can be
> used by the manufacturer.
>
> Of course, as far as I know, no manufacturer has used this feature for the time
> being. But I think we should add a new feature.
>
> Or other people have other ideas.

I believe we should treat it as fix and avoid a new feature bit as spec is not released, and it is very recent change.

> From: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> Sent: Tuesday, February 7, 2023 6:50 AM
>
> On Tue, 7 Feb 2023 13:25:13 +0200, Alvaro Karsz <alvaro.karsz@solid-run.com>
> wrote:
> > Hi Heng,
> >
> > > Currently, the coalescing profile is directly applied to all queues.
> > > This patch supports configuring the parameters for a specified queue.
> > >
> > > 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>
> > > ---
> > > content.tex | 49 ++++++++++++++++++++++++++++++++++++++++++-------
> > > 1 file changed, 42 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/content.tex b/content.tex index e863709..049c0e4 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_PERQUEUE_NOTF_COAL(52)] Device supports per-
> queue
> > > + notifications coalescing.
> > > +
> >
> > Since this feature allows us to change the coalescing parameters for
> > all the queues when rx/tx_qid = 0xFFFF, and since version 1.3 wasn't
> > released yet, maybe the "per-vq" functionality can be added to
> > VIRTIO_NET_F_NOTF_COAL instead of adding a new feature?
>
>
> According to my understanding, all the features of voting are formal. It can be
> used by the manufacturer.
>
> Of course, as far as I know, no manufacturer has used this feature for the time
> being. But I think we should add a new feature.
>
> Or other people have other ideas.

I believe we should treat it as fix and avoid a new feature bit as spec is not released, and it is very recent change.

On Tue, Feb 07, 2023 at 12:51:26PM +0000, Parav Pandit wrote:
>
> > From: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > Sent: Tuesday, February 7, 2023 6:50 AM
> >
> > On Tue, 7 Feb 2023 13:25:13 +0200, Alvaro Karsz <alvaro.karsz@solid-run.com>
> > wrote:
> > > Hi Heng,
> > >
> > > > Currently, the coalescing profile is directly applied to all queues.
> > > > This patch supports configuring the parameters for a specified queue.
> > > >
> > > > 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>
> > > > ---
> > > > content.tex | 49 ++++++++++++++++++++++++++++++++++++++++++-------
> > > > 1 file changed, 42 insertions(+), 7 deletions(-)
> > > >
> > > > diff --git a/content.tex b/content.tex index e863709..049c0e4 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_PERQUEUE_NOTF_COAL(52)] Device supports per-
> > queue
> > > > + notifications coalescing.
> > > > +
> > >
> > > Since this feature allows us to change the coalescing parameters for
> > > all the queues when rx/tx_qid = 0xFFFF, and since version 1.3 wasn't
> > > released yet, maybe the "per-vq" functionality can be added to
> > > VIRTIO_NET_F_NOTF_COAL instead of adding a new feature?
> >
> >
> > According to my understanding, all the features of voting are formal. It can be
> > used by the manufacturer.
> >
> > Of course, as far as I know, no manufacturer has used this feature for the time
> > being. But I think we should add a new feature.
> >
> > Or other people have other ideas.
>
> I believe we should treat it as fix and avoid a new feature bit as spec is not released, and it is very recent change.

Arguably it's true.
It will all be up to the committee vote of course ...
To keep things a bit safer how about we just allow commands
without qid instead of a special qid value?
Also if we are going to change things how about adding a query command too?

Also Alvaro what is your take on whether the gloabal version counts
packets and time for all queues together or per queue? The spec
does not make it clear ATM.

--
MST

On Tue, Feb 07, 2023 at 12:51:26PM +0000, Parav Pandit wrote:
>
> > From: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > Sent: Tuesday, February 7, 2023 6:50 AM
> >
> > On Tue, 7 Feb 2023 13:25:13 +0200, Alvaro Karsz <alvaro.karsz@solid-run.com>
> > wrote:
> > > Hi Heng,
> > >
> > > > Currently, the coalescing profile is directly applied to all queues.
> > > > This patch supports configuring the parameters for a specified queue.
> > > >
> > > > 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>
> > > > ---
> > > > content.tex | 49 ++++++++++++++++++++++++++++++++++++++++++-------
> > > > 1 file changed, 42 insertions(+), 7 deletions(-)
> > > >
> > > > diff --git a/content.tex b/content.tex index e863709..049c0e4 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_PERQUEUE_NOTF_COAL(52)] Device supports per-
> > queue
> > > > + notifications coalescing.
> > > > +
> > >
> > > Since this feature allows us to change the coalescing parameters for
> > > all the queues when rx/tx_qid = 0xFFFF, and since version 1.3 wasn't
> > > released yet, maybe the "per-vq" functionality can be added to
> > > VIRTIO_NET_F_NOTF_COAL instead of adding a new feature?
> >
> >
> > According to my understanding, all the features of voting are formal. It can be
> > used by the manufacturer.
> >
> > Of course, as far as I know, no manufacturer has used this feature for the time
> > being. But I think we should add a new feature.
> >
> > Or other people have other ideas.
>
> I believe we should treat it as fix and avoid a new feature bit as spec is not released, and it is very recent change.

Arguably it's true.
It will all be up to the committee vote of course ...
To keep things a bit safer how about we just allow commands
without qid instead of a special qid value?
Also if we are going to change things how about adding a query command too?

Also Alvaro what is your take on whether the gloabal version counts
packets and time for all queues together or per queue? The spec
does not make it clear ATM.

--
MST

> Arguably it's true.
> It will all be up to the committee vote of course ...
> To keep things a bit safer how about we just allow commands
> without qid instead of a special qid value?
> Also if we are going to change things how about adding a query command too?
>
> Also Alvaro what is your take on whether the gloabal version counts
> packets and time for all queues together or per queue? The spec
> does not make it clear ATM.

My intention was for it to be global, mirroring ethtool coalescing parameters.
The notification coalescing section doesn't mention virtqueues, but it
is not directly mentioned that these are global parameters.

> Arguably it's true.
> It will all be up to the committee vote of course ...
> To keep things a bit safer how about we just allow commands
> without qid instead of a special qid value?
> Also if we are going to change things how about adding a query command too?
>
> Also Alvaro what is your take on whether the gloabal version counts
> packets and time for all queues together or per queue? The spec
> does not make it clear ATM.

My intention was for it to be global, mirroring ethtool coalescing parameters.
The notification coalescing section doesn't mention virtqueues, but it
is not directly mentioned that these are global parameters.

On Tue, Feb 07, 2023 at 04:40:56PM +0200, Alvaro Karsz wrote:
> > Arguably it's true.
> > It will all be up to the committee vote of course ...
> > To keep things a bit safer how about we just allow commands
> > without qid instead of a special qid value?
> > Also if we are going to change things how about adding a query command too?
> >
> > Also Alvaro what is your take on whether the gloabal version counts
> > packets and time for all queues together or per queue? The spec
> > does not make it clear ATM.
>
> My intention was for it to be global, mirroring ethtool coalescing parameters.
> The notification coalescing section doesn't mention virtqueues, but it
> is not directly mentioned that these are global parameters.

An example of a question is this: we have two RX queues say 1 and 2 each
with a distinct interrupt vector. coalescing is set to 10 packets. Now
9 packets arrive on queue 1 and 1 on queue 2. Do you expect an
interrupt? If yes which one - 1 or 2 or both?

--
MST

On Tue, Feb 07, 2023 at 04:40:56PM +0200, Alvaro Karsz wrote:
> > Arguably it's true.
> > It will all be up to the committee vote of course ...
> > To keep things a bit safer how about we just allow commands
> > without qid instead of a special qid value?
> > Also if we are going to change things how about adding a query command too?
> >
> > Also Alvaro what is your take on whether the gloabal version counts
> > packets and time for all queues together or per queue? The spec
> > does not make it clear ATM.
>
> My intention was for it to be global, mirroring ethtool coalescing parameters.
> The notification coalescing section doesn't mention virtqueues, but it
> is not directly mentioned that these are global parameters.

An example of a question is this: we have two RX queues say 1 and 2 each
with a distinct interrupt vector. coalescing is set to 10 packets. Now
9 packets arrive on queue 1 and 1 on queue 2. Do you expect an
interrupt? If yes which one - 1 or 2 or both?

--
MST

> An example of a question is this: we have two RX queues say 1 and 2 each
> with a distinct interrupt vector. coalescing is set to 10 packets. Now
> 9 packets arrive on queue 1 and 1 on queue 2. Do you expect an
> interrupt? If yes which one - 1 or 2 or both?

Sorry, I wasn't clear enough.
In this case, you should not issue an interrupt until one of the
queues receives the 10'th packet.
The counters are per queue, the coalescing parameters (usecs and
max_packets) are global.

I agree that this should be mentioned in the spec, I missed it.

> An example of a question is this: we have two RX queues say 1 and 2 each
> with a distinct interrupt vector. coalescing is set to 10 packets. Now
> 9 packets arrive on queue 1 and 1 on queue 2. Do you expect an
> interrupt? If yes which one - 1 or 2 or both?

Sorry, I wasn't clear enough.
In this case, you should not issue an interrupt until one of the
queues receives the 10'th packet.
The counters are per queue, the coalescing parameters (usecs and
max_packets) are global.

I agree that this should be mentioned in the spec, I missed it.

On Tue, Feb 07, 2023 at 04:56:06PM +0200, Alvaro Karsz wrote:
> > An example of a question is this: we have two RX queues say 1 and 2 each
> > with a distinct interrupt vector. coalescing is set to 10 packets. Now
> > 9 packets arrive on queue 1 and 1 on queue 2. Do you expect an
> > interrupt? If yes which one - 1 or 2 or both?
>
> Sorry, I wasn't clear enough.
> In this case, you should not issue an interrupt until one of the
> queues receives the 10'th packet.
> The counters are per queue, the coalescing parameters (usecs and
> max_packets) are global.
>
> I agree that this should be mentioned in the spec, I missed it.

Right. So in my humble opinion, ability to configure parameters per
queue is not completely free, as this is doubling the memory requirements.
Sounds like a feature bit is somewhat justified. No?

--
MST

On Tue, Feb 07, 2023 at 04:56:06PM +0200, Alvaro Karsz wrote:
> > An example of a question is this: we have two RX queues say 1 and 2 each
> > with a distinct interrupt vector. coalescing is set to 10 packets. Now
> > 9 packets arrive on queue 1 and 1 on queue 2. Do you expect an
> > interrupt? If yes which one - 1 or 2 or both?
>
> Sorry, I wasn't clear enough.
> In this case, you should not issue an interrupt until one of the
> queues receives the 10'th packet.
> The counters are per queue, the coalescing parameters (usecs and
> max_packets) are global.
>
> I agree that this should be mentioned in the spec, I missed it.

Right. So in my humble opinion, ability to configure parameters per
queue is not completely free, as this is doubling the memory requirements.
Sounds like a feature bit is somewhat justified. No?

--
MST

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Tuesday, February 7, 2023 10:09 AM
>
> On Tue, Feb 07, 2023 at 04:56:06PM +0200, Alvaro Karsz wrote:
> > > An example of a question is this: we have two RX queues say 1 and 2
> > > each with a distinct interrupt vector. coalescing is set to 10
> > > packets. Now
> > > 9 packets arrive on queue 1 and 1 on queue 2. Do you expect an
> > > interrupt? If yes which one - 1 or 2 or both?
> >
> > Sorry, I wasn't clear enough.
> > In this case, you should not issue an interrupt until one of the
> > queues receives the 10'th packet.
> > The counters are per queue, the coalescing parameters (usecs and
> > max_packets) are global.
> >
Counters and parameters go hand-in hand.
It should be per VQ.

> > I agree that this should be mentioned in the spec, I missed it.
>
> Right. So in my humble opinion, ability to configure parameters per queue is
> not completely free, as this is doubling the memory requirements.
> Sounds like a feature bit is somewhat justified. No?
>
> --
> MST

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Tuesday, February 7, 2023 10:09 AM
>
> On Tue, Feb 07, 2023 at 04:56:06PM +0200, Alvaro Karsz wrote:
> > > An example of a question is this: we have two RX queues say 1 and 2
> > > each with a distinct interrupt vector. coalescing is set to 10
> > > packets. Now
> > > 9 packets arrive on queue 1 and 1 on queue 2. Do you expect an
> > > interrupt? If yes which one - 1 or 2 or both?
> >
> > Sorry, I wasn't clear enough.
> > In this case, you should not issue an interrupt until one of the
> > queues receives the 10'th packet.
> > The counters are per queue, the coalescing parameters (usecs and
> > max_packets) are global.
> >
Counters and parameters go hand-in hand.
It should be per VQ.

> > I agree that this should be mentioned in the spec, I missed it.
>
> Right. So in my humble opinion, ability to configure parameters per queue is
> not completely free, as this is doubling the memory requirements.
> Sounds like a feature bit is somewhat justified. No?
>
> --
> MST

On Tue, Feb 07, 2023 at 03:25:55PM +0000, Parav Pandit wrote:
>
>
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Tuesday, February 7, 2023 10:09 AM
> >
> > On Tue, Feb 07, 2023 at 04:56:06PM +0200, Alvaro Karsz wrote:
> > > > An example of a question is this: we have two RX queues say 1 and 2
> > > > each with a distinct interrupt vector. coalescing is set to 10
> > > > packets. Now
> > > > 9 packets arrive on queue 1 and 1 on queue 2. Do you expect an
> > > > interrupt? If yes which one - 1 or 2 or both?
> > >
> > > Sorry, I wasn't clear enough.
> > > In this case, you should not issue an interrupt until one of the
> > > queues receives the 10'th packet.
> > > The counters are per queue, the coalescing parameters (usecs and
> > > max_packets) are global.
> > >
> Counters and parameters go hand-in hand.
> It should be per VQ.

Well you could have a global parameter and per queue counter.
In fact ethtool only has a global parameter support - how
will users configure per-queue ones?

> > > I agree that this should be mentioned in the spec, I missed it.
> >
> > Right. So in my humble opinion, ability to configure parameters per queue is
> > not completely free, as this is doubling the memory requirements.
> > Sounds like a feature bit is somewhat justified. No?
> >
> > --
> > MST

On Tue, Feb 07, 2023 at 03:25:55PM +0000, Parav Pandit wrote:
>
>
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Tuesday, February 7, 2023 10:09 AM
> >
> > On Tue, Feb 07, 2023 at 04:56:06PM +0200, Alvaro Karsz wrote:
> > > > An example of a question is this: we have two RX queues say 1 and 2
> > > > each with a distinct interrupt vector. coalescing is set to 10
> > > > packets. Now
> > > > 9 packets arrive on queue 1 and 1 on queue 2. Do you expect an
> > > > interrupt? If yes which one - 1 or 2 or both?
> > >
> > > Sorry, I wasn't clear enough.
> > > In this case, you should not issue an interrupt until one of the
> > > queues receives the 10'th packet.
> > > The counters are per queue, the coalescing parameters (usecs and
> > > max_packets) are global.
> > >
> Counters and parameters go hand-in hand.
> It should be per VQ.

Well you could have a global parameter and per queue counter.
In fact ethtool only has a global parameter support - how
will users configure per-queue ones?

> > > I agree that this should be mentioned in the spec, I missed it.
> >
> > Right. So in my humble opinion, ability to configure parameters per queue is
> > not completely free, as this is doubling the memory requirements.
> > Sounds like a feature bit is somewhat justified. No?
> >
> > --
> > MST

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Tuesday, February 7, 2023 10:29 AM
>
> On Tue, Feb 07, 2023 at 03:25:55PM +0000, Parav Pandit wrote:
> >
> >
> > > From: Michael S. Tsirkin <mst@redhat.com>
> > > Sent: Tuesday, February 7, 2023 10:09 AM
> > >
> > > On Tue, Feb 07, 2023 at 04:56:06PM +0200, Alvaro Karsz wrote:
> > > > > An example of a question is this: we have two RX queues say 1
> > > > > and 2 each with a distinct interrupt vector. coalescing is set
> > > > > to 10 packets. Now
> > > > > 9 packets arrive on queue 1 and 1 on queue 2. Do you expect an
> > > > > interrupt? If yes which one - 1 or 2 or both?
> > > >
> > > > Sorry, I wasn't clear enough.
> > > > In this case, you should not issue an interrupt until one of the
> > > > queues receives the 10'th packet.
> > > > The counters are per queue, the coalescing parameters (usecs and
> > > > max_packets) are global.
> > > >
> > Counters and parameters go hand-in hand.
> > It should be per VQ.
>
> Well you could have a global parameter and per queue counter.
> In fact ethtool only has a global parameter support - how will users configure
> per-queue ones?
>
Net dim infrastructure will use per VQ knobs.

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Tuesday, February 7, 2023 10:29 AM
>
> On Tue, Feb 07, 2023 at 03:25:55PM +0000, Parav Pandit wrote:
> >
> >
> > > From: Michael S. Tsirkin <mst@redhat.com>
> > > Sent: Tuesday, February 7, 2023 10:09 AM
> > >
> > > On Tue, Feb 07, 2023 at 04:56:06PM +0200, Alvaro Karsz wrote:
> > > > > An example of a question is this: we have two RX queues say 1
> > > > > and 2 each with a distinct interrupt vector. coalescing is set
> > > > > to 10 packets. Now
> > > > > 9 packets arrive on queue 1 and 1 on queue 2. Do you expect an
> > > > > interrupt? If yes which one - 1 or 2 or both?
> > > >
> > > > Sorry, I wasn't clear enough.
> > > > In this case, you should not issue an interrupt until one of the
> > > > queues receives the 10'th packet.
> > > > The counters are per queue, the coalescing parameters (usecs and
> > > > max_packets) are global.
> > > >
> > Counters and parameters go hand-in hand.
> > It should be per VQ.
>
> Well you could have a global parameter and per queue counter.
> In fact ethtool only has a global parameter support - how will users configure
> per-queue ones?
>
Net dim infrastructure will use per VQ knobs.

On Tue, 7 Feb 2023 10:28:37 -0500, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Tue, Feb 07, 2023 at 03:25:55PM +0000, Parav Pandit wrote:
> >
> >
> > > From: Michael S. Tsirkin <mst@redhat.com>
> > > Sent: Tuesday, February 7, 2023 10:09 AM
> > >
> > > On Tue, Feb 07, 2023 at 04:56:06PM +0200, Alvaro Karsz wrote:
> > > > > An example of a question is this: we have two RX queues say 1 and 2
> > > > > each with a distinct interrupt vector. coalescing is set to 10
> > > > > packets. Now
> > > > > 9 packets arrive on queue 1 and 1 on queue 2. Do you expect an
> > > > > interrupt? If yes which one - 1 or 2 or both?
> > > >
> > > > Sorry, I wasn't clear enough.
> > > > In this case, you should not issue an interrupt until one of the
> > > > queues receives the 10'th packet.
> > > > The counters are per queue, the coalescing parameters (usecs and
> > > > max_packets) are global.
> > > >
> > Counters and parameters go hand-in hand.
> > It should be per VQ.
>
> Well you could have a global parameter and per queue counter.

The global parameter configuration I understand is to configure the counter
of all queues to the same value through a command.

Thanks.


> In fact ethtool only has a global parameter support - how
> will users configure per-queue ones?
>
> > > > I agree that this should be mentioned in the spec, I missed it.
> > >
> > > Right. So in my humble opinion, ability to configure parameters per queue is
> > > not completely free, as this is doubling the memory requirements.
> > > Sounds like a feature bit is somewhat justified. No?
> > >
> > > --
> > > MST
>

On Tue, 7 Feb 2023 10:28:37 -0500, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Tue, Feb 07, 2023 at 03:25:55PM +0000, Parav Pandit wrote:
> >
> >
> > > From: Michael S. Tsirkin <mst@redhat.com>
> > > Sent: Tuesday, February 7, 2023 10:09 AM
> > >
> > > On Tue, Feb 07, 2023 at 04:56:06PM +0200, Alvaro Karsz wrote:
> > > > > An example of a question is this: we have two RX queues say 1 and 2
> > > > > each with a distinct interrupt vector. coalescing is set to 10
> > > > > packets. Now
> > > > > 9 packets arrive on queue 1 and 1 on queue 2. Do you expect an
> > > > > interrupt? If yes which one - 1 or 2 or both?
> > > >
> > > > Sorry, I wasn't clear enough.
> > > > In this case, you should not issue an interrupt until one of the
> > > > queues receives the 10'th packet.
> > > > The counters are per queue, the coalescing parameters (usecs and
> > > > max_packets) are global.
> > > >
> > Counters and parameters go hand-in hand.
> > It should be per VQ.
>
> Well you could have a global parameter and per queue counter.

The global parameter configuration I understand is to configure the counter
of all queues to the same value through a command.

Thanks.


> In fact ethtool only has a global parameter support - how
> will users configure per-queue ones?
>
> > > > I agree that this should be mentioned in the spec, I missed it.
> > >
> > > Right. So in my humble opinion, ability to configure parameters per queue is
> > > not completely free, as this is doubling the memory requirements.
> > > Sounds like a feature bit is somewhat justified. No?
> > >
> > > --
> > > MST
>

? 2023/2/7 ??10:29, Michael S. Tsirkin ??:
> On Tue, Feb 07, 2023 at 12:51:26PM +0000, Parav Pandit wrote:
>>> From: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
>>> Sent: Tuesday, February 7, 2023 6:50 AM
>>>
>>> On Tue, 7 Feb 2023 13:25:13 +0200, Alvaro Karsz <alvaro.karsz@solid-run.com>
>>> wrote:
>>>> Hi Heng,
>>>>
>>>>> Currently, the coalescing profile is directly applied to all queues.
>>>>> This patch supports configuring the parameters for a specified queue.
>>>>>
>>>>> 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>
>>>>> ---
>>>>> content.tex | 49 ++++++++++++++++++++++++++++++++++++++++++-------
>>>>> 1 file changed, 42 insertions(+), 7 deletions(-)
>>>>>
>>>>> diff --git a/content.tex b/content.tex index e863709..049c0e4 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_PERQUEUE_NOTF_COAL(52)] Device supports per-
>>> queue
>>>>> + notifications coalescing.
>>>>> +
>>>> Since this feature allows us to change the coalescing parameters for
>>>> all the queues when rx/tx_qid = 0xFFFF, and since version 1.3 wasn't
>>>> released yet, maybe the "per-vq" functionality can be added to
>>>> VIRTIO_NET_F_NOTF_COAL instead of adding a new feature?
>>>
>>> According to my understanding, all the features of voting are formal. It can be
>>> used by the manufacturer.
>>>
>>> Of course, as far as I know, no manufacturer has used this feature for the time
>>> being. But I think we should add a new feature.
>>>
>>> Or other people have other ideas.
>> I believe we should treat it as fix and avoid a new feature bit as spec is not released, and it is very recent change.
> Arguably it's true.
> It will all be up to the committee vote of course ...
> To keep things a bit safer how about we just allow commands
> without qid instead of a special qid value?

Good idea, the new command seems to make compatibility easier to achieve.
An error can be returned when the old device does not recognize the new
command.

> Also if we are going to change things how about adding a query command too?

Yes, we should.

Thanks.

>
> Also Alvaro what is your take on whether the gloabal version counts
> packets and time for all queues together or per queue? The spec
> does not make it clear ATM.
>

On Wed, Feb 08, 2023 at 10:20:08AM +0800, Heng Qi wrote:
>
>
> ? 2023/2/7 ??10:29, Michael S. Tsirkin ??:
> > On Tue, Feb 07, 2023 at 12:51:26PM +0000, Parav Pandit wrote:
> > > > From: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > > > Sent: Tuesday, February 7, 2023 6:50 AM
> > > >
> > > > On Tue, 7 Feb 2023 13:25:13 +0200, Alvaro Karsz <alvaro.karsz@solid-run.com>
> > > > wrote:
> > > > > Hi Heng,
> > > > >
> > > > > > Currently, the coalescing profile is directly applied to all queues.
> > > > > > This patch supports configuring the parameters for a specified queue.
> > > > > >
> > > > > > 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>
> > > > > > ---
> > > > > > content.tex | 49 ++++++++++++++++++++++++++++++++++++++++++-------
> > > > > > 1 file changed, 42 insertions(+), 7 deletions(-)
> > > > > >
> > > > > > diff --git a/content.tex b/content.tex index e863709..049c0e4 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_PERQUEUE_NOTF_COAL(52)] Device supports per-
> > > > queue
> > > > > > + notifications coalescing.
> > > > > > +
> > > > > Since this feature allows us to change the coalescing parameters for
> > > > > all the queues when rx/tx_qid = 0xFFFF, and since version 1.3 wasn't
> > > > > released yet, maybe the "per-vq" functionality can be added to
> > > > > VIRTIO_NET_F_NOTF_COAL instead of adding a new feature?
> > > >
> > > > According to my understanding, all the features of voting are formal. It can be
> > > > used by the manufacturer.
> > > >
> > > > Of course, as far as I know, no manufacturer has used this feature for the time
> > > > being. But I think we should add a new feature.
> > > >
> > > > Or other people have other ideas.
> > > I believe we should treat it as fix and avoid a new feature bit as spec is not released, and it is very recent change.
> > Arguably it's true.
> > It will all be up to the committee vote of course ...
> > To keep things a bit safer how about we just allow commands
> > without qid instead of a special qid value?
>
> Good idea, the new command seems to make compatibility easier to achieve.
> An error can be returned when the old device does not recognize the new
> command.

Not that is a bad way to figure out what is supported. E.g. driver
can't reasonably probe a ton of commands just to check there's
compatibility for migration. If it's optional pls gate by feature
bit or some such.

> > Also if we are going to change things how about adding a query command too?
>
> Yes, we should.
>
> Thanks.
>
> >
> > Also Alvaro what is your take on whether the gloabal version counts
> > packets and time for all queues together or per queue? The spec
> > does not make it clear ATM.
> >

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Wednesday, February 8, 2023 4:56 AM
>
> On Wed, Feb 08, 2023 at 10:20:08AM +0800, Heng Qi wrote:
> >
> >
> > ? 2023/2/7 ??10:29, Michael S. Tsirkin ??:
> > > On Tue, Feb 07, 2023 at 12:51:26PM +0000, Parav Pandit wrote:
> > > > > From: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > > > > Sent: Tuesday, February 7, 2023 6:50 AM
> > > > >
> > > > > On Tue, 7 Feb 2023 13:25:13 +0200, Alvaro Karsz
> > > > > <alvaro.karsz@solid-run.com>
> > > > > wrote:
> > > > > > Hi Heng,
> > > > > >
> > > > > > > Currently, the coalescing profile is directly applied to all queues.
> > > > > > > This patch supports configuring the parameters for a specified
> queue.
> > > > > > >
> > > > > > > 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>
> > > > > > > ---
> > > > > > > content.tex | 49
> ++++++++++++++++++++++++++++++++++++++++++-------
> > > > > > > 1 file changed, 42 insertions(+), 7 deletions(-)
> > > > > > >
> > > > > > > diff --git a/content.tex b/content.tex index
> > > > > > > e863709..049c0e4 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_PERQUEUE_NOTF_COAL(52)] Device
> supports
> > > > > > > +per-
> > > > > queue
> > > > > > > + notifications coalescing.
> > > > > > > +
> > > > > > Since this feature allows us to change the coalescing
> > > > > > parameters for all the queues when rx/tx_qid = 0xFFFF, and
> > > > > > since version 1.3 wasn't released yet, maybe the "per-vq"
> > > > > > functionality can be added to VIRTIO_NET_F_NOTF_COAL instead of
> adding a new feature?
> > > > >
> > > > > According to my understanding, all the features of voting are
> > > > > formal. It can be used by the manufacturer.
> > > > >
> > > > > Of course, as far as I know, no manufacturer has used this
> > > > > feature for the time being. But I think we should add a new feature.
> > > > >
> > > > > Or other people have other ideas.
> > > > I believe we should treat it as fix and avoid a new feature bit as spec is not
> released, and it is very recent change.
> > > Arguably it's true.
> > > It will all be up to the committee vote of course ...
> > > To keep things a bit safer how about we just allow commands without
> > > qid instead of a special qid value?
> >
> > Good idea, the new command seems to make compatibility easier to achieve.
> > An error can be returned when the old device does not recognize the
> > new command.
>
> Not that is a bad way to figure out what is supported. E.g. driver can't
> reasonably probe a ton of commands just to check there's compatibility for
> migration. If it's optional pls gate by feature bit or some such.

There is no need to issue ton of commands.
One command can return lot of capabilities.
But it is different discussion. Not applicable here.


>
> > > Also if we are going to change things how about adding a query command
> too?
> >
This is useful.

> > Yes, we should.
> >
> > Thanks.
> >
> > >
> > > Also Alvaro what is your take on whether the gloabal version counts
> > > packets and time for all queues together or per queue? The spec does
> > > not make it clear ATM.
> > >

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Wednesday, February 8, 2023 4:56 AM
>
> On Wed, Feb 08, 2023 at 10:20:08AM +0800, Heng Qi wrote:
> >
> >
> > ? 2023/2/7 ??10:29, Michael S. Tsirkin ??:
> > > On Tue, Feb 07, 2023 at 12:51:26PM +0000, Parav Pandit wrote:
> > > > > From: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > > > > Sent: Tuesday, February 7, 2023 6:50 AM
> > > > >
> > > > > On Tue, 7 Feb 2023 13:25:13 +0200, Alvaro Karsz
> > > > > <alvaro.karsz@solid-run.com>
> > > > > wrote:
> > > > > > Hi Heng,
> > > > > >
> > > > > > > Currently, the coalescing profile is directly applied to all queues.
> > > > > > > This patch supports configuring the parameters for a specified
> queue.
> > > > > > >
> > > > > > > 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>
> > > > > > > ---
> > > > > > > content.tex | 49
> ++++++++++++++++++++++++++++++++++++++++++-------
> > > > > > > 1 file changed, 42 insertions(+), 7 deletions(-)
> > > > > > >
> > > > > > > diff --git a/content.tex b/content.tex index
> > > > > > > e863709..049c0e4 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_PERQUEUE_NOTF_COAL(52)] Device
> supports
> > > > > > > +per-
> > > > > queue
> > > > > > > + notifications coalescing.
> > > > > > > +
> > > > > > Since this feature allows us to change the coalescing
> > > > > > parameters for all the queues when rx/tx_qid = 0xFFFF, and
> > > > > > since version 1.3 wasn't released yet, maybe the "per-vq"
> > > > > > functionality can be added to VIRTIO_NET_F_NOTF_COAL instead of
> adding a new feature?
> > > > >
> > > > > According to my understanding, all the features of voting are
> > > > > formal. It can be used by the manufacturer.
> > > > >
> > > > > Of course, as far as I know, no manufacturer has used this
> > > > > feature for the time being. But I think we should add a new feature.
> > > > >
> > > > > Or other people have other ideas.
> > > > I believe we should treat it as fix and avoid a new feature bit as spec is not
> released, and it is very recent change.
> > > Arguably it's true.
> > > It will all be up to the committee vote of course ...
> > > To keep things a bit safer how about we just allow commands without
> > > qid instead of a special qid value?
> >
> > Good idea, the new command seems to make compatibility easier to achieve.
> > An error can be returned when the old device does not recognize the
> > new command.
>
> Not that is a bad way to figure out what is supported. E.g. driver can't
> reasonably probe a ton of commands just to check there's compatibility for
> migration. If it's optional pls gate by feature bit or some such.

There is no need to issue ton of commands.
One command can return lot of capabilities.
But it is different discussion. Not applicable here.


>
> > > Also if we are going to change things how about adding a query command
> too?
> >
This is useful.

> > Yes, we should.
> >
> > Thanks.
> >
> > >
> > > Also Alvaro what is your take on whether the gloabal version counts
> > > packets and time for all queues together or per queue? The spec does
> > > not make it clear ATM.
> > >

On Wed, Feb 08, 2023 at 10:20:08AM +0800, Heng Qi wrote:
>
>
> ? 2023/2/7 ??10:29, Michael S. Tsirkin ??:
> > On Tue, Feb 07, 2023 at 12:51:26PM +0000, Parav Pandit wrote:
> > > > From: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > > > Sent: Tuesday, February 7, 2023 6:50 AM
> > > >
> > > > On Tue, 7 Feb 2023 13:25:13 +0200, Alvaro Karsz <alvaro.karsz@solid-run.com>
> > > > wrote:
> > > > > Hi Heng,
> > > > >
> > > > > > Currently, the coalescing profile is directly applied to all queues.
> > > > > > This patch supports configuring the parameters for a specified queue.
> > > > > >
> > > > > > 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>
> > > > > > ---
> > > > > > content.tex | 49 ++++++++++++++++++++++++++++++++++++++++++-------
> > > > > > 1 file changed, 42 insertions(+), 7 deletions(-)
> > > > > >
> > > > > > diff --git a/content.tex b/content.tex index e863709..049c0e4 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_PERQUEUE_NOTF_COAL(52)] Device supports per-
> > > > queue
> > > > > > + notifications coalescing.
> > > > > > +
> > > > > Since this feature allows us to change the coalescing parameters for
> > > > > all the queues when rx/tx_qid = 0xFFFF, and since version 1.3 wasn't
> > > > > released yet, maybe the "per-vq" functionality can be added to
> > > > > VIRTIO_NET_F_NOTF_COAL instead of adding a new feature?
> > > >
> > > > According to my understanding, all the features of voting are formal. It can be
> > > > used by the manufacturer.
> > > >
> > > > Of course, as far as I know, no manufacturer has used this feature for the time
> > > > being. But I think we should add a new feature.
> > > >
> > > > Or other people have other ideas.
> > > I believe we should treat it as fix and avoid a new feature bit as spec is not released, and it is very recent change.
> > Arguably it's true.
> > It will all be up to the committee vote of course ...
> > To keep things a bit safer how about we just allow commands
> > without qid instead of a special qid value?
>
> Good idea, the new command seems to make compatibility easier to achieve.
> An error can be returned when the old device does not recognize the new
> command.

Not that is a bad way to figure out what is supported. E.g. driver
can't reasonably probe a ton of commands just to check there's
compatibility for migration. If it's optional pls gate by feature
bit or some such.

> > Also if we are going to change things how about adding a query command too?
>
> Yes, we should.
>
> Thanks.
>
> >
> > Also Alvaro what is your take on whether the gloabal version counts
> > packets and time for all queues together or per queue? The spec
> > does not make it clear ATM.
> >

? 2023/2/7 ??10:29, Michael S. Tsirkin ??:
> On Tue, Feb 07, 2023 at 12:51:26PM +0000, Parav Pandit wrote:
>>> From: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
>>> Sent: Tuesday, February 7, 2023 6:50 AM
>>>
>>> On Tue, 7 Feb 2023 13:25:13 +0200, Alvaro Karsz <alvaro.karsz@solid-run.com>
>>> wrote:
>>>> Hi Heng,
>>>>
>>>>> Currently, the coalescing profile is directly applied to all queues.
>>>>> This patch supports configuring the parameters for a specified queue.
>>>>>
>>>>> 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>
>>>>> ---
>>>>> content.tex | 49 ++++++++++++++++++++++++++++++++++++++++++-------
>>>>> 1 file changed, 42 insertions(+), 7 deletions(-)
>>>>>
>>>>> diff --git a/content.tex b/content.tex index e863709..049c0e4 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_PERQUEUE_NOTF_COAL(52)] Device supports per-
>>> queue
>>>>> + notifications coalescing.
>>>>> +
>>>> Since this feature allows us to change the coalescing parameters for
>>>> all the queues when rx/tx_qid = 0xFFFF, and since version 1.3 wasn't
>>>> released yet, maybe the "per-vq" functionality can be added to
>>>> VIRTIO_NET_F_NOTF_COAL instead of adding a new feature?
>>>
>>> According to my understanding, all the features of voting are formal. It can be
>>> used by the manufacturer.
>>>
>>> Of course, as far as I know, no manufacturer has used this feature for the time
>>> being. But I think we should add a new feature.
>>>
>>> Or other people have other ideas.
>> I believe we should treat it as fix and avoid a new feature bit as spec is not released, and it is very recent change.
> Arguably it's true.
> It will all be up to the committee vote of course ...
> To keep things a bit safer how about we just allow commands
> without qid instead of a special qid value?

Good idea, the new command seems to make compatibility easier to achieve.
An error can be returned when the old device does not recognize the new
command.

> Also if we are going to change things how about adding a query command too?

Yes, we should.

Thanks.

>
> Also Alvaro what is your take on whether the gloabal version counts
> packets and time for all queues together or per queue? The spec
> does not make it clear ATM.
>

On Tue, Feb 07, 2023 at 07:16:34PM +0800, Heng Qi wrote:
> Currently, the coalescing profile is directly applied to all queues.
> This patch supports configuring the parameters for a specified queue.
>
> 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.

ethtool does not support this though, does it? what's the plan?

> Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
> Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>

What I dislike about this interface is that if
VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated, then
in the common case of same parameters for all queues
driver has to issue multiple commands.
I can see either a special vq index (0xffff ?) or a special
command used to set it for all queues.


> ---
> content.tex | 49 ++++++++++++++++++++++++++++++++++++++++++-------
> 1 file changed, 42 insertions(+), 7 deletions(-)
>
> diff --git a/content.tex b/content.tex
> index e863709..049c0e4 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_PERQUEUE_NOTF_COAL(52)] Device supports per-queue
> + 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_PERQUEUE_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}
> @@ -4488,16 +4492,21 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
>
> If the VIRTIO_NET_F_NOTF_COAL feature is negotiated, the driver can
> send control commands for dynamically changing the coalescing parameters.
> +If additionally VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated, the driver
> +can send control commands to configure the coalescing parameters of a
> +specified receiveq or transmitq.
>
> \begin{lstlisting}
> struct virtio_net_ctrl_coal_rx {
> le32 rx_max_packets;
> le32 rx_usecs;
> + le16 rx_qid; (Only if VIRTIO_NET_F_PERQUEUE_NOTF_COAL negotiated)
> };
>
> struct virtio_net_ctrl_coal_tx {
> le32 tx_max_packets;
> le32 tx_usecs;
> + le16 tx_qid; (Only if VIRTIO_NET_F_PERQUEUE_NOTF_COAL negotiated)
> };
>
> #define VIRTIO_NET_CTRL_NOTF_COAL 6

I think it's a good idea to do this on top of Alvaro's patch
unifying these two structures.

> @@ -4507,17 +4516,34 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
>
> Coalescing parameters:
> \begin{itemize}
> -\item \field{rx_usecs}: Maximum number of usecs to delay a RX notification.
> -\item \field{tx_usecs}: Maximum number of usecs to delay a TX notification.
> +\item \field{rx_qid}: Index of which receiveq to change the coalescing parameters.
> + If the value is between 0 and 0x7FFF, it represents the index of the specified
> + receiveq. Otherwise, if the value is 0xFFFF, it indicates to change the
> + coalescing parameters for all receiveqs.

what if the index does not map to a receive queue?

> +\item \field{tx_qid}: Index of which transmitq to change the coalescing parameters.
> + If the value is between 0 and 0x7FFF, it represents the index of the specified
> + transmitq. Otherwise, if the value is 0xFFFF, it indicates to change the
> + coalescing parameters for all transmitqs.
> +\item \field{rx_usecs}: Maximum number of usecs to delay a RX notification. If additionally
> + VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated, it works for the specified recieveq
> + or all receieveqs.
> +\item \field{tx_usecs}: Maximum number of usecs to delay a TX notification. If additionally
> + VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated, it works for the specified transmitq
> + or all transmitqs.
> \item \field{rx_max_packets}: Maximum number of packets to receive before a RX notification.
> + If additionally VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated, it works for the
> + specified receiveq or all receiveqs.
> \item \field{tx_max_packets}: Maximum number of packets to send before a TX notification.
> + If additionally VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated, it works for the
> + specified transmitq or all transmitqs.
> \end{itemize}
>
> -
> The class VIRTIO_NET_CTRL_NOTF_COAL has 2 commands:
> \begin{enumerate}
> -\item VIRTIO_NET_CTRL_NOTF_COAL_TX_SET: set the \field{tx_usecs} and \field{tx_max_packets} parameters.
> -\item VIRTIO_NET_CTRL_NOTF_COAL_RX_SET: set the \field{rx_usecs} and \field{rx_max_packets} parameters.
> +\item VIRTIO_NET_CTRL_NOTF_COAL_TX_SET: set the \field{tx_usecs} and \field{tx_max_packets} parameters,
> + and set the \field{tx_qid} if VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated.
> +\item VIRTIO_NET_CTRL_NOTF_COAL_RX_SET: set the \field{rx_usecs} and \field{rx_max_packets} parameters,
> + and set the \field{rx_qid} if VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated.
> \end{enumerate}
>
> \subparagraph{RX Notifications}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Notifications Coalescing / RX Notifications}
> @@ -4531,7 +4557,11 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
> The device will operate as follows:
>
> \begin{itemize}
> -\item The device will count received packets until it accumulates 15, or until 10 usecs elapsed since the first one was received.
> +\item The device will count received packets until it accumulates 15, or until
> + 10 usecs elapsed since the first one was received. If additionally
> + VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated: when \field{rx_qid} is set
> + to 0, the accumulation was only from the receiveq1; when \field{rx_qid} is
> + set to 0xFFFF, the accumulation was from all receiveqs.
> \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}
>
> @@ -4546,7 +4576,12 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
> The device will operate as follows:
>
> \begin{itemize}
> -\item The device will count sent packets until it accumulates 15, or until 10 usecs elapsed since the first one was sent.
> +\item The device will count sent packets on the transmitq1 until it accumulates 15,
> + or until 10 usecs elapsed since the first one was sent. If additionally
> + VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated: when \field{tx_qid} is set
> + to 0, the accumulation was only from the transmitq1; when \field{tx_qid} is
> + set to 0xFFFF, the accumulation was from all transmitqs.
> +
> \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}
>
> --
> 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/

On Tue, Feb 07, 2023 at 07:16:34PM +0800, Heng Qi wrote:
> Currently, the coalescing profile is directly applied to all queues.
> This patch supports configuring the parameters for a specified queue.
>
> 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.

ethtool does not support this though, does it? what's the plan?

> Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
> Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>

What I dislike about this interface is that if
VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated, then
in the common case of same parameters for all queues
driver has to issue multiple commands.
I can see either a special vq index (0xffff ?) or a special
command used to set it for all queues.


> ---
> content.tex | 49 ++++++++++++++++++++++++++++++++++++++++++-------
> 1 file changed, 42 insertions(+), 7 deletions(-)
>
> diff --git a/content.tex b/content.tex
> index e863709..049c0e4 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_PERQUEUE_NOTF_COAL(52)] Device supports per-queue
> + 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_PERQUEUE_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}
> @@ -4488,16 +4492,21 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
>
> If the VIRTIO_NET_F_NOTF_COAL feature is negotiated, the driver can
> send control commands for dynamically changing the coalescing parameters.
> +If additionally VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated, the driver
> +can send control commands to configure the coalescing parameters of a
> +specified receiveq or transmitq.
>
> \begin{lstlisting}
> struct virtio_net_ctrl_coal_rx {
> le32 rx_max_packets;
> le32 rx_usecs;
> + le16 rx_qid; (Only if VIRTIO_NET_F_PERQUEUE_NOTF_COAL negotiated)
> };
>
> struct virtio_net_ctrl_coal_tx {
> le32 tx_max_packets;
> le32 tx_usecs;
> + le16 tx_qid; (Only if VIRTIO_NET_F_PERQUEUE_NOTF_COAL negotiated)
> };
>
> #define VIRTIO_NET_CTRL_NOTF_COAL 6

I think it's a good idea to do this on top of Alvaro's patch
unifying these two structures.

> @@ -4507,17 +4516,34 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
>
> Coalescing parameters:
> \begin{itemize}
> -\item \field{rx_usecs}: Maximum number of usecs to delay a RX notification.
> -\item \field{tx_usecs}: Maximum number of usecs to delay a TX notification.
> +\item \field{rx_qid}: Index of which receiveq to change the coalescing parameters.
> + If the value is between 0 and 0x7FFF, it represents the index of the specified
> + receiveq. Otherwise, if the value is 0xFFFF, it indicates to change the
> + coalescing parameters for all receiveqs.

what if the index does not map to a receive queue?

> +\item \field{tx_qid}: Index of which transmitq to change the coalescing parameters.
> + If the value is between 0 and 0x7FFF, it represents the index of the specified
> + transmitq. Otherwise, if the value is 0xFFFF, it indicates to change the
> + coalescing parameters for all transmitqs.
> +\item \field{rx_usecs}: Maximum number of usecs to delay a RX notification. If additionally
> + VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated, it works for the specified recieveq
> + or all receieveqs.
> +\item \field{tx_usecs}: Maximum number of usecs to delay a TX notification. If additionally
> + VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated, it works for the specified transmitq
> + or all transmitqs.
> \item \field{rx_max_packets}: Maximum number of packets to receive before a RX notification.
> + If additionally VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated, it works for the
> + specified receiveq or all receiveqs.
> \item \field{tx_max_packets}: Maximum number of packets to send before a TX notification.
> + If additionally VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated, it works for the
> + specified transmitq or all transmitqs.
> \end{itemize}
>
> -
> The class VIRTIO_NET_CTRL_NOTF_COAL has 2 commands:
> \begin{enumerate}
> -\item VIRTIO_NET_CTRL_NOTF_COAL_TX_SET: set the \field{tx_usecs} and \field{tx_max_packets} parameters.
> -\item VIRTIO_NET_CTRL_NOTF_COAL_RX_SET: set the \field{rx_usecs} and \field{rx_max_packets} parameters.
> +\item VIRTIO_NET_CTRL_NOTF_COAL_TX_SET: set the \field{tx_usecs} and \field{tx_max_packets} parameters,
> + and set the \field{tx_qid} if VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated.
> +\item VIRTIO_NET_CTRL_NOTF_COAL_RX_SET: set the \field{rx_usecs} and \field{rx_max_packets} parameters,
> + and set the \field{rx_qid} if VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated.
> \end{enumerate}
>
> \subparagraph{RX Notifications}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Notifications Coalescing / RX Notifications}
> @@ -4531,7 +4557,11 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
> The device will operate as follows:
>
> \begin{itemize}
> -\item The device will count received packets until it accumulates 15, or until 10 usecs elapsed since the first one was received.
> +\item The device will count received packets until it accumulates 15, or until
> + 10 usecs elapsed since the first one was received. If additionally
> + VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated: when \field{rx_qid} is set
> + to 0, the accumulation was only from the receiveq1; when \field{rx_qid} is
> + set to 0xFFFF, the accumulation was from all receiveqs.
> \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}
>
> @@ -4546,7 +4576,12 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
> The device will operate as follows:
>
> \begin{itemize}
> -\item The device will count sent packets until it accumulates 15, or until 10 usecs elapsed since the first one was sent.
> +\item The device will count sent packets on the transmitq1 until it accumulates 15,
> + or until 10 usecs elapsed since the first one was sent. If additionally
> + VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated: when \field{tx_qid} is set
> + to 0, the accumulation was only from the transmitq1; when \field{tx_qid} is
> + set to 0xFFFF, the accumulation was from all transmitqs.
> +
> \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}
>
> --
> 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/

On Tue, Feb 07, 2023 at 09:06:24AM -0500, Michael S. Tsirkin wrote:
> On Tue, Feb 07, 2023 at 07:16:34PM +0800, Heng Qi wrote:
> > Currently, the coalescing profile is directly applied to all queues.
> > This patch supports configuring the parameters for a specified queue.
> >
> > 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.
>
> ethtool does not support this though, does it? what's the plan?
>
> > Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
> > Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
>
> What I dislike about this interface is that if
> VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated, then
> in the common case of same parameters for all queues
> driver has to issue multiple commands.
> I can see either a special vq index (0xffff ?) or a special
> command used to set it for all queues.

Oh wait this is exactly what this patch did. Donnu why I missed this -
not enough coffee? Sorry pls ignore this part.


>
> > ---
> > content.tex | 49 ++++++++++++++++++++++++++++++++++++++++++-------
> > 1 file changed, 42 insertions(+), 7 deletions(-)
> >
> > diff --git a/content.tex b/content.tex
> > index e863709..049c0e4 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_PERQUEUE_NOTF_COAL(52)] Device supports per-queue
> > + 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_PERQUEUE_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}
> > @@ -4488,16 +4492,21 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
> >
> > If the VIRTIO_NET_F_NOTF_COAL feature is negotiated, the driver can
> > send control commands for dynamically changing the coalescing parameters.
> > +If additionally VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated, the driver
> > +can send control commands to configure the coalescing parameters of a
> > +specified receiveq or transmitq.
> >
> > \begin{lstlisting}
> > struct virtio_net_ctrl_coal_rx {
> > le32 rx_max_packets;
> > le32 rx_usecs;
> > + le16 rx_qid; (Only if VIRTIO_NET_F_PERQUEUE_NOTF_COAL negotiated)
> > };
> >
> > struct virtio_net_ctrl_coal_tx {
> > le32 tx_max_packets;
> > le32 tx_usecs;
> > + le16 tx_qid; (Only if VIRTIO_NET_F_PERQUEUE_NOTF_COAL negotiated)
> > };
> >
> > #define VIRTIO_NET_CTRL_NOTF_COAL 6
>
> I think it's a good idea to do this on top of Alvaro's patch
> unifying these two structures.
>
> > @@ -4507,17 +4516,34 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
> >
> > Coalescing parameters:
> > \begin{itemize}
> > -\item \field{rx_usecs}: Maximum number of usecs to delay a RX notification.
> > -\item \field{tx_usecs}: Maximum number of usecs to delay a TX notification.
> > +\item \field{rx_qid}: Index of which receiveq to change the coalescing parameters.
> > + If the value is between 0 and 0x7FFF, it represents the index of the specified
> > + receiveq. Otherwise, if the value is 0xFFFF, it indicates to change the
> > + coalescing parameters for all receiveqs.
>
> what if the index does not map to a receive queue?
>
> > +\item \field{tx_qid}: Index of which transmitq to change the coalescing parameters.
> > + If the value is between 0 and 0x7FFF, it represents the index of the specified
> > + transmitq. Otherwise, if the value is 0xFFFF, it indicates to change the
> > + coalescing parameters for all transmitqs.
> > +\item \field{rx_usecs}: Maximum number of usecs to delay a RX notification. If additionally
> > + VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated, it works for the specified recieveq
> > + or all receieveqs.
> > +\item \field{tx_usecs}: Maximum number of usecs to delay a TX notification. If additionally
> > + VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated, it works for the specified transmitq
> > + or all transmitqs.
> > \item \field{rx_max_packets}: Maximum number of packets to receive before a RX notification.
> > + If additionally VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated, it works for the
> > + specified receiveq or all receiveqs.
> > \item \field{tx_max_packets}: Maximum number of packets to send before a TX notification.
> > + If additionally VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated, it works for the
> > + specified transmitq or all transmitqs.
> > \end{itemize}
> >
> > -
> > The class VIRTIO_NET_CTRL_NOTF_COAL has 2 commands:
> > \begin{enumerate}
> > -\item VIRTIO_NET_CTRL_NOTF_COAL_TX_SET: set the \field{tx_usecs} and \field{tx_max_packets} parameters.
> > -\item VIRTIO_NET_CTRL_NOTF_COAL_RX_SET: set the \field{rx_usecs} and \field{rx_max_packets} parameters.
> > +\item VIRTIO_NET_CTRL_NOTF_COAL_TX_SET: set the \field{tx_usecs} and \field{tx_max_packets} parameters,
> > + and set the \field{tx_qid} if VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated.
> > +\item VIRTIO_NET_CTRL_NOTF_COAL_RX_SET: set the \field{rx_usecs} and \field{rx_max_packets} parameters,
> > + and set the \field{rx_qid} if VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated.
> > \end{enumerate}
> >
> > \subparagraph{RX Notifications}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Notifications Coalescing / RX Notifications}
> > @@ -4531,7 +4557,11 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
> > The device will operate as follows:
> >
> > \begin{itemize}
> > -\item The device will count received packets until it accumulates 15, or until 10 usecs elapsed since the first one was received.
> > +\item The device will count received packets until it accumulates 15, or until
> > + 10 usecs elapsed since the first one was received. If additionally
> > + VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated: when \field{rx_qid} is set
> > + to 0, the accumulation was only from the receiveq1; when \field{rx_qid} is
> > + set to 0xFFFF, the accumulation was from all receiveqs.
> > \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}
> >
> > @@ -4546,7 +4576,12 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
> > The device will operate as follows:
> >
> > \begin{itemize}
> > -\item The device will count sent packets until it accumulates 15, or until 10 usecs elapsed since the first one was sent.
> > +\item The device will count sent packets on the transmitq1 until it accumulates 15,
> > + or until 10 usecs elapsed since the first one was sent. If additionally
> > + VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated: when \field{tx_qid} is set
> > + to 0, the accumulation was only from the transmitq1; when \field{tx_qid} is
> > + set to 0xFFFF, the accumulation was from all transmitqs.
> > +
> > \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}
> >
> > --
> > 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/

On Tue, Feb 07, 2023 at 09:06:24AM -0500, Michael S. Tsirkin wrote:
> On Tue, Feb 07, 2023 at 07:16:34PM +0800, Heng Qi wrote:
> > Currently, the coalescing profile is directly applied to all queues.
> > This patch supports configuring the parameters for a specified queue.
> >
> > 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.
>
> ethtool does not support this though, does it? what's the plan?
>
> > Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
> > Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
>
> What I dislike about this interface is that if
> VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated, then
> in the common case of same parameters for all queues
> driver has to issue multiple commands.
> I can see either a special vq index (0xffff ?) or a special
> command used to set it for all queues.

Oh wait this is exactly what this patch did. Donnu why I missed this -
not enough coffee? Sorry pls ignore this part.


>
> > ---
> > content.tex | 49 ++++++++++++++++++++++++++++++++++++++++++-------
> > 1 file changed, 42 insertions(+), 7 deletions(-)
> >
> > diff --git a/content.tex b/content.tex
> > index e863709..049c0e4 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_PERQUEUE_NOTF_COAL(52)] Device supports per-queue
> > + 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_PERQUEUE_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}
> > @@ -4488,16 +4492,21 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
> >
> > If the VIRTIO_NET_F_NOTF_COAL feature is negotiated, the driver can
> > send control commands for dynamically changing the coalescing parameters.
> > +If additionally VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated, the driver
> > +can send control commands to configure the coalescing parameters of a
> > +specified receiveq or transmitq.
> >
> > \begin{lstlisting}
> > struct virtio_net_ctrl_coal_rx {
> > le32 rx_max_packets;
> > le32 rx_usecs;
> > + le16 rx_qid; (Only if VIRTIO_NET_F_PERQUEUE_NOTF_COAL negotiated)
> > };
> >
> > struct virtio_net_ctrl_coal_tx {
> > le32 tx_max_packets;
> > le32 tx_usecs;
> > + le16 tx_qid; (Only if VIRTIO_NET_F_PERQUEUE_NOTF_COAL negotiated)
> > };
> >
> > #define VIRTIO_NET_CTRL_NOTF_COAL 6
>
> I think it's a good idea to do this on top of Alvaro's patch
> unifying these two structures.
>
> > @@ -4507,17 +4516,34 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
> >
> > Coalescing parameters:
> > \begin{itemize}
> > -\item \field{rx_usecs}: Maximum number of usecs to delay a RX notification.
> > -\item \field{tx_usecs}: Maximum number of usecs to delay a TX notification.
> > +\item \field{rx_qid}: Index of which receiveq to change the coalescing parameters.
> > + If the value is between 0 and 0x7FFF, it represents the index of the specified
> > + receiveq. Otherwise, if the value is 0xFFFF, it indicates to change the
> > + coalescing parameters for all receiveqs.
>
> what if the index does not map to a receive queue?
>
> > +\item \field{tx_qid}: Index of which transmitq to change the coalescing parameters.
> > + If the value is between 0 and 0x7FFF, it represents the index of the specified
> > + transmitq. Otherwise, if the value is 0xFFFF, it indicates to change the
> > + coalescing parameters for all transmitqs.
> > +\item \field{rx_usecs}: Maximum number of usecs to delay a RX notification. If additionally
> > + VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated, it works for the specified recieveq
> > + or all receieveqs.
> > +\item \field{tx_usecs}: Maximum number of usecs to delay a TX notification. If additionally
> > + VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated, it works for the specified transmitq
> > + or all transmitqs.
> > \item \field{rx_max_packets}: Maximum number of packets to receive before a RX notification.
> > + If additionally VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated, it works for the
> > + specified receiveq or all receiveqs.
> > \item \field{tx_max_packets}: Maximum number of packets to send before a TX notification.
> > + If additionally VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated, it works for the
> > + specified transmitq or all transmitqs.
> > \end{itemize}
> >
> > -
> > The class VIRTIO_NET_CTRL_NOTF_COAL has 2 commands:
> > \begin{enumerate}
> > -\item VIRTIO_NET_CTRL_NOTF_COAL_TX_SET: set the \field{tx_usecs} and \field{tx_max_packets} parameters.
> > -\item VIRTIO_NET_CTRL_NOTF_COAL_RX_SET: set the \field{rx_usecs} and \field{rx_max_packets} parameters.
> > +\item VIRTIO_NET_CTRL_NOTF_COAL_TX_SET: set the \field{tx_usecs} and \field{tx_max_packets} parameters,
> > + and set the \field{tx_qid} if VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated.
> > +\item VIRTIO_NET_CTRL_NOTF_COAL_RX_SET: set the \field{rx_usecs} and \field{rx_max_packets} parameters,
> > + and set the \field{rx_qid} if VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated.
> > \end{enumerate}
> >
> > \subparagraph{RX Notifications}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Notifications Coalescing / RX Notifications}
> > @@ -4531,7 +4557,11 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
> > The device will operate as follows:
> >
> > \begin{itemize}
> > -\item The device will count received packets until it accumulates 15, or until 10 usecs elapsed since the first one was received.
> > +\item The device will count received packets until it accumulates 15, or until
> > + 10 usecs elapsed since the first one was received. If additionally
> > + VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated: when \field{rx_qid} is set
> > + to 0, the accumulation was only from the receiveq1; when \field{rx_qid} is
> > + set to 0xFFFF, the accumulation was from all receiveqs.
> > \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}
> >
> > @@ -4546,7 +4576,12 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
> > The device will operate as follows:
> >
> > \begin{itemize}
> > -\item The device will count sent packets until it accumulates 15, or until 10 usecs elapsed since the first one was sent.
> > +\item The device will count sent packets on the transmitq1 until it accumulates 15,
> > + or until 10 usecs elapsed since the first one was sent. If additionally
> > + VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated: when \field{tx_qid} is set
> > + to 0, the accumulation was only from the transmitq1; when \field{tx_qid} is
> > + set to 0xFFFF, the accumulation was from all transmitqs.
> > +
> > \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}
> >
> > --
> > 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/

On Tue, 7 Feb 2023 09:06:19 -0500, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Tue, Feb 07, 2023 at 07:16:34PM +0800, Heng Qi wrote:
> > Currently, the coalescing profile is directly applied to all queues.
> > This patch supports configuring the parameters for a specified queue.
> >
> > 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.
>
> ethtool does not support this though, does it? what's the plan?


Although it can be done, I think it is difficult to let users use ethtool to
modify the parameters of each queue.

At present ethtool supports adaptive-rx/adaptive-tx. This is that the driver
automatically adapted to the appropriate parameter. Generally, it is implemented
using netdim in the driver. At this time, this interface is needed.

>
> > Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
> > Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
>
> What I dislike about this interface is that if
> VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated, then
> in the common case of same parameters for all queues
> driver has to issue multiple commands.
> I can see either a special vq index (0xffff ?) or a special
> command used to set it for all queues.

Although the structure is very similar, in fact, adding a new command may be
clearer.

Thanks.

>
>
> > ---
> > content.tex | 49 ++++++++++++++++++++++++++++++++++++++++++-------
> > 1 file changed, 42 insertions(+), 7 deletions(-)
> >
> > diff --git a/content.tex b/content.tex
> > index e863709..049c0e4 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_PERQUEUE_NOTF_COAL(52)] Device supports per-queue
> > + 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_PERQUEUE_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}
> > @@ -4488,16 +4492,21 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
> >
> > If the VIRTIO_NET_F_NOTF_COAL feature is negotiated, the driver can
> > send control commands for dynamically changing the coalescing parameters.
> > +If additionally VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated, the driver
> > +can send control commands to configure the coalescing parameters of a
> > +specified receiveq or transmitq.
> >
> > \begin{lstlisting}
> > struct virtio_net_ctrl_coal_rx {
> > le32 rx_max_packets;
> > le32 rx_usecs;
> > + le16 rx_qid; (Only if VIRTIO_NET_F_PERQUEUE_NOTF_COAL negotiated)
> > };
> >
> > struct virtio_net_ctrl_coal_tx {
> > le32 tx_max_packets;
> > le32 tx_usecs;
> > + le16 tx_qid; (Only if VIRTIO_NET_F_PERQUEUE_NOTF_COAL negotiated)
> > };
> >
> > #define VIRTIO_NET_CTRL_NOTF_COAL 6
>
> I think it's a good idea to do this on top of Alvaro's patch
> unifying these two structures.
>
> > @@ -4507,17 +4516,34 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
> >
> > Coalescing parameters:
> > \begin{itemize}
> > -\item \field{rx_usecs}: Maximum number of usecs to delay a RX notification.
> > -\item \field{tx_usecs}: Maximum number of usecs to delay a TX notification.
> > +\item \field{rx_qid}: Index of which receiveq to change the coalescing parameters.
> > + If the value is between 0 and 0x7FFF, it represents the index of the specified
> > + receiveq. Otherwise, if the value is 0xFFFF, it indicates to change the
> > + coalescing parameters for all receiveqs.
>
> what if the index does not map to a receive queue?
>
> > +\item \field{tx_qid}: Index of which transmitq to change the coalescing parameters.
> > + If the value is between 0 and 0x7FFF, it represents the index of the specified
> > + transmitq. Otherwise, if the value is 0xFFFF, it indicates to change the
> > + coalescing parameters for all transmitqs.
> > +\item \field{rx_usecs}: Maximum number of usecs to delay a RX notification. If additionally
> > + VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated, it works for the specified recieveq
> > + or all receieveqs.
> > +\item \field{tx_usecs}: Maximum number of usecs to delay a TX notification. If additionally
> > + VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated, it works for the specified transmitq
> > + or all transmitqs.
> > \item \field{rx_max_packets}: Maximum number of packets to receive before a RX notification.
> > + If additionally VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated, it works for the
> > + specified receiveq or all receiveqs.
> > \item \field{tx_max_packets}: Maximum number of packets to send before a TX notification.
> > + If additionally VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated, it works for the
> > + specified transmitq or all transmitqs.
> > \end{itemize}
> >
> > -
> > The class VIRTIO_NET_CTRL_NOTF_COAL has 2 commands:
> > \begin{enumerate}
> > -\item VIRTIO_NET_CTRL_NOTF_COAL_TX_SET: set the \field{tx_usecs} and \field{tx_max_packets} parameters.
> > -\item VIRTIO_NET_CTRL_NOTF_COAL_RX_SET: set the \field{rx_usecs} and \field{rx_max_packets} parameters.
> > +\item VIRTIO_NET_CTRL_NOTF_COAL_TX_SET: set the \field{tx_usecs} and \field{tx_max_packets} parameters,
> > + and set the \field{tx_qid} if VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated.
> > +\item VIRTIO_NET_CTRL_NOTF_COAL_RX_SET: set the \field{rx_usecs} and \field{rx_max_packets} parameters,
> > + and set the \field{rx_qid} if VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated.
> > \end{enumerate}
> >
> > \subparagraph{RX Notifications}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Notifications Coalescing / RX Notifications}
> > @@ -4531,7 +4557,11 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
> > The device will operate as follows:
> >
> > \begin{itemize}
> > -\item The device will count received packets until it accumulates 15, or until 10 usecs elapsed since the first one was received.
> > +\item The device will count received packets until it accumulates 15, or until
> > + 10 usecs elapsed since the first one was received. If additionally
> > + VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated: when \field{rx_qid} is set
> > + to 0, the accumulation was only from the receiveq1; when \field{rx_qid} is
> > + set to 0xFFFF, the accumulation was from all receiveqs.
> > \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}
> >
> > @@ -4546,7 +4576,12 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
> > The device will operate as follows:
> >
> > \begin{itemize}
> > -\item The device will count sent packets until it accumulates 15, or until 10 usecs elapsed since the first one was sent.
> > +\item The device will count sent packets on the transmitq1 until it accumulates 15,
> > + or until 10 usecs elapsed since the first one was sent. If additionally
> > + VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated: when \field{tx_qid} is set
> > + to 0, the accumulation was only from the transmitq1; when \field{tx_qid} is
> > + set to 0xFFFF, the accumulation was from all transmitqs.
> > +
> > \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}
> >
> > --
> > 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/
>

> From: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> Sent: Tuesday, February 7, 2023 8:46 PM
>
> On Tue, 7 Feb 2023 09:06:19 -0500, "Michael S. Tsirkin" <mst@redhat.com>
> wrote:
> > On Tue, Feb 07, 2023 at 07:16:34PM +0800, Heng Qi wrote:
> > > Currently, the coalescing profile is directly applied to all queues.
> > > This patch supports configuring the parameters for a specified queue.
> > >
> > > 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.
> >
> > ethtool does not support this though, does it? what's the plan?
>
>
> Although it can be done, I think it is difficult to let users use ethtool to modify
> the parameters of each queue.
>
> At present ethtool supports adaptive-rx/adaptive-tx. This is that the driver
> automatically adapted to the appropriate parameter. Generally, it is
> implemented using netdim in the driver. At this time, this interface is needed.
>
> >
> > > Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
> > > Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> >
> > What I dislike about this interface is that if
> > VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated, then in the common
> case
> > of same parameters for all queues driver has to issue multiple
> > commands.
> > I can see either a special vq index (0xffff ?) or a special command
> > used to set it for all queues.
>
> Although the structure is very similar, in fact, adding a new command may be
> clearer.

O(N) loop is not that bad when user want to issue change it per device, as this is something done very often.
Once per VQ is supported, driver may just use the default net-dim to have best out of box experience, whenever device supports it.

On Wed, 8 Feb 2023 02:20:27 +0000, Parav Pandit <parav@nvidia.com> wrote:
>
> > From: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > Sent: Tuesday, February 7, 2023 8:46 PM
> >
> > On Tue, 7 Feb 2023 09:06:19 -0500, "Michael S. Tsirkin" <mst@redhat.com>
> > wrote:
> > > On Tue, Feb 07, 2023 at 07:16:34PM +0800, Heng Qi wrote:
> > > > Currently, the coalescing profile is directly applied to all queues.
> > > > This patch supports configuring the parameters for a specified queue.
> > > >
> > > > 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.
> > >
> > > ethtool does not support this though, does it? what's the plan?
> >
> >
> > Although it can be done, I think it is difficult to let users use ethtool to modify
> > the parameters of each queue.
> >
> > At present ethtool supports adaptive-rx/adaptive-tx. This is that the driver
> > automatically adapted to the appropriate parameter. Generally, it is
> > implemented using netdim in the driver. At this time, this interface is needed.
> >
> > >
> > > > Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
> > > > Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > >
> > > What I dislike about this interface is that if
> > > VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated, then in the common
> > case
> > > of same parameters for all queues driver has to issue multiple
> > > commands.
> > > I can see either a special vq index (0xffff ?) or a special command
> > > used to set it for all queues.
> >
> > Although the structure is very similar, in fact, adding a new command may be
> > clearer.
>
> O(N) loop is not that bad when user want to issue change it per device, as this is something done very often.
> Once per VQ is supported, driver may just use the default net-dim to have best out of box experience, whenever device supports it.


So you mean, we only support the parameters based on Per-Queue. My original idea
is to support two methods at the same time.

Thanks.

> From: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> Sent: Tuesday, February 7, 2023 9:25 PM
>
> On Wed, 8 Feb 2023 02:20:27 +0000, Parav Pandit <parav@nvidia.com> wrote:
> >
> > > From: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > > Sent: Tuesday, February 7, 2023 8:46 PM
> > >
> > > On Tue, 7 Feb 2023 09:06:19 -0500, "Michael S. Tsirkin"
> > > <mst@redhat.com>
> > > wrote:
> > > > On Tue, Feb 07, 2023 at 07:16:34PM +0800, Heng Qi wrote:
> > > > > Currently, the coalescing profile is directly applied to all queues.
> > > > > This patch supports configuring the parameters for a specified queue.
> > > > >
> > > > > 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.
> > > >
> > > > ethtool does not support this though, does it? what's the plan?
> > >
> > >
> > > Although it can be done, I think it is difficult to let users use
> > > ethtool to modify the parameters of each queue.
> > >
> > > At present ethtool supports adaptive-rx/adaptive-tx. This is that
> > > the driver automatically adapted to the appropriate parameter.
> > > Generally, it is implemented using netdim in the driver. At this time, this
> interface is needed.
> > >
> > > >
> > > > > Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
> > > > > Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > > >
> > > > What I dislike about this interface is that if
> > > > VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated, then in the
> common
> > > case
> > > > of same parameters for all queues driver has to issue multiple
> > > > commands.
> > > > I can see either a special vq index (0xffff ?) or a special
> > > > command used to set it for all queues.
> > >
> > > Although the structure is very similar, in fact, adding a new
> > > command may be clearer.
> >
> > O(N) loop is not that bad when user want to issue change it per device, as this
> is something done very often.
> > Once per VQ is supported, driver may just use the default net-dim to have
> best out of box experience, whenever device supports it.
>
>
> So you mean, we only support the parameters based on Per-Queue. My
> original idea is to support two methods at the same time.

Yes, if we want to support both the modes, that better to have such command.
Because otherwise the device implementation is bit clumsy which needs to do the guess work.
For example, driver has configured global params per device.
Now suddenly driver configured first queues parameter.
At this point device should run N-1 queues using global mode and 1 queue with per Q param.

Similar sequence also occurs when there is per q params configured and suddenly driver configures global param.
Even in this case now device either must iterate internally and move per q to global values.

Better to avoid such complexity in device around implicit and confusing behavior.

I see two options.
1. Just have per VQ params. Software has the full knowledge of in which it is operating, and state remains at software level.
This effectively achieves both the mode.

2. Have a mode cmd,
Mode = (a) per device or (b) per VQ (c) disable
After the mode is set, driver can set per device or per VQ.

On Wed, Feb 08, 2023 at 02:43:02AM +0000, Parav Pandit wrote:
>
> > From: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > Sent: Tuesday, February 7, 2023 9:25 PM
> >
> > On Wed, 8 Feb 2023 02:20:27 +0000, Parav Pandit <parav@nvidia.com> wrote:
> > >
> > > > From: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > > > Sent: Tuesday, February 7, 2023 8:46 PM
> > > >
> > > > On Tue, 7 Feb 2023 09:06:19 -0500, "Michael S. Tsirkin"
> > > > <mst@redhat.com>
> > > > wrote:
> > > > > On Tue, Feb 07, 2023 at 07:16:34PM +0800, Heng Qi wrote:
> > > > > > Currently, the coalescing profile is directly applied to all queues.
> > > > > > This patch supports configuring the parameters for a specified queue.
> > > > > >
> > > > > > 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.
> > > > >
> > > > > ethtool does not support this though, does it? what's the plan?
> > > >
> > > >
> > > > Although it can be done, I think it is difficult to let users use
> > > > ethtool to modify the parameters of each queue.
> > > >
> > > > At present ethtool supports adaptive-rx/adaptive-tx. This is that
> > > > the driver automatically adapted to the appropriate parameter.
> > > > Generally, it is implemented using netdim in the driver. At this time, this
> > interface is needed.
> > > >
> > > > >
> > > > > > Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
> > > > > > Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > > > >
> > > > > What I dislike about this interface is that if
> > > > > VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated, then in the
> > common
> > > > case
> > > > > of same parameters for all queues driver has to issue multiple
> > > > > commands.
> > > > > I can see either a special vq index (0xffff ?) or a special
> > > > > command used to set it for all queues.
> > > >
> > > > Although the structure is very similar, in fact, adding a new
> > > > command may be clearer.
> > >
> > > O(N) loop is not that bad when user want to issue change it per device, as this
> > is something done very often.
> > > Once per VQ is supported, driver may just use the default net-dim to have
> > best out of box experience, whenever device supports it.
> >
> >
> > So you mean, we only support the parameters based on Per-Queue. My
> > original idea is to support two methods at the same time.
>
> Yes, if we want to support both the modes, that better to have such command.
> Because otherwise the device implementation is bit clumsy which needs to do the guess work.
> For example, driver has configured global params per device.
> Now suddenly driver configured first queues parameter.
> At this point device should run N-1 queues using global mode and 1 queue with per Q param.

Seems fine to me.

> Similar sequence also occurs when there is per q params configured and suddenly driver configures global param.

In this case I feel global one should win. I think global is just a
shortcut for running per queue.

> Even in this case now device either must iterate internally and move per q to global values.
>
> Better to avoid such complexity in device around implicit and confusing behavior.
>
> I see two options.
> 1. Just have per VQ params. Software has the full knowledge of in which it is operating, and state remains at software level.
> This effectively achieves both the mode.
>
> 2. Have a mode cmd,
> Mode = (a) per device or (b) per VQ (c) disable
> After the mode is set, driver can set per device or per VQ.

I like 2 better. I also think a and b are enough. c can be achieved through a.

--
MST

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Wednesday, February 8, 2023 5:07 AM
> >
> > 2. Have a mode cmd,
> > Mode = (a) per device or (b) per VQ (c) disable After the mode is set,
> > driver can set per device or per VQ.
>
> I like 2 better. I also think a and b are enough. c can be achieved through a.
>
Yep. (c) is just to indicate that its disabled via the mode in a command.

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Wednesday, February 8, 2023 5:07 AM
> >
> > 2. Have a mode cmd,
> > Mode = (a) per device or (b) per VQ (c) disable After the mode is set,
> > driver can set per device or per VQ.
>
> I like 2 better. I also think a and b are enough. c can be achieved through a.
>
Yep. (c) is just to indicate that its disabled via the mode in a command.

On Wed, Feb 08, 2023 at 02:43:02AM +0000, Parav Pandit wrote:
>
> > From: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > Sent: Tuesday, February 7, 2023 9:25 PM
> >
> > On Wed, 8 Feb 2023 02:20:27 +0000, Parav Pandit <parav@nvidia.com> wrote:
> > >
> > > > From: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > > > Sent: Tuesday, February 7, 2023 8:46 PM
> > > >
> > > > On Tue, 7 Feb 2023 09:06:19 -0500, "Michael S. Tsirkin"
> > > > <mst@redhat.com>
> > > > wrote:
> > > > > On Tue, Feb 07, 2023 at 07:16:34PM +0800, Heng Qi wrote:
> > > > > > Currently, the coalescing profile is directly applied to all queues.
> > > > > > This patch supports configuring the parameters for a specified queue.
> > > > > >
> > > > > > 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.
> > > > >
> > > > > ethtool does not support this though, does it? what's the plan?
> > > >
> > > >
> > > > Although it can be done, I think it is difficult to let users use
> > > > ethtool to modify the parameters of each queue.
> > > >
> > > > At present ethtool supports adaptive-rx/adaptive-tx. This is that
> > > > the driver automatically adapted to the appropriate parameter.
> > > > Generally, it is implemented using netdim in the driver. At this time, this
> > interface is needed.
> > > >
> > > > >
> > > > > > Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
> > > > > > Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > > > >
> > > > > What I dislike about this interface is that if
> > > > > VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated, then in the
> > common
> > > > case
> > > > > of same parameters for all queues driver has to issue multiple
> > > > > commands.
> > > > > I can see either a special vq index (0xffff ?) or a special
> > > > > command used to set it for all queues.
> > > >
> > > > Although the structure is very similar, in fact, adding a new
> > > > command may be clearer.
> > >
> > > O(N) loop is not that bad when user want to issue change it per device, as this
> > is something done very often.
> > > Once per VQ is supported, driver may just use the default net-dim to have
> > best out of box experience, whenever device supports it.
> >
> >
> > So you mean, we only support the parameters based on Per-Queue. My
> > original idea is to support two methods at the same time.
>
> Yes, if we want to support both the modes, that better to have such command.
> Because otherwise the device implementation is bit clumsy which needs to do the guess work.
> For example, driver has configured global params per device.
> Now suddenly driver configured first queues parameter.
> At this point device should run N-1 queues using global mode and 1 queue with per Q param.

Seems fine to me.

> Similar sequence also occurs when there is per q params configured and suddenly driver configures global param.

In this case I feel global one should win. I think global is just a
shortcut for running per queue.

> Even in this case now device either must iterate internally and move per q to global values.
>
> Better to avoid such complexity in device around implicit and confusing behavior.
>
> I see two options.
> 1. Just have per VQ params. Software has the full knowledge of in which it is operating, and state remains at software level.
> This effectively achieves both the mode.
>
> 2. Have a mode cmd,
> Mode = (a) per device or (b) per VQ (c) disable
> After the mode is set, driver can set per device or per VQ.

I like 2 better. I also think a and b are enough. c can be achieved through a.

--
MST

? 2023/2/8 ??10:43, Parav Pandit ??:
>> From: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
>> Sent: Tuesday, February 7, 2023 9:25 PM
>>
>> On Wed, 8 Feb 2023 02:20:27 +0000, Parav Pandit <parav@nvidia.com> wrote:
>>>> From: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
>>>> Sent: Tuesday, February 7, 2023 8:46 PM
>>>>
>>>> On Tue, 7 Feb 2023 09:06:19 -0500, "Michael S. Tsirkin"
>>>> <mst@redhat.com>
>>>> wrote:
>>>>> On Tue, Feb 07, 2023 at 07:16:34PM +0800, Heng Qi wrote:
>>>>>> Currently, the coalescing profile is directly applied to all queues.
>>>>>> This patch supports configuring the parameters for a specified queue.
>>>>>>
>>>>>> 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.
>>>>> ethtool does not support this though, does it? what's the plan?
>>>>
>>>> Although it can be done, I think it is difficult to let users use
>>>> ethtool to modify the parameters of each queue.
>>>>
>>>> At present ethtool supports adaptive-rx/adaptive-tx. This is that
>>>> the driver automatically adapted to the appropriate parameter.
>>>> Generally, it is implemented using netdim in the driver. At this time, this
>> interface is needed.
>>>>>> Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
>>>>>> Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
>>>>> What I dislike about this interface is that if
>>>>> VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated, then in the
>> common
>>>> case
>>>>> of same parameters for all queues driver has to issue multiple
>>>>> commands.
>>>>> I can see either a special vq index (0xffff ?) or a special
>>>>> command used to set it for all queues.
>>>> Although the structure is very similar, in fact, adding a new
>>>> command may be clearer.
>>> O(N) loop is not that bad when user want to issue change it per device, as this
>> is something done very often.
>>> Once per VQ is supported, driver may just use the default net-dim to have
>> best out of box experience, whenever device supports it.
>>
>>
>> So you mean, we only support the parameters based on Per-Queue. My
>> original idea is to support two methods at the same time.
> Yes, if we want to support both the modes, that better to have such command.
> Because otherwise the device implementation is bit clumsy which needs to do the guess work.

I think whether it is a per-queue parameters or a per-device parameters,
the latter should override the first, because we didn't make too many
guesses about which is more important.

>
> Better to avoid such complexity in device around implicit and confusing behavior.
>
> I see two options.
> 1. Just have per VQ params. Software has the full knowledge of in which it is operating, and state remains at software level.
> This effectively achieves both the mode.
>
> 2. Have a mode cmd,
> Mode = (a) per device or (b) per VQ (c) disable
> After the mode is set, driver can set per device or per VQ.

I find this more clear.

Thanks.

On Wed, Feb 08, 2023 at 07:30:34PM +0800, Heng Qi wrote:
> > I see two options.
> > 1. Just have per VQ params. Software has the full knowledge of in which it is operating, and state remains at software level.
> > This effectively achieves both the mode.
> >
> > 2. Have a mode cmd,
> > Mode = (a) per device or (b) per VQ (c) disable
> > After the mode is set, driver can set per device or per VQ.
>
> I find this more clear.
>
> Thanks.
>

Rereading this I think I misunderstood the proposal.
Now we are burning memory on maintaining mode, and this information is
duplicated.

I'd say let's just add a new command COAL_QUEUE_SET with vqn
as parameter. Existing commands are simply defined as
a shortcut to running COAL_QUEUE_SET on all tx/rx queues
respectively.

Latest command dictates the parameters. To disable just set everything
to 0 (btw we should make this explicit in the spec, but it can be
guessed from:
Upon reset, a device MUST initialize all coalescing parameters to 0.
)

--
MST

On Wed, Feb 08, 2023 at 07:30:34PM +0800, Heng Qi wrote:
> > I see two options.
> > 1. Just have per VQ params. Software has the full knowledge of in which it is operating, and state remains at software level.
> > This effectively achieves both the mode.
> >
> > 2. Have a mode cmd,
> > Mode = (a) per device or (b) per VQ (c) disable
> > After the mode is set, driver can set per device or per VQ.
>
> I find this more clear.
>
> Thanks.
>

Rereading this I think I misunderstood the proposal.
Now we are burning memory on maintaining mode, and this information is
duplicated.

I'd say let's just add a new command COAL_QUEUE_SET with vqn
as parameter. Existing commands are simply defined as
a shortcut to running COAL_QUEUE_SET on all tx/rx queues
respectively.

Latest command dictates the parameters. To disable just set everything
to 0 (btw we should make this explicit in the spec, but it can be
guessed from:
Upon reset, a device MUST initialize all coalescing parameters to 0.
)

--
MST

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Wednesday, February 8, 2023 9:18 AM
>
> On Wed, Feb 08, 2023 at 07:30:34PM +0800, Heng Qi wrote:
> > > I see two options.
> > > 1. Just have per VQ params. Software has the full knowledge of in which it is
> operating, and state remains at software level.
> > > This effectively achieves both the mode.
> > >
> > > 2. Have a mode cmd,
> > > Mode = (a) per device or (b) per VQ (c) disable After the mode is
> > > set, driver can set per device or per VQ.
> >
> > I find this more clear.
> >
> > Thanks.
> >
>
> Rereading this I think I misunderstood the proposal.
> Now we are burning memory on maintaining mode, and this information is
> duplicated.
>
It is not maintained in the pci resident memory, so it doesn't hurt.

> I'd say let's just add a new command COAL_QUEUE_SET with vqn as parameter.
> Existing commands are simply defined as a shortcut to running
> COAL_QUEUE_SET on all tx/rx queues respectively.
>
> Latest command dictates the parameters. To disable just set everything to 0
> (btw we should make this explicit in the spec, but it can be guessed from:
> Upon reset, a device MUST initialize all coalescing parameters to 0.
> )
>
Switching between the modes (per q vs per device) implicitly is ambiguous and it only means device may need to iterate.
This state is either better maintained in sw by always having per vq or have clearly defined mode of what device should do.

Per Q is very common even for several years old devices.
Last time I counted, there were at least 15 such devices supporting it.

So actual usage wise, I practically see that most implementations will end up with per vq mode.
I like to hear from Heng or Alvaro if they see any use of per device.

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Wednesday, February 8, 2023 9:18 AM
>
> On Wed, Feb 08, 2023 at 07:30:34PM +0800, Heng Qi wrote:
> > > I see two options.
> > > 1. Just have per VQ params. Software has the full knowledge of in which it is
> operating, and state remains at software level.
> > > This effectively achieves both the mode.
> > >
> > > 2. Have a mode cmd,
> > > Mode = (a) per device or (b) per VQ (c) disable After the mode is
> > > set, driver can set per device or per VQ.
> >
> > I find this more clear.
> >
> > Thanks.
> >
>
> Rereading this I think I misunderstood the proposal.
> Now we are burning memory on maintaining mode, and this information is
> duplicated.
>
It is not maintained in the pci resident memory, so it doesn't hurt.

> I'd say let's just add a new command COAL_QUEUE_SET with vqn as parameter.
> Existing commands are simply defined as a shortcut to running
> COAL_QUEUE_SET on all tx/rx queues respectively.
>
> Latest command dictates the parameters. To disable just set everything to 0
> (btw we should make this explicit in the spec, but it can be guessed from:
> Upon reset, a device MUST initialize all coalescing parameters to 0.
> )
>
Switching between the modes (per q vs per device) implicitly is ambiguous and it only means device may need to iterate.
This state is either better maintained in sw by always having per vq or have clearly defined mode of what device should do.

Per Q is very common even for several years old devices.
Last time I counted, there were at least 15 such devices supporting it.

So actual usage wise, I practically see that most implementations will end up with per vq mode.
I like to hear from Heng or Alvaro if they see any use of per device.

On Wed, Feb 08, 2023 at 02:37:55PM +0000, Parav Pandit wrote:
>
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Wednesday, February 8, 2023 9:18 AM
> >
> > On Wed, Feb 08, 2023 at 07:30:34PM +0800, Heng Qi wrote:
> > > > I see two options.
> > > > 1. Just have per VQ params. Software has the full knowledge of in which it is
> > operating, and state remains at software level.
> > > > This effectively achieves both the mode.
> > > >
> > > > 2. Have a mode cmd,
> > > > Mode = (a) per device or (b) per VQ (c) disable After the mode is
> > > > set, driver can set per device or per VQ.
> > >
> > > I find this more clear.
> > >
> > > Thanks.
> > >
> >
> > Rereading this I think I misunderstood the proposal.
> > Now we are burning memory on maintaining mode, and this information is
> > duplicated.
> >
> It is not maintained in the pci resident memory, so it doesn't hurt.
>
> > I'd say let's just add a new command COAL_QUEUE_SET with vqn as parameter.
> > Existing commands are simply defined as a shortcut to running
> > COAL_QUEUE_SET on all tx/rx queues respectively.
> >
> > Latest command dictates the parameters. To disable just set everything to 0
> > (btw we should make this explicit in the spec, but it can be guessed from:
> > Upon reset, a device MUST initialize all coalescing parameters to 0.
> > )
> >
> Switching between the modes (per q vs per device) implicitly is ambiguous and it only means device may need to iterate.

hmm i feel it's only ambiguous because i failed to explain in well.

> This state is either better maintained in sw by always having per vq or have clearly defined mode of what device should do.
>
> Per Q is very common even for several years old devices.
> Last time I counted, there were at least 15 such devices supporting it.
>
> So actual usage wise, I practically see that most implementations will end up with per vq mode.
> I like to hear from Heng or Alvaro if they see any use of per device.


Right so given this, most devices will be in per queue mode all the
time. why do you want a mode then? just keep per queue.
existing commands are kept around for compat but
internally just translate to per-queue.

--
MST

On Wed, Feb 08, 2023 at 02:37:55PM +0000, Parav Pandit wrote:
>
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Wednesday, February 8, 2023 9:18 AM
> >
> > On Wed, Feb 08, 2023 at 07:30:34PM +0800, Heng Qi wrote:
> > > > I see two options.
> > > > 1. Just have per VQ params. Software has the full knowledge of in which it is
> > operating, and state remains at software level.
> > > > This effectively achieves both the mode.
> > > >
> > > > 2. Have a mode cmd,
> > > > Mode = (a) per device or (b) per VQ (c) disable After the mode is
> > > > set, driver can set per device or per VQ.
> > >
> > > I find this more clear.
> > >
> > > Thanks.
> > >
> >
> > Rereading this I think I misunderstood the proposal.
> > Now we are burning memory on maintaining mode, and this information is
> > duplicated.
> >
> It is not maintained in the pci resident memory, so it doesn't hurt.
>
> > I'd say let's just add a new command COAL_QUEUE_SET with vqn as parameter.
> > Existing commands are simply defined as a shortcut to running
> > COAL_QUEUE_SET on all tx/rx queues respectively.
> >
> > Latest command dictates the parameters. To disable just set everything to 0
> > (btw we should make this explicit in the spec, but it can be guessed from:
> > Upon reset, a device MUST initialize all coalescing parameters to 0.
> > )
> >
> Switching between the modes (per q vs per device) implicitly is ambiguous and it only means device may need to iterate.

hmm i feel it's only ambiguous because i failed to explain in well.

> This state is either better maintained in sw by always having per vq or have clearly defined mode of what device should do.
>
> Per Q is very common even for several years old devices.
> Last time I counted, there were at least 15 such devices supporting it.
>
> So actual usage wise, I practically see that most implementations will end up with per vq mode.
> I like to hear from Heng or Alvaro if they see any use of per device.


Right so given this, most devices will be in per queue mode all the
time. why do you want a mode then? just keep per queue.
existing commands are kept around for compat but
internally just translate to per-queue.

--
MST

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Wednesday, February 8, 2023 9:43 AM
>
> On Wed, Feb 08, 2023 at 02:37:55PM +0000, Parav Pandit wrote:
> >
> > > From: Michael S. Tsirkin <mst@redhat.com>
> > > Sent: Wednesday, February 8, 2023 9:18 AM
> > >
> > > On Wed, Feb 08, 2023 at 07:30:34PM +0800, Heng Qi wrote:
> > > > > I see two options.
> > > > > 1. Just have per VQ params. Software has the full knowledge of
> > > > > in which it is
> > > operating, and state remains at software level.
> > > > > This effectively achieves both the mode.
> > > > >
> > > > > 2. Have a mode cmd,
> > > > > Mode = (a) per device or (b) per VQ (c) disable After the mode
> > > > > is set, driver can set per device or per VQ.
> > > >
> > > > I find this more clear.
> > > >
> > > > Thanks.
> > > >
> > >
> > > Rereading this I think I misunderstood the proposal.
> > > Now we are burning memory on maintaining mode, and this information
> > > is duplicated.
> > >
> > It is not maintained in the pci resident memory, so it doesn't hurt.
> >
> > > I'd say let's just add a new command COAL_QUEUE_SET with vqn as
> parameter.
> > > Existing commands are simply defined as a shortcut to running
> > > COAL_QUEUE_SET on all tx/rx queues respectively.
> > >
> > > Latest command dictates the parameters. To disable just set
> > > everything to 0 (btw we should make this explicit in the spec, but it can be
> guessed from:
> > > Upon reset, a device MUST initialize all coalescing parameters to 0.
> > > )
> > >
> > Switching between the modes (per q vs per device) implicitly is ambiguous
> and it only means device may need to iterate.
>
> hmm i feel it's only ambiguous because i failed to explain in well.
>
> > This state is either better maintained in sw by always having per vq or have
> clearly defined mode of what device should do.
> >
> > Per Q is very common even for several years old devices.
> > Last time I counted, there were at least 15 such devices supporting it.
> >
> > So actual usage wise, I practically see that most implementations will end up
> with per vq mode.
> > I like to hear from Heng or Alvaro if they see any use of per device.
>
>
> Right so given this, most devices will be in per queue mode all the time. why do
> you want a mode then? just keep per queue.
> existing commands are kept around for compat but internally just translate to
> per-queue.
Since the space is not released, do we need to keep the compat?

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Wednesday, February 8, 2023 9:43 AM
>
> On Wed, Feb 08, 2023 at 02:37:55PM +0000, Parav Pandit wrote:
> >
> > > From: Michael S. Tsirkin <mst@redhat.com>
> > > Sent: Wednesday, February 8, 2023 9:18 AM
> > >
> > > On Wed, Feb 08, 2023 at 07:30:34PM +0800, Heng Qi wrote:
> > > > > I see two options.
> > > > > 1. Just have per VQ params. Software has the full knowledge of
> > > > > in which it is
> > > operating, and state remains at software level.
> > > > > This effectively achieves both the mode.
> > > > >
> > > > > 2. Have a mode cmd,
> > > > > Mode = (a) per device or (b) per VQ (c) disable After the mode
> > > > > is set, driver can set per device or per VQ.
> > > >
> > > > I find this more clear.
> > > >
> > > > Thanks.
> > > >
> > >
> > > Rereading this I think I misunderstood the proposal.
> > > Now we are burning memory on maintaining mode, and this information
> > > is duplicated.
> > >
> > It is not maintained in the pci resident memory, so it doesn't hurt.
> >
> > > I'd say let's just add a new command COAL_QUEUE_SET with vqn as
> parameter.
> > > Existing commands are simply defined as a shortcut to running
> > > COAL_QUEUE_SET on all tx/rx queues respectively.
> > >
> > > Latest command dictates the parameters. To disable just set
> > > everything to 0 (btw we should make this explicit in the spec, but it can be
> guessed from:
> > > Upon reset, a device MUST initialize all coalescing parameters to 0.
> > > )
> > >
> > Switching between the modes (per q vs per device) implicitly is ambiguous
> and it only means device may need to iterate.
>
> hmm i feel it's only ambiguous because i failed to explain in well.
>
> > This state is either better maintained in sw by always having per vq or have
> clearly defined mode of what device should do.
> >
> > Per Q is very common even for several years old devices.
> > Last time I counted, there were at least 15 such devices supporting it.
> >
> > So actual usage wise, I practically see that most implementations will end up
> with per vq mode.
> > I like to hear from Heng or Alvaro if they see any use of per device.
>
>
> Right so given this, most devices will be in per queue mode all the time. why do
> you want a mode then? just keep per queue.
> existing commands are kept around for compat but internally just translate to
> per-queue.
Since the space is not released, do we need to keep the compat?

On Wed, Feb 08, 2023 at 02:44:37PM +0000, Parav Pandit wrote:
>
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Wednesday, February 8, 2023 9:43 AM
> >
> > On Wed, Feb 08, 2023 at 02:37:55PM +0000, Parav Pandit wrote:
> > >
> > > > From: Michael S. Tsirkin <mst@redhat.com>
> > > > Sent: Wednesday, February 8, 2023 9:18 AM
> > > >
> > > > On Wed, Feb 08, 2023 at 07:30:34PM +0800, Heng Qi wrote:
> > > > > > I see two options.
> > > > > > 1. Just have per VQ params. Software has the full knowledge of
> > > > > > in which it is
> > > > operating, and state remains at software level.
> > > > > > This effectively achieves both the mode.
> > > > > >
> > > > > > 2. Have a mode cmd,
> > > > > > Mode = (a) per device or (b) per VQ (c) disable After the mode
> > > > > > is set, driver can set per device or per VQ.
> > > > >
> > > > > I find this more clear.
> > > > >
> > > > > Thanks.
> > > > >
> > > >
> > > > Rereading this I think I misunderstood the proposal.
> > > > Now we are burning memory on maintaining mode, and this information
> > > > is duplicated.
> > > >
> > > It is not maintained in the pci resident memory, so it doesn't hurt.
> > >
> > > > I'd say let's just add a new command COAL_QUEUE_SET with vqn as
> > parameter.
> > > > Existing commands are simply defined as a shortcut to running
> > > > COAL_QUEUE_SET on all tx/rx queues respectively.
> > > >
> > > > Latest command dictates the parameters. To disable just set
> > > > everything to 0 (btw we should make this explicit in the spec, but it can be
> > guessed from:
> > > > Upon reset, a device MUST initialize all coalescing parameters to 0.
> > > > )
> > > >
> > > Switching between the modes (per q vs per device) implicitly is ambiguous
> > and it only means device may need to iterate.
> >
> > hmm i feel it's only ambiguous because i failed to explain in well.
> >
> > > This state is either better maintained in sw by always having per vq or have
> > clearly defined mode of what device should do.
> > >
> > > Per Q is very common even for several years old devices.
> > > Last time I counted, there were at least 15 such devices supporting it.
> > >
> > > So actual usage wise, I practically see that most implementations will end up
> > with per vq mode.
> > > I like to hear from Heng or Alvaro if they see any use of per device.
> >
> >
> > Right so given this, most devices will be in per queue mode all the time. why do
> > you want a mode then? just keep per queue.
> > existing commands are kept around for compat but internally just translate to
> > per-queue.
> Since the space is not released, do we need to keep the compat?

It's been accepted for half a year so we can't say for sure no one built this.
The way I propose is just a bit of firmware on device that scans all
queues and copies same parameters everywhere. Seems easier than
worrying about this, and we get disabling coalescing for free
which you wanted. With an extra mode its extra logic in the device
fast path. Maybe it's cheap on hardware side but in software it's an
extra branch, not free.



--
MST

On Wed, Feb 08, 2023 at 02:44:37PM +0000, Parav Pandit wrote:
>
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Wednesday, February 8, 2023 9:43 AM
> >
> > On Wed, Feb 08, 2023 at 02:37:55PM +0000, Parav Pandit wrote:
> > >
> > > > From: Michael S. Tsirkin <mst@redhat.com>
> > > > Sent: Wednesday, February 8, 2023 9:18 AM
> > > >
> > > > On Wed, Feb 08, 2023 at 07:30:34PM +0800, Heng Qi wrote:
> > > > > > I see two options.
> > > > > > 1. Just have per VQ params. Software has the full knowledge of
> > > > > > in which it is
> > > > operating, and state remains at software level.
> > > > > > This effectively achieves both the mode.
> > > > > >
> > > > > > 2. Have a mode cmd,
> > > > > > Mode = (a) per device or (b) per VQ (c) disable After the mode
> > > > > > is set, driver can set per device or per VQ.
> > > > >
> > > > > I find this more clear.
> > > > >
> > > > > Thanks.
> > > > >
> > > >
> > > > Rereading this I think I misunderstood the proposal.
> > > > Now we are burning memory on maintaining mode, and this information
> > > > is duplicated.
> > > >
> > > It is not maintained in the pci resident memory, so it doesn't hurt.
> > >
> > > > I'd say let's just add a new command COAL_QUEUE_SET with vqn as
> > parameter.
> > > > Existing commands are simply defined as a shortcut to running
> > > > COAL_QUEUE_SET on all tx/rx queues respectively.
> > > >
> > > > Latest command dictates the parameters. To disable just set
> > > > everything to 0 (btw we should make this explicit in the spec, but it can be
> > guessed from:
> > > > Upon reset, a device MUST initialize all coalescing parameters to 0.
> > > > )
> > > >
> > > Switching between the modes (per q vs per device) implicitly is ambiguous
> > and it only means device may need to iterate.
> >
> > hmm i feel it's only ambiguous because i failed to explain in well.
> >
> > > This state is either better maintained in sw by always having per vq or have
> > clearly defined mode of what device should do.
> > >
> > > Per Q is very common even for several years old devices.
> > > Last time I counted, there were at least 15 such devices supporting it.
> > >
> > > So actual usage wise, I practically see that most implementations will end up
> > with per vq mode.
> > > I like to hear from Heng or Alvaro if they see any use of per device.
> >
> >
> > Right so given this, most devices will be in per queue mode all the time. why do
> > you want a mode then? just keep per queue.
> > existing commands are kept around for compat but internally just translate to
> > per-queue.
> Since the space is not released, do we need to keep the compat?

It's been accepted for half a year so we can't say for sure no one built this.
The way I propose is just a bit of firmware on device that scans all
queues and copies same parameters everywhere. Seems easier than
worrying about this, and we get disabling coalescing for free
which you wanted. With an extra mode its extra logic in the device
fast path. Maybe it's cheap on hardware side but in software it's an
extra branch, not free.



--
MST

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Wednesday, February 8, 2023 9:48 AM
>
> On Wed, Feb 08, 2023 at 02:44:37PM +0000, Parav Pandit wrote:
> >
> > > From: Michael S. Tsirkin <mst@redhat.com>
> > > Sent: Wednesday, February 8, 2023 9:43 AM
> > >
> > > On Wed, Feb 08, 2023 at 02:37:55PM +0000, Parav Pandit wrote:
> > > >
> > > > > From: Michael S. Tsirkin <mst@redhat.com>
> > > > > Sent: Wednesday, February 8, 2023 9:18 AM
> > > > >
> > > > > On Wed, Feb 08, 2023 at 07:30:34PM +0800, Heng Qi wrote:
> > > > > > > I see two options.
> > > > > > > 1. Just have per VQ params. Software has the full knowledge
> > > > > > > of in which it is
> > > > > operating, and state remains at software level.
> > > > > > > This effectively achieves both the mode.
> > > > > > >
> > > > > > > 2. Have a mode cmd,
> > > > > > > Mode = (a) per device or (b) per VQ (c) disable After the
> > > > > > > mode is set, driver can set per device or per VQ.
> > > > > >
> > > > > > I find this more clear.
> > > > > >
> > > > > > Thanks.
> > > > > >
> > > > >
> > > > > Rereading this I think I misunderstood the proposal.
> > > > > Now we are burning memory on maintaining mode, and this
> > > > > information is duplicated.
> > > > >
> > > > It is not maintained in the pci resident memory, so it doesn't hurt.
> > > >
> > > > > I'd say let's just add a new command COAL_QUEUE_SET with vqn as
> > > parameter.
> > > > > Existing commands are simply defined as a shortcut to running
> > > > > COAL_QUEUE_SET on all tx/rx queues respectively.
> > > > >
> > > > > Latest command dictates the parameters. To disable just set
> > > > > everything to 0 (btw we should make this explicit in the spec,
> > > > > but it can be
> > > guessed from:
> > > > > Upon reset, a device MUST initialize all coalescing parameters to 0.
> > > > > )
> > > > >
> > > > Switching between the modes (per q vs per device) implicitly is
> > > > ambiguous
> > > and it only means device may need to iterate.
> > >
> > > hmm i feel it's only ambiguous because i failed to explain in well.
> > >
> > > > This state is either better maintained in sw by always having per
> > > > vq or have
> > > clearly defined mode of what device should do.
> > > >
> > > > Per Q is very common even for several years old devices.
> > > > Last time I counted, there were at least 15 such devices supporting it.
> > > >
> > > > So actual usage wise, I practically see that most implementations
> > > > will end up
> > > with per vq mode.
> > > > I like to hear from Heng or Alvaro if they see any use of per device.
> > >
> > >
> > > Right so given this, most devices will be in per queue mode all the
> > > time. why do you want a mode then? just keep per queue.
> > > existing commands are kept around for compat but internally just
> > > translate to per-queue.
> > Since the space is not released, do we need to keep the compat?
>
> It's been accepted for half a year so we can't say for sure no one built this.
That is likely but we should have the ability to have the Errata/ECN to correct it, specially for unrelease spec.

> The way I propose is just a bit of firmware on device that scans all queues and
> copies same parameters everywhere.
This scanning loop in sw appears cheaper to me than some embedded fw.
But is not a lot of concern.

> Seems easier than worrying about this,
> and we get disabling coalescing for free which you wanted. With an extra mode
> its extra logic in the device fast path. Maybe it's cheap on hardware side but in
> software it's an extra branch, not free.

Most performant data path wouldn't implement and read the extra mode.
It is always fw that is going to program same value, or per queue valued or disable value in each Q regardless whichever way we craft the CVQ cmd.

The sequence that bothers me is below.
1. driver set global params
2. few minutes later, now driver set param for Q=1

On this command, a device need to decide:
Should Q = 2 to N
(a) either work with previous globals, or
(b) because per Q was set for one queue, they rest of the queues implicitly disable it.

If it is (b),
When a command on Q object =1 is issued, it affects other Q objects. <- This I want to avoid.
A cmd that modifies the object, should only modify that object.

If it is (a), it is mixed mode operation, which is ambiguous definition.

A better semantic is to define such change at device level and no extra cost in the data path.

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Wednesday, February 8, 2023 9:48 AM
>
> On Wed, Feb 08, 2023 at 02:44:37PM +0000, Parav Pandit wrote:
> >
> > > From: Michael S. Tsirkin <mst@redhat.com>
> > > Sent: Wednesday, February 8, 2023 9:43 AM
> > >
> > > On Wed, Feb 08, 2023 at 02:37:55PM +0000, Parav Pandit wrote:
> > > >
> > > > > From: Michael S. Tsirkin <mst@redhat.com>
> > > > > Sent: Wednesday, February 8, 2023 9:18 AM
> > > > >
> > > > > On Wed, Feb 08, 2023 at 07:30:34PM +0800, Heng Qi wrote:
> > > > > > > I see two options.
> > > > > > > 1. Just have per VQ params. Software has the full knowledge
> > > > > > > of in which it is
> > > > > operating, and state remains at software level.
> > > > > > > This effectively achieves both the mode.
> > > > > > >
> > > > > > > 2. Have a mode cmd,
> > > > > > > Mode = (a) per device or (b) per VQ (c) disable After the
> > > > > > > mode is set, driver can set per device or per VQ.
> > > > > >
> > > > > > I find this more clear.
> > > > > >
> > > > > > Thanks.
> > > > > >
> > > > >
> > > > > Rereading this I think I misunderstood the proposal.
> > > > > Now we are burning memory on maintaining mode, and this
> > > > > information is duplicated.
> > > > >
> > > > It is not maintained in the pci resident memory, so it doesn't hurt.
> > > >
> > > > > I'd say let's just add a new command COAL_QUEUE_SET with vqn as
> > > parameter.
> > > > > Existing commands are simply defined as a shortcut to running
> > > > > COAL_QUEUE_SET on all tx/rx queues respectively.
> > > > >
> > > > > Latest command dictates the parameters. To disable just set
> > > > > everything to 0 (btw we should make this explicit in the spec,
> > > > > but it can be
> > > guessed from:
> > > > > Upon reset, a device MUST initialize all coalescing parameters to 0.
> > > > > )
> > > > >
> > > > Switching between the modes (per q vs per device) implicitly is
> > > > ambiguous
> > > and it only means device may need to iterate.
> > >
> > > hmm i feel it's only ambiguous because i failed to explain in well.
> > >
> > > > This state is either better maintained in sw by always having per
> > > > vq or have
> > > clearly defined mode of what device should do.
> > > >
> > > > Per Q is very common even for several years old devices.
> > > > Last time I counted, there were at least 15 such devices supporting it.
> > > >
> > > > So actual usage wise, I practically see that most implementations
> > > > will end up
> > > with per vq mode.
> > > > I like to hear from Heng or Alvaro if they see any use of per device.
> > >
> > >
> > > Right so given this, most devices will be in per queue mode all the
> > > time. why do you want a mode then? just keep per queue.
> > > existing commands are kept around for compat but internally just
> > > translate to per-queue.
> > Since the space is not released, do we need to keep the compat?
>
> It's been accepted for half a year so we can't say for sure no one built this.
That is likely but we should have the ability to have the Errata/ECN to correct it, specially for unrelease spec.

> The way I propose is just a bit of firmware on device that scans all queues and
> copies same parameters everywhere.
This scanning loop in sw appears cheaper to me than some embedded fw.
But is not a lot of concern.

> Seems easier than worrying about this,
> and we get disabling coalescing for free which you wanted. With an extra mode
> its extra logic in the device fast path. Maybe it's cheap on hardware side but in
> software it's an extra branch, not free.

Most performant data path wouldn't implement and read the extra mode.
It is always fw that is going to program same value, or per queue valued or disable value in each Q regardless whichever way we craft the CVQ cmd.

The sequence that bothers me is below.
1. driver set global params
2. few minutes later, now driver set param for Q=1

On this command, a device need to decide:
Should Q = 2 to N
(a) either work with previous globals, or
(b) because per Q was set for one queue, they rest of the queues implicitly disable it.

If it is (b),
When a command on Q object =1 is issued, it affects other Q objects. <- This I want to avoid.
A cmd that modifies the object, should only modify that object.

If it is (a), it is mixed mode operation, which is ambiguous definition.

A better semantic is to define such change at device level and no extra cost in the data path.

On Wed, Feb 08, 2023 at 03:04:03PM +0000, Parav Pandit wrote:
>
>
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Wednesday, February 8, 2023 9:48 AM
> >
> > On Wed, Feb 08, 2023 at 02:44:37PM +0000, Parav Pandit wrote:
> > >
> > > > From: Michael S. Tsirkin <mst@redhat.com>
> > > > Sent: Wednesday, February 8, 2023 9:43 AM
> > > >
> > > > On Wed, Feb 08, 2023 at 02:37:55PM +0000, Parav Pandit wrote:
> > > > >
> > > > > > From: Michael S. Tsirkin <mst@redhat.com>
> > > > > > Sent: Wednesday, February 8, 2023 9:18 AM
> > > > > >
> > > > > > On Wed, Feb 08, 2023 at 07:30:34PM +0800, Heng Qi wrote:
> > > > > > > > I see two options.
> > > > > > > > 1. Just have per VQ params. Software has the full knowledge
> > > > > > > > of in which it is
> > > > > > operating, and state remains at software level.
> > > > > > > > This effectively achieves both the mode.
> > > > > > > >
> > > > > > > > 2. Have a mode cmd,
> > > > > > > > Mode = (a) per device or (b) per VQ (c) disable After the
> > > > > > > > mode is set, driver can set per device or per VQ.
> > > > > > >
> > > > > > > I find this more clear.
> > > > > > >
> > > > > > > Thanks.
> > > > > > >
> > > > > >
> > > > > > Rereading this I think I misunderstood the proposal.
> > > > > > Now we are burning memory on maintaining mode, and this
> > > > > > information is duplicated.
> > > > > >
> > > > > It is not maintained in the pci resident memory, so it doesn't hurt.
> > > > >
> > > > > > I'd say let's just add a new command COAL_QUEUE_SET with vqn as
> > > > parameter.
> > > > > > Existing commands are simply defined as a shortcut to running
> > > > > > COAL_QUEUE_SET on all tx/rx queues respectively.
> > > > > >
> > > > > > Latest command dictates the parameters. To disable just set
> > > > > > everything to 0 (btw we should make this explicit in the spec,
> > > > > > but it can be
> > > > guessed from:
> > > > > > Upon reset, a device MUST initialize all coalescing parameters to 0.
> > > > > > )
> > > > > >
> > > > > Switching between the modes (per q vs per device) implicitly is
> > > > > ambiguous
> > > > and it only means device may need to iterate.
> > > >
> > > > hmm i feel it's only ambiguous because i failed to explain in well.
> > > >
> > > > > This state is either better maintained in sw by always having per
> > > > > vq or have
> > > > clearly defined mode of what device should do.
> > > > >
> > > > > Per Q is very common even for several years old devices.
> > > > > Last time I counted, there were at least 15 such devices supporting it.
> > > > >
> > > > > So actual usage wise, I practically see that most implementations
> > > > > will end up
> > > > with per vq mode.
> > > > > I like to hear from Heng or Alvaro if they see any use of per device.
> > > >
> > > >
> > > > Right so given this, most devices will be in per queue mode all the
> > > > time. why do you want a mode then? just keep per queue.
> > > > existing commands are kept around for compat but internally just
> > > > translate to per-queue.
> > > Since the space is not released, do we need to keep the compat?
> >
> > It's been accepted for half a year so we can't say for sure no one built this.
> That is likely but we should have the ability to have the Errata/ECN to correct it, specially for unrelease spec.

There's an errata process for sure.

> > The way I propose is just a bit of firmware on device that scans all queues and
> > copies same parameters everywhere.
> This scanning loop in sw appears cheaper to me than some embedded fw.
> But is not a lot of concern.
>
> > Seems easier than worrying about this,
> > and we get disabling coalescing for free which you wanted. With an extra mode
> > its extra logic in the device fast path. Maybe it's cheap on hardware side but in
> > software it's an extra branch, not free.
>
> Most performant data path wouldn't implement and read the extra mode.
> It is always fw that is going to program same value, or per queue valued or disable value in each Q regardless whichever way we craft the CVQ cmd.
>
> The sequence that bothers me is below.
> 1. driver set global params
> 2. few minutes later, now driver set param for Q=1
>
> On this command, a device need to decide:
> Should Q = 2 to N
> (a) either work with previous globals, or
> (b) because per Q was set for one queue, they rest of the queues implicitly disable it.
>
> If it is (b),
> When a command on Q object =1 is issued, it affects other Q objects. <- This I want to avoid.
> A cmd that modifies the object, should only modify that object.
>
> If it is (a), it is mixed mode operation, which is ambiguous definition.
>
> A better semantic is to define such change at device level and no extra cost in the data path.

Ugh. Looks like I didn't explain it well, yet again :(.
Here is my proposal in pseudo-code:


if (cmd == VQ_SET)
vq[cmd.index].param = cmd.param;

if (cmd == TX_SET)
for (i = 0; ++i; i < maxvqn / 2)
vq[i * 2].param = cmd.param;

if (cmd == RX_SET)
for (i = 0; ++i; i < maxvqn / 2)
vq[i * 2 + 1].param = cmd.param;



there's nothing to decide at all. No modes. TX_SET and RX_SET affect
half vqs, VQ_SET affects one vq.

--
MST

On Wed, Feb 08, 2023 at 03:04:03PM +0000, Parav Pandit wrote:
>
>
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Wednesday, February 8, 2023 9:48 AM
> >
> > On Wed, Feb 08, 2023 at 02:44:37PM +0000, Parav Pandit wrote:
> > >
> > > > From: Michael S. Tsirkin <mst@redhat.com>
> > > > Sent: Wednesday, February 8, 2023 9:43 AM
> > > >
> > > > On Wed, Feb 08, 2023 at 02:37:55PM +0000, Parav Pandit wrote:
> > > > >
> > > > > > From: Michael S. Tsirkin <mst@redhat.com>
> > > > > > Sent: Wednesday, February 8, 2023 9:18 AM
> > > > > >
> > > > > > On Wed, Feb 08, 2023 at 07:30:34PM +0800, Heng Qi wrote:
> > > > > > > > I see two options.
> > > > > > > > 1. Just have per VQ params. Software has the full knowledge
> > > > > > > > of in which it is
> > > > > > operating, and state remains at software level.
> > > > > > > > This effectively achieves both the mode.
> > > > > > > >
> > > > > > > > 2. Have a mode cmd,
> > > > > > > > Mode = (a) per device or (b) per VQ (c) disable After the
> > > > > > > > mode is set, driver can set per device or per VQ.
> > > > > > >
> > > > > > > I find this more clear.
> > > > > > >
> > > > > > > Thanks.
> > > > > > >
> > > > > >
> > > > > > Rereading this I think I misunderstood the proposal.
> > > > > > Now we are burning memory on maintaining mode, and this
> > > > > > information is duplicated.
> > > > > >
> > > > > It is not maintained in the pci resident memory, so it doesn't hurt.
> > > > >
> > > > > > I'd say let's just add a new command COAL_QUEUE_SET with vqn as
> > > > parameter.
> > > > > > Existing commands are simply defined as a shortcut to running
> > > > > > COAL_QUEUE_SET on all tx/rx queues respectively.
> > > > > >
> > > > > > Latest command dictates the parameters. To disable just set
> > > > > > everything to 0 (btw we should make this explicit in the spec,
> > > > > > but it can be
> > > > guessed from:
> > > > > > Upon reset, a device MUST initialize all coalescing parameters to 0.
> > > > > > )
> > > > > >
> > > > > Switching between the modes (per q vs per device) implicitly is
> > > > > ambiguous
> > > > and it only means device may need to iterate.
> > > >
> > > > hmm i feel it's only ambiguous because i failed to explain in well.
> > > >
> > > > > This state is either better maintained in sw by always having per
> > > > > vq or have
> > > > clearly defined mode of what device should do.
> > > > >
> > > > > Per Q is very common even for several years old devices.
> > > > > Last time I counted, there were at least 15 such devices supporting it.
> > > > >
> > > > > So actual usage wise, I practically see that most implementations
> > > > > will end up
> > > > with per vq mode.
> > > > > I like to hear from Heng or Alvaro if they see any use of per device.
> > > >
> > > >
> > > > Right so given this, most devices will be in per queue mode all the
> > > > time. why do you want a mode then? just keep per queue.
> > > > existing commands are kept around for compat but internally just
> > > > translate to per-queue.
> > > Since the space is not released, do we need to keep the compat?
> >
> > It's been accepted for half a year so we can't say for sure no one built this.
> That is likely but we should have the ability to have the Errata/ECN to correct it, specially for unrelease spec.

There's an errata process for sure.

> > The way I propose is just a bit of firmware on device that scans all queues and
> > copies same parameters everywhere.
> This scanning loop in sw appears cheaper to me than some embedded fw.
> But is not a lot of concern.
>
> > Seems easier than worrying about this,
> > and we get disabling coalescing for free which you wanted. With an extra mode
> > its extra logic in the device fast path. Maybe it's cheap on hardware side but in
> > software it's an extra branch, not free.
>
> Most performant data path wouldn't implement and read the extra mode.
> It is always fw that is going to program same value, or per queue valued or disable value in each Q regardless whichever way we craft the CVQ cmd.
>
> The sequence that bothers me is below.
> 1. driver set global params
> 2. few minutes later, now driver set param for Q=1
>
> On this command, a device need to decide:
> Should Q = 2 to N
> (a) either work with previous globals, or
> (b) because per Q was set for one queue, they rest of the queues implicitly disable it.
>
> If it is (b),
> When a command on Q object =1 is issued, it affects other Q objects. <- This I want to avoid.
> A cmd that modifies the object, should only modify that object.
>
> If it is (a), it is mixed mode operation, which is ambiguous definition.
>
> A better semantic is to define such change at device level and no extra cost in the data path.

Ugh. Looks like I didn't explain it well, yet again :(.
Here is my proposal in pseudo-code:


if (cmd == VQ_SET)
vq[cmd.index].param = cmd.param;

if (cmd == TX_SET)
for (i = 0; ++i; i < maxvqn / 2)
vq[i * 2].param = cmd.param;

if (cmd == RX_SET)
for (i = 0; ++i; i < maxvqn / 2)
vq[i * 2 + 1].param = cmd.param;



there's nothing to decide at all. No modes. TX_SET and RX_SET affect
half vqs, VQ_SET affects one vq.

--
MST