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