Re: [PATCH net-next v2] net: mvpp2: cls: Add Classification offload support

[Maxime Chevallier]

Hello Saeed,

On Wed, 24 Apr 2019 18:05:51 +0000
Saeed Mahameed <saeedm@xxxxxxxxxxxx> 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