All of lore.kernel.org
 help / color / mirror / Atom feed
* [virtio-comment] [PATCH v2] Introduction of Virtio Network device notifications coalescing feature
@ 2022-05-16 10:32 Alvaro Karsz
  2022-05-16 15:45 ` Stefan Hajnoczi
  0 siblings, 1 reply; 12+ messages in thread
From: Alvaro Karsz @ 2022-05-16 10:32 UTC (permalink / raw)
  To: virtio-comment; +Cc: rabeeh, Alvaro Karsz

Control a network device notifications coalescing parameters using the control virtqueue.
A new control class was added: VIRTIO_NET_CTRL_NOTF_COAL.

This class provides 2 commands:
- VIRTIO_NET_CTRL_NOTF_COAL_USECS_SET:
  Ask the network device to change the rx-usecs and tx-usecs parameters.
  rx-usecs - Time to delay an RX notification after packet arrival in microseconds.
  tx-usecs - Time to delay a TX notification after a sending a packet in microseconds.

- VIRTIO_NET_CTRL_NOTF_COAL_FRAMES_SET:
  Ask the network device to change the rx-max-frames and tx-max-frames parameters.
  rx-max-frames - Number of packets to delay an RX notification after packet arrival.
  tx-max-frames - Number of packets to delay a TX notification after sending a packet.

--
v2:
	- Usage of notification terminology.
	- Add a few lines on what device should do when driver asked to
	  suppress notifications.

Signed-off-by: Alvaro Karsz <alvaro.karsz@solid-run.com>
---
 content.tex | 148 +++++++++++++++++++++++++++++++++-------------------
 1 file changed, 95 insertions(+), 53 deletions(-)

diff --git a/content.tex b/content.tex
index 7508dd1..161acc0 100644
--- a/content.tex
+++ b/content.tex
@@ -167,11 +167,11 @@ \section{Notifications}\label{sec:Basic Facilities of a Virtio Device
 to driver) plays an important role in this specification. The
 modus operandi of the notifications is transport specific.
 
-There are three types of notifications: 
+There are three types of notifications:
 \begin{itemize}
 \item configuration change notification
 \item available buffer notification
-\item used buffer notification. 
+\item used buffer notification.
 \end{itemize}
 
 Configuration change notifications and used buffer notifications are sent
@@ -586,7 +586,7 @@ \section{Virtio Over PCI Bus}\label{sec:Virtio Transport Options / Virtio Over P
 A Virtio device can be implemented as any kind of PCI device:
 a Conventional PCI device or a PCI Express
 device.  To assure designs meet the latest level
-requirements, see 
+requirements, see
 the PCI-SIG home page at \url{http://www.pcisig.com} for any
 approved changes.
 
@@ -595,7 +595,7 @@ \section{Virtio Over PCI Bus}\label{sec:Virtio Transport Options / Virtio Over P
 guest an interface that meets the specification requirements of
 the appropriate PCI specification: \hyperref[intro:PCI]{[PCI]}
 and \hyperref[intro:PCIe]{[PCIe]}
-respectively. 
+respectively.
 
 \subsection{PCI Device Discovery}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Discovery}
 
@@ -1167,7 +1167,7 @@ \subsubsection{ISR status capability}\label{sec:Virtio Transport Options / Virti
 
 \devicenormative{\paragraph}{ISR status capability}{Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / ISR status capability}
 
-The device MUST present at least one VIRTIO_PCI_CAP_ISR_CFG capability.  
+The device MUST present at least one VIRTIO_PCI_CAP_ISR_CFG capability.
 
 The device MUST set the Device Configuration Interrupt bit
 in \field{ISR status} before sending a device configuration
@@ -1526,7 +1526,7 @@ \subsubsection{Device Initialization}\label{sec:Virtio Transport Options / Virti
 \end{lstlisting}
 
 Note that mapping an event to vector might require device to
-allocate internal device resources, and thus could fail. 
+allocate internal device resources, and thus could fail.
 
 \devicenormative{\subparagraph}{MSI-X Vector Configuration}{Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Device Initialization / MSI-X Vector Configuration}
 
@@ -1554,7 +1554,7 @@ \subsubsection{Device Initialization}\label{sec:Virtio Transport Options / Virti
 unless it is impossible for the device to satisfy the mapping
 request.  Devices MUST report mapping
 failures by returning the NO_VECTOR value when the relevant
-\field{config_msix_vector}/\field{queue_msix_vector} field is read. 
+\field{config_msix_vector}/\field{queue_msix_vector} field is read.
 
 \drivernormative{\subparagraph}{MSI-X Vector Configuration}{Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Device Initialization / MSI-X Vector Configuration}
 
@@ -1776,21 +1776,21 @@ \subsection{MMIO Device Register Layout}\label{sec:Virtio Transport Options / Vi
   \caption {MMIO Device Register Layout}
   \label{tab:Virtio Trasport Options / Virtio Over MMIO / MMIO Device Register Layout} \\
   \hline
-  \mmioreg{Name}{Function}{Offset from base}{Direction}{Description} 
-  \hline 
-  \hline 
+  \mmioreg{Name}{Function}{Offset from base}{Direction}{Description}
+  \hline
+  \hline
   \endfirsthead
   \hline
-  \mmioreg{Name}{Function}{Offset from the base}{Direction}{Description} 
-  \hline 
-  \hline 
+  \mmioreg{Name}{Function}{Offset from the base}{Direction}{Description}
+  \hline
+  \hline
   \endhead
   \endfoot
   \endlastfoot
   \mmioreg{MagicValue}{Magic value}{0x000}{R}{%
     0x74726976
     (a Little Endian equivalent of the ``virt'' string).
-  } 
+  }
   \hline
   \mmioreg{Version}{Device version number}{0x004}{R}{%
     0x2.
@@ -1798,7 +1798,7 @@ \subsection{MMIO Device Register Layout}\label{sec:Virtio Transport Options / Vi
       Legacy devices (see \ref{sec:Virtio Transport Options / Virtio Over MMIO / Legacy interface}~\nameref{sec:Virtio Transport Options / Virtio Over MMIO / Legacy interface}) used 0x1.
     \end{note}
   }
-  \hline 
+  \hline
   \mmioreg{DeviceID}{Virtio Subsystem Device ID}{0x008}{R}{%
     See \ref{sec:Device Types}~\nameref{sec:Device Types} for possible values.
     Value zero (0x0) is used to
@@ -1806,9 +1806,9 @@ \subsection{MMIO Device Register Layout}\label{sec:Virtio Transport Options / Vi
     well known addresses, assigning functions to them depending
     on user's needs.
   }
-  \hline 
+  \hline
   \mmioreg{VendorID}{Virtio Subsystem Vendor ID}{0x00c}{R}{}
-  \hline 
+  \hline
   \mmioreg{DeviceFeatures}{Flags representing features the device supports}{0x010}{R}{%
     Reading from this register returns 32 consecutive flag bits,
     the least significant bit depending on the last value written to
@@ -1818,12 +1818,12 @@ \subsection{MMIO Device Register Layout}\label{sec:Virtio Transport Options / Vi
     features bits 32 to 63 if \field{DeviceFeaturesSel} is set to 1.
     Also see \ref{sec:Basic Facilities of a Virtio Device / Feature Bits}~\nameref{sec:Basic Facilities of a Virtio Device / Feature Bits}.
   }
-  \hline 
+  \hline
   \mmioreg{DeviceFeaturesSel}{Device (host) features word selection.}{0x014}{W}{%
     Writing to this register selects a set of 32 device feature bits
     accessible by reading from \field{DeviceFeatures}.
   }
-  \hline 
+  \hline
   \mmioreg{DriverFeatures}{Flags representing device features understood and activated by the driver}{0x020}{W}{%
     Writing to this register sets 32 consecutive flag bits, the least significant
     bit depending on the last value written to \field{DriverFeaturesSel}.
@@ -1832,41 +1832,41 @@ \subsection{MMIO Device Register Layout}\label{sec:Virtio Transport Options / Vi
     \field{DriverFeaturesSel} is set to 0 and features bits 32 to 63 if
     \field{DriverFeaturesSel} is set to 1. Also see \ref{sec:Basic Facilities of a Virtio Device / Feature Bits}~\nameref{sec:Basic Facilities of a Virtio Device / Feature Bits}.
   }
-  \hline 
+  \hline
   \mmioreg{DriverFeaturesSel}{Activated (guest) features word selection}{0x024}{W}{%
     Writing to this register selects a set of 32 activated feature
     bits accessible by writing to \field{DriverFeatures}.
   }
-  \hline 
+  \hline
   \mmioreg{QueueSel}{Virtual queue index}{0x030}{W}{%
     Writing to this register selects the virtual queue that the
     following operations on \field{QueueNumMax}, \field{QueueNum}, \field{QueueReady},
     \field{QueueDescLow}, \field{QueueDescHigh}, \field{QueueDriverlLow}, \field{QueueDriverHigh},
     \field{QueueDeviceLow}, \field{QueueDeviceHigh} and \field{QueueReset} apply to. The index
-    number of the first queue is zero (0x0). 
+    number of the first queue is zero (0x0).
   }
-  \hline 
+  \hline
   \mmioreg{QueueNumMax}{Maximum virtual queue size}{0x034}{R}{%
     Reading from the register returns the maximum size (number of
     elements) of the queue the device is ready to process or
     zero (0x0) if the queue is not available. This applies to the
     queue selected by writing to \field{QueueSel}.
   }
-  \hline 
+  \hline
   \mmioreg{QueueNum}{Virtual queue size}{0x038}{W}{%
     Queue size is the number of elements in the queue.
     Writing to this register notifies the device what size of the
     queue the driver will use. This applies to the queue selected by
     writing to \field{QueueSel}.
   }
-  \hline 
+  \hline
   \mmioreg{QueueReady}{Virtual queue ready bit}{0x044}{RW}{%
     Writing one (0x1) to this register notifies the device that it can
     execute requests from this virtual queue. Reading from this register
     returns the last value written to it. Both read and write
     accesses apply to the queue selected by writing to \field{QueueSel}.
   }
-  \hline 
+  \hline
   \mmioreg{QueueNotify}{Queue notifier}{0x050}{W}{%
     Writing a value to this register notifies the device that
     there are new buffers to process in a queue.
@@ -1882,7 +1882,7 @@ \subsection{MMIO Device Register Layout}\label{sec:Virtio Transport Options / Vi
     See \ref{sec:Virtqueues / Driver notifications}~\nameref{sec:Virtqueues / Driver notifications}
     for the definition of the components.
   }
-  \hline 
+  \hline
   \mmioreg{InterruptStatus}{Interrupt status}{0x60}{R}{%
     Reading from this register returns a bit mask of events that
     caused the device interrupt to be asserted.
@@ -1895,49 +1895,49 @@ \subsection{MMIO Device Register Layout}\label{sec:Virtio Transport Options / Vi
         asserted because the configuration of the device has changed.
     \end{description}
   }
-  \hline 
+  \hline
   \mmioreg{InterruptACK}{Interrupt acknowledge}{0x064}{W}{%
     Writing a value with bits set as defined in \field{InterruptStatus}
     to this register notifies the device that events causing
     the interrupt have been handled.
   }
-  \hline 
+  \hline
   \mmioreg{Status}{Device status}{0x070}{RW}{%
     Reading from this register returns the current device status
     flags.
     Writing non-zero values to this register sets the status flags,
     indicating the driver progress. Writing zero (0x0) to this
-    register triggers a device reset. 
+    register triggers a device reset.
     See also p. \ref{sec:Virtio Transport Options / Virtio Over MMIO / MMIO-specific Initialization And Device Operation / Device Initialization}~\nameref{sec:Virtio Transport Options / Virtio Over MMIO / MMIO-specific Initialization And Device Operation / Device Initialization}.
   }
-  \hline 
+  \hline
   \mmiodreg{QueueDescLow}{QueueDescHigh}{Virtual queue's Descriptor Area 64 bit long physical address}{0x080}{0x084}{W}{%
     Writing to these two registers (lower 32 bits of the address
     to \field{QueueDescLow}, higher 32 bits to \field{QueueDescHigh}) notifies
     the device about location of the Descriptor Area of the queue
     selected by writing to \field{QueueSel} register.
   }
-  \hline 
+  \hline
   \mmiodreg{QueueDriverLow}{QueueDriverHigh}{Virtual queue's Driver Area 64 bit long physical address}{0x090}{0x094}{W}{%
     Writing to these two registers (lower 32 bits of the address
     to \field{QueueDriverLow}, higher 32 bits to \field{QueueDriverHigh}) notifies
     the device about location of the Driver Area of the queue
     selected by writing to \field{QueueSel}.
   }
-  \hline 
+  \hline
   \mmiodreg{QueueDeviceLow}{QueueDeviceHigh}{Virtual queue's Device Area 64 bit long physical address}{0x0a0}{0x0a4}{W}{%
     Writing to these two registers (lower 32 bits of the address
     to \field{QueueDeviceLow}, higher 32 bits to \field{QueueDeviceHigh}) notifies
     the device about location of the Device Area of the queue
     selected by writing to \field{QueueSel}.
   }
-  \hline 
+  \hline
   \mmioreg{SHMSel}{Shared memory id}{0x0ac}{W}{%
     Writing to this register selects the shared memory region \ref{sec:Basic Facilities of a Virtio Device / Shared Memory Regions}
     following operations on \field{SHMLenLow}, \field{SHMLenHigh},
     \field{SHMBaseLow} and \field{SHMBaseHigh} apply to.
   }
-  \hline 
+  \hline
   \mmiodreg{SHMLenLow}{SHMLenHigh}{Shared memory region 64 bit long length}{0x0b0}{0x0b4}{R}{%
     These registers return the length of the shared memory
     region in bytes, as defined by the device for the region selected by
@@ -1947,7 +1947,7 @@ \subsection{MMIO Device Register Layout}\label{sec:Virtio Transport Options / Vi
     region (i.e. where the ID written to \field{SHMSel} is unused)
     results in a length of -1.
   }
-  \hline 
+  \hline
   \mmiodreg{SHMBaseLow}{SHMBaseHigh}{Shared memory region 64 bit long physical address}{0x0b8}{0x0bc}{R}{%
     The driver reads these registers to discover the base address
     of the region in physical address space.  This address is
@@ -1958,7 +1958,7 @@ \subsection{MMIO Device Register Layout}\label{sec:Virtio Transport Options / Vi
     \field{SHMSel} is unused) results in a base address of
     0xffffffffffffffff.
   }
-  \hline 
+  \hline
   \mmioreg{QueueReset}{Virtual queue reset bit}{0x0c0}{RW}{%
     If VIRTIO_F_RING_RESET has been negotiated, writing one (0x1) to this
     register selectively resets the queue. Both read and write accesses
@@ -1972,7 +1972,7 @@ \subsection{MMIO Device Register Layout}\label{sec:Virtio Transport Options / Vi
     If the values are different, the configuration space accesses were not atomic and the driver has to perform the operations again.
     See also \ref {sec:Basic Facilities of a Virtio Device / Device Configuration Space}.
   }
-  \hline 
+  \hline
   \mmioreg{Config}{Configuration space}{0x100+}{RW}{
     Device-specific configuration space starts at the offset 0x100
     and is accessed with byte alignment. Its meaning and size
@@ -2149,7 +2149,7 @@ \subsection{Legacy interface}\label{sec:Virtio Transport Options / Virtio Over M
 in a slightly different control register layout, the device
 initialization and the virtual queue configuration procedure.
 
-Table \ref{tab:Virtio Trasport Options / Virtio Over MMIO / MMIO Device Legacy Register Layout} 
+Table \ref{tab:Virtio Trasport Options / Virtio Over MMIO / MMIO Device Legacy Register Layout}
 presents control registers layout, omitting
 descriptions of registers which did not change their function
 nor behaviour:
@@ -2158,14 +2158,14 @@ \subsection{Legacy interface}\label{sec:Virtio Transport Options / Virtio Over M
   \caption {MMIO Device Legacy Register Layout}
   \label{tab:Virtio Trasport Options / Virtio Over MMIO / MMIO Device Legacy Register Layout} \\
   \hline
-  \mmioreg{Name}{Function}{Offset from base}{Direction}{Description} 
-  \hline 
-  \hline 
+  \mmioreg{Name}{Function}{Offset from base}{Direction}{Description}
+  \hline
+  \hline
   \endfirsthead
   \hline
-  \mmioreg{Name}{Function}{Offset from the base}{Direction}{Description} 
-  \hline 
-  \hline 
+  \mmioreg{Name}{Function}{Offset from the base}{Direction}{Description}
+  \hline
+  \hline
   \endhead
   \endfoot
   \endlastfoot
@@ -2184,7 +2184,7 @@ \subsection{Legacy interface}\label{sec:Virtio Transport Options / Virtio Over M
   \mmioreg{GuestFeatures}{Flags representing device features understood and activated by the driver}{0x020}{W}{}
   \hline
   \mmioreg{GuestFeaturesSel}{Activated (guest) features word selection}{0x024}{W}{}
-  \hline 
+  \hline
   \mmioreg{GuestPageSize}{Guest page size}{0x028}{W}{%
     The driver writes the guest page size in bytes to the
     register during initialization, before any queues are used.
@@ -2197,7 +2197,7 @@ \subsection{Legacy interface}\label{sec:Virtio Transport Options / Virtio Over M
     Writing to this register selects the virtual queue that the
     following operations on the \field{QueueNumMax}, \field{QueueNum}, \field{QueueAlign}
     and \field{QueuePFN} registers apply to. The index
-    number of the first queue is zero (0x0). 
+    number of the first queue is zero (0x0).
 .
   }
   \hline
@@ -2351,13 +2351,13 @@ \subsection{Basic Concepts}\label{sec:Virtio Transport Options / Virtio over cha
 \end{tabular}
 
 A virtio-ccw proxy device facilitates:
-\begin{itemize} 
+\begin{itemize}
 \item Discovery and attachment of virtio devices (as described above).
 \item Initialization of virtqueues and transport-specific facilities (using
       virtio-specific channel commands).
 \item Notifications (via hypercall and a combination of I/O interrupts
       and indicator bits).
-\end{itemize} 
+\end{itemize}
 
 \subsubsection{Channel Commands for Virtio}\label{sec:Virtio Transport Options / Virtio
 over channel I/O / Basic Concepts/ Channel Commands for Virtio}
@@ -2402,7 +2402,7 @@ \subsubsection{Notifications}\label{sec:Virtio Transport Options / Virtio
 Host->Guest Notification / Notification via Classic I/O Interrupts} and
 \ref{sec:Virtio Transport Options / Virtio over channel I/O / Device
 Operation / Host->Guest Notification / Notification via Adapter I/O
-Interrupts} respectively. 
+Interrupts} respectively.
 
 Configuration change notifications are done using so-called classic I/O
 interrupts. The initialization is described in section \ref{sec:Virtio
@@ -3033,7 +3033,7 @@ \subsection{Virtqueues}\label{sec:Device Types / Network Device / Virtqueues}
 \subsection{Feature bits}\label{sec:Device Types / Network Device / Feature bits}
 
 \begin{description}
-\item[VIRTIO_NET_F_CSUM (0)] Device handles packets with partial checksum.   This 
+\item[VIRTIO_NET_F_CSUM (0)] Device handles packets with partial checksum.   This
   ``checksum offload'' is a common feature on modern network cards.
 
 \item[VIRTIO_NET_F_GUEST_CSUM (1)] Driver handles packets with partial checksum.
@@ -3084,6 +3084,8 @@ \subsection{Feature bits}\label{sec:Device Types / Network Device / Feature bits
 \item[VIRTIO_NET_F_CTRL_MAC_ADDR(23)] Set MAC address through control
     channel.
 
+\item[VIRTIO_NET_F_NOTF_COAL(55)] Device supports notifications coalescing.
+
 \item[VIRTIO_NET_F_HOST_USO (56)] Device can receive USO packets. Unlike UFO
  (fragmenting the packet) the USO splits large UDP packet
  to several segments when each of these smaller packets has UDP header.
@@ -3129,6 +3131,7 @@ \subsubsection{Feature bit requirements}\label{sec:Device Types / Network Device
 \item[VIRTIO_NET_F_GUEST_ANNOUNCE] Requires VIRTIO_NET_F_CTRL_VQ.
 \item[VIRTIO_NET_F_MQ] Requires VIRTIO_NET_F_CTRL_VQ.
 \item[VIRTIO_NET_F_CTRL_MAC_ADDR] Requires VIRTIO_NET_F_CTRL_VQ.
+\item[VIRTIO_NET_F_NOTF_COAL] Requires VIRTIO_NET_F_CTRL_VQ.
 \item[VIRTIO_NET_F_RSC_EXT] Requires VIRTIO_NET_F_HOST_TSO4 or VIRTIO_NET_F_HOST_TSO6.
 \item[VIRTIO_NET_F_RSS] Requires VIRTIO_NET_F_CTRL_VQ.
 \end{description}
@@ -4309,13 +4312,13 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
 
 \drivernormative{\subparagraph}{Automatic receive steering in multiqueue mode}{Device Types / Network Device / Device Operation / Control Virtqueue / Automatic receive steering in multiqueue mode}
 
-The driver MUST configure the virtqueues before enabling them with the 
+The driver MUST configure the virtqueues before enabling them with the
 VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET command.
 
 The driver MUST NOT request a \field{virtqueue_pairs} of 0 or
 greater than \field{max_virtqueue_pairs} in the device configuration space.
 
-The driver MUST queue packets only on any transmitq1 before the 
+The driver MUST queue packets only on any transmitq1 before the
 VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET command.
 
 The driver MUST NOT queue packets on transmit queues greater than
@@ -4464,6 +4467,45 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
 (necessarily when not using the legacy interface) little-endian.
 
 
+\paragraph{Notifications Coalescing}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Notifications Coalescing}
+
+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_usec {
+    le32 tx_usecs;
+    le32 rx_usecs;
+};
+
+struct virtio_net_ctrl_coal_frames {
+    le32 tx_frames_max;
+    le32 rx_frames_max;
+};
+
+#define VIRTIO_NET_CTRL_NOTF_COAL 6
+ #define VIRTIO_NET_CTRL_NOTF_COAL_USECS_SET  0
+ #define VIRTIO_NET_CTRL_NOTF_COAL_FRAMES_SET 1
+\end{lstlisting}
+
+The class VIRTIO_NET_CTRL_NOTF_COAL has 2 commands:
+\begin{itemize}
+\item VIRTIO_NET_CTRL_NOTF_COAL_USECS_SET: set the rx-usecs (time to delay an RX notification after packet arrival in microseconds) and tx-usecs (time to delay a TX notification after a sending a packet in microseconds) parameters.
+\item VIRTIO_NET_CTRL_NOTF_COAL_FRAMES_SET: set the rx-max-frames (number of packets to delay an RX notification after packet arrival) and tx-max-frames (number of packets to delay a TX notification after sending a packet) parameters.
+\end{itemize}
+
+\drivernormative{\subparagraph}{Notifications Coalescing}{Device Types / Network Device / Device Operation / Control Virtqueue / Notifications Coalescing}
+
+
+A driver MUST NOT send to the device VIRTIO_NET_CTRL_NOTF_COAL commands if the VIRTIO_NET_F_NOTF_COAL feature
+has not been negotiated.
+
+\devicenormative{\subparagraph}{Notifications Coalescing}{Device Types / Network Device / Device Operation / Control Virtqueue / Notifications Coalescing}
+
+A device SHOULD respond to the VIRTIO_NET_CTRL_NOTF_COAL commands with VIRTIO_NET_ERR if was not able to change the parameters.\\ \\
+A device SHOULD NOT send notifications to the driver, if the driver asked to suppress notifications by setting the suppress notifications flag in the driver area.\\ \\
+If VIRTIO_F_EVENT_IDX feature was negotiated, the device SHOULD start counting the coalescing parameters from the point it reached to the buffer specified by the driver.
+
 \subsubsection{Legacy Interface: Framing Requirements}\label{sec:Device
 Types / Network Device / Legacy Interface: Framing Requirements}
 
-- 
2.32.0


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/


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [virtio-comment] [PATCH v2] Introduction of Virtio Network device notifications coalescing feature
  2022-05-16 10:32 [virtio-comment] [PATCH v2] Introduction of Virtio Network device notifications coalescing feature Alvaro Karsz
@ 2022-05-16 15:45 ` Stefan Hajnoczi
  2022-05-16 19:00   ` Alvaro Karsz
  0 siblings, 1 reply; 12+ messages in thread
From: Stefan Hajnoczi @ 2022-05-16 15:45 UTC (permalink / raw)
  To: Alvaro Karsz; +Cc: virtio-comment, rabeeh, Michael S. Tsirkin, jasowang

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

On Mon, May 16, 2022 at 01:32:56PM +0300, Alvaro Karsz wrote:
> Control a network device notifications coalescing parameters using the control virtqueue.
> A new control class was added: VIRTIO_NET_CTRL_NOTF_COAL.
> 
> This class provides 2 commands:
> - VIRTIO_NET_CTRL_NOTF_COAL_USECS_SET:
>   Ask the network device to change the rx-usecs and tx-usecs parameters.
>   rx-usecs - Time to delay an RX notification after packet arrival in microseconds.
>   tx-usecs - Time to delay a TX notification after a sending a packet in microseconds.
> 
> - VIRTIO_NET_CTRL_NOTF_COAL_FRAMES_SET:
>   Ask the network device to change the rx-max-frames and tx-max-frames parameters.
>   rx-max-frames - Number of packets to delay an RX notification after packet arrival.
>   tx-max-frames - Number of packets to delay a TX notification after sending a packet.
> 
> --
> v2:
> 	- Usage of notification terminology.
> 	- Add a few lines on what device should do when driver asked to
> 	  suppress notifications.
> 
> Signed-off-by: Alvaro Karsz <alvaro.karsz@solid-run.com>
> ---
>  content.tex | 148 +++++++++++++++++++++++++++++++++-------------------
>  1 file changed, 95 insertions(+), 53 deletions(-)
> 
> diff --git a/content.tex b/content.tex
> index 7508dd1..161acc0 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -167,11 +167,11 @@ \section{Notifications}\label{sec:Basic Facilities of a Virtio Device
>  to driver) plays an important role in this specification. The
>  modus operandi of the notifications is transport specific.
>  
> -There are three types of notifications: 
> +There are three types of notifications:

There are whitespace changes in this patch that should probably be
dropped. If you want to clean up whitespace, please submit a separate
patch so that this patch only does one thing.

> +\devicenormative{\subparagraph}{Notifications Coalescing}{Device Types / Network Device / Device Operation / Control Virtqueue / Notifications Coalescing}
> +
> +A device SHOULD respond to the VIRTIO_NET_CTRL_NOTF_COAL commands with VIRTIO_NET_ERR if was not able to change the parameters.\\ \\
> +A device SHOULD NOT send notifications to the driver, if the driver asked to suppress notifications by setting the suppress notifications flag in the driver area.\\ \\
> +If VIRTIO_F_EVENT_IDX feature was negotiated, the device SHOULD start counting the coalescing parameters from the point it reached to the buffer specified by the driver.

I can't parse the last sentence. My guess is that VIRTIO_F_EVENT_IDX and
*-max-frames have similar effects in suppressing notifications. However,
*-max-frames counts packets (I guess the driver's view of
pre-TSO/GSO/etc packets, not actual packets on the network), while
VIRTIO_F_EVENT_IDX counts virtqueue buffers and therefore diverges due
to VIRTIO_NET_F_MRG_RXBUF (multiple virtqueue buffers can be used to
represent one rx packet).

The rx code would be something like:

  add_used_buf(rx_buf)
  if coal_notf(RX): # coalescing happens here
      notify_used_buf() # Used Buffer Notification Suppression here

This means drivers that poll will already see used buffers. They just
don't get notifications until both coalescing and Used Buffer
Notification Suppression raise an interrupt.

If rx-max-frames = 10 and EVENT_IDX is +20 then the driver sees no
notification after 10 frames because EVENT_IDX will suppress the
notification.

I guess there's a danger of receiving no interrupts or at least weird
suppression patterns when both these mechanisms are enabled
simultaneously. Should drivers avoid using both at the same time?

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [virtio-comment] [PATCH v2] Introduction of Virtio Network device notifications coalescing feature
  2022-05-16 15:45 ` Stefan Hajnoczi
