[virtio-comment] [PATCH] virtio-net: support per-queue coalescing moderation

[virtio-comment] [PATCH] virtio-net: support per-queue coalescing moderation

[Heng Qi]

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


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

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

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

[virtio-comment] Re: [PATCH] virtio-net: support per-queue coalescing moderation

[Alvaro Karsz]

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
>

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

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

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

Re: [virtio-comment] Re: [PATCH] virtio-net: support per-queue coalescing moderation

[Xuan Zhuo]

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

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

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

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

RE: [virtio-comment] Re: [PATCH] virtio-net: support per-queue coalescing moderation

[Parav Pandit]

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

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

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

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

Re: [virtio-comment] [PATCH] virtio-net: support per-queue coalescing moderation

[Michael S. Tsirkin]

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/


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

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

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

Re: [virtio-comment] [PATCH] virtio-net: support per-queue coalescing moderation

[Michael S. Tsirkin]

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/


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

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

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

[virtio-dev] Re: [virtio-comment] Re: [PATCH] virtio-net: support per-queue coalescing moderation

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


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

Re: [virtio-dev] Re: [virtio-comment] Re: [PATCH] virtio-net: support per-queue coalescing moderation

[Alvaro Karsz]

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

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

Re: [virtio-dev] Re: [virtio-comment] Re: [PATCH] virtio-net: support per-queue coalescing moderation

[Michael S. Tsirkin]

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


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

Re: [virtio-dev] Re: [virtio-comment] Re: [PATCH] virtio-net: support per-queue coalescing moderation

[Alvaro Karsz]

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

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

Re: [virtio-dev] Re: [virtio-comment] Re: [PATCH] virtio-net: support per-queue coalescing moderation

[Michael S. Tsirkin]

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


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

RE: [virtio-dev] Re: [virtio-comment] Re: [PATCH] virtio-net: support per-queue coalescing moderation

[Parav Pandit]

> 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


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

[virtio-comment] Re: [virtio-dev] Re: [virtio-comment] Re: [PATCH] virtio-net: support per-queue coalescing moderation

[Michael S. Tsirkin]

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


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

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

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

[virtio-comment] RE: [virtio-dev] Re: [virtio-comment] Re: [PATCH] virtio-net: support per-queue coalescing moderation

[Parav Pandit]

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

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

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

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

Re: [virtio-comment] [PATCH] virtio-net: support per-queue coalescing moderation

[Xuan Zhuo]

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

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

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

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

[virtio-comment] Re: [virtio-dev] Re: [virtio-comment] [PATCH] virtio-net: support per-queue coalescing moderation

[Heng Qi]

在 2023/2/7 下午10:06, Michael S. Tsirkin 写道:
> 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?

Yes, ethtool does not support this function yet, and this function will 
not conflict with ethool.
Our current goal is to let virtio-net support netdim first.

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

I saw Alvaro's patch, but it doesn't seem to be stable yet, is there a 
good way for me to
unify the two structures, since a patch should only do one thing.

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

The spec mentioned
"A device SHOULD respond to the VIRTIO_NET_CTRL_NOTF_COAL commands with 
VIRTIO_NET_ERR if it was not able to change the parameters.",
I think this belongs to this situation.

Thanks.

>
>> +\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/
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org


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

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

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

[virtio-comment] Re: [virtio-dev] Re: [virtio-comment] Re: [PATCH] virtio-net: support per-queue coalescing moderation

[Xuan Zhuo]

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
>

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

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

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

Re: [virtio-comment] Re: [PATCH] virtio-net: support per-queue coalescing moderation

[Heng Qi]

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


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

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

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

RE: [virtio-comment] [PATCH] virtio-net: support per-queue coalescing moderation

[Parav Pandit]

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

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

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

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

Re: RE: [virtio-comment] [PATCH] virtio-net: support per-queue coalescing moderation

[Xuan Zhuo]

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.


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

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

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

Re: RE: [virtio-comment] [PATCH] virtio-net: support per-queue coalescing moderation

[Xuan Zhuo]

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.

Considering the software device, I think that sending too many commands in a row
may not be particularly friendly. I still want to retain global config.


> Once per VQ is supported, driver may just use the default net-dim to have best out of box experience, whenever device supports it.

I agree.

Thanks.

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

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

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

[virtio-comment] Re: [virtio-dev] RE: [virtio-comment] [PATCH] virtio-net: support per-queue coalescing moderation

[Heng Qi]

在 2023/2/8 上午10:20, Parav Pandit 写道:
>> 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.

However, a global configuration requires sending a command for each 
queue, which makes the device suffer.
I think the hardware might not want this.

Thanks.

> Once per VQ is supported, driver may just use the default net-dim to have best out of box experience, whenever device supports it.
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org


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

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

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

RE: RE: [virtio-comment] [PATCH] virtio-net: support per-queue coalescing moderation

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

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

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

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

[virtio-comment] RE: [virtio-dev] RE: [virtio-comment] [PATCH] virtio-net: support per-queue coalescing moderation

[Parav Pandit]

> From: Heng Qi <hengqi@linux.alibaba.com>
> Sent: Tuesday, February 7, 2023 9:36 PM
> 
> 
> 在 2023/2/8 上午10:20, Parav Pandit 写道:
> >> 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.
> 
> However, a global configuration requires sending a command for each queue,
> which makes the device suffer.
> I think the hardware might not want this.
> 
There are many commands that a hw nic device needs to service.
So I agree that for high q count, a single command is better.
However, when a device is with such high q count, the possibility of needing it as per q params is also extremely high for best perf (and variance each q might have).
So couldn’t see a lot of value in doing as global param.

But yes, if it helps, a command with explicit mode to avoid ambiguity is fine to have.
Both approaches of [1] looks good to me.

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

Re: [virtio-comment] Re: [PATCH] virtio-net: support per-queue coalescing moderation

[Michael S. Tsirkin]

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


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

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

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

[virtio-comment] Re: [virtio-dev] Re: [virtio-comment] [PATCH] virtio-net: support per-queue coalescing moderation

[Michael S. Tsirkin]

On Wed, Feb 08, 2023 at 09:57:54AM +0800, Heng Qi wrote:
> 
> 
> 在 2023/2/7 下午10:06, Michael S. Tsirkin 写道:
> > 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?
> 
> Yes, ethtool does not support this function yet, and this function will not
> conflict with ethool.
> Our current goal is to let virtio-net support netdim first.
> 
> > 
> > > 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.
> 
> I saw Alvaro's patch, but it doesn't seem to be stable yet, is there a good
> way for me to
> unify the two structures, since a patch should only do one thing.
> 
> > 
> > > @@ -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?
> 
> The spec mentioned
> "A device SHOULD respond to the VIRTIO_NET_CTRL_NOTF_COAL commands with
> VIRTIO_NET_ERR if it was not able to change the parameters.",
> I think this belongs to this situation.
> 
> Thanks.

Maybe but it's worth calling out. Because another interpretation
is that if qid matches rx queue we change rx parameters and
if it matches tx queue we change tx parameters.
There's a redundancy here I don't like different devices
might handle the error differently.
Maybe a new command is better?

VIRTIO_NET_CTRL_NOTF_COAL_QUEUE_SET ?

qid selects whether it's rx or tx.

and then it's a natural thing with a feature bit controlling a new
command. we have lots of examples like this.


something we should also mention is that VIRTIO_NET_CTRL_NOTF_COAL_RX_SET
is the same as calling VIRTIO_NET_CTRL_NOTF_COAL_QUEUE_SET
for all RX queues.

> > 
> > > +\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/
> > 
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> > For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org


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

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

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

Re: RE: [virtio-comment] [PATCH] virtio-net: support per-queue coalescing moderation

[Michael S. Tsirkin]

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


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

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

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

Re: [virtio-dev] Re: [virtio-comment] [PATCH] virtio-net: support per-queue coalescing moderation

[Michael S. Tsirkin]

