Re: [PATCH] VIRTIO_F_USED_EVENT_AUTO_DISABLE: add new used buffer notification suppression mechanism

["Michael S. Tsirkin" <mst@redhat.com>]

Thanks for the patch!
Yet something to improve:


On Mon, Jul 01, 2024 at 11:44:35AM +0800, Xiaoguang Wang wrote:
> Currently for the PCI transport, used buffer notification suppression
> informations are located in guest memory, for example, pvirtq_event_suppress
> for packed ring and virtq_avail for split ring, then PCI devices simulated
> by hypervisors, such as QEMU, can vist these suppression informations
> directly before issuing used event, the only overheads may be just proper
> memory barriers, but for some hardware PCI devices offered by
> DPU(Data Processing Unit), DPU would need to submit DMAs to copy guest
> suppression informations to DPU internal memory, then check whether
> used event should be notified by examing this internal memory, which
> is obvious expensive, and this DMA operation is needed every time
> DPU attempts to issue used event, DPU may also choose to add a hardware
> component to polling suppression informations by submit DMAs continuously
> to get newest used event suppression infos, but it's also expensive.
> 
> To help improve this sitiuations, add a new feature:
> VIRTIO_F_USED_EVENT_AUTO_DISABLE. When this feature bit is negotiated,
> devices will place new notification area for used buffer suppression
> notification in PCI bar specfied by virtio_pci_notify_cap, also for better
> used buffer notification coalescing, the device will disable later used
> buffer notifications automatically after sending one used buffer notification
> to driver, and after the driver finishes handling current used buffer event,
> it needs to notify the device to enable used buffer notification again by
> writing above new 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.
> 
> Signed-off-by: Xiaoguang Wang <lege.wang@jaguarmicro.com>


This proposal raises a lot of questions.
See some of them below.



