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

[PATCH net-next v2 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 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      | 27 +++++++++
 .../chelsio/cxgb4/cxgb4_tc_matchall.c         | 55 +++++++++++++++++++
 .../net/ethernet/freescale/enetc/enetc_qos.c  | 31 +++++++++++
 .../ethernet/marvell/octeontx2/nic/otx2_tc.c  | 54 ++++++++++++++++++
 .../net/ethernet/mellanox/mlx5/core/en_tc.c   | 27 +++++++++
 .../ethernet/mellanox/mlxsw/spectrum_flower.c | 27 +++++++++
 drivers/net/ethernet/mscc/ocelot_flower.c     | 28 ++++++++++
 drivers/net/ethernet/mscc/ocelot_net.c        | 27 +++++++++
 .../ethernet/netronome/nfp/flower/qos_conf.c  | 28 ++++++++++
 include/net/flow_offload.h                    | 19 +++++++
 include/net/tc_act/tc_police.h                | 30 ++++++++++
 net/sched/act_police.c                        | 46 ++++++++++++++++
 12 files changed, 399 insertions(+)

-- 
2.26.2

[PATCH net-next v2 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     | 13 ++++++++++
 include/net/tc_act/tc_police.h | 30 ++++++++++++++++++++++
 net/sched/act_police.c         | 46 ++++++++++++++++++++++++++++++++++
 3 files changed, 89 insertions(+)

diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
index 5b8c54eb7a6b..94cde6bbc8a5 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,20 @@ 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                     index;
+			} exceed;
+			struct {
+				enum flow_action_id     act_id;
+				u32                     index;
+			} 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..0457b6c9c4e7 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, int *index)
+{
+	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;
+		*index = tc_act & TC_ACT_EXT_VAL_MASK;
+	} else if (TC_ACT_EXT_CMP(tc_act, TC_ACT_JUMP)) {
+		act_id = FLOW_ACTION_JUMP;
+		*index = 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.index);
+		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.index);
+		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 v2 1/2] net: flow_offload: add tc police action parameters

[Baowen Zheng]

On February 17, 2022 4:28 PM, Jianbo 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     | 13 ++++++++++
> include/net/tc_act/tc_police.h | 30 ++++++++++++++++++++++
> net/sched/act_police.c         | 46 ++++++++++++++++++++++++++++++++++
> 3 files changed, 89 insertions(+)
>
>diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h index
>5b8c54eb7a6b..94cde6bbc8a5 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,20 @@ 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                     index;
>+			} exceed;
>+			struct {
>+				enum flow_action_id     act_id;
>+				u32                     index;
>+			} notexceed;
It seems exceed and notexceed use the same format struct, will it be more simpler to define as:
			struct {
				enum flow_action_id     act_id;
				u32                     index;
			} 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..0457b6c9c4e7 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, int *index) {
>+	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;
>+		*index = tc_act & TC_ACT_EXT_VAL_MASK;
For the TC_ACT_GOTO_CHAIN  action, the goto_chain information is missing from software to hardware, is it useful for hardware to check?

>+	} else if (TC_ACT_EXT_CMP(tc_act, TC_ACT_JUMP)) {
>+		act_id = FLOW_ACTION_JUMP;
>+		*index = 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.index);
>+		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.index);
>+		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 v2 1/2] net: flow_offload: add tc police action parameters

[Roi Dayan]

On 2022-02-17 12:25 PM, Baowen Zheng wrote:
> On February 17, 2022 4:28 PM, Jianbo 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     | 13 ++++++++++
>> include/net/tc_act/tc_police.h | 30 ++++++++++++++++++++++
>> net/sched/act_police.c         | 46 ++++++++++++++++++++++++++++++++++
>> 3 files changed, 89 insertions(+)
>>
>> diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h index
>> 5b8c54eb7a6b..94cde6bbc8a5 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,20 @@ 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                     index;
>> +			} exceed;
>> +			struct {
>> +				enum flow_action_id     act_id;
>> +				u32                     index;
>> +			} notexceed;
> It seems exceed and notexceed use the same format struct, will it be more simpler to define as:
> 			struct {
> 				enum flow_action_id     act_id;
> 				u32                     index;
> 			} exceed, notexceed;

right. it can be.

> 
>> 		} 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..0457b6c9c4e7 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, int *index) {
>> +	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;
>> +		*index = tc_act & TC_ACT_EXT_VAL_MASK;
> For the TC_ACT_GOTO_CHAIN  action, the goto_chain information is missing from software to hardware, is it useful for hardware to check?
> 

what information do you mean?

>> +	} else if (TC_ACT_EXT_CMP(tc_act, TC_ACT_JUMP)) {
>> +		act_id = FLOW_ACTION_JUMP;
>> +		*index = 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.index);
>> +		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.index);
>> +		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 v2 1/2] net: flow_offload: add tc police action parameters

[Baowen Zheng]

