I will post a new version including the discussed points. On Wed, Jun 1, 2022 at 6:19 AM Jason Wang wrote: > On Tue, May 31, 2022 at 3:02 PM Alvaro Karsz > 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/ > > > > -- *Alvaro Karsz, *Software +972-50-7696862| https://www.solid-run.com