Re: [PATCH v7 2/5] virtio-net: Add flow filter capabilities read commands

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

On Fri, Nov 24, 2023 at 02:57:49AM +0000, Parav Pandit wrote:
> 
> 
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Friday, November 24, 2023 4:28 AM
> > 
> > On Thu, Nov 23, 2023 at 06:40:59PM +0000, Parav Pandit wrote:
> > >
> > >
> > > > From: Michael S. Tsirkin <mst@redhat.com>
> > > > Sent: Thursday, November 23, 2023 7:44 PM
> > > >
> > > > On Thu, Nov 23, 2023 at 11:21:16AM +0200, Parav Pandit wrote:
> > > > > The device responds flow filter capabilities using two commands.
> > > > > One command indicates generic flow filter device limits such as
> > > > > number of flow filters, number of flow filter groups, support or
> > > > > multiple transports etc.
> > > > >
> > > > > The second command indicates supported match types, and fields of
> > > > > the packet.
> > > > >
> > > > > Fixes: https://github.com/oasis-tcs/virtio-spec/issues/179
> > > > > Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
> > > > > Signed-off-by: Parav Pandit <parav@nvidia.com>
> > > >
> > > > So I am still unsure about these commands.  What exactly is the point?
> > > >
> > > Two reasons.
> > > The device reports maximum capabilities that the driver knows upfront. So
> > that it does not need to come to device to know when to fail.
> > 
> > But that requires driver to keep extra state which can easily get out of sync.
> Not sure what you mean by get out of sync from whom?
> Not from the device.

yes from the device. I can't say for sure how because I now see this
patch is implying driver requirements that you didn't document. I
thought the IDs can be any number. It appears that's not what you meant.
I can try and guess what you meant but I'd rather have you rewrite this
in a way that makes the meaning explicit.


> There are many capabilities here each with a different use.

And you are under the impression that dumping all kind of stuff in a
single place is good somehow?

> For many caps, it is to know what is supported/not to decide in the driver.

I can't parse this sentence.

> Certain caps are max to know what is the range of id that driver can use like flow filter id, group id.

Maybe then. But there's apparently no requirement for either device or
driver to put it in this range.

> > For what benefit?
> >
> For functional work.

You don't really explain what kind of work in this patchset, or in the
commit logs. Maybe it's obvious for you but not for everyone.

> > > In future one may want to provision certain max limits that device can have
> > as they eventually will consume certain device hardware resources.
> > 
> > I don't exactly see how this helps provisioning.
> > 
> Provisioning side will set parameters which will reflect these limits to be same on src and dst hypervisor when device migration is used.

But making provisioning depend on driver being fully loaded
and accessing the command is creating a chicken and egg problem.
Provisioning is much more likely to use some new admin commands
over an owner device. So this command is useless for it.



> > 
> > > > Patch 5/5 mandates that device validates all fields already.
> > >
> > > > Are there guests that will actually look at these caps as opposed to
> > > > just sending commands and looking at the return status?
> > > When the device reaches the limit it would not even send the command.
> > 
> > 
> > That's more logic for the driver to implement. Why should it bother when it
> > can just send it?
> >
> It can just send it and fail and flood with error logs.

If the spec says device can fail and that is expected then it won't flood.
If you don't want driver to send some commands you should say that
instead of saying device should filter them.

> Also it cannot just send some random id based numbers that device does not support.

Aha. I think I'm beginning to guess. You wrote:
	field{max_groups} indicates total number of flow filter groups supported
	by the device whose group identifiers can be any value in the range from 0 to
	\field{max_groups - 1}.
and you think this implies that it can't be out of that range.
That's not how logic works though ;)


> > > For example if the device supports only one group, it wont implement
> > ethtool ntuple and will chose only arfs as one example.
> > 
> > Sounds like a contrived example why does device even expose flow steering
> > then?
> It is not contrived at all.
> 
> A device may be support one or multiple groups. Each group in the OS has multiple users.
> One group is enough for either ARFS or ethool but not both.
> Two groups can allow ARFS and ethtool to co-exists.
> 3 groups will allow in future to expose some queues to user apps.

Oh I misunderstood.
Then why would driver decide to force a decision to ARFS
specifically?
It will likely leave this to the user.

-- 
MST