[virtio-dev] [PATCH] virtio-net: Fix and update VIRTIO_NET_F_NOTF_COAL feature

[virtio-dev] [PATCH] virtio-net: Fix and update VIRTIO_NET_F_NOTF_COAL feature

[Alvaro Karsz]

This patch makes several improvements to the notification coalescing
feature, including:

- Consolidating virtio_net_ctrl_coal_tx and virtio_net_ctrl_coal_rx
  into a single struct, virtio_net_ctrl_coal, as they are identical.
- Emphasizing that the coalescing commands are best-effort.
- Defining the behavior of coalescing with regards to delivering
  notifications when a change occur.

Signed-off-by: Alvaro Karsz <alvaro.karsz@solid-run.com>
---
 device-types/net/description.tex | 40 ++++++++++++++++++--------------
 1 file changed, 22 insertions(+), 18 deletions(-)

diff --git a/device-types/net/description.tex b/device-types/net/description.tex
index 1741c79..2a98411 100644
--- a/device-types/net/description.tex
+++ b/device-types/net/description.tex
@@ -1514,15 +1514,12 @@ \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.
 
-\begin{lstlisting}
-struct virtio_net_ctrl_coal_rx {
-    le32 rx_max_packets;
-    le32 rx_usecs;
-};
+Note: In general, these commands are best-effort: A device could send a notification even if it is not supposed to.
 
-struct virtio_net_ctrl_coal_tx {
-    le32 tx_max_packets;
-    le32 tx_usecs;
+\begin{lstlisting}
+struct virtio_net_ctrl_coal {
+    le32 max_packets;
+    le32 usecs;
 };
 
 #define VIRTIO_NET_CTRL_NOTF_COAL 6
@@ -1532,25 +1529,25 @@ \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_max_packets}: Maximum number of packets to receive before a RX notification.
-\item \field{tx_max_packets}: Maximum number of packets to send before a TX notification.
+\item \field{usecs} for RX: Maximum number of usecs to delay a RX notification.
+\item \field{usecs} for TX: Maximum number of usecs to delay a TX notification.
+\item \field{max_packets} for RX: Maximum number of packets to receive before a RX notification.
+\item \field{max_packets} for TX: Maximum number of packets to send before a TX notification.
 \end{itemize}
 
 
 The class VIRTIO_NET_CTRL_NOTF_COAL has 2 commands:
 \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{usecs} and \field{max_packets} parameters for TX.
+\item VIRTIO_NET_CTRL_NOTF_COAL_RX_SET: set the \field{usecs} and \field{max_packets} parameters for RX.
 \end{enumerate}
 
 \subparagraph{RX Notifications}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Notifications Coalescing / RX Notifications}
 
 If, for example:
 \begin{itemize}
-\item \field{rx_usecs} = 10.
-\item \field{rx_max_packets} = 15.
+\item \field{usecs} = 10.
+\item \field{max_packets} = 15.
 \end{itemize}
 
 The device will operate as follows:
@@ -1564,8 +1561,8 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
 
 If, for example:
 \begin{itemize}
-\item \field{tx_usecs} = 10.
-\item \field{tx_max_packets} = 15.
+\item \field{usecs} = 10.
+\item \field{max_packets} = 15.
 \end{itemize}
 
 The device will operate as follows:
@@ -1575,6 +1572,13 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
 \item If the notifications are not suppressed by the driver, the device will send an used buffer notification, otherwise, the device will not send an used buffer notification as long as the notifications are suppressed.
 \end{itemize}
 
+\subparagraph{Notifications When Coalescing Parameters Change}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Notifications Coalescing / Notifications When Coalescing Parameters Change}
+
+When a device changes the coalescing parameters, the device needs to check if the new parameters are met and issue a notification if so.
+
+For example, \field{max_packets} = 15 for TX.
+If the device sends 10 packets, then it receives a VIRTIO_NET_CTRL_NOTF_COAL_TX_SET command with \field{max_packets} = 8, the device needs to immediately send a TX notification, if the notifications are not suppressed by the driver.
+
 \drivernormative{\subparagraph}{Notifications Coalescing}{Device Types / Network Device / Device Operation / Control Virtqueue / Notifications Coalescing}
 
 If the VIRTIO_NET_F_NOTF_COAL feature has not been negotiated, the driver MUST NOT issue VIRTIO_NET_CTRL_NOTF_COAL commands.
-- 
2.34.1


---------------------------------------------------------------------
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] [PATCH] virtio-net: Fix and update VIRTIO_NET_F_NOTF_COAL feature

[Parav Pandit]

> From: virtio-dev@lists.oasis-open.org <virtio-dev@lists.oasis-open.org> On
> Behalf Of Alvaro Karsz
> 
> This patch makes several improvements to the notification coalescing feature,
> including:
> 
> - Consolidating virtio_net_ctrl_coal_tx and virtio_net_ctrl_coal_rx
>   into a single struct, virtio_net_ctrl_coal, as they are identical.
> - Emphasizing that the coalescing commands are best-effort.
> - Defining the behavior of coalescing with regards to delivering
>   notifications when a change occur.
>
Patch needs to do one thing at a time.
Please split above into three patches.
 
> Signed-off-by: Alvaro Karsz <alvaro.karsz@solid-run.com>
> ---
>  device-types/net/description.tex | 40 ++++++++++++++++++--------------
>  1 file changed, 22 insertions(+), 18 deletions(-)
> 
> diff --git a/device-types/net/description.tex b/device-types/net/description.tex
> index 1741c79..2a98411 100644
> --- a/device-types/net/description.tex
> +++ b/device-types/net/description.tex
> @@ -1514,15 +1514,12 @@ \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.
> 
> -\begin{lstlisting}
> -struct virtio_net_ctrl_coal_rx {
> -    le32 rx_max_packets;
> -    le32 rx_usecs;
> -};
> +Note: In general, these commands are best-effort: A device could send a
> notification even if it is not supposed to.
>
Please remove this note from this patch.
Instead of Note, we need to describe this device requirements description.
We better need to describe the motivation for it.
We may want to say there may be jitter in notification, but device should not be sending when it is not supposed to.

I also have more description to add in this area with regards to GSO and LRO.
I make humble suggestion that we draft is jointly in separate patch combining these clarifications.
 