On Wed, Feb 08, 2023 at 09:57:54AM +0800, Heng Qi wrote:
> > I think it's a good idea to do this on top of Alvaro's patch
> > unifying these two structures.
> 
> I saw Alvaro's patch, but it doesn't seem to be stable yet, is there a good
> way for me to
> unify the two structures, since a patch should only do one thing.

Problem is you were trying to change these existing structures too
so the patches conflicted. However I think at this point
we are in agreement on a new command with a new structure.
In this case there won't be a conflict as you are
not touching existing commands so no need for you
to depend on Alvaro's patch.

-- 
MST


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

Re: [virtio-comment] Re: [virtio-dev] Re: [virtio-comment] [PATCH] virtio-net: support per-queue coalescing moderation

[Heng Qi]

在 2023/2/8 下午6:04, Michael S. Tsirkin 写道:
> On Wed, Feb 08, 2023 at 09:57:54AM +0800, Heng Qi wrote:
>>
>> 在 2023/2/7 下午10:06, Michael S. Tsirkin 写道:
>>> 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?
>> Yes, ethtool does not support this function yet, and this function will not
>> conflict with ethool.
>> Our current goal is to let virtio-net support netdim first.
>>
>>>> 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.
>> I saw Alvaro's patch, but it doesn't seem to be stable yet, is there a good
>> way for me to
>> unify the two structures, since a patch should only do one thing.
>>
>>>> @@ -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?
>> The spec mentioned
>> "A device SHOULD respond to the VIRTIO_NET_CTRL_NOTF_COAL commands with
>> VIRTIO_NET_ERR if it was not able to change the parameters.",
>> I think this belongs to this situation.
>>
>> Thanks.
> Maybe but it's worth calling out. Because another interpretation
> is that if qid matches rx queue we change rx parameters and
> if it matches tx queue we change tx parameters.
> There's a redundancy here I don't like different devices
> might handle the error differently.

Ok, I'll clarify here.

> Maybe a new command is better?

I agree, but there is one thing to confirm below.

>
> VIRTIO_NET_CTRL_NOTF_COAL_QUEUE_SET ?
>
> qid selects whether it's rx or tx.

I think the qid you expressed here should refer to the virtqueue id?
If qid represents the index of recieveq or transmitq, we can use 
VIRTIO_NET_CTRL_NOTF_COAL_RX_QUEUE_SET and 
VIRTIO_NET_CTRL_NOTF_COAL_TX_QUEUE_SET.

Which do you think is more suitable? I think the latter (qid means the 
id of receiveq or transmitq) seems to be more aligned with the current spec?

>
> and then it's a natural thing with a feature bit controlling a new
> command. we have lots of examples like this.
>
>
> something we should also mention is that VIRTIO_NET_CTRL_NOTF_COAL_RX_SET
> is the same as calling VIRTIO_NET_CTRL_NOTF_COAL_QUEUE_SET
> for all RX queues.

Sure.

Thanks.

>
>>>> +\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/
>>> ---------------------------------------------------------------------
>>> To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
>>> For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
>
> This publicly archived list offers a means to provide input to the
> OASIS Virtual I/O Device (VIRTIO) TC.
>
> In order to verify user consent to the Feedback License terms and
> to minimize spam in the list archive, subscription is required
> before posting.
>
> Subscribe: virtio-comment-subscribe@lists.oasis-open.org
> Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
> List help: virtio-comment-help@lists.oasis-open.org
> List archive: https://lists.oasis-open.org/archives/virtio-comment/
> Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
> List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
> Committee: https://www.oasis-open.org/committees/virtio/
> Join OASIS: https://www.oasis-open.org/join/


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

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

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

Re: [virtio-dev] Re: [virtio-comment] [PATCH] virtio-net: support per-queue coalescing moderation

[Heng Qi]

在 2023/2/8 下午6:10, Michael S. Tsirkin 写道:
> On Wed, Feb 08, 2023 at 09:57:54AM +0800, Heng Qi wrote:
>>> I think it's a good idea to do this on top of Alvaro's patch
>>> unifying these two structures.
>> I saw Alvaro's patch, but it doesn't seem to be stable yet, is there a good
>> way for me to
>> unify the two structures, since a patch should only do one thing.
> Problem is you were trying to change these existing structures too
> so the patches conflicted. However I think at this point
> we are in agreement on a new command with a new structure.
> In this case there won't be a conflict as you are
> not touching existing commands so no need for you
> to depend on Alvaro's patch.

I get it.

Thanks.

>


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

Re: [virtio-comment] [PATCH] virtio-net: support per-queue coalescing moderation

[Heng Qi]

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



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

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

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

[virtio-dev] Re: [virtio-comment] Re: [virtio-dev] Re: [virtio-comment] [PATCH] virtio-net: support per-queue coalescing moderation

[Michael S. Tsirkin]

On Wed, Feb 08, 2023 at 07:23:10PM +0800, Heng Qi wrote:
> 
> 
> 在 2023/2/8 下午6:04, Michael S. Tsirkin 写道:
> > On Wed, Feb 08, 2023 at 09:57:54AM +0800, Heng Qi wrote:
> > > 
> > > 在 2023/2/7 下午10:06, Michael S. Tsirkin 写道:
> > > > 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?
> > > Yes, ethtool does not support this function yet, and this function will not
> > > conflict with ethool.
> > > Our current goal is to let virtio-net support netdim first.
> > > 
> > > > > 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.
> > > I saw Alvaro's patch, but it doesn't seem to be stable yet, is there a good
> > > way for me to
> > > unify the two structures, since a patch should only do one thing.
> > > 
> > > > > @@ -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?
> > > The spec mentioned
> > > "A device SHOULD respond to the VIRTIO_NET_CTRL_NOTF_COAL commands with
> > > VIRTIO_NET_ERR if it was not able to change the parameters.",
> > > I think this belongs to this situation.
> > > 
> > > Thanks.
> > Maybe but it's worth calling out. Because another interpretation
> > is that if qid matches rx queue we change rx parameters and
> > if it matches tx queue we change tx parameters.
> > There's a redundancy here I don't like different devices
> > might handle the error differently.
> 
> Ok, I'll clarify here.
> 
> > Maybe a new command is better?
> 
> I agree, but there is one thing to confirm below.
> 
> > 
> > VIRTIO_NET_CTRL_NOTF_COAL_QUEUE_SET ?
> > 
> > qid selects whether it's rx or tx.
> 
> I think the qid you expressed here should refer to the virtqueue id?
> If qid represents the index of recieveq or transmitq, we can use
> VIRTIO_NET_CTRL_NOTF_COAL_RX_QUEUE_SET and
> VIRTIO_NET_CTRL_NOTF_COAL_TX_QUEUE_SET.
> 
> Which do you think is more suitable? I think the latter (qid means the id of
> receiveq or transmitq) seems to be more aligned with the current spec?


We do in fact have both. Calling vq number an index is well established
in the spec. index in the sense of  "floor(vq index / 2)" is only used
once where we document the RSS indirection table, so only for receive.

I would be inclined to talk about vq numbers and change RSS to do that
too.

Saying somewhere in
\section{Virtqueues}\label{sec:Basic Facilities of a Virtio Device / Virtqueues}
that each virtqueue is associated with a 16 bit virtqueue index
can be a good idea, too.

-- 
MST


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

RE: [virtio-comment] Re: [PATCH] virtio-net: support per-queue coalescing moderation

[Parav Pandit]

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

[virtio-dev] RE: RE: [virtio-comment] [PATCH] virtio-net: support per-queue coalescing moderation

[Parav Pandit]

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

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

[virtio-dev] Re: [virtio-comment] [PATCH] virtio-net: support per-queue coalescing moderation

[Michael S. Tsirkin]

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


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

RE: [virtio-comment] [PATCH] virtio-net: support per-queue coalescing moderation

[Parav Pandit]

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

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

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

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

[virtio-dev] Re: [virtio-comment] [PATCH] virtio-net: support per-queue coalescing moderation

[Michael S. Tsirkin]

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


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

Re: [virtio-comment] [PATCH] virtio-net: support per-queue coalescing moderation

