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

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

[Pablo Neira Ayuso]

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>
---
v4: Update mlxsw as Jiri prefers.

 .../net/ethernet/mellanox/mlxsw/spectrum_flower.c  |  3 ++-
 include/net/flow_offload.h                         |  9 ++++++++-
 net/sched/cls_api.c                                | 14 ++++++++++++--
 3 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c
index 51117a5a6bbf..890b078851c9 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c
@@ -36,7 +36,8 @@ static int mlxsw_sp_flower_parse_actions(struct mlxsw_sp *mlxsw_sp,
 		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) {
+	} else if (act->hw_stats != FLOW_ACTION_HW_STATS_DISABLED &&
+		   act->hw_stats != FLOW_ACTION_HW_STATS_DONT_CARE) {
 		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 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,
 	FLOW_ACTION_HW_STATS_IMMEDIATE =
 		BIT(FLOW_ACTION_HW_STATS_IMMEDIATE_BIT),
 	FLOW_ACTION_HW_STATS_DELAYED = BIT(FLOW_ACTION_HW_STATS_DELAYED_BIT),
 	FLOW_ACTION_HW_STATS_ANY = FLOW_ACTION_HW_STATS_IMMEDIATE |
 				   FLOW_ACTION_HW_STATS_DELAYED,
+	FLOW_ACTION_HW_STATS_DISABLED =
+		BIT(FLOW_ACTION_HW_STATS_DISABLED_BIT),
 };
 
 typedef void (*action_destr)(void *priv);
@@ -325,7 +328,11 @@ __flow_action_hw_stats_check(const struct flow_action *action,
 		return true;
 	if (!flow_action_mixed_hw_stats_check(action, extack))
 		return false;
+
 	action_entry = flow_action_first_entry_get(action);
+	if (action_entry->hw_stats == FLOW_ACTION_HW_STATS_DONT_CARE)
+		return true;
+
 	if (!check_allow_bit &&
 	    action_entry->hw_stats != FLOW_ACTION_HW_STATS_ANY) {
 		NL_SET_ERR_MSG_MOD(extack, "Driver supports only default HW stats type \"any\"");
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 55bd1429678f..56cf1b9e1e24 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -3523,6 +3523,16 @@ static void tcf_sample_get_group(struct flow_action_entry *entry,
 #endif
 }
 
+static enum flow_action_hw_stats tc_act_hw_stats(u8 hw_stats)
+{
+	if (WARN_ON_ONCE(hw_stats > TCA_ACT_HW_STATS_ANY))
+		return FLOW_ACTION_HW_STATS_DONT_CARE;
+	else if (!hw_stats)
+		return FLOW_ACTION_HW_STATS_DISABLED;
+
+	return hw_stats;
+}
+
 int tc_setup_flow_action(struct flow_action *flow_action,
 			 const struct tcf_exts *exts)
 {
@@ -3546,7 +3556,7 @@ int tc_setup_flow_action(struct flow_action *flow_action,
 		if (err)
 			goto err_out_locked;
 
-		entry->hw_stats = act->hw_stats;
+		entry->hw_stats = tc_act_hw_stats(act->hw_stats);
 
 		if (is_tcf_gact_ok(act)) {
 			entry->id = FLOW_ACTION_ACCEPT;
@@ -3614,7 +3624,7 @@ int tc_setup_flow_action(struct flow_action *flow_action,
 				entry->mangle.mask = tcf_pedit_mask(act, k);
 				entry->mangle.val = tcf_pedit_val(act, k);
 				entry->mangle.offset = tcf_pedit_offset(act, k);
-				entry->hw_stats = act->hw_stats;
+				entry->hw_stats = tc_act_hw_stats(act->hw_stats);
 				entry = &flow_action->entries[++j];
 			}
 		} else if (is_tcf_csum(act)) {
-- 
2.20.1

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

[David Miller]

From: Pablo Neira Ayuso <pablo@netfilter.org>
Date: Wed,  6 May 2020 20:34:50 +0200

> 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>
> ---
> v4: Update mlxsw as Jiri prefers.

Applied, thank you.

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

[Edward Cree]

On 06/05/2020 19:34, Pablo Neira Ayuso wrote> -	} else if (act->hw_stats != FLOW_ACTION_HW_STATS_DISABLED) {
> +	} else if (act->hw_stats != FLOW_ACTION_HW_STATS_DISABLED &&
> +		   act->hw_stats != FLOW_ACTION_HW_STATS_DONT_CARE) {
>  		NL_SET_ERR_MSG_MOD(extack, "Unsupported action HW stats type");
>  		return -EOPNOTSUPP;
>  	}
FWIW my whole reason for suggesting DONT_CARE==0 in the first place
 was so that drivers could just use it as a boolean, e.g.
    if (act->hw_stats && !(act->hw_stats & FLOW_ACTION_HW_STATS_BLAH))
        error("driver only supports BLAH stats");
If you're not even doing that then the case for DONT_CARE == ~0 is
 even stronger.
Sorry I wasn't quick enough on the draw to say this before v4 was
 applied (I was waiting for an answer on the v2 thread; posting a
 Nack on v3 felt like it might come across as needlessly combative).

-ed

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

[Pablo Neira Ayuso]

On Thu, May 07, 2020 at 11:38:24AM +0100, Edward Cree wrote:
> On 06/05/2020 19:34, Pablo Neira Ayuso wrote> -	} else if (act->hw_stats != FLOW_ACTION_HW_STATS_DISABLED) {
> > +	} else if (act->hw_stats != FLOW_ACTION_HW_STATS_DISABLED &&
> > +		   act->hw_stats != FLOW_ACTION_HW_STATS_DONT_CARE) {
> >  		NL_SET_ERR_MSG_MOD(extack, "Unsupported action HW stats type");
> >  		return -EOPNOTSUPP;
> >  	}
> FWIW my whole reason for suggesting DONT_CARE==0 in the first place
>  was so that drivers could just use it as a boolean, e.g.
>     if (act->hw_stats && !(act->hw_stats & FLOW_ACTION_HW_STATS_BLAH))
>         error("driver only supports BLAH stats");
>
> If you're not even doing that then the case for DONT_CARE == ~0 is
>  even stronger.

Could you point to what driver might have any problem with this update?

Thank you.

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

[Edward Cree]

On 07/05/2020 12:44, Pablo Neira Ayuso wrote:
> Could you point to what driver might have any problem with this update?
Drivers *can* implement the API in this patch.  It's just that the
 alternative API Jakub proposed would make for simpler driver code.
I.e. I'm not saying it's bad, just that it could be made better.
That's why I didn't hard-NACK it at any point.
I guess I should send the change I'm suggesting as a patch, rather
 than asking it of you — I'll try to get that done today.
(Although I'm not sure if it's really 'net' material or if I should
 wait for David to merge net into net-next and make the patch
 against the latter — wdyt?)

-ed