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

[virtio-dev] [PATCH v2] 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.
- Stating that the commands should apply to all the receive/transmit
  virtqueues.
- Stating that every receive/transmit virtqueue should count it's own
  packets.

Signed-off-by: Alvaro Karsz <alvaro.karsz@solid-run.com>
---
v2:
	- Add the last 2 points to the patch.
	- Rephrase the "commands are best-effort" note.
	- Replace "notification" with "used buffer notification" to be
	  more consistent.

 device-types/net/description.tex | 49 ++++++++++++++++++--------------
 1 file changed, 28 insertions(+), 21 deletions(-)

diff --git a/device-types/net/description.tex b/device-types/net/description.tex
index 1741c79..74144fc 100644
--- a/device-types/net/description.tex
+++ b/device-types/net/description.tex
@@ -1514,15 +1514,14 @@ \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;
-};
+\begin{note}
+In general, these commands are best-effort: spurious notifications could still arrive.
+\end{note}
 
-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,28 +1531,29 @@ \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 all the transmit virtqueues.
+\item VIRTIO_NET_CTRL_NOTF_COAL_RX_SET: set the \field{usecs} and \field{max_packets} parameters for all the receive virtqueues.
 \end{enumerate}
 
+Every transmit and receive virtqueue needs to count its own packets and should not be affected by notifications sent by other virtqueues.
+
 \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:
+A device with a single receive virtqueue 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.
@@ -1564,17 +1564,24 @@ \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:
+A device with a single transmit virtqueue 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 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, it needs to check if the new coalescing parameters are met and send a used buffer notification if so.
+
+For example, \field{max_packets} = 15 for a device with a single transmit virtqueue.
+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 used buffer 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: [PATCH v2] virtio-net: Fix and update VIRTIO_NET_F_NOTF_COAL feature

[Alvaro Karsz]

Splitting this patch as suggested in v1 will be messy,

We have lines affected by multiple changes, for example:

+\item VIRTIO_NET_CTRL_NOTF_COAL_RX_SET: set the \field{usecs} and
\field{max_packets} parameters for all the receive virtqueues.

Is affected by the consolidation of the structs and by the statement
that the command should apply to all virtqueues.

Do you think that this is ok to keep it as a single patch that makes
multiple minor changes?

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

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

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

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

[Michael S. Tsirkin]

On Mon, Feb 13, 2023 at 10:30:19AM +0200, Alvaro Karsz wrote:
> 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.
> - Stating that the commands should apply to all the receive/transmit
>   virtqueues.
> - Stating that every receive/transmit virtqueue should count it's own
>   packets.
> 
> Signed-off-by: Alvaro Karsz <alvaro.karsz@solid-run.com>
> ---
> v2:
> 	- Add the last 2 points to the patch.
> 	- Rephrase the "commands are best-effort" note.
> 	- Replace "notification" with "used buffer notification" to be
> 	  more consistent.
> 
>  device-types/net/description.tex | 49 ++++++++++++++++++--------------
>  1 file changed, 28 insertions(+), 21 deletions(-)
> 
> diff --git a/device-types/net/description.tex b/device-types/net/description.tex
> index 1741c79..74144fc 100644
> --- a/device-types/net/description.tex
> +++ b/device-types/net/description.tex
> @@ -1514,15 +1514,14 @@ \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;
> -};
> +\begin{note}
> +In general, these commands are best-effort: spurious notifications could still arrive.
> +\end{note}
>  
> -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,28 +1531,29 @@ \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 all the transmit virtqueues.
> +\item VIRTIO_NET_CTRL_NOTF_COAL_RX_SET: set the \field{usecs} and \field{max_packets} parameters for all the receive virtqueues.
>  \end{enumerate}
>  
> +Every transmit and receive virtqueue needs to count its own packets and should not be affected by notifications sent by other virtqueues.

I find this slightly confusing in that it describes counting
which we did not previously mention. and it is not virtqueue iself
counting of course it is the device. We also need to
rewrite to avoid "should" outside conformance sections.
Also we do not have to count for each queue only
where coalescing is enabled.

I came up with

Packets are counted separately for each transmit or receive virtqueue
until max_packets is reached or until usecs elapsed since ....


and here I am stuck. What does usecs mean exactly? since when?
we also have vring notication suppression we should
explain it is kind of on top.

I also feel it would be helpful to say that e get 1 notification per
max_packets or per usecs which ever is the smallest.



> +
>  \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:
> +A device with a single receive virtqueue 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.
> @@ -1564,17 +1564,24 @@ \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:
> +A device with a single transmit virtqueue 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 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, it needs to check if the new coalescing parameters are met and send a used buffer notification if so.

I feel we still need to clarify "met" here please. Or add a conformance
statement defining this more formally.

> +
> +For example, \field{max_packets} = 15 for a device with a single transmit virtqueue.
> +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 used buffer 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


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

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

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

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

[Michael S. Tsirkin]