[Michael S. Tsirkin]

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.

It can't reasonably be read from RAM at each packet, you need to cache it
in memory.


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

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

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

RE: [virtio-comment] [PATCH] virtio-net: support per-queue coalescing moderation

[Parav Pandit]

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

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

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

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

Re: [virtio-comment] [PATCH] virtio-net: support per-queue coalescing moderation

[Michael S. Tsirkin]

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


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

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

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

RE: [virtio-comment] [PATCH] virtio-net: support per-queue coalescing moderation

[Parav Pandit]

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

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

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

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

Re: [virtio-comment] [PATCH] virtio-net: support per-queue coalescing moderation

[Michael S. Tsirkin]

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


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

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

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

RE: [virtio-comment] [PATCH] virtio-net: support per-queue coalescing moderation

[Parav Pandit]

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Wednesday, February 8, 2023 10:20 AM

[..]

> >
> > 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;
> 
>
Currently we have cmd = GLOBAL_SET the code is:

 if (cmd == GLOBAL_SET)
	for (i = 0; i < maxvqn; i++)
		vq[i].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.
>
I explained above that, when cmd=GLOBAL_SET and after that if driver issues cmd=VQ_SET, at that point, the device is in partial mode.
One VQ is running with its own param, and rest are in global mode.

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

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

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

Re: [virtio-comment] [PATCH] virtio-net: support per-queue coalescing moderation

[Alvaro Karsz]

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

I think that (a) is the way to go.
I don't think that we should work with operation modes at all.

In my opinion:

We should have 2 features:
VIRTIO_NET_F_PERQUEUE_NOTF_COAL and VIRTIO_NET_F_NOTF_COAL.

VIRTIO_NET_F_PERQUEUE_NOTF_COAL sets per queue parameters, and
VIRTIO_NET_F_NOTF_COAL sets parameters for all queues.

VIRTIO_NET_F_NOTF_COAL has 2 commands:
    VIRTIO_NET_CTRL_NOTF_COAL_RX_SET
    VIRTIO_NET_CTRL_NOTF_COAL_TX_SET

VIRTIO_NET_F_PERQUEUE_NOTF_COAL has 2 commands:
    VIRTIO_NET_CTRL_NOTF_COAL_PER_QUEUE_TX_SET
    VIRTIO_NET_CTRL_NOTF_COAL_PER_QUEUE_RX_SET

We can see VIRTIO_NET_CTRL_NOTF_COAL_RX_SET as a virtio level shortcut
for setting all queues with one command, exactly as intended with
rx_qid= 0xFFFF, and without breaking devices following the current
spec.

The device's FW can decide if it stores parameters received with
VIRTIO_NET_CTRL_NOTF_COAL_RX_SET in a global set, or if it iterates
through all queues, but IMO the best way it to iterate through all
queues.

Seems like a win-win situation to me.
We achieve the same functionality as described in the patch, but
without breaking devices following the current spec.

Now, if we follow this method,
VIRTIO_NET_CTRL_NOTF_COAL_PER_QUEUE_RX_SET with rx_qid= 0xFFF seems
redundant.
If VIRTIO_NET_F_PERQUEUE_NOTF_COAL requires VIRTIO_NET_F_NOTF_COAL, a
device supporting VIRTIO_NET_F_PERQUEUE_NOTF_COAL can achieve the same
functionality with the VIRTIO_NET_CTRL_NOTF_COAL_RX_SET command.

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

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

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

[virtio-dev] Re: [virtio-comment] [PATCH] virtio-net: support per-queue coalescing moderation

[Parav Pandit]

