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

[Parav Pandit <parav@nvidia.com>]

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Friday, November 24, 2023 11:29 AM
> 
> 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 cannot be any number. The device requirement has captured it.
Couldn't see a lot of point of duplicating it.
The device requirement implies what driver has to do.
But if you insist, I can duplicate in the driver requirement too.

> 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.
> 
If you insist, I can duplicate in driver requirements too.
> 
> > 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?
It is not dumped.
It is structured based on the functionality.
The config space proposal tends to dump everything in unstructured way at single place that cannot extended elegantly.

For example, Xuan's dynamic array cannot be placed next to other dynamic array of flow filter.

> 
> > For many caps, it is to know what is supported/not to decide in the driver.
> 
> I can't parse this sentence.
> 
The driver checks things upfront what is supported while serving the command, instead of random trial and error guess work game with he device.

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

I will add this requirement statements in device requirements and that will make things clear.
Good point.

> > > 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.
What text are you expecting?
We have undergone all the requirements study that you conveniently choose to not participate for 2 months.

> 
> > > > 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.
I didn't explain well likely.
Provisioning as you described, will be on owner device.
This command tells driver what is provisioned.

> Provisioning is much more likely to use some new admin commands over an
> owner device. So this command is useless for it.
> 
Provisioning owner device will use exact same structure while provisioning.

> 
> 
> > >
> > > > > 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.
I will add the normative in the driver requirement section.

> 
> > 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 ;)
> 
Which logic?
The driver will honor the limit given to him.
The device will also naturally honor what it published to driver.

> 
> > > > 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.
It is the driver implementation choice,
For example: 
if there is only one group, driver can choose to give strong preference, say for ARFS only.

Or it can let it be on first come first usage, for example, if ethtool wants to use first, it will allow it, not letting ARFS to work.