@ 2022-05-16 19:00   ` Alvaro Karsz
  2022-05-17  9:33     ` Stefan Hajnoczi
  2022-05-18  8:56     ` Jason Wang
  0 siblings, 2 replies; 12+ messages in thread
From: Alvaro Karsz @ 2022-05-16 19:00 UTC (permalink / raw)
  To: Stefan Hajnoczi
  Cc: virtio-comment, Rabeeh Khoury, Michael S. Tsirkin, Jason Wang

> There are whitespace changes in this patch that should probably be
> dropped. If you want to clean up whitespace, please submit a separate
> patch so that this patch only does one thing.

Sorry, I will remove the white spaces from the patch in the next version.

> I can't parse the last sentence

What I meant, using your example:
- Device's next buffer to consume is #0.
- EVENT_IDX  is #20, meaning that the driver doesn't want to be
interrupted on #0-19 buffers.
-  max-frames is 10.

In this case, the device should not interrupt the driver on #10,
neither on #20, just on #30, or any number above #19, after the usecs
value elapsed.


> add_used_buf(rx_buf)
>  if coal_notf(RX): # coalescing happens here
>     notify_used_buf() # Used Buffer Notification Suppression here

I guess you are talking about the code on the device's side, seems right.
The device should reset the coalescing parameters in coal_notf
function, I guess we don't want it to trigger an interrupt immediately
after the notification suppression is removed.

> I guess there's a danger of receiving no interrupts or at least weird
> suppression patterns when both these mechanisms are enabled
> simultaneously. Should drivers avoid using both at the same time?

I'm not sure how using both mechanisms at the same time will lead to
problems, can you please give an example?

The only scenarios that I can imagine when no interrupts will be
received at all are when:

1) Driver suppress all events.

2) Driver suppress events, then enables them and suppress again before
device sent any interrupt, because max-frames and usecs values not
meet.
This seems fine to me, since if driver enables events, then suppresses
them again, it is aware of the new buffers, and doesn't want to be
interrupted.


Another point is the coalescing default values.
A device can have default coalescing values, even without receiving a
command from the driver asking to change the parameters.

In this case, you could use ethtool to read the coalescing parameters
and see 0, although the device is using the default values, which may
not be 0.

The initial values could be added in the virtio_net_config struct.

What do you think? is this necessary?

It won't change the functionality, it will only show false data when
using ethtool before a VIRTIO_NET_CTRL_NOTF_COAL was sent, and if the
device has default values that aren't 0 (interrupt every packet).

We could specifying that the device should not use default parameters
without receiving a VIRTIO_NET_CTRL_NOTF_COAL command instead of using
the default values.


Regards, Alvaro

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/


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [virtio-comment] [PATCH v2] Introduction of Virtio Network device notifications coalescing feature
  2022-05-16 19:00   ` Alvaro Karsz