On, February 17, 2022 8:10 PM, Roi wrote:
>On 2022-02-17 12:25 PM, Baowen Zheng wrote:
>> On February 17, 2022 4:28 PM, Jianbo 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     | 13 ++++++++++
>>> include/net/tc_act/tc_police.h | 30 ++++++++++++++++++++++
>>> net/sched/act_police.c         | 46
>++++++++++++++++++++++++++++++++++
>>> 3 files changed, 89 insertions(+)
>>>
>>> diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
>>> index
>>> 5b8c54eb7a6b..94cde6bbc8a5 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,20 @@ 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                     index;
>>> +			} exceed;
>>> +			struct {
>>> +				enum flow_action_id     act_id;
>>> +				u32                     index;
>>> +			} notexceed;
>> It seems exceed and notexceed use the same format struct, will it be more
>simpler to define as:
>> 			struct {
>> 				enum flow_action_id     act_id;
>> 				u32                     index;
>> 			} exceed, notexceed;
>
>right. it can be.
>
>>
>>> 		} 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..0457b6c9c4e7 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, int *index) {
>>> +	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;
>>> +		*index = tc_act & TC_ACT_EXT_VAL_MASK;
>> For the TC_ACT_GOTO_CHAIN  action, the goto_chain information is missing
>from software to hardware, is it useful for hardware to check?
>>
>
>what information do you mean?
Sorry, I do not realize the chain index is in the return value of index, so please just ignore.
It seems the definition of index is a little confused since in TC_ACT_GOTO_CHAIN case, it means chain index and in TC_ACT_JUMP case, it means jump count. 
Just a suggestion, can we change the index definition as a union as:
	union {
			u32 chain_index;
			u32 jmp_cnt;
		{
WDYT?
>
>>> +	} else if (TC_ACT_EXT_CMP(tc_act, TC_ACT_JUMP)) {
>>> +		act_id = FLOW_ACTION_JUMP;
>>> +		*index = 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.index);
>>> +		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.index);
>>> +		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 v2 1/2] net: flow_offload: add tc police action parameters

[Jianbo Liu]

