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

Parav Pandit
2024-06-27T11:28:02Z

Hi Halil,

> From: Halil Pasic <pasic@linux.ibm.com>
> Sent: Thursday, June 27, 2024 4:08 PM
>
> 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 device does not do interrupt coalescing, it will be worse because of interrupt per packet.

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

> Michael, Jason: Can you guys weigh in on this?
>
> > >
> > > 4. the virtio-net driver even after negotiating
> VIRTIO_NET_F_VQ_NOTF_COAL, still kept the its dim disabled.
> > > vi->rq[queue].dim_enabled = false by default.
> > >
> > > 5. The virtio net driver enables dim only on user request via ethtool.
> > >
> > > So here, just because one is using a new driver and new device, it may get
> lower performance.
> > Impossible, older driver / device doesn't have coalescing advertise /
> > enabled, and new device & new driver with coalescing negotiated starts
> > safe with 0 with coalescing disabled effectively, how can it get lower
> > performance?
>
> Si-Wei and Parav, I think it is the other way around. Without the change
> proposed here, with no user/management software intervention assumed,
> we are guaranteed to not see any performance regression but also no
> performance benefit because of the coalescing mechanism, because with
> zeros as default it is essentially dormant.
>
I will avoid quoting the term "performance regression".
I believe there will be behavior regression.
For example, devices do apply notification suppression today when VIRTIO_NET_F_VQ_NOTF_COAL is not negotiated.
Not doing so will not allow virtio-net to come this far.

By setting them to zero, device stops applying what it was doing until now.
And it results in "behavior regression".
One can argue that now the latency is best because very first packet will trigger a notification.

The proposed change in my humble view is allowing the device to restore its behavior when driver has NOT configured notification params and yet negotiated VIRTIO_NET_F_VQ_NOTF_COAL.

> With the change proposed here however, we open up the possibility to see
> either a performance improvement or a performance degradation depending
> on the defaults chosen by the device and the workload (sill assuming new
> device & new driver & no intervention by management software).
>
Exactly. I agree with you.
It seems fine because it restores the behavior. Whether that behavior is good/bad/average - it is upto the device implementation.

>
> >
> > >
> > > Do we agree to above problem description?
> > This narrative has two problems:
> >
> > - The term "lower performance" is misleading and inaccurate, the fact
> > or implication of coalescing is that it yields worse latency, causing
> > additional delay and jitter that would hurt quite a lot of real world
> > workload that is latency sensitive.  So, rather than say coalescing
> > helps performance, it should clearly tell the truth that the
> > coalescing parameter is no more than a tuning knob that has
> > performance impact/implication, being positive or negative it is very
> > subject to the guest workload. So the default disposition or initial
> > parameters loaded from device just reflects the preference of the host
> > admin/ device owner/ cloud vendor of one's own, for e.g. it is
> > optimized for throughput oriented load versus latency oriented. Be
> > noted, generally end users don't care what coalescing is about, they
> > do not connect performance to "throughput" or "bandwidth" as that in your
> mind.
> >
>
> Si-Wei, I sympathies with your argument but I'm willing to tolerate this change
> nevertheless.
>
> Yes the device and via the device whoever controls these parameters can
> shoot the guest in the foot, if the guest admin is not going out of his way to do
> something about controlling coalescing.
>
Right.

> But via the same mechanism we could end up in a better place as well.
>
Right if not better, at least it brings to the same place as where "this feature is not offered".

In the second example for txq, current Linux dim does not have netdim for TX. And negotiating this feature (and not doing actual TXQ level moderation) results in zero tx moderation.

All of these would have been fine, if the Linux net driver would have implemented and enabled netdim whenever the feature is negotiated.

Driver negotiating and driver not controlling is the main issue.
The device is eagerly hoping that driver will control, but it doesn't.

The proposed change overcomes this limitation, that until driver controls, it let it control the moderation.
Once driver takes over (by issuing first command), device stops applying its own logic.

> Because I assume that the device and its "admin" are at least trying to do
> good, I'm not straight out against this proposal, as long nothing wonky
> happens with the code which is already out there, in a sense of a split brain
> scenario where Linux thinks those values are 0 but they aren't. Unfortunately
> can't tell you how we stand on this.
>
> [..]
> > > If no, I likely missed something in the long discussion and need to
> > > read all of it. :(
> > >
> > > If yes, few solutions are:
> > > 1. A user intervention is must to avoid this regression. The user in the guest
> VM must enable DIM if the device supports it.
>
> I don't think burdening up the users with avoiding regressions is a good idea.
>
The user is the vnet driver who doesnt even has netdim implemented for the TX.
So device is in dark, should it offer this feature or not?
If it offers (without the proposed change), there is behavior regression until the driver fix is done.

Hence, I agree to Halil's point.
We should relax the requirement to restore the behavior.
New feature bit won't help the behavior restoration..

> Regards,
> Halil
>
> > > Very hard to do this plumbing.
> > >
> > > 2. A net driver enables the DIM by default as long as
> VIRTIO_NET_F_VQ_NOTF_COAL is negotiated.
> > > (without ethtool setting)
> > >
> > > 3. A device relaxes the limitation and continue to apply the coalescing, until
> driver overrides it.
> > >
> > > In my humble opinion, Heng is solving it using option #3, that tends to
> work with existing and future drivers who may/may not enable the DIM.
> > >
> > >
> > >
> > >
> >