@ 2022-05-17  9:33     ` Stefan Hajnoczi
  2022-05-17 10:01       ` Alvaro Karsz
  2022-05-18  8:56     ` Jason Wang
  1 sibling, 1 reply; 12+ messages in thread
From: Stefan Hajnoczi @ 2022-05-17  9:33 UTC (permalink / raw)
  To: Alvaro Karsz
  Cc: virtio-comment, Rabeeh Khoury, Michael S. Tsirkin, Jason Wang

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

On Mon, May 16, 2022 at 10:00:38PM +0300, Alvaro Karsz wrote:
> > There are whitespace changes in this patch that should probably be
> > dropped. If you want to clean up whitespace, please submit a separate
> > patch so that this patch only does one thing.
> 
> Sorry, I will remove the white spaces from the patch in the next version.
> 
> > I can't parse the last sentence
> 
> What I meant, using your example:
> - Device's next buffer to consume is #0.
> - EVENT_IDX  is #20, meaning that the driver doesn't want to be
> interrupted on #0-19 buffers.
> -  max-frames is 10.
> 
> In this case, the device should not interrupt the driver on #10,
> neither on #20, just on #30, or any number above #19, after the usecs
> value elapsed.

I'm thinking about the scenario where EVENT_IDX and max-frames are not
multiples and the coalesce timer is disabled (0). Can the driver be in a
situation where is waits forever?

max-frames = 2
EVENT_IDX = 3

If the driver only has 3 tx buffers it will not be able to make more
available until it processes the Used Buffer Notification. The problem
is that after max-frames (2) EVENT_IDX hasn't been reached, so we
suppress the Used Buffer Notification. After EVENT_IDX (3) the
max-frames state has been reset so we suppress the Used Buffer
Notification. The driver is left waiting forever.

I think the max-frames state should only be reset when the Used Buffer
Notification is sent. That way a notification is sent after 3 tx buffers
have been used and the driver will be able to make progress.

(This may be a silly example, but the idea is that the driver may rely
on Used Buffer Notifications in order to make progress so we cannot
assume the driver can always submit more buffers.)

> > I guess there's a danger of receiving no interrupts or at least weird
> > suppression patterns when both these mechanisms are enabled
> > simultaneously. Should drivers avoid using both at the same time?
> 
> I'm not sure how using both mechanisms at the same time will lead to
> problems, can you please give an example?
> 
> The only scenarios that I can imagine when no interrupts will be
> received at all are when:
> 
> 1) Driver suppress all events.
> 
> 2) Driver suppress events, then enables them and suppress again before
> device sent any interrupt, because max-frames and usecs values not
> meet.
> This seems fine to me, since if driver enables events, then suppresses
> them again, it is aware of the new buffers, and doesn't want to be
> interrupted.

I agree with scenarios #1 and #2. Interrupts will be received/suppressed
as desired by the driver.

> Another point is the coalescing default values.
> A device can have default coalescing values, even without receiving a
> command from the driver asking to change the parameters.
> 
> In this case, you could use ethtool to read the coalescing parameters
> and see 0, although the device is using the default values, which may
> not be 0.
> 
> The initial values could be added in the virtio_net_config struct.
> 
> What do you think? is this necessary?
> 
> It won't change the functionality, it will only show false data when
> using ethtool before a VIRTIO_NET_CTRL_NOTF_COAL was sent, and if the
> device has default values that aren't 0 (interrupt every packet).
> 
> We could specifying that the device should not use default parameters
> without receiving a VIRTIO_NET_CTRL_NOTF_COAL command instead of using
> the default values.

I don't think *-max-frames default values make sense because a driver
might wait forever. For example, if the device defaults to
tx-max-frames = 10 but the driver uses only 4 tx buffers as part of its
memory allocation strategy (e.g. a PXE boot ROM that runs in a tiny
memory footprint), it will never see an interrupt. So in practice
drivers would have to set *-max-frames to override the device's default.
Therefore I don't think it's worth having a default. Instead the
*-max-frames values must be initialized to 0 by the device.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [virtio-comment] [PATCH v2] Introduction of Virtio Network device notifications coalescing feature
  2022-05-17  9:33     ` Stefan Hajnoczi
