[PATCH net-next v3 0/2] flow_offload: add tc police parameters

[PATCH net-next v3 0/2] flow_offload: add tc police parameters

[Jianbo Liu]

As a preparation for more advanced police offload in mlx5 (e.g.,
jumping to another chain when bandwidth is not exceeded), extend the
flow offload API with more tc-police parameters. Adjust existing
drivers to reject unsupported configurations.

Changes since v2:
  * Rename index to extval in exceed and notexceed acts.
  * Add policer validate functions for all drivers.

Changes since v1:
  * Add one more strict validation for the control of drop/ok.

Jianbo Liu (2):
  net: flow_offload: add tc police action parameters
  flow_offload: reject offload for all drivers with invalid police
    parameters

 drivers/net/dsa/sja1105/sja1105_flower.c      | 47 +++++++++++++--
 .../chelsio/cxgb4/cxgb4_tc_matchall.c         | 59 +++++++++++++++----
 .../net/ethernet/freescale/enetc/enetc_qos.c  | 47 +++++++++++++--
 .../ethernet/marvell/octeontx2/nic/otx2_tc.c  | 43 ++++++++++++++
 .../net/ethernet/mellanox/mlx5/core/en_tc.c   | 48 +++++++++++++--
 .../ethernet/mellanox/mlxsw/spectrum_flower.c | 47 +++++++++++++--
 drivers/net/ethernet/mscc/ocelot_flower.c     | 14 +++--
 drivers/net/ethernet/mscc/ocelot_net.c        | 10 ++--
 drivers/net/ethernet/mscc/ocelot_police.c     | 41 +++++++++++++
 drivers/net/ethernet/mscc/ocelot_police.h     |  5 ++
 .../ethernet/netronome/nfp/flower/qos_conf.c  | 40 +++++++++++++
 include/net/flow_offload.h                    | 15 +++++
 include/net/tc_act/tc_police.h                | 30 ++++++++++
 net/sched/act_police.c                        | 46 +++++++++++++++
 14 files changed, 454 insertions(+), 38 deletions(-)

-- 
2.26.2

[PATCH net-next v3 1/2] net: flow_offload: add tc police action parameters

[Jianbo Liu]

The current police offload action entry is missing exceed/notexceed
actions and parameters that can be configured by tc police action.
Add the missing parameters as a pre-step for offloading police actions
to hardware.

Signed-off-by: Jianbo Liu <jianbol@nvidia.com>
Signed-off-by: Roi Dayan <roid@nvidia.com>
Reviewed-by: Ido Schimmel <idosch@nvidia.com>
---
 include/net/flow_offload.h     |  9 +++++++
 include/net/tc_act/tc_police.h | 30 ++++++++++++++++++++++
 net/sched/act_police.c         | 46 ++++++++++++++++++++++++++++++++++
 3 files changed, 85 insertions(+)

diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
index 5b8c54eb7a6b..74f44d44abe3 100644
--- a/include/net/flow_offload.h
+++ b/include/net/flow_offload.h
@@ -148,6 +148,8 @@ enum flow_action_id {
 	FLOW_ACTION_MPLS_MANGLE,
 	FLOW_ACTION_GATE,
 	FLOW_ACTION_PPPOE_PUSH,
+	FLOW_ACTION_JUMP,
+	FLOW_ACTION_PIPE,
 	NUM_FLOW_ACTIONS,
 };
 
@@ -235,9 +237,16 @@ struct flow_action_entry {
 		struct {				/* FLOW_ACTION_POLICE */
 			u32			burst;
 			u64			rate_bytes_ps;
+			u64			peakrate_bytes_ps;
+			u32			avrate;
+			u16			overhead;
 			u64			burst_pkt;
 			u64			rate_pkt_ps;
 			u32			mtu;
+			struct {
+				enum flow_action_id	act_id;
+				u32			extval;
+			} exceed, notexceed;
 		} police;
 		struct {				/* FLOW_ACTION_CT */
 			int action;
diff --git a/include/net/tc_act/tc_police.h b/include/net/tc_act/tc_police.h
index 72649512dcdd..283bde711a42 100644
--- a/include/net/tc_act/tc_police.h
+++ b/include/net/tc_act/tc_police.h
@@ -159,4 +159,34 @@ static inline u32 tcf_police_tcfp_mtu(const struct tc_action *act)
 	return params->tcfp_mtu;
 }
 
+static inline u64 tcf_police_peakrate_bytes_ps(const struct tc_action *act)
+{
+	struct tcf_police *police = to_police(act);
+	struct tcf_police_params *params;
+
+	params = rcu_dereference_protected(police->params,
+					   lockdep_is_held(&police->tcf_lock));
+	return params->peak.rate_bytes_ps;
+}
+
+static inline u32 tcf_police_tcfp_ewma_rate(const struct tc_action *act)
+{
+	struct tcf_police *police = to_police(act);
+	struct tcf_police_params *params;
+
+	params = rcu_dereference_protected(police->params,
+					   lockdep_is_held(&police->tcf_lock));
+	return params->tcfp_ewma_rate;
+}
+
+static inline u16 tcf_police_rate_overhead(const struct tc_action *act)
+{
+	struct tcf_police *police = to_police(act);
+	struct tcf_police_params *params;
+
+	params = rcu_dereference_protected(police->params,
+					   lockdep_is_held(&police->tcf_lock));
+	return params->rate.overhead;
+}
+
 #endif /* __NET_TC_POLICE_H */
diff --git a/net/sched/act_police.c b/net/sched/act_police.c
index 0923aa2b8f8a..a2275eef6877 100644
--- a/net/sched/act_police.c
+++ b/net/sched/act_police.c
@@ -405,20 +405,66 @@ static int tcf_police_search(struct net *net, struct tc_action **a, u32 index)
 	return tcf_idr_search(tn, a, index);
 }
 