> -struct virtio_net_ctrl_coal_tx {
> -    le32 tx_max_packets;
> -    le32 tx_usecs;
> +\begin{lstlisting}
> +struct virtio_net_ctrl_coal {
> +    le32 max_packets;
> +    le32 usecs;
>  };
> 
This is one good change to go as separate patch.

>  #define VIRTIO_NET_CTRL_NOTF_COAL 6
> @@ -1532,25 +1529,25 @@ \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_max_packets}: Maximum number of packets to receive before
> a RX notification.
> -\item \field{tx_max_packets}: Maximum number of packets to send before a
> TX notification.
> +\item \field{usecs} for RX: Maximum number of usecs to delay a RX
> notification.
> +\item \field{usecs} for TX: Maximum number of usecs to delay a TX
> notification.
> +\item \field{max_packets} for RX: Maximum number of packets to receive
> before a RX notification.
> +\item \field{max_packets} for TX: Maximum number of packets to send before
> a TX notification.
>  \end{itemize}
> 
s/for Rx/For receive virtqueue
s/for Tx/For transmit virtqueue
> 
>  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{usecs} and
> \field{max_packets} parameters for TX.
> +\item VIRTIO_NET_CTRL_NOTF_COAL_RX_SET: set the \field{usecs} and
> \field{max_packets} parameters for RX.
>  \end{enumerate}
> 
>  \subparagraph{RX Notifications}\label{sec:Device Types / Network Device /
> Device Operation / Control Virtqueue / Notifications Coalescing / RX
> Notifications}
> 
>  If, for example:
>  \begin{itemize}
> -\item \field{rx_usecs} = 10.
> -\item \field{rx_max_packets} = 15.
> +\item \field{usecs} = 10.
> +\item \field{max_packets} = 15.
>  \end{itemize}
> 
>  The device will operate as follows:
> @@ -1564,8 +1561,8 @@ \subsubsection{Control Virtqueue}\label{sec:Device
> Types / Network Device / Devi
> 
>  If, for example:
>  \begin{itemize}
> -\item \field{tx_usecs} = 10.
> -\item \field{tx_max_packets} = 15.
> +\item \field{usecs} = 10.
> +\item \field{max_packets} = 15.
>  \end{itemize}
> 
>  The device will operate as follows:
> @@ -1575,6 +1572,13 @@ \subsubsection{Control
> Virtqueue}\label{sec:Device Types / Network Device / Devi  \item If the
> notifications are not suppressed by the driver, the device will send an used
> buffer notification, otherwise, the device will not send an used buffer
> notification as long as the notifications are suppressed.
>  \end{itemize}
> 
> +\subparagraph{Notifications When Coalescing Parameters
> +Change}\label{sec:Device Types / Network Device / Device Operation /
> +Control Virtqueue / Notifications Coalescing / Notifications When
> +Coalescing Parameters Change}
> +
> +When a device changes the coalescing parameters, the device needs to check
> if the new parameters are met and issue a notification if so.
> +
> +For example, \field{max_packets} = 15 for TX.
> +If the device sends 10 packets, then it receives a
> VIRTIO_NET_CTRL_NOTF_COAL_TX_SET command with \field{max_packets} =
> 8, the device needs to immediately send a TX notification, if the notifications
> are not suppressed by the driver.
> +
Its better written as,
When device applies new coalescing parameters, if the new parameters already meet the current packet counters, device SHOULD generate immediate notification.

For example, current \field{max_packets} is 15 for transmit virtqueue, and device already sent 10 packets.
When device receives a VIRTIO_NET_CTRL_NOTF_COAL_TX_SET command, the device SHOULD immediately send a TX notification.

Above clarification also should be in same patch series as second patch.

>  \drivernormative{\subparagraph}{Notifications Coalescing}{Device Types /
> Network Device / Device Operation / Control Virtqueue / Notifications
> Coalescing}
> 
>  If the VIRTIO_NET_F_NOTF_COAL feature has not been negotiated, the driver
> MUST NOT issue VIRTIO_NET_CTRL_NOTF_COAL commands.
> --
> 2.34.1
> 
> 
> ---------------------------------------------------------------------
> 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-dev] [PATCH] virtio-net: Fix and update VIRTIO_NET_F_NOTF_COAL feature

[Michael S. Tsirkin]

On Mon, Feb 06, 2023 at 07:13:43PM +0000, Parav Pandit wrote:
> 
> 
> > From: virtio-dev@lists.oasis-open.org <virtio-dev@lists.oasis-open.org> On
> > Behalf Of Alvaro Karsz
> > 
> > This patch makes several improvements to the notification coalescing feature,
> > including:
> > 
> > - Consolidating virtio_net_ctrl_coal_tx and virtio_net_ctrl_coal_rx
> >   into a single struct, virtio_net_ctrl_coal, as they are identical.
> > - Emphasizing that the coalescing commands are best-effort.
> > - Defining the behavior of coalescing with regards to delivering
> >   notifications when a change occur.
> >
> Patch needs to do one thing at a time.
> Please split above into three patches.
>  
> > Signed-off-by: Alvaro Karsz <alvaro.karsz@solid-run.com>
> > ---
> >  device-types/net/description.tex | 40 ++++++++++++++++++--------------
> >  1 file changed, 22 insertions(+), 18 deletions(-)
> > 
> > diff --git a/device-types/net/description.tex b/device-types/net/description.tex
> > index 1741c79..2a98411 100644
> > --- a/device-types/net/description.tex
> > +++ b/device-types/net/description.tex
> > @@ -1514,15 +1514,12 @@ \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.
> > 
> > -\begin{lstlisting}
> > -struct virtio_net_ctrl_coal_rx {
> > -    le32 rx_max_packets;
> > -    le32 rx_usecs;
> > -};
> > +Note: In general, these commands are best-effort: A device could send a
> > notification even if it is not supposed to.
> >
> Please remove this note from this patch.
> Instead of Note, we need to describe this device requirements description.
> We better need to describe the motivation for it.
> We may want to say there may be jitter in notification, but device should not be sending when it is not supposed to.

It's explicitly allowed:

split-ring.tex:The driver MUST handle spurious notifications from the device.
split-ring.tex:The device MUST handle spurious notifications from the driver.



> I also have more description to add in this area with regards to GSO and LRO.
> I make humble suggestion that we draft is jointly in separate patch combining these clarifications.
>  
> > -struct virtio_net_ctrl_coal_tx {
> > -    le32 tx_max_packets;
> > -    le32 tx_usecs;
> > +\begin{lstlisting}
> > +struct virtio_net_ctrl_coal {
> > +    le32 max_packets;
> > +    le32 usecs;
> >  };
> > 
> This is one good change to go as separate patch.
> 
> >  #define VIRTIO_NET_CTRL_NOTF_COAL 6
> > @@ -1532,25 +1529,25 @@ \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_max_packets}: Maximum number of packets to receive before
> > a RX notification.
> > -\item \field{tx_max_packets}: Maximum number of packets to send before a
> > TX notification.
> > +\item \field{usecs} for RX: Maximum number of usecs to delay a RX
> > notification.
> > +\item \field{usecs} for TX: Maximum number of usecs to delay a TX
> > notification.
> > +\item \field{max_packets} for RX: Maximum number of packets to receive
> > before a RX notification.
> > +\item \field{max_packets} for TX: Maximum number of packets to send before
> > a TX notification.
> >  \end{itemize}
> > 
> s/for Rx/For receive virtqueue
> s/for Tx/For transmit virtqueue

Which virtqueue? It says TX/RX pretty consistently in this text.
Changing to receive virtqueue/transmit virtqueue would be a big
change and frankly for a very modest gain in readability.
Rather maybe just say RX/TX where we describe virtqueue.

> > 
> >  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{usecs} and
> > \field{max_packets} parameters for TX.
> > +\item VIRTIO_NET_CTRL_NOTF_COAL_RX_SET: set the \field{usecs} and
> > \field{max_packets} parameters for RX.
> >  \end{enumerate}
> > 
> >  \subparagraph{RX Notifications}\label{sec:Device Types / Network Device /
> > Device Operation / Control Virtqueue / Notifications Coalescing / RX
> > Notifications}
> > 
> >  If, for example:
> >  \begin{itemize}
> > -\item \field{rx_usecs} = 10.
> > -\item \field{rx_max_packets} = 15.
> > +\item \field{usecs} = 10.
> > +\item \field{max_packets} = 15.
> >  \end{itemize}
> > 
> >  The device will operate as follows:
> > @@ -1564,8 +1561,8 @@ \subsubsection{Control Virtqueue}\label{sec:Device
> > Types / Network Device / Devi
> > 
> >  If, for example:
> >  \begin{itemize}
> > -\item \field{tx_usecs} = 10.
> > -\item \field{tx_max_packets} = 15.
> > +\item \field{usecs} = 10.
> > +\item \field{max_packets} = 15.
> >  \end{itemize}
> > 
> >  The device will operate as follows:
> > @@ -1575,6 +1572,13 @@ \subsubsection{Control
> > Virtqueue}\label{sec:Device Types / Network Device / Devi  \item If the
> > notifications are not suppressed by the driver, the device will send an used
> > buffer notification, otherwise, the device will not send an used buffer
> > notification as long as the notifications are suppressed.
> >  \end{itemize}
> > 
> > +\subparagraph{Notifications When Coalescing Parameters
> > +Change}\label{sec:Device Types / Network Device / Device Operation /
> > +Control Virtqueue / Notifications Coalescing / Notifications When
> > +Coalescing Parameters Change}
> > +
> > +When a device changes the coalescing parameters, the device needs to check
> > if the new parameters are met and issue a notification if so.
> > +
> > +For example, \field{max_packets} = 15 for TX.
> > +If the device sends 10 packets, then it receives a
> > VIRTIO_NET_CTRL_NOTF_COAL_TX_SET command with \field{max_packets} =
> > 8, the device needs to immediately send a TX notification, if the notifications
> > are not suppressed by the driver.
> > +
> Its better written as,
> When device applies new coalescing parameters, if the new parameters already meet the current packet counters, device SHOULD generate immediate notification.

what are packet counters though? they aren't defined anywhere.

> For example, current \field{max_packets} is 15 for transmit virtqueue, and device already sent 10 packets.

I assume it is all per virtqueue? Makes sense but it does not actually
say anywhere.

> When device receives a VIRTIO_NET_CTRL_NOTF_COAL_TX_SET command, the device SHOULD immediately send a TX notification.

always? why?

> Above clarification also should be in same patch series as second patch.
> 
> >  \drivernormative{\subparagraph}{Notifications Coalescing}{Device Types /
> > Network Device / Device Operation / Control Virtqueue / Notifications
> > Coalescing}
> > 
> >  If the VIRTIO_NET_F_NOTF_COAL feature has not been negotiated, the driver
> > MUST NOT issue VIRTIO_NET_CTRL_NOTF_COAL commands.
> > --
> > 2.34.1
> > 
> > 
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> > For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org


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

[virtio-comment] RE: [virtio-dev] [PATCH] virtio-net: Fix and update VIRTIO_NET_F_NOTF_COAL feature

[Parav Pandit]

> From: virtio-dev@lists.oasis-open.org <virtio-dev@lists.oasis-open.org> On
> Behalf Of Michael S. Tsirkin
> 
> On Mon, Feb 06, 2023 at 07:13:43PM +0000, Parav Pandit wrote:
> >
> >
> > > From: virtio-dev@lists.oasis-open.org
> > > <virtio-dev@lists.oasis-open.org> On Behalf Of Alvaro Karsz
> > >
> > > This patch makes several improvements to the notification coalescing
> > > feature,
> > > including:
> > >
> > > - Consolidating virtio_net_ctrl_coal_tx and virtio_net_ctrl_coal_rx
> > >   into a single struct, virtio_net_ctrl_coal, as they are identical.
> > > - Emphasizing that the coalescing commands are best-effort.
> > > - Defining the behavior of coalescing with regards to delivering
> > >   notifications when a change occur.
> > >
> > Patch needs to do one thing at a time.
> > Please split above into three patches.
> >
> > > Signed-off-by: Alvaro Karsz <alvaro.karsz@solid-run.com>
> > > ---
> > >  device-types/net/description.tex | 40
> > > ++++++++++++++++++--------------
> > >  1 file changed, 22 insertions(+), 18 deletions(-)
> > >
> > > diff --git a/device-types/net/description.tex
> > > b/device-types/net/description.tex
> > > index 1741c79..2a98411 100644
> > > --- a/device-types/net/description.tex
> > > +++ b/device-types/net/description.tex
> > > @@ -1514,15 +1514,12 @@ \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.
> > >
> > > -\begin{lstlisting}
> > > -struct virtio_net_ctrl_coal_rx {
> > > -    le32 rx_max_packets;
> > > -    le32 rx_usecs;
> > > -};
> > > +Note: In general, these commands are best-effort: A device could
> > > +send a
> > > notification even if it is not supposed to.
> > >
> > Please remove this note from this patch.
> > Instead of Note, we need to describe this device requirements description.
> > We better need to describe the motivation for it.
> > We may want to say there may be jitter in notification, but device should not
> be sending when it is not supposed to.
> 
> It's explicitly allowed:
> 
> split-ring.tex:The driver MUST handle spurious notifications from the device.
> split-ring.tex:The device MUST handle spurious notifications from the driver.
> 
The intent is the guide to device implementation to have less spurious interrupts.
Best effort wording says that device is free to implement timer of any granularity of choice, which kind of defeats the purpose of IM.

So, both can handle/generate spurious notifications, but it shouldn't be best line guidance.

> 
> 
> > I also have more description to add in this area with regards to GSO and LRO.
> > I make humble suggestion that we draft is jointly in separate patch combining
> these clarifications.
> >
> > > -struct virtio_net_ctrl_coal_tx {
> > > -    le32 tx_max_packets;
> > > -    le32 tx_usecs;
> > > +\begin{lstlisting}
> > > +struct virtio_net_ctrl_coal {
> > > +    le32 max_packets;
> > > +    le32 usecs;
> > >  };
> > >
> > This is one good change to go as separate patch.
> >
> > >  #define VIRTIO_NET_CTRL_NOTF_COAL 6 @@ -1532,25 +1529,25 @@
> > > \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_max_packets}: Maximum number of packets to receive
> > > before a RX notification.
> > > -\item \field{tx_max_packets}: Maximum number of packets to send
> > > before a TX notification.
> > > +\item \field{usecs} for RX: Maximum number of usecs to delay a RX
> > > notification.
> > > +\item \field{usecs} for TX: Maximum number of usecs to delay a TX
> > > notification.
> > > +\item \field{max_packets} for RX: Maximum number of packets to
> > > +receive
> > > before a RX notification.
> > > +\item \field{max_packets} for TX: Maximum number of packets to send
> > > +before
> > > a TX notification.
> > >  \end{itemize}
> > >
> > s/for Rx/For receive virtqueue
> > s/for Tx/For transmit virtqueue
> 
> Which virtqueue? It says TX/RX pretty consistently in this text.
> Changing to receive virtqueue/transmit virtqueue would be a big change and
> frankly for a very modest gain in readability.
> Rather maybe just say RX/TX where we describe virtqueue.
> 
We are describing rest of the previous sections as transmitq, receiveq.
Would like to keep this section consistent with rest of the Network device section, and not just notification coalescing section.

> > >
> > >  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{usecs} and
> > > \field{max_packets} parameters for TX.
> > > +\item VIRTIO_NET_CTRL_NOTF_COAL_RX_SET: set the \field{usecs} and
> > > \field{max_packets} parameters for RX.
> > >  \end{enumerate}
> > >
> > >  \subparagraph{RX Notifications}\label{sec:Device Types / Network
> > > Device / Device Operation / Control Virtqueue / Notifications
> > > Coalescing / RX Notifications}
> > >
> > >  If, for example:
> > >  \begin{itemize}
> > > -\item \field{rx_usecs} = 10.
> > > -\item \field{rx_max_packets} = 15.
> > > +\item \field{usecs} = 10.
> > > +\item \field{max_packets} = 15.
> > >  \end{itemize}
> > >
> > >  The device will operate as follows:
> > > @@ -1564,8 +1561,8 @@ \subsubsection{Control
> > > Virtqueue}\label{sec:Device Types / Network Device / Devi
> > >
> > >  If, for example:
> > >  \begin{itemize}
> > > -\item \field{tx_usecs} = 10.
> > > -\item \field{tx_max_packets} = 15.
> > > +\item \field{usecs} = 10.
> > > +\item \field{max_packets} = 15.
> > >  \end{itemize}
> > >
> > >  The device will operate as follows:
> > > @@ -1575,6 +1572,13 @@ \subsubsection{Control
> > > Virtqueue}\label{sec:Device Types / Network Device / Devi  \item If
> > > the notifications are not suppressed by the driver, the device will
> > > send an used buffer notification, otherwise, the device will not
> > > send an used buffer notification as long as the notifications are suppressed.
> > >  \end{itemize}
> > >
> > > +\subparagraph{Notifications When Coalescing Parameters
> > > +Change}\label{sec:Device Types / Network Device / Device Operation
> > > +/ Control Virtqueue / Notifications Coalescing / Notifications When
> > > +Coalescing Parameters Change}
> > > +
> > > +When a device changes the coalescing parameters, the device needs
> > > +to check
> > > if the new parameters are met and issue a notification if so.
> > > +
> > > +For example, \field{max_packets} = 15 for TX.
> > > +If the device sends 10 packets, then it receives a
> > > VIRTIO_NET_CTRL_NOTF_COAL_TX_SET command with
> \field{max_packets} =
> > > 8, the device needs to immediately send a TX notification, if the
> > > notifications are not suppressed by the driver.
> > > +
> > Its better written as,
> > When device applies new coalescing parameters, if the new parameters
> already meet the current packet counters, device SHOULD generate immediate
> notification.
> 
> what are packet counters though? they aren't defined anywhere.
> 
Notification coalescing is counting packets in current text as " The device will count sent packets until it accumulates 15 ..."

> > For example, current \field{max_packets} is 15 for transmit virtqueue, and
> device already sent 10 packets.
> 
> I assume it is all per virtqueue? Makes sense but it does not actually say
> anywhere.
> 
Yes, it is per virtqueue because the coalescing commands are per virtqueue.
Not sure explicitly keep adding per virtqueue in every line improves the readability.

> > When device receives a VIRTIO_NET_CTRL_NOTF_COAL_TX_SET command,
> the device SHOULD immediately send a TX notification.
> 
> always? why?
Ah no. not always. It was in the context of the example continuation.

May be better to rewrite as,

For example, current \field{max_packets} is 15 for the transmit virtqueue, and device already sent 10 packets; in such case when device receives a VIRTIO_NET_CTRL_NOTF_COAL_TX_SET command, the device SHOULD immediately send a TX notification.

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] [PATCH] virtio-net: Fix and update VIRTIO_NET_F_NOTF_COAL feature

[Michael S. Tsirkin]

On Mon, Feb 06, 2023 at 09:53:18PM +0000, Parav Pandit wrote:
> 
> 
> > From: virtio-dev@lists.oasis-open.org <virtio-dev@lists.oasis-open.org> On
> > Behalf Of Michael S. Tsirkin
> > 
> > On Mon, Feb 06, 2023 at 07:13:43PM +0000, Parav Pandit wrote:
> > >
> > >
> > > > From: virtio-dev@lists.oasis-open.org
> > > > <virtio-dev@lists.oasis-open.org> On Behalf Of Alvaro Karsz
> > > >
> > > > This patch makes several improvements to the notification coalescing
> > > > feature,
> > > > including:
> > > >
> > > > - Consolidating virtio_net_ctrl_coal_tx and virtio_net_ctrl_coal_rx
> > > >   into a single struct, virtio_net_ctrl_coal, as they are identical.
> > > > - Emphasizing that the coalescing commands are best-effort.
> > > > - Defining the behavior of coalescing with regards to delivering
> > > >   notifications when a change occur.
> > > >
> > > Patch needs to do one thing at a time.
> > > Please split above into three patches.
> > >
> > > > Signed-off-by: Alvaro Karsz <alvaro.karsz@solid-run.com>
> > > > ---
> > > >  device-types/net/description.tex | 40
> > > > ++++++++++++++++++--------------
> > > >  1 file changed, 22 insertions(+), 18 deletions(-)
> > > >
> > > > diff --git a/device-types/net/description.tex
> > > > b/device-types/net/description.tex
> > > > index 1741c79..2a98411 100644
> > > > --- a/device-types/net/description.tex
> > > > +++ b/device-types/net/description.tex
> > > > @@ -1514,15 +1514,12 @@ \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.
> > > >
> > > > -\begin{lstlisting}
> > > > -struct virtio_net_ctrl_coal_rx {
> > > > -    le32 rx_max_packets;
> > > > -    le32 rx_usecs;
> > > > -};
> > > > +Note: In general, these commands are best-effort: A device could
> > > > +send a
> > > > notification even if it is not supposed to.
> > > >
> > > Please remove this note from this patch.
> > > Instead of Note, we need to describe this device requirements description.
> > > We better need to describe the motivation for it.
> > > We may want to say there may be jitter in notification, but device should not
> > be sending when it is not supposed to.
> > 
> > It's explicitly allowed:
> > 
> > split-ring.tex:The driver MUST handle spurious notifications from the device.
> > split-ring.tex:The device MUST handle spurious notifications from the driver.
> > 
> The intent is the guide to device implementation to have less spurious interrupts.
> Best effort wording says that device is free to implement timer of any granularity of choice, which kind of defeats the purpose of IM.
> 
> So, both can handle/generate spurious notifications, but it shouldn't be best line guidance.

Don't really see why not. Lots of spurious interrupts would defeat the
purpose of the feature clearly. But it might be handy e.g. for migration
purposes. Overall it's tough to document in detail but if there are
too many interrupts then surely it's a quality of implementation issue.
I feel trying to go into too much detail here will just pointlessly make
it verbose without making it clearer.

> > 
> > 
> > > I also have more description to add in this area with regards to GSO and LRO.
> > > I make humble suggestion that we draft is jointly in separate patch combining
> > these clarifications.
> > >
> > > > -struct virtio_net_ctrl_coal_tx {
> > > > -    le32 tx_max_packets;
> > > > -    le32 tx_usecs;
> > > > +\begin{lstlisting}
> > > > +struct virtio_net_ctrl_coal {
> > > > +    le32 max_packets;
> > > > +    le32 usecs;
> > > >  };
> > > >
> > > This is one good change to go as separate patch.
> > >
> > > >  #define VIRTIO_NET_CTRL_NOTF_COAL 6 @@ -1532,25 +1529,25 @@
> > > > \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_max_packets}: Maximum number of packets to receive
> > > > before a RX notification.
> > > > -\item \field{tx_max_packets}: Maximum number of packets to send
> > > > before a TX notification.
> > > > +\item \field{usecs} for RX: Maximum number of usecs to delay a RX
> > > > notification.
> > > > +\item \field{usecs} for TX: Maximum number of usecs to delay a TX
> > > > notification.
> > > > +\item \field{max_packets} for RX: Maximum number of packets to
> > > > +receive
> > > > before a RX notification.
> > > > +\item \field{max_packets} for TX: Maximum number of packets to send
> > > > +before
> > > > a TX notification.
> > > >  \end{itemize}
> > > >
> > > s/for Rx/For receive virtqueue
> > > s/for Tx/For transmit virtqueue
> > 
> > Which virtqueue? It says TX/RX pretty consistently in this text.
> > Changing to receive virtqueue/transmit virtqueue would be a big change and
> > frankly for a very modest gain in readability.
> > Rather maybe just say RX/TX where we describe virtqueue.
> > 
> We are describing rest of the previous sections as transmitq, receiveq.
> Would like to keep this section consistent with rest of the Network device section, and not just notification coalescing section.

Maybe, but let's change one thing at a time.

> > > >
> > > >  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{usecs} and
> > > > \field{max_packets} parameters for TX.
> > > > +\item VIRTIO_NET_CTRL_NOTF_COAL_RX_SET: set the \field{usecs} and
> > > > \field{max_packets} parameters for RX.
> > > >  \end{enumerate}
> > > >
> > > >  \subparagraph{RX Notifications}\label{sec:Device Types / Network
> > > > Device / Device Operation / Control Virtqueue / Notifications
> > > > Coalescing / RX Notifications}
> > > >
> > > >  If, for example:
> > > >  \begin{itemize}
> > > > -\item \field{rx_usecs} = 10.
> > > > -\item \field{rx_max_packets} = 15.
> > > > +\item \field{usecs} = 10.
> > > > +\item \field{max_packets} = 15.
> > > >  \end{itemize}
> > > >
> > > >  The device will operate as follows:
> > > > @@ -1564,8 +1561,8 @@ \subsubsection{Control
> > > > Virtqueue}\label{sec:Device Types / Network Device / Devi
> > > >
> > > >  If, for example:
> > > >  \begin{itemize}
> > > > -\item \field{tx_usecs} = 10.
> > > > -\item \field{tx_max_packets} = 15.
> > > > +\item \field{usecs} = 10.
> > > > +\item \field{max_packets} = 15.
> > > >  \end{itemize}
> > > >
> > > >  The device will operate as follows:
> > > > @@ -1575,6 +1572,13 @@ \subsubsection{Control
> > > > Virtqueue}\label{sec:Device Types / Network Device / Devi  \item If
> > > > the notifications are not suppressed by the driver, the device will
> > > > send an used buffer notification, otherwise, the device will not
> > > > send an used buffer notification as long as the notifications are suppressed.
> > > >  \end{itemize}
> > > >
> > > > +\subparagraph{Notifications When Coalescing Parameters
> > > > +Change}\label{sec:Device Types / Network Device / Device Operation
> > > > +/ Control Virtqueue / Notifications Coalescing / Notifications When
> > > > +Coalescing Parameters Change}
> > > > +
> > > > +When a device changes the coalescing parameters, the device needs
> > > > +to check
> > > > if the new parameters are met and issue a notification if so.
> > > > +
> > > > +For example, \field{max_packets} = 15 for TX.
> > > > +If the device sends 10 packets, then it receives a
> > > > VIRTIO_NET_CTRL_NOTF_COAL_TX_SET command with
> > \field{max_packets} =
> > > > 8, the device needs to immediately send a TX notification, if the
> > > > notifications are not suppressed by the driver.
> > > > +
> > > Its better written as,
> > > When device applies new coalescing parameters, if the new parameters
> > already meet the current packet counters, device SHOULD generate immediate
> > notification.
> > 
> > what are packet counters though? they aren't defined anywhere.
> > 
> Notification coalescing is counting packets in current text as " The device will count sent packets until it accumulates 15 ..."

so it says counts packets but it does not mean there's a counter.
And "meet" does not really work with "parameters"...

> > > For example, current \field{max_packets} is 15 for transmit virtqueue, and
> > device already sent 10 packets.
> > 
> > I assume it is all per virtqueue? Makes sense but it does not actually say
> > anywhere.
> > 
> Yes, it is per virtqueue because the coalescing commands are per virtqueue.

in what sense? there's a single set of parameters per device.
\begin{lstlisting}
struct virtio_net_ctrl_coal_rx {
    le32 rx_max_packets;
    le32 rx_usecs;
};

struct virtio_net_ctrl_coal_tx {
    le32 tx_max_packets;
    le32 tx_usecs;
};

#define VIRTIO_NET_CTRL_NOTF_COAL 6
 #define VIRTIO_NET_CTRL_NOTF_COAL_TX_SET  0
 #define VIRTIO_NET_CTRL_NOTF_COAL_RX_SET 1
\end{lstlisting}

does not specify to which vq this applies, I think the implication
is it applies to all of them.



> Not sure explicitly keep adding per virtqueue in every line improves the readability.

well we have to say this somewhere at least and AFAIK we don't.

> > > When device receives a VIRTIO_NET_CTRL_NOTF_COAL_TX_SET command,
> > the device SHOULD immediately send a TX notification.
> > 
> > always? why?
> Ah no. not always. It was in the context of the example continuation.
> 
> May be better to rewrite as,
> 
> For example, current \field{max_packets} is 15 for the transmit virtqueue, and device already sent 10 packets; in such case when device receives a VIRTIO_NET_CTRL_NOTF_COAL_TX_SET command, the device SHOULD immediately send a TX notification.


Parav this is not the best way to give comments I feel. Either just send
a competing patch or (preferably) explain what is wrong with this one.
Piecemeal "Its better written as" without anything in the way of
explanation is not helpful. I for one don't have the time to review and
reply to not just patches but also all the random suggestions you are
throwing around.

-- 
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] [PATCH] virtio-net: Fix and update VIRTIO_NET_F_NOTF_COAL feature

[Parav Pandit]

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Monday, February 6, 2023 5:27 PM

[..]
> > So, both can handle/generate spurious notifications, but it shouldn't be best
> line guidance.
> 
> Don't really see why not. Lots of spurious interrupts would defeat the purpose
> of the feature clearly. But it might be handy e.g. for migration purposes.
Why migration generate too many spurious interrupts?
May be one or two per vector at destination..

> Overall it's tough to document in detail but if there are too many interrupts
> then surely it's a quality of implementation issue.
> I feel trying to go into too much detail here will just pointlessly make it verbose
> without making it clearer.
A generic note that device can generate any number of spurious interrupts is not a spec material...

> > > what are packet counters though? they aren't defined anywhere.
> > >
> > Notification coalescing is counting packets in current text as " The device will
> count sent packets until it accumulates 15 ..."
> 
> so it says counts packets but it does not mean there's a counter.
Counting packet without a counter... interesting. :)
I see your point to better word it without "counter".

> And "meet" does not really work with "parameters"...
> 
A better wording is necessary.

> > > > For example, current \field{max_packets} is 15 for transmit
> > > > virtqueue, and
> > > device already sent 10 packets.
> > >
> > > I assume it is all per virtqueue? Makes sense but it does not
> > > actually say anywhere.
> > >
> > Yes, it is per virtqueue because the coalescing commands are per virtqueue.
> 
> in what sense? there's a single set of parameters per device.
> \begin{lstlisting}
> struct virtio_net_ctrl_coal_rx {
>     le32 rx_max_packets;
>     le32 rx_usecs;
> };
> 
> struct virtio_net_ctrl_coal_tx {
>     le32 tx_max_packets;
>     le32 tx_usecs;
> };
> 
> #define VIRTIO_NET_CTRL_NOTF_COAL 6
>  #define VIRTIO_NET_CTRL_NOTF_COAL_TX_SET  0  #define
> VIRTIO_NET_CTRL_NOTF_COAL_RX_SET 1 \end{lstlisting}
> 
> does not specify to which vq this applies, I think the implication is it applies to
> all of them.
> 
Sadly yes.
I missed this basic thing missing in spec.
Even 5 years old algorithm like net dim may not work effectively with device global coalescing parameters.
Since 1.3 is not released, lets please fix it now to have it per VQ to avoid having yet another command.
A tool that relies on global device level will still work with per VQ level too.

> 
> 
> > Not sure explicitly keep adding per virtqueue in every line improves the
> readability.
> 
> well we have to say this somewhere at least and AFAIK we don't.
> 
> > > > When device receives a VIRTIO_NET_CTRL_NOTF_COAL_TX_SET
> command,
> > > the device SHOULD immediately send a TX notification.
> > >
> > > always? why?
> > Ah no. not always. It was in the context of the example continuation.
> >
> > May be better to rewrite as,
> >
> > For example, current \field{max_packets} is 15 for the transmit virtqueue,
> and device already sent 10 packets; in such case when device receives a
> VIRTIO_NET_CTRL_NOTF_COAL_TX_SET command, the device SHOULD
> immediately send a TX notification.
> 
> 
> Parav this is not the best way to give comments I feel. Either just send a
> competing patch or (preferably) explain what is wrong with this one.
> Piecemeal "Its better written as" without anything in the way of explanation is
> not helpful. I for one don't have the time to review and reply to not just patches
> but also all the random suggestions you are throwing around.

It is not random and not "all".
The part I missed is current command limits to per device.
I didn't expect VQN to be missing. :(

Please don't attribute suggestion of "avoid device is good to generate spurious interrupt" as random..

I understood your suggestion that when suggesting, describe the problem and the solution both.
I will improve this area going forward. Thanks for the inputs.

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: [virtio-dev] [PATCH] virtio-net: Fix and update VIRTIO_NET_F_NOTF_COAL feature

[Heng Qi]

在 2023/2/7 上午5:53, Parav Pandit 写道:
>
>> From: virtio-dev@lists.oasis-open.org <virtio-dev@lists.oasis-open.org> On
>> Behalf Of Michael S. Tsirkin
>>
>> On Mon, Feb 06, 2023 at 07:13:43PM +0000, Parav Pandit wrote:
>>>
>>>> From: virtio-dev@lists.oasis-open.org
>>>> <virtio-dev@lists.oasis-open.org> On Behalf Of Alvaro Karsz
>>>>
>>>> This patch makes several improvements to the notification coalescing
>>>> feature,
>>>> including:
>>>>
>>>> - Consolidating virtio_net_ctrl_coal_tx and virtio_net_ctrl_coal_rx
>>>>    into a single struct, virtio_net_ctrl_coal, as they are identical.
>>>> - Emphasizing that the coalescing commands are best-effort.
>>>> - Defining the behavior of coalescing with regards to delivering
>>>>    notifications when a change occur.
>>>>
>>> Patch needs to do one thing at a time.
>>> Please split above into three patches.
>>>
>>>> Signed-off-by: Alvaro Karsz <alvaro.karsz@solid-run.com>
>>>> ---
>>>>   device-types/net/description.tex | 40
>>>> ++++++++++++++++++--------------
>>>>   1 file changed, 22 insertions(+), 18 deletions(-)
>>>>
>>>> diff --git a/device-types/net/description.tex
>>>> b/device-types/net/description.tex
>>>> index 1741c79..2a98411 100644
>>>> --- a/device-types/net/description.tex
>>>> +++ b/device-types/net/description.tex
>>>> @@ -1514,15 +1514,12 @@ \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.
>>>>
>>>> -\begin{lstlisting}
>>>> -struct virtio_net_ctrl_coal_rx {
>>>> -    le32 rx_max_packets;
>>>> -    le32 rx_usecs;
>>>> -};
>>>> +Note: In general, these commands are best-effort: A device could
>>>> +send a
>>>> notification even if it is not supposed to.
>>>>
>>> Please remove this note from this patch.
>>> Instead of Note, we need to describe this device requirements description.
>>> We better need to describe the motivation for it.
>>> We may want to say there may be jitter in notification, but device should not
>> be sending when it is not supposed to.
>>
>> It's explicitly allowed:
>>
>> split-ring.tex:The driver MUST handle spurious notifications from the device.
>> split-ring.tex:The device MUST handle spurious notifications from the driver.
>>
> The intent is the guide to device implementation to have less spurious interrupts.
> Best effort wording says that device is free to implement timer of any granularity of choice, which kind of defeats the purpose of IM.
>
> So, both can handle/generate spurious notifications, but it shouldn't be best line guidance.
>
>>
>>> I also have more description to add in this area with regards to GSO and LRO.
>>> I make humble suggestion that we draft is jointly in separate patch combining
>> these clarifications.
>>>> -struct virtio_net_ctrl_coal_tx {
>>>> -    le32 tx_max_packets;
>>>> -    le32 tx_usecs;
>>>> +\begin{lstlisting}
>>>> +struct virtio_net_ctrl_coal {
>>>> +    le32 max_packets;
>>>> +    le32 usecs;
>>>>   };
>>>>
>>> This is one good change to go as separate patch.
>>>
>>>>   #define VIRTIO_NET_CTRL_NOTF_COAL 6 @@ -1532,25 +1529,25 @@
>>>> \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_max_packets}: Maximum number of packets to receive
>>>> before a RX notification.
>>>> -\item \field{tx_max_packets}: Maximum number of packets to send
>>>> before a TX notification.
>>>> +\item \field{usecs} for RX: Maximum number of usecs to delay a RX
>>>> notification.
>>>> +\item \field{usecs} for TX: Maximum number of usecs to delay a TX
>>>> notification.
>>>> +\item \field{max_packets} for RX: Maximum number of packets to
>>>> +receive
>>>> before a RX notification.
>>>> +\item \field{max_packets} for TX: Maximum number of packets to send
>>>> +before
>>>> a TX notification.
>>>>   \end{itemize}
>>>>
>>> s/for Rx/For receive virtqueue
>>> s/for Tx/For transmit virtqueue
>> Which virtqueue? It says TX/RX pretty consistently in this text.
>> Changing to receive virtqueue/transmit virtqueue would be a big change and
>> frankly for a very modest gain in readability.
>> Rather maybe just say RX/TX where we describe virtqueue.
>>
> We are describing rest of the previous sections as transmitq, receiveq.
> Would like to keep this section consistent with rest of the Network device section, and not just notification coalescing section.
>
>>>>   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{usecs} and
>>>> \field{max_packets} parameters for TX.
>>>> +\item VIRTIO_NET_CTRL_NOTF_COAL_RX_SET: set the \field{usecs} and
>>>> \field{max_packets} parameters for RX.
>>>>   \end{enumerate}
>>>>
>>>>   \subparagraph{RX Notifications}\label{sec:Device Types / Network
>>>> Device / Device Operation / Control Virtqueue / Notifications
>>>> Coalescing / RX Notifications}
>>>>
>>>>   If, for example:
>>>>   \begin{itemize}
>>>> -\item \field{rx_usecs} = 10.
>>>> -\item \field{rx_max_packets} = 15.
>>>> +\item \field{usecs} = 10.
>>>> +\item \field{max_packets} = 15.
>>>>   \end{itemize}
>>>>
>>>>   The device will operate as follows:
>>>> @@ -1564,8 +1561,8 @@ \subsubsection{Control
>>>> Virtqueue}\label{sec:Device Types / Network Device / Devi
>>>>
>>>>   If, for example:
>>>>   \begin{itemize}
>>>> -\item \field{tx_usecs} = 10.
>>>> -\item \field{tx_max_packets} = 15.
>>>> +\item \field{usecs} = 10.
>>>> +\item \field{max_packets} = 15.
>>>>   \end{itemize}
>>>>
>>>>   The device will operate as follows:
>>>> @@ -1575,6 +1572,13 @@ \subsubsection{Control
>>>> Virtqueue}\label{sec:Device Types / Network Device / Devi  \item If
>>>> the notifications are not suppressed by the driver, the device will
>>>> send an used buffer notification, otherwise, the device will not
>>>> send an used buffer notification as long as the notifications are suppressed.
>>>>   \end{itemize}
>>>>
>>>> +\subparagraph{Notifications When Coalescing Parameters
>>>> +Change}\label{sec:Device Types / Network Device / Device Operation
>>>> +/ Control Virtqueue / Notifications Coalescing / Notifications When
>>>> +Coalescing Parameters Change}
>>>> +
>>>> +When a device changes the coalescing parameters, the device needs
>>>> +to check
>>>> if the new parameters are met and issue a notification if so.
>>>> +
>>>> +For example, \field{max_packets} = 15 for TX.
>>>> +If the device sends 10 packets, then it receives a
>>>> VIRTIO_NET_CTRL_NOTF_COAL_TX_SET command with
>> \field{max_packets} =
>>>> 8, the device needs to immediately send a TX notification, if the
>>>> notifications are not suppressed by the driver.
>>>> +
>>> Its better written as,
>>> When device applies new coalescing parameters, if the new parameters
>> already meet the current packet counters, device SHOULD generate immediate
>> notification.
>>
>> what are packet counters though? they aren't defined anywhere.
>>
> Notification coalescing is counting packets in current text as " The device will count sent packets until it accumulates 15 ..."
>
>>> For example, current \field{max_packets} is 15 for transmit virtqueue, and
>> device already sent 10 packets.
>>
>> I assume it is all per virtqueue? Makes sense but it does not actually say
>> anywhere.
>>
> Yes, it is per virtqueue because the coalescing commands are per virtqueue.
> Not sure explicitly keep adding per virtqueue in every line improves the readability.

Oh, what a coincidence! We are working with our hardware team on NetDIM,
NetDIM needs to determine its own interrupt parameters based on the traffic
information of each queue, because it is not reasonable to use the 
traffic of one
queue to determine the interrupt moderation profile of all queues.

We have preliminarily modified the virtio specification and will issue 
it today
or tomorrow, please wait for review.

Thanks! :)

>
>>> When device receives a VIRTIO_NET_CTRL_NOTF_COAL_TX_SET command,
>> the device SHOULD immediately send a TX notification.
>>
>> always? why?
> Ah no. not always. It was in the context of the example continuation.
>
> May be better to rewrite as,
>
> For example, current \field{max_packets} is 15 for the transmit virtqueue, and device already sent 10 packets; in such case when device receives a VIRTIO_NET_CTRL_NOTF_COAL_TX_SET command, the device SHOULD immediately send a TX notification.
>
> 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: [virtio-dev] [PATCH] virtio-net: Fix and update VIRTIO_NET_F_NOTF_COAL feature

[Parav Pandit]

> From: Heng Qi <hengqi@linux.alibaba.com>
> Sent: Monday, February 6, 2023 7:34 PM

> > Yes, it is per virtqueue because the coalescing commands are per virtqueue.
> > Not sure explicitly keep adding per virtqueue in every line improves the
> readability.
> 
> Oh, what a coincidence! We are working with our hardware team on NetDIM,
> NetDIM needs to determine its own interrupt parameters based on the traffic
> information of each queue, because it is not reasonable to use the traffic of one
> queue to determine the interrupt moderation profile of all queues.
> 
> We have preliminarily modified the virtio specification and will issue it today or
> tomorrow, please wait for review.
> 
> Thanks! :)
> 
Right.
Awesome.
Looking forward for the fix.

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] [PATCH] virtio-net: Fix and update VIRTIO_NET_F_NOTF_COAL feature

[Alvaro Karsz]

Hi Parav, thanks for the comments.

> > +Note: In general, these commands are best-effort: A device could send a
> > notification even if it is not supposed to.
> >
> Please remove this note from this patch.
> Instead of Note, we need to describe this device requirements description.
> We better need to describe the motivation for it.
> We may want to say there may be jitter in notification, but device should not be sending when it is not supposed to.

I don't think that this note welcomes devices to send notifications
when not supposed to, but alert drivers that spurious notifications
may arrive.
It seems pointless to me writing that the device should not be sending
when it is not supposed to, it's obvious.

> > -struct virtio_net_ctrl_coal_tx {
> > -    le32 tx_max_packets;
> > -    le32 tx_usecs;
> > +\begin{lstlisting}
> > +struct virtio_net_ctrl_coal {
> > +    le32 max_packets;
> > +    le32 usecs;
> >  };
> >
> This is one good change to go as separate patch.
>
> >  #define VIRTIO_NET_CTRL_NOTF_COAL 6
> > @@ -1532,25 +1529,25 @@ \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_max_packets}: Maximum number of packets to receive before
> > a RX notification.
> > -\item \field{tx_max_packets}: Maximum number of packets to send before a
> > TX notification.
> > +\item \field{usecs} for RX: Maximum number of usecs to delay a RX
> > notification.
> > +\item \field{usecs} for TX: Maximum number of usecs to delay a TX
> > notification.
> > +\item \field{max_packets} for RX: Maximum number of packets to receive
> > before a RX notification.
> > +\item \field{max_packets} for TX: Maximum number of packets to send before
> > a TX notification.
> >  \end{itemize}
> >
> s/for Rx/For receive virtqueue
> s/for Tx/For transmit virtqueue
> >
> >  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{usecs} and
> > \field{max_packets} parameters for TX.
> > +\item VIRTIO_NET_CTRL_NOTF_COAL_RX_SET: set the \field{usecs} and
> > \field{max_packets} parameters for RX.
> >  \end{enumerate}
> >
> >  \subparagraph{RX Notifications}\label{sec:Device Types / Network Device /
> > Device Operation / Control Virtqueue / Notifications Coalescing / RX
> > Notifications}
> >
> >  If, for example:
> >  \begin{itemize}
> > -\item \field{rx_usecs} = 10.
> > -\item \field{rx_max_packets} = 15.
> > +\item \field{usecs} = 10.
> > +\item \field{max_packets} = 15.
> >  \end{itemize}
> >
> >  The device will operate as follows:
> > @@ -1564,8 +1561,8 @@ \subsubsection{Control Virtqueue}\label{sec:Device
> > Types / Network Device / Devi
> >
> >  If, for example:
> >  \begin{itemize}
> > -\item \field{tx_usecs} = 10.
> > -\item \field{tx_max_packets} = 15.
> > +\item \field{usecs} = 10.
> > +\item \field{max_packets} = 15.
> >  \end{itemize}
> >
> >  The device will operate as follows:
> > @@ -1575,6 +1572,13 @@ \subsubsection{Control
> > Virtqueue}\label{sec:Device Types / Network Device / Devi  \item If the
> > notifications are not suppressed by the driver, the device will send an used
> > buffer notification, otherwise, the device will not send an used buffer
> > notification as long as the notifications are suppressed.
> >  \end{itemize}
> >
> > +\subparagraph{Notifications When Coalescing Parameters
> > +Change}\label{sec:Device Types / Network Device / Device Operation /
> > +Control Virtqueue / Notifications Coalescing / Notifications When
> > +Coalescing Parameters Change}
> > +
> > +When a device changes the coalescing parameters, the device needs to check
> > if the new parameters are met and issue a notification if so.
> > +
> > +For example, \field{max_packets} = 15 for TX.
> > +If the device sends 10 packets, then it receives a
> > VIRTIO_NET_CTRL_NOTF_COAL_TX_SET command with \field{max_packets} =
> > 8, the device needs to immediately send a TX notification, if the notifications
> > are not suppressed by the driver.
> > +
> Its better written as,
> When device applies new coalescing parameters, if the new parameters already meet the current packet counters, device SHOULD generate immediate notification.

The command may change the usecs field, so "packet counters" is
not general enough.




> For example, current \field{max_packets} is 15 for transmit virtqueue, and device already sent 10 packets.
> When device receives a VIRTIO_NET_CTRL_NOTF_COAL_TX_SET command, the device SHOULD immediately send a TX notification.
>
> Above clarification also should be in same patch series as second patch.
>
> >  \drivernormative{\subparagraph}{Notifications Coalescing}{Device Types /
> > Network Device / Device Operation / Control Virtqueue / Notifications
> > Coalescing}
> >
> >  If the VIRTIO_NET_F_NOTF_COAL feature has not been negotiated, the driver
> > MUST NOT issue VIRTIO_NET_CTRL_NOTF_COAL commands.

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] [PATCH] virtio-net: Fix and update VIRTIO_NET_F_NOTF_COAL feature

[Michael S. Tsirkin]

On Mon, Feb 06, 2023 at 11:08:47PM +0000, Parav Pandit wrote:
> 
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Monday, February 6, 2023 5:27 PM
> 
> [..]
> > > So, both can handle/generate spurious notifications, but it shouldn't be best
> > line guidance.
> > 
> > Don't really see why not. Lots of spurious interrupts would defeat the purpose
> > of the feature clearly. But it might be handy e.g. for migration purposes.
> Why migration generate too many spurious interrupts?

Because, you might want to migrate from hardware with to hardware without
coalescing features. So you just tell guest "sure I will
coalesce" but in fact send interrupts normally.

> May be one or two per vector at destination..
> 
> > Overall it's tough to document in detail but if there are too many interrupts
> > then surely it's a quality of implementation issue.
> > I feel trying to go into too much detail here will just pointlessly make it verbose
> > without making it clearer.
> A generic note that device can generate any number of spurious interrupts is not a spec material...
> 
> > > > what are packet counters though? they aren't defined anywhere.
> > > >
> > > Notification coalescing is counting packets in current text as " The device will
> > count sent packets until it accumulates 15 ..."
> > 
> > so it says counts packets but it does not mean there's a counter.
> Counting packet without a counter... interesting. :)
> I see your point to better word it without "counter".
> 
> > And "meet" does not really work with "parameters"...
> > 
> A better wording is necessary.
> 
> > > > > For example, current \field{max_packets} is 15 for transmit
> > > > > virtqueue, and
> > > > device already sent 10 packets.
> > > >
> > > > I assume it is all per virtqueue? Makes sense but it does not
> > > > actually say anywhere.
> > > >
> > > Yes, it is per virtqueue because the coalescing commands are per virtqueue.
> > 
> > in what sense? there's a single set of parameters per device.
> > \begin{lstlisting}
> > struct virtio_net_ctrl_coal_rx {
> >     le32 rx_max_packets;
> >     le32 rx_usecs;
> > };
> > 
> > struct virtio_net_ctrl_coal_tx {
> >     le32 tx_max_packets;
> >     le32 tx_usecs;
> > };
> > 
> > #define VIRTIO_NET_CTRL_NOTF_COAL 6
> >  #define VIRTIO_NET_CTRL_NOTF_COAL_TX_SET  0  #define
> > VIRTIO_NET_CTRL_NOTF_COAL_RX_SET 1 \end{lstlisting}
> > 
> > does not specify to which vq this applies, I think the implication is it applies to
> > all of them.
> > 
> Sadly yes.
> I missed this basic thing missing in spec.
> Even 5 years old algorithm like net dim may not work effectively with device global coalescing parameters.
> Since 1.3 is not released, lets please fix it now to have it per VQ to avoid having yet another command.
> A tool that relies on global device level will still work with per VQ level too.

I agree it's a spec bug, it does not say this way or that.
Not sure what's the best thing to do for compatibility though -
allow all interpretations? select one? I'll ponder this but feel
free to propose a patch.

> > 
> > 
> > > Not sure explicitly keep adding per virtqueue in every line improves the
> > readability.
> > 
> > well we have to say this somewhere at least and AFAIK we don't.
> > 
> > > > > When device receives a VIRTIO_NET_CTRL_NOTF_COAL_TX_SET
> > command,
> > > > the device SHOULD immediately send a TX notification.
> > > >
> > > > always? why?
> > > Ah no. not always. It was in the context of the example continuation.
> > >
> > > May be better to rewrite as,
> > >
> > > For example, current \field{max_packets} is 15 for the transmit virtqueue,
> > and device already sent 10 packets; in such case when device receives a
> > VIRTIO_NET_CTRL_NOTF_COAL_TX_SET command, the device SHOULD
> > immediately send a TX notification.
> > 
> > 
> > Parav this is not the best way to give comments I feel. Either just send a
> > competing patch or (preferably) explain what is wrong with this one.
> > Piecemeal "Its better written as" without anything in the way of explanation is
> > not helpful. I for one don't have the time to review and reply to not just patches
> > but also all the random suggestions you are throwing around.
> 
> It is not random and not "all".
> The part I missed is current command limits to per device.
> I didn't expect VQN to be missing. :(
> 
> Please don't attribute suggestion of "avoid device is good to generate spurious interrupt" as random..
> 
> I understood your suggestion that when suggesting, describe the problem and the solution both.
> I will improve this area going forward. Thanks for the inputs.

Oh when I said random I meant that they can appear anywhere in
the mail text so they are easy to miss. Sorry about being
unclear.

-- 
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] RE: [virtio-dev] [PATCH] virtio-net: Fix and update VIRTIO_NET_F_NOTF_COAL feature

[Alvaro Karsz]

Seems that we still have some disagreements here.

Should I remove the "best effort" note? I actually think that this is
a good note, but I'll remove it if you think that I should.
Should I replace TX and RX with receive virtqueue and transmit
virtqueue? This may be related to Heng's patch, so maybe we should do
it there?
Should I rephrase the "Notifications When Coalescing Parameters
Change" paragraph?

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] RE: [virtio-dev] [PATCH] virtio-net: Fix and update VIRTIO_NET_F_NOTF_COAL feature

[Parav Pandit]

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Tuesday, February 7, 2023 9:18 AM

> > Why migration generate too many spurious interrupts?
> 
> Because, you might want to migrate from hardware with to hardware without
> coalescing features. So you just tell guest "sure I will coalesce" but in fact send
> interrupts normally.

For the hardware that has fake coalescing, HV wouldn't know it anyway without doing pre verification.
And HV may not migrate in such case for best experience.
HV may choose to migrate with low accuracy as you say, which is fine.

But the spec guidance for the device implementations is to promote some reasonable level of accuracy.
Hard to define in words here.
Best effort is wide spectrum of range. :)

