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

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

On Thu, Jun 27, 2024 at 01:03:51PM +0000, Parav Pandit wrote:
> 
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Thursday, June 27, 2024 6:23 PM
> > 
> > On Thu, Jun 27, 2024 at 12:45:49PM +0000, Parav Pandit wrote:
> > > Hi Michael,
> > >
> > > > From: Michael S. Tsirkin <mst@redhat.com>
> > > > Sent: Thursday, June 27, 2024 6:05 PM
> > > >
> > > > On Thu, Jun 27, 2024 at 12:37:32PM +0200, Halil Pasic wrote:
> > > > > On Tue, 25 Jun 2024 18:14:15 -0700 Si-Wei Liu
> > > > > <si-wei.liu@oracle.com> wrote:
> > > > >
> > > > > > On 6/24/2024 10:56 PM, Parav Pandit wrote:
> > > > > [..]
> > > > > > > I saw the need of this proposal slightly differently in the
> > > > > > > discussion with
> > > > Heng in v4.
> > > > > > > The way I understood is, proposed relaxation enables below
> > > > > > > Linux driver
> > > > flow to work as equally as without device offering
> > > > VIRTIO_NET_F_VQ_NOTF_COAL.
> > > > > > >
> > > > > > > Flow is:
> > > > > > > 1. The device offered feature VIRTIO_NET_F_VQ_NOTF_COAL 2. The
> > > > > > > virtio-net driver negotiated VIRTNET_FEATURES that has
> > > > > > > VIRTIO_NET_F_VQ_NOTF_COAL
> > > > > > >
> > > > > > > 3. Because VIRTIO_NET_F_VQ_NOTF_COAL is negotiated, device is
> > > > > > > not
> > > > applying any coalescing on the VQ, in a good hope that driver will
> > > > perform VQ notification coalescing.
> > > > >
> > > > > I have certainly understood this differently. When
> > > > > VIRTIO_NET_F_VQ_NOTF_COAL is not negotiated then the device is not
> > > > supposed/allowed to do any interrupt coalescing (notification
> > > > suppression may still apply).
> > > > >
> > > > > If VIRTIO_NET_F_VQ_NOTF_COAL is negotiated the device is supposed
> > > > > to/MUST do the coalescing according to the parameters as described
> > > > > by the virtio spec.
> > > > >
> > > > > Michael, Jason: Can you guys weigh in on this?
> > > >
> > > > I still don't understand why this change is needed.
> > > > We have this text:
> > > >
> > > > 	The device may generate notifications more or less frequently than
> > > > 	specified by set commands of the VIRTIO_NET_CTRL_NOTF_COAL
> > class.
> > > >
> > > > and
> > > >
> > > > 	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.
> > > >
> > > > So with no spec changes, devices already can do what this patch says
> > > > they can do - send notifications less frequently.
> > > >
> > > > Re-reading this spec text, maybe the confusion is that it mentions
> > > > set commands specifically? And it's also stuck in the middle where
> > > > it's easy to miss.
> > > >
> > > > So it would seem that the following should be sufficient, and it
> > > > looks like a small clarification we could just apply and include in
> > > > the vote for the csd. What do you guys think?
> > > >
> > > >
> > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > >
> > > > ----
> > > >
> > > >
> > > > diff --git a/device-types/net/description.tex b/device-
> > > > types/net/description.tex index 76585b0..d6788df 100644
> > > > --- a/device-types/net/description.tex
> > > > +++ b/device-types/net/description.tex
> > > > @@ -1711,8 +1711,6 @@ \subsubsection{Control
> > > > Virtqueue}\label{sec:Device Types / Network Device / Devi
> > > >                                          for an enabled
> > > > transmit/receive virtqueue whose index is \field{vq_index}.
> > > >  \end{enumerate}
> > > >
> > > > -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  virtqueue, regardless of the
> > > > command used to set the parameters. Use the following command
> > > > sequence  with two pairs of virtqueues as an example:
> > > > @@ -1726,6 +1724,9 @@ \subsubsection{Control
> > > > Virtqueue}\label{sec:Device Types / Network Device / Devi  \item
> > Command6:
> > > > VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET with \field{vq_index} = 1, the
> > > > device responds with coalescing parameters of index 1 set by command5.
> > > >  \end{itemize}
> > > >
> > > > +The device can generate notifications more or less frequently than
> > > > +specified by the coalescing parameters.
> > > > +
> > > >  \subparagraph{Operation}\label{sec:Device Types / Network Device /
> > > > Device Operation / Control Virtqueue / Notifications Coalescing /
> > > > Operation}
> > > >
> > > >  The device sends a used buffer notification once the notification
> > > > conditions are met and if the notifications are not suppressed as
> > > > explained in \ref{sec:Basic Facilities of a Virtio Device /
> > > > Virtqueues / Used Buffer Notification Suppression}.
> > > > @@ -1798,7 +1799,7 @@ \subsubsection{Control
> > > > Virtqueue}\label{sec:Device Types / Network Device / Devi  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.
> > > >
> > > > -The behavior of the device in response to set commands of the
> > > > VIRTIO_NET_CTRL_NOTF_COAL class is best-effort:
> > > > +The behavior of the device in response to specific coalescing
> > > > +parameters 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.
> > > >
> > >
> > > Your changes above are good and they are applicable when the specific
> > coalescing parameters are set by the driver.
> > 
> > What makes you say this?
> Because I was reading this in the CVQ section of the spec where CVQ commands are setting it.


So? Reset also sets it.

> > No they apply universally whenever parameters are specified by the spec:
> > whether parameters are set by driver or by device.
> I see. Upon reset they are set to 0 in the below text, means no coalescing, means quick interrupts, right?

But device can make them as slow as it likes, spec says so.


> > Is that unclear somehow?
> > I suspect you are just reading the wrong part of the patch ...
> > 
> > 
> > > The issue that is being resolved is, when these parameters are NOT set by the
> > driver, when feature is negotiated.
> > > What should device do?
> > > The parameters are not specified...
> > 
> > They are specified - set to 0.
> > 
> > > The spec says " Upon reset, a device MUST initialize all coalescing parameters
> > to 0.".
> > 
> > yes. and device can then coalesce if it wants to.
> > 
> You mean 0 means coalescing is disabled (and not quick notifications) ?
> And hence, it is device chosen value?
> If so, that interpretation would work too.

No 0 means send notification after any # of packets.


> > > And Heng attempt is to clarify that by adding a changing above line
> > > something like,
> > >
> > > "when driver does not set coalescing params, but have negotiated the
> > feature, device may generate notifications more or less frequently than
> > specified".
> > 
> > 
> > Maybe that is the intention, but the proposed text is instead saying the
> > parameters themselves are !0. 
> A clarification like above would be better or 0 means device chosen parameters would also work fine.


> > Which is what triggered the whole mess with a
> > misvote: commit log says it's a clarification when it is actually a behaviour
> > change.
> > 
> It is a behavior change to fix/restore.
> But if you suggest: 0 means device chosen value, that will work too to restore current regression.