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: Monday, November 27, 2023 5:10 PM
> 
> On Mon, Nov 27, 2023 at 11:33:45AM +0000, Parav Pandit wrote:
> >
> > > From: Michael S. Tsirkin <mst@redhat.com>
> > > Sent: Monday, November 27, 2023 4:53 PM
> > >
> > > On Mon, Nov 27, 2023 at 10:19:47AM +0000, Parav Pandit wrote:
> > > > > > > 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
> > >
> > > Now I think I don't understand what this array does at all.
> > > Why are there multiple entries with each entry also including a mask
> > > of multiple fields?
> > >
> > Each entry indicates a bitmask of supported fields that can be filtered.
> > The type indicates how to interpret each fields_map.
> > Any new type for MPLS can be added easily by defining its type and mask.
> > And same type enums and field mask also used by the data path too.
> 
> Oh wait I think I begin to remember now. There are 12 bits defined so far and
> you made it this huge array. Right? With code to parse and format it all. Just
> add a 64 bit field and it will last us long enough. No parsing and formatting just
> #define.
The infrastructure defines 12 bits only because this is first minimal viable work.
There will be more fields to be added for more types.
It is not good to mix up all the fields and bit definitions.

And there are per device max limits which is way more than 12 bits.

Your attitude to take narrow view and trying to kill this command is not good either.
> 
> 
> > >
> > > > >
> > > > >
> > > > > > >
> > > > > > > > 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.
> > >
> > > What we do not need is both driver and device checking these values.
> > >
> > Driver is mainly to using to know its valid ranges and to fail the commands
> early.
> > Device is referring it for any validations that it may need to do.
> >
> > >
> > > > > 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.
> > >
> > > yes. capability check is initialization time though.
> > >
> > Right, for runtime config like this cap, it is not in the initialization time
> needed.
> >
> > >
> > > > >
> > > > > > >
> > > > > > >
> > > > > > > > >
> > > > > > > > > > > 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.
> > >
> > > No, your sentence does not say it must. It says it can be in range.
> > > Can it be out of range, e.g. 5? You don't state either way.
> > I understand. I will that normative in v7.
> >
> > > The second sentence says it can not be out of range so 5 is illegal.
> > > Are there illegal values inside the range? E.g. might 3 be illegal?
> > > According to second sentence, maybe. According to 1st sentence, 3 is legal.
> > >
> > > > It can be any value.
> > >
> > > :(
> > >
> > >
> > > > In device and driver requirements, I will write the explicit line
> > > > that it MUST be
> > > in this range.
> > >
> > > Then it will be different.
> > Will write as two different normative respective for each section.
> > For driver it is SHOULD, for device it is MUST wherever applicable.
> >
> > >
> > > > >
> > > > >
> > > > > > 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.
> > >
> > > I don't know what that means.
> > >
> > Packet processing order is defined by the group.
> > You should have participated in Aug or before when all of these were
> discussed.
> > Anyways, next time you can participate in other features.
> >
> > > > > 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.
> > >
> > > Then won't driver expect inifinite # of groups to be supported?
> > >
> > A cheaper device can place anything it wants.
> >
> > > > The check in the device will be done as it uses such an id for its
> > > > own
> > > bookkeeping.
> > > >
> > > I don't know what that means.
> > > > > --
> > > > > MST