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

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

[Xiaoguang Wang]

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>
---
 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.
+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.
+\end{description}
+
 \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.
+
+  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, but add a 4 bytes offset:
+\begin{lstlisting}
+        cap.offset + queue_notify_off * notify_off_multiplier + 4
+\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.
@@ -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
-- 
2.34.1

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

[Lege Wang]

Hi Vamsi,

I had planed to submit this v1 last week, but was really busy with other work, sorry for delay.
And please help to review this patch, which I still think it's not mature enough.

Regards,
Xiaoguang Wang

> -----Original Message-----
> From: Lege Wang <lege.wang@jaguarmicro.com>
> Sent: Monday, July 1, 2024 11:45 AM
> To: virtio-comment@lists.linux.dev
> Cc: vattunuru@marvell.com; ndabilpuram@marvell.com; parav@nvidia.com;
> mst@redhat.com; Leo Liu <leo.liu@jaguarmicro.com>; Angus Chen
> <angus.chen@jaguarmicro.com>; Lege Wang <lege.wang@jaguarmicro.com>
> Subject: [PATCH] VIRTIO_F_USED_EVENT_AUTO_DISABLE: add new used buffer
> notification suppression mechanism
> 
> 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>
> ---
>  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.
> +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.
> +\end{description}
> +
>  \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.
> +
> +  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, but add a 4 bytes
> offset:
> +\begin{lstlisting}
> +        cap.offset + queue_notify_off * notify_off_multiplier + 4
> +\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.
> @@ -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
> --
> 2.34.1

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

[Xuan Zhuo]

On Mon,  1 Jul 2024 11:44:35 +0800, Xiaoguang Wang <lege.wang@jaguarmicro.com> 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


How about reuse the doorbells, like other devices?

We can do some refactors for the doorbell? Let it support some new flags.

Thanks.


> 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>
> ---
>  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.
> +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.
> +\end{description}
> +
>  \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.
> +
> +  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, but add a 4 bytes offset:
> +\begin{lstlisting}
> +        cap.offset + queue_notify_off * notify_off_multiplier + 4
> +\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.
> @@ -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
> --
> 2.34.1
>
>

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

[Vamsi Krishna Attunuru]

>-----Original Message-----
>From: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
>Sent: Tuesday, July 2, 2024 6:45 AM
>To: Xiaoguang Wang <lege.wang@jaguarmicro.com>
>Cc: Vamsi Krishna Attunuru <vattunuru@marvell.com>; Nithin Kumar
>Dabilpuram <ndabilpuram@marvell.com>; parav@nvidia.com;
>mst@redhat.com; leo.liu@jaguarmicro.com; angus.chen@jaguarmicro.com;
>Xiaoguang Wang <lege.wang@jaguarmicro.com>; virtio-
>comment@lists.linux.dev
>Subject: [EXTERNAL] Re: [PATCH] VIRTIO_F_USED_EVENT_AUTO_DISABLE:
>add new used buffer notification suppression mechanism
>
>On Mon, 1 Jul 2024 11: 44: 35 +0800, Xiaoguang Wang
><lege. wang@ jaguarmicro. com> wrote: > Currently for the PCI transport,
>used buffer notification suppression > informations are located in guest
>memory, for example, pvirtq_event_suppress 
>On Mon,  1 Jul 2024 11:44:35 +0800, Xiaoguang Wang
><lege.wang@jaguarmicro.com> 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
>
>
>How about reuse the doorbells, like other devices?
>
>We can do some refactors for the doorbell? Let it support some new flags.
>
I think refactoring or overloading the current doorbell region would introduce synchronization issues on the device side.
Anyways this proposal places the new notification region next to the doorbell region only right.

>Thanks.
>
>
>> 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>
>> ---
>>  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.
>> +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.
>> +\end{description}

Once driver sets enable_notify to '1', it's not supposed to update the Offset field unless
device clears/disables the notifications right. It's good to document the sequence that driver
is supposed to follow for updating offset & enable_notify fields.

>> +
>>  \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.
>> +
>> +  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, but add a 4 bytes
>offset:
>> +\begin{lstlisting}
>> +        cap.offset + queue_notify_off * notify_off_multiplier + 4
>> +\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.
>> @@ -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
>> --
>> 2.34.1
>>
>>

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

[Jason Wang]

On Mon, Jul 1, 2024 at 11:51 AM Xiaoguang Wang
<lege.wang@jaguarmicro.com> 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,

Several things could be used to help here:

1) With the event index, as long as the used index doesn't pass used
events you don't need to fetch even index every time
2) Interrupt coalescing may help to throttle the interrupt rate
3) Spec doesn't forbid implementation specific heuristics for
interrupt moderation

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

Performance numbers are welcomed:

1) compare disabling via register against memory
2) compare auto disable against no auto

>
> Signed-off-by: Xiaoguang Wang <lege.wang@jaguarmicro.com>
> ---
>  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.
> +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.
> +\end{description}

Any reason we can't reuse the existing meachsim like used index /
event suppression here?

Basically it just sounds like we need a transport specific method for
them besides being part of the virtqueue.

Thanks

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

[Michael S. Tsirkin]

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

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

[Lege Wang]

hi

Thanks for your review.

> 
> > 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,
> 
> Several things could be used to help here:
> 
> 1) With the event index, as long as the used index doesn't pass used
> events you don't need to fetch even index every time
Yeah, I agree VIRTIO_F_EVENT_IDX could help here, but I think it's a relatively
crude mechanism, I have two questions below:
  1. Used event notification suppression structure is still located in
  host memory(in dpu case), I'm not sure whether used_event would
  be allowed to update in the running of one virtio device, If it's allowed,
  seems devices still need to fetch newest used_event info timely.
  2. According to virtio-spec, for example, if used_event is 0, then a device with
  VIRTIO_F_EVENT_IDX would send a used buffer notification to the driver after
  the first buffer is used (and again after the 65536th buffer), should this behavior
  be strictly followed? For example, driver with VIRTIO_F_EVENT_IDX negotiated
  sets used_event to 0, used buffer notification still could be sent for any used buffer event.

> 2) Interrupt coalescing may help to throttle the interrupt rate
> 3) Spec doesn't forbid implementation specific heuristics for
> interrupt moderation
Yes, indeed we're trying to use the information about the latest used ring entry
driver has processed, which is introduced in this patch, to do interrupt coalescing or moderation.

> > 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.
> 
> Performance numbers are welcomed:
> 
> 1) compare disabling via register against memory
> 2) compare auto disable against no auto
I see, but sorry, currently we don't have any performance numbers yet,
but theoretical analysis and think it'll help improve performance. Some
popular nic also supports this feature, which disables interrupt after sending
one and needs driver to enable it again. When enabling interrupt again, drivers
can attach some extra information.

>
> > +      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.
> > +\end{description}
> 
> Any reason we can't reuse the existing meachsim like used index /
> event suppression here?
Please see my comments above.

Regards,
Xiaoguang Wang
> 
> Basically it just sounds like we need a transport specific method for
> them besides being part of the virtqueue.
> 
> Thanks

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

[Jason Wang]

On Wed, Jul 3, 2024 at 12:33 PM Lege Wang <lege.wang@jaguarmicro.com> wrote:
>
> hi
>
> Thanks for your review.
>
> >
> > > 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,
> >
> > Several things could be used to help here:
> >
> > 1) With the event index, as long as the used index doesn't pass used
> > events you don't need to fetch even index every time
> Yeah, I agree VIRTIO_F_EVENT_IDX could help here, but I think it's a relatively
> crude mechanism, I have two questions below:
>   1. Used event notification suppression structure is still located in
>   host memory(in dpu case), I'm not sure whether used_event would
>   be allowed to update in the running of one virtio device,

What did you mean by "update" here?

>   If it's allowed,
>   seems devices still need to fetch newest used_event info timely.

It depends on how you define "timely", I mean unless the used event is
not crossed, you don't need to fetch it from the main memory?

But basically, I meant putting used_event in a cap/register other than
inventing something completely new.

>   2. According to virtio-spec, for example, if used_event is 0, then a device with
>   VIRTIO_F_EVENT_IDX would send a used buffer notification to the driver after
>   the first buffer is used (and again after the 65536th buffer), should this behavior
>   be strictly followed?

Used index is best effort, and the driver must handle spurious interrupt.

>  For example, driver with VIRTIO_F_EVENT_IDX negotiated
>   sets used_event to 0, used buffer notification still could be sent for any used buffer event.

Only once in every #vring_size, no?

>
> > 2) Interrupt coalescing may help to throttle the interrupt rate
> > 3) Spec doesn't forbid implementation specific heuristics for
> > interrupt moderation
> Yes, indeed we're trying to use the information about the latest used ring entry
> driver has processed, which is introduced in this patch, to do interrupt coalescing or moderation.
>
> > > 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.
> >
> > Performance numbers are welcomed:
> >
> > 1) compare disabling via register against memory
> > 2) compare auto disable against no auto
> I see, but sorry, currently we don't have any performance numbers yet,
> but theoretical analysis

This might be also helpful, for example, without this proposal x pci
transactions per packet, with this proposal y pci transactions per
packet.

>  and think it'll help improve performance. Some
> popular nic also supports this feature, which disables interrupt after sending
> one and needs driver to enable it again. When enabling interrupt again, drivers
> can attach some extra information.

I'm not saying it doesn't make sense, but t

1) the movitiaton needs to be well explained
2) we need to know if the existing mechanism can be reused (e.g place
used_event in a register)
3) or how the new method interact with existing interrupt moderation
features (event index, interrupt coalescing etc)

>
> >
> > > +      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.
> > > +\end{description}
> >
> > Any reason we can't reuse the existing meachsim like used index /
> > event suppression here?
> Please see my comments above.
>
> Regards,
> Xiaoguang Wang
> >
> > Basically it just sounds like we need a transport specific method for
> > them besides being part of the virtqueue.
> >
> > Thanks

Thanks

>

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

[Lege Wang]

hi,

> > +\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.
OK, will add some explanations in V2.


> > +\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?
Not enable used buffer notification. It means that if enable_notify field is
zero, only info about the latest used ring entry the driver has processed
is passed to device, and device is still not allowed to send used buffer notification.
> 
> > +\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
Sorry, I should write as you suggest, will improve these descriptions.

> 
> > +  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.
Ok, seems they should be beside above used notify suppressions descriptions

> 
> 
> 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?

Parav Pandit had suggested to add a new capability in my initial RFC patch, and Initially I also
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

See https://lore.kernel.org/all/SI2PR06MB53850CBD8F76EDC65DF2DD38FF7C2@SI2PR06MB5385.apcprd06.prod.outlook.com/

