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

Parav Pandit
2024-06-27T13:04:05Z

> 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.

> 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?

> 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.

> > 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.