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,

Sorry for late response, I took a while to understand your comments, thanks.
> > notification area in PCI bar. Device will receive this notification reliably, and
> > device knows that it can issue used event now, but it may choose appropriate
> > timing to send used events according to its notification coalescing policy ,
> now
> > it also does not need to DMA guest memory to obtain notification
> > suppression.
> >
> Good description describing the problem and solution well.
> Yes, we discussed this requirement already before at [1].
> 
> [1]
> https://lore.kernel.org/virtio-comment/1689820586.8879051-7-xuanzhuo@li
> nux.alibaba.com/T/#mce485d2c7187b800575c5a63d83b2e9e77f474cc
Cool, thanks for this sharing.
I'd also like to help develop following kernel virtio core patch for this feature if
this spec patch is accepted finally.

> 
> >
> > +\subsection{Used Buffer Event Enable Notifications} \label{sec:Basic
> > +Facilities of a Virtio Device / Driver notifications / Used Buffer Event Enable
> > Notifications} This kind of notification is only necessary when
> > VIRTIO_F_USED_EVENT_AUTO_DISABLE is negotiated.
> > +When this feature bit is negotiated, the device will disable later used
> Remove the word "later".
OK.

> 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
> 2. explicit enable/disable bit to enable the notification
> 
> 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.
Thanks for this valuable suggestion, which clarifies driver and device's behavior clearly.

> And for that may be current notification can be made 64-bits or as suggested
> in your patch to have new location.
After some thoughts, I decide to just make current notification extended to 64-bits for simplicity.

> The wording "disable later" is confusing, does not read proper.
> 
> May be to reword it as,
> 
> This feature indicates that the device will disable used buffer notification
> automatically after sending one used buffer notification.
OK, thanks.

> > +
> > +        le16 queue_intr_notify_off;     /* read-only for driver */
> >  };
> I think it should be renamed to queue_used_buf_notify_off;
> This is because notifications are slightly worded differently than interrupts.
Since it may be simple to just extend current notification data to 64-bits, there's
no needs to add this field.

> > +        which this virtqueue is located.
> > +        \begin{note} this is \em{not} an offset in bytes.
> > +        See \ref{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI
> Device
> > Layout / Notification capability} below.
> > +        \end{note}
> > +
> This is strange. The notification is in the PCI BAR, right?
> it can be done simple as just the BAR offset.
See below comment.

> 
> >
> > + queue_intr_notify_off. Only if VIRTIO_F_USED_EVENT_AUTO_DISABLE is
> > + negotiated. */
> >  };
> >  \end{lstlisting}
> >
> I would really like to avoid extending this structure which is not in the PCI
> extended capability.
> And we are almost full with 256B limit.
> It is the time to have the PCI extended cap defined and place this field there.
Yes, I understand your concerns. Initially I intended to add new PCI capability
to describe this new notification area, but found that this new capability would
be similar to virtio_pci_notify_cap, and will introduce many duplicate descriptions
in virtio spec doc. If we choose to extent current notification area, there're no needs
to touch current pci capability, the corresponding queue notify address to enable
used buffer notification within a BAR for a virtqueue would look like below:
  cap.offset + queue_notify_off * notify_off_multiplier + 4

> If this multiplier is not useful, maybe it should be added only if needed.
> Did you add it because to match the current flexibility of the spec, or your hw
> actually needs this multiplier?
Yes, just to match the current spec ??

Regards,
lege.wang