RE: [virtio-comment] [RFC PATCH] VIRTIO_F_USED_EVENT_AUTO_DISABLE: add new used buffer notification suppression mechanism

[Parav Pandit <parav@nvidia.com>]

> From: Lege Wang <lege.wang@jaguarmicro.com>
> Sent: Wednesday, January 31, 2024 2:10 PM
> 
> hi,
> 
> I'd like to have some more discussions about used buffer notification
> mechanism, thanks.
> > > +\item [next_wrap] Wrap Counter.
> > > +      With VIRTIO_F_RING_PACKED this is the wrap counter
> > > +      referring to the next used descriptor.
> > > +      Without VIRTIO_F_RING_PACKED this is the most significant bit
> > > +      (bit 15) of the used index.
> > > +\end{description}
> > > +
> > This forces the driver to process _all_ used buffer notifications
> > before enabling them again.
> > There is a race condition that used ring updates are occurring from
> > the device (without notification) and driver updates the value.
> > For example,
> > 1. driver posted descriptors from index 0 to 9.
> > 2. Used buffer notification arrived for 0 to 3.
> > 3. Driver processes used buffer ring from 0 to 3 and enables used
> > buffer notification for index 4.
> > 4. While #3 is happening, the device already updated the used buffer entries.
> > 5. So device view of used ring entry is messed up as it already passed that
> point.
> >
> > Hence a better driver used buffer notification should be,
> >
> > 1. index upto which it has processed
> I would change it to " index upto which it will process next", the reason is that
> virtio core driver codes have already a well-maintained "struct
> vring_virtqueue->last_used_idx", we can use it for notification, and using this
> value, the device will also be able to learn the driver consumption rate.
> 
It the index upto which the driver _has_ already processed, right?

> > 2. explicit enable/disable bit to enable the notification
> I wonder if there's a case that the driver just notifies the used index info, but
> not enable the device notification, 
Yes. there is a scenario.
For example, device has received 28 packets.
When the driver is polling, it was given budget to poll only 20 packets. In this case NAPI context will come back again to poll rest of the 8 packets.
In this scenario, it is desired to still update the polled completions (used elements), but not enable the notification.
Notification can be enabled later when remaining 8 or more packets are processed.

> the driver will always need one enable
> operation finally. And the notification data would be similar to:
> le32 {
>   union {
>     vq_index: 16;           /* Used if VIRTIO_F_NOTIF_CONFIG_DATA not
> negotiated */
>     vq_notif_config_data: 16; /* Used if VIRTIO_F_NOTIF_CONFIG_DATA
> negotiated */
>   };
>   next_off : 15;
>   next_wrap : 1;
> };
> next_off and next_wrap will denote last_used_idx info, and vq_index or
> vq_notif_config_data is also necessary, so for one 32-bit register, there's no
> room for this enable/disable bit.
> 
Right. There isn't enough room for enable bit.
I see two options.

1. When device wants to still work with 32-bit doorbells, it can reserve top bit of vq_index or next_off for enable/disable.
Here the restriction is the queue depth would be limited to 16K descriptors, which is fairly large enough.
Or having top bix of vq_index is better as, it may be well within limit for a device to offer 32K queues.
For non rdma devices 32K queues is also seems reasonable large value.

2. Device can offer 64-bit format where enable bit is located in the upper 32-bit.

3. Or may be option for the device with two different feature bits, because 64-bit doorbells are relatively new for virtio to adapt to.

> 
> Regards,
> lege.wang
> >
> > These two fields clearly communicate to the device, upto how much used
> > buffer is consumed and understand the driver consumption rate.
> > This way, there is no ambiguity for device on where to write next used
> > ring entry.
> > (where to write next used entry is not driven by the notification
> enablement).
> >
> > And the explicit enable bit indicates notifications are enabled back.
> >
> > Technically, only enable bit is needed, but doing #1 is helpful for
> > the device to understand driver consumption pace.
> > And for that may be current notification can be made 64-bits or as
> > suggested in your patch to have new location.
> >