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

[Jianbo Liu <jianbol@nvidia.com>]

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