Anyway, I'm ok to add a new capability, which will make the logic much cleaner.
> > +
> >  \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?
Hmm, I think it doesn't matter, Only the used buffer enable notification is
transmitted to device successfully, device starts to judge whether it's time
to send used buffer notification to driver according to its internal interrupt policy.

Regards,
Xiaoguang Wang
> 
> > --
> > 2.34.1

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

[Lege Wang]

hi,

> > >
> > > 1) With the event index, as long as the used index doesn't pass used
> > > events you don't need to fetch even index every time
> > Yeah, I agree VIRTIO_F_EVENT_IDX could help here, but I think it's a relatively
> > crude mechanism, I have two questions below:
> >   1. Used event notification suppression structure is still located in
> >   host memory(in dpu case), I'm not sure whether used_event would
> >   be allowed to update in the running of one virtio device,
> 
> What did you mean by "update" here?
I mean "modify".

> 
> >   If it's allowed,
> >   seems devices still need to fetch newest used_event info timely.
> 
> It depends on how you define "timely", I mean unless the used event is
> not crossed, you don't need to fetch it from the main memory?
Yes, I got your point here.
> 
> But basically, I meant putting used_event in a cap/register other than
> inventing something completely new.
Sorry, I don't get your point here. What does " cap/register " mean, used_event
Is located in main memory, right?

> 
> >   2. According to virtio-spec, for example, if used_event is 0, then a device
> with
> >   VIRTIO_F_EVENT_IDX would send a used buffer notification to the driver
> after
> >   the first buffer is used (and again after the 65536th buffer), should this
> behavior
> >   be strictly followed?
> 
> Used index is best effort, and the driver must handle spurious interrupt.
OK, I'll discuss this info with my colleague, to see whether it could help.

> > >
> > > 1) compare disabling via register against memory
> > > 2) compare auto disable against no auto
> > I see, but sorry, currently we don't have any performance numbers yet,
> > but theoretical analysis
> 
> This might be also helpful, for example, without this proposal x pci
> transactions per packet, with this proposal y pci transactions per
> packet.
I see, later I'll need to see if there's any performance data based on our simulation
environment to share.

> 
> >  and think it'll help improve performance. Some
> > popular nic also supports this feature, which disables interrupt after sending
> > one and needs driver to enable it again. When enabling interrupt again, drivers
> > can attach some extra information.
> 
> I'm not saying it doesn't make sense, but t
> 
> 1) the movitiaton needs to be well explained
Totally agree.

> 2) we need to know if the existing mechanism can be reused (e.g place
> used_event in a register)
That would be better, here the "register" you mentioned is located in a virtio device's
pci bar?  

> 3) or how the new method interact with existing interrupt moderation
> features (event index, interrupt coalescing etc)

Regards,
Xiaoguang Wang

> 
> >
> > >
> > > > +      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.
> > > > +\end{description}
> > >
> > > Any reason we can't reuse the existing meachsim like used index /
> > > event suppression here?
> > Please see my comments above.
> >
> > Regards,
> > Xiaoguang Wang
> > >
> > > Basically it just sounds like we need a transport specific method for
> > > them besides being part of the virtqueue.
> > >
> > > Thanks
> 
> Thanks
> 
> >

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

[Jason Wang]

On Wed, Jul 3, 2024 at 3:37 PM Lege Wang <lege.wang@jaguarmicro.com> wrote:
>
> hi,
>
> > > >
> > > > 1) With the event index, as long as the used index doesn't pass used
> > > > events you don't need to fetch even index every time
> > > Yeah, I agree VIRTIO_F_EVENT_IDX could help here, but I think it's a relatively
> > > crude mechanism, I have two questions below:
> > >   1. Used event notification suppression structure is still located in
> > >   host memory(in dpu case), I'm not sure whether used_event would
> > >   be allowed to update in the running of one virtio device,
> >
> > What did you mean by "update" here?
> I mean "modify".
>
> >
> > >   If it's allowed,
> > >   seems devices still need to fetch newest used_event info timely.
> >
> > It depends on how you define "timely", I mean unless the used event is
> > not crossed, you don't need to fetch it from the main memory?
> Yes, I got your point here.
> >
> > But basically, I meant putting used_event in a cap/register other than
> > inventing something completely new.
> Sorry, I don't get your point here. What does " cap/register " mean, used_event
> Is located in main memory, right?

I meant something like this.

Introduce a capability to allow the driver to duplicate used_event in
the register. And say when the feature is negotiated, the driver MUST
update both used_event in the memory and the register.

Not saying it can work, but we need to know why it can't work like this.

>
> >
> > >   2. According to virtio-spec, for example, if used_event is 0, then a device
> > with
> > >   VIRTIO_F_EVENT_IDX would send a used buffer notification to the driver
> > after
> > >   the first buffer is used (and again after the 65536th buffer), should this
> > behavior
> > >   be strictly followed?
> >
> > Used index is best effort, and the driver must handle spurious interrupt.
> OK, I'll discuss this info with my colleague, to see whether it could help.

Or if you found a bug in the event index, we need to fix it in the spec first.

>
> > > >
> > > > 1) compare disabling via register against memory
> > > > 2) compare auto disable against no auto
> > > I see, but sorry, currently we don't have any performance numbers yet,
> > > but theoretical analysis
> >
> > This might be also helpful, for example, without this proposal x pci
> > transactions per packet, with this proposal y pci transactions per
> > packet.
> I see, later I'll need to see if there's any performance data based on our simulation
> environment to share.

Great.

>
> >
> > >  and think it'll help improve performance. Some
> > > popular nic also supports this feature, which disables interrupt after sending
> > > one and needs driver to enable it again. When enabling interrupt again, drivers
> > > can attach some extra information.
> >
> > I'm not saying it doesn't make sense, but t
> >
> > 1) the movitiaton needs to be well explained
> Totally agree.
>
> > 2) we need to know if the existing mechanism can be reused (e.g place
> > used_event in a register)
> That would be better, here the "register" you mentioned is located in a virtio device's
> pci bar?

Something like this or other place or we can say it could be placed in
a transport specific place.

>
> > 3) or how the new method interact with existing interrupt moderation
> > features (event index, interrupt coalescing etc)
>
> Regards,
> Xiaoguang Wang

Thanks

>
> >
> > >
> > > >
> > > > > +      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.
> > > > > +\end{description}
> > > >
> > > > Any reason we can't reuse the existing meachsim like used index /
> > > > event suppression here?
> > > Please see my comments above.
> > >
> > > Regards,
> > > Xiaoguang Wang
> > > >
> > > > Basically it just sounds like we need a transport specific method for
> > > > them besides being part of the virtqueue.
> > > >
> > > > Thanks
> >
> > Thanks
> >
> > >
>

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

[Michael S. Tsirkin]

On Wed, Jul 03, 2024 at 06:52:03AM +0000, Lege Wang wrote:
> > > +
> > >  \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?
> Hmm, I think it doesn't matter, Only the used buffer enable notification is
> transmitted to device successfully, device starts to judge whether it's time
> to send used buffer notification to driver according to its internal interrupt policy.

I don't understand what you are saying. If device has some magical
interrupt policy why do we need a notification from the driver
at all.

Conversely, if device relies on the notification and thinks that an
interrupt is not needed (because it did not get the notification yet),
while at the same time the driver is blocked waiting for an interrupt,
we get a deadlock.


-- 
MST

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

[Michael S. Tsirkin]

On Wed, Jul 03, 2024 at 03:59:11PM +0800, Jason Wang wrote:
> On Wed, Jul 3, 2024 at 3:37 PM Lege Wang <lege.wang@jaguarmicro.com> wrote:
> >
> > hi,
> >
> > > > >
> > > > > 1) With the event index, as long as the used index doesn't pass used
> > > > > events you don't need to fetch even index every time
> > > > Yeah, I agree VIRTIO_F_EVENT_IDX could help here, but I think it's a relatively
> > > > crude mechanism, I have two questions below:
> > > >   1. Used event notification suppression structure is still located in
> > > >   host memory(in dpu case), I'm not sure whether used_event would
> > > >   be allowed to update in the running of one virtio device,
> > >
> > > What did you mean by "update" here?
> > I mean "modify".
> >
> > >
> > > >   If it's allowed,
> > > >   seems devices still need to fetch newest used_event info timely.
> > >
> > > It depends on how you define "timely", I mean unless the used event is
> > > not crossed, you don't need to fetch it from the main memory?
> > Yes, I got your point here.
> > >
> > > But basically, I meant putting used_event in a cap/register other than
> > > inventing something completely new.
> > Sorry, I don't get your point here. What does " cap/register " mean, used_event
> > Is located in main memory, right?
> 
> I meant something like this.
> 
> Introduce a capability to allow the driver to duplicate used_event in
> the register. And say when the feature is negotiated, the driver MUST
> update both used_event in the memory and the register.
> 
> Not saying it can work, but we need to know why it can't work like this.

Well I feel if you are proposing a mechanism it's up to you to
explain how it works without races.
The current notification suppression works because the read
of the notification by the device flushes out used buffer writes by
the device.
If you move it to a separate domain (such as the pci bar of the device) 
this no longer holds.


> >
> > >
> > > >   2. According to virtio-spec, for example, if used_event is 0, then a device
> > > with
> > > >   VIRTIO_F_EVENT_IDX would send a used buffer notification to the driver
> > > after
> > > >   the first buffer is used (and again after the 65536th buffer), should this
> > > behavior
> > > >   be strictly followed?
> > >
> > > Used index is best effort, and the driver must handle spurious interrupt.
> > OK, I'll discuss this info with my colleague, to see whether it could help.
> 
> Or if you found a bug in the event index, we need to fix it in the spec first.

I don't think there's a bug but it's designed to reside in same memory
as the used ring.

> >
> > > > >
> > > > > 1) compare disabling via register against memory
> > > > > 2) compare auto disable against no auto
> > > > I see, but sorry, currently we don't have any performance numbers yet,
> > > > but theoretical analysis
> > >
> > > This might be also helpful, for example, without this proposal x pci
> > > transactions per packet, with this proposal y pci transactions per
> > > packet.
> > I see, later I'll need to see if there's any performance data based on our simulation
> > environment to share.
> 
> Great.
> 
> >
> > >
> > > >  and think it'll help improve performance. Some
> > > > popular nic also supports this feature, which disables interrupt after sending
> > > > one and needs driver to enable it again. When enabling interrupt again, drivers
> > > > can attach some extra information.
> > >
> > > I'm not saying it doesn't make sense, but t
> > >
> > > 1) the movitiaton needs to be well explained
> > Totally agree.
> >
> > > 2) we need to know if the existing mechanism can be reused (e.g place
> > > used_event in a register)
> > That would be better, here the "register" you mentioned is located in a virtio device's
> > pci bar?
> 
> Something like this or other place or we can say it could be placed in
> a transport specific place.