@ 2022-05-17 10:01       ` Alvaro Karsz
  2022-05-17 13:30         ` Stefan Hajnoczi
  0 siblings, 1 reply; 12+ messages in thread
From: Alvaro Karsz @ 2022-05-17 10:01 UTC (permalink / raw)
  To: Stefan Hajnoczi
  Cc: virtio-comment, Rabeeh Khoury, Michael S. Tsirkin, Jason Wang

I forgot to address this point in your previous  email:

> My guess is that VIRTIO_F_EVENT_IDX and
> *-max-frames have similar effects in suppressing notifications.

One big difference is that if you want to ask the device to send an
interrupt every 20 packets, you should set VIRTIO_F_EVENT_IDX to 20.
Then when an interrupt is received, change it to 40, then to 60..

Using the notifications coalescing feature, you just need to set the
max-frames values once, and that's it.

> I'm thinking about the scenario where EVENT_IDX and max-frames are not
> multiples and the coalesce timer is disabled (0). Can the driver be in a
> situation where is waits forever?

The timer can't be disabled.
Without the timer, you'll have problems even without  EVENT_IDX/
notification suppressions.
Assume a scenario when max-frames is 10, and the device receives only
5 packets, and then the network is silent.
In this case, these packets will never reach the driver.

So, the timer must always work if a device supports this feature.

