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

On Fri, 2022-02-18 at 01:46 +0000, Baowen Zheng wrote:

On, February 17, 2022 8:10 PM, Roi wrote:

quoted

On 2022-02-17 12:25 PM, Baowen Zheng wrote:

quoted

On February 17, 2022 4:28 PM, Jianbo wrote:

quoted

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 <redacted>
Signed-off-by: Roi Dayan <redacted>
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

++++++++++++++++++++++++++++++++++

quoted

quoted

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:

quoted

                        struct {
                                enum flow_action_id     act_id;
                                u32                     index;
                        } exceed, notexceed;

right. it can be.

quoted

quoted

                } 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?

quoted

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!