Messages in this thread | | | Date | Thu, 25 Apr 2019 09:12:22 +0200 | From | Maxime Chevallier <> | Subject | Re: [PATCH net-next v2] net: mvpp2: cls: Add Classification offload support |
| |
Hello Saeed,
On Wed, 24 Apr 2019 18:05:51 +0000 Saeed Mahameed <saeedm@mellanox.com> wrote:
>Maybe ethtool doesn't do anything with the return value, but if the >user is not using any special flag, then the interpretation should be >absolute location/ID as provided by the user, see below scenario >example > >> 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. >> > >why ? even with your hw clear class separation, user can use any loc >for udp4 and tcp4 or any flow for that matter, in case they won't >overlap. > >And in case they do overlap, then I think you must have a global >location scheme! take this scenario for instance: > >scenario 1: >loc 0 ip4 action 2 >loc 1 udp4 action -1 >loc 2 tcp4 action -1 > >This should result of all udp4, tcp4, and ip4 traffic to go to rx ring >2, even if the user asked to drop udp/tcp4. once rule at location 0 is >deleted then udp/tcp4 traffic will be dropped. > >scenario 2: >loc 0 udp4 action -1 >loc 1 tcp4 action -1 >loc 2 ip4 action 2 > >should result in dropping all upd4/tcp4 but allow receiving ip4 on ring >4. > >User doesn't see and should not see your hw tables scheme, i feel that >for scenario 1 your implementation will drop udp4 and tcp4 since they >will be separated from ip4 rule at loc 0, which is not what the user >expects, please correct me if i am wrong.
You're correct, this is what's going to happen with the current implementation.
>that being said, i think you should keep the global location scheme at >least from user perspective and respect the prioritization of the user >inserted rules especially when there are overlapping. > >even if there is no overlapping, location could mean: priorities rules >at lower locations in hw processing so they can get higher performance.
Ok, I'll have to rework the design of the tables a bit to be compliant with this, but this is achievable.
I'll make sure to CC you on next revisions.
>> Maybe in this case I should stick with insertions thay rely on >> (such as "first", "last", "any") and have a scheme >> where priorisation is based strictly on the rule insertion order ? >> > >Sure for when the special flags are set, but you will have to report >RX_CLS_LOC_SPECIAL on ETHTOOL_GRXCLSRLCNT. > >also if you don't want to support the global location scheme then >return -EOPNOTSUPP/-EINVAL when user specifies a non special location >?
Given your review, I'll keep support for the global location scheme.
>> > 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. >> > >hmm, i think all driver should be aligned and provide same behavior, at >least for the non special flag use case, >vendors must report -EOPNOTSUPPORT if a specific use case operation is >not supported.
I agree, I'll however document what the limitations are in terms of supported features, etc.
Thanks for the clarifications, it really helps a lot.
Maxime
| |