| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <78743f8f96040450f2e37c7627007d7e564b022f.camel@nvidia.com>
Date: Wed, 23 Feb 2022 01:54:07 +0000
From: Jianbo Liu <jianbol@...dia.com>
To: Roi Dayan <roid@...dia.com>,
"baowen.zheng@...igine.com" <baowen.zheng@...igine.com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"linux-rdma@...r.kernel.org" <linux-rdma@...r.kernel.org>
CC: "claudiu.manoil@....com" <claudiu.manoil@....com>,
"andrew@...n.ch" <andrew@...n.ch>,
"vivien.didelot@...il.com" <vivien.didelot@...il.com>,
"olteanv@...il.com" <olteanv@...il.com>,
"jhs@...atatu.com" <jhs@...atatu.com>,
"oss-drivers@...igine.com" <oss-drivers@...igine.com>,
Petr Machata <petrm@...dia.com>,
"leon@...nel.org" <leon@...nel.org>,
"davem@...emloft.net" <davem@...emloft.net>,
"hkelam@...vell.com" <hkelam@...vell.com>,
"louis.peens@...ronome.com" <louis.peens@...ronome.com>,
"alexandre.belloni@...tlin.com" <alexandre.belloni@...tlin.com>,
"UNGLinuxDriver@...rochip.com" <UNGLinuxDriver@...rochip.com>,
"jiri@...nulli.us" <jiri@...nulli.us>,
"rajur@...lsio.com" <rajur@...lsio.com>,
Ido Schimmel <idosch@...dia.com>,
"simon.horman@...igine.com" <simon.horman@...igine.com>,
"sbhatta@...vell.com" <sbhatta@...vell.com>,
"xiyou.wangcong@...il.com" <xiyou.wangcong@...il.com>,
"kuba@...nel.org" <kuba@...nel.org>,
Saeed Mahameed <saeedm@...dia.com>,
"sgoutham@...vell.com" <sgoutham@...vell.com>,
"gakula@...vell.com" <gakula@...vell.com>,
"peng.zhang@...igine.com" <peng.zhang@...igine.com>,
"f.fainelli@...il.com" <f.fainelli@...il.com>
Subject: 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:
> > 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@...dia.com>
> > > > Signed-off-by: Roi Dayan <roid@...dia.com>
> > > > Reviewed-by: Ido Schimmel <idosch@...dia.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
> > >
Powered by blists - more mailing lists