On Mon, Feb 13, 2023 at 10:33:32AM +0200, Alvaro Karsz wrote:
> Splitting this patch as suggested in v1 will be messy,
> 
> We have lines affected by multiple changes, for example:
> 
> +\item VIRTIO_NET_CTRL_NOTF_COAL_RX_SET: set the \field{usecs} and
> \field{max_packets} parameters for all the receive virtqueues.
> 
> Is affected by the consolidation of the structs and by the statement
> that the command should apply to all virtqueues.
> 
> Do you think that this is ok to keep it as a single patch that makes
> multiple minor changes?

It is reasonably small so maybe ok.

-- 
MST


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

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

[Alvaro Karsz]

> > +Every transmit and receive virtqueue needs to count its own packets and should not be affected by notifications sent by other virtqueues.
>
> I find this slightly confusing in that it describes counting
> which we did not previously mention. and it is not virtqueue iself
> counting of course it is the device. We also need to
> rewrite to avoid "should" outside conformance sections.
> Also we do not have to count for each queue only
> where coalescing is enabled.
>
> I came up with
>
> Packets are counted separately for each transmit or receive virtqueue
> until max_packets is reached or until usecs elapsed since ....
>
>
> and here I am stuck. What does usecs mean exactly? since when?

I don't think we need the "until max_packets is reached or .." part.

"Packets are counted separately for each transmit or receive
virtqueue" seems good enough for me.
The "until max_packets is reached or until .." part is explained in
the TX/RX examples.

> we also have vring notication suppression we should
> explain it is kind of on top.

Mentioning in every example/explanation that we should send a
notification only if not suppressed by the driver may be too verbose
IMO.

The current approach ignores the notifications suppression, and adds
the following line in the conformance section:

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

Don't you think that this covers all the scenarios in just one line?

But in order to be more consistent, I may need to remove the "if the
notifications are not suppressed by the driver" part from the
Notifications When Coalescing Parameters Change subparagraph.

> I also feel it would be helpful to say that e get 1 notification per
> max_packets or per usecs which ever is the smallest.

This is shown in the TX/RX examples, but maybe we can fix it as I'm
suggesting below.

>
>
> > +
> >  \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:
> > +A device with a single receive virtqueue 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.
> > @@ -1564,17 +1564,24 @@ \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:
> > +A device with a single transmit virtqueue 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 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, it needs to check if the new coalescing parameters are met and send a used buffer notification if so.
>
> I feel we still need to clarify "met" here please. Or add a conformance
> statement defining this more formally.
>

How about adding the following line in the main paragraph:

Coalescing parameters are met when the number of sent/received packets
reaches \field{tx_max_packets} since the last used buffer
notification, or when \field{usecs} elapses since the last used buffer
notification, whichever comes first.

And adding the following line in the conformance section:
A device SHOULD NOT send a used buffer notification if \field{usecs}
elapsed since the last used buffer notification and no packets have
been received.

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

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

[Michael S. Tsirkin]

On Mon, Feb 13, 2023 at 02:04:19PM +0200, Alvaro Karsz wrote:
> > > +Every transmit and receive virtqueue needs to count its own packets and should not be affected by notifications sent by other virtqueues.
> >
> > I find this slightly confusing in that it describes counting
> > which we did not previously mention. and it is not virtqueue iself
> > counting of course it is the device. We also need to
> > rewrite to avoid "should" outside conformance sections.
> > Also we do not have to count for each queue only
> > where coalescing is enabled.
> >
> > I came up with
> >
> > Packets are counted separately for each transmit or receive virtqueue
> > until max_packets is reached or until usecs elapsed since ....
> >
> >
> > and here I am stuck. What does usecs mean exactly? since when?
> 
> I don't think we need the "until max_packets is reached or .." part.
> 
> "Packets are counted separately for each transmit or receive
> virtqueue" seems good enough for me.

Well okay but what does max_packets do? I know we do not
count 

> The "until max_packets is reached or until .." part is explained in
> the TX/RX examples.

it is not that good to only explain by example, preferably we should
have a generic explanation too.

> > we also have vring notication suppression we should
> > explain it is kind of on top.
> 
> Mentioning in every example/explanation that we should send a
> notification only if not suppressed by the driver may be too verbose
> IMO.
> 
> The current approach ignores the notifications suppression, and adds
> the following line in the conformance section:
> 
> A device SHOULD NOT send used buffer notifications to the driver, if
> the notifications are suppressed as explained in \ref{sec:Basic
> Facilities of a Virtio Device / Virtqueues / Used Buffer Notification
> Suppression} ....
> 
> Don't you think that this covers all the scenarios in just one line?
> 
> But in order to be more consistent, I may need to remove the "if the
> notifications are not suppressed by the driver" part from the
> Notifications When Coalescing Parameters Change subparagraph.

it is not very clear what is going on frankly.
i think what happens is this

we have packet and usec limits. when packet arrives
these start counting. if one of these expires