[-- Attachment #1: Type: text/plain, Size: 2591 bytes --]

You are right. Two commands are there.
Regardless the ambiguity remains
________________________________
From: Parav Pandit
Sent: Wednesday, February 8, 2023 8:57 PM
To: Michael S. Tsirkin <mst@redhat.com>
Cc: Heng Qi <hengqi@linux.alibaba.com>; Xuan Zhuo <xuanzhuo@linux.alibaba.com>; virtio-comment @ lists . oasis-open . org <virtio-comment@lists.oasis-open.org>; virtio-dev @ lists . oasis-open . org <virtio-dev@lists.oasis-open.org>; Jason Wang <jasowang@redhat.com>; Alvaro Karsz <alvaro.karsz@solid-run.com>
Subject: RE: [virtio-comment] [PATCH] virtio-net: support per-queue coalescing moderation


> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Wednesday, February 8, 2023 10:20 AM

[..]

> >
> > 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;
>
>
Currently we have cmd = GLOBAL_SET the code is:

 if (cmd == GLOBAL_SET)
        for (i = 0; i < maxvqn; i++)
                vq[i].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.
>
I explained above that, when cmd=GLOBAL_SET and after that if driver issues cmd=VQ_SET, at that point, the device is in partial mode.
One VQ is running with its own param, and rest are in global mode.

[-- Attachment #2: Type: text/html, Size: 4224 bytes --]

Re: [virtio-comment] [PATCH] virtio-net: support per-queue coalescing moderation

[Michael S. Tsirkin]

On Wed, Feb 08, 2023 at 07:23:12PM +0000, Parav Pandit wrote:
> You are right. Two commands are there.
> Regardless the ambiguity remains
> ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
> From: Parav Pandit
> Sent: Wednesday, February 8, 2023 8:57 PM
> To: Michael S. Tsirkin <mst@redhat.com>
> Cc: Heng Qi <hengqi@linux.alibaba.com>; Xuan Zhuo <xuanzhuo@linux.alibaba.com>;
> virtio-comment @ lists . oasis-open . org
> <virtio-comment@lists.oasis-open.org>; virtio-dev @ lists . oasis-open . org
> <virtio-dev@lists.oasis-open.org>; Jason Wang <jasowang@redhat.com>; Alvaro
> Karsz <alvaro.karsz@solid-run.com>
> Subject: RE: [virtio-comment] [PATCH] virtio-net: support per-queue coalescing
> moderation
>  
> 
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Wednesday, February 8, 2023 10:20 AM
> 
> [..]
> 
> > >
> > > 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;
> >
> >
> Currently we have cmd = GLOBAL_SET the code is:
> 
>  if (cmd == GLOBAL_SET)
>         for (i = 0; i < maxvqn; i++)
>                 vq[i].param = cmd.param;

exactly.

> >
> > there's nothing to decide at all. No modes. TX_SET and RX_SET affect half
> vqs,
> > VQ_SET affects one vq.
> >
> I explained above that, when cmd=GLOBAL_SET and after that if driver issues cmd
> =VQ_SET, at that point, the device is in partial mode.
> One VQ is running with its own param, and rest are in global mode.


Because there's no "global mode" - it is always per queue and each queue
uses a set of parameters, that is all. No ambiguity. You can quickly set
them all with a special command, or you can set them one by one.

For example consider a driver that loops over all vqs
and does RX_SET(0). Are we in global mode, per queue mode or disabled
mode? The questions is pointless is it not?

-- 
MST


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

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

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

Re: [virtio-comment] [PATCH] virtio-net: support per-queue coalescing moderation

[Michael S. Tsirkin]

On Wed, Feb 08, 2023 at 07:53:09PM +0200, Alvaro Karsz 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.
> >
> > > 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.
> 
> I think that (a) is the way to go.
> I don't think that we should work with operation modes at all.
> 
> In my opinion:
> 
> We should have 2 features:
> VIRTIO_NET_F_PERQUEUE_NOTF_COAL and VIRTIO_NET_F_NOTF_COAL.
> 
> VIRTIO_NET_F_PERQUEUE_NOTF_COAL sets per queue parameters, and
> VIRTIO_NET_F_NOTF_COAL sets parameters for all queues.
> 
> VIRTIO_NET_F_NOTF_COAL has 2 commands:
>     VIRTIO_NET_CTRL_NOTF_COAL_RX_SET
>     VIRTIO_NET_CTRL_NOTF_COAL_TX_SET
> 
> VIRTIO_NET_F_PERQUEUE_NOTF_COAL has 2 commands:
>     VIRTIO_NET_CTRL_NOTF_COAL_PER_QUEUE_TX_SET
>     VIRTIO_NET_CTRL_NOTF_COAL_PER_QUEUE_RX_SET
> 
> We can see VIRTIO_NET_CTRL_NOTF_COAL_RX_SET as a virtio level shortcut
> for setting all queues with one command, exactly as intended with
> rx_qid= 0xFFFF, and without breaking devices following the current
> spec.
> 
> The device's FW can decide if it stores parameters received with
> VIRTIO_NET_CTRL_NOTF_COAL_RX_SET in a global set, or if it iterates
> through all queues, but IMO the best way it to iterate through all
> queues.
> 
> Seems like a win-win situation to me.
> We achieve the same functionality as described in the patch, but
> without breaking devices following the current spec.
> 
> Now, if we follow this method,
> VIRTIO_NET_CTRL_NOTF_COAL_PER_QUEUE_RX_SET with rx_qid= 0xFFF seems
> redundant.
> If VIRTIO_NET_F_PERQUEUE_NOTF_COAL requires VIRTIO_NET_F_NOTF_COAL, a
> device supporting VIRTIO_NET_F_PERQUEUE_NOTF_COAL can achieve the same
> functionality with the VIRTIO_NET_CTRL_NOTF_COAL_RX_SET command.

Yes. Just some comments:

- I don't think we need two commands. We have RX and TX because we
  did not have vq number previously. No we do so just pass that.
  It's also clearer since struct name can match command exactly.

- Once we do that we can use a short _VQ_ instead of the wordy "PER_QUEUE".

- Accordingly a well understood "vqn" instead of our own "qid" which
  we then need to define.

- And yes no need for a reserved "qid" - it's a distinct command.

-- 
MST


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

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

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

RE: [virtio-comment] [PATCH] virtio-net: support per-queue coalescing moderation

[Parav Pandit]

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Wednesday, February 8, 2023 3:52 PM
> 
> On Wed, Feb 08, 2023 at 07:53:09PM +0200, Alvaro Karsz 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.
> > >
> > > > 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.
> >
> > I think that (a) is the way to go.
> > I don't think that we should work with operation modes at all.
> >
> > In my opinion:
> >
> > We should have 2 features:
> > VIRTIO_NET_F_PERQUEUE_NOTF_COAL and VIRTIO_NET_F_NOTF_COAL.
> >
> > VIRTIO_NET_F_PERQUEUE_NOTF_COAL sets per queue parameters, and
> > VIRTIO_NET_F_NOTF_COAL sets parameters for all queues.
> >
> > VIRTIO_NET_F_NOTF_COAL has 2 commands:
> >     VIRTIO_NET_CTRL_NOTF_COAL_RX_SET
> >     VIRTIO_NET_CTRL_NOTF_COAL_TX_SET
> >
> > VIRTIO_NET_F_PERQUEUE_NOTF_COAL has 2 commands:
> >     VIRTIO_NET_CTRL_NOTF_COAL_PER_QUEUE_TX_SET
> >     VIRTIO_NET_CTRL_NOTF_COAL_PER_QUEUE_RX_SET
> >
> > We can see VIRTIO_NET_CTRL_NOTF_COAL_RX_SET as a virtio level shortcut
> > for setting all queues with one command, exactly as intended with
> > rx_qid= 0xFFFF, and without breaking devices following the current
> > spec.
> >
> > The device's FW can decide if it stores parameters received with
> > VIRTIO_NET_CTRL_NOTF_COAL_RX_SET in a global set, or if it iterates
> > through all queues, but IMO the best way it to iterate through all
> > queues.
> >
> > Seems like a win-win situation to me.
> > We achieve the same functionality as described in the patch, but
> > without breaking devices following the current spec.
> >
> > Now, if we follow this method,
> > VIRTIO_NET_CTRL_NOTF_COAL_PER_QUEUE_RX_SET with rx_qid= 0xFFF
> seems
> > redundant.
> > If VIRTIO_NET_F_PERQUEUE_NOTF_COAL requires
> VIRTIO_NET_F_NOTF_COAL, a
> > device supporting VIRTIO_NET_F_PERQUEUE_NOTF_COAL can achieve the
> same
> > functionality with the VIRTIO_NET_CTRL_NOTF_COAL_RX_SET command.
> 
> Yes. Just some comments:
> 
> - I don't think we need two commands. We have RX and TX because we
>   did not have vq number previously. No we do so just pass that.
>   It's also clearer since struct name can match command exactly.
>
+1. Consolidate the structure, followed by single command is good.
 
> - Once we do that we can use a short _VQ_ instead of the wordy
> "PER_QUEUE".
> 
> - Accordingly a well understood "vqn" instead of our own "qid" which
>   we then need to define.
> 
> - And yes no need for a reserved "qid" - it's a distinct command.

I want to ask one more time, is there any sw that used this?
If not, we are better of just having single per vq command and everything is good.
For extra safety, we can reserve this command value and feature bit forever, and have new per VQ as new cmd.

Alvaro,
Do you know if any software used it? Can you get some real data?

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

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

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

[virtio-dev] Re: [virtio-comment] [PATCH] virtio-net: support per-queue coalescing moderation

[Alvaro Karsz]

> On Wed, Feb 08, 2023 at 07:53:09PM +0200, Alvaro Karsz 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.
> > >
> > > > 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.
> >
> > I think that (a) is the way to go.
> > I don't think that we should work with operation modes at all.
> >
> > In my opinion:
> >
> > We should have 2 features:
> > VIRTIO_NET_F_PERQUEUE_NOTF_COAL and VIRTIO_NET_F_NOTF_COAL.
> >
> > VIRTIO_NET_F_PERQUEUE_NOTF_COAL sets per queue parameters, and
> > VIRTIO_NET_F_NOTF_COAL sets parameters for all queues.
> >
> > VIRTIO_NET_F_NOTF_COAL has 2 commands:
> >     VIRTIO_NET_CTRL_NOTF_COAL_RX_SET
> >     VIRTIO_NET_CTRL_NOTF_COAL_TX_SET
> >
> > VIRTIO_NET_F_PERQUEUE_NOTF_COAL has 2 commands:
> >     VIRTIO_NET_CTRL_NOTF_COAL_PER_QUEUE_TX_SET
> >     VIRTIO_NET_CTRL_NOTF_COAL_PER_QUEUE_RX_SET
> >
> > We can see VIRTIO_NET_CTRL_NOTF_COAL_RX_SET as a virtio level shortcut
> > for setting all queues with one command, exactly as intended with
> > rx_qid= 0xFFFF, and without breaking devices following the current
> > spec.
> >
> > The device's FW can decide if it stores parameters received with
> > VIRTIO_NET_CTRL_NOTF_COAL_RX_SET in a global set, or if it iterates
> > through all queues, but IMO the best way it to iterate through all
> > queues.
> >
> > Seems like a win-win situation to me.
> > We achieve the same functionality as described in the patch, but
> > without breaking devices following the current spec.
> >
> > Now, if we follow this method,
> > VIRTIO_NET_CTRL_NOTF_COAL_PER_QUEUE_RX_SET with rx_qid= 0xFFF seems
> > redundant.
> > If VIRTIO_NET_F_PERQUEUE_NOTF_COAL requires VIRTIO_NET_F_NOTF_COAL, a
> > device supporting VIRTIO_NET_F_PERQUEUE_NOTF_COAL can achieve the same
> > functionality with the VIRTIO_NET_CTRL_NOTF_COAL_RX_SET command.
>
> Yes. Just some comments:
>
> - I don't think we need two commands. We have RX and TX because we
>   did not have vq number previously. No we do so just pass that.
>   It's also clearer since struct name can match command exactly.
>

I agree, good point.
Just need to be sure that we won't put the control vq index in there :)
(maybe we should mention this in the conformance sections?)

> - Once we do that we can use a short _VQ_ instead of the wordy "PER_QUEUE".
>
> - Accordingly a well understood "vqn" instead of our own "qid" which
>   we then need to define.
>

You're right, I used the definitions from the proposed patch to make a
point, I wasn't trying to imply that that's what it should be called.

> - And yes no need for a reserved "qid" - it's a distinct command.
>
> --
> MST
>

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

[virtio-dev] Re: [virtio-comment] [PATCH] virtio-net: support per-queue coalescing moderation

[Alvaro Karsz]

> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Wednesday, February 8, 2023 3:52 PM
> >
> > On Wed, Feb 08, 2023 at 07:53:09PM +0200, Alvaro Karsz 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.
> > > >
> > > > > 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.
> > >
> > > I think that (a) is the way to go.
> > > I don't think that we should work with operation modes at all.
> > >
> > > In my opinion:
> > >
> > > We should have 2 features:
> > > VIRTIO_NET_F_PERQUEUE_NOTF_COAL and VIRTIO_NET_F_NOTF_COAL.
> > >
> > > VIRTIO_NET_F_PERQUEUE_NOTF_COAL sets per queue parameters, and
> > > VIRTIO_NET_F_NOTF_COAL sets parameters for all queues.
> > >
> > > VIRTIO_NET_F_NOTF_COAL has 2 commands:
> > >     VIRTIO_NET_CTRL_NOTF_COAL_RX_SET
> > >     VIRTIO_NET_CTRL_NOTF_COAL_TX_SET
> > >
> > > VIRTIO_NET_F_PERQUEUE_NOTF_COAL has 2 commands:
> > >     VIRTIO_NET_CTRL_NOTF_COAL_PER_QUEUE_TX_SET
> > >     VIRTIO_NET_CTRL_NOTF_COAL_PER_QUEUE_RX_SET
> > >
> > > We can see VIRTIO_NET_CTRL_NOTF_COAL_RX_SET as a virtio level shortcut
> > > for setting all queues with one command, exactly as intended with
> > > rx_qid= 0xFFFF, and without breaking devices following the current
> > > spec.
> > >
> > > The device's FW can decide if it stores parameters received with
> > > VIRTIO_NET_CTRL_NOTF_COAL_RX_SET in a global set, or if it iterates
> > > through all queues, but IMO the best way it to iterate through all
> > > queues.
> > >
> > > Seems like a win-win situation to me.
> > > We achieve the same functionality as described in the patch, but
> > > without breaking devices following the current spec.
> > >
> > > Now, if we follow this method,
> > > VIRTIO_NET_CTRL_NOTF_COAL_PER_QUEUE_RX_SET with rx_qid= 0xFFF
> > seems
> > > redundant.
> > > If VIRTIO_NET_F_PERQUEUE_NOTF_COAL requires
> > VIRTIO_NET_F_NOTF_COAL, a
> > > device supporting VIRTIO_NET_F_PERQUEUE_NOTF_COAL can achieve the
> > same
> > > functionality with the VIRTIO_NET_CTRL_NOTF_COAL_RX_SET command.
> >
> > Yes. Just some comments:
> >
> > - I don't think we need two commands. We have RX and TX because we
> >   did not have vq number previously. No we do so just pass that.
> >   It's also clearer since struct name can match command exactly.
> >
> +1. Consolidate the structure, followed by single command is good.
>
> > - Once we do that we can use a short _VQ_ instead of the wordy
> > "PER_QUEUE".
> >
> > - Accordingly a well understood "vqn" instead of our own "qid" which
> >   we then need to define.
> >
> > - And yes no need for a reserved "qid" - it's a distinct command.
>
> I want to ask one more time, is there any sw that used this?
> If not, we are better of just having single per vq command and everything is good.
> For extra safety, we can reserve this command value and feature bit forever, and have new per VQ as new cmd.
>
> Alvaro,
> Do you know if any software used it? Can you get some real data?

I implemented this feature in our DPU, so at least 1 vendor is using
this feature (I will add support for the per vq command of course).
I really don't know about other vendors..

You are suggesting to reserve the command and feature bit for safety,
so, if we reserve them, why not just use them? What do we lose here?

Not having this feature/command even complicates things now that we
are talking about removing the RX and TX from the per vq command, how
do you change parameters to all TX queues? to all RX queues? we'll
need 2 special indexes, so we now need le32 to hold the queue index.

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

RE: [virtio-comment] [PATCH] virtio-net: support per-queue coalescing moderation

[Parav Pandit]

> From: Alvaro Karsz <alvaro.karsz@solid-run.com>
> Sent: Wednesday, February 8, 2023 4:56 PM

> > Alvaro,
> > Do you know if any software used it? Can you get some real data?
> 
> I implemented this feature in our DPU, so at least 1 vendor is using this feature

But which software (virtio net driver) in which OS is using this?

> (I will add support for the per vq command of course).
> I really don't know about other vendors..
>
> You are suggesting to reserve the command and feature bit for safety, so, if we
> reserve them, why not just use them? What do we lose here?
> 
If it is used by some unknown software, only that sw breaks by using non release spec.
If we use it by changing the definition, it may break that unknown sw.
If we know there is no known sw, we are better of with redefinition (by adding vqn, and by removing tx,rx from it).

> Not having this feature/command even complicates things now that we are
> talking about removing the RX and TX from the per vq command, how do you
> change parameters to all TX queues? to all RX queues? we'll need 2 special
> indexes, so we now need le32 to hold the queue index.
No need for special index.
How does a driver disable all queues or reset all queues? -> One by one.
So if user want to change for all TXQ, sw can do it one by one by iterating TXQ vqns.

[virtio-dev] Re: [virtio-comment] [PATCH] virtio-net: support per-queue coalescing moderation

[Michael S. Tsirkin]

On Wed, Feb 08, 2023 at 09:05:47PM +0000, Parav Pandit wrote:
> 
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Wednesday, February 8, 2023 3:52 PM
> > 
> > On Wed, Feb 08, 2023 at 07:53:09PM +0200, Alvaro Karsz 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.
> > > >
> > > > > 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.
> > >
> > > I think that (a) is the way to go.
> > > I don't think that we should work with operation modes at all.
> > >
> > > In my opinion:
> > >
> > > We should have 2 features:
> > > VIRTIO_NET_F_PERQUEUE_NOTF_COAL and VIRTIO_NET_F_NOTF_COAL.
> > >
> > > VIRTIO_NET_F_PERQUEUE_NOTF_COAL sets per queue parameters, and
> > > VIRTIO_NET_F_NOTF_COAL sets parameters for all queues.
> > >
> > > VIRTIO_NET_F_NOTF_COAL has 2 commands:
> > >     VIRTIO_NET_CTRL_NOTF_COAL_RX_SET
> > >     VIRTIO_NET_CTRL_NOTF_COAL_TX_SET
> > >
> > > VIRTIO_NET_F_PERQUEUE_NOTF_COAL has 2 commands:
> > >     VIRTIO_NET_CTRL_NOTF_COAL_PER_QUEUE_TX_SET
> > >     VIRTIO_NET_CTRL_NOTF_COAL_PER_QUEUE_RX_SET
> > >
> > > We can see VIRTIO_NET_CTRL_NOTF_COAL_RX_SET as a virtio level shortcut
> > > for setting all queues with one command, exactly as intended with
> > > rx_qid= 0xFFFF, and without breaking devices following the current
> > > spec.
> > >
> > > The device's FW can decide if it stores parameters received with
> > > VIRTIO_NET_CTRL_NOTF_COAL_RX_SET in a global set, or if it iterates
> > > through all queues, but IMO the best way it to iterate through all
> > > queues.
> > >
> > > Seems like a win-win situation to me.
> > > We achieve the same functionality as described in the patch, but
> > > without breaking devices following the current spec.
> > >
> > > Now, if we follow this method,
> > > VIRTIO_NET_CTRL_NOTF_COAL_PER_QUEUE_RX_SET with rx_qid= 0xFFF
> > seems
> > > redundant.
> > > If VIRTIO_NET_F_PERQUEUE_NOTF_COAL requires
> > VIRTIO_NET_F_NOTF_COAL, a
> > > device supporting VIRTIO_NET_F_PERQUEUE_NOTF_COAL can achieve the
> > same
> > > functionality with the VIRTIO_NET_CTRL_NOTF_COAL_RX_SET command.
> > 
> > Yes. Just some comments:
> > 
> > - I don't think we need two commands. We have RX and TX because we
> >   did not have vq number previously. No we do so just pass that.
> >   It's also clearer since struct name can match command exactly.
> >
> +1. Consolidate the structure, followed by single command is good.
>  
> > - Once we do that we can use a short _VQ_ instead of the wordy
> > "PER_QUEUE".
> > 
> > - Accordingly a well understood "vqn" instead of our own "qid" which
> >   we then need to define.
> > 
> > - And yes no need for a reserved "qid" - it's a distinct command.
> 
> I want to ask one more time, is there any sw that used this?
> If not, we are better of just having single per vq command and everything is good.

I'm not so sure, I feel a command affecting all queues in one go is a
nice shortcut for ethtool to use. Maybe if we didn't include it
originally I would not bother but given we already have it, it seems a
waste to drop it now.
And sure, devices can just ignore the feature bit and the commands
if they want to.


> For extra safety, we can reserve this command value and feature bit forever, and have new per VQ as new cmd.
> 
> Alvaro,
> Do you know if any software used it? Can you get some real data?


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

[virtio-dev] Re: [virtio-comment] [PATCH] virtio-net: support per-queue coalescing moderation

[Michael S. Tsirkin]

On Wed, Feb 08, 2023 at 10:08:41PM +0000, Parav Pandit wrote:
> 
> > From: Alvaro Karsz <alvaro.karsz@solid-run.com>
> > Sent: Wednesday, February 8, 2023 4:56 PM
> 
> > > Alvaro,
> > > Do you know if any software used it? Can you get some real data?
> > 
> > I implemented this feature in our DPU, so at least 1 vendor is using this feature
> 
> But which software (virtio net driver) in which OS is using this?
> 
> > (I will add support for the per vq command of course).
> > I really don't know about other vendors..
> >
> > You are suggesting to reserve the command and feature bit for safety, so, if we
> > reserve them, why not just use them? What do we lose here?
> > 
> If it is used by some unknown software, only that sw breaks by using non release spec.
> If we use it by changing the definition, it may break that unknown sw.
> If we know there is no known sw, we are better of with redefinition (by adding vqn, and by removing tx,rx from it).
> 
> > Not having this feature/command even complicates things now that we are
> > talking about removing the RX and TX from the per vq command, how do you
> > change parameters to all TX queues? to all RX queues? we'll need 2 special
> > indexes, so we now need le32 to hold the queue index.
> No need for special index.
> How does a driver disable all queues or reset all queues? -> One by one.
> So if user want to change for all TXQ, sw can do it one by one by iterating TXQ vqns.

I think we can split this effort in two parts:

1. add a new command and feature bit
2. drop the old command


Looks like there's consensus on 1?  And it looks like Alvaro wants to
work on 1 but not 2. Once that is in Parav can work on 2 if he wants to.

-- 
MST


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

RE: [virtio-comment] [PATCH] virtio-net: support per-queue coalescing moderation

[Parav Pandit]

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Wednesday, February 8, 2023 5:16 PM
> > > Not having this feature/command even complicates things now that we
> > > are talking about removing the RX and TX from the per vq command,
> > > how do you change parameters to all TX queues? to all RX queues?
> > > we'll need 2 special indexes, so we now need le32 to hold the queue index.
> > No need for special index.
> > How does a driver disable all queues or reset all queues? -> One by one.
> > So if user want to change for all TXQ, sw can do it one by one by iterating TXQ
> vqns.
> 
> I think we can split this effort in two parts:
> 
> 1. add a new command and feature bit
> 2. drop the old command
> 
> 
> Looks like there's consensus on 1?  And it looks like Alvaro wants to work on 1
> but not 2. Once that is in Parav can work on 2 if he wants to.

3. rename the current one if there is no sw driver that used it.
May be lets wait for Alvaro's answer if there is any sw that used.

Doing #3 would be the simplest for all devices and sw to implement and consume respectively.

I can work with Alvaro to have 1 and 2 as part of same series as two patches, so that we don't have to re-discuss this again.
Without that 
a. negotiating two bits at same time is hard.
b. Wording #1 also need to mention about what happens when global is also negotiated and device is in partial mode.

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

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

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

[virtio-dev] Re: [virtio-comment] [PATCH] virtio-net: support per-queue coalescing moderation

[Michael S. Tsirkin]

On Wed, Feb 08, 2023 at 10:23:00PM +0000, Parav Pandit wrote:
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Wednesday, February 8, 2023 5:16 PM
> > > > Not having this feature/command even complicates things now that we
> > > > are talking about removing the RX and TX from the per vq command,
> > > > how do you change parameters to all TX queues? to all RX queues?
> > > > we'll need 2 special indexes, so we now need le32 to hold the queue index.
> > > No need for special index.
> > > How does a driver disable all queues or reset all queues? -> One by one.
> > > So if user want to change for all TXQ, sw can do it one by one by iterating TXQ
> > vqns.
> > 
> > I think we can split this effort in two parts:
> > 
> > 1. add a new command and feature bit
> > 2. drop the old command
> > 
> > 
> > Looks like there's consensus on 1?  And it looks like Alvaro wants to work on 1
> > but not 2. Once that is in Parav can work on 2 if he wants to.
> 
> 3. rename the current one if there is no sw driver that used it.

That's probably the worst of all options, silently changing behaviour.

> May be lets wait for Alvaro's answer if there is any sw that used.
> 
> Doing #3 would be the simplest for all devices and sw to implement and consume respectively.
> 
> I can work with Alvaro to have 1 and 2 as part of same series as two patches, so that we don't have to re-discuss this again.
> Without that 
> a. negotiating two bits at same time is hard.
> b. Wording #1 also need to mention about what happens when global is also negotiated and device is in partial mode.

There's no mode, partial or otherwise, in either Alvaro's or my
proposal.

-- 
MST


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

RE: [virtio-comment] [PATCH] virtio-net: support per-queue coalescing moderation

[Parav Pandit]

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Wednesday, February 8, 2023 5:29 PM
> 
> On Wed, Feb 08, 2023 at 10:23:00PM +0000, Parav Pandit wrote:
> > > From: Michael S. Tsirkin <mst@redhat.com>
> > > Sent: Wednesday, February 8, 2023 5:16 PM
> > > > > Not having this feature/command even complicates things now that
> > > > > we are talking about removing the RX and TX from the per vq
> > > > > command, how do you change parameters to all TX queues? to all RX
> queues?
> > > > > we'll need 2 special indexes, so we now need le32 to hold the queue
> index.
> > > > No need for special index.
> > > > How does a driver disable all queues or reset all queues? -> One by one.
> > > > So if user want to change for all TXQ, sw can do it one by one by
> > > > iterating TXQ
> > > vqns.
> > >
> > > I think we can split this effort in two parts:
> > >
> > > 1. add a new command and feature bit 2. drop the old command
> > >
> > >
> > > Looks like there's consensus on 1?  And it looks like Alvaro wants
> > > to work on 1 but not 2. Once that is in Parav can work on 2 if he wants to.
> >
> > 3. rename the current one if there is no sw driver that used it.
> 
> That's probably the worst of all options, silently changing behaviour.
> 
How come? Once #2 is done, only #1 is left and that is equivalent silence.
It is effectively #3. So why not do in same series?

> > May be lets wait for Alvaro's answer if there is any sw that used.
> >
> > Doing #3 would be the simplest for all devices and sw to implement and
> consume respectively.
> >
> > I can work with Alvaro to have 1 and 2 as part of same series as two patches,
> so that we don't have to re-discuss this again.
> > Without that
> > a. negotiating two bits at same time is hard.
> > b. Wording #1 also need to mention about what happens when global is also
> negotiated and device is in partial mode.
> 
> There's no mode, partial or otherwise, in either Alvaro's or my proposal.
It is clearly ambitious spec for some unknown software to be in mixed mode that unlikely to find user for.


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

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

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

[virtio-dev] Re: [virtio-comment] [PATCH] virtio-net: support per-queue coalescing moderation

[Alvaro Karsz]

> > From: Alvaro Karsz <alvaro.karsz@solid-run.com>
> > Sent: Wednesday, February 8, 2023 4:56 PM
>
> > > Alvaro,
> > > Do you know if any software used it? Can you get some real data?
> >
> > I implemented this feature in our DPU, so at least 1 vendor is using this feature
>
> But which software (virtio net driver) in which OS is using this?

Sorry, I'm not sure I understand your question.

The feature is implemented in the linux kernel
https://github.com/torvalds/linux/commit/699b045a8e43bd1063db4795be685bfd659649dc
So we'll always have kernel versions accepting this feature, if offered.

> > (I will add support for the per vq command of course).
> > I really don't know about other vendors..
> >
> > You are suggesting to reserve the command and feature bit for safety, so, if we
> > reserve them, why not just use them? What do we lose here?
> >
> If it is used by some unknown software, only that sw breaks by using non release spec.
> If we use it by changing the definition, it may break that unknown sw.
> If we know there is no known sw, we are better of with redefinition (by adding vqn, and by removing tx,rx from it).
>
> > Not having this feature/command even complicates things now that we are
> > talking about removing the RX and TX from the per vq command, how do you
> > change parameters to all TX queues? to all RX queues? we'll need 2 special
> > indexes, so we now need le32 to hold the queue index.
> No need for special index.
> How does a driver disable all queues or reset all queues? -> One by one.
> So if user want to change for all TXQ, sw can do it one by one by iterating TXQ vqns.

Yes, but resetting the queues doesn't require a control command.
If a server has 64K queues, and a user wants to set all coalescing
parameters to X (maybe with ethtool), it will generate 64K control
commands..

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

Re: [virtio-comment] [PATCH] virtio-net: support per-queue coalescing moderation

[Alvaro Karsz]

Maybe we should do something more future friendly.
We may want to add more coalescing related features in the future.
* Maybe max/min sets?
* Maybe to offload the adaptive algo to the DPU?

It will require more feature bits..

We could rename this new feature from VIRTIO_NET_F_PERQUEUE_NOTF_COAL
to something like VIRTIO_NET_F_ADVANCED_NOTF_COAL, and introduce a new
bitmask in the virto_net_config.

We can implement just one bit at first.
BIT 0 - device supports per queue coalescing sets

Then, we could add more features in the future without having to use
more feature bits.

Every device that offers VIRTIO_NET_F_ADVANCED_NOTF_COAL should
maintain a valid bitmask.

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

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

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

Re: [virtio-comment] [PATCH] virtio-net: support per-queue coalescing moderation

[Michael S. Tsirkin]

On Thu, Feb 09, 2023 at 12:45:42AM +0200, Alvaro Karsz wrote:
> Maybe we should do something more future friendly.
> We may want to add more coalescing related features in the future.
> * Maybe max/min sets?
> * Maybe to offload the adaptive algo to the DPU?
> 
> It will require more feature bits..
> 
> We could rename this new feature from VIRTIO_NET_F_PERQUEUE_NOTF_COAL
> to something like VIRTIO_NET_F_ADVANCED_NOTF_COAL, and introduce a new
> bitmask in the virto_net_config.
> 
> We can implement just one bit at first.
> BIT 0 - device supports per queue coalescing sets
> 
> Then, we could add more features in the future without having to use
> more feature bits.
> 
> Every device that offers VIRTIO_NET_F_ADVANCED_NOTF_COAL should
> maintain a valid bitmask.

Without at least two bits there it sounds like over-engineering to me:
we won't really know whether the fancy infrastructure is good for
any extensions.

-- 
MST


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

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

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

Re: [virtio-comment] [PATCH] virtio-net: support per-queue coalescing moderation

[Michael S. Tsirkin]

On Thu, Feb 09, 2023 at 12:35:42AM +0200, Alvaro Karsz wrote:
> > > From: Alvaro Karsz <alvaro.karsz@solid-run.com>
> > > Sent: Wednesday, February 8, 2023 4:56 PM
> >
> > > > Alvaro,
> > > > Do you know if any software used it? Can you get some real data?
> > >
> > > I implemented this feature in our DPU, so at least 1 vendor is using this feature
> >
> > But which software (virtio net driver) in which OS is using this?
> 
> Sorry, I'm not sure I understand your question.
> 
> The feature is implemented in the linux kernel
> https://github.com/torvalds/linux/commit/699b045a8e43bd1063db4795be685bfd659649dc
> So we'll always have kernel versions accepting this feature, if offered.
> 
> > > (I will add support for the per vq command of course).
> > > I really don't know about other vendors..
> > >
> > > You are suggesting to reserve the command and feature bit for safety, so, if we
> > > reserve them, why not just use them? What do we lose here?
> > >
> > If it is used by some unknown software, only that sw breaks by using non release spec.
> > If we use it by changing the definition, it may break that unknown sw.
> > If we know there is no known sw, we are better of with redefinition (by adding vqn, and by removing tx,rx from it).
> >
> > > Not having this feature/command even complicates things now that we are
> > > talking about removing the RX and TX from the per vq command, how do you
> > > change parameters to all TX queues? to all RX queues? we'll need 2 special
> > > indexes, so we now need le32 to hold the queue index.
> > No need for special index.
> > How does a driver disable all queues or reset all queues? -> One by one.
> > So if user want to change for all TXQ, sw can do it one by one by iterating TXQ vqns.
> 
> Yes, but resetting the queues doesn't require a control command.
> If a server has 64K queues, and a user wants to set all coalescing
> parameters to X (maybe with ethtool), it will generate 64K control
> commands..

Amount of time spent on bike-shedding here is mind-boggling.  Pls just
send a new version and we'll go from there.  In the end it's a majority
we don't have to have consensus if we can't get it.

-- 
MST


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

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

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

RE: [virtio-comment] [PATCH] virtio-net: support per-queue coalescing moderation

[Parav Pandit]

> From: Alvaro Karsz <alvaro.karsz@solid-run.com>
> Sent: Wednesday, February 8, 2023 5:36 PM
> 
> > > From: Alvaro Karsz <alvaro.karsz@solid-run.com>
> > > Sent: Wednesday, February 8, 2023 4:56 PM
> >
> > > > Alvaro,
> > > > Do you know if any software used it? Can you get some real data?
> > >
> > > I implemented this feature in our DPU, so at least 1 vendor is using
> > > this feature
> >
> > But which software (virtio net driver) in which OS is using this?
> 
> Sorry, I'm not sure I understand your question.
> 
> The feature is implemented in the linux kernel

Ok. so, there is sw using it. This global feature bit stays.

> > How does a driver disable all queues or reset all queues? -> One by one.
> > So if user want to change for all TXQ, sw can do it one by one by iterating TXQ
> vqns.
> 
> Yes, but resetting the queues doesn't require a control command.
> If a server has 64K queues, and a user wants to set all coalescing parameters to
> X (maybe with ethtool), it will generate 64K control commands..
He he. What an example.
64K control cmds. 64 register writes and poll. All same from sw pov.

The conclusion is there is sw using the feature bit defined in spec 1.3. Discussion ends.
I wish you pointed to this sw way earlier in the discussion.

Please proceed with the new feature bit and new command.



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

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

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

[virtio-comment] Re: [virtio-dev] Re: [virtio-comment] [PATCH] virtio-net: support per-queue coalescing moderation

[Heng Qi]

在 2023/2/9 上午1:53, Alvaro Karsz 写道:
>   > > 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.
> I think that (a) is the way to go.
> I don't think that we should work with operation modes at all.

I agree to keep the current global settings (VIRTIO_NET_F_NOTF_COAL and 
its corresponding commands),
because our hardware team has limited resources for the control queue, 
and they don't want to send a separate
cmd for each queue when send a global setting cmd.

Then adding a VIRTIO_NET_F_PERQUEUE_NOTF_COAL or 
VIRTIO_NET_F_VQ_NOTF_COAL feature bit and new commands for per-queue or 
VQ setting looks better to me.
> In my opinion:
>
> We should have 2 features:
> VIRTIO_NET_F_PERQUEUE_NOTF_COAL and VIRTIO_NET_F_NOTF_COAL.
>
> VIRTIO_NET_F_PERQUEUE_NOTF_COAL sets per queue parameters, and
> VIRTIO_NET_F_NOTF_COAL sets parameters for all queues.
>
> VIRTIO_NET_F_NOTF_COAL has 2 commands:
>      VIRTIO_NET_CTRL_NOTF_COAL_RX_SET
>      VIRTIO_NET_CTRL_NOTF_COAL_TX_SET
>
> VIRTIO_NET_F_PERQUEUE_NOTF_COAL has 2 commands:
>      VIRTIO_NET_CTRL_NOTF_COAL_PER_QUEUE_TX_SET
>      VIRTIO_NET_CTRL_NOTF_COAL_PER_QUEUE_RX_SET
>
> We can see VIRTIO_NET_CTRL_NOTF_COAL_RX_SET as a virtio level shortcut
> for setting all queues with one command, exactly as intended with
> rx_qid= 0xFFFF, and without breaking devices following the current
> spec.

Yes.

>
> The device's FW can decide if it stores parameters received with
> VIRTIO_NET_CTRL_NOTF_COAL_RX_SET in a global set, or if it iterates
> through all queues, but IMO the best way it to iterate through all
> queues.
>
> Seems like a win-win situation to me.
> We achieve the same functionality as described in the patch, but
> without breaking devices following the current spec.
>
> Now, if we follow this method,
> VIRTIO_NET_CTRL_NOTF_COAL_PER_QUEUE_RX_SET with rx_qid= 0xFFF seems
> redundant.

Yes, I'll remove this.

Thanks.

> If VIRTIO_NET_F_PERQUEUE_NOTF_COAL requires VIRTIO_NET_F_NOTF_COAL, a
> device supporting VIRTIO_NET_F_PERQUEUE_NOTF_COAL can achieve the same
> functionality with the VIRTIO_NET_CTRL_NOTF_COAL_RX_SET command.
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org


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

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

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

[virtio-comment] Re: [virtio-dev] Re: [virtio-comment] [PATCH] virtio-net: support per-queue coalescing moderation

[Heng Qi]

在 2023/2/9 上午6:35, Alvaro Karsz 写道:
>>> From: Alvaro Karsz <alvaro.karsz@solid-run.com>
>>> Sent: Wednesday, February 8, 2023 4:56 PM
>>>> Alvaro,
>>>> Do you know if any software used it? Can you get some real data?
>>> I implemented this feature in our DPU, so at least 1 vendor is using this feature
>> But which software (virtio net driver) in which OS is using this?
> Sorry, I'm not sure I understand your question.
>
> The feature is implemented in the linux kernel
> https://github.com/torvalds/linux/commit/699b045a8e43bd1063db4795be685bfd659649dc
> So we'll always have kernel versions accepting this feature, if offered.
>
>>> (I will add support for the per vq command of course).
>>> I really don't know about other vendors..
>>>
>>> You are suggesting to reserve the command and feature bit for safety, so, if we
>>> reserve them, why not just use them? What do we lose here?
>>>
>> If it is used by some unknown software, only that sw breaks by using non release spec.
>> If we use it by changing the definition, it may break that unknown sw.
>> If we know there is no known sw, we are better of with redefinition (by adding vqn, and by removing tx,rx from it).
>>
>>> Not having this feature/command even complicates things now that we are
>>> talking about removing the RX and TX from the per vq command, how do you
>>> change parameters to all TX queues? to all RX queues? we'll need 2 special
>>> indexes, so we now need le32 to hold the queue index.
>> No need for special index.
>> How does a driver disable all queues or reset all queues? -> One by one.
>> So if user want to change for all TXQ, sw can do it one by one by iterating TXQ vqns.
> Yes, but resetting the queues doesn't require a control command.
> If a server has 64K queues, and a user wants to set all coalescing
> parameters to X (maybe with ethtool), it will generate 64K control
> commands..

At least our hardware design doesn't expect that.

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


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

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

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

Re: [virtio-comment] [PATCH] virtio-net: support per-queue coalescing moderation

[Heng Qi]

在 2023/2/9 上午4:52, Michael S. Tsirkin 写道:
> On Wed, Feb 08, 2023 at 07:53:09PM +0200, Alvaro Karsz 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.
>>>
>>>> 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.
>> I think that (a) is the way to go.
>> I don't think that we should work with operation modes at all.
>>
>> In my opinion:
>>
>> We should have 2 features:
>> VIRTIO_NET_F_PERQUEUE_NOTF_COAL and VIRTIO_NET_F_NOTF_COAL.
>>
>> VIRTIO_NET_F_PERQUEUE_NOTF_COAL sets per queue parameters, and
>> VIRTIO_NET_F_NOTF_COAL sets parameters for all queues.
>>
>> VIRTIO_NET_F_NOTF_COAL has 2 commands:
>>      VIRTIO_NET_CTRL_NOTF_COAL_RX_SET
>>      VIRTIO_NET_CTRL_NOTF_COAL_TX_SET
>>
>> VIRTIO_NET_F_PERQUEUE_NOTF_COAL has 2 commands:
>>      VIRTIO_NET_CTRL_NOTF_COAL_PER_QUEUE_TX_SET
>>      VIRTIO_NET_CTRL_NOTF_COAL_PER_QUEUE_RX_SET
>>
>> We can see VIRTIO_NET_CTRL_NOTF_COAL_RX_SET as a virtio level shortcut
>> for setting all queues with one command, exactly as intended with
>> rx_qid= 0xFFFF, and without breaking devices following the current
>> spec.
>>
>> The device's FW can decide if it stores parameters received with
>> VIRTIO_NET_CTRL_NOTF_COAL_RX_SET in a global set, or if it iterates
>> through all queues, but IMO the best way it to iterate through all
>> queues.
>>
>> Seems like a win-win situation to me.
>> We achieve the same functionality as described in the patch, but
>> without breaking devices following the current spec.
>>
>> Now, if we follow this method,
>> VIRTIO_NET_CTRL_NOTF_COAL_PER_QUEUE_RX_SET with rx_qid= 0xFFF seems
>> redundant.
>> If VIRTIO_NET_F_PERQUEUE_NOTF_COAL requires VIRTIO_NET_F_NOTF_COAL, a
>> device supporting VIRTIO_NET_F_PERQUEUE_NOTF_COAL can achieve the same
>> functionality with the VIRTIO_NET_CTRL_NOTF_COAL_RX_SET command.
> Yes. Just some comments:
>
> - I don't think we need two commands. We have RX and TX because we
>    did not have vq number previously. No we do so just pass that.
>    It's also clearer since struct name can match command exactly.

Yes, I believe I got your point yesterday.

>
> - Once we do that we can use a short _VQ_ instead of the wordy "PER_QUEUE".

This seems clearer and more straightforward.

> - Accordingly a well understood "vqn" instead of our own "qid" which
>    we then need to define.

Sure.

Thanks.

>
> - And yes no need for a reserved "qid" - it's a distinct command.
>


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

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

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

[virtio-comment] Re: [virtio-dev] RE: [virtio-comment] [PATCH] virtio-net: support per-queue coalescing moderation

[Heng Qi]

在 2023/2/8 下午11:04, Parav Pandit 写道:
>
>> 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.

I think it should be a. I think we should blur the concept of mode. 
There seems to be no mode here.
 From the perspective of the device, it only needs to distinguish 
commands and do what it should do.

Thanks.

>
> A better semantic is to define such change at device level and no extra cost in the data path.
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org


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

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

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