Re: [virtio-comment] [PATCH v3] Introduction of Virtio Network device no

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

On Wed, Jun 1, 2022 at 6:19 AM Jason Wang <jasowang@redhat.com> wrote:

On Tue, May 31, 2022 at 3:02 PM Alvaro Karsz <alvaro.karsz@solid-run.com> wrote:
>
> > So according to the name of the feature, it tries to coalesce
> > notification. But from what you said here, it counts by frames. This
> > seems to be a conflict:
> >
> >
> > We never had per frame notification in the past but per packet used
> > buffer notification.
> >
> >
> > What's more, looking at the spec of real hardware NIC like e810 or the
> > ancient e1000, I don't see any per frame interrupt. They only have per
> > TX/RX descriptor writeback interrupt which signals the completion of
> > TX/RX pacekt.
>
>
> You're right, coalescing packets was my initial intention, but I
> understood from the comments on V2 that is should count ethernet
> frames from Stefan's comment:
>
> >Â I think the virtio-net-level coalescing makes sense
> > since it counts Ethernet frames rather than virtqueue buffers.
>
>
> Anyway, I think that counting packets makes more sense, and I will
> change it accordingly if you all are Ok with that.

I think it's better to coalesce packets (used buffers).

>
> >
> > An example, if we tx-frames to 16, but we transmit 64K GSO on 1500
> > MSS. The packets were segmented to ~43 frames. If we send a
> > notification every 16 frames, the first 2 notifications are
> > meaningless, there's nothing that a guest driver can do. So did the
> > receiving. I don't get why not simply coalescing the used buffer
> > notification.
>
>
> You're right, that was my point in V2:
> >
> > x-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.
>
>
>
> > So I feel counting by used buffer is simpler and more useful than
> > counting by frames. In order to make them work in parallel, we can
> > simply say: when the coalescing condition is not met, the event
> > notification will be delayed until we meet the coalescing condition
> > otherwise the notification is armed and raised.
>
>
> We have 2 approaches in implementing the device's side:
>
> Approach 1:
> The device should always count packets, even if notifications are not enabled:
> Something like:
> - Device uses a buffer.
> - Device increases the packet counter.
> - Device checks if coalescing conditions are met.
> If coalescing conditions indeed met:
> - Device resets the coalescing counters.
> - Device sends a notification, only if notifications are enabled.
>
> Code example:
>
> mark_desc_as_used()
> update_coal_counters()
> if (coal_cond_met) {
>Â Â Âreset_coal_counters()
>Â Â Âif (events_enabled)

I guess "events_enabled" means driver allow us to send a notification either:

1) NO_NOTIFY is clear
or
2) used idx matches used_event at least once since last notification?

>Â Â Â Â Âsend_notification()

I think we should reset the counters only after we send a notification?

> }

Need to specify what happens if the condition is not met, I think the
behaviour is to delay the notification until the condition is met.

>
> Approach 2:
> The device counts packets only if notifications are enabled.
> Something like:
> - Device uses a buffer.
> - Device checks if notifications are enabled, if not, it just resets
> coalescing counters.
> If notifications are enabled:
> - Device increases the packet counter.
> - Device checks if coalescing conditions are met.
> - Device reset the coalescing counters.
> - Device sends a notification.
>
> Code example:
>
> mark_desc_as_used()
> if (events_enabled) {
>Â Â Âupdate_coal_counters()
>Â Â Âif (coal_cond_met) {
>Â Â Â Â Âreset_coal_counters()
>Â Â Â Â Âsend_notification()
>Â Â Â}
> } else {
>Â Â Âreset_coal_counters()
> }
>
> Approach 1 is simpler, specially for a Real HW device, which needs to
> make a PCI transaction and wait for completion to check if
> notifications are enabled.

Yes.

> But, we may have a minor issue with EVENT_IDX using the first approach:
> Assume:
> ring current location #0
> EVENT_IDX #9
> max-frames 10
>
> Using approach 1: A notification will be sent to the driver on buffer
> #10, not #19.
> Using approach 2: A notification will be sent to the driver on buffer #19.

I don't see how this could be useful (since it may introduce much more
delay in this case).

>
> I think that we should use approach 1, and it's not a big deal to send
> on #10 in this example.
>
> What do you think?

I think approach 2 may lead to confusion to the user and approach 1
looks more clear:

1) coalescing working at packet level
2) event idx work at virtio ring level

>
>
> Another point with EVENT_IDX is that the device should "remember" if
> it already sent a notification for a given event_idx.
> If the driver sets event_idx to #20, and never changes it, the device
> should send a notification after #20, then should not send any
> notification until the 16bit counter overflows, and it passes #20
> again.
>
> Do you agree?

Yes, this is how it works without coalescing.

Thanks

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

virtio-comment message