Moving things around changes ordering rules.

> >
> > > 3) or how the new method interact with existing interrupt moderation
> > > features (event index, interrupt coalescing etc)
> >
> > Regards,
> > Xiaoguang Wang
> 
> Thanks
> 
> >
> > >
> > > >
> > > > >
> > > > > > +      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.
> > > > > > +\end{description}
> > > > >
> > > > > Any reason we can't reuse the existing meachsim like used index /
> > > > > event suppression here?
> > > > Please see my comments above.
> > > >
> > > > Regards,
> > > > Xiaoguang Wang
> > > > >
> > > > > Basically it just sounds like we need a transport specific method for
> > > > > them besides being part of the virtqueue.
> > > > >
> > > > > Thanks
> > >
> > > Thanks
> > >
> > > >
> >

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

[Jason Wang]

On Wed, Jul 3, 2024 at 4:36 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Wed, Jul 03, 2024 at 03:59:11PM +0800, Jason Wang wrote:
> > On Wed, Jul 3, 2024 at 3:37 PM Lege Wang <lege.wang@jaguarmicro.com> wrote:
> > >
> > > hi,
> > >
> > > > > >
> > > > > > 1) With the event index, as long as the used index doesn't pass used
> > > > > > events you don't need to fetch even index every time
> > > > > Yeah, I agree VIRTIO_F_EVENT_IDX could help here, but I think it's a relatively
> > > > > crude mechanism, I have two questions below:
> > > > >   1. Used event notification suppression structure is still located in
> > > > >   host memory(in dpu case), I'm not sure whether used_event would
> > > > >   be allowed to update in the running of one virtio device,
> > > >
> > > > What did you mean by "update" here?
> > > I mean "modify".
> > >
> > > >
> > > > >   If it's allowed,
> > > > >   seems devices still need to fetch newest used_event info timely.
> > > >
> > > > It depends on how you define "timely", I mean unless the used event is
> > > > not crossed, you don't need to fetch it from the main memory?
> > > Yes, I got your point here.
> > > >
> > > > But basically, I meant putting used_event in a cap/register other than
> > > > inventing something completely new.
> > > Sorry, I don't get your point here. What does " cap/register " mean, used_event
> > > Is located in main memory, right?
> >
> > I meant something like this.
> >
> > Introduce a capability to allow the driver to duplicate used_event in
> > the register. And say when the feature is negotiated, the driver MUST
> > update both used_event in the memory and the register.
> >
> > Not saying it can work, but we need to know why it can't work like this.
>
> Well I feel if you are proposing a mechanism it's up to you to
> explain how it works without races.

I agree, that's why I'm saying "Not saying it can work". But what I
meant is really to find a way to reuse the event index instead of
introducing something completely new.

> The current notification suppression works because the read
> of the notification by the device flushes out used buffer writes by
> the device.

You meant read after write is ordered by PCI?

> If you move it to a separate domain (such as the pci bar of the device)
> this no longer holds.

Would this be implementation specific details or could it be done by PCI?

>
>
> > >
> > > >
> > > > >   2. According to virtio-spec, for example, if used_event is 0, then a device
> > > > with
> > > > >   VIRTIO_F_EVENT_IDX would send a used buffer notification to the driver
> > > > after
> > > > >   the first buffer is used (and again after the 65536th buffer), should this
> > > > behavior
> > > > >   be strictly followed?
> > > >
> > > > Used index is best effort, and the driver must handle spurious interrupt.
> > > OK, I'll discuss this info with my colleague, to see whether it could help.
> >
> > Or if you found a bug in the event index, we need to fix it in the spec first.
>
> I don't think there's a bug but it's designed to reside in same memory
> as the used ring.

Actually, I don't fully understand what Lege said, that's why I'm
saying "if it's a bug" ...

>
> > >
> > > > > >
> > > > > > 1) compare disabling via register against memory
> > > > > > 2) compare auto disable against no auto
> > > > > I see, but sorry, currently we don't have any performance numbers yet,
> > > > > but theoretical analysis
> > > >
> > > > This might be also helpful, for example, without this proposal x pci
> > > > transactions per packet, with this proposal y pci transactions per
> > > > packet.
> > > I see, later I'll need to see if there's any performance data based on our simulation
> > > environment to share.
> >
> > Great.
> >
> > >
> > > >
> > > > >  and think it'll help improve performance. Some
> > > > > popular nic also supports this feature, which disables interrupt after sending
> > > > > one and needs driver to enable it again. When enabling interrupt again, drivers
> > > > > can attach some extra information.
> > > >
> > > > I'm not saying it doesn't make sense, but t
> > > >
> > > > 1) the movitiaton needs to be well explained
> > > Totally agree.
> > >
> > > > 2) we need to know if the existing mechanism can be reused (e.g place
> > > > used_event in a register)
> > > That would be better, here the "register" you mentioned is located in a virtio device's
> > > pci bar?
> >
> > Something like this or other place or we can say it could be placed in
> > a transport specific place.
>
> Moving things around changes ordering rules.

Yes, something needs to be addressed.

Thanks

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

[Michael S. Tsirkin]

On Wed, Jul 03, 2024 at 04:47:41PM +0800, Jason Wang wrote:
> On Wed, Jul 3, 2024 at 4:36 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Wed, Jul 03, 2024 at 03:59:11PM +0800, Jason Wang wrote:
> > > On Wed, Jul 3, 2024 at 3:37 PM Lege Wang <lege.wang@jaguarmicro.com> wrote:
> > > >
> > > > hi,
> > > >
> > > > > > >
> > > > > > > 1) With the event index, as long as the used index doesn't pass used
> > > > > > > events you don't need to fetch even index every time
> > > > > > Yeah, I agree VIRTIO_F_EVENT_IDX could help here, but I think it's a relatively
> > > > > > crude mechanism, I have two questions below:
> > > > > >   1. Used event notification suppression structure is still located in
> > > > > >   host memory(in dpu case), I'm not sure whether used_event would
> > > > > >   be allowed to update in the running of one virtio device,
> > > > >
> > > > > What did you mean by "update" here?
> > > > I mean "modify".
> > > >
> > > > >
> > > > > >   If it's allowed,
> > > > > >   seems devices still need to fetch newest used_event info timely.
> > > > >
> > > > > It depends on how you define "timely", I mean unless the used event is
> > > > > not crossed, you don't need to fetch it from the main memory?
> > > > Yes, I got your point here.
> > > > >
> > > > > But basically, I meant putting used_event in a cap/register other than
> > > > > inventing something completely new.
> > > > Sorry, I don't get your point here. What does " cap/register " mean, used_event
> > > > Is located in main memory, right?
> > >
> > > I meant something like this.
> > >
> > > Introduce a capability to allow the driver to duplicate used_event in
> > > the register. And say when the feature is negotiated, the driver MUST
> > > update both used_event in the memory and the register.
> > >
> > > Not saying it can work, but we need to know why it can't work like this.
> >
> > Well I feel if you are proposing a mechanism it's up to you to
> > explain how it works without races.
> 
> I agree, that's why I'm saying "Not saying it can work". But what I
> meant is really to find a way to reuse the event index instead of
> introducing something completely new.
> 
> > The current notification suppression works because the read
> > of the notification by the device flushes out used buffer writes by
> > the device.
> 
> You meant read after write is ordered by PCI?

pci read responses do not bypass writes, yes.

> > If you move it to a separate domain (such as the pci bar of the device)
> > this no longer holds.
> 
> Would this be implementation specific details or could it be done by PCI?

what do you want done by PCI? Generally if things are in one place
they are easier to synchronize, if you spread them around you
need to synchronize them.

> >
> >
> > > >
> > > > >
> > > > > >   2. According to virtio-spec, for example, if used_event is 0, then a device
> > > > > with
> > > > > >   VIRTIO_F_EVENT_IDX would send a used buffer notification to the driver
> > > > > after
> > > > > >   the first buffer is used (and again after the 65536th buffer), should this
> > > > > behavior
> > > > > >   be strictly followed?
> > > > >
> > > > > Used index is best effort, and the driver must handle spurious interrupt.
> > > > OK, I'll discuss this info with my colleague, to see whether it could help.
> > >
> > > Or if you found a bug in the event index, we need to fix it in the spec first.
> >
> > I don't think there's a bug but it's designed to reside in same memory
> > as the used ring.
> 
> Actually, I don't fully understand what Lege said, that's why I'm
> saying "if it's a bug" ...
> 
> >
> > > >
> > > > > > >
> > > > > > > 1) compare disabling via register against memory
> > > > > > > 2) compare auto disable against no auto
> > > > > > I see, but sorry, currently we don't have any performance numbers yet,
> > > > > > but theoretical analysis
> > > > >
> > > > > This might be also helpful, for example, without this proposal x pci
> > > > > transactions per packet, with this proposal y pci transactions per
> > > > > packet.
> > > > I see, later I'll need to see if there's any performance data based on our simulation
> > > > environment to share.
> > >
> > > Great.
> > >
> > > >
> > > > >
> > > > > >  and think it'll help improve performance. Some
> > > > > > popular nic also supports this feature, which disables interrupt after sending
> > > > > > one and needs driver to enable it again. When enabling interrupt again, drivers
> > > > > > can attach some extra information.
> > > > >
> > > > > I'm not saying it doesn't make sense, but t
> > > > >
> > > > > 1) the movitiaton needs to be well explained
> > > > Totally agree.
> > > >
> > > > > 2) we need to know if the existing mechanism can be reused (e.g place
> > > > > used_event in a register)
> > > > That would be better, here the "register" you mentioned is located in a virtio device's
> > > > pci bar?
> > >
> > > Something like this or other place or we can say it could be placed in
> > > a transport specific place.
> >
> > Moving things around changes ordering rules.
> 
> Yes, something needs to be addressed.
> 
> Thanks

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

[Lege Wang]

hi,

> > > 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?
> > Hmm, I think it doesn't matter, Only the used buffer enable notification is
> > transmitted to device successfully, device starts to judge whether it's time
> > to send used buffer notification to driver according to its internal interrupt
> policy.
> 
> I don't understand what you are saying. If device has some magical
> interrupt policy why do we need a notification from the driver
> at all.
> 
> Conversely, if device relies on the notification and thinks that an
> interrupt is not needed (because it did not get the notification yet),
> while at the same time the driver is blocked waiting for an interrupt,
> we get a deadlock.
No, I'm not saying that our interrupt policy is magical?? Let me explain a bit
more about its simple mechanism here, there're two basic parameters to control
whether device should send used buffer notification to driver:
  1. number of used ring entries written since last used buffer notification to driver, 
    say threshold value is N.
  2. Max wait time to send a used buffer notification to driver.