Typically, we say in the spec as SHOULD. 
So, lets skip the best-effort wording and stick to SHOULD part like rest of the spec.


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: [virtio-dev] [PATCH] virtio-net: Fix and update VIRTIO_NET_F_NOTF_COAL feature

[Michael S. Tsirkin]

On Wed, Feb 08, 2023 at 05:30:28PM +0000, Parav Pandit wrote:
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Tuesday, February 7, 2023 9:18 AM
> 
> > > Why migration generate too many spurious interrupts?
> > 
> > Because, you might want to migrate from hardware with to hardware without
> > coalescing features. So you just tell guest "sure I will coalesce" but in fact send
> > interrupts normally.
> 
> For the hardware that has fake coalescing, HV wouldn't know it anyway without doing pre verification.
> And HV may not migrate in such case for best experience.
> HV may choose to migrate with low accuracy as you say, which is fine.
> 
> But the spec guidance for the device implementations is to promote some reasonable level of accuracy.
> Hard to define in words here.
> Best effort is wide spectrum of range. :)
> 
> Typically, we say in the spec as SHOULD. 
> So, lets skip the best-effort wording and stick to SHOULD part like rest of the spec.

I think the point of best-effort is that driver must handle interrupts
that arrive earlier. This is how we used it elsewhere. What else does
it include in your opinion that we absolutely must exclude?
I feel it's a good fit for a non-conformance section which is
by nature a bit informal.