+static int tcf_police_act_to_flow_act(int tc_act, u32 *extval)
+{
+	int act_id = -EOPNOTSUPP;
+
+	if (!TC_ACT_EXT_OPCODE(tc_act)) {
+		if (tc_act == TC_ACT_OK)
+			act_id = FLOW_ACTION_ACCEPT;
+		else if (tc_act ==  TC_ACT_SHOT)
+			act_id = FLOW_ACTION_DROP;
+		else if (tc_act == TC_ACT_PIPE)
+			act_id = FLOW_ACTION_PIPE;
+	} else if (TC_ACT_EXT_CMP(tc_act, TC_ACT_GOTO_CHAIN)) {
+		act_id = FLOW_ACTION_GOTO;
+		*extval = tc_act & TC_ACT_EXT_VAL_MASK;
+	} else if (TC_ACT_EXT_CMP(tc_act, TC_ACT_JUMP)) {
+		act_id = FLOW_ACTION_JUMP;
+		*extval = tc_act & TC_ACT_EXT_VAL_MASK;
+	}
+
+	return act_id;
+}
+
 static int tcf_police_offload_act_setup(struct tc_action *act, void *entry_data,
 					u32 *index_inc, bool bind)
 {
 	if (bind) {
 		struct flow_action_entry *entry = entry_data;
+		struct tcf_police *police = to_police(act);
+		struct tcf_police_params *p;
+		int act_id;
+
+		p = rcu_dereference_protected(police->params,
+					      lockdep_is_held(&police->tcf_lock));
 
 		entry->id = FLOW_ACTION_POLICE;
 		entry->police.burst = tcf_police_burst(act);
 		entry->police.rate_bytes_ps =
 			tcf_police_rate_bytes_ps(act);
+		entry->police.peakrate_bytes_ps = tcf_police_peakrate_bytes_ps(act);
+		entry->police.avrate = tcf_police_tcfp_ewma_rate(act);
+		entry->police.overhead = tcf_police_rate_overhead(act);
 		entry->police.burst_pkt = tcf_police_burst_pkt(act);
 		entry->police.rate_pkt_ps =
 			tcf_police_rate_pkt_ps(act);
 		entry->police.mtu = tcf_police_tcfp_mtu(act);
+
+		act_id = tcf_police_act_to_flow_act(police->tcf_action,
+						    &entry->police.exceed.extval);
+		if (act_id < 0)
+			return act_id;
+
+		entry->police.exceed.act_id = act_id;
+
+		act_id = tcf_police_act_to_flow_act(p->tcfp_result,
+						    &entry->police.notexceed.extval);
+		if (act_id < 0)
+			return act_id;
+
+		entry->police.notexceed.act_id = act_id;
+
 		*index_inc = 1;
 	} else {
 		struct flow_offload_action *fl_action = entry_data;
-- 
2.26.2

Re: [PATCH net-next v3 1/2] net: flow_offload: add tc police action parameters

[Vladimir Oltean]

Hello Jianbo,

On Thu, Feb 24, 2022 at 10:29:07AM +0000, Jianbo Liu wrote:
> The current police offload action entry is missing exceed/notexceed
> actions and parameters that can be configured by tc police action.
> Add the missing parameters as a pre-step for offloading police actions
> to hardware.
> 
> Signed-off-by: Jianbo Liu <jianbol@nvidia.com>
> Signed-off-by: Roi Dayan <roid@nvidia.com>
> Reviewed-by: Ido Schimmel <idosch@nvidia.com>
> ---
>  include/net/flow_offload.h     |  9 +++++++
>  include/net/tc_act/tc_police.h | 30 ++++++++++++++++++++++
>  net/sched/act_police.c         | 46 ++++++++++++++++++++++++++++++++++
>  3 files changed, 85 insertions(+)
> 
> diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
> index 5b8c54eb7a6b..74f44d44abe3 100644
> --- a/include/net/flow_offload.h
> +++ b/include/net/flow_offload.h
> @@ -148,6 +148,8 @@ enum flow_action_id {
>  	FLOW_ACTION_MPLS_MANGLE,
>  	FLOW_ACTION_GATE,
>  	FLOW_ACTION_PPPOE_PUSH,
> +	FLOW_ACTION_JUMP,
> +	FLOW_ACTION_PIPE,
>  	NUM_FLOW_ACTIONS,
>  };
>  
> @@ -235,9 +237,16 @@ struct flow_action_entry {
>  		struct {				/* FLOW_ACTION_POLICE */
>  			u32			burst;
>  			u64			rate_bytes_ps;
> +			u64			peakrate_bytes_ps;
> +			u32			avrate;
> +			u16			overhead;
>  			u64			burst_pkt;
>  			u64			rate_pkt_ps;
>  			u32			mtu;
> +			struct {
> +				enum flow_action_id	act_id;
> +				u32			extval;
> +			} exceed, notexceed;
>  		} police;
>  		struct {				/* FLOW_ACTION_CT */
>  			int action;
> diff --git a/include/net/tc_act/tc_police.h b/include/net/tc_act/tc_police.h
> index 72649512dcdd..283bde711a42 100644
> --- a/include/net/tc_act/tc_police.h
> +++ b/include/net/tc_act/tc_police.h
> @@ -159,4 +159,34 @@ static inline u32 tcf_police_tcfp_mtu(const struct tc_action *act)
>  	return params->tcfp_mtu;
>  }
>  
> +static inline u64 tcf_police_peakrate_bytes_ps(const struct tc_action *act)
> +{
> +	struct tcf_police *police = to_police(act);
> +	struct tcf_police_params *params;
> +
> +	params = rcu_dereference_protected(police->params,
> +					   lockdep_is_held(&police->tcf_lock));
> +	return params->peak.rate_bytes_ps;
> +}
> +
> +static inline u32 tcf_police_tcfp_ewma_rate(const struct tc_action *act)
> +{
> +	struct tcf_police *police = to_police(act);
> +	struct tcf_police_params *params;
> +
> +	params = rcu_dereference_protected(police->params,
> +					   lockdep_is_held(&police->tcf_lock));
> +	return params->tcfp_ewma_rate;
> +}
> +
> +static inline u16 tcf_police_rate_overhead(const struct tc_action *act)
> +{
> +	struct tcf_police *police = to_police(act);
> +	struct tcf_police_params *params;
> +
> +	params = rcu_dereference_protected(police->params,
> +					   lockdep_is_held(&police->tcf_lock));
> +	return params->rate.overhead;
> +}
> +
>  #endif /* __NET_TC_POLICE_H */
> diff --git a/net/sched/act_police.c b/net/sched/act_police.c
> index 0923aa2b8f8a..a2275eef6877 100644
> --- a/net/sched/act_police.c
> +++ b/net/sched/act_police.c
> @@ -405,20 +405,66 @@ static int tcf_police_search(struct net *net, struct tc_action **a, u32 index)
>  	return tcf_idr_search(tn, a, index);
>  }
>  
> +static int tcf_police_act_to_flow_act(int tc_act, u32 *extval)
> +{
> +	int act_id = -EOPNOTSUPP;
> +
> +	if (!TC_ACT_EXT_OPCODE(tc_act)) {
> +		if (tc_act == TC_ACT_OK)
> +			act_id = FLOW_ACTION_ACCEPT;
> +		else if (tc_act ==  TC_ACT_SHOT)
> +			act_id = FLOW_ACTION_DROP;
> +		else if (tc_act == TC_ACT_PIPE)
> +			act_id = FLOW_ACTION_PIPE;
> +	} else if (TC_ACT_EXT_CMP(tc_act, TC_ACT_GOTO_CHAIN)) {
> +		act_id = FLOW_ACTION_GOTO;
> +		*extval = tc_act & TC_ACT_EXT_VAL_MASK;
> +	} else if (TC_ACT_EXT_CMP(tc_act, TC_ACT_JUMP)) {
> +		act_id = FLOW_ACTION_JUMP;
> +		*extval = tc_act & TC_ACT_EXT_VAL_MASK;
> +	}
> +
> +	return act_id;
> +}
> +
>  static int tcf_police_offload_act_setup(struct tc_action *act, void *entry_data,
>  					u32 *index_inc, bool bind)
>  {
>  	if (bind) {
>  		struct flow_action_entry *entry = entry_data;
> +		struct tcf_police *police = to_police(act);
> +		struct tcf_police_params *p;
> +		int act_id;
> +
> +		p = rcu_dereference_protected(police->params,
> +					      lockdep_is_held(&police->tcf_lock));
>  
>  		entry->id = FLOW_ACTION_POLICE;
>  		entry->police.burst = tcf_police_burst(act);
>  		entry->police.rate_bytes_ps =
>  			tcf_police_rate_bytes_ps(act);
> +		entry->police.peakrate_bytes_ps = tcf_police_peakrate_bytes_ps(act);
> +		entry->police.avrate = tcf_police_tcfp_ewma_rate(act);
> +		entry->police.overhead = tcf_police_rate_overhead(act);
>  		entry->police.burst_pkt = tcf_police_burst_pkt(act);
>  		entry->police.rate_pkt_ps =
>  			tcf_police_rate_pkt_ps(act);
>  		entry->police.mtu = tcf_police_tcfp_mtu(act);
> +
> +		act_id = tcf_police_act_to_flow_act(police->tcf_action,
> +						    &entry->police.exceed.extval);

I don't know why just now, but I observed an apparent regression here
with these commands:

root@debian:~# tc qdisc add dev swp3 clsact
root@debian:~# tc filter add dev swp3 ingress protocol ip flower skip_sw ip_proto icmp action police rate 100Mbit burst 10000
[   45.767900] tcf_police_act_to_flow_act: 434: tc_act 1
[   45.773100] tcf_police_offload_act_setup: 475, act_id -95
Error: cls_flower: Failed to setup flow action.
We have an error talking to the kernel, -1

The reason why I'm not sure is because I don't know if this should have
worked as intended or not. I am remarking just now in "man tc-police"
that the default conform-exceed action is "reclassify".

So if I specify "conform-exceed drop", things are as expected, but with
the default (implicitly "conform-exceed reclassify") things fail with
-EOPNOTSUPP because tcf_police_act_to_flow_act() doesn't handle a
police->tcf_action of TC_ACT_RECLASSIFY.

Should it?

> +		if (act_id < 0)
> +			return act_id;
> +
> +		entry->police.exceed.act_id = act_id;
> +
> +		act_id = tcf_police_act_to_flow_act(p->tcfp_result,
> +						    &entry->police.notexceed.extval);
> +		if (act_id < 0)
> +			return act_id;
> +
> +		entry->police.notexceed.act_id = act_id;
> +
>  		*index_inc = 1;
>  	} else {
>  		struct flow_offload_action *fl_action = entry_data;
> -- 
> 2.26.2
> 

Re: [PATCH net-next v3 1/2] net: flow_offload: add tc police action parameters

[Ido Schimmel]

On Tue, Mar 15, 2022 at 09:13:58PM +0200, Vladimir Oltean wrote:
> Hello Jianbo,
> 
> On Thu, Feb 24, 2022 at 10:29:07AM +0000, Jianbo Liu wrote:
> > The current police offload action entry is missing exceed/notexceed
> > actions and parameters that can be configured by tc police action.
> > Add the missing parameters as a pre-step for offloading police actions
> > to hardware.
> > 
> > Signed-off-by: Jianbo Liu <jianbol@nvidia.com>
> > Signed-off-by: Roi Dayan <roid@nvidia.com>
> > Reviewed-by: Ido Schimmel <idosch@nvidia.com>
> > ---
> >  include/net/flow_offload.h     |  9 +++++++
> >  include/net/tc_act/tc_police.h | 30 ++++++++++++++++++++++
> >  net/sched/act_police.c         | 46 ++++++++++++++++++++++++++++++++++
> >  3 files changed, 85 insertions(+)
> > 
> > diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
> > index 5b8c54eb7a6b..74f44d44abe3 100644
> > --- a/include/net/flow_offload.h
> > +++ b/include/net/flow_offload.h
> > @@ -148,6 +148,8 @@ enum flow_action_id {
> >  	FLOW_ACTION_MPLS_MANGLE,
> >  	FLOW_ACTION_GATE,
> >  	FLOW_ACTION_PPPOE_PUSH,
> > +	FLOW_ACTION_JUMP,
> > +	FLOW_ACTION_PIPE,
> >  	NUM_FLOW_ACTIONS,
> >  };
> >  
> > @@ -235,9 +237,16 @@ struct flow_action_entry {
> >  		struct {				/* FLOW_ACTION_POLICE */
> >  			u32			burst;
> >  			u64			rate_bytes_ps;
> > +			u64			peakrate_bytes_ps;
> > +			u32			avrate;
> > +			u16			overhead;
> >  			u64			burst_pkt;
> >  			u64			rate_pkt_ps;
> >  			u32			mtu;
> > +			struct {
> > +				enum flow_action_id	act_id;
> > +				u32			extval;
> > +			} exceed, notexceed;
> >  		} police;
> >  		struct {				/* FLOW_ACTION_CT */
> >  			int action;
> > diff --git a/include/net/tc_act/tc_police.h b/include/net/tc_act/tc_police.h
> > index 72649512dcdd..283bde711a42 100644
> > --- a/include/net/tc_act/tc_police.h
> > +++ b/include/net/tc_act/tc_police.h
> > @@ -159,4 +159,34 @@ static inline u32 tcf_police_tcfp_mtu(const struct tc_action *act)
> >  	return params->tcfp_mtu;
> >  }
> >  
> > +static inline u64 tcf_police_peakrate_bytes_ps(const struct tc_action *act)
> > +{
> > +	struct tcf_police *police = to_police(act);
> > +	struct tcf_police_params *params;
> > +
> > +	params = rcu_dereference_protected(police->params,
> > +					   lockdep_is_held(&police->tcf_lock));
> > +	return params->peak.rate_bytes_ps;
> > +}
> > +
> > +static inline u32 tcf_police_tcfp_ewma_rate(const struct tc_action *act)
> > +{
> > +	struct tcf_police *police = to_police(act);
> > +	struct tcf_police_params *params;
> > +
> > +	params = rcu_dereference_protected(police->params,
> > +					   lockdep_is_held(&police->tcf_lock));
> > +	return params->tcfp_ewma_rate;
> > +}
> > +
> > +static inline u16 tcf_police_rate_overhead(const struct tc_action *act)
> > +{
> > +	struct tcf_police *police = to_police(act);
> > +	struct tcf_police_params *params;
> > +
> > +	params = rcu_dereference_protected(police->params,
> > +					   lockdep_is_held(&police->tcf_lock));
> > +	return params->rate.overhead;
> > +}
> > +
> >  #endif /* __NET_TC_POLICE_H */
> > diff --git a/net/sched/act_police.c b/net/sched/act_police.c
> > index 0923aa2b8f8a..a2275eef6877 100644
> > --- a/net/sched/act_police.c
> > +++ b/net/sched/act_police.c
> > @@ -405,20 +405,66 @@ static int tcf_police_search(struct net *net, struct tc_action **a, u32 index)
> >  	return tcf_idr_search(tn, a, index);
> >  }
> >  
> > +static int tcf_police_act_to_flow_act(int tc_act, u32 *extval)
> > +{
> > +	int act_id = -EOPNOTSUPP;
> > +
> > +	if (!TC_ACT_EXT_OPCODE(tc_act)) {
> > +		if (tc_act == TC_ACT_OK)
> > +			act_id = FLOW_ACTION_ACCEPT;
> > +		else if (tc_act ==  TC_ACT_SHOT)
> > +			act_id = FLOW_ACTION_DROP;
> > +		else if (tc_act == TC_ACT_PIPE)
> > +			act_id = FLOW_ACTION_PIPE;
> > +	} else if (TC_ACT_EXT_CMP(tc_act, TC_ACT_GOTO_CHAIN)) {
> > +		act_id = FLOW_ACTION_GOTO;
> > +		*extval = tc_act & TC_ACT_EXT_VAL_MASK;
> > +	} else if (TC_ACT_EXT_CMP(tc_act, TC_ACT_JUMP)) {
> > +		act_id = FLOW_ACTION_JUMP;
> > +		*extval = tc_act & TC_ACT_EXT_VAL_MASK;
> > +	}
> > +
> > +	return act_id;
> > +}
> > +
> >  static int tcf_police_offload_act_setup(struct tc_action *act, void *entry_data,
> >  					u32 *index_inc, bool bind)
> >  {
> >  	if (bind) {
> >  		struct flow_action_entry *entry = entry_data;
> > +		struct tcf_police *police = to_police(act);
> > +		struct tcf_police_params *p;
> > +		int act_id;
> > +
> > +		p = rcu_dereference_protected(police->params,
> > +					      lockdep_is_held(&police->tcf_lock));
> >  
> >  		entry->id = FLOW_ACTION_POLICE;
> >  		entry->police.burst = tcf_police_burst(act);
> >  		entry->police.rate_bytes_ps =
> >  			tcf_police_rate_bytes_ps(act);
> > +		entry->police.peakrate_bytes_ps = tcf_police_peakrate_bytes_ps(act);
> > +		entry->police.avrate = tcf_police_tcfp_ewma_rate(act);
> > +		entry->police.overhead = tcf_police_rate_overhead(act);
> >  		entry->police.burst_pkt = tcf_police_burst_pkt(act);
> >  		entry->police.rate_pkt_ps =
> >  			tcf_police_rate_pkt_ps(act);
> >  		entry->police.mtu = tcf_police_tcfp_mtu(act);
> > +
> > +		act_id = tcf_police_act_to_flow_act(police->tcf_action,
> > +						    &entry->police.exceed.extval);
> 
> I don't know why just now, but I observed an apparent regression here
> with these commands:
> 
> root@debian:~# tc qdisc add dev swp3 clsact
> root@debian:~# tc filter add dev swp3 ingress protocol ip flower skip_sw ip_proto icmp action police rate 100Mbit burst 10000
> [   45.767900] tcf_police_act_to_flow_act: 434: tc_act 1
> [   45.773100] tcf_police_offload_act_setup: 475, act_id -95
> Error: cls_flower: Failed to setup flow action.
> We have an error talking to the kernel, -1
> 
> The reason why I'm not sure is because I don't know if this should have
> worked as intended or not. I am remarking just now in "man tc-police"
> that the default conform-exceed action is "reclassify".
> 
> So if I specify "conform-exceed drop", things are as expected, but with
> the default (implicitly "conform-exceed reclassify") things fail with
> -EOPNOTSUPP because tcf_police_act_to_flow_act() doesn't handle a
> police->tcf_action of TC_ACT_RECLASSIFY.
> 
> Should it?

Even if tcf_police_act_to_flow_act() handled "reclassify", the
configuration would have been rejected later on by the relevant device
driver since they all support "drop" for exceed action and nothing else.

I don't know why iproute2 defaults to "reclassify", but the
configuration in the example does something different in the SW and HW
data paths. One ugly suggestion to keep this case working it to have
tcf_police_act_to_flow_act() default to "drop" and emit a warning via
extack so that user space is at least aware of this misconfiguration.

> 
> > +		if (act_id < 0)
> > +			return act_id;
> > +
> > +		entry->police.exceed.act_id = act_id;
> > +
> > +		act_id = tcf_police_act_to_flow_act(p->tcfp_result,
> > +						    &entry->police.notexceed.extval);
> > +		if (act_id < 0)
> > +			return act_id;
> > +
> > +		entry->police.notexceed.act_id = act_id;
> > +
> >  		*index_inc = 1;
> >  	} else {
> >  		struct flow_offload_action *fl_action = entry_data;
> > -- 
> > 2.26.2
> > 

Re: [PATCH net-next v3 1/2] net: flow_offload: add tc police action parameters

[Vladimir Oltean]

On Thu, Mar 17, 2022 at 03:22:42PM +0200, Ido Schimmel wrote:
> > I don't know why just now, but I observed an apparent regression here
> > with these commands:
> > 
> > root@debian:~# tc qdisc add dev swp3 clsact
> > root@debian:~# tc filter add dev swp3 ingress protocol ip flower skip_sw ip_proto icmp action police rate 100Mbit burst 10000
> > [   45.767900] tcf_police_act_to_flow_act: 434: tc_act 1
> > [   45.773100] tcf_police_offload_act_setup: 475, act_id -95
> > Error: cls_flower: Failed to setup flow action.
> > We have an error talking to the kernel, -1
> > 
> > The reason why I'm not sure is because I don't know if this should have
> > worked as intended or not. I am remarking just now in "man tc-police"
> > that the default conform-exceed action is "reclassify".
> > 
> > So if I specify "conform-exceed drop", things are as expected, but with
> > the default (implicitly "conform-exceed reclassify") things fail with
> > -EOPNOTSUPP because tcf_police_act_to_flow_act() doesn't handle a
> > police->tcf_action of TC_ACT_RECLASSIFY.
> > 
> > Should it?
> 
> Even if tcf_police_act_to_flow_act() handled "reclassify", the
> configuration would have been rejected later on by the relevant device
> driver since they all support "drop" for exceed action and nothing else.

This is correct, but currently, the error is:

Error: cls_flower: Failed to setup flow action.
We have an error talking to the kernel, -1

I'd appreciate if the error was instead:

Error: mscc_ocelot: Offload not supported when exceed action is not drop.

which is basically what Jianbo was trying to achieve when he added the
policer_validate() functions. At least I'd know what's wrong. No?

> I don't know why iproute2 defaults to "reclassify", but the
> configuration in the example does something different in the SW and HW
> data paths. One ugly suggestion to keep this case working it to have
> tcf_police_act_to_flow_act() default to "drop" and emit a warning via
> extack so that user space is at least aware of this misconfiguration.

I don't want to force a reinterpretation of "reclassify" just to make
something that used to work by mistake continue to work. It sucks to
have to adapt, but not being able to make progress because of such
things sucks even more.

I'd just like the 'reclassify' action to be propagated in some reasonable
way to flow offload, considering that at the moment the error is quite cryptic.

> > > +		if (act_id < 0)
> > > +			return act_id;
> > > +
> > > +		entry->police.exceed.act_id = act_id;
> > > +
> > > +		act_id = tcf_police_act_to_flow_act(p->tcfp_result,
> > > +						    &entry->police.notexceed.extval);
> > > +		if (act_id < 0)
> > > +			return act_id;
> > > +
> > > +		entry->police.notexceed.act_id = act_id;
> > > +
> > >  		*index_inc = 1;
> > >  	} else {
> > >  		struct flow_offload_action *fl_action = entry_data;
> > > -- 
> > > 2.26.2
> > > 

Re: [PATCH net-next v3 1/2] net: flow_offload: add tc police action parameters

[Ido Schimmel]

On Thu, Mar 17, 2022 at 08:52:49PM +0200, Vladimir Oltean wrote:
> I'd just like the 'reclassify' action to be propagated in some reasonable
> way to flow offload, considering that at the moment the error is quite cryptic.

OK, will check next week. Might be best to simply propagate extack to
offload_act_setup() and return a meaningful message in
tcf_police_offload_act_setup(). There are a bunch of other actions whose
callback simply returns '-EOPNOTSUPP' that can benefit from it.

Re: [PATCH net-next v3 1/2] net: flow_offload: add tc police action parameters

[Ido Schimmel]

On Thu, Mar 17, 2022 at 09:37:22PM +0200, Ido Schimmel wrote:
> On Thu, Mar 17, 2022 at 08:52:49PM +0200, Vladimir Oltean wrote:
> > I'd just like the 'reclassify' action to be propagated in some reasonable
> > way to flow offload, considering that at the moment the error is quite cryptic.
> 
> OK, will check next week. Might be best to simply propagate extack to
> offload_act_setup() and return a meaningful message in
> tcf_police_offload_act_setup(). There are a bunch of other actions whose
> callback simply returns '-EOPNOTSUPP' that can benefit from it.

# tc filter add dev dummy0 ingress protocol ip flower skip_sw ip_proto icmp action police rate 100Mbit burst 10000
Error: act_police: Offload not supported when conform/exceed action is "reclassify".
We have an error talking to the kernel

Available here:
https://github.com/idosch/linux/commits/tc_extack

I plan to submit the patches after net-next reopens.

Re: [PATCH net-next v3 1/2] net: flow_offload: add tc police action parameters

[Vladimir Oltean]

On Tue, Mar 22, 2022 at 12:13:31PM +0200, Ido Schimmel wrote:
> On Thu, Mar 17, 2022 at 09:37:22PM +0200, Ido Schimmel wrote:
> > On Thu, Mar 17, 2022 at 08:52:49PM +0200, Vladimir Oltean wrote:
> > > I'd just like the 'reclassify' action to be propagated in some reasonable
> > > way to flow offload, considering that at the moment the error is quite cryptic.
> > 
> > OK, will check next week. Might be best to simply propagate extack to
> > offload_act_setup() and return a meaningful message in
> > tcf_police_offload_act_setup(). There are a bunch of other actions whose
> > callback simply returns '-EOPNOTSUPP' that can benefit from it.
> 
> # tc filter add dev dummy0 ingress protocol ip flower skip_sw ip_proto icmp action police rate 100Mbit burst 10000
> Error: act_police: Offload not supported when conform/exceed action is "reclassify".
> We have an error talking to the kernel
> 
> Available here:
> https://github.com/idosch/linux/commits/tc_extack
> 
> I plan to submit the patches after net-next reopens.

Thanks. I've tested these partially and at least the case that I
reported is now covered.

[PATCH net-next v3 2/2] flow_offload: reject offload for all drivers with invalid police parameters

[Jianbo Liu]

As more police parameters are passed to flow_offload, driver can check
them to make sure hardware handles packets in the way indicated by tc.
The conform-exceed control should be drop/pipe or drop/ok. Besides,
for drop/ok, the police should be the last action. As hardware can't
configure peakrate/avrate/overhead, offload should not be supported if
any of them is configured.

Signed-off-by: Jianbo Liu <jianbol@nvidia.com>
Reviewed-by: Ido Schimmel <idosch@nvidia.com>
---
 drivers/net/dsa/sja1105/sja1105_flower.c      | 47 +++++++++++++--
 .../chelsio/cxgb4/cxgb4_tc_matchall.c         | 59 +++++++++++++++----
 .../net/ethernet/freescale/enetc/enetc_qos.c  | 47 +++++++++++++--
 .../ethernet/marvell/octeontx2/nic/otx2_tc.c  | 43 ++++++++++++++
 .../net/ethernet/mellanox/mlx5/core/en_tc.c   | 48 +++++++++++++--
 .../ethernet/mellanox/mlxsw/spectrum_flower.c | 47 +++++++++++++--
 drivers/net/ethernet/mscc/ocelot_flower.c     | 14 +++--
 drivers/net/ethernet/mscc/ocelot_net.c        | 10 ++--
 drivers/net/ethernet/mscc/ocelot_police.c     | 41 +++++++++++++
 drivers/net/ethernet/mscc/ocelot_police.h     |  5 ++
 .../ethernet/netronome/nfp/flower/qos_conf.c  | 40 +++++++++++++
 include/net/flow_offload.h                    |  6 ++
 12 files changed, 369 insertions(+), 38 deletions(-)

diff --git a/drivers/net/dsa/sja1105/sja1105_flower.c b/drivers/net/dsa/sja1105/sja1105_flower.c
index 7dcdd784aea4..fad5afe3819c 100644
--- a/drivers/net/dsa/sja1105/sja1105_flower.c
+++ b/drivers/net/dsa/sja1105/sja1105_flower.c
@@ -300,6 +300,46 @@ static int sja1105_flower_parse_key(struct sja1105_private *priv,
 	return -EOPNOTSUPP;
 }
 
+static int sja1105_policer_validate(const struct flow_action *action,
+				    const struct flow_action_entry *act,
+				    struct netlink_ext_ack *extack)
+{
+	if (act->police.exceed.act_id != FLOW_ACTION_DROP) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "Offload not supported when exceed action is not drop");
+		return -EOPNOTSUPP;
+	}
+
+	if (act->police.notexceed.act_id != FLOW_ACTION_PIPE &&
+	    act->police.notexceed.act_id != FLOW_ACTION_ACCEPT) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "Offload not supported when conform action is not pipe or ok");
+		return -EOPNOTSUPP;
+	}
+
+	if (act->police.notexceed.act_id == FLOW_ACTION_ACCEPT &&
+	    !flow_action_is_last_entry(action, act)) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "Offload not supported when conform action is ok, but action is not last");
+		return -EOPNOTSUPP;
+	}
+
+	if (act->police.peakrate_bytes_ps ||
+	    act->police.avrate || act->police.overhead) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "Offload not supported when peakrate/avrate/overhead is configured");
+		return -EOPNOTSUPP;
+	}
+
+	if (act->police.rate_pkt_ps) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "QoS offload not support packets per second");
+		return -EOPNOTSUPP;
+	}
+
+	return 0;
+}
+
 int sja1105_cls_flower_add(struct dsa_switch *ds, int port,
 			   struct flow_cls_offload *cls, bool ingress)
 {
@@ -321,12 +361,9 @@ int sja1105_cls_flower_add(struct dsa_switch *ds, int port,
 	flow_action_for_each(i, act, &rule->action) {
 		switch (act->id) {
 		case FLOW_ACTION_POLICE:
-			if (act->police.rate_pkt_ps) {
-				NL_SET_ERR_MSG_MOD(extack,
-						   "QoS offload not support packets per second");
-				rc = -EOPNOTSUPP;
+			rc = sja1105_policer_validate(&rule->action, act, extack);
+			if (rc)
 				goto out;
-			}
 
 			rc = sja1105_flower_policer(priv, port, extack, cookie,
 						    &key,
diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_matchall.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_matchall.c
index 28fd2de9e4cf..1672d3afe5be 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_matchall.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_matchall.c
@@ -8,6 +8,46 @@
 #include "cxgb4_filter.h"
 #include "cxgb4_tc_flower.h"
 
+static int cxgb4_policer_validate(const struct flow_action *action,
+				  const struct flow_action_entry *act,
+				  struct netlink_ext_ack *extack)
+{
+	if (act->police.exceed.act_id != FLOW_ACTION_DROP) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "Offload not supported when exceed action is not drop");
+		return -EOPNOTSUPP;
+	}
+
+	if (act->police.notexceed.act_id != FLOW_ACTION_PIPE &&
+	    act->police.notexceed.act_id != FLOW_ACTION_ACCEPT) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "Offload not supported when conform action is not pipe or ok");
+		return -EOPNOTSUPP;
+	}
+
+	if (act->police.notexceed.act_id == FLOW_ACTION_ACCEPT &&
+	    !flow_action_is_last_entry(action, act)) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "Offload not supported when conform action is ok, but action is not last");
+		return -EOPNOTSUPP;
+	}
+
+	if (act->police.peakrate_bytes_ps ||
+	    act->police.avrate || act->police.overhead) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "Offload not supported when peakrate/avrate/overhead is configured");
+		return -EOPNOTSUPP;
+	}
+
+	if (act->police.rate_pkt_ps) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "QoS offload not support packets per second");
+		return -EOPNOTSUPP;
+	}
+
+	return 0;
+}
+
 static int cxgb4_matchall_egress_validate(struct net_device *dev,
 					  struct tc_cls_matchall_offload *cls)
 {
@@ -48,11 +88,10 @@ static int cxgb4_matchall_egress_validate(struct net_device *dev,
 	flow_action_for_each(i, entry, actions) {
 		switch (entry->id) {
 		case FLOW_ACTION_POLICE:
-			if (entry->police.rate_pkt_ps) {
-				NL_SET_ERR_MSG_MOD(extack,
-						   "QoS offload not support packets per second");
-				return -EOPNOTSUPP;
-			}
+			ret = cxgb4_policer_validate(actions, entry, extack);
+			if (ret)
+				return ret;
+
 			/* Convert bytes per second to bits per second */
 			if (entry->police.rate_bytes_ps * 8 > max_link_rate) {
 				NL_SET_ERR_MSG_MOD(extack,
@@ -150,11 +189,11 @@ static int cxgb4_matchall_alloc_tc(struct net_device *dev,
 	flow_action_for_each(i, entry, &cls->rule->action)
 		if (entry->id == FLOW_ACTION_POLICE)
 			break;
-	if (entry->police.rate_pkt_ps) {
-		NL_SET_ERR_MSG_MOD(extack,
-				   "QoS offload not support packets per second");
-		return -EOPNOTSUPP;
-	}
+
+	ret = cxgb4_policer_validate(&cls->rule->action, entry, extack);
+	if (ret)
+		return ret;
+
 	/* Convert from bytes per second to Kbps */
 	p.u.params.maxrate = div_u64(entry->police.rate_bytes_ps * 8, 1000);
 	p.u.params.channel = pi->tx_chan;
diff --git a/drivers/net/ethernet/freescale/enetc/enetc_qos.c b/drivers/net/ethernet/freescale/enetc/enetc_qos.c
index 3555c12edb45..6782a76aeaf6 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc_qos.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc_qos.c
@@ -1074,6 +1074,46 @@ static struct actions_fwd *enetc_check_flow_actions(u64 acts,
 	return NULL;
 }
 
+static int enetc_psfp_policer_validate(const struct flow_action *action,
+				       const struct flow_action_entry *act,
+				       struct netlink_ext_ack *extack)
+{
+	if (act->police.exceed.act_id != FLOW_ACTION_DROP) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "Offload not supported when exceed action is not drop");
+		return -EOPNOTSUPP;
+	}
+
+	if (act->police.notexceed.act_id != FLOW_ACTION_PIPE &&
+	    act->police.notexceed.act_id != FLOW_ACTION_ACCEPT) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "Offload not supported when conform action is not pipe or ok");
+		return -EOPNOTSUPP;
+	}
+
+	if (act->police.notexceed.act_id == FLOW_ACTION_ACCEPT &&
+	    !flow_action_is_last_entry(action, act)) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "Offload not supported when conform action is ok, but action is not last");
+		return -EOPNOTSUPP;
+	}
+
+	if (act->police.peakrate_bytes_ps ||
+	    act->police.avrate || act->police.overhead) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "Offload not supported when peakrate/avrate/overhead is configured");
+		return -EOPNOTSUPP;
+	}
+
+	if (act->police.rate_pkt_ps) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "QoS offload not support packets per second");
+		return -EOPNOTSUPP;
+	}
+
+	return 0;
+}
+
 static int enetc_psfp_parse_clsflower(struct enetc_ndev_priv *priv,
 				      struct flow_cls_offload *f)
 {
@@ -1230,11 +1270,10 @@ static int enetc_psfp_parse_clsflower(struct enetc_ndev_priv *priv,
 
 	/* Flow meter and max frame size */
 	if (entryp) {
-		if (entryp->police.rate_pkt_ps) {
-			NL_SET_ERR_MSG_MOD(extack, "QoS offload not support packets per second");
-			err = -EOPNOTSUPP;
+		err = enetc_psfp_policer_validate(&rule->action, entryp, extack);
+		if (err)
 			goto free_sfi;
-		}
+
 		if (entryp->police.burst) {
 			fmi = kzalloc(sizeof(*fmi), GFP_KERNEL);
 			if (!fmi) {
diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c
index 626961a41089..8c90be81677d 100644
--- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c
+++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c
@@ -190,6 +190,40 @@ static int otx2_tc_validate_flow(struct otx2_nic *nic,
 	return 0;
 }
 
+static int otx2_policer_validate(const struct flow_action *action,
+				 const struct flow_action_entry *act,
+				 struct netlink_ext_ack *extack)
+{
+	if (act->police.exceed.act_id != FLOW_ACTION_DROP) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "Offload not supported when exceed action is not drop");
+		return -EOPNOTSUPP;
+	}
+
+	if (act->police.notexceed.act_id != FLOW_ACTION_PIPE &&
+	    act->police.notexceed.act_id != FLOW_ACTION_ACCEPT) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "Offload not supported when conform action is not pipe or ok");
+		return -EOPNOTSUPP;
+	}
+
+	if (act->police.notexceed.act_id == FLOW_ACTION_ACCEPT &&
+	    !flow_action_is_last_entry(action, act)) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "Offload not supported when conform action is ok, but action is not last");
+		return -EOPNOTSUPP;
+	}
+
+	if (act->police.peakrate_bytes_ps ||
+	    act->police.avrate || act->police.overhead) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "Offload not supported when peakrate/avrate/overhead is configured");
+		return -EOPNOTSUPP;
+	}
+
+	return 0;
+}
+
 static int otx2_tc_egress_matchall_install(struct otx2_nic *nic,
 					   struct tc_cls_matchall_offload *cls)
 {
@@ -212,6 +246,10 @@ static int otx2_tc_egress_matchall_install(struct otx2_nic *nic,
 	entry = &cls->rule->action.entries[0];
 	switch (entry->id) {
 	case FLOW_ACTION_POLICE:
+		err = otx2_policer_validate(&cls->rule->action, entry, extack);
+		if (err)
+			return err;
+
 		if (entry->police.rate_pkt_ps) {
 			NL_SET_ERR_MSG_MOD(extack, "QoS offload not support packets per second");
 			return -EOPNOTSUPP;
@@ -315,6 +353,7 @@ static int otx2_tc_parse_actions(struct otx2_nic *nic,
 	u8 nr_police = 0;
 	bool pps = false;
 	u64 rate;
+	int err;
 	int i;
 
 	if (!flow_action_has_entries(flow_action)) {
@@ -355,6 +394,10 @@ static int otx2_tc_parse_actions(struct otx2_nic *nic,
 				return -EOPNOTSUPP;
 			}
 
+			err = otx2_policer_validate(flow_action, act, extack);
+			if (err)
+				return err;
+
 			if (act->police.rate_bytes_ps > 0) {
 				rate = act->police.rate_bytes_ps * 8;
 				burst = act->police.burst;
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
index 1287193a019b..69a2f8d52b2b 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
@@ -4170,6 +4170,46 @@ static int apply_police_params(struct mlx5e_priv *priv, u64 rate,
 	return err;
 }
 
+static int mlx5e_policer_validate(const struct flow_action *action,
+				  const struct flow_action_entry *act,
+				  struct netlink_ext_ack *extack)
+{
+	if (act->police.exceed.act_id != FLOW_ACTION_DROP) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "Offload not supported when exceed action is not drop");
+		return -EOPNOTSUPP;
+	}
+
+	if (act->police.notexceed.act_id != FLOW_ACTION_PIPE &&
+	    act->police.notexceed.act_id != FLOW_ACTION_ACCEPT) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "Offload not supported when conform action is not pipe or ok");
+		return -EOPNOTSUPP;
+	}
+
+	if (act->police.notexceed.act_id == FLOW_ACTION_ACCEPT &&
+	    !flow_action_is_last_entry(action, act)) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "Offload not supported when conform action is ok, but action is not last");
+		return -EOPNOTSUPP;
+	}
+
+	if (act->police.peakrate_bytes_ps ||
+	    act->police.avrate || act->police.overhead) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "Offload not supported when peakrate/avrate/overhead is configured");
+		return -EOPNOTSUPP;
+	}
+
+	if (act->police.rate_pkt_ps) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "QoS offload not support packets per second");
+		return -EOPNOTSUPP;
+	}
+
+	return 0;
+}
+
 static int scan_tc_matchall_fdb_actions(struct mlx5e_priv *priv,
 					struct flow_action *flow_action,
 					struct netlink_ext_ack *extack)
