Re: [virtio-comment] [PATCH v2] Introduction of Virtio Network device notifications coalescing feature

[Stefan Hajnoczi <stefanha@redhat.com>]

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: signature.asc
Description: PGP signature