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

Jason Wang
2024-07-03T05:01:39Z

On Wed, Jul 3, 2024 at 4:38 AM Halil Pasic <pasic@linux.ibm.com> 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.

Exactly.

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

I agree and unfortunately it could be understood differently by
vendors (and we don't even have an open source software implementation
so far to refer to).

Btw, here is the one that is used by Intel E810. It seems better than
the "afterwards" we used here.

"""
Interrupt throttling is a mechanism that guarantees a minimum gap
between two consecutive interrupts (other than possible jitter caused
by handling the interrupts). The E810 counts the time since the last
interrupt is scheduled and compares it against the ITR setting. If an
event associated with this ITR happens before the ITR expires, the
interrupt assertion is delayed until the ITR expires. If the ITR
expires before any event associated with this interrupt, the interrupt
logic is “armed” and the interrupt can be asserted the moment the
event happens.
"""

And if we follow this it should be "after the notification has been sent".

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

Probably, because we don't tell the driver how many interrupts were
coalesced. I guess that's why it is usually named as "interrupt
moderation" for most of the nic vendors.

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

This is what I try to achieve. Not sure if it's the best. Or we can
have a pseudo code to explain if it helps.

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

Thanks

>
>
>
>