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

Lege Wang
2024-07-03T06:52:11Z

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