Message ID | 2cf9024d-1568-4594-5763-6c4e4e8fe47b@solarflare.com |
---|---|
State | Superseded |
Delegated to: | David Miller |
Headers | show |
Series | [v3,net-next] net: flow_offload: simplify hw stats check handling | expand |
On 20/05/2020 18:21, Edward Cree wrote: > @@ -582,7 +590,7 @@ nf_flow_offload_rule_alloc(struct net *net, > const struct flow_offload_tuple *tuple; > struct nf_flow_rule *flow_rule; > struct dst_entry *other_dst; > - int err = -ENOMEM; > + int err = -ENOMEM, i; > > flow_rule = kzalloc(sizeof(*flow_rule), GFP_KERNEL); > if (!flow_rule) Whoops, this changebar isn't meant to be there. Somehow I missed the unused var warning when I built it, too. Drop this, I'll spin v4.
On Wed, May 20, 2020 at 06:31:05PM +0100, Edward Cree wrote: > On 20/05/2020 18:21, Edward Cree wrote: > > @@ -582,7 +590,7 @@ nf_flow_offload_rule_alloc(struct net *net, > > const struct flow_offload_tuple *tuple; > > struct nf_flow_rule *flow_rule; > > struct dst_entry *other_dst; > > - int err = -ENOMEM; > > + int err = -ENOMEM, i; > > > > flow_rule = kzalloc(sizeof(*flow_rule), GFP_KERNEL); > > if (!flow_rule) > Whoops, this changebar isn't meant to be there. Somehow I missed > the unused var warning when I built it, too. > Drop this, I'll spin v4. The nf_tables_offload.c update is missing, please include this in v4.
On 20/05/2020 18:32, Pablo Neira Ayuso wrote: > On Wed, May 20, 2020 at 06:31:05PM +0100, Edward Cree wrote: >> On 20/05/2020 18:21, Edward Cree wrote: >>> @@ -582,7 +590,7 @@ nf_flow_offload_rule_alloc(struct net *net, >>> const struct flow_offload_tuple *tuple; >>> struct nf_flow_rule *flow_rule; >>> struct dst_entry *other_dst; >>> - int err = -ENOMEM; >>> + int err = -ENOMEM, i; >>> >>> flow_rule = kzalloc(sizeof(*flow_rule), GFP_KERNEL); >>> if (!flow_rule) >> Whoops, this changebar isn't meant to be there. Somehow I missed >> the unused var warning when I built it, too. >> Drop this, I'll spin v4. > The nf_tables_offload.c update is missing, please include this in v4. Hmm. Rather than me trying to whack-a-mole all the places in netfilter that create actions... given that the other user is TC, which explicitly sets hw_stats, maybe I should instead make flow_rule_alloc() populate all the hw_stats with DONT_CARE? It certainly makes for a shorter patch, and makes it less likely that bugs will be introduced later when new action offloads get added to netfilter by forgetting to set hw_stats. And it means the patch doesn't rely on me knowing things about netfilter internals which I apparently don't. -ed
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c index b286fe158820..51e1b3930c56 100644 --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c @@ -30,14 +30,14 @@ static int mlxsw_sp_flower_parse_actions(struct mlxsw_sp *mlxsw_sp, return -EOPNOTSUPP; act = flow_action_first_entry_get(flow_action); - if (act->hw_stats == FLOW_ACTION_HW_STATS_ANY || - act->hw_stats == FLOW_ACTION_HW_STATS_IMMEDIATE) { + if (act->hw_stats & FLOW_ACTION_HW_STATS_DISABLED) { + /* Nothing to do */ + } else if (act->hw_stats & FLOW_ACTION_HW_STATS_IMMEDIATE) { /* Count action is inserted first */ err = mlxsw_sp_acl_rulei_act_count(mlxsw_sp, rulei, extack); if (err) return err; - } else if (act->hw_stats != FLOW_ACTION_HW_STATS_DISABLED && - act->hw_stats != FLOW_ACTION_HW_STATS_DONT_CARE) { + } else { NL_SET_ERR_MSG_MOD(extack, "Unsupported action HW stats type"); return -EOPNOTSUPP; } diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h index 4001ffb04f0d..95d633785ef9 100644 --- a/include/net/flow_offload.h +++ b/include/net/flow_offload.h @@ -168,10 +168,11 @@ enum flow_action_hw_stats_bit { FLOW_ACTION_HW_STATS_IMMEDIATE_BIT, FLOW_ACTION_HW_STATS_DELAYED_BIT, FLOW_ACTION_HW_STATS_DISABLED_BIT, + + FLOW_ACTION_HW_STATS_NUM_BITS }; enum flow_action_hw_stats { - FLOW_ACTION_HW_STATS_DONT_CARE = 0, FLOW_ACTION_HW_STATS_IMMEDIATE = BIT(FLOW_ACTION_HW_STATS_IMMEDIATE_BIT), FLOW_ACTION_HW_STATS_DELAYED = BIT(FLOW_ACTION_HW_STATS_DELAYED_BIT), @@ -179,6 +180,7 @@ enum flow_action_hw_stats { FLOW_ACTION_HW_STATS_DELAYED, FLOW_ACTION_HW_STATS_DISABLED = BIT(FLOW_ACTION_HW_STATS_DISABLED_BIT), + FLOW_ACTION_HW_STATS_DONT_CARE = BIT(FLOW_ACTION_HW_STATS_NUM_BITS) - 1, }; typedef void (*action_destr)(void *priv); @@ -340,11 +342,12 @@ __flow_action_hw_stats_check(const struct flow_action *action, return false; action_entry = flow_action_first_entry_get(action); - if (action_entry->hw_stats == FLOW_ACTION_HW_STATS_DONT_CARE) - return true; + + /* Zero is not a legal value for hw_stats, catch anyone passing it */ + WARN_ON_ONCE(!action_entry->hw_stats); if (!check_allow_bit && - action_entry->hw_stats != FLOW_ACTION_HW_STATS_ANY) { + ~action_entry->hw_stats & FLOW_ACTION_HW_STATS_ANY) { NL_SET_ERR_MSG_MOD(extack, "Driver supports only default HW stats type \"any\""); return false; } else if (check_allow_bit && diff --git a/net/netfilter/nf_flow_table_offload.c b/net/netfilter/nf_flow_table_offload.c index 2ff4087007a6..60f94a2d15cc 100644 --- a/net/netfilter/nf_flow_table_offload.c +++ b/net/netfilter/nf_flow_table_offload.c @@ -165,9 +165,17 @@ static void flow_offload_mangle(struct flow_action_entry *entry, static inline struct flow_action_entry * flow_action_entry_next(struct nf_flow_rule *flow_rule) { - int i = flow_rule->rule->action.num_entries++; + struct flow_action *acts = &flow_rule->rule->action; + struct flow_action_entry *act; + int i = acts->num_entries++; - return &flow_rule->rule->action.entries[i]; + act = acts->entries + i; + /* Pre-fill action hw_stats with DONT_CARE. Caller can override this + * if it wants stats for its action + */ + act->hw_stats = FLOW_ACTION_HW_STATS_DONT_CARE; + + return act; } static int flow_offload_eth_src(struct net *net, @@ -582,7 +590,7 @@ nf_flow_offload_rule_alloc(struct net *net, const struct flow_offload_tuple *tuple; struct nf_flow_rule *flow_rule; struct dst_entry *other_dst; - int err = -ENOMEM; + int err = -ENOMEM, i; flow_rule = kzalloc(sizeof(*flow_rule), GFP_KERNEL); if (!flow_rule) diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c index 9adff83b523b..b3b68dacadd0 100644 --- a/net/sched/act_ct.c +++ b/net/sched/act_ct.c @@ -61,6 +61,11 @@ tcf_ct_flow_table_flow_action_get_next(struct flow_action *flow_action) { int i = flow_action->num_entries++; + /* Pre-fill action hw_stats with DONT_CARE. Caller can override this + * if it wants stats for its action + */ + flow_action->entries[i].hw_stats = FLOW_ACTION_HW_STATS_DONT_CARE; + return &flow_action->entries[i]; }
Make FLOW_ACTION_HW_STATS_DONT_CARE be all bits, rather than none, so that drivers and __flow_action_hw_stats_check can use simple bitwise checks. Also ensure that netfilter explicitly sets its actions to DONT_CARE, rather than relying on implicit semantics of zero. Only the kernel's internal API semantics change; the TC uAPI is unaffected. v3: set DONT_CARE in nft and ct offload. Tested the latter with an experimental driver; conntrack entry actions had hw_stats=7, as expected. v2: rebased on net-next, removed RFC tags. Signed-off-by: Edward Cree <ecree@solarflare.com> --- I don't have hardware that does TC_SETUP_FT offload. Could someone from mlx test that nft offload comes through with hw_stats=DONT_CARE? .../net/ethernet/mellanox/mlxsw/spectrum_flower.c | 8 ++++---- include/net/flow_offload.h | 11 +++++++---- net/netfilter/nf_flow_table_offload.c | 14 +++++++++++--- net/sched/act_ct.c | 5 +++++ 4 files changed, 27 insertions(+), 11 deletions(-)