@@ -4197,10 +4237,10 @@ static int scan_tc_matchall_fdb_actions(struct mlx5e_priv *priv,
 	flow_action_for_each(i, act, flow_action) {
 		switch (act->id) {
 		case FLOW_ACTION_POLICE:
-			if (act->police.rate_pkt_ps) {
-				NL_SET_ERR_MSG_MOD(extack, "QoS offload not support packets per second");
-				return -EOPNOTSUPP;
-			}
+			err = mlx5e_policer_validate(flow_action, act, extack);
+			if (err)
+				return err;
+
 			err = apply_police_params(priv, act->police.rate_bytes_ps, extack);
 			if (err)
 				return err;
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c
index bb417db773b9..8c31b73088bb 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c
@@ -15,6 +15,46 @@
 #include "spectrum.h"
 #include "core_acl_flex_keys.h"
 
+static int mlxsw_sp_policer_validate(const struct flow_action *action,
+				     const struct flow_action_entry *act,
+				     struct netlink_ext_ack *extack)
+{
+	if (act->police.exceed.act_id != FLOW_ACTION_DROP) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "Offload not supported when exceed action is not drop");
+		return -EOPNOTSUPP;
+	}
+
+	if (act->police.notexceed.act_id != FLOW_ACTION_PIPE &&
+	    act->police.notexceed.act_id != FLOW_ACTION_ACCEPT) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "Offload not supported when conform action is not pipe or ok");
+		return -EOPNOTSUPP;
+	}
+
+	if (act->police.notexceed.act_id == FLOW_ACTION_ACCEPT &&
+	    !flow_action_is_last_entry(action, act)) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "Offload not supported when conform action is ok, but action is not last");
+		return -EOPNOTSUPP;
+	}
+
+	if (act->police.peakrate_bytes_ps ||
+	    act->police.avrate || act->police.overhead) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "Offload not supported when peakrate/avrate/overhead is configured");
+		return -EOPNOTSUPP;
+	}
+
+	if (act->police.rate_pkt_ps) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "QoS offload not support packets per second");
+		return -EOPNOTSUPP;
+	}
+
+	return 0;
+}
+
 static int mlxsw_sp_flower_parse_actions(struct mlxsw_sp *mlxsw_sp,
 					 struct mlxsw_sp_flow_block *block,
 					 struct mlxsw_sp_acl_rule_info *rulei,
@@ -191,10 +231,9 @@ static int mlxsw_sp_flower_parse_actions(struct mlxsw_sp *mlxsw_sp,
 				return -EOPNOTSUPP;
 			}
 
-			if (act->police.rate_pkt_ps) {
-				NL_SET_ERR_MSG_MOD(extack, "QoS offload not support packets per second");
-				return -EOPNOTSUPP;
-			}
+			err = mlxsw_sp_policer_validate(flow_action, act, extack);
+			if (err)
+				return err;
 
 			/* The kernel might adjust the requested burst size so
 			 * that it is not exactly a power of two. Re-adjust it
diff --git a/drivers/net/ethernet/mscc/ocelot_flower.c b/drivers/net/ethernet/mscc/ocelot_flower.c
index 949858891973..70191ba9d8af 100644
--- a/drivers/net/ethernet/mscc/ocelot_flower.c
+++ b/drivers/net/ethernet/mscc/ocelot_flower.c
@@ -6,6 +6,7 @@
 #include <net/pkt_cls.h>
 #include <net/tc_act/tc_gact.h>
 #include <soc/mscc/ocelot_vcap.h>
+#include "ocelot_police.h"
 #include "ocelot_vcap.h"
 
 /* Arbitrarily chosen constants for encoding the VCAP block and lookup number
@@ -217,6 +218,7 @@ static int ocelot_flower_parse_action(struct ocelot *ocelot, int port,
 				      bool ingress, struct flow_cls_offload *f,
 				      struct ocelot_vcap_filter *filter)
 {
+	const struct flow_action *action = &f->rule->action;
 	struct netlink_ext_ack *extack = f->common.extack;
 	bool allow_missing_goto_target = false;
 	const struct flow_action_entry *a;
@@ -244,7 +246,7 @@ static int ocelot_flower_parse_action(struct ocelot *ocelot, int port,
 	filter->goto_target = -1;
 	filter->type = OCELOT_VCAP_FILTER_DUMMY;
 
-	flow_action_for_each(i, a, &f->rule->action) {
+	flow_action_for_each(i, a, action) {
 		switch (a->id) {
 		case FLOW_ACTION_DROP:
 			if (filter->block_id != VCAP_IS2) {
@@ -296,11 +298,11 @@ static int ocelot_flower_parse_action(struct ocelot *ocelot, int port,
 						   "Last action must be GOTO");
 				return -EOPNOTSUPP;
 			}
-			if (a->police.rate_pkt_ps) {
-				NL_SET_ERR_MSG_MOD(extack,
-						   "QoS offload not support packets per second");
-				return -EOPNOTSUPP;
-			}
+
+			err = ocelot_policer_validate(action, a, extack);
+			if (err)
+				return err;
+
 			filter->action.police_ena = true;
 
 			pol_ix = a->hw_index + ocelot->vcap_pol.base;
diff --git a/drivers/net/ethernet/mscc/ocelot_net.c b/drivers/net/ethernet/mscc/ocelot_net.c
index e271b6225b72..e6467831d05a 100644
--- a/drivers/net/ethernet/mscc/ocelot_net.c
+++ b/drivers/net/ethernet/mscc/ocelot_net.c
@@ -14,6 +14,7 @@
 #include <linux/phy/phy.h>
 #include <net/pkt_cls.h>
 #include "ocelot.h"
+#include "ocelot_police.h"
 #include "ocelot_vcap.h"
 #include "ocelot_fdma.h"
 
@@ -258,11 +259,10 @@ static int ocelot_setup_tc_cls_matchall(struct ocelot_port_private *priv,
 			return -EEXIST;
 		}
 
-		if (action->police.rate_pkt_ps) {
-			NL_SET_ERR_MSG_MOD(extack,
-					   "QoS offload not support packets per second");
-			return -EOPNOTSUPP;
-		}
+		err = ocelot_policer_validate(&f->rule->action, action,
+					      extack);
+		if (err)
+			return err;
 
 		pol.rate = (u32)div_u64(action->police.rate_bytes_ps, 1000) * 8;
 		pol.burst = action->police.burst;
diff --git a/drivers/net/ethernet/mscc/ocelot_police.c b/drivers/net/ethernet/mscc/ocelot_police.c
index 6f5068c1041a..a65606bb84a0 100644
--- a/drivers/net/ethernet/mscc/ocelot_police.c
+++ b/drivers/net/ethernet/mscc/ocelot_police.c
@@ -154,6 +154,47 @@ int qos_policer_conf_set(struct ocelot *ocelot, int port, u32 pol_ix,
 	return 0;
 }
 
+int ocelot_policer_validate(const struct flow_action *action,
+			    const struct flow_action_entry *a,
+			    struct netlink_ext_ack *extack)
+{
+	if (a->police.exceed.act_id != FLOW_ACTION_DROP) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "Offload not supported when exceed action is not drop");
+		return -EOPNOTSUPP;
+	}
+
+	if (a->police.notexceed.act_id != FLOW_ACTION_PIPE &&
+	    a->police.notexceed.act_id != FLOW_ACTION_ACCEPT) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "Offload not supported when conform action is not pipe or ok");
+		return -EOPNOTSUPP;
+	}
+
+	if (a->police.notexceed.act_id == FLOW_ACTION_ACCEPT &&
+	    !flow_action_is_last_entry(action, a)) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "Offload not supported when conform action is ok, but police action is not last");
+		return -EOPNOTSUPP;
+	}
+
+	if (a->police.peakrate_bytes_ps ||
+	    a->police.avrate || a->police.overhead) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "Offload not supported when peakrate/avrate/overhead is configured");
+		return -EOPNOTSUPP;
+	}
+
+	if (a->police.rate_pkt_ps) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "Offload does not support packets per second");
+		return -EOPNOTSUPP;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL(ocelot_policer_validate);
+
 int ocelot_port_policer_add(struct ocelot *ocelot, int port,
 			    struct ocelot_policer *pol)
 {
diff --git a/drivers/net/ethernet/mscc/ocelot_police.h b/drivers/net/ethernet/mscc/ocelot_police.h
index 7adb05f71999..7552995f8b17 100644
--- a/drivers/net/ethernet/mscc/ocelot_police.h
+++ b/drivers/net/ethernet/mscc/ocelot_police.h
@@ -8,6 +8,7 @@
 #define _MSCC_OCELOT_POLICE_H_
 
 #include "ocelot.h"
+#include <net/flow_offload.h>
 
 enum mscc_qos_rate_mode {
 	MSCC_QOS_RATE_MODE_DISABLED, /* Policer/shaper disabled */
@@ -33,4 +34,8 @@ struct qos_policer_conf {
 int qos_policer_conf_set(struct ocelot *ocelot, int port, u32 pol_ix,
 			 struct qos_policer_conf *conf);
 
+int ocelot_policer_validate(const struct flow_action *action,
+			    const struct flow_action_entry *a,
+			    struct netlink_ext_ack *extack);
+
 #endif /* _MSCC_OCELOT_POLICE_H_ */
diff --git a/drivers/net/ethernet/netronome/nfp/flower/qos_conf.c b/drivers/net/ethernet/netronome/nfp/flower/qos_conf.c
index 784c6dbf8bc4..6c8f0e114307 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/qos_conf.c
+++ b/drivers/net/ethernet/netronome/nfp/flower/qos_conf.c
@@ -67,6 +67,40 @@ struct nfp_police_stats_reply {
 	__be64 drop_pkts;
 };
 
+static int nfp_policer_validate(const struct flow_action *action,
+				const struct flow_action_entry *act,
+				struct netlink_ext_ack *extack)
+{
+	if (act->police.exceed.act_id != FLOW_ACTION_DROP) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "Offload not supported when exceed action is not drop");
+		return -EOPNOTSUPP;
+	}
+
+	if (act->police.notexceed.act_id != FLOW_ACTION_PIPE &&
+	    act->police.notexceed.act_id != FLOW_ACTION_ACCEPT) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "Offload not supported when conform action is not pipe or ok");
+		return -EOPNOTSUPP;
+	}
+
+	if (act->police.notexceed.act_id == FLOW_ACTION_ACCEPT &&
+	    !flow_action_is_last_entry(action, act)) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "Offload not supported when conform action is ok, but action is not last");
+		return -EOPNOTSUPP;
+	}
+
+	if (act->police.peakrate_bytes_ps ||
+	    act->police.avrate || act->police.overhead) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "Offload not supported when peakrate/avrate/overhead is configured");
+		return -EOPNOTSUPP;
+	}
+
+	return 0;
+}
+
 static int
 nfp_flower_install_rate_limiter(struct nfp_app *app, struct net_device *netdev,
 				struct tc_cls_matchall_offload *flow,
@@ -86,6 +120,7 @@ nfp_flower_install_rate_limiter(struct nfp_app *app, struct net_device *netdev,
 	u32 pps_num = 0;
 	u32 burst;
 	u64 rate;
+	int err;
 
 	if (!nfp_netdev_is_nfp_repr(netdev)) {
 		NL_SET_ERR_MSG_MOD(extack, "unsupported offload: qos rate limit offload not supported on higher level port");
@@ -132,6 +167,11 @@ nfp_flower_install_rate_limiter(struct nfp_app *app, struct net_device *netdev,
 					   "unsupported offload: qos rate limit offload requires police action");
 			return -EOPNOTSUPP;
 		}
+
+		err = nfp_policer_validate(&flow->rule->action, action, extack);
+		if (err)
+			return err;
+
 		if (action->police.rate_bytes_ps > 0) {
 			if (bps_num++) {
 				NL_SET_ERR_MSG_MOD(extack,
diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
index 74f44d44abe3..92267d23083e 100644
--- a/include/net/flow_offload.h
+++ b/include/net/flow_offload.h
@@ -311,6 +311,12 @@ static inline bool flow_offload_has_one_action(const struct flow_action *action)
 	return action->num_entries == 1;
 }
 
+static inline bool flow_action_is_last_entry(const struct flow_action *action,
+					     const struct flow_action_entry *entry)
+{
+	return entry == &action->entries[action->num_entries - 1];
+}
+
 #define flow_action_for_each(__i, __act, __actions)			\
         for (__i = 0, __act = &(__actions)->entries[0];			\
 	     __i < (__actions)->num_entries;				\
-- 
2.26.2

Re: [PATCH net-next v3 0/2] flow_offload: add tc police parameters

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (master)
by David S. Miller <davem@davemloft.net>:

On Thu, 24 Feb 2022 10:29:06 +0000 you wrote:
> As a preparation for more advanced police offload in mlx5 (e.g.,
> jumping to another chain when bandwidth is not exceeded), extend the
> flow offload API with more tc-police parameters. Adjust existing
> drivers to reject unsupported configurations.
> 
> Changes since v2:
>   * Rename index to extval in exceed and notexceed acts.
>   * Add policer validate functions for all drivers.
> 
> [...]

Here is the summary with links:
  - [net-next,v3,1/2] net: flow_offload: add tc police action parameters
    https://git.kernel.org/netdev/net-next/c/b8cd5831c61c
  - [net-next,v3,2/2] flow_offload: reject offload for all drivers with invalid police parameters
    (no matching commit)

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html