The work flow would look like below:
1. driver enables used buffer notification initially when enabling virtio device
2. driver is blocked waiting interrupt.
3. if the number of used ring entries written by device is equal to or greater than above threshold N,
  a used buffered notification is sent to driver immediately, or if this condition is not met, but the period
  specified by max wait time has reached, device will still send one used buffer notification to driver.
  And device will recount number of used ring entries written by device from zero and start another
  Max wait time timer.
4. driver is waken up by interrupt, after it has processed used ring entries, it'll start to wait for interrupt
  again and enable used buffer notification.
5. go to step 2.

From above work flow, I don't think the deadlock you describe would happen.

Regards,
Xiaoguang Wang

> 
> 
> --
> MST

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

[Michael S. Tsirkin]

On Wed, Jul 03, 2024 at 12:14:01PM +0000, Lege Wang wrote:
> hi,
> 
> > > > 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?
> > > Hmm, I think it doesn't matter, Only the used buffer enable notification is
> > > transmitted to device successfully, device starts to judge whether it's time
> > > to send used buffer notification to driver according to its internal interrupt
> > policy.
> > 
> > I don't understand what you are saying. If device has some magical
> > interrupt policy why do we need a notification from the driver
> > at all.
> > 
> > Conversely, if device relies on the notification and thinks that an
> > interrupt is not needed (because it did not get the notification yet),
> > while at the same time the driver is blocked waiting for an interrupt,
> > we get a deadlock.
> No, I'm not saying that our interrupt policy is magical?? Let me explain a bit
> more about its simple mechanism here, there're two basic parameters to control
> whether device should send used buffer notification to driver:
>   1. number of used ring entries written since last used buffer notification to driver, 
>     say threshold value is N.
>   2. Max wait time to send a used buffer notification to driver.
> 
> The work flow would look like below:
> 1. driver enables used buffer notification initially when enabling virtio device
> 2. driver is blocked waiting interrupt.
> 3. if the number of used ring entries written by device is equal to or greater than above threshold N,
>   a used buffered notification is sent to driver immediately, or if this condition is not met, but the period
>   specified by max wait time has reached, device will still send one used buffer notification to driver.
>   And device will recount number of used ring entries written by device from zero and start another
>   Max wait time timer.
> 4. driver is waken up by interrupt, after it has processed used ring entries, it'll start to wait for interrupt
>   again and enable used buffer notification.
> 5. go to step 2.
> 
> >From above work flow, I don't think the deadlock you describe would happen.
> 
> Regards,
> Xiaoguang Wang

1. device sends an interrupt
2. device writes a used entry

Will another interrupt be sent, with driver doing nothing at all?



> > 
> > 
> > --
> > MST

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

[Lege Wang]

hi,

> -----Original Message-----
> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Wednesday, July 3, 2024 8:34 PM
> To: Lege Wang <lege.wang@jaguarmicro.com>
> Cc: virtio-comment@lists.linux.dev; vattunuru@marvell.com;
> ndabilpuram@marvell.com; parav@nvidia.com; Leo Liu
> <leo.liu@jaguarmicro.com>; Angus Chen <angus.chen@jaguarmicro.com>
> Subject: Re: [PATCH] VIRTIO_F_USED_EVENT_AUTO_DISABLE: add new used
> buffer notification suppression mechanism
> 
> External Mail: This email originated from OUTSIDE of the organization!
> Do not click links, open attachments or provide ANY information unless you
> recognize the sender and know the content is safe.
> 
> 
> On Wed, Jul 03, 2024 at 12:14:01PM +0000, Lege Wang wrote:
> > hi,
> >
> > > > > 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?
> > > > Hmm, I think it doesn't matter, Only the used buffer enable notification is
> > > > transmitted to device successfully, device starts to judge whether it's time
> > > > to send used buffer notification to driver according to its internal
> interrupt
> > > policy.
> > >
> > > I don't understand what you are saying. If device has some magical
> > > interrupt policy why do we need a notification from the driver
> > > at all.
> > >
> > > Conversely, if device relies on the notification and thinks that an
> > > interrupt is not needed (because it did not get the notification yet),
> > > while at the same time the driver is blocked waiting for an interrupt,
> > > we get a deadlock.
> > No, I'm not saying that our interrupt policy is magical?? Let me explain a bit
> > more about its simple mechanism here, there're two basic parameters to
> control
> > whether device should send used buffer notification to driver:
> >   1. number of used ring entries written since last used buffer notification to
> driver,
> >     say threshold value is N.
> >   2. Max wait time to send a used buffer notification to driver.
> >
> > The work flow would look like below:
> > 1. driver enables used buffer notification initially when enabling virtio device
> > 2. driver is blocked waiting interrupt.
> > 3. if the number of used ring entries written by device is equal to or greater
> than above threshold N,
> >   a used buffered notification is sent to driver immediately, or if this
> condition is not met, but the period
> >   specified by max wait time has reached, device will still send one used
> buffer notification to driver.
> >   And device will recount number of used ring entries written by device
> from zero and start another
> >   Max wait time timer.
> > 4. driver is waken up by interrupt, after it has processed used ring entries, it'll
> start to wait for interrupt
> >   again and enable used buffer notification.
> > 5. go to step 2.
> >
> > >From above work flow, I don't think the deadlock you describe would
> happen.
> >
> > Regards,
> > Xiaoguang Wang
> 
> 1. device sends an interrupt
> 2. device writes a used entry
> 
> Will another interrupt be sent, with driver doing nothing at all?
Unless driver enables device's used buffer notification, the device will
not send an interrupt.

Regards,
Xiaoguang Wang
> 
> 
> 
> > >
> > >
> > > --
> > > MST

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

[Jason Wang]

On Wed, Jul 3, 2024 at 5:09 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Wed, Jul 03, 2024 at 04:47:41PM +0800, Jason Wang wrote:
> > On Wed, Jul 3, 2024 at 4:36 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Wed, Jul 03, 2024 at 03:59:11PM +0800, Jason Wang wrote:
> > > > On Wed, Jul 3, 2024 at 3:37 PM Lege Wang <lege.wang@jaguarmicro.com> wrote:
> > > > >
> > > > > hi,
> > > > >
> > > > > > > >
> > > > > > > > 1) With the event index, as long as the used index doesn't pass used
> > > > > > > > events you don't need to fetch even index every time
> > > > > > > Yeah, I agree VIRTIO_F_EVENT_IDX could help here, but I think it's a relatively
> > > > > > > crude mechanism, I have two questions below:
> > > > > > >   1. Used event notification suppression structure is still located in
> > > > > > >   host memory(in dpu case), I'm not sure whether used_event would
> > > > > > >   be allowed to update in the running of one virtio device,
> > > > > >
> > > > > > What did you mean by "update" here?
> > > > > I mean "modify".
> > > > >
> > > > > >
> > > > > > >   If it's allowed,
> > > > > > >   seems devices still need to fetch newest used_event info timely.
> > > > > >
> > > > > > It depends on how you define "timely", I mean unless the used event is
> > > > > > not crossed, you don't need to fetch it from the main memory?
> > > > > Yes, I got your point here.
> > > > > >
> > > > > > But basically, I meant putting used_event in a cap/register other than
> > > > > > inventing something completely new.
> > > > > Sorry, I don't get your point here. What does " cap/register " mean, used_event
> > > > > Is located in main memory, right?
> > > >
> > > > I meant something like this.
> > > >
> > > > Introduce a capability to allow the driver to duplicate used_event in
> > > > the register. And say when the feature is negotiated, the driver MUST
> > > > update both used_event in the memory and the register.
> > > >
> > > > Not saying it can work, but we need to know why it can't work like this.
> > >
> > > Well I feel if you are proposing a mechanism it's up to you to
> > > explain how it works without races.
> >
> > I agree, that's why I'm saying "Not saying it can work". But what I
> > meant is really to find a way to reuse the event index instead of
> > introducing something completely new.
> >
> > > The current notification suppression works because the read
> > > of the notification by the device flushes out used buffer writes by
> > > the device.
> >
> > You meant read after write is ordered by PCI?
>
> pci read responses do not bypass writes, yes.
>
> > > If you move it to a separate domain (such as the pci bar of the device)
> > > this no longer holds.
> >
> > Would this be implementation specific details or could it be done by PCI?
>
> what do you want done by PCI? Generally if things are in one place
> they are easier to synchronize, if you spread them around you
> need to synchronize them.

I meant the synchronization looks more like an implementation detail
in the device.  Synchronizing with device internal logic should be
simpler than with PCI/memory.

For example, did you mean the synchronization between driver write to
register (PCI write) and device read from that (internal logic). If
needed, an implementation needs to serialize those two, then we are
probably fine.

Thanks

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

[Michael S. Tsirkin]

On Thu, Jul 04, 2024 at 01:37:44PM +0800, Jason Wang wrote:
> On Wed, Jul 3, 2024 at 5:09 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Wed, Jul 03, 2024 at 04:47:41PM +0800, Jason Wang wrote:
> > > On Wed, Jul 3, 2024 at 4:36 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > >
> > > > On Wed, Jul 03, 2024 at 03:59:11PM +0800, Jason Wang wrote:
> > > > > On Wed, Jul 3, 2024 at 3:37 PM Lege Wang <lege.wang@jaguarmicro.com> wrote:
> > > > > >
> > > > > > hi,
> > > > > >
> > > > > > > > >
> > > > > > > > > 1) With the event index, as long as the used index doesn't pass used
> > > > > > > > > events you don't need to fetch even index every time
> > > > > > > > Yeah, I agree VIRTIO_F_EVENT_IDX could help here, but I think it's a relatively
> > > > > > > > crude mechanism, I have two questions below:
> > > > > > > >   1. Used event notification suppression structure is still located in
> > > > > > > >   host memory(in dpu case), I'm not sure whether used_event would
> > > > > > > >   be allowed to update in the running of one virtio device,
> > > > > > >
> > > > > > > What did you mean by "update" here?
> > > > > > I mean "modify".
> > > > > >
> > > > > > >
> > > > > > > >   If it's allowed,
> > > > > > > >   seems devices still need to fetch newest used_event info timely.
> > > > > > >
> > > > > > > It depends on how you define "timely", I mean unless the used event is
> > > > > > > not crossed, you don't need to fetch it from the main memory?
> > > > > > Yes, I got your point here.
> > > > > > >
> > > > > > > But basically, I meant putting used_event in a cap/register other than
> > > > > > > inventing something completely new.
> > > > > > Sorry, I don't get your point here. What does " cap/register " mean, used_event
> > > > > > Is located in main memory, right?
> > > > >
> > > > > I meant something like this.
> > > > >
> > > > > Introduce a capability to allow the driver to duplicate used_event in
> > > > > the register. And say when the feature is negotiated, the driver MUST
> > > > > update both used_event in the memory and the register.
> > > > >
> > > > > Not saying it can work, but we need to know why it can't work like this.
> > > >
> > > > Well I feel if you are proposing a mechanism it's up to you to
> > > > explain how it works without races.
> > >
> > > I agree, that's why I'm saying "Not saying it can work". But what I
> > > meant is really to find a way to reuse the event index instead of
> > > introducing something completely new.
> > >
> > > > The current notification suppression works because the read
> > > > of the notification by the device flushes out used buffer writes by
> > > > the device.
> > >
> > > You meant read after write is ordered by PCI?
> >
> > pci read responses do not bypass writes, yes.
> >
> > > > If you move it to a separate domain (such as the pci bar of the device)
> > > > this no longer holds.
> > >
> > > Would this be implementation specific details or could it be done by PCI?
> >
> > what do you want done by PCI? Generally if things are in one place
> > they are easier to synchronize, if you spread them around you
> > need to synchronize them.
> 
> I meant the synchronization looks more like an implementation detail
> in the device.  Synchronizing with device internal logic should be
> simpler than with PCI/memory.
> 
> For example, did you mean the synchronization between driver write to
> register (PCI write) and device read from that (internal logic). If
> needed, an implementation needs to serialize those two, then we are
> probably fine.
> 
> Thanks

