[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Tue, 5 May 2020 12:43:43 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Pablo Neira Ayuso <pablo@...filter.org>
Cc: netfilter-devel@...r.kernel.org, davem@...emloft.net,
netdev@...r.kernel.org, jiri@...nulli.us, ecree@...arflare.com
Subject: Re: [PATCH net,v2] net: flow_offload: skip hw stats check for
FLOW_ACTION_HW_STATS_DONT_CARE
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@...filter.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.
Powered by blists - more mailing lists