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 12:04 PM
> 
> 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@l
> > i 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.
I think having 2 32-bit notification registers is better.
Existing one for avail desc posting.
New 32-bit one for used buffer consumption and re-enablement of notification.
This way we get good backward compatibility and also the optional behavior.
3rd advantage of keeping them as two is when used buffer notification enablement is done, it does not need to touch the device for avail posting.

In some systems, a 64-bit single PCIe TLP write may not be possible, through rare, it may be there.
So having it as separate 32-bit used buffer notification is good.


> 
> > 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
> 
Just to double check, 

If avail buffer notification is at offset = X
Used buffer notification is at offset X + 4.
Right?

> > 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 ??
> 
Ok. that explains.
Yes lets keep things simple as X, and X+4 offsets?

> Regards,
> lege.wang