I mean synchronization between driver write to device and
device write to memory.



What I see working, is something I proposed a long time ago:
notify devices about changes to the notification suppression
area. This adds more overhead
driver notify -> device read -> memory read response
but I think it works.

What I couldn't decide, is whether it's worth sending a notification
when switching from enable to disable with packed ring.



-- 
MST

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

[Lege Wang]

hi

Thanks for your comments in previous discussions.
I'd like to confirm that do you understand this proposal's idea now. If yes,
I may start to prepare a more formal V2.

Regards,
Xiaoguang Wang

> -----Original Message-----
> From: Lege Wang <lege.wang@jaguarmicro.com>
> Sent: Monday, July 1, 2024 11:45 AM
> To: virtio-comment@lists.linux.dev
> Cc: vattunuru@marvell.com; ndabilpuram@marvell.com; parav@nvidia.com;
> mst@redhat.com; Leo Liu <leo.liu@jaguarmicro.com>; Angus Chen
> <angus.chen@jaguarmicro.com>; Lege Wang <lege.wang@jaguarmicro.com>
> Subject: [PATCH] VIRTIO_F_USED_EVENT_AUTO_DISABLE: add new used buffer
> notification suppression mechanism
> 
> 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>
> ---
>  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.
> +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.
> +\end{description}
> +
>  \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.
> +
> +  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, but add a 4 bytes
> offset:
> +\begin{lstlisting}
> +        cap.offset + queue_notify_off * notify_off_multiplier + 4
> +\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.
> @@ -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
> --
> 2.34.1

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

[Parav Pandit]

Hi Lege Wang,


> From: Lege Wang <lege.wang@jaguarmicro.com>
> Sent: Friday, July 5, 2024 9:05 AM
> 
> hi
> 
> Thanks for your comments in previous discussions.
> I'd like to confirm that do you understand this proposal's idea now. If yes, I
> may start to prepare a more formal V2.
> 

Notification enablement offset adjacent to current notification offset seems a better approach than adding the new capability for following reasons.

1. pci capability area (non-extended cap) is full to its capacity. New addition can only work in theory.
An obvious alternative is to add the extended cap and add there.
However, it is not a good idea either as the PCI spec strongly recommends to not keep adding vendor specific stuff in the PCI capability section.
(even though an option is kept for the vendor capability).

2. New area adjacent to current offset can further is simple enough to utilize.
There is no good reason to complicate by another extended cap.
If there is one, lets discuss it first.


> Regards,
> Xiaoguang Wang
> 
> > -----Original Message-----
> > From: Lege Wang <lege.wang@jaguarmicro.com>
> > Sent: Monday, July 1, 2024 11:45 AM
> > To: virtio-comment@lists.linux.dev
> > Cc: vattunuru@marvell.com; ndabilpuram@marvell.com;
> parav@nvidia.com;
> > mst@redhat.com; Leo Liu <leo.liu@jaguarmicro.com>; Angus Chen
> > <angus.chen@jaguarmicro.com>; Lege Wang
> <lege.wang@jaguarmicro.com>
> > Subject: [PATCH] VIRTIO_F_USED_EVENT_AUTO_DISABLE: add new used
> buffer
> > notification suppression mechanism
> >
> > 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>
> > ---
> >  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.
> > +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.
> > +\end{description}
> > +
> >  \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.
> > +
> > +  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, but add a 4
> > +bytes
> > offset:
> > +\begin{lstlisting}
> > +        cap.offset + queue_notify_off * notify_off_multiplier + 4
> > +\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.
> > @@ -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
> > --
> > 2.34.1

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

[Jason Wang]

On Thu, Jul 4, 2024 at 4:31 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Thu, Jul 04, 2024 at 01:37:44PM +0800, Jason Wang wrote:
> > On Wed, Jul 3, 2024 at 5:09 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Wed, Jul 03, 2024 at 04:47:41PM +0800, Jason Wang wrote:
> > > > On Wed, Jul 3, 2024 at 4:36 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > >
> > > > > On Wed, Jul 03, 2024 at 03:59:11PM +0800, Jason Wang wrote:
> > > > > > On Wed, Jul 3, 2024 at 3:37 PM Lege Wang <lege.wang@jaguarmicro.com> wrote:
> > > > > > >
> > > > > > > hi,
> > > > > > >
> > > > > > > > > >
> > > > > > > > > > 1) With the event index, as long as the used index doesn't pass used
> > > > > > > > > > events you don't need to fetch even index every time
> > > > > > > > > Yeah, I agree VIRTIO_F_EVENT_IDX could help here, but I think it's a relatively
> > > > > > > > > crude mechanism, I have two questions below:
> > > > > > > > >   1. Used event notification suppression structure is still located in
> > > > > > > > >   host memory(in dpu case), I'm not sure whether used_event would
> > > > > > > > >   be allowed to update in the running of one virtio device,
> > > > > > > >
> > > > > > > > What did you mean by "update" here?
> > > > > > > I mean "modify".
> > > > > > >
> > > > > > > >
> > > > > > > > >   If it's allowed,
> > > > > > > > >   seems devices still need to fetch newest used_event info timely.
> > > > > > > >
> > > > > > > > It depends on how you define "timely", I mean unless the used event is
> > > > > > > > not crossed, you don't need to fetch it from the main memory?
> > > > > > > Yes, I got your point here.
> > > > > > > >
> > > > > > > > But basically, I meant putting used_event in a cap/register other than
> > > > > > > > inventing something completely new.
> > > > > > > Sorry, I don't get your point here. What does " cap/register " mean, used_event
> > > > > > > Is located in main memory, right?
> > > > > >
> > > > > > I meant something like this.
> > > > > >
> > > > > > Introduce a capability to allow the driver to duplicate used_event in
> > > > > > the register. And say when the feature is negotiated, the driver MUST
> > > > > > update both used_event in the memory and the register.
> > > > > >
> > > > > > Not saying it can work, but we need to know why it can't work like this.
> > > > >
> > > > > Well I feel if you are proposing a mechanism it's up to you to
> > > > > explain how it works without races.
> > > >
> > > > I agree, that's why I'm saying "Not saying it can work". But what I
> > > > meant is really to find a way to reuse the event index instead of
> > > > introducing something completely new.
> > > >
> > > > > The current notification suppression works because the read
> > > > > of the notification by the device flushes out used buffer writes by
> > > > > the device.
> > > >
> > > > You meant read after write is ordered by PCI?
> > >
> > > pci read responses do not bypass writes, yes.
> > >
> > > > > If you move it to a separate domain (such as the pci bar of the device)
> > > > > this no longer holds.
> > > >
> > > > Would this be implementation specific details or could it be done by PCI?
> > >
> > > what do you want done by PCI? Generally if things are in one place
> > > they are easier to synchronize, if you spread them around you
> > > need to synchronize them.
> >
> > I meant the synchronization looks more like an implementation detail
> > in the device.  Synchronizing with device internal logic should be
> > simpler than with PCI/memory.
> >
> > For example, did you mean the synchronization between driver write to
> > register (PCI write) and device read from that (internal logic). If
> > needed, an implementation needs to serialize those two, then we are
> > probably fine.
> >
> > Thanks
>
> I mean synchronization between driver write to device and
> device write to memory.

Not sure I get you, I meant put used_event in a register. Driver will
only write to that and the device will only read from that..

>
>
>
> What I see working, is something I proposed a long time ago:
> notify devices about changes to the notification suppression
> area.

I think this is something similar to my proposal, but the difference
is that I propose to carry the notification suppression into the write
(e.g used event).

> This adds more overhead
> driver notify -> device read -> memory read response
> but I think it works.
>
> What I couldn't decide, is whether it's worth sending a notification
> when switching from enable to disable with packed ring.

It needs a feature so for software devices it can choose to not have
those overheads.

Thanks

>
>
>
> --
> MST
>

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

[Michael S. Tsirkin]

