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

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

On Tue, Jul 02, 2024 at 10:37:35PM +0200, Halil Pasic wrote:
> 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.

Very good points below.


> 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.

I agree as such but I also think we can just go ahead and make this
explicit. That is s/after/at some point in time after/.

And then let's add text that actually explains how
- devices can defer notifications for performance
- some devices can have device specific mechanisms
  for driver to control how much notifications are deferred.


WDYT?



> 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
> 
> 
>