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

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

On Tue, Jun 11, 2024 at 06:40:33PM +0800, Heng Qi wrote:
> On Mon, 10 Jun 2024 22:19:00 +0200, Halil Pasic <pasic@linux.ibm.com> wrote:
> > On Mon, 10 Jun 2024 21:35:45 +0800
> > Heng Qi <hengqi@linux.alibaba.com> wrote:
> > 
> > > On Mon, 10 Jun 2024 14:46:02 +0200, Halil Pasic <pasic@linux.ibm.com> wrote:
> > [..]
> > 
> > > > > 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? 
> > 
> > Disclaimer: I'm not very knowledgeable when it comes to networking and
> > NICs. Please be patient with me.
> > 
> > If I understood that properly have previously stated that it is basically
> > a trade-off between "latency friendly"  (downside: overhead and not so
> > throughput friendly) and "throughput friendly" (downside: not so latency
> > friendly). And that makes sense to me.
> > 
> 
> Yes, I am correcting the description about the trade-off thing.

It does not look like this thing is ready yet.


How we can we make progress with
the realease but sure we don't make backwards compat a pain?

Ideas?



> > So I would think, the answer to the question what is the best trade-off
> > should also depend on the workload.
> 
> device-side dim or driver-side dim to solve this problem. But, when the device
> is reset, the driver may not enable dim, the device or driver needs to  have a
> static coalescing parameters (0 or non-zero) for the trade-off.
> 
> (don't trust dim too much now, I'm doing some updates to optimize dim.)
> 
> 
> > 
> > Now as far as I understand although we call the parameters max_usecs and 
> > max_packets the notification condition is dictated by those two values. I.e.
> > there won't be a notification unless the compound condition is met.
> 
> Right, just one of the conditions needs to be met.
> 
> > 
> > When there is no traffic 0 and 0 looks like reasonable values to me: I
> > want the first of possibly many packets ASAP. Depending on the actual
> > load, maybe one could employ some sort of a heuristics to keep good
> > balance -- maybe based on a frequency of interrupts. Maybe DIM is
> > actually exactly what I have in mind.
> 
> This patch is not to solve the scenario where dim exists, but what the
> static value of the coalescing parameter is when the device is reset.
> (please check some hhistorical discussion of this topic)
> 
> > 
> > You seem more knowledgeable on the topic. How is this usually done? How
> > are the optimal values correlated with device characteristics?
> 
> 
> NICs such as mlx, ena, ice, and HiSilicon all have a non-zero static
> coalescing value. Although they all support netdim, the static value is
> still useful when dim is disabled.
> 
> As for the specific value, I think it is best to notify the driver through the
> device itself, such as adding capability fileds or this:
> https://lore.kernel.org/all/20240426065441.120710-3-hengqi@linux.alibaba.com/
> 
> > 
> > > The parameters should be
> > > exposed by each device.
> > 
> > I would like to better understand why. My intention is not to question
> > the correctness of this statement, but to gain a better understanding.
> > 
> > > 
> > > > 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.
> > >
> > 
> > Maybe my statement was wrong. So let me make a question out of it. What
> > entity or entities do we expect to change the parameter values, and when
> > or under which conditions do we expect them to change the parameter
> > values?
> 
> Changed by the driver when the load changes or user..
> 
> >  
> > > > 
> > > > 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.
> > 
> > This ties in to my previous question about the relationship between
> > device characteristics and the optimal values for max_usecs and
> > max_packets.
> 
> Device characteristics and optimal max_usecs/frames are preferably communicated
> via device capability, but at least should not be forced to a specific value by
> the driver.
> 
> > 
> > > The source of this problem is that we are
> > > missing fields like default_{rx, tx}_coalesicng_params that indicate the device
> > > capabilities. No?
> > 
> > I don't think so! In my understanding is that with your proposal
> > after a reset, the OS/driver has no problem obtaining the defaults that
> > indicate the device capabilities. But I would really like to better
> > understand that device capabilities part.
> > 
> > And yes another way around this would have been say:
> > * let us introduce those defaults fields, e.g. to the config space
> 
> Do you want to solve it by adding another new feature?
> Otherwise, how do you solve the compatibility problem?
> 
> > * make the driver read those values as a part of the initialization
> 
> The spec is in conflict with this, because the driver can only read 0 from
> devices that conform to the spec. I submitted this patch to achieve this goal:
> https://lore.kernel.org/all/20240426065441.120710-3-hengqi@linux.alibaba.com/
> 
> > * and set those values as the initial parameters.
> 
> I don't support this, the device doesn't want to accept any initial
> unreasonable settings.
> 
> Thanks.
> 
> > But frankly I see no benefit of that over what you propose here.
> > 
> > Regards,
> > Halil