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,

A friendly reminder in case you missed this email

Regards,
lege.wang

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