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

[Saeed Mahameed]

On Wed, 2019-04-24 at 09:01 +0200, Maxime Chevallier wrote:
> 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)
> 

 * For %ETHTOOL_GRXCLSRLCNT, @rule_cnt is set to the number of defined
 * rules on return.  If @data is non-zero on return then it is the
 * size of the rule table, plus the flag % if the
 * driver supports any special location values.  If that flag is not
 * set in @data then special location values should not be used.

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.

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.

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

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

> Thanks again for the review,
> 
> Maxime