Re: RE: RE: RE: [PATCH v2] virtio-net: improve description of default coalescing parameters

Re: RE: RE: RE: [PATCH v2] virtio-net: improve description of default coalescing parameters

[Heng Qi]

On Tue, 14 May 2024 13:09:32 +0000, Parav Pandit <parav@nvidia.com> wrote:
> 
> > From: Heng Qi <hengqi@linux.alibaba.com>
> > Sent: Tuesday, May 14, 2024 1:23 PM
> > 
> > On Tue, 14 May 2024 07:48:55 +0000, Parav Pandit <parav@nvidia.com>
> > wrote:
> > >
> > >
> > > > From: Heng Qi <hengqi@linux.alibaba.com>
> > > > Sent: Tuesday, May 14, 2024 11:23 AM
> > > >
> > > > On Tue, 14 May 2024 03:59:52 +0000, Parav Pandit <parav@nvidia.com>
> > > > wrote:
> > > > > Hi Heng,
> > > > >
> > > > >
> > > > > > From: Heng Qi <hengqi@linux.alibaba.com>
> > > > > > Sent: Monday, May 13, 2024 6:27 PM
> > > > > >
> > > > > > Currently, a device may initialize each vq's coalescing
> > > > > > parameters to empirically non-zero values, so the description of
> > > > > > this part is supplemented for the virtio spec.
> > > > > >
> > > > > Currently the spec says to initialize the vq's coalescing parameters to zero.
> > > > >
> > > > > Spec snippet: " Upon reset, a device MUST initialize all
> > > > > coalescing
> > > > parameters to 0."
> > > > >
> > > > > So I didn't understand why you say "current the device may initialize".
> > > > >
> > > > > Do you mean, you want to change the spec to say, by default the
> > > > > device has
> > > > chosen to set zero or non-zero default parameter as it likes.
> > > > > (and not zero).
> > > >
> > > > Yes.
> > > >
> > > > If a device supports interrupt coalescing moderation, it usually has
> > > > a non- zero default value to provide better performance.
> > > When VIRTIO_NET_F_VQ_NOTF_COAL or VIRTIO_NET_F_NOTF_COAL is not
> > negotiated, the device can apply any notification coalescing value.
> > > (assuming EVENT_IDX is off).
> > >
> > > But when they above features are negotiated, it is expected that
> > > driver is going to driver it,
> > 
> > The driver drives the runtime parameters without conflicting with the initial
> > default values.
> > And most users/drivers without dim enable may not drive it.
> > 
> There is no conflict in runtime or default parameters.
> For driver it starts after 10msec or 4 minutes, both are runtime parameters.
> 
> > The device need to be defensive about default performance.
> > 
> The device can surely offer the default performance with non zero default values.

That's what I'm mentioning.

> 
> > >in such case what is the good motivation to start with some arbitrary value?
> > >
> > > > I'm trying to fix this, and
> > > > we shouldn't force the device to always initialize with 0.
> > > >
> > > If driver is in control, driver decides what values to set.
> > > And hence, arbitrary value is
> > 
> > Then maybe the driver is never set and the device uses a 0 which messes up
> > most data scenarios.
> > 
> If driver does not want to set it, why did it enable the feature in first place?
> So asking again, is it because driver enabled featured too early in the driver load sequence, and not DIM is also disabled?

I don't see the connection.

> 
> If feature is negotiated, than DIM can be enabled by default, and therefore there is no need for the defaults?

Right. But if DIM is not enabled (pls remember that DIM is just one of the
application scenarios of VQ_NOTF_COAL, right?), the user can query the initial
value of the device, which may be any value.

> 
> Or you are saying, that enabling DIM as default has some issue, and until that point you prefer to have some default in the device?

Partly true. The complete purpose is that the device can have any default values
(0 or non-zero).

> 
> > Cloud vendors cannot do this. And I don’t seem to see any interference with
> > other devices from this proposal?
> > If the device does not want a non-zero value, a value of 0 can be used as the
> > default value.
> > 
> I didn’t understand above.
> 
> > >
> > > > Especially when the DIM is disabled from the enabled state, allowing
> > > > the driver to restore default parameters for device will prevent
> > > > occasional performance degradation.
> > > >
> > > When DIM is disabled, do you mean driver does not have any good defaults?
> > 
> > DIM is not guaranteed to have a friendly value when disabled.
> > 
> I didn’t follow. 
> 
> (a) When DIM is enabled, DIM decides the value.
> (b) When DIM is disabled, zero is not good default in the device. Did I understand right?
> 
> I agree with (a) and (b).