> ---
>  content.tex       | 38 ++++++++++++++++++++++++++++++++++++++
>  transport-pci.tex | 26 +++++++++++++++++++++++++-
>  2 files changed, 63 insertions(+), 1 deletion(-)
> 
> diff --git a/content.tex b/content.tex
> index 0a62dce..84505da 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -448,6 +448,7 @@ \section{Driver Notifications} \label{sec:Basic Facilities of a Virtio Device /
>  The driver is sometimes required to send an available buffer
>  notification to the device.
>  
> +\subsection{Available Buffer Notifications} \label{sec:Basic Facilities of a Virtio Device / Driver notifications / Available Buffer Notifications}
>  When VIRTIO_F_NOTIFICATION_DATA has not been negotiated,
>  this notification contains either a virtqueue index if
>  VIRTIO_F_NOTIF_CONFIG_DATA is not negotiated or device supplied virtqueue
> @@ -488,6 +489,35 @@ \section{Driver Notifications} \label{sec:Basic Facilities of a Virtio Device /
>  has been negotiated, these notifications would then have
>  identical \field{next_off} and \field{next_wrap} values.
>  
> +\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 has been negotiated.

This is not how you start a section. Some text from commit log
might be helpful here.


> +When this feature bit is negotiated, the device will disable used buffer notification
> +automatically after sending one used buffer notification to the driver, and after the driver
> +finishes handling current used buffer event, it needs to notify the device to enable used
> +buffer notification again.
> +
> +\begin{description}
> +\item [vq_index or vq_notif_config_data] Either virtqueue index or device
> +      supplied queue notification config data corresponding to a virtqueue.
> +\item [used_off] Offset
> +      within the ring where the latest used ring entry the driver has processed.
> +      Without VIRTIO_F_RING_PACKED this refers to the 14 least significant bits of the
> +      offset (in units of descriptor entries) within the descriptor ring where the latest used
> +      ring entry the driver has processed. With VIRTIO_F_RING_PACKED this refers to the offset
> +      (in units of descriptor entries) within the descriptor ring where the latest used ring
> +      entry the driver has processed.
> +\item [used_wrap] Wrap Counter.
> +      With VIRTIO_F_RING_PACKED this is the wrap counter
> +      referring to the latest used descriptor where the driver has processed.
> +      Without VIRTIO_F_RING_PACKED this refers to the most significant bit(bit 14) of the
> +      offset (in units of descriptor entries) within the descriptor ring where the latest used ring
> +      entry the driver has processed..
> +\item [enable_notify] Whether to enable used buffer notification.
> +      The driver may benefit from just informing device the latest used ring entry
> +      it has processed, but not enabling used buffer notification. This field only
> +      occupies one bit, if set to 1, it will enable used buffer notification, otherwise not.

not what?

> +\end{description}
> +


I don't really understand what is this list supposed to mean.
You seem to be defining a bunch of fields which are never used?


Wouldn't a better way to put all this be to say that
the driver sends a copy of notification suppression
info to device?
In case of split - flags  + used_event
In case of packed - struct pvirtq_event_suppress



>  \input{shared-mem.tex}
>  
>  \section{Exporting Objects}\label{sec:Basic Facilities of a Virtio Device / Exporting Objects}
> @@ -872,6 +902,14 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
>  	\ref{devicenormative:Basic Facilities of a Virtio Device / Feature Bits} for
>  	handling features reserved for future use.
>  
> +  \item[VIRTIO_F_USED_EVENT_AUTO_DISABLE(42)] This feature indicates that the device
> +  will disable used buffer notification automatically after sending one used buffer notification.
> +  Note: when VIRTIO_F_RING_PACKED has not been negotiated, the maximum queue
> +  size MUST NOT be greater than 16384, when VIRTIO_F_RING_PACKED has been negotiated,
> +  the maximum queue size MUST NOT be greater than 16383.

This is not the place for this kind of note.


> +
> +  See \ref{sec:Basic Facilities of a Virtio Device / Driver notifications / Used Buffer Event Enable Notifications}
> +
>  \end{description}
>  
>  \drivernormative{\section}{Reserved Feature Bits}{Reserved Feature Bits}
> diff --git a/transport-pci.tex b/transport-pci.tex
> index a5c6719..62ba9dc 100644
> --- a/transport-pci.tex
> +++ b/transport-pci.tex
> @@ -551,6 +551,13 @@ \subsubsection{Notification structure layout}\label{sec:Virtio Transport Options
>          cap.offset + queue_notify_off * notify_off_multiplier
>  \end{lstlisting}
>  
> +If the VIRTIO_F_USED_EVENT_AUTO_DISABLE feature has been negotiated, \field{notify_off_multiplier} is also
> +combined with the \field{queue_notify_off} to derive the Queue Notify address for enabling
> +used buffer notifications within a BAR for a virtqueue,

each term has to be first explained, then used.
you never bother saying what is "Queue Notify address for enabling
used buffer notifications"

> but add a 4 bytes offset:
> +\begin{lstlisting}
> +        cap.offset + queue_notify_off * notify_off_multiplier + 4

quite a hack. why not just a new capability?

> +\end{lstlisting}
> +
>  The \field{cap.offset} and \field{notify_off_multiplier} are taken from the
>  notification capability structure above, and the \field{queue_notify_off} is
>  taken from the common configuration structure.


All this is quite vague and informal.


> @@ -563,7 +570,7 @@ \subsubsection{Notification structure layout}\label{sec:Virtio Transport Options
>  \devicenormative{\paragraph}{Notification capability}{Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Notification capability}
>  The device MUST present at least one notification capability.
>  
> -For devices not offering VIRTIO_F_NOTIFICATION_DATA:
> +For devices not offering VIRTIO_F_NOTIFICATION_DATA or VIRTIO_F_USED_EVENT_AUTO_DISABLE:
>  
>  The \field{cap.offset} MUST be 2-byte aligned.
>  
> @@ -596,6 +603,23 @@ \subsubsection{Notification structure layout}\label{sec:Virtio Transport Options
>  cap.length >= queue_notify_off * notify_off_multiplier + 4
>  \end{lstlisting}
>  
> +For devices offering VIRTIO_F_USED_EVENT_AUTO_DISABLE:
> +
> +The device MUST either present \field{notify_off_multiplier} as a
> +number that is a power of 2 that is also a multiple 4,
> +or present \field{notify_off_multiplier} as 0.
> +
> +The \field{cap.offset} MUST be 4-byte aligned.
> +
> +The value \field{cap.length} presented by the device MUST be at least 8
> +and MUST be large enough to support queue notification offsets
> +for all supported queues in all possible configurations.
> +
> +For all queues, the value \field{cap.length} presented by the device MUST satisfy:
> +\begin{lstlisting}
> +cap.length >= queue_notify_off * notify_off_multiplier + 8
> +\end{lstlisting}
> +
>  \subsubsection{ISR status capability}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / ISR status capability}
>  
>  The VIRTIO_PCI_CAP_ISR_CFG capability


After reading this and the text, I can't figure out how is this feature
supposed to work without races.  What if the device uses a buffer while
the driver notification is in flight to the device?

> -- 
> 2.34.1