timer=0 should mean that an interrupt is sent if 0 usecs elapsed since
the last interrupt, which always will be "yes".


> I think the max-frames state should only be reset when the Used Buffer
> Notification is sent. That way a notification is sent after 3 tx buffers
> have been used and the driver will be able to make progress.

It's possible to ask the device to reset the coalescing counters only
when an interrupt is sent, but I think that it is not required because
of the coalescing timer.


Alvaro

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/


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [virtio-comment] [PATCH v2] Introduction of Virtio Network device notifications coalescing feature
  2022-05-17 10:01       ` Alvaro Karsz
@ 2022-05-17 13:30         ` Stefan Hajnoczi
  2022-05-17 15:50           ` Alvaro Karsz
  0 siblings, 1 reply; 12+ messages in thread
From: Stefan Hajnoczi @ 2022-05-17 13:30 UTC (permalink / raw)
  To: Alvaro Karsz
  Cc: virtio-comment, Rabeeh Khoury, Michael S. Tsirkin, Jason Wang

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

On Tue, May 17, 2022 at 01:01:54PM +0300, Alvaro Karsz wrote:
> I forgot to address this point in your previous  email:
> 
> > My guess is that VIRTIO_F_EVENT_IDX and
> > *-max-frames have similar effects in suppressing notifications.
> 
> One big difference is that if you want to ask the device to send an
> interrupt every 20 packets, you should set VIRTIO_F_EVENT_IDX to 20.
> Then when an interrupt is received, change it to 40, then to 60..
> 
> Using the notifications coalescing feature, you just need to set the
> max-frames values once, and that's it.

That's a good point. It makes me wonder whether there should be a
virtqueue-level coalescing feature. The device would not need to do a
DMA read like it does with VIRTIO_F_EVENT_IDX because the coalescing
value is set once by the driver and rarely updated. I don't want to
distract you though, I think the virtio-net-level coalescing makes sense
since it counts Ethernet frames rather than virtqueue buffers.

> > I'm thinking about the scenario where EVENT_IDX and max-frames are not
> > multiples and the coalesce timer is disabled (0). Can the driver be in a
> > situation where is waits forever?
> 
> The timer can't be disabled.
> Without the timer, you'll have problems even without  EVENT_IDX/
> notification suppressions.
> Assume a scenario when max-frames is 10, and the device receives only
> 5 packets, and then the network is silent.
> In this case, these packets will never reach the driver.
> 
> So, the timer must always work if a device supports this feature.

This reasoning applies to rx but not tx. It would be fine to disable the
timer since the driver controls tx buffer submission.

> timer=0 should mean that an interrupt is sent if 0 usecs elapsed since
> the last interrupt, which always will be "yes".
> 
> 
> > I think the max-frames state should only be reset when the Used Buffer
> > Notification is sent. That way a notification is sent after 3 tx buffers
> > have been used and the driver will be able to make progress.
> 
> It's possible to ask the device to reset the coalescing counters only
> when an interrupt is sent, but I think that it is not required because
> of the coalescing timer.

I agree with you. Please add a paragraph to the spec that summarizes
what we've discussed regarding EVENT_IDX and timer vs max-frames. That
way it will be clear exactly how this should be implemented in drivers
and devices.

Thanks,
Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [virtio-comment] [PATCH v2] Introduction of Virtio Network device notifications coalescing feature
  2022-05-17 13:30         ` Stefan Hajnoczi
