Re: [PATCH net,v2] net: flow_offload: skip hw stats check for FLOW_ACTION_HW_STATS_DONT_CARE

[Jakub Kicinski <kuba@kernel.org>]

On Tue, 5 May 2020 21:31:45 +0200 Pablo Neira Ayuso wrote:
> On Tue, May 05, 2020 at 11:40:10AM -0700, Jakub Kicinski wrote:
> > On Tue,  5 May 2020 19:47:36 +0200 Pablo Neira Ayuso wrote:  
> > > This patch adds FLOW_ACTION_HW_STATS_DONT_CARE which tells the driver
> > > that the frontend does not need counters, this hw stats type request
> > > never fails. The FLOW_ACTION_HW_STATS_DISABLED type explicitly requests
> > > the driver to disable the stats, however, if the driver cannot disable
> > > counters, it bails out.
> > > 
> > > TCA_ACT_HW_STATS_* maintains the 1:1 mapping with FLOW_ACTION_HW_STATS_*
> > > except by disabled which is mapped to FLOW_ACTION_HW_STATS_DISABLED
> > > (this is 0 in tc). Add tc_act_hw_stats() to perform the mapping between
> > > TCA_ACT_HW_STATS_* and FLOW_ACTION_HW_STATS_*.
> > > 
> > > Fixes: 319a1d19471e ("flow_offload: check for basic action hw stats type")
> > > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> > > ---
> > > v2: define FLOW_ACTION_HW_STATS_DISABLED at the end of the enumeration
> > >     as Jiri suggested. Keep the 1:1 mapping between TCA_ACT_HW_STATS_*
> > >     and FLOW_ACTION_HW_STATS_* except by the disabled case.
> > > 
> > >  include/net/flow_offload.h |  9 ++++++++-
> > >  net/sched/cls_api.c        | 14 ++++++++++++--
> > >  2 files changed, 20 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
> > > index 3619c6acf60f..efc8350b42fb 100644
> > > --- a/include/net/flow_offload.h
> > > +++ b/include/net/flow_offload.h
> > > @@ -166,15 +166,18 @@ enum flow_action_mangle_base {
> > >  enum flow_action_hw_stats_bit {
> > >  	FLOW_ACTION_HW_STATS_IMMEDIATE_BIT,
> > >  	FLOW_ACTION_HW_STATS_DELAYED_BIT,
> > > +	FLOW_ACTION_HW_STATS_DISABLED_BIT,
> > >  };
> > >  
> > >  enum flow_action_hw_stats {
> > > -	FLOW_ACTION_HW_STATS_DISABLED = 0,
> > > +	FLOW_ACTION_HW_STATS_DONT_CARE = 0,  
> > 
> > Why not ~0? Or ANY | DISABLED? 
> > Otherwise you may confuse drivers which check bit by bit.  
> 
> I'm confused, you agreed with this behaviour:

I was expecting the 0 to be exposed at UAPI level, and then kernel
would translate that to a full mask internally.

From the other reply:

> I can send a v3 to handle the _DONT_CARE type from the mlxsw.

Seems a little unnecessary for all drivers to cater to the special
case, when we made the argument be a bitfield specifically so that 
the drivers can function as long as they match on any of the bits.