Re: [PATCH v2 1/6] notifications: unify notifications wording in core

[Halil Pasic <pasic@linux.ibm.com>]

On 06/13/2018 03:04 PM, Stefan Hajnoczi wrote:
> On Mon, Jun 11, 2018 at 06:11:53PM +0200, Halil Pasic wrote:
> > @@ -65,11 +65,12 @@ in which their processing is complete.
> >   
> >   The Device Event Suppression data structure is write-only by the
> >   device. It includes information for reducing the number of
> > -device events - i.e. driver notifications to device.
> > +device events - i.e. sending an available buffer notification
> > +to the device.
> >   
> >   The Driver Event Suppression data structure is read-only by the
> >   device. It includes information for reducing the number of
> > -driver events - i.e. device interrupts to driver.
> > +driver events - i.e. send a used buffer notification to the driver.
>
> This makes the "i.e." more natural:
>
> "sending fewer used buffer notifications to the driver"
>
> because the "send a used buffer notification to the driver" phrase
> doesn't directly relate to any preceding subphrase ("driver events",
> "number of driver events", or "reducing the number of driver events").

Will do. My sentence does not make sense.

> >   
> >   \subsection{Driver and Device Ring Wrap Counters}
> >   \label{sec:Packed Virtqueues / Driver and Device Ring Wrap Counters}
> > @@ -309,22 +310,20 @@ in the ring.
> >   
> >   \subsection{Driver and Device Event Suppression}
> >   \label{sec:Packed Virtqueues / Driver and Device Event Suppression}
> > -In many systems driver and device notifications involve
> > +In many systems used and available buffer notifications involve
> >   significant overhead. To mitigate this overhead,
> >   each virtqueue includes two identical structures used for
> >   controlling notifications between the device and the driver.
> >   
> >   The Driver Event Suppression structure is read-only by the
> > -device and controls the events sent by the device
> > -to the driver (e.g. interrupts).
> > +device and controls the used buffer notifications (sent by the device
> > +to the driver).
> >   
> >   The Device Event Suppression structure is read-only by
> > -the driver and controls the events sent by the driver
> > -to the device (e.g. IO).
> > +the driver and controls the available buffer notifications (sent by the
> > +driver to the device).
> >   
> > -Each of these Event Suppression structures controls
> > -both Descriptor Ring events and structure events, and
> > -each includes the following fields:
> > +Each of these Event Suppression includes the following fields:
>
> Something is missing, "these" is plural but Event Suppression is
> singular:
>
> Each of these Event Suppression structures ...

Will put back structures. Connie spotted it too.

> >   Otherwise, if the VIRTIO_F_EVENT_IDX feature bit is negotiated:
> >   \begin{itemize}
> >   \item The driver MUST set \field{flags} to 0.
> > -\item The driver MAY use \field{used_event} to advise the device that interrupts are unnecessary until the device writes entry with an index specified by \field{used_event} into the used ring (equivalently, until \field{idx} in the
> > +\item The driver MAY use \field{used_event} to advise the device that
> > +notifications are unnecessary until the device writes entry with an index specified by \field{used_event} into the used ring (equivalently, until \field{idx} in the
>
> Pre-existing issue:
>
>    the device writes *an* entry

I will fix it if already touching the sentence. Thanks
for spotting it!

> > @@ -562,8 +570,8 @@ The driver offers buffers to one of the device's virtqueues as follows:
> >   \item The driver performs a suitable memory barrier to ensure that it updates
> >     the \field{idx} field before checking for notification suppression.
> >   
> > -\item If notifications are not suppressed, the driver notifies the device
> > -    of the new available buffers.
> > +\item The driver sends an available buffers notification to the device if
> > +    such notifications are not suppressed.
>
> For consistency:
>
> s/available buffers notification/available buffer notification/

Sorry, I missed this one. Will fix.

Thanks for your review!

Regards,
Halil