@ 2022-05-17 15:50           ` Alvaro Karsz
  2022-05-18  8:52             ` Jason Wang
  0 siblings, 1 reply; 12+ messages in thread
From: Alvaro Karsz @ 2022-05-17 15:50 UTC (permalink / raw)
  To: Stefan Hajnoczi
  Cc: virtio-comment, Rabeeh Khoury, Michael S. Tsirkin, Jason Wang

> This reasoning applies to rx but not tx. It would be fine to disable the
> timer since the driver controls tx buffer submission.

I'm not sure I understand why this can't happen in TX.
Assume the following situation:
tx-max-frames is 10.
The driver wants to send 5 packets (ideal case, every packet is small
enough to fit in one buffer), then no more packets to send for a long
time.

If event suppression isn't on, the driver won't know that the
descriptors were used by device, since the device won't interrupt the
driver.
I'm assuming that the driver won't check the descriptor flags without
being interrupted, which seems like a "good" behaviour when event
suppression isn't on.

Am I missing something?


Alvaro

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/


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [virtio-comment] [PATCH v2] Introduction of Virtio Network device notifications coalescing feature
  2022-05-17 15:50           ` Alvaro Karsz
@ 2022-05-18  8:52             ` Jason Wang
  0 siblings, 0 replies; 12+ messages in thread
