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

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

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.


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