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@xxxxxxxxxx>
> > > > Signed-off-by: Roi Dayan <roid@xxxxxxxxxx>
> > > > Reviewed-by: Ido Schimmel <idosch@xxxxxxxxxx>
> > > > ---
> > > > 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!
