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 3:45 PM
> 
> On Fri, Nov 24, 2023 at 06:27:34AM +0000, Parav Pandit wrote:
> >
> > > 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.
> 
> Which requirement?
> 
This is the one I replied before that I will add as explicitly in device and driver requirements section.
I can see a disconnect for you.

> > Couldn't see a lot of point of duplicating it.
> > The device requirement implies what driver has to do.
> 
> Nothing of the sort.
> 
> > But if you insist, I can duplicate in the driver requirement too.
> 
> I think spec should list the requirements we have not imply them.
> In particular driver authors should be able to go over driver conformance
> sections, use non-normative sections to interpret them and then verify their
> conformance.
> Device normative sections is not something that should interest them.
> We don't always do a good job of this - not a reason to keep adding to that
> problem.
> 
Ok. Sounds fine. I will add the driver requirements for max limits.

> > > 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.
> 
> I don't know what "dynamic array" is exactly but generally, Any kind of a big
> array is not appropriate in config space - I don't see any of these here, though.
Ah, now I see the disconnect.

There is.
struct virtio_net_ctrl_ff_match_types

> 
> 
> > >
> > > > 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.
> 
> There's no real guesswork with these specific commands: user types an
> ethtool command, driver sends it on, it either succeeds or fails.
It is a guess work without driver knowing if its supported or not.

> However if you feel it important to do checks in driver - OK.
Yes, because there are many parsing fields and good to know.

> But that makes cap query an init time thing.
Yes, caps does not change dynamically.
But sure it is different among different member devices and owner too.

> 
> 
> > > > 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.
> 
> Yea ... I'm sorry. Things have been going on here, man.
> I'm glad I'm back and able to participate though.
> 
Great.

> > >
> > > > > > 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.
> 
> So the advantage of using config space is that we can have a single generic
> provision command that gets device config space format. Your approach will
> need some kind of net device specific thing.
> 

I see the advantage. But it very small and largely hidden beneath the sw layers how a provisioning command sets things.
A provisioning command sets the value, it surfaces at different level.

Multiple device vendors want to follow the design pattern to use " most uses it is better to use a virtqueue to
update configuration information"

Instead of RO = config space, RW = cvq.

The more pragmatic design pattern is:
driver initialization time => feature bit + virtio config space.
Driver runtime => cvq.

> 
> > >
> > >
> > > > >
> > > > > > > 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 aristotelian logic we commonly use.  You say "group identifiers can be any
> value in the range".  You seem to mean "group identifiers can not be any value
> not in the range". These two statements are not equivalent and neither implies
> the other.
I fail to see the difference between the two English sentence you wrote.
Can you please have an example, how second is any different?

In my example max_groups = 5, so group id must be from 0 to 4.
It can be any value.

In device and driver requirements, I will write the explicit line that it MUST be in this range.

> 
> 
> > 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.
> >
> 
> Practically, drivers are highly unlikely to make the choice.
It will make the choice when there is only one group available.

> That is why it is reasonable not to have checks in the driver if device is doing
> them anyway.
The groups particularly have the priority as well. So the check in device does not work.

> But OTOH if you want driver to do the checks then I don't see why we should
> also add them to device unless there's a good reason to.
> Less checks in devices -> cheaper simpler devices.
> 
The cheaper devices can always place UINT_32 max values and ignore any checking etc.

The check in the device will be done as it uses such an id for its own bookkeeping.

> --
> MST