On Fri, 2022-02-18 at 01:46 +0000, Baowen Zheng wrote:
> On, February 17, 2022 8:10 PM, Roi wrote:
> > On 2022-02-17 12:25 PM, Baowen Zheng wrote:
> > > On February 17, 2022 4:28 PM, Jianbo 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     | 13 ++++++++++
> > > > include/net/tc_act/tc_police.h | 30 ++++++++++++++++++++++
> > > > net/sched/act_police.c         | 46
> > ++++++++++++++++++++++++++++++++++
> > > > 3 files changed, 89 insertions(+)
> > > > 
> > > > diff --git a/include/net/flow_offload.h
> > > > b/include/net/flow_offload.h
> > > > index
> > > > 5b8c54eb7a6b..94cde6bbc8a5 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,20 @@ 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                     index;
> > > > +                       } exceed;
> > > > +                       struct {
> > > > +                               enum flow_action_id     act_id;
> > > > +                               u32                     index;
> > > > +                       } notexceed;
> > > It seems exceed and notexceed use the same format struct, will it
> > > be more
> > simpler to define as:
> > >                         struct {
> > >                                 enum flow_action_id     act_id;
> > >                                 u32                     index;
> > >                         } exceed, notexceed;
> > 
> > right. it can be.
> > 
> > > 
> > > >                 } 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..0457b6c9c4e7 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, int *index)
> > > > {
> > > > +       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;
> > > > +               *index = tc_act & TC_ACT_EXT_VAL_MASK;
> > > For the TC_ACT_GOTO_CHAIN  action, the goto_chain information is
> > > missing
> > from software to hardware, is it useful for hardware to check?
> > > 
> > 
> > what information do you mean?
> Sorry, I do not realize the chain index is in the return value of
> index, so please just ignore.

OK

> It seems the definition of index is a little confused since in
> TC_ACT_GOTO_CHAIN case, it means chain index and in TC_ACT_JUMP case,
> it means jump count. 
> Just a suggestion, can we change the index definition as a union as:
>         union {
>                         u32 chain_index;
>                         u32 jmp_cnt;
>                 {
> WDYT?
> 

Yes, we will consider that. Thanks!

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

[Jianbo Liu]

On Fri, 2022-02-18 at 01:46 +0000, Baowen Zheng wrote:
> On, February 17, 2022 8:10 PM, Roi wrote:
> > On 2022-02-17 12:25 PM, Baowen Zheng wrote:
> > > On February 17, 2022 4:28 PM, Jianbo 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     | 13 ++++++++++
> > > > include/net/tc_act/tc_police.h | 30 ++++++++++++++++++++++
> > > > net/sched/act_police.c         | 46
> > ++++++++++++++++++++++++++++++++++
> > > > 3 files changed, 89 insertions(+)
> > > > 
> > > > diff --git a/include/net/flow_offload.h
> > > > b/include/net/flow_offload.h
> > > > index
> > > > 5b8c54eb7a6b..94cde6bbc8a5 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,20 @@ 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                     index;
> > > > +                       } exceed;
> > > > +                       struct {
> > > > +                               enum flow_action_id     act_id;
> > > > +                               u32                     index;
> > > > +                       } notexceed;
> > > It seems exceed and notexceed use the same format struct, will it
> > > be more
> > simpler to define as:
> > >                         struct {
> > >                                 enum flow_action_id     act_id;
> > >                                 u32                     index;
> > >                         } exceed, notexceed;
> > 
> > right. it can be.
> > 
> > > 
> > > >                 } 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..0457b6c9c4e7 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, int *index)
> > > > {
> > > > +       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;
> > > > +               *index = tc_act & TC_ACT_EXT_VAL_MASK;
> > > For the TC_ACT_GOTO_CHAIN  action, the goto_chain information is
> > > missing
> > from software to hardware, is it useful for hardware to check?
> > > 
> > 
> > what information do you mean?
> Sorry, I do not realize the chain index is in the return value of
> index, so please just ignore.
> It seems the definition of index is a little confused since in
> TC_ACT_GOTO_CHAIN case, it means chain index and in TC_ACT_JUMP case,
> it means jump count. 
> Just a suggestion, can we change the index definition as a union as:
>         union {
>                         u32 chain_index;
>                         u32 jmp_cnt;
>                 {
> WDYT?
> > 

Hi Baowen, 
If changing to inline union, either the pointer of chain_index or
jmp_cnt should be passed to tcf_police_act_to_flow_act(). But the
caller doesn't know which one to use, because it doesn't know if the
action is goto or jump.   
Besides, it's not a must as we alreay know what type the action is from
act_id. So what about just renaming index to extval?

Thanks!
Jianbo

> > > > +       } else if (TC_ACT_EXT_CMP(tc_act, TC_ACT_JUMP)) {
> > > > +               act_id = FLOW_ACTION_JUMP;
> > > > +               *index = 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.index);
> > > > +               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.index);
> > > > +               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
> > > 

[PATCH net-next v2 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: Roi Dayan <roid@nvidia.com>
Reviewed-by: Ido Schimmel <idosch@nvidia.com>
---
 drivers/net/dsa/sja1105/sja1105_flower.c      | 27 +++++++++
 .../chelsio/cxgb4/cxgb4_tc_matchall.c         | 55 +++++++++++++++++++
 .../net/ethernet/freescale/enetc/enetc_qos.c  | 31 +++++++++++
 .../ethernet/marvell/octeontx2/nic/otx2_tc.c  | 54 ++++++++++++++++++
 .../net/ethernet/mellanox/mlx5/core/en_tc.c   | 27 +++++++++
 .../ethernet/mellanox/mlxsw/spectrum_flower.c | 27 +++++++++
 drivers/net/ethernet/mscc/ocelot_flower.c     | 28 ++++++++++
 drivers/net/ethernet/mscc/ocelot_net.c        | 27 +++++++++
 .../ethernet/netronome/nfp/flower/qos_conf.c  | 28 ++++++++++
 include/net/flow_offload.h                    |  6 ++
 10 files changed, 310 insertions(+)

diff --git a/drivers/net/dsa/sja1105/sja1105_flower.c b/drivers/net/dsa/sja1105/sja1105_flower.c
index 7dcdd784aea4..8a14df8cf91e 100644
--- a/drivers/net/dsa/sja1105/sja1105_flower.c
+++ b/drivers/net/dsa/sja1105/sja1105_flower.c
@@ -321,6 +321,33 @@ 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.exceed.act_id != FLOW_ACTION_DROP) {
+				NL_SET_ERR_MSG_MOD(extack,
+						   "Police offload is not supported when the 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,
+						   "Police offload is not supported when the conform action is not pipe or ok");
+				return -EOPNOTSUPP;
+			}
+
+			if (act->police.notexceed.act_id == FLOW_ACTION_ACCEPT &&
+			    !flow_action_is_last_entry(&rule->action, act)) {
+				NL_SET_ERR_MSG_MOD(extack,
+						   "Police offload is not supported when the conform action is ok, but police action is not last");
+				return -EOPNOTSUPP;
+			}
+
+			if (act->police.peakrate_bytes_ps ||
+			    act->police.avrate || act->police.overhead) {
+				NL_SET_ERR_MSG_MOD(extack,
+						   "Police offload is 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");
diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_matchall.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_matchall.c
index 28fd2de9e4cf..124e65116b2d 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_matchall.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_matchall.c
@@ -48,6 +48,33 @@ 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.exceed.act_id != FLOW_ACTION_DROP) {
+				NL_SET_ERR_MSG_MOD(extack,
+						   "Police offload is not supported when the exceed action is not drop");
+				return -EOPNOTSUPP;
+			}
+
+			if (entry->police.notexceed.act_id != FLOW_ACTION_PIPE &&
+			    entry->police.notexceed.act_id != FLOW_ACTION_ACCEPT) {
+				NL_SET_ERR_MSG_MOD(extack,
+						   "Police offload is not supported when the conform action is not pipe or ok");
+				return -EOPNOTSUPP;
+			}
+
+			if (entry->police.notexceed.act_id == FLOW_ACTION_ACCEPT &&
+			    !flow_action_is_last_entry(actions, entry)) {
+				NL_SET_ERR_MSG_MOD(extack,
+						   "Police offload is not supported when the conform action is ok, but police action is not last");
+				return -EOPNOTSUPP;
+			}
+
+			if (entry->police.peakrate_bytes_ps ||
+			    entry->police.avrate || entry->police.overhead) {
+				NL_SET_ERR_MSG_MOD(extack,
+						   "Police offload is not supported when peakrate/avrate/overhead is configured");
+				return -EOPNOTSUPP;
+			}
+
 			if (entry->police.rate_pkt_ps) {
 				NL_SET_ERR_MSG_MOD(extack,
 						   "QoS offload not support packets per second");
@@ -150,6 +177,34 @@ 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.exceed.act_id != FLOW_ACTION_DROP) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "Police offload is not supported when the exceed action is not drop");
+		return -EOPNOTSUPP;
+	}
+
+	if (entry->police.notexceed.act_id != FLOW_ACTION_PIPE &&
+	    entry->police.notexceed.act_id != FLOW_ACTION_ACCEPT) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "Police offload is not supported when the conform action is not pipe or ok");
+		return -EOPNOTSUPP;
+	}
+
+	if (entry->police.notexceed.act_id == FLOW_ACTION_ACCEPT &&
+	    !flow_action_is_last_entry(&cls->rule->action, entry)) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "Police offload is not supported when the conform action is ok, but police action is not last");
+		return -EOPNOTSUPP;
+	}
+
+	if (entry->police.peakrate_bytes_ps ||
+	    entry->police.avrate || entry->police.overhead) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "Police offload is not supported when peakrate/avrate/overhead is configured");
+		return -EOPNOTSUPP;
+	}
+
 	if (entry->police.rate_pkt_ps) {
 		NL_SET_ERR_MSG_MOD(extack,
 				   "QoS offload not support packets per second");
diff --git a/drivers/net/ethernet/freescale/enetc/enetc_qos.c b/drivers/net/ethernet/freescale/enetc/enetc_qos.c
index 3555c12edb45..c992d9e86467 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc_qos.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc_qos.c
@@ -1230,6 +1230,37 @@ static int enetc_psfp_parse_clsflower(struct enetc_ndev_priv *priv,
 
 	/* Flow meter and max frame size */
 	if (entryp) {
+		if (entryp->police.exceed.act_id != FLOW_ACTION_DROP) {
+			NL_SET_ERR_MSG_MOD(extack,
+					   "Police offload is not supported when the exceed action is not drop");
+			err = -EOPNOTSUPP;
+			goto free_sfi;
+		}
+
+		if (entryp->police.notexceed.act_id != FLOW_ACTION_PIPE &&
+		    entryp->police.notexceed.act_id != FLOW_ACTION_ACCEPT) {
+			NL_SET_ERR_MSG_MOD(extack,
+					   "Police offload is not supported when the conform action is not pipe or ok");
+			err = -EOPNOTSUPP;
+			goto free_sfi;
+		}
+
+		if (entryp->police.notexceed.act_id == FLOW_ACTION_ACCEPT &&
+		    !flow_action_is_last_entry(&rule->action, entryp)) {
+			NL_SET_ERR_MSG_MOD(extack,
+					   "Police offload is not supported when the conform action is ok, but police action is not last");
+			err = -EOPNOTSUPP;
+			goto free_sfi;
+		}
+
+		if (entryp->police.peakrate_bytes_ps ||
+		    entryp->police.avrate || entryp->police.overhead) {
+			NL_SET_ERR_MSG_MOD(extack,
+					   "Police offload is not supported when peakrate/avrate/overhead is configured");
+			err = -EOPNOTSUPP;
+			goto free_sfi;
+		}
+
 		if (entryp->police.rate_pkt_ps) {
 			NL_SET_ERR_MSG_MOD(extack, "QoS offload not support packets per second");
 			err = -EOPNOTSUPP;
diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c
index 626961a41089..4b6e17765b2f 100644
--- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c
+++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c
@@ -212,6 +212,33 @@ static int otx2_tc_egress_matchall_install(struct otx2_nic *nic,
 	entry = &cls->rule->action.entries[0];
 	switch (entry->id) {
 	case FLOW_ACTION_POLICE:
+		if (entry->police.exceed.act_id != FLOW_ACTION_DROP) {
+			NL_SET_ERR_MSG_MOD(extack,
+					   "Police offload is not supported when the exceed action is not drop");
+			return -EOPNOTSUPP;
+		}
+
+		if (entry->police.notexceed.act_id != FLOW_ACTION_PIPE &&
+		    entry->police.notexceed.act_id != FLOW_ACTION_ACCEPT) {
+			NL_SET_ERR_MSG_MOD(extack,
+					   "Police offload is not supported when the conform action is not pipe or ok");
+			return -EOPNOTSUPP;
+		}
+
+		if (entry->police.notexceed.act_id == FLOW_ACTION_ACCEPT &&
+		    !flow_action_is_last_entry(&cls->rule->action, entry)) {
+			NL_SET_ERR_MSG_MOD(extack,
+					   "Police offload is not supported when the conform action is ok, but police action is not last");
+			return -EOPNOTSUPP;
+		}
+
+		if (entry->police.peakrate_bytes_ps ||
+		    entry->police.avrate || entry->police.overhead) {
+			NL_SET_ERR_MSG_MOD(extack,
+					   "Police offload is not supported when peakrate/avrate/overhead is configured");
+			return -EOPNOTSUPP;
+		}
+
 		if (entry->police.rate_pkt_ps) {
 			NL_SET_ERR_MSG_MOD(extack, "QoS offload not support packets per second");
 			return -EOPNOTSUPP;
@@ -355,6 +382,33 @@ static int otx2_tc_parse_actions(struct otx2_nic *nic,
 				return -EOPNOTSUPP;
 			}
 
+			if (act->police.exceed.act_id != FLOW_ACTION_DROP) {
+				NL_SET_ERR_MSG_MOD(extack,
+						   "Police offload is not supported when the 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,
+						   "Police offload is not supported when the conform action is not pipe or ok");
+				return -EOPNOTSUPP;
+			}
+
+			if (act->police.notexceed.act_id == FLOW_ACTION_ACCEPT &&
+			    !flow_action_is_last_entry(flow_action, act)) {
+				NL_SET_ERR_MSG_MOD(extack,
+						   "Police offload is not supported when the conform action is ok, but police action is not last");
+				return -EOPNOTSUPP;
+			}
+
+			if (act->police.peakrate_bytes_ps ||
+			    act->police.avrate || act->police.overhead) {
+				NL_SET_ERR_MSG_MOD(extack,
+						   "Police offload is not supported when peakrate/avrate/overhead is configured");
+				return -EOPNOTSUPP;
+			}
+
 			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..7c160961c92e 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
@@ -4197,6 +4197,33 @@ 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.exceed.act_id != FLOW_ACTION_DROP) {
+				NL_SET_ERR_MSG_MOD(extack,
+						   "Police offload is not supported when the 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,
+						   "Police offload is not supported when the conform action is not pipe or ok");
+				return -EOPNOTSUPP;
+			}
+
+			if (act->police.notexceed.act_id == FLOW_ACTION_ACCEPT &&
+			    !flow_action_is_last_entry(flow_action, act)) {
+				NL_SET_ERR_MSG_MOD(extack,
+						   "Police offload is not supported when the conform action is ok, but police action is not last");
+				return -EOPNOTSUPP;
+			}
+
+			if (act->police.peakrate_bytes_ps ||
+			    act->police.avrate || act->police.overhead) {
+				NL_SET_ERR_MSG_MOD(extack,
+						   "Police offload is 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;
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c
index bb417db773b9..b33c3da4daf8 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c
@@ -191,6 +191,33 @@ static int mlxsw_sp_flower_parse_actions(struct mlxsw_sp *mlxsw_sp,
 				return -EOPNOTSUPP;
 			}
 
+			if (act->police.exceed.act_id != FLOW_ACTION_DROP) {
+				NL_SET_ERR_MSG_MOD(extack,
+						   "Police offload is not supported when the 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,
+						   "Police offload is not supported when the conform action is not pipe or ok");
+				return -EOPNOTSUPP;
+			}
+
+			if (act->police.notexceed.act_id == FLOW_ACTION_ACCEPT &&
+			    !flow_action_is_last_entry(flow_action, act)) {
+				NL_SET_ERR_MSG_MOD(extack,
+						   "Police offload is not supported when the conform action is ok, but police action is not last");
+				return -EOPNOTSUPP;
+			}
+
+			if (act->police.peakrate_bytes_ps ||
+			    act->police.avrate || act->police.overhead) {
+				NL_SET_ERR_MSG_MOD(extack,
+						   "Police offload is 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;
diff --git a/drivers/net/ethernet/mscc/ocelot_flower.c b/drivers/net/ethernet/mscc/ocelot_flower.c
index 949858891973..eb478ddb0f90 100644
--- a/drivers/net/ethernet/mscc/ocelot_flower.c
+++ b/drivers/net/ethernet/mscc/ocelot_flower.c
@@ -296,6 +296,34 @@ static int ocelot_flower_parse_action(struct ocelot *ocelot, int port,
 						   "Last action must be GOTO");
 				return -EOPNOTSUPP;
 			}
+
+			if (a->police.exceed.act_id != FLOW_ACTION_DROP) {
+				NL_SET_ERR_MSG_MOD(extack,
+						   "Police offload is not supported when the 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,
+						   "Police offload is not supported when the conform action is not pipe or ok");
+				return -EOPNOTSUPP;
+			}
+
+			if (a->police.notexceed.act_id == FLOW_ACTION_ACCEPT &&
+			    !flow_action_is_last_entry(&f->rule->action, a)) {
+				NL_SET_ERR_MSG_MOD(extack,
+						   "Police offload is not supported when the 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,
+						   "Police offload is not supported when peakrate/avrate/overhead is configured");
+				return -EOPNOTSUPP;
+			}
+
 			if (a->police.rate_pkt_ps) {
 				NL_SET_ERR_MSG_MOD(extack,
 						   "QoS offload not support packets per second");
diff --git a/drivers/net/ethernet/mscc/ocelot_net.c b/drivers/net/ethernet/mscc/ocelot_net.c
index e271b6225b72..69afb560ab98 100644
--- a/drivers/net/ethernet/mscc/ocelot_net.c
+++ b/drivers/net/ethernet/mscc/ocelot_net.c
@@ -258,6 +258,33 @@ static int ocelot_setup_tc_cls_matchall(struct ocelot_port_private *priv,
 			return -EEXIST;
 		}
 
+		if (action->police.exceed.act_id != FLOW_ACTION_DROP) {
+			NL_SET_ERR_MSG_MOD(extack,
+					   "Police offload is not supported when the exceed action is not drop");
+			return -EOPNOTSUPP;
+		}
+
+		if (action->police.notexceed.act_id != FLOW_ACTION_PIPE &&
+		    action->police.notexceed.act_id != FLOW_ACTION_ACCEPT) {
+			NL_SET_ERR_MSG_MOD(extack,
+					   "Police offload is not supported when the conform action is not pipe or ok");
+			return -EOPNOTSUPP;
+		}
+
+		if (action->police.notexceed.act_id == FLOW_ACTION_ACCEPT &&
+		    !flow_action_is_last_entry(&f->rule->action, action)) {
+			NL_SET_ERR_MSG_MOD(extack,
+					   "Police offload is not supported when the conform action is ok, but police action is not last");
+			return -EOPNOTSUPP;
+		}
+
+		if (action->police.peakrate_bytes_ps ||
+		    action->police.avrate || action->police.overhead) {
+			NL_SET_ERR_MSG_MOD(extack,
+					   "Police offload is not supported when peakrate/avrate/overhead is configured");
+			return -EOPNOTSUPP;
+		}
+
 		if (action->police.rate_pkt_ps) {
 			NL_SET_ERR_MSG_MOD(extack,
 					   "QoS offload not support packets per second");
diff --git a/drivers/net/ethernet/netronome/nfp/flower/qos_conf.c b/drivers/net/ethernet/netronome/nfp/flower/qos_conf.c
index 784c6dbf8bc4..247984d50b49 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/qos_conf.c
+++ b/drivers/net/ethernet/netronome/nfp/flower/qos_conf.c
@@ -132,6 +132,34 @@ nfp_flower_install_rate_limiter(struct nfp_app *app, struct net_device *netdev,
 					   "unsupported offload: qos rate limit offload requires police action");
 			return -EOPNOTSUPP;
 		}
+
+		if (action->police.exceed.act_id != FLOW_ACTION_DROP) {
+			NL_SET_ERR_MSG_MOD(extack,
+					   "Police offload is not supported when the exceed action is not drop");
+			return -EOPNOTSUPP;
+		}
+
+		if (action->police.notexceed.act_id != FLOW_ACTION_PIPE &&
+		    action->police.notexceed.act_id != FLOW_ACTION_ACCEPT) {
+			NL_SET_ERR_MSG_MOD(extack,
+					   "Police offload is not supported when the conform action is not pipe or ok");
+			return -EOPNOTSUPP;
+		}
+
+		if (action->police.notexceed.act_id == FLOW_ACTION_ACCEPT &&
+		    !flow_action_is_last_entry(&flow->rule->action, action)) {
+			NL_SET_ERR_MSG_MOD(extack,
+					   "Police offload is not supported when the conform action is ok, but police action is not last");
+			return -EOPNOTSUPP;
+		}
+
+		if (action->police.peakrate_bytes_ps ||
+		    action->police.avrate || action->police.overhead) {
+			NL_SET_ERR_MSG_MOD(extack,
+					   "Police offload is not supported when peakrate/avrate/overhead is configured");
+			return -EOPNOTSUPP;
+		}
+
 		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 94cde6bbc8a5..2b9890177aa1 100644
--- a/include/net/flow_offload.h
+++ b/include/net/flow_offload.h
@@ -315,6 +315,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 v2 2/2] flow_offload: reject offload for all drivers with invalid police parameters

[Vladimir Oltean]

On Thu, Feb 17, 2022 at 08:28:03AM +0000, Jianbo Liu wrote:
> 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: Roi Dayan <roid@nvidia.com>
> Reviewed-by: Ido Schimmel <idosch@nvidia.com>
> ---

Tested-by: Vladimir Oltean <vladimir.oltean@nxp.com>

But could we cut down on line length a little? Example for sja1105
(messages were also shortened):

diff --git a/drivers/net/dsa/sja1105/sja1105_flower.c b/drivers/net/dsa/sja1105/sja1105_flower.c
index 8a14df8cf91e..54a16369a39e 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,39 +361,10 @@ 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.exceed.act_id != FLOW_ACTION_DROP) {
-				NL_SET_ERR_MSG_MOD(extack,
-						   "Police offload is not supported when the 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,
-						   "Police offload is not supported when the conform action is not pipe or ok");
-				return -EOPNOTSUPP;
-			}
-
-			if (act->police.notexceed.act_id == FLOW_ACTION_ACCEPT &&
-			    !flow_action_is_last_entry(&rule->action, act)) {
-				NL_SET_ERR_MSG_MOD(extack,
-						   "Police offload is not supported when the conform action is ok, but police action is not last");
-				return -EOPNOTSUPP;
-			}
-
-			if (act->police.peakrate_bytes_ps ||
-			    act->police.avrate || act->police.overhead) {
-				NL_SET_ERR_MSG_MOD(extack,
-						   "Police offload is 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");
-				rc = -EOPNOTSUPP;
+			rc = sja1105_policer_validate(&rule->action, act,
+						      extack);
+			if (rc)
 				goto out;
-			}
 
 			rc = sja1105_flower_policer(priv, port, extack, cookie,
 						    &key,

Also, if you create a "validate" function for every driver, you'll
remove code duplication for those drivers that support both matchall and
flower policers.

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

[Ido Schimmel]

On Thu, Feb 17, 2022 at 02:49:35PM +0200, Vladimir Oltean wrote:
> Tested-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Thanks for testing

> 
> But could we cut down on line length a little? Example for sja1105
> (messages were also shortened):

No problem

[...]

> Also, if you create a "validate" function for every driver, you'll
> remove code duplication for those drivers that support both matchall and
> flower policers.

Will do

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

[Jianbo Liu]

On Thu, 2022-02-17 at 14:49 +0200, Vladimir Oltean wrote:
> On Thu, Feb 17, 2022 at 08:28:03AM +0000, Jianbo Liu wrote:
> > 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: Roi Dayan <roid@nvidia.com>
> > Reviewed-by: Ido Schimmel <idosch@nvidia.com>
> > ---
> 
> Tested-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> 
> But could we cut down on line length a little? Example for sja1105
> (messages were also shortened):
> 
> diff --git a/drivers/net/dsa/sja1105/sja1105_flower.c
> b/drivers/net/dsa/sja1105/sja1105_flower.c
> index 8a14df8cf91e..54a16369a39e 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,39 +361,10 @@ 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.exceed.act_id !=
> FLOW_ACTION_DROP) {
> -                               NL_SET_ERR_MSG_MOD(extack,
> -                                                  "Police offload is
> not supported when the 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,
> -                                                  "Police offload is
> not supported when the conform action is not pipe or ok");
> -                               return -EOPNOTSUPP;
> -                       }
> -
> -                       if (act->police.notexceed.act_id ==
> FLOW_ACTION_ACCEPT &&
> -                           !flow_action_is_last_entry(&rule->action,
> act)) {
> -                               NL_SET_ERR_MSG_MOD(extack,
> -                                                  "Police offload is
> not supported when the conform action is ok, but police action is not
> last");
> -                               return -EOPNOTSUPP;
> -                       }
> -
> -                       if (act->police.peakrate_bytes_ps ||
> -                           act->police.avrate || act-
> >police.overhead) {
> -                               NL_SET_ERR_MSG_MOD(extack,
> -                                                  "Police offload is
> 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");
> -                               rc = -EOPNOTSUPP;
> +                       rc = sja1105_policer_validate(&rule->action,
> act,
> +                                                     extack);
> +                       if (rc)
>                                 goto out;
> -                       }
>  
>                         rc = sja1105_flower_policer(priv, port,
> extack, cookie,
>                                                     &key,
> 
> Also, if you create a "validate" function for every driver, you'll
> remove code duplication for those drivers that support both matchall
> and
> flower policers.

Hi Vladimir,

I'd love to hear your suggestion regarding where this validate function
to be placed for drivers/net/ethernet/mscc, as it will be used by both
ocelot_net.c and ocelot_flower.c. 

Thanks!
Jianbo

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

[Vladimir Oltean]

[-- Attachment #1: Type: text/plain, Size: 350 bytes --]

Hi Jianbo,

On Tue, Feb 22, 2022 at 01:58:23AM +0000, Jianbo Liu wrote:
> Hi Vladimir,
> 
> I'd love to hear your suggestion regarding where this validate function
> to be placed for drivers/net/ethernet/mscc, as it will be used by both
> ocelot_net.c and ocelot_flower.c. 
> 
> Thanks!
> Jianbo

Try the attached patch on top of yours.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-ocelot-policer-validate.patch --]
[-- Type: text/x-diff; name="0001-ocelot-policer-validate.patch", Size: 7837 bytes --]

From d401f55a34390c27b35ae647d260e2b900528517 Mon Sep 17 00:00:00 2001
From: Vladimir Oltean <vladimir.oltean@nxp.com>
Date: Tue, 22 Feb 2022 12:05:59 +0200
Subject: [PATCH] ocelot policer validate

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/ethernet/mscc/ocelot_flower.c | 38 ++++-----------------
 drivers/net/ethernet/mscc/ocelot_net.c    | 37 +++-----------------
 drivers/net/ethernet/mscc/ocelot_police.c | 41 +++++++++++++++++++++++
 drivers/net/ethernet/mscc/ocelot_police.h |  5 +++
 4 files changed, 57 insertions(+), 64 deletions(-)

diff --git a/drivers/net/ethernet/mscc/ocelot_flower.c b/drivers/net/ethernet/mscc/ocelot_flower.c
index 4680a62b4d9a..b3f5418dc622 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) {
@@ -298,38 +300,10 @@ static int ocelot_flower_parse_action(struct ocelot *ocelot, int port,
 				return -EOPNOTSUPP;
 			}
 
-			if (a->police.exceed.act_id != FLOW_ACTION_DROP) {
-				NL_SET_ERR_MSG_MOD(extack,
-						   "Police offload is not supported when the 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,
-						   "Police offload is not supported when the conform action is not pipe or ok");
-				return -EOPNOTSUPP;
-			}
-
-			if (a->police.notexceed.act_id == FLOW_ACTION_ACCEPT &&
-			    !flow_action_is_last_entry(&f->rule->action, a)) {
-				NL_SET_ERR_MSG_MOD(extack,
-						   "Police offload is not supported when the 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,
-						   "Police offload is not supported when peakrate/avrate/overhead is configured");
-				return -EOPNOTSUPP;
-			}
+			err = ocelot_policer_validate(action, a, extack);
+			if (err)
+				return err;
 
-			if (a->police.rate_pkt_ps) {
-				NL_SET_ERR_MSG_MOD(extack,
-						   "QoS offload not support packets per second");
-				return -EOPNOTSUPP;
-			}
 			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 4e38d41dba29..5767e38c0c5a 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,38 +259,10 @@ static int ocelot_setup_tc_cls_matchall(struct ocelot_port_private *priv,
 			return -EEXIST;
 		}
 
-		if (action->police.exceed.act_id != FLOW_ACTION_DROP) {
-			NL_SET_ERR_MSG_MOD(extack,
-					   "Police offload is not supported when the exceed action is not drop");
-			return -EOPNOTSUPP;
-		}
-
-		if (action->police.notexceed.act_id != FLOW_ACTION_PIPE &&
-		    action->police.notexceed.act_id != FLOW_ACTION_ACCEPT) {
-			NL_SET_ERR_MSG_MOD(extack,
-					   "Police offload is not supported when the conform action is not pipe or ok");
-			return -EOPNOTSUPP;
-		}
-
-		if (action->police.notexceed.act_id == FLOW_ACTION_ACCEPT &&
-		    !flow_action_is_last_entry(&f->rule->action, action)) {
-			NL_SET_ERR_MSG_MOD(extack,
-					   "Police offload is not supported when the conform action is ok, but police action is not last");
-			return -EOPNOTSUPP;
-		}
-
-		if (action->police.peakrate_bytes_ps ||
-		    action->police.avrate || action->police.overhead) {
-			NL_SET_ERR_MSG_MOD(extack,
-					   "Police offload is not supported when peakrate/avrate/overhead is configured");
-			return -EOPNOTSUPP;
-		}
-
-		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_ */
-- 
2.25.1

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

[Jianbo Liu]

On Tue, 2022-02-22 at 10:09 +0000, Vladimir Oltean wrote:
> Hi Jianbo,
> 
> On Tue, Feb 22, 2022 at 01:58:23AM +0000, Jianbo Liu wrote:
> > Hi Vladimir,
> > 
> > I'd love to hear your suggestion regarding where this validate
> > function
> > to be placed for drivers/net/ethernet/mscc, as it will be used by
> > both
> > ocelot_net.c and ocelot_flower.c. 
> > 
> > Thanks!
> > Jianbo
> 
> Try the attached patch on top of yours.

OK, thanks!

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

[Baowen Zheng]

Since almost all the drivers that support to offload police action make the similar validation, if it make sense to add the validation in the file of flow_offload.h or flow_offload.c?
Then the other drivers do not need to make the similar validation.
WDYT?

On Tuesday, February 22, 2022 6:10 PM, Vladimir wrote:
>On Tue, Feb 22, 2022 at 01:58:23AM +0000, Jianbo Liu wrote:
>> Hi Vladimir,
>>
>> I'd love to hear your suggestion regarding where this validate
>> function to be placed for drivers/net/ethernet/mscc, as it will be
>> used by both ocelot_net.c and ocelot_flower.c.
>>
>> Thanks!
>> Jianbo
>
>Try the attached patch on top of yours.

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

[Ido Schimmel]

On Tue, Feb 22, 2022 at 10:29:57AM +0000, Baowen Zheng wrote:
> Since almost all the drivers that support to offload police action make the similar validation, if it make sense to add the validation in the file of flow_offload.h or flow_offload.c?
> Then the other drivers do not need to make the similar validation.
> WDYT?

But not all the drivers need the same validation. For example, nfp is
one of the few drivers that supports policing based on packet rate. The
octeontx2 driver has different restrictions based on whether the policer
is attached to matchall or flower.

We can put the restrictions that are common between all the drivers
somewhere, but it's not that much and it will also change over time,
resulting in needless churn where checks are moved to individual
drivers.

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

[Simon Horman]

On Thu, Feb 17, 2022 at 08:28:01AM +0000, Jianbo Liu 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.

Hi,

I have a concern that
a) patch 1 introduces a facility that may break existing drivers; and
b) patch 2 then fixes this

I'd slightly prefer if the series was rearranged to avoid this problem.

...

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

[Roi Dayan]

On 2022-02-17 1:34 PM, Simon Horman wrote:
> On Thu, Feb 17, 2022 at 08:28:01AM +0000, Jianbo Liu 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.
> 
> Hi,
> 
> I have a concern that
> a) patch 1 introduces a facility that may break existing drivers; and
> b) patch 2 then fixes this
> 
> I'd slightly prefer if the series was rearranged to avoid this problem.
> 
> ...

Hi Simon,

It can't be rearranged as patch 2 can't compile without patch 1.
Patch 1 only adds more information passing to the driver.

The drivers functionality doesn't change. drivers today ignore
police information, like actions, and still being ignored after patch 1.

patch 2 updates the drivers to use that information instead of
ignoring it. so it fixes the drivers that without patch 1 can't be
fixed.

Thanks,
Roi

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

[Simon Horman]

On Thu, Feb 17, 2022 at 01:52:47PM +0200, Roi Dayan wrote:
> On 2022-02-17 1:34 PM, Simon Horman wrote:
> > On Thu, Feb 17, 2022 at 08:28:01AM +0000, Jianbo Liu 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.
> > 
> > Hi,
> > 
> > I have a concern that
> > a) patch 1 introduces a facility that may break existing drivers; and
> > b) patch 2 then fixes this
> > 
> > I'd slightly prefer if the series was rearranged to avoid this problem.
> > 
> > ...
> 
> Hi Simon,
> 
> It can't be rearranged as patch 2 can't compile without patch 1.
> Patch 1 only adds more information passing to the driver.
> 
> The drivers functionality doesn't change. drivers today ignore
> police information, like actions, and still being ignored after patch 1.
> 
> patch 2 updates the drivers to use that information instead of
> ignoring it. so it fixes the drivers that without patch 1 can't be
> fixed.

I think it would be possible, but...

On Thu, Feb 17, 2022 at 01:56:51PM +0200, Ido Schimmel wrote:

...

> Not sure what you mean by the above. Patch #1 extends the flow offload
> API with tc-police parameters that weren't communicated to drivers until
> now. Drivers still ignore the new parameters after this patch. It is
> only in patch #2 that these drivers reject configurations where the
> parameters are set.
> 
> Therefore, the only breakage I see is the one that can happen after
> patch #2: unaware user space that was installing actions that weren't
> fully reflected to hardware.
> 
> If we want to be on the safe side, it is possible to remove the errors,
> but keep the extack messages so that user space is at least somewhat
> aware.

Yes, I see what you mean.
I'm now comfortable with the way this patchset is arranged.

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

[Ido Schimmel]

On Thu, Feb 17, 2022 at 12:34:39PM +0100, Simon Horman wrote:
> On Thu, Feb 17, 2022 at 08:28:01AM +0000, Jianbo Liu 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.
> 
> Hi,
> 
> I have a concern that
> a) patch 1 introduces a facility that may break existing drivers; and
> b) patch 2 then fixes this
> 
> I'd slightly prefer if the series was rearranged to avoid this problem.

Not sure what you mean by the above. Patch #1 extends the flow offload
API with tc-police parameters that weren't communicated to drivers until
now. Drivers still ignore the new parameters after this patch. It is
only in patch #2 that these drivers reject configurations where the
parameters are set.

Therefore, the only breakage I see is the one that can happen after
patch #2: unaware user space that was installing actions that weren't
fully reflected to hardware.

If we want to be on the safe side, it is possible to remove the errors,
but keep the extack messages so that user space is at least somewhat
aware.