Right.

> 
> > > I guess this issue surfaces from the feature negotiation limitation that, it
> > must be done early enough before DRIVER_OK.
> > > And DIM is still disabled in driver.
> > 
> > We lack the capability filed of default coalescing parameters, so GET is used to
> > alleviate this.
> > 
> When GET is not supported, ethtool should get -ENOSUPP instead of 0.

We don't have a separate feature bit for GET, right? So if VQ_NOTF_COAL is
negotiated, the device supports the GET command.

> 
> > >
> > > That explains.
> > >
> > > Commit log needs to explain this limitation and the gain by relaxing it to be
> > non-zero.
> > >
> > > Any reason DIM is disabled by default in the driver?
> > > If VQ_NOTF_COAL is supported by device, DIM should be enabled by
> > default.
> > 
> > virtio-net support for netdim needs to be optimized. Of course, I also found
> > that other network cards also have optimization points.
> > 
> > I'm trying to enable DIM for upstream by default, after I've done the profile list
> > tuning etc.
> > 
> > >
> > > > >
> > > > > > Since the VIRTIO_NET_F_NOTF_COAL feature does not provide a
> > > > > > related GET command or config field, we still consider its default values
> > to be 0.
> > > > > >
> > > > > The behavior for VIRTIO_NET_F_NOTF_COAL and
> > > > VIRTIO_NET_F_VQ_NOTF_COAL can stay same for the default value.
> > > > > Can you please explain the motivation for running different
> > > > > default values
> > > > for two different features in this commit log?
> > > >
> > > > The current spec leaves coalescing parameters for both features at 0.
> > > >
> > > > F_VQ_NOTF_COAL provides a GET command, that is, non-zero 0 can be
> > > > obtained by the driver from the device, but F_NOTF_COAL does not
> > > > have a similar GET command.
> > > >
> > > GET command doesn't give the ability to define the defaults.
> > > So both can have non zero defaults.
> > 
> > Makes sense.
> > 
> > >
> > > > So when vq reset occurs or dim is disabled from the enabled state,
> > > > the driver with F_VQ_NOTF_COAL negotiated has the ability to restore
> > > > non-zero values, but only when F_NOTF_COAL is negotiated, the driver
> > > > has no path to obtain the non-zero value, let alone restore non-zero
> > values.
> > > >
> > > When F_NOTF_COAL is negotiated, if the device has its non zero defaults,
> > why does driver need to restore anything?
> > 
> > Yes, device will set these.
> >
> Ok. so no restore needed.
>  
> > > > >
> > > > > > Suggested-by: Jason Wang <jasowang@redhat.com>
> > > > > > Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
> > > > > > ---
> > > > > > v1->v2:
> > > > > >   - Update description @Jason.
> > > > > >
> > > > > >  device-types/net/description.tex | 22 +++++++++++++++-------
> > > > > >  1 file changed, 15 insertions(+), 7 deletions(-)
> > > > > >
> > > > > > diff --git a/device-types/net/description.tex b/device-
> > > > > > types/net/description.tex index 61cce1f..cf4b12c 100644
> > > > > > --- a/device-types/net/description.tex
> > > > > > +++ b/device-types/net/description.tex
> > > > > > @@ -1805,6 +1805,10 @@ \subsubsection{Control
> > > > > > Virtqueue}\label{sec:Device Types / Network Device / Devi
> > > > > >
> > > > > >  The device may generate notifications more or less frequently
> > > > > > than specified by set commands of the VIRTIO_NET_CTRL_NOTF_COAL
> > class.
> > > > > >
> > > > > > +If the VIRTIO_NET_F_VQ_NOTF_COAL feature is offered, the device
> > > > > > +may initialize the coalescing parameters for each transmit or
> > > > > > +receive virtqueue to non-zero values, otherwise, to 0, which
> > > > > > +are called default
> > > > > > coalescing parameters.
> > > > > > +
> > > > > Instead of 'offered', it should be 'negotiated'. Because if may be
> > > > > offered,
> > > > but driver may not have enabled it.
> > > >
> > > > I do not think so.
> > > >
> > > > Even if the driver does not negotiate, as long as the device has the
> > > > ability to adjust coalescing parameters (i.e. offered), the device
> > > > will initialize any values.
> > > >
> > > The device can initialize coalescing parameters regardless of VQ_NOTF_COAL
> > offered or not.
> > >
> > 
> > Right.
> > 
> > >
> > > > >
> > > > > >  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:
> > > > > > @@ -1873,6 +1877,10 @@ \subsubsection{Control
> > > > > > Virtqueue}\label{sec:Device Types / Network Device / Devi
> > > > > >
> > > > > >  The driver MUST ignore the values of coalescing parameters
> > > > > > received from the VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET command if
> > > > > > the device responds with VIRTIO_NET_ERR.
> > > > > >
> > > > > > +If the VIRTIO_NET_F_VQ_NOTF_COAL feature is negotiated, the
> > > > > > +driver
> > > > > > MUST
> > > > > > +get coalescing parameters for each enabled transmit or receive
> > > > > > +virtqueue through the VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET
> > > > command
> > > > > > after a successful device reset.
> > > > > > +
> > > > > The driver is free to not call GET call at all. It can always just do set.
> > > > > So it cannot be must requirement for the driver to get them.
> > > >
> > > > This is mainly forced to MUST during the probe phase.
> > > > Other timings, the existing spec has already described it clearly.
> > > >
> > > I don't see it is needed to force driver to read.
> > > Why is it must for the driver to read it? It can operate without read.
> > 
> > Otherwise the device has a non-zero default value, but users using ethtool -c
> > see a value of 0.
> > 
> When GET is supported, ethtool callback should query the current value.
> Driver can read it once at start time, but it is not a MUST requirement.
> Driver can read it runtime too. It is the driver implementation choice.
> We don’t need to tell in the spec, when driver MUST read it.

I agree with this.

> 
> > >
> > > > >
> > > > > The wording should be, the driver MAY get ...
> > > > >
> > > > > >  \devicenormative{\subparagraph}{Notifications
> > > > > > Coalescing}{Device Types / Network Device / Device Operation /
> > > > > > Control Virtqueue / Notifications Coalescing}
> > > > > >
> > > > > >  The device MUST ignore \field{reserved}.
> > > > > > @@ -1884,18 +1892,18 @@ \subsubsection{Control
> > > > > > Virtqueue}\label{sec:Device Types / Network Device / Devi  The
> > > > > > device MUST respond to VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET and
> > > > > > VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET commands with
> > > > 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.
> > > > > > -
> > > > > > -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 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 MUST set default coalescing parameters for
> > > > > > +all transmit or receive virtqueues.
> > > > > > +
> > > > > The device MAY set parameters to zero or non zero values..
> > > >
> > > > Ok.
> > > >
> > > > > I am still missing the motivation part of why to set non zero
> > > > > value, even if
> > > > the driver has negotiated the feature.
> > > >
> > > > Please see the above description.
> > > >
> > > If I understood is right, the case is:
> > > _VQ_NOTF_COAL is negotiated, and DIM is disabled.
> > > And device is unable to apply some good non zero defaults.
> > 
> > 
> > If _VQ_NOTF_COAL is negotiated, and DIM is disabled:
> > 
> > 1. The device wants to set a non-zero value for good perf,
> >    but the user sees a value of 0.
> > 
> The only limitation that we want to remove is, on device reset, the device may have any default value.

Right, and the driver may query the value using the GET command.

> 
> > 2. When the user has not actively modified the value, DIM switches from the
> >    enabled state to the disabled state, then the driver expects to issue a
> >    setting command with a default value. To avoid performance degradation
> >    caused by a bad value.
> > 
> This #2 seems like some hack. Should I read the Linux code in this area?

No. This is some kind of performance optimization at the code level. When DIM is
turned off, the driver configures default values for the device to avoid any big
performance regression.

> 
> > We assume that most users do not have the prerequisite knowledge to modify
> > coalecing parameters.
> > 
> Right. Hence the DIM should be used.
> When DIM is not used, a non zero default is good to have.
> (without forcing the driver to read it).

+1

Thanks.

> 
> > Thanks.
> > 
> > >
> > > Right?
> > >
> > >
> > > > Thanks.
> > > >
> > > > >
> > > > > > +Regardless of whether the coalescing parameters have been
> > > > > > +overridden by set commands from the
> > VIRTIO_NET_CTRL_NOTF_COAL
> > > > > > +class, upon
> > > > > > disabling
> > > > > > +and re-enabling a transmit or receive virtqueue, the device
> > > > > > +MUST set the previous parameters for the virtqueue.
> > > > > >
> > > > > >  \paragraph{Device Statistics}\label{sec:Device Types / Network
> > > > > > Device / Device Operation / Control Virtqueue / Device
> > > > > > Statistics}
> > > > > >
> > > > > > --
> > > > > > 2.32.0.3.g01195cf9f
> > > > >
>