From: Jason Wang @ 2022-05-18  8:52 UTC (permalink / raw)
  To: Alvaro Karsz, Stefan Hajnoczi
  Cc: virtio-comment, Rabeeh Khoury, Michael S. Tsirkin


在 2022/5/17 23:50, Alvaro Karsz 写道:
>> This reasoning applies to rx but not tx. It would be fine to disable the
>> timer since the driver controls tx buffer submission.


The driver is required to some post processing after a packet has been sent.

A well behaved driver should rely somehow on the tx interrupt. It can 
provide the accounting timely which may help for the TCP performance.


> I'm not sure I understand why this can't happen in TX.
> Assume the following situation:
> tx-max-frames is 10.
> The driver wants to send 5 packets (ideal case, every packet is small
> enough to fit in one buffer), then no more packets to send for a long
> time.
>
> If event suppression isn't on, the driver won't know that the
> descriptors were used by device, since the device won't interrupt the
> driver.
> I'm assuming that the driver won't check the descriptor flags without
> being interrupted, which seems like a "good" behaviour when event
> suppression isn't on.
>
> Am I missing something?


Maybe we can say the driver should not disable timer.

Thanks


>
>
> Alvaro
>


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/


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [virtio-comment] [PATCH v2] Introduction of Virtio Network device notifications coalescing feature
  2022-05-16 19:00   ` Alvaro Karsz
  2022-05-17  9:33     ` Stefan Hajnoczi
@ 2022-05-18  8:56     ` Jason Wang
  2022-05-18  8:59       ` Alvaro Karsz
  1 sibling, 1 reply; 12+ messages in thread
From: Jason Wang @ 2022-05-18  8:56 UTC (permalink / raw)
  To: Alvaro Karsz, Stefan Hajnoczi
  Cc: virtio-comment, Rabeeh Khoury, Michael S. Tsirkin


在 2022/5/17 03:00, Alvaro Karsz 写道:
>> There are whitespace changes in this patch that should probably be
>> dropped. If you want to clean up whitespace, please submit a separate
>> patch so that this patch only does one thing.
> Sorry, I will remove the white spaces from the patch in the next version.
>
>> I can't parse the last sentence
> What I meant, using your example:
> - Device's next buffer to consume is #0.
> - EVENT_IDX  is #20, meaning that the driver doesn't want to be
> interrupted on #0-19 buffers.
> -  max-frames is 10.
>
> In this case, the device should not interrupt the driver on #10,
> neither on #20, just on #30, or any number above #19, after the usecs
> value elapsed.
>
>
>> add_used_buf(rx_buf)
>>   if coal_notf(RX): # coalescing happens here
>>      notify_used_buf() # Used Buffer Notification Suppression here
> I guess you are talking about the code on the device's side, seems right.
> The device should reset the coalescing parameters in coal_notf
> function, I guess we don't want it to trigger an interrupt immediately
> after the notification suppression is removed.
>
>> I guess there's a danger of receiving no interrupts or at least weird
>> suppression patterns when both these mechanisms are enabled
>> simultaneously. Should drivers avoid using both at the same time?
> I'm not sure how using both mechanisms at the same time will lead to
> problems, can you please give an example?
>
> The only scenarios that I can imagine when no interrupts will be
> received at all are when:
>
> 1) Driver suppress all events.
>
> 2) Driver suppress events, then enables them and suppress again before
> device sent any interrupt, because max-frames and usecs values not
> meet.
> This seems fine to me, since if driver enables events, then suppresses
> them again, it is aware of the new buffers, and doesn't want to be
> interrupted.
>
>
> Another point is the coalescing default values.
> A device can have default coalescing values, even without receiving a
> command from the driver asking to change the parameters.
>
> In this case, you could use ethtool to read the coalescing parameters
> and see 0, although the device is using the default values, which may
> not be 0.
>
> The initial values could be added in the virtio_net_config struct.
>
> What do you think? is this necessary?


I think it should be sufficient to have default values (all zero?) as 
disabled defined in the spec.

Allowing arbitrary values may bring troubles in migration and compatibility.

Thanks


