Messages in this thread | | | Date | Wed, 24 Apr 2019 09:01:23 +0200 | From | Maxime Chevallier <> | Subject | Re: [PATCH net-next v2] net: mvpp2: cls: Add Classification offload support |
| |
Hello Saeed,
Thanks for the review,
>> When inserting a rule in a given flow, the location given is relative >> to >> the flow : >> >> ethtool -N eth0 flow-type udp4 dst-port 1234 action 2 loc 0 >> >> ethtool -N eth0 flow-type tcp4 dst-port 1234 action 3 loc 0 >> >> However when removing a rule, the global location is to be used. This >> location can be retrieved by using ethtool -n <interface>. >> > >I am not sure what you mean by "the location given is relative to the >flow", it seems like the rule will end up in a different location than >the user intended, but looking at ethtool documentation it clearly says >that the location the user provides is an absolute rule id/location, >which will be used to delete this rule. > >from "man ethtool": >loc N: >Specify the location/ID to insert the rule. This will overwrite any >rule present in that location and will not go through any of the rule >ordering process. > >delete N >Deletes the RX classification rule with the given ID.
I was unsure about this, so I'm glad you commented. One thing that made me think what I did could be okay is that the documentation for ETHTOOL_SRXCLSRLINS in ethtool.h says :
"For %ETHTOOL_SRXCLSRLINS, @fs specifies the rule to add or update. @fs.@location either specifies the location to use or is a special location value with %RX_CLS_LOC_SPECIAL flag set. On return, @fs.@location is the actual rule location."
I interpreted the "On return [...]" part as if we could rewrite the location if needed when inserting a rule (although it seems ethtool doesn't do anything with this return value)
The point for doing so is that we have a clear separation in our classification tables between different traffic classes, so we have a range of entries for tcp4, one for udp4, one for tcp6, etc.
Having a "global" location numbering scheme would, I think, also be confusing, since it would make the user use loc 0->7 for tcp4, loc 8->15 for udp4 and so on.
Maybe in this case I should stick with insertions thay rely on RX_CLS_LOC_SPECIAL (such as "first", "last", "any") and have a scheme where priorisation is based strictly on the rule insertion order ?
>So the above example should result in one flow rule in your hardware. >but according the code below the calculated index in >mvpp2_ethtool_cls_rule_ins might end up different than the requested >location, which will confuse the user.
I'm also working on writing a proper documentation for this driver, including the behaviour of the classifier implementation, hopefully this would help.
Thanks again for the review,
Maxime
| |