Re: [PATCH v5] virtio-net: clarify coalescing parameters settings

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

On Mon, Jun 17, 2024 at 10:35:38AM +0800, Heng Qi wrote:
> On Tue, 11 Jun 2024 19:03:11 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > On Tue, May 28, 2024 at 12:47:02PM +0800, Heng Qi wrote:
> > > The device can set any initial coalescing parameters (0 or non-zero)
> > > for the receive/send queue before the setting command is executed,
> > > not just 0, enhancing device performance even without DIM enabled.
> > > 
> > > So we need to clarify descriptions that don't fit the behavior.
> > > 
> > > Fixes: https://github.com/oasis-tcs/virtio-spec/issues/194
> > > Suggested-by: Jason Wang <jasowang@redhat.com>
> > > Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
> > > Reviewed-by: Parav Pandit <parav@nvidia.com>
> > > Reviewed-by: Jason Wang <jasowang@redhat.com>
> > > ---
> > > v4->v5:
> > >   - Add RB tags from Parav and Jason. Thanks!
> > > 
> > > v3->v4:
> > >   - Doesn't force the device to remember more stuff. @Parav
> > > 
> > > v2->v3:
> > >   - Clarify description to be more generic. @Parav
> > > 
> > > v1->v2:
> > >   - Update description. @Jason
> > > 
> > >  device-types/net/description.tex | 14 +++++++++++---
> > >  1 file changed, 11 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/device-types/net/description.tex b/device-types/net/description.tex
> > > index 61cce1f..00c1b36 100644
> > > --- a/device-types/net/description.tex
> > > +++ b/device-types/net/description.tex
> > > @@ -1803,6 +1803,14 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
> > >                                          for an enabled transmit/receive virtqueue whose index is \field{vq_index}.
> > >  \end{enumerate}
> > >  
> > > +If the VIRTIO_NET_F_NOTF_COAL or VIRTIO_NET_F_VQ_NOTF_COAL feature is negotiated,
> > > +the device may apply any coalescing parameters to each transmit/receive virtqueue
> > > +before the driver successfully performs one of the VIRTIO_NET_CTRL_NOTF_COAL set commands.
> > 
> > may outside of conformance section, not good.
> 
> This is outside of the conformance section. Do you want inside?

needs to be rephrased to avoid using reserved keywords.

> > 
> > > +
> > > +The driver can query the coalescing parameters of any enabled transmit/receive
> > > +virtqueue using the VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET command, before or after any
> > > +VIRTIO_NET_CTRL_NOTF_COAL set command is done.
> > > +
> > 
> > Vague. There are just 3 set commands ust list them.
> > 
> > >  The device may generate notifications more or less frequently than specified by set commands of the VIRTIO_NET_CTRL_NOTF_COAL class.
> > >  
> > >  If coalescing parameters are being set, the device applies the last coalescing parameters set for a
> > 
> > This needs more work but I guess we can skip the above part.
> > 
> > > @@ -1885,17 +1893,17 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
> > >  VIRTIO_NET_ERR if the designated virtqueue is not an enabled transmit or receive virtqueue.
> > >  
> > >  Upon disabling and re-enabling a transmit virtqueue, the device MUST set the coalescing parameters of the virtqueue
> > > -to those configured through the VIRTIO_NET_CTRL_NOTF_COAL_TX_SET command, or, if the driver did not set any TX coalescing parameters, to 0.
> > > +to those configured through the VIRTIO_NET_CTRL_NOTF_COAL_TX_SET command, or, if the driver did not set any TX coalescing parameters, the device MAY initialize them to any values.
> > >  
> > 
> > If device does not initialize them then what?
> 
> The device's customized behavior, at least, does not conform to the spec.

Then why do you say "may"?


> > Actually something like 
> > 0 -> the default TX coalescing parameters chosen by the device?
> 
> Deefault coalescing parameters should also have a concrete "value".

and I just proposed how to say this in a clear way.


> > 
> > 
> > >  Upon disabling and re-enabling a receive virtqueue, the device MUST set the coalescing parameters of the virtqueue
> > > -to those configured through the VIRTIO_NET_CTRL_NOTF_COAL_RX_SET command, or, if the driver did not set any RX coalescing parameters, to 0.
> > > +to those configured through the VIRTIO_NET_CTRL_NOTF_COAL_RX_SET command, or, if the driver did not set any RX coalescing parameters, the device MAY initialize them to any values.
> > >  
> > >  The behavior of the device in response to set commands of the VIRTIO_NET_CTRL_NOTF_COAL class is best-effort:
> > >  the device MAY generate notifications more or less frequently than specified.
> > >  
> > >  A device SHOULD NOT send used buffer notifications to the driver if the notifications are suppressed, even if the notification conditions are met.
> > >  
> > > -Upon reset, a device MUST initialize all coalescing parameters to 0.
> > > +Upon reset, a device MAY set any coalescing parameters for all transmit and receive virtqueues.
> > 
> > Confusing.
> > This seems to allow any vq to have a different set of parameters.
> 
> YES.
> 
> > Now if driver calls VIRTIO_NET_CTRL_NOTF_COAL_RX_SET then what?
> > 
> 
> Then the coalescing parameters of all RX queues are updated to the values
> set by VIRTIO_NET_CTRL_NOTF_COAL_RX_SET, and the coalescing parameters of all
> TX queues continue to retain the previous values.
> 
> Thanks.

It's not at all nice that there's apparently no way for driver to
get back to the default values.


> > 
> > >  
> > >  \paragraph{Device Statistics}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Device Statistics}
> > 
> > 
> > 
> > 
> > >  
> > > -- 
> > > 2.32.0.3.g01195cf9f
> >