On Fri, Jul 05, 2024 at 01:48:36PM +0800, Jason Wang wrote:
> On Thu, Jul 4, 2024 at 4:31 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Thu, Jul 04, 2024 at 01:37:44PM +0800, Jason Wang wrote:
> > > On Wed, Jul 3, 2024 at 5:09 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > >
> > > > On Wed, Jul 03, 2024 at 04:47:41PM +0800, Jason Wang wrote:
> > > > > On Wed, Jul 3, 2024 at 4:36 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > > >
> > > > > > On Wed, Jul 03, 2024 at 03:59:11PM +0800, Jason Wang wrote:
> > > > > > > On Wed, Jul 3, 2024 at 3:37 PM Lege Wang <lege.wang@jaguarmicro.com> wrote:
> > > > > > > >
> > > > > > > > hi,
> > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > 1) With the event index, as long as the used index doesn't pass used
> > > > > > > > > > > events you don't need to fetch even index every time
> > > > > > > > > > Yeah, I agree VIRTIO_F_EVENT_IDX could help here, but I think it's a relatively
> > > > > > > > > > crude mechanism, I have two questions below:
> > > > > > > > > >   1. Used event notification suppression structure is still located in
> > > > > > > > > >   host memory(in dpu case), I'm not sure whether used_event would
> > > > > > > > > >   be allowed to update in the running of one virtio device,
> > > > > > > > >
> > > > > > > > > What did you mean by "update" here?
> > > > > > > > I mean "modify".
> > > > > > > >
> > > > > > > > >
> > > > > > > > > >   If it's allowed,
> > > > > > > > > >   seems devices still need to fetch newest used_event info timely.
> > > > > > > > >
> > > > > > > > > It depends on how you define "timely", I mean unless the used event is
> > > > > > > > > not crossed, you don't need to fetch it from the main memory?
> > > > > > > > Yes, I got your point here.
> > > > > > > > >
> > > > > > > > > But basically, I meant putting used_event in a cap/register other than
> > > > > > > > > inventing something completely new.
> > > > > > > > Sorry, I don't get your point here. What does " cap/register " mean, used_event
> > > > > > > > Is located in main memory, right?
> > > > > > >
> > > > > > > I meant something like this.
> > > > > > >
> > > > > > > Introduce a capability to allow the driver to duplicate used_event in
> > > > > > > the register. And say when the feature is negotiated, the driver MUST
> > > > > > > update both used_event in the memory and the register.
> > > > > > >
> > > > > > > Not saying it can work, but we need to know why it can't work like this.
> > > > > >
> > > > > > Well I feel if you are proposing a mechanism it's up to you to
> > > > > > explain how it works without races.
> > > > >
> > > > > I agree, that's why I'm saying "Not saying it can work". But what I
> > > > > meant is really to find a way to reuse the event index instead of
> > > > > introducing something completely new.
> > > > >
> > > > > > The current notification suppression works because the read
> > > > > > of the notification by the device flushes out used buffer writes by
> > > > > > the device.
> > > > >
> > > > > You meant read after write is ordered by PCI?
> > > >
> > > > pci read responses do not bypass writes, yes.
> > > >
> > > > > > If you move it to a separate domain (such as the pci bar of the device)
> > > > > > this no longer holds.
> > > > >
> > > > > Would this be implementation specific details or could it be done by PCI?
> > > >
> > > > what do you want done by PCI? Generally if things are in one place
> > > > they are easier to synchronize, if you spread them around you
> > > > need to synchronize them.
> > >
> > > I meant the synchronization looks more like an implementation detail
> > > in the device.  Synchronizing with device internal logic should be
> > > simpler than with PCI/memory.
> > >
> > > For example, did you mean the synchronization between driver write to
> > > register (PCI write) and device read from that (internal logic). If
> > > needed, an implementation needs to serialize those two, then we are
> > > probably fine.
> > >
> > > Thanks
> >
> > I mean synchronization between driver write to device and
> > device write to memory.
> 
> Not sure I get you, I meant put used_event in a register. Driver will
> only write to that and the device will only read from that..

But the ring is in driver memory.


> >
> >
> >
> > What I see working, is something I proposed a long time ago:
> > notify devices about changes to the notification suppression
> > area.
> 
> I think this is something similar to my proposal, but the difference
> is that I propose to carry the notification suppression into the write
> (e.g used event).

Then it becomes racy.

> > This adds more overhead
> > driver notify -> device read -> memory read response
> > but I think it works.
> >
> > What I couldn't decide, is whether it's worth sending a notification
> > when switching from enable to disable with packed ring.
> 
> It needs a feature so for software devices it can choose to not have
> those overheads.
> 
> Thanks

In short, it's not a trivial thing.

> >
> >
> >
> > --
> > MST
> >

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

[Michael S. Tsirkin]

On Fri, Jul 05, 2024 at 04:42:52AM +0000, Parav Pandit wrote:
> Hi Lege Wang,
> 
> 
> > From: Lege Wang <lege.wang@jaguarmicro.com>
> > Sent: Friday, July 5, 2024 9:05 AM
> > 
> > hi
> > 
> > Thanks for your comments in previous discussions.
> > I'd like to confirm that do you understand this proposal's idea now. If yes, I
> > may start to prepare a more formal V2.
> > 
> 
> Notification enablement offset adjacent to current notification offset seems a better approach than adding the new capability for following reasons.
> 
> 1. pci capability area (non-extended cap) is full to its capacity. New addition can only work in theory.
> An obvious alternative is to add the extended cap and add there.
> However, it is not a good idea either as the PCI spec strongly recommends to not keep adding vendor specific stuff in the PCI capability section.

Can you point me to the relevant section in the spec?

> (even though an option is kept for the vendor capability).

Interesting. EP guys are also asking about putting
the capabilities in a BAR. So we might want an option
to move all capabilities there.

> 2. New area adjacent to current offset can further is simple enough to utilize.
> There is no good reason to complicate by another extended cap.
> If there is one, lets discuss it first.

We might want to pass more data inline in the notification
going forward.
Also kick and suppression can be handled by different entities,
they would need to be in different pages.
Basically, don't combine unrelated things in one place.

-- 
MST

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

[Michael S. Tsirkin]

On Thu, Jul 04, 2024 at 02:27:20AM +0000, Lege Wang wrote:
> hi,
> 
> > -----Original Message-----
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Wednesday, July 3, 2024 8:34 PM
> > To: Lege Wang <lege.wang@jaguarmicro.com>
> > Cc: virtio-comment@lists.linux.dev; vattunuru@marvell.com;
> > ndabilpuram@marvell.com; parav@nvidia.com; Leo Liu
> > <leo.liu@jaguarmicro.com>; Angus Chen <angus.chen@jaguarmicro.com>
> > Subject: Re: [PATCH] VIRTIO_F_USED_EVENT_AUTO_DISABLE: add new used
> > buffer notification suppression mechanism
> > 
> > External Mail: This email originated from OUTSIDE of the organization!
> > Do not click links, open attachments or provide ANY information unless you
> > recognize the sender and know the content is safe.
> > 
> > 
> > On Wed, Jul 03, 2024 at 12:14:01PM +0000, Lege Wang wrote:
> > > hi,
> > >
> > > > > > 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?
> > > > > Hmm, I think it doesn't matter, Only the used buffer enable notification is
> > > > > transmitted to device successfully, device starts to judge whether it's time
> > > > > to send used buffer notification to driver according to its internal
> > interrupt
> > > > policy.
> > > >
> > > > I don't understand what you are saying. If device has some magical
> > > > interrupt policy why do we need a notification from the driver
> > > > at all.
> > > >
> > > > Conversely, if device relies on the notification and thinks that an
> > > > interrupt is not needed (because it did not get the notification yet),
> > > > while at the same time the driver is blocked waiting for an interrupt,
> > > > we get a deadlock.
> > > No, I'm not saying that our interrupt policy is magical?? Let me explain a bit
> > > more about its simple mechanism here, there're two basic parameters to
> > control
> > > whether device should send used buffer notification to driver:
> > >   1. number of used ring entries written since last used buffer notification to
> > driver,
> > >     say threshold value is N.
> > >   2. Max wait time to send a used buffer notification to driver.
> > >
> > > The work flow would look like below:
> > > 1. driver enables used buffer notification initially when enabling virtio device
> > > 2. driver is blocked waiting interrupt.
> > > 3. if the number of used ring entries written by device is equal to or greater
> > than above threshold N,
> > >   a used buffered notification is sent to driver immediately, or if this
> > condition is not met, but the period
> > >   specified by max wait time has reached, device will still send one used
> > buffer notification to driver.
> > >   And device will recount number of used ring entries written by device
> > from zero and start another
> > >   Max wait time timer.
> > > 4. driver is waken up by interrupt, after it has processed used ring entries, it'll
> > start to wait for interrupt
> > >   again and enable used buffer notification.
> > > 5. go to step 2.
> > >
> > > >From above work flow, I don't think the deadlock you describe would
> > happen.
> > >
> > > Regards,
> > > Xiaoguang Wang
> > 
> > 1. device sends an interrupt
> > 2. device writes a used entry
> > 
> > Will another interrupt be sent, with driver doing nothing at all?
> Unless driver enables device's used buffer notification, the device will
> not send an interrupt.
> 
> Regards,
> Xiaoguang Wang

how about this:
1. device sends an interrupt
2. device writes a used entry
2. driver enables interrupt



> > 
> > 
> > 
> > > >
> > > >
> > > > --
> > > > MST

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

[Michael S. Tsirkin]

On Fri, Jul 05, 2024 at 03:35:17AM +0000, Lege Wang wrote:
> hi
> 
> Thanks for your comments in previous discussions.
> I'd like to confirm that do you understand this proposal's idea now. If yes,
> I may start to prepare a more formal V2.
> 
> Regards,
> Xiaoguang Wang

It's not that I don't understand the proposal, it's that I don't
think it's a good proposal. It seems to rely on timeouts to solve
race conditions, which are all hidden outside of spec.
We should explain how things are supposed to work.

-- 
MST

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

[Michael S. Tsirkin]

On Fri, Jul 05, 2024 at 04:42:52AM +0000, Parav Pandit wrote:
> 1. pci capability area (non-extended cap) is full to its capacity. New addition can only work in theory.
> An obvious alternative is to add the extended cap and add there.
> However, it is not a good idea either as the PCI spec strongly recommends to not keep adding vendor specific stuff in the PCI capability section.
> (even though an option is kept for the vendor capability).

To stress, we need to solve this problem, even if the specific issue
can be worked around differently, we keep adding new caps.

-- 
MST

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

[Lege Wang]

hi,

> >
> > > -----Original Message-----
> > > From: Michael S. Tsirkin <mst@redhat.com>
> > > Sent: Wednesday, July 3, 2024 8:34 PM
> > > To: Lege Wang <lege.wang@jaguarmicro.com>
> > > Cc: virtio-comment@lists.linux.dev; vattunuru@marvell.com;
> > > ndabilpuram@marvell.com; parav@nvidia.com; Leo Liu
> > > <leo.liu@jaguarmicro.com>; Angus Chen <angus.chen@jaguarmicro.com>
> > > Subject: Re: [PATCH] VIRTIO_F_USED_EVENT_AUTO_DISABLE: add new used
> > > buffer notification suppression mechanism
> > >
> > > External Mail: This email originated from OUTSIDE of the organization!
> > > Do not click links, open attachments or provide ANY information unless you
> > > recognize the sender and know the content is safe.
> > >
> > >
> > > On Wed, Jul 03, 2024 at 12:14:01PM +0000, Lege Wang wrote:
> > > > hi,
> > > >
> > > > > > > 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?
> > > > > > Hmm, I think it doesn't matter, Only the used buffer enable notification
> is
> > > > > > transmitted to device successfully, device starts to judge whether it's
> time
> > > > > > to send used buffer notification to driver according to its internal
> > > interrupt
> > > > > policy.
> > > > >
> > > > > I don't understand what you are saying. If device has some magical
> > > > > interrupt policy why do we need a notification from the driver
> > > > > at all.
> > > > >
> > > > > Conversely, if device relies on the notification and thinks that an
> > > > > interrupt is not needed (because it did not get the notification yet),
> > > > > while at the same time the driver is blocked waiting for an interrupt,
> > > > > we get a deadlock.
> > > > No, I'm not saying that our interrupt policy is magical?? Let me explain a
> bit
> > > > more about its simple mechanism here, there're two basic parameters to
> > > control
> > > > whether device should send used buffer notification to driver:
> > > >   1. number of used ring entries written since last used buffer notification
> to
> > > driver,
> > > >     say threshold value is N.
> > > >   2. Max wait time to send a used buffer notification to driver.
> > > >
> > > > The work flow would look like below:
> > > > 1. driver enables used buffer notification initially when enabling virtio
> device
> > > > 2. driver is blocked waiting interrupt.
> > > > 3. if the number of used ring entries written by device is equal to or
> greater
> > > than above threshold N,
> > > >   a used buffered notification is sent to driver immediately, or if this
> > > condition is not met, but the period
> > > >   specified by max wait time has reached, device will still send one used
> > > buffer notification to driver.
> > > >   And device will recount number of used ring entries written by device
> > > from zero and start another
> > > >   Max wait time timer.
> > > > 4. driver is waken up by interrupt, after it has processed used ring entries,
> it'll
> > > start to wait for interrupt
> > > >   again and enable used buffer notification.
> > > > 5. go to step 2.
> > > >
> > > > >From above work flow, I don't think the deadlock you describe would
> > > happen.
> > > >
> > > > Regards,
> > > > Xiaoguang Wang
> > >
> > > 1. device sends an interrupt
> > > 2. device writes a used entry
> > >
> > > Will another interrupt be sent, with driver doing nothing at all?
> > Unless driver enables device's used buffer notification, the device will
> > not send an interrupt.
> >
> > Regards,
> > Xiaoguang Wang
> 
> how about this:
> 1. device sends an interrupt
> 2. device writes a used entry
> 2. driver enables interrupt

