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

[Lege Wang <lege.wang@jaguarmicro.com>]

hi,

I'm back from a long vacation, and have some internal work to do, sorry for
long late response.

> > >
> > > 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?
Yes, and your previous comment is right, ignore my comment above.

> 
> > > 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.
OK, I see.

> 
> > 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.
I'd like to not add these restrictions, in case some devices need bigger number of
descriptors or queue depth. 

> 
> 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.
Sounds feasible, I'll try this way.
I'll prepare a V1 patch as soon as possible, thanks for your patience.

Regards,
Lege.wang
> 
> >
> > 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.
> > >