For a conformance section SHOULD is indeed a good fit.

-- 
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: [virtio-dev] [PATCH] virtio-net: Fix and update VIRTIO_NET_F_NOTF_COAL feature

[Parav Pandit]

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Wednesday, February 8, 2023 12:39 PM
> 
> On Wed, Feb 08, 2023 at 05:30:28PM +0000, Parav Pandit wrote:
> > > From: Michael S. Tsirkin <mst@redhat.com>
> > > Sent: Tuesday, February 7, 2023 9:18 AM
> >
> > > > Why migration generate too many spurious interrupts?
> > >
> > > Because, you might want to migrate from hardware with to hardware
> > > without coalescing features. So you just tell guest "sure I will
> > > coalesce" but in fact send interrupts normally.
> >
> > For the hardware that has fake coalescing, HV wouldn't know it anyway
> without doing pre verification.
> > And HV may not migrate in such case for best experience.
> > HV may choose to migrate with low accuracy as you say, which is fine.
> >
> > But the spec guidance for the device implementations is to promote some
> reasonable level of accuracy.
> > Hard to define in words here.
> > Best effort is wide spectrum of range. :)
> >
> > Typically, we say in the spec as SHOULD.
> > So, lets skip the best-effort wording and stick to SHOULD part like rest of the
> spec.
> 
> I think the point of best-effort is that driver must handle interrupts that arrive
> earlier. 
Ok. so Let's put this must requirement in the driver section like the rest.