Indeed enabling used buffer notification and sending used buffer notification are matched operations.
Time-sequence |  driver           |  device
T1           | enable notification  |
T2           |                  |  send notification
T3           |                  |
T4           | enable notification  |
T5           |                  |  send notification

That means once driver enables interrupt, device will ensure that it will send an
Interrupt if there are used events written. If driver does not enable interrupt at all,
Device will not send interrupt.

But yet there is a case to take care, which driver does not handle all used buffer events it received,
parav explained It would happen before. As I said in this proposal, when driver enables interrupt, it'll
also send driver's last_used_idx info to device, see below handling:
  1 driver enable interrupt
  2 driver submit 5 requests
  3 all 5 request are completed, device sends interrupt, now device's used_idx is 5.
  4 driver gets interrupt, and it just handled 3 requests, so last_used_idx is 3.
  5 driver enables interrupt and attach last_used_idx(3) info.
  6 device will know that driver does not handle 2 requests and will add these two requests to
    the number of used ring entries written since last used buffer notification, that means
    though if there is no new used ring entries written, because of these two outstanding requests,
    device will still send an interrupt.

So I still think the deadlock you describe would not happen.

Regards,
Xiaoguang Wang

> to

Regards,
Xiaoguang Wang
> 
> 
> 
> > >
> > >
> > >
> > > > >
> > > > >
> > > > > --
> > > > > MST

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

[Michael S. Tsirkin]

On Fri, Jul 05, 2024 at 11:12:47AM +0000, Lege Wang wrote:
> hi,
> 
> > >
> > > > -----Original Message-----
> > > > From: Michael S. Tsirkin <mst@redhat.com>
> > > > Sent: Wednesday, July 3, 2024 8:34 PM
> > > > To: Lege Wang <lege.wang@jaguarmicro.com>
> > > > Cc: virtio-comment@lists.linux.dev; vattunuru@marvell.com;
> > > > ndabilpuram@marvell.com; parav@nvidia.com; Leo Liu
> > > > <leo.liu@jaguarmicro.com>; Angus Chen <angus.chen@jaguarmicro.com>
> > > > Subject: Re: [PATCH] VIRTIO_F_USED_EVENT_AUTO_DISABLE: add new used
> > > > buffer notification suppression mechanism
> > > >
> > > > External Mail: This email originated from OUTSIDE of the organization!
> > > > Do not click links, open attachments or provide ANY information unless you
> > > > recognize the sender and know the content is safe.
> > > >
> > > >
> > > > On Wed, Jul 03, 2024 at 12:14:01PM +0000, Lege Wang wrote:
> > > > > hi,
> > > > >
> > > > > > > > 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?
> > > > > > > Hmm, I think it doesn't matter, Only the used buffer enable notification
> > is
> > > > > > > transmitted to device successfully, device starts to judge whether it's
> > time
> > > > > > > to send used buffer notification to driver according to its internal
> > > > interrupt
> > > > > > policy.
> > > > > >
> > > > > > I don't understand what you are saying. If device has some magical
> > > > > > interrupt policy why do we need a notification from the driver
> > > > > > at all.
> > > > > >
> > > > > > Conversely, if device relies on the notification and thinks that an
> > > > > > interrupt is not needed (because it did not get the notification yet),
> > > > > > while at the same time the driver is blocked waiting for an interrupt,
> > > > > > we get a deadlock.
> > > > > No, I'm not saying that our interrupt policy is magical?? Let me explain a
> > bit
> > > > > more about its simple mechanism here, there're two basic parameters to
> > > > control
> > > > > whether device should send used buffer notification to driver:
> > > > >   1. number of used ring entries written since last used buffer notification
> > to
> > > > driver,
> > > > >     say threshold value is N.
> > > > >   2. Max wait time to send a used buffer notification to driver.
> > > > >
> > > > > The work flow would look like below:
> > > > > 1. driver enables used buffer notification initially when enabling virtio
> > device
> > > > > 2. driver is blocked waiting interrupt.
> > > > > 3. if the number of used ring entries written by device is equal to or
> > greater
> > > > than above threshold N,
> > > > >   a used buffered notification is sent to driver immediately, or if this
> > > > condition is not met, but the period
> > > > >   specified by max wait time has reached, device will still send one used
> > > > buffer notification to driver.
> > > > >   And device will recount number of used ring entries written by device
> > > > from zero and start another
> > > > >   Max wait time timer.
> > > > > 4. driver is waken up by interrupt, after it has processed used ring entries,
> > it'll
> > > > start to wait for interrupt
> > > > >   again and enable used buffer notification.
> > > > > 5. go to step 2.
> > > > >
> > > > > >From above work flow, I don't think the deadlock you describe would
> > > > happen.
> > > > >
> > > > > Regards,
> > > > > Xiaoguang Wang
> > > >
> > > > 1. device sends an interrupt
> > > > 2. device writes a used entry
> > > >
> > > > Will another interrupt be sent, with driver doing nothing at all?
> > > Unless driver enables device's used buffer notification, the device will
> > > not send an interrupt.
> > >
> > > Regards,
> > > Xiaoguang Wang
> > 
> > how about this:
> > 1. device sends an interrupt
> > 2. device writes a used entry
> > 2. driver enables interrupt
> 
> Indeed enabling used buffer notification and sending used buffer notification are matched operations.
> Time-sequence |  driver           |  device
> T1           | enable notification  |
> T2           |                  |  send notification
> T3           |                  |
> T4           | enable notification  |
> T5           |                  |  send notification
> 
> That means once driver enables interrupt, device will ensure that it will send an
> Interrupt if there are used events written. If driver does not enable interrupt at all,
> Device will not send interrupt.
> 
> But yet there is a case to take care, which driver does not handle all used buffer events it received,
> parav explained It would happen before. As I said in this proposal, when driver enables interrupt, it'll
> also send driver's last_used_idx info to device, see below handling:
>   1 driver enable interrupt
>   2 driver submit 5 requests
>   3 all 5 request are completed, device sends interrupt, now device's used_idx is 5.
>   4 driver gets interrupt, and it just handled 3 requests, so last_used_idx is 3.
>   5 driver enables interrupt and attach last_used_idx(3) info.
>   6 device will know that driver does not handle 2 requests and will add these two requests to
>     the number of used ring entries written since last used buffer notification, that means
>     though if there is no new used ring entries written, because of these two outstanding requests,
>     device will still send an interrupt.
> 
> So I still think the deadlock you describe would not happen.
> 
> Regards,
> Xiaoguang Wang

Hmm this is different from event index (which simple triggers when
device is using a buffer at a given index).

We will need to define exactly what is the index sent in the
request and how does device handle it. Here you described
an example, but I don't really know what exactly is expected
generally. So device gets some index, how does it decide whether
to send this immediate notification?


Also looks like this is less "enabling notifications" and more "request
notifications".





> > to
> 
> Regards,
> Xiaoguang Wang
> > 
> > 
> > 
> > > >
> > > >
> > > >
> > > > > >
> > > > > >
> > > > > > --
> > > > > > MST

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

[Jason Wang]