> > I also feel it would be helpful to say that e get 1 notification per
> > max_packets or per usecs which ever is the smallest.
> 
> This is shown in the TX/RX examples, but maybe we can fix it as I'm
> suggesting below.
> 
> >
> >
> > > +
> > >  \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:
> > > +A device with a single receive virtqueue 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.
> > > @@ -1564,17 +1564,24 @@ \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:
> > > +A device with a single transmit virtqueue 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 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, it needs to check if the new coalescing parameters are met and send a used buffer notification if so.
> >
> > I feel we still need to clarify "met" here please. Or add a conformance
> > statement defining this more formally.
> >
> 
> How about adding the following line in the main paragraph:
> 
> Coalescing parameters are met when the number of sent/received packets
> reaches \field{tx_max_packets} since the last used buffer
> notification, or when \field{usecs} elapses since the last used buffer
> notification, whichever comes first.

But this does mean we are tying things to used buffer
notifications which have their own suppression thing.
which yes is documented there but I would prefer to
make things easier to grok without jumping back and forth.
maybe we need a chapter explaining this idea.
E.g. add a concept of "a packet notification".
explain how they are suppressed and they may or may
not trigger a used buffer notification depending on whether
driver requested them.

> And adding the following line in the conformance section:
> A device SHOULD NOT send a used buffer notification if \field{usecs}
> elapsed since the last used buffer notification and no packets have
> been received.

please insert this correction in the description above.
this idea that we will tell reader the wrong thing and then
later correct it is not good since readers are not reading
things in the same order as you are writing them.


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

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

[Alvaro Karsz]

I'll add an intro explaining how the entire coalescing thing works,
without relying on the examples.

> > How about adding the following line in the main paragraph:
> >
> > Coalescing parameters are met when the number of sent/received packets
> > reaches \field{tx_max_packets} since the last used buffer
> > notification, or when \field{usecs} elapses since the last used buffer
> > notification, whichever comes first.
>
> But this does mean we are tying things to used buffer
> notifications which have their own suppression thing.
> which yes is documented there but I would prefer to
> make things easier to grok without jumping back and forth.
> maybe we need a chapter explaining this idea.
> E.g. add a concept of "a packet notification".
> explain how they are suppressed and they may or may
> not trigger a used buffer notification depending on whether
> driver requested them.
>

Honestly, having a "packet notification" concept that may evaluate to
nothing seems a bit confusing to me.
At the end of the day, the device sends used buffer notifications.

Eventually, we send a notification only if the notifications are not
suppressed by the driver, so we'll need to refer to the section
explaining how the notifications suppression works (and reader will
need to jump to another paragraph), or we'll need to re-explain it in
the NOTF_COAL paragraph (which seems redundant).

Maybe we can explain in the intro how the notifications coalescing
works, and then mention that even if the coalescing parameters are met
(this will be explained in the intro as well), we send a notification
only if the notifications are not suppressed (then refer to the
relevant part).

Seems ok to you?

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

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

[Michael S. Tsirkin]

On Mon, Feb 13, 2023 at 03:43:55PM +0200, Alvaro Karsz wrote:
> I'll add an intro explaining how the entire coalescing thing works,
> without relying on the examples.
> 
> > > How about adding the following line in the main paragraph:
> > >
> > > Coalescing parameters are met when the number of sent/received packets
> > > reaches \field{tx_max_packets} since the last used buffer
> > > notification, or when \field{usecs} elapses since the last used buffer
> > > notification, whichever comes first.
> >
> > But this does mean we are tying things to used buffer
> > notifications which have their own suppression thing.
> > which yes is documented there but I would prefer to
> > make things easier to grok without jumping back and forth.
> > maybe we need a chapter explaining this idea.
> > E.g. add a concept of "a packet notification".
> > explain how they are suppressed and they may or may
> > not trigger a used buffer notification depending on whether
> > driver requested them.
> >
> 
> Honestly, having a "packet notification" concept that may evaluate to
> nothing seems a bit confusing to me.
> At the end of the day, the device sends used buffer notifications.
> 
> Eventually, we send a notification only if the notifications are not
> suppressed by the driver, so we'll need to refer to the section
> explaining how the notifications suppression works (and reader will
> need to jump to another paragraph), or we'll need to re-explain it in
> the NOTF_COAL paragraph (which seems redundant).
> 
> Maybe we can explain in the intro how the notifications coalescing
> works, and then mention that even if the coalescing parameters are met
> (this will be explained in the intro as well), we send a notification
> only if the notifications are not suppressed (then refer to the
> relevant part).
> 
> Seems ok to you?

ok so the event we are looking for is "coalescing parameters are met".
It is ok but I would like something that works even if
there is no coalescing. How about "notification conditions are met" then?
The coalescing parameters specify the conditions then


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

[Alvaro Karsz]

> ok so the event we are looking for is "coalescing parameters are met".
> It is ok but I would like something that works even if
> there is no coalescing. How about "notification conditions are met" then?
> The coalescing parameters specify the conditions then
>

Sounds good.

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

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

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