> This is how we used it elsewhere. 
In a quick grep I see best effort shows two matches one for rx filter and one for vlan.
Vlan we lately know was (close) to incorrect.

> What else does it include in your
> opinion that we absolutely must exclude?
> I feel it's a good fit for a non-conformance section which is by nature a bit
> informal.
> 
> For a conformance section SHOULD is indeed a good fit.
> 
Yes, must in driver section and should in device section looks good to me too.

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: [virtio-dev] [PATCH] virtio-net: Fix and update VIRTIO_NET_F_NOTF_COAL feature

[Alvaro Karsz]

> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Wednesday, February 8, 2023 12:39 PM
> >
> > On Wed, Feb 08, 2023 at 05:30:28PM +0000, Parav Pandit wrote:
> > > > From: Michael S. Tsirkin <mst@redhat.com>
> > > > Sent: Tuesday, February 7, 2023 9:18 AM
> > >
> > > > > Why migration generate too many spurious interrupts?
> > > >
> > > > Because, you might want to migrate from hardware with to hardware
> > > > without coalescing features. So you just tell guest "sure I will
> > > > coalesce" but in fact send interrupts normally.
> > >
> > > For the hardware that has fake coalescing, HV wouldn't know it anyway
> > without doing pre verification.
> > > And HV may not migrate in such case for best experience.
> > > HV may choose to migrate with low accuracy as you say, which is fine.
> > >
> > > But the spec guidance for the device implementations is to promote some
> > reasonable level of accuracy.
> > > Hard to define in words here.
> > > Best effort is wide spectrum of range. :)
> > >
> > > Typically, we say in the spec as SHOULD.
> > > So, lets skip the best-effort wording and stick to SHOULD part like rest of the
> > spec.
> >
> > I think the point of best-effort is that driver must handle interrupts that arrive
> > earlier.
> Ok. so Let's put this must requirement in the driver section like the rest.

But this requirement is already written in "Driver Requirements: Used
Buffer Notification Suppression".
Handling spurious interrupts is not NOTF_COAL specific, but generic,
regardless of NOTF_COAL.
So I don't think that this should be written in the NOTF_COAL driver section.

> > This is how we used it elsewhere.
> In a quick grep I see best effort shows two matches one for rx filter and one for vlan.
> Vlan we lately know was (close) to incorrect.
>
> > What else does it include in your
> > opinion that we absolutely must exclude?
> > I feel it's a good fit for a non-conformance section which is by nature a bit
> > informal.
> >
> > For a conformance section SHOULD is indeed a good fit.
> >
> Yes, must in driver section and should in device section looks good to me too.

The whole NOTF_COAL section is about explaining when a device should
issue an interrupt and when not, so writing "The device SHOULD NOT
issue an interrupt when it's not supposed to" seems a bit redundant to
me.

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

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

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