>
> It won't change the functionality, it will only show false data when
> using ethtool before a VIRTIO_NET_CTRL_NOTF_COAL was sent, and if the
> device has default values that aren't 0 (interrupt every packet).
>
> We could specifying that the device should not use default parameters
> without receiving a VIRTIO_NET_CTRL_NOTF_COAL command instead of using
> the default values.
>
>
> Regards, Alvaro
>


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/


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [virtio-comment] [PATCH v2] Introduction of Virtio Network device notifications coalescing feature
  2022-05-18  8:56     ` Jason Wang
@ 2022-05-18  8:59       ` Alvaro Karsz
  2022-05-18 12:07         ` Alvaro Karsz
  0 siblings, 1 reply; 12+ messages in thread
From: Alvaro Karsz @ 2022-05-18  8:59 UTC (permalink / raw)
  To: Jason Wang
  Cc: Stefan Hajnoczi, virtio-comment, Rabeeh Khoury, Michael S. Tsirkin

> Maybe we can say the driver should not disable timer.

I agree,

I think that the following should apply for both TX and RX
>
> timer=0 should mean that an interrupt is sent if 0 usecs elapsed since
> the last interrupt, which always will be "yes".


So, X-max-frames value will be meaningless if X-usecs is 0, and vice versa.

I will create Patch v3 shortly.

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/


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [virtio-comment] [PATCH v2] Introduction of Virtio Network device notifications coalescing feature
  2022-05-18  8:59       ` Alvaro Karsz
@ 2022-05-18 12:07         ` Alvaro Karsz
  2022-05-19  4:16           ` Jason Wang
  0 siblings, 1 reply; 12+ messages in thread
From: Alvaro Karsz @ 2022-05-18 12:07 UTC (permalink / raw)
  To: Jason Wang
  Cc: Stefan Hajnoczi, virtio-comment, Rabeeh Khoury, Michael S. Tsirkin

> I don't want to
> distract you though, I think the virtio-net-level coalescing makes sense
> since it counts Ethernet frames rather than virtqueue buffers.

I'm not sure it should count ethernet frames, I think that it should
count descriptors.
Assume the following example:

tx-max-frames = 5
The driver wants to send a 64k packet, and VIRTIO_NET_F_HOST_TSO4 is negotiated.
The driver will write a 64K description on the descriptor area.
The device will segment the payload and will send more than 40 frames
(with standard MTU) in a very short time (so the timer is not a
factor).
The device will send more than 8 notifications to the driver for just
1 used descriptor.

I think that speaking of descriptors instead of ethernet frames makes
more sense.

This is relevant for RX as well.
Assume that VIRTIO_NET_F_GUEST_TSO4 is negotiated, and
VIRTIO_NET_F_MRG_RXBUF is not.
We could receive many frames that will be written in the same
descriptor chain, and we are in the same situation as described for
TX.


I think that this feature should be at a "higher level".

The device operates normally, ignoring the coalescing parameters.

If the device "decides" that it should send a notification according
to VirtIO "rules" (before checking if the events are suppressed by the
driver), it will then increase the internal coalescing counters, and
decide if it should send the notification.
So, in the case of GSO, it will increase the internal tx-fames counter
by 1 if the descriptor is of a 64B packet, or a 64KB  packet.


What do you think?

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/


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [virtio-comment] [PATCH v2] Introduction of Virtio Network device notifications coalescing feature
  2022-05-18 12:07         ` Alvaro Karsz
@ 2022-05-19  4:16           ` Jason Wang
  0 siblings, 0 replies; 12+ messages in thread
From: Jason Wang @ 2022-05-19  4:16 UTC (permalink / raw)
  To: Alvaro Karsz
  Cc: Stefan Hajnoczi, virtio-comment, Rabeeh Khoury, Michael S. Tsirkin

On Wed, May 18, 2022 at 8:08 PM Alvaro Karsz <alvaro.karsz@solid-run.com> wrote:
>
> > I don't want to
> > distract you though, I think the virtio-net-level coalescing makes sense
> > since it counts Ethernet frames rather than virtqueue buffers.
>
> I'm not sure it should count ethernet frames, I think that it should
> count descriptors.
> Assume the following example:
>
> tx-max-frames = 5
> The driver wants to send a 64k packet, and VIRTIO_NET_F_HOST_TSO4 is negotiated.
> The driver will write a 64K description on the descriptor area.
> The device will segment the payload and will send more than 40 frames
> (with standard MTU) in a very short time (so the timer is not a
> factor).
> The device will send more than 8 notifications to the driver for just
> 1 used descriptor.

I think you meant 1 used buffer actually, the notification was checked
and raised when the device had a new used buffer. In your case without
coalsing we may have at most 1 notification.

Having 8 notifications doesn't make much sense since there's no used
buffer until the last notification.

>
> I think that speaking of descriptors instead of ethernet frames makes
> more sense.

So here is the doc for ethtool_coalesce:

 * @rx_max_coalesced_frames: Maximum number of packets to receive
 *      before an RX interrupt.

It counted by packets. So counting the packets makes more sense.
Especially considering we have mergeable rx buffers where a single
packet can consume multiple buffers.

>
> This is relevant for RX as well.
> Assume that VIRTIO_NET_F_GUEST_TSO4 is negotiated, and
> VIRTIO_NET_F_MRG_RXBUF is not.
> We could receive many frames that will be written in the same
> descriptor chain, and we are in the same situation as described for
> TX.
>
>
> I think that this feature should be at a "higher level".
>
> The device operates normally, ignoring the coalescing parameters.
>
> If the device "decides" that it should send a notification according
> to VirtIO "rules" (before checking if the events are suppressed by the
> driver), it will then increase the internal coalescing counters, and
> decide if it should send the notification.
> So, in the case of GSO, it will increase the internal tx-fames counter
> by 1 if the descriptor is of a 64B packet, or a 64KB  packet.

I'm not sure I understand here. I think the right one is to increase
the counter by 1 after the device receives a packet no matter how many
descriptors/buffers are used for this packet.

Thanks

>
>
> What do you think?
>


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/


^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2022-05-19  4:17 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-16 10:32 [virtio-comment] [PATCH v2] Introduction of Virtio Network device notifications coalescing feature Alvaro Karsz
2022-05-16 15:45 ` Stefan Hajnoczi
2022-05-16 19:00   ` Alvaro Karsz
2022-05-17  9:33     ` Stefan Hajnoczi
2022-05-17 10:01       ` Alvaro Karsz
2022-05-17 13:30         ` Stefan Hajnoczi
2022-05-17 15:50           ` Alvaro Karsz
2022-05-18  8:52             ` Jason Wang
2022-05-18  8:56     ` Jason Wang
2022-05-18  8:59       ` Alvaro Karsz
2022-05-18 12:07         ` Alvaro Karsz
2022-05-19  4:16           ` Jason Wang

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.