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

["Michael S. Tsirkin" <mst@redhat.com>]

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