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

Parav Pandit
2024-06-10T15:12:13Z

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Monday, June 10, 2024 8:20 PM
>
> On Mon, Jun 10, 2024 at 09:35:45PM +0800, Heng Qi wrote:
> > On Mon, 10 Jun 2024 14:46:02 +0200, Halil Pasic <pasic@linux.ibm.com>
> wrote:
> > > On Sat, 8 Jun 2024 10:34:22 +0800
> > > Heng Qi <hengqi@linux.alibaba.com> wrote:
> > >
> > > > On Fri, 7 Jun 2024 22:02:46 +0200, Halil Pasic <pasic@linux.ibm.com>
> wrote:
> > > > > On Tue, 28 May 2024 12:47:02 +0800 Heng Qi
> > > > > <hengqi@linux.alibaba.com> 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.
> > > > >
> > > > > Sorry I'm late to the party -- again! Just for my understanding:
> > > > > how/why is this a clarification and not just a (basically incompatible)
> change?
> > > >
> > > > In my opinion, "clarification" means that something may have been
> > > > described incorrectly before, and we now need to discuss, explain
> > > > clearly, and correct the possibly incorrect description.
> > > >
> > >
> > > I figure the difference in perceived semantics of the word
> > > "clarification" is at the root of my confusion. Let us have a look
> > > at
> > > https://dictionary.cambridge.org/de/worterbuch/englisch/clarificatio
> > > n
> > >
> > > According to my understanding a "clarification", while an
> > > improvement in ease of understanding and/or decrease of ambiguity
> > > (possibly to no ambiguity at all) implies that what receiving a
> > > clarification is not outright wrong.
> > >
> > > When rectifying something that is outright incorrect or wrong, I
> > > would refer to that with words like "correction", "fix", "erratum"
> > > or "corrigendum".
> > >
> > > > >
> > > > > I mean if I read this correctly, before the driver had the
> > > > > guaranty that if the parameters are not set by the driver,
> > > > > negotiating the feature does not introduce any coalescing. After
> > > > > this in theory the device could just pick some max value and
> > > > > potentially introduce maximal latency in certain scenarios.
> > > >
> > > > "maximum latency" also means "throughput improvement".
> > > >
> > >
> > > Under certain assumptions. But not necessarily. Again my concern is
> > > mostly the type of change. The virtio standard maintain a revision
> > > history appendix, and I would like to avoid the nature of this
> > > change being misrepresented there. If Connie and/or Michael think it
> > > is worth fixing, I believe it can be fixed with an editorial change.
> > >
> > > AFAIU VIRTIO_NET_F_NOTF_COAL and VIRTIO_NET_F_VQ_NOTF_COAL
> are about
> > > to land with virtio-1.3, i.e. there is no released/standardized
> > > virtio version where the "initialize to 0" is released. In that
> > > sense it looks like we are still on time to change this. But I am
> > > not 100% certain. In any case I don't think this as a huge impact
> > > and I'm fine going ahead with the change.
> >
> > Sorry for the late reply, I'm on vacation.
> >
> > I agree with this, and I prefer to release this patch as a correction
> > for
> > virtio1.3 instead of a new patch for virtio1.4, because if devices
> > support moderation coalescing based on virtio1.3, and after virtio1.4
> > is released, these devices need to be updated again for a more reasonable
> coalescing parameters.

Heng's patch is bringing only the relaxation to the device that it can have nonzero value on reset.

I don't have strong opinion either way.
Either approach looks fine to me as it does not seem to break any functionality for driver and device.
It only improves the performance when driver has not set them.

'Clarify' to 'correct' or 'fix' subject line would have been obviously better as Pasic Halil mentioned, but probably while merging the patch, it can be corrected.

> >
> > Cornelia and Michael, what do you think?
>
> The TC is just voting to start the public review process.
> We can defer that by a couple more weeks if there is a known issue to address.
>
> For that I expect we want a final patch and a couple of acks on list from TC
> members by end of the ballot, June 13.
>
>
>
> > >
> > > > >
> > > > > I understand that it is probably in the best interest of the
> > > > > devices to not pick stupid defaults. But it is also probably in
> > > > > the best interest of the driver to set those params, and if the
> > > > > driver is going to set its values, the devices defaults are moot
> > > > > unless we assume that those may end up being used by the driver
> > > > > as a hint when deciding which parameter values to choose.
> > > >
> > > > "Any values" is compatible with "0 for max-usecs and max-frames",
> > > > and the device can choose "no coalescing".
> > > >
> > > > "No coalescing" means "latency friendly", but it also means "a lot
> > > > of interruptions and throughput unfriendly".
> > >
> > > To what extent does virtio's "normal" notification suppression
> > > (VIRTIO_F_EVENT_IDX) would alleviate that in practice? At least in
> > > theory the interruption suppression could save us there right?
> > >
> >
> > Interruption suppression is useful, but many scenes still have a lot
> > of interruptions.
> >
> > >
> > > > If the device chooses a stupid
> > > > maximum value, it is his choice (spec should give more devices
> > > > choices instead of forcing them to choose "0" which is not the
> > > > best practice). We can't talk about performance for drivers when the
> devices tend to choose any "stupid" designs.
> > > >
> > > > We need relaxe the restrictions and makes the spec more reasonable.
> > > >
> > >
> > > Hm, I see Linux virtio-net changes have landed with v6.0 and if I
> > > read those correctly the driver -- contrary to my initial
> > > expectation -- negotiates the feature, but does not set the
> > > parameters explicitly and
> >
> > How does the driver know what parameters to set? The parameters should
> > be exposed by each device.
> >
> > > thus keeps the defaults (until userspace decides to set the parameters).
> > > So it does matter whether the defaults are guaranteed to be 0 or
> > > not, and if not it does matter what defaults are chosen by the device.
> >
> > Didn't follow this. More below.
> >
> > >
> > > One could even argue that those patches have been reviewed under the
> > > assumption that the device needs to use 0 as the default parameter value.
> >
> > The default value should not be explicitly specified in the spec,
> > because one size does not fit multiple devices. The source of this
> > problem is that we are missing fields like default_{rx,
> > tx}_coalesicng_params that indicate the device capabilities. No?
> >
> > Thanks.
> >
> > >
> > > Well no strong opinions here. If the community is fine with it, I'm
> > > fine with it as well.
> > >
> > > Thank you!
> > >
> > > Regards,
> > > Halil
>