Re: [PATCH v5] virtio-net: clarify coalescing parameters settings

[Halil Pasic <pasic@linux.ibm.com>]

On Fri, 28 Jun 2024 16:23:17 +0800
Jason Wang <jasowang@redhat.com> wrote:

> > Hmmm, this next sentence you reference above was indeed for the
> > interaction between coalescing and used buffer suppression. Then what's
> > the best-effort part was about, really? Round-up or round down the set
> > value to the power of 2 to save space? How is it relevant to our
> > discussion? I think even with rounding it shouldn't be too off? (as
> > said, by best-effort v.s. give up)  
> 
> Would it help if we add something like below in the guidance?
>

It would IMHO greatly benefit clarity, provided we do get it right.
 
> When the device wants to send a notification it needs:
> 
> 1) check notification suppression, don't send interrupt if it is
> suppressed otherwise goto 2)
> 2) check event index, don't send interrupt if event index is not
> crossed otherwise goto 3)
> 3) check coalescing, don't send interrupt if the coalescing limit is
> not exceed, otherwise send the interrupt
> 
> (Just to demonstrate the idea, needs tweaking for sure)

Yes I agree this indeed needs tweaking. Let me make some points.

1) for virtio-net we have 3 distinct mechanisms that police used
buffer notification avoidance, but 2 of the three are actually
mutually exclusive if I understand it correctly. Namely we have
coalescing guarded by VIRTIO_NET_F_NOTF_COAL and then depending on
whether _F_EVENT_IDX was negotiated or not, we have "event_idx based" or
"crude flag based" notification suppression.

2) Let me just quote the entire section on the good old used buffer
notification suppression
"""
2.7.7.2 Device Requirements: Used Buffer Notification Suppression

If the VIRTIO_F_EVENT_IDX feature bit is not negotiated:

    * The device MUST ignore the used_event value.
    * After the device writes a descriptor index into the used ring:
       -  If flags is 1, the device SHOULD NOT send a notification.
       -  If flags is 0, the device MUST send a notification.

Otherwise, if the VIRTIO_F_EVENT_IDX feature bit is negotiated:

    * The device MUST ignore the lower bit of flags.
    * After the device writes a descriptor index into the used ring:
       - If the idx field in the used ring (which determined where that
      descriptor index was placed) was equal to used_event, the device
      MUST send a notification.
       - Otherwise the device SHOULD NOT send a notification.

Note: For example, if used_event is 0, then a device using

VIRTIO_F_EVENT_IDX would send a used buffer notification to the driver
after the first buffer is used (and again after the 65536th buffer,
etc).
""" 

Frankly, a MUST is a must, and I don't see very little leeway
for the coalescing to avoid any notifications. But on the other hand,
that is the very purpose of the coalescing so IMHO we have a problem
here.

The only loophole here is "after", which could be stretched to
"eventually, before the end of the world". Which would basically reduce
the MUST ad-absurdum, and I would not like that.

3) AFAIU if multiple suppression mechanisms are active
concurrently we are aiming for some sort of an logical AND semantic i.e.
a further reduction over just a single one being employed (and not OR).

But IMHO the both the wording for the event_idx based and curde
suppression, as well as for coalescing.

Let me quote the full description for the coalescing:

"""
5.1.6.5.9.1 Operation
The device sends a used buffer notification once the notification conditions are met and if the notifications are not suppressed as explained in 2.7.7.

When the device has non-zero max_usecs and non-zero max_packets, it starts counting microseconds and packets upon receiving/sending a packet. The device counts packets and microseconds for each receive virtqueue and transmit virtqueue separately. In this case, the notification conditions are met when max_usecs microseconds elapse, or upon sending/receiving max_packets packets, whichever happens first. Afterwards, the device waits for the next packet and starts counting packets and microseconds again.

When the device has max_usecs = 0 or max_packets = 0, the notification conditions are met after every packet received/sent. 
"""

Here "Afterwards" is ambiguous. What we want here is "afterwards" ==
"after the notification has been sent", but one can also read it as
"after the condition has been met".

4) I see two ways to make sense out of it.

My preferred way would be to replace "send a notification" with
"generate a notification" in the main text including "2.7.7.2 Device
Requirements: Used Buffer Notification Suppression", along with adding an
explanation which states that generated notifications are sent
immediately unless a device has an active facility that under a certain
set of conditions  retains and possibly coalesces notifications. 

Please notice that here coalescence is used with its dictionary
meaning (see https://dictionary.cambridge.org/dictionary/english/coalesce), 
and that the current description of NET_F_NOTIF_COAL is more akin to a
suppression mechanism where notifications are suppressed or discarded and
not coalesced.

The notification coalescing facility would then retain the first
used buffer notification request for each queue (those are still
generated by the same rules), until either the packet count or the
timeout type max frequency (or delay we need to clarify that) condition
is met, and coalesce any further notification requests with the retained
one until it is sent.

The other way, which I'm not sure is actually viable, is basically to
try to clarify the hell out of the current approach as taken by
"5.1.6.5.9.1 Operation" by making it crystal clear when exactly are the
"notification conditions met" and what is the complement state of that,
and add a sentence to 2.7.7.2 that references device specific mechanisms
to do more suppression.  And  of course we would have to find a nice
definition for "if the notifications are not suppressed as explained in
2.7.7." such that loss of initiative is not possible.

IMHO any solution where things can stall because we would have sent
an used buffer notification without "coalescing" but we did not because
of coalescing  is not acceptable. This could happen if we cross
event_idx when the coalescing notification conditions are not met and
thus do not notify, and then when the coalescing notification condition
are finally met (e.g. via timeout) the event_id type suppression
is active again because we crossed event_idx when coalescing
notification condition was not met, and we end up not notifying again.

Sorry for the wall of text.

Regards,
Halil