RE: [PATCH v11] virtio-net: support the virtqueue coalescing moderation

[Parav Pandit <parav@nvidia.com>]

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Thursday, March 9, 2023 2:19 PM
> 
> On Thu, Mar 09, 2023 at 01:53:37PM -0500, Parav Pandit wrote:
> >
> > > Network Device / Device Operation / Control Virtqueue /
> > > Notifications Coalescing} -If the VIRTIO_NET_F_NOTF_COAL feature is
> negotiated, the driver can -send control commands for dynamically changing
> the coalescing parameters.
> > > +If the VIRTIO_NET_F_NOTF_COAL feature is negotiated, a driver can
> > > +send
> > Above text had "the driver".
> > Better to not change it.
> 
> I don't see why it matters. In most other cases we omit it.

The hunk that changes article from "the" to "a" doesn't belong to this patch.

> > > +Upon disabling and re-enabling the transmit virtqueue, the device
> > > +will set the coalescing parameters of the virtqueue to those configured
> through the VIRTIO_NET_CTRL_NOTF_COAL_TX_SET command, or to 0 if the
> command did not set any TX coalescing parameters.
> > > +
> > s/if the command/if the driver/
> 
> please don't just tell people what to do. provide reasoning.
> 
Reason I think is: 
The command is not the object or the actor. 
The device and driver are the object or actor setting the parameters through the command.

> +command, or, if the command
> > s/if the command/if the driver/
> > > +did not configure TX/RX coalescing parameters, to 0.
> >
> > Also the sentence structure should be:
> > set params to value A configured using method1, else to params B if
> > not configured.
> >
> > Instead currently it is,
> > set params to value A using method1, else not configured to param B.
> 
> I don't see anything wrong with the proposed structure, and the way you want it
> with if after else is IMHO weird and hard to read.
> 
> 
> 
> > So to keep if-else pattern:
> >
> > the device MUST set coalescing parameters of the virtqueue to those
> > configured using the
> >
> VIRTIO_NET_CTRL_NOTF_COAL_TX_SET/VIRTIO_NET_CTRL_NOTF_COAL_RX_S
> ET
> > command, or to 0 if the driver did not configure TX/RX coalescing
> parameters.
> >
> > Just like how you wrote it in the non normative places. :)
> >
> 
> maybe fix non-normative too. If  A then B is simple and clear.
> B if A is understandable but is harder to read.
> 
Documenting one way is needed and without exact duplication at two places.