On Fri, Jul 5, 2024 at 3:48 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Fri, Jul 05, 2024 at 01:48:36PM +0800, Jason Wang wrote:
> > On Thu, Jul 4, 2024 at 4:31 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Thu, Jul 04, 2024 at 01:37:44PM +0800, Jason Wang wrote:
> > > > On Wed, Jul 3, 2024 at 5:09 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > >
> > > > > On Wed, Jul 03, 2024 at 04:47:41PM +0800, Jason Wang wrote:
> > > > > > On Wed, Jul 3, 2024 at 4:36 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > > > >
> > > > > > > On Wed, Jul 03, 2024 at 03:59:11PM +0800, Jason Wang wrote:
> > > > > > > > On Wed, Jul 3, 2024 at 3:37 PM Lege Wang <lege.wang@jaguarmicro.com> wrote:
> > > > > > > > >
> > > > > > > > > hi,
> > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > > 1) With the event index, as long as the used index doesn't pass used
> > > > > > > > > > > > events you don't need to fetch even index every time
> > > > > > > > > > > Yeah, I agree VIRTIO_F_EVENT_IDX could help here, but I think it's a relatively
> > > > > > > > > > > crude mechanism, I have two questions below:
> > > > > > > > > > >   1. Used event notification suppression structure is still located in
> > > > > > > > > > >   host memory(in dpu case), I'm not sure whether used_event would
> > > > > > > > > > >   be allowed to update in the running of one virtio device,
> > > > > > > > > >
> > > > > > > > > > What did you mean by "update" here?
> > > > > > > > > I mean "modify".
> > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > >   If it's allowed,
> > > > > > > > > > >   seems devices still need to fetch newest used_event info timely.
> > > > > > > > > >
> > > > > > > > > > It depends on how you define "timely", I mean unless the used event is
> > > > > > > > > > not crossed, you don't need to fetch it from the main memory?
> > > > > > > > > Yes, I got your point here.
> > > > > > > > > >
> > > > > > > > > > But basically, I meant putting used_event in a cap/register other than
> > > > > > > > > > inventing something completely new.
> > > > > > > > > Sorry, I don't get your point here. What does " cap/register " mean, used_event
> > > > > > > > > Is located in main memory, right?
> > > > > > > >
> > > > > > > > I meant something like this.
> > > > > > > >
> > > > > > > > Introduce a capability to allow the driver to duplicate used_event in
> > > > > > > > the register. And say when the feature is negotiated, the driver MUST
> > > > > > > > update both used_event in the memory and the register.
> > > > > > > >
> > > > > > > > Not saying it can work, but we need to know why it can't work like this.
> > > > > > >
> > > > > > > Well I feel if you are proposing a mechanism it's up to you to
> > > > > > > explain how it works without races.
> > > > > >
> > > > > > I agree, that's why I'm saying "Not saying it can work". But what I
> > > > > > meant is really to find a way to reuse the event index instead of
> > > > > > introducing something completely new.
> > > > > >
> > > > > > > The current notification suppression works because the read
> > > > > > > of the notification by the device flushes out used buffer writes by
> > > > > > > the device.
> > > > > >
> > > > > > You meant read after write is ordered by PCI?
> > > > >
> > > > > pci read responses do not bypass writes, yes.
> > > > >
> > > > > > > If you move it to a separate domain (such as the pci bar of the device)
> > > > > > > this no longer holds.
> > > > > >
> > > > > > Would this be implementation specific details or could it be done by PCI?
> > > > >
> > > > > what do you want done by PCI? Generally if things are in one place
> > > > > they are easier to synchronize, if you spread them around you
> > > > > need to synchronize them.
> > > >
> > > > I meant the synchronization looks more like an implementation detail
> > > > in the device.  Synchronizing with device internal logic should be
> > > > simpler than with PCI/memory.
> > > >
> > > > For example, did you mean the synchronization between driver write to
> > > > register (PCI write) and device read from that (internal logic). If
> > > > needed, an implementation needs to serialize those two, then we are
> > > > probably fine.
> > > >
> > > > Thanks
> > >
> > > I mean synchronization between driver write to device and
> > > device write to memory.
> >
> > Not sure I get you, I meant put used_event in a register. Driver will
> > only write to that and the device will only read from that..
>
> But the ring is in driver memory.

Right but which part could be written by both driver and device?

For split virtqueue, avail and descriptor is written by driver. The
used ring is written by device.
For packed virtqueue, avail and used are the same with split and
descriptor ring writes were synchronized with indices.

Or which driver memory writes into its memory needs to be synchronized
with the used_event register? Maybe you can give me an example?

Thanks

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

[Parav Pandit]

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Friday, July 5, 2024 1:55 PM
> To: Parav Pandit <parav@nvidia.com>
> Cc: Lege Wang <lege.wang@jaguarmicro.com>; Jason Wang
> <jasowang@redhat.com>; virtio-comment@lists.linux.dev;
> vattunuru@marvell.com; ndabilpuram@marvell.com; Leo Liu
> <leo.liu@jaguarmicro.com>; Angus Chen <angus.chen@jaguarmicro.com>
> Subject: Re: [PATCH] VIRTIO_F_USED_EVENT_AUTO_DISABLE: add new used
> buffer notification suppression mechanism
> 
> On Fri, Jul 05, 2024 at 04:42:52AM +0000, Parav Pandit wrote:
> > 1. pci capability area (non-extended cap) is full to its capacity. New addition
> can only work in theory.
> > An obvious alternative is to add the extended cap and add there.
> > However, it is not a good idea either as the PCI spec strongly recommends
> to not keep adding vendor specific stuff in the PCI capability section.
> > (even though an option is kept for the vendor capability).
> 
> To stress, we need to solve this problem, even if the specific issue can be
> worked around differently, we keep adding new caps.
> 
Yes.
When the new use case arise, lets understand, whether its need to be discovered before device reset or after device reset.
Depending on use case capabilities can be learnt by different ways.

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

[Parav Pandit]

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Friday, July 5, 2024 1:23 PM
> 
> On Fri, Jul 05, 2024 at 04:42:52AM +0000, Parav Pandit wrote:
> > Hi Lege Wang,
> >
> >
> > > From: Lege Wang <lege.wang@jaguarmicro.com>
> > > Sent: Friday, July 5, 2024 9:05 AM
> > >
> > > hi
> > >
> > > Thanks for your comments in previous discussions.
> > > I'd like to confirm that do you understand this proposal's idea now.
> > > If yes, I may start to prepare a more formal V2.
> > >
> >
> > Notification enablement offset adjacent to current notification offset seems
> a better approach than adding the new capability for following reasons.
> >
> > 1. pci capability area (non-extended cap) is full to its capacity. New addition
> can only work in theory.
> > An obvious alternative is to add the extended cap and add there.
> > However, it is not a good idea either as the PCI spec strongly recommends
> to not keep adding vendor specific stuff in the PCI capability section.
> 
> Can you point me to the relevant section in the spec?
>
Yes, the section of "DEVICE-SPECIFIC REGISTERS IN CONFIGURATION SPACE".
 
> > (even though an option is kept for the vendor capability).
> 
> Interesting. EP guys are also asking about putting the capabilities in a BAR. So
> we might want an option to move all capabilities there.
> 
> > 2. New area adjacent to current offset can further is simple enough to
> utilize.
> > There is no good reason to complicate by another extended cap.
> > If there is one, lets discuss it first.
> 
> We might want to pass more data inline in the notification going forward.
> Also kick and suppression can be handled by different entities, they would
> need to be in different pages.
The kick and suppression are indeed different entities who handled.
I didn't understand the need of different pages. Most nics and storage adapters have them in single page.
Can you please explain why it should be in different page?

> Basically, don't combine unrelated things in one place.
Would like to understand if separate address is enough instead of page.

> 
> --
> MST

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

[Vamsi Krishna Attunuru]

Hi Wang, Michael, Jason,

I wanted to follow up on the previous discussions regarding this new used buffer notification suppression.

Since this proposal involves two changes, how about separating it out and explore the feasibilities to support interrupt coalescing with existing options.?
1) Support pvirtq_event_suppress/virtq_avail in PCI bar
2) Support used buffer notification coalescing

Regards
Vamsi  

>-----Original Message-----
>From: Michael S. Tsirkin <mst@redhat.com>
>Sent: Friday, July 5, 2024 1:31 PM
>To: Lege Wang <lege.wang@jaguarmicro.com>
>Cc: Jason Wang <jasowang@redhat.com>; virtio-comment@lists.linux.dev;
>Vamsi Krishna Attunuru <vattunuru@marvell.com>; Nithin Kumar Dabilpuram
><ndabilpuram@marvell.com>; parav@nvidia.com; Leo Liu
><leo.liu@jaguarmicro.com>; Angus Chen <angus.chen@jaguarmicro.com>
>Subject: [EXTERNAL] Re: [PATCH] VIRTIO_F_USED_EVENT_AUTO_DISABLE:
>add new used buffer notification suppression mechanism
>
>On Fri, Jul 05, 2024 at 03: 35: 17AM +0000, Lege Wang wrote: > hi > > Thanks for
>your comments in previous discussions. > I'd like to confirm that do you
>understand this proposal's idea now. If yes, > I may start to prepare a more
>
>On Fri, Jul 05, 2024 at 03:35:17AM +0000, Lege Wang wrote:
>> hi
>>
>> Thanks for your comments in previous discussions.
>> I'd like to confirm that do you understand this proposal's idea now.
>> If yes, I may start to prepare a more formal V2.
>>
>> Regards,
>> Xiaoguang Wang
>
>It's not that I don't understand the proposal, it's that I don't think it's a good
>proposal. It seems to rely on timeouts to solve race conditions, which are all
>hidden outside of spec.
>We should explain how things are supposed to work.
>
>--
>MST

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

[Jason Wang]

On Wed, Nov 6, 2024 at 12:23 AM Vamsi Krishna Attunuru
<vattunuru@marvell.com> wrote:
>
> Hi Wang, Michael, Jason,
>
> I wanted to follow up on the previous discussions regarding this new used buffer notification suppression.
>
> Since this proposal involves two changes, how about separating it out and explore the feasibilities to support interrupt coalescing with existing options.?
> 1) Support pvirtq_event_suppress/virtq_avail in PCI bar
> 2) Support used buffer notification coalescing
>
> Regards
> Vamsi

Maybe post a new RFC to see?

Thanks

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

[Michael S. Tsirkin]

On Tue, Nov 05, 2024 at 04:23:25PM +0000, Vamsi Krishna Attunuru wrote:
> Hi Wang, Michael, Jason,
> 
> I wanted to follow up on the previous discussions regarding this new used buffer notification suppression.
> 
> Since this proposal involves two changes, how about separating it out and explore the feasibilities to support interrupt coalescing with existing options.?
> 1) Support pvirtq_event_suppress/virtq_avail in PCI bar
> 2) Support used buffer notification coalescing
> 
> Regards
> Vamsi  

Indeed, since there's a lot of controversy, splitting the changes up
might help move this forward.

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

[Vamsi Krishna Attunuru]

>-----Original Message-----
>From: Jason Wang <jasowang@redhat.com>
>Sent: Wednesday, November 6, 2024 10:03 AM
>To: Vamsi Krishna Attunuru <vattunuru@marvell.com>
>Cc: Michael S. Tsirkin <mst@redhat.com>; Lege Wang
><lege.wang@jaguarmicro.com>; virtio-comment@lists.linux.dev; Nithin
>Kumar Dabilpuram <ndabilpuram@marvell.com>; parav@nvidia.com; Leo Liu
><leo.liu@jaguarmicro.com>; Angus Chen <angus.chen@jaguarmicro.com>
>Subject: Re: [EXTERNAL] Re: [PATCH]
>VIRTIO_F_USED_EVENT_AUTO_DISABLE: add new used buffer notification
>suppression mechanism
>
>On Wed, Nov 6, 2024 at 12: 23 AM Vamsi Krishna Attunuru
><vattunuru@ marvell. com> wrote: > > Hi Wang, Michael, Jason, > > I wanted
>to follow up on the previous discussions regarding this new used buffer
>notification suppression. 
>On Wed, Nov 6, 2024 at 12:23 AM Vamsi Krishna Attunuru
><vattunuru@marvell.com> wrote:
>>
>> Hi Wang, Michael, Jason,
>>
>> I wanted to follow up on the previous discussions regarding this new used
>buffer notification suppression.
>>
>> Since this proposal involves two changes, how about separating it out and
>explore the feasibilities to support interrupt coalescing with existing options.?
>> 1) Support pvirtq_event_suppress/virtq_avail in PCI bar
>> 2) Support used buffer notification coalescing
>>
>> Regards
>> Vamsi
>
>Maybe post a new RFC to see?

Sure, thanks.
>
>Thanks