From: Simon Horman <hidden> Date: 2021-10-28 11:07:07
Baowen Zheng says:
Allow use of flow_indr_dev_register/flow_indr_dev_setup_offload to offload
tc actions independent of flows.
The motivation for this work is to prepare for using TC police action
instances to provide hardware offload of OVS metering feature - which calls
for policers that may be used by multiple flows and whose lifecycle is
independent of any flows that use them.
This patch includes basic changes to offload drivers to return EOPNOTSUPP
if this feature is used - it is not yet supported by any driver.
Tc cli command to offload and quote an action:
tc qdisc del dev $DEV ingress && sleep 1 || true
tc actions delete action police index 99 || true
tc qdisc add dev $DEV ingress
tc qdisc show dev $DEV ingress
tc actions add action police index 99 rate 1mbit burst 100k skip_sw
tc actions list action police
tc filter add dev $DEV protocol ip parent ffff:
flower ip_proto tcp action police index 99
tc -s -d filter show dev $DEV protocol ip parent ffff:
tc filter add dev $DEV protocol ipv6 parent ffff:
flower skip_sw ip_proto tcp action police index 99
tc -s -d filter show dev $DEV protocol ipv6 parent ffff:
tc actions list action police
tc qdisc del dev $DEV ingress && sleep 1
tc actions delete action police index 99
tc actions list action police
Changes compared to v2 patches:
* Made changes according to the review comments.
* Delete in_hw and not_in_hw flag and user can judge if the action is
offloaded to any hardware by in_hw_count.
* Split the main patch of the action offload to three single patch to
facilitate code review.
Posting this revision of the patchset as an RFC as while we feel it is
ready for review we would like an opportunity to conduct further testing
before acceptance into upstream.
Baowen Zheng (8):
flow_offload: fill flags to action structure
flow_offload: reject to offload tc actions in offload drivers
flow_offload: allow user to offload tc action to net device
flow_offload: add skip_hw and skip_sw to control if offload the action
flow_offload: add process to update action stats from hardware
net: sched: save full flags for tc action
flow_offload: add reoffload process to update hw_count
flow_offload: validate flags of filter and actions
drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c | 2 +-
.../ethernet/mellanox/mlx5/core/en/rep/tc.c | 3 +
.../ethernet/netronome/nfp/flower/offload.c | 3 +
include/linux/netdevice.h | 1 +
include/net/act_api.h | 34 +-
include/net/flow_offload.h | 17 +
include/net/pkt_cls.h | 61 ++-
include/uapi/linux/pkt_cls.h | 9 +-
net/core/flow_offload.c | 48 +-
net/sched/act_api.c | 440 +++++++++++++++++-
net/sched/act_bpf.c | 2 +-
net/sched/act_connmark.c | 2 +-
net/sched/act_ctinfo.c | 2 +-
net/sched/act_gate.c | 2 +-
net/sched/act_ife.c | 2 +-
net/sched/act_ipt.c | 2 +-
net/sched/act_mpls.c | 2 +-
net/sched/act_nat.c | 2 +-
net/sched/act_pedit.c | 2 +-
net/sched/act_police.c | 2 +-
net/sched/act_sample.c | 2 +-
net/sched/act_simple.c | 2 +-
net/sched/act_skbedit.c | 2 +-
net/sched/act_skbmod.c | 2 +-
net/sched/cls_api.c | 55 ++-
net/sched/cls_flower.c | 3 +-
net/sched/cls_matchall.c | 4 +-
net/sched/cls_u32.c | 7 +-
28 files changed, 661 insertions(+), 54 deletions(-)
--
2.20.1
@@ -145,7 +145,7 @@ static int __tcf_ipt_init(struct net *net, unsigned int id, struct nlattr *nla,if(!exists){ret=tcf_idr_create(tn,index,est,a,ops,bind,-false,0);+false,flags);if(ret){tcf_idr_cleanup(tn,index);returnret;
From: Simon Horman <hidden> Date: 2021-10-28 11:07:11
From: Baowen Zheng <redacted>
A follow-up patch will allow users to offload tc actions independent of
classifier in the software datapath.
In preparation for this, teach all drivers that support offload of the flow
tables to reject such configuration as currently none of them support it.
Signed-off-by: Baowen Zheng <redacted>
Signed-off-by: Louis Peens <redacted>
Signed-off-by: Simon Horman <redacted>
---
drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c | 2 +-
drivers/net/ethernet/mellanox/mlx5/core/en/rep/tc.c | 3 +++
drivers/net/ethernet/netronome/nfp/flower/offload.c | 3 +++
3 files changed, 7 insertions(+), 1 deletion(-)
From: Simon Horman <hidden> Date: 2021-10-28 11:07:15
From: Baowen Zheng <redacted>
Use flow_indr_dev_register/flow_indr_dev_setup_offload to
offload tc action.
We need to call tc_cleanup_flow_action to clean up tc action entry since
in tc_setup_action, some actions may hold dev refcnt, especially the mirror
action.
Signed-off-by: Baowen Zheng <redacted>
Signed-off-by: Louis Peens <redacted>
Signed-off-by: Simon Horman <redacted>
---
include/linux/netdevice.h | 1 +
include/net/act_api.h | 2 +-
include/net/flow_offload.h | 17 ++++
include/net/pkt_cls.h | 15 ++++
net/core/flow_offload.c | 43 ++++++++--
net/sched/act_api.c | 166 +++++++++++++++++++++++++++++++++++++
net/sched/cls_api.c | 29 ++++++-
7 files changed, 260 insertions(+), 13 deletions(-)
@@ -916,6 +916,7 @@ enum tc_setup_type {TC_SETUP_QDISC_TBF,TC_SETUP_QDISC_FIFO,TC_SETUP_QDISC_HTB,+TC_SETUP_ACT,};/* These structures hold the attributes of bpf state that are being passed
@@ -1061,6 +1077,154 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,returnERR_PTR(err);}+staticintflow_action_init(structflow_offload_action*fl_action,+structtc_action*act,+enumflow_act_commandcmd,+structnetlink_ext_ack*extack)+{+if(!fl_action)+return-EINVAL;++fl_action->extack=extack;+fl_action->command=cmd;+fl_action->index=act->tcfa_index;++if(is_tcf_gact_ok(act)){+fl_action->id=FLOW_ACTION_ACCEPT;+}elseif(is_tcf_gact_shot(act)){+fl_action->id=FLOW_ACTION_DROP;+}elseif(is_tcf_gact_trap(act)){+fl_action->id=FLOW_ACTION_TRAP;+}elseif(is_tcf_gact_goto_chain(act)){+fl_action->id=FLOW_ACTION_GOTO;+}elseif(is_tcf_mirred_egress_redirect(act)){+fl_action->id=FLOW_ACTION_REDIRECT;+}elseif(is_tcf_mirred_egress_mirror(act)){+fl_action->id=FLOW_ACTION_MIRRED;+}elseif(is_tcf_mirred_ingress_redirect(act)){+fl_action->id=FLOW_ACTION_REDIRECT_INGRESS;+}elseif(is_tcf_mirred_ingress_mirror(act)){+fl_action->id=FLOW_ACTION_MIRRED_INGRESS;+}elseif(is_tcf_vlan(act)){+switch(tcf_vlan_action(act)){+caseTCA_VLAN_ACT_PUSH:+fl_action->id=FLOW_ACTION_VLAN_PUSH;+break;+caseTCA_VLAN_ACT_POP:+fl_action->id=FLOW_ACTION_VLAN_POP;+break;+caseTCA_VLAN_ACT_MODIFY:+fl_action->id=FLOW_ACTION_VLAN_MANGLE;+break;+default:+return-EOPNOTSUPP;+}+}elseif(is_tcf_tunnel_set(act)){+fl_action->id=FLOW_ACTION_TUNNEL_ENCAP;+}elseif(is_tcf_tunnel_release(act)){+fl_action->id=FLOW_ACTION_TUNNEL_DECAP;+}elseif(is_tcf_csum(act)){+fl_action->id=FLOW_ACTION_CSUM;+}elseif(is_tcf_skbedit_mark(act)){+fl_action->id=FLOW_ACTION_MARK;+}elseif(is_tcf_sample(act)){+fl_action->id=FLOW_ACTION_SAMPLE;+}elseif(is_tcf_police(act)){+fl_action->id=FLOW_ACTION_POLICE;+}elseif(is_tcf_ct(act)){+fl_action->id=FLOW_ACTION_CT;+}elseif(is_tcf_mpls(act)){+switch(tcf_mpls_action(act)){+caseTCA_MPLS_ACT_PUSH:+fl_action->id=FLOW_ACTION_MPLS_PUSH;+break;+caseTCA_MPLS_ACT_POP:+fl_action->id=FLOW_ACTION_MPLS_POP;+break;+caseTCA_MPLS_ACT_MODIFY:+fl_action->id=FLOW_ACTION_MPLS_MANGLE;+break;+default:+return-EOPNOTSUPP;+}+}elseif(is_tcf_skbedit_ptype(act)){+fl_action->id=FLOW_ACTION_PTYPE;+}elseif(is_tcf_skbedit_priority(act)){+fl_action->id=FLOW_ACTION_PRIORITY;+}elseif(is_tcf_gate(act)){+fl_action->id=FLOW_ACTION_GATE;+}else{+return-EOPNOTSUPP;+}++return0;+}++staticinttcf_action_offload_cmd(structflow_offload_action*fl_act,+structnetlink_ext_ack*extack)+{+interr;++if(IS_ERR(fl_act))+returnPTR_ERR(fl_act);++err=flow_indr_dev_setup_offload(NULL,NULL,TC_SETUP_ACT,+fl_act,NULL,NULL);+if(err<0)+returnerr;++return0;+}++/* offload the tc command after inserted */+staticinttcf_action_offload_add(structtc_action*action,+structnetlink_ext_ack*extack)+{+structtc_action*actions[TCA_ACT_MAX_PRIO]={+[0]=action,+};+structflow_offload_action*fl_action;+interr=0;++fl_action=flow_action_alloc(tcf_act_num_actions_single(action));+if(!fl_action)+return-EINVAL;++err=flow_action_init(fl_action,action,FLOW_ACT_REPLACE,extack);+if(err)+gotofl_err;++err=tc_setup_action(&fl_action->action,actions);+if(err){+NL_SET_ERR_MSG_MOD(extack,+"Failed to setup tc actions for offload\n");+gotofl_err;+}++err=tcf_action_offload_cmd(fl_action,extack);+tc_cleanup_flow_action(&fl_action->action);++fl_err:+kfree(fl_action);++returnerr;+}++inttcf_action_offload_del(structtc_action*action)+{+structflow_offload_actionfl_act;+interr=0;++if(!action)+return-EINVAL;++err=flow_action_init(&fl_act,action,FLOW_ACT_DESTROY,NULL);+if(err)+returnerr;++returntcf_action_offload_cmd(&fl_act,NULL);+}+/* Returns numbers of initialized actions or negative error. */inttcf_action_init(structnet*net,structtcf_proto*tp,structnlattr*nla,
@@ -1103,6 +1267,8 @@ int tcf_action_init(struct net *net, struct tcf_proto *tp, struct nlattr *nla,sz+=tcf_action_fill_size(act);/* Start from index 0 */actions[i-1]=act;+if(!(flags&TCA_ACT_FLAGS_BIND))+tcf_action_offload_add(act,extack);}/* We have to commit them all together, because if any error happened in
From: Simon Horman <hidden> Date: 2021-10-28 11:07:16
From: Baowen Zheng <redacted>
We add skip_hw and skip_sw for user to control if offload the action
to hardware.
We also add in_hw_count for user to indicate if the action is offloaded
to any hardware.
Signed-off-by: Baowen Zheng <redacted>
Signed-off-by: Louis Peens <redacted>
Signed-off-by: Simon Horman <redacted>
---
include/net/act_api.h | 7 +++++
include/net/pkt_cls.h | 23 +++++++++++++++
include/uapi/linux/pkt_cls.h | 9 ++++--
net/sched/act_api.c | 54 ++++++++++++++++++++++++++++++++----
4 files changed, 84 insertions(+), 9 deletions(-)
@@ -1161,6 +1174,7 @@ static int flow_action_init(struct flow_offload_action *fl_action,}staticinttcf_action_offload_cmd(structflow_offload_action*fl_act,+u32*hw_count,structnetlink_ext_ack*extack){interr;
@@ -1173,6 +1187,9 @@ static int tcf_action_offload_cmd(struct flow_offload_action *fl_act,if(err<0)returnerr;+if(hw_count)+*hw_count=err;+return0;}
@@ -1180,12 +1197,17 @@ static int tcf_action_offload_cmd(struct flow_offload_action *fl_act,staticinttcf_action_offload_add(structtc_action*action,structnetlink_ext_ack*extack){+boolskip_sw=tc_act_skip_sw(action->tcfa_flags);structtc_action*actions[TCA_ACT_MAX_PRIO]={[0]=action,};structflow_offload_action*fl_action;+u32in_hw_count=0;interr=0;+if(tc_act_skip_hw(action->tcfa_flags))+return0;+fl_action=flow_action_alloc(tcf_act_num_actions_single(action));if(!fl_action)return-EINVAL;
@@ -1201,7 +1223,13 @@ static int tcf_action_offload_add(struct tc_action *action,gotofl_err;}-err=tcf_action_offload_cmd(fl_action,extack);+err=tcf_action_offload_cmd(fl_action,&in_hw_count,extack);+if(!err)+flow_action_hw_count_set(action,in_hw_count);++if(skip_sw&&!tc_act_in_hw(action))+err=-EINVAL;+tc_cleanup_flow_action(&fl_action->action);fl_err:
@@ -1213,16 +1241,27 @@ static int tcf_action_offload_add(struct tc_action *action,inttcf_action_offload_del(structtc_action*action){structflow_offload_actionfl_act;+u32in_hw_count=0;interr=0;if(!action)return-EINVAL;+if(!tc_act_in_hw(action))+return0;+err=flow_action_init(&fl_act,action,FLOW_ACT_DESTROY,NULL);if(err)returnerr;-returntcf_action_offload_cmd(&fl_act,NULL);+err=tcf_action_offload_cmd(&fl_act,&in_hw_count,NULL);+if(err)+returnerr;++if(action->in_hw_count!=in_hw_count)+return-EINVAL;++return0;}/* Returns numbers of initialized actions or negative error. */
@@ -1267,8 +1306,11 @@ int tcf_action_init(struct net *net, struct tcf_proto *tp, struct nlattr *nla,sz+=tcf_action_fill_size(act);/* Start from index 0 */actions[i-1]=act;-if(!(flags&TCA_ACT_FLAGS_BIND))-tcf_action_offload_add(act,extack);+if(!(flags&TCA_ACT_FLAGS_BIND)){+err=tcf_action_offload_add(act,extack);+if(tc_act_skip_sw(act->tcfa_flags)&&err)+gotoerr;+}}/* We have to commit them all together, because if any error happened in
From: Simon Horman <hidden> Date: 2021-10-28 11:07:23
From: Baowen Zheng <redacted>
When collecting stats for actions update them using both
both hardware and software counters.
Stats update process should not in context of preempt_disable.
Signed-off-by: Baowen Zheng <redacted>
Signed-off-by: Louis Peens <redacted>
Signed-off-by: Simon Horman <redacted>
---
include/net/act_api.h | 1 +
include/net/pkt_cls.h | 18 ++++++++++--------
net/sched/act_api.c | 37 +++++++++++++++++++++++++++++++++++++
3 files changed, 48 insertions(+), 8 deletions(-)
@@ -1238,6 +1238,40 @@ static int tcf_action_offload_add(struct tc_action *action,returnerr;}+inttcf_action_update_hw_stats(structtc_action*action)+{+structflow_offload_actionfl_act={};+interr=0;++if(!tc_act_in_hw(action))+return-EOPNOTSUPP;++err=flow_action_init(&fl_act,action,FLOW_ACT_STATS,NULL);+if(err)+gotoerr_out;++err=tcf_action_offload_cmd(&fl_act,NULL,NULL);++if(!err&&fl_act.stats.lastused){+preempt_disable();+tcf_action_stats_update(action,fl_act.stats.bytes,+fl_act.stats.pkts,+fl_act.stats.drops,+fl_act.stats.lastused,+true);+preempt_enable();+action->used_hw_stats=fl_act.stats.used_hw_stats;+action->used_hw_stats_valid=true;+err=0;+}else{+err=-EOPNOTSUPP;+}++err_out:+returnerr;+}+EXPORT_SYMBOL(tcf_action_update_hw_stats);+inttcf_action_offload_del(structtc_action*action){structflow_offload_actionfl_act;
@@ -1362,6 +1396,9 @@ int tcf_action_copy_stats(struct sk_buff *skb, struct tc_action *p,if(p==NULL)gotoerrout;+/* update hw stats for this action */+tcf_action_update_hw_stats(p);+/* compat_mode being true specifies a call that is supposed*toaddadditionalbackwardcompatibilitystatisticTLVs.*/
From: Simon Horman <hidden> Date: 2021-10-28 11:07:24
From: Baowen Zheng <redacted>
Add reoffload process to update hw_count when driver
is inserted or removed.
When reoffloading actions, we still offload the actions
that are added independent of filters.
Signed-off-by: Baowen Zheng <redacted>
Signed-off-by: Louis Peens <redacted>
Signed-off-by: Simon Horman <redacted>
---
include/net/act_api.h | 24 +++++
include/net/pkt_cls.h | 5 +
net/core/flow_offload.c | 5 +
net/sched/act_api.c | 213 ++++++++++++++++++++++++++++++++++++----
4 files changed, 228 insertions(+), 19 deletions(-)
@@ -638,6 +638,59 @@ EXPORT_SYMBOL(tcf_idrinfo_destroy);staticLIST_HEAD(act_base);staticDEFINE_RWLOCK(act_mod_lock);+/* since act ops id is stored in pernet subsystem list,+*thenthereisnowaytowalkthroughonlyalltheaction+*subsystem,sowekeeptcactionpernetopsidfor+*reoffloadtowalkthrough.+*/+staticLIST_HEAD(act_pernet_id_list);+staticDEFINE_MUTEX(act_id_mutex);+structtc_act_pernet_id{+structlist_headlist;+unsignedintid;+};++staticinttcf_pernet_add_id_list(unsignedintid)+{+structtc_act_pernet_id*id_ptr;+intret=0;++mutex_lock(&act_id_mutex);+list_for_each_entry(id_ptr,&act_pernet_id_list,list){+if(id_ptr->id==id){+ret=-EEXIST;+gotoerr_out;+}+}++id_ptr=kzalloc(sizeof(*id_ptr),GFP_KERNEL);+if(!id_ptr){+ret=-ENOMEM;+gotoerr_out;+}+id_ptr->id=id;++list_add_tail(&id_ptr->list,&act_pernet_id_list);++err_out:+mutex_unlock(&act_id_mutex);+returnret;+}++staticvoidtcf_pernet_del_id_list(unsignedintid)+{+structtc_act_pernet_id*id_ptr;++mutex_lock(&act_id_mutex);+list_for_each_entry(id_ptr,&act_pernet_id_list,list){+if(id_ptr->id==id){+list_del(&id_ptr->list);+kfree(id_ptr);+break;+}+}+mutex_unlock(&act_id_mutex);+}inttcf_register_action(structtc_action_ops*act,structpernet_operations*ops)
@@ -656,18 +709,30 @@ int tcf_register_action(struct tc_action_ops *act,if(ret)returnret;+if(ops->id){+ret=tcf_pernet_add_id_list(*ops->id);+if(ret)+gotoid_err;+}+write_lock(&act_mod_lock);list_for_each_entry(a,&act_base,head){if(act->id==a->id||(strcmp(act->kind,a->kind)==0)){-write_unlock(&act_mod_lock);-unregister_pernet_subsys(ops);-return-EEXIST;+ret=-EEXIST;+gotoerr_out;}}list_add_tail(&act->head,&act_base);write_unlock(&act_mod_lock);return0;++err_out:+write_unlock(&act_mod_lock);+tcf_pernet_del_id_list(*ops->id);+id_err:+unregister_pernet_subsys(ops);+returnret;}EXPORT_SYMBOL(tcf_register_action);
@@ -686,8 +751,11 @@ int tcf_unregister_action(struct tc_action_ops *act,}}write_unlock(&act_mod_lock);-if(!err)+if(!err){unregister_pernet_subsys(ops);+if(ops->id)+tcf_pernet_del_id_list(*ops->id);+}returnerr;}EXPORT_SYMBOL(tcf_unregister_action);
@@ -1175,15 +1243,11 @@ static int flow_action_init(struct flow_offload_action *fl_action,return0;}-staticinttcf_action_offload_cmd(structflow_offload_action*fl_act,-u32*hw_count,-structnetlink_ext_ack*extack)+staticinttcf_action_offload_cmd_ex(structflow_offload_action*fl_act,+u32*hw_count){interr;-if(IS_ERR(fl_act))-returnPTR_ERR(fl_act);-err=flow_indr_dev_setup_offload(NULL,NULL,TC_SETUP_ACT,fl_act,NULL,NULL);if(err<0)
@@ -1195,9 +1259,41 @@ static int tcf_action_offload_cmd(struct flow_offload_action *fl_act,return0;}+staticinttcf_action_offload_cmd_cb_ex(structflow_offload_action*fl_act,+u32*hw_count,+flow_indr_block_bind_cb_t*cb,+void*cb_priv)+{+interr;++err=cb(NULL,NULL,cb_priv,TC_SETUP_ACT,NULL,fl_act,NULL);+if(err<0)+returnerr;++if(hw_count)+*hw_count=1;++return0;+}++staticinttcf_action_offload_cmd(structflow_offload_action*fl_act,+u32*hw_count,+flow_indr_block_bind_cb_t*cb,+void*cb_priv)+{+if(IS_ERR(fl_act))+returnPTR_ERR(fl_act);++returncb?tcf_action_offload_cmd_cb_ex(fl_act,hw_count,+cb,cb_priv):+tcf_action_offload_cmd_ex(fl_act,hw_count);+}+/* offload the tc command after inserted */-staticinttcf_action_offload_add(structtc_action*action,-structnetlink_ext_ack*extack)+staticinttcf_action_offload_add_ex(structtc_action*action,+structnetlink_ext_ack*extack,+flow_indr_block_bind_cb_t*cb,+void*cb_priv){boolskip_sw=tc_act_skip_sw(action->tcfa_flags);structtc_action*actions[TCA_ACT_MAX_PRIO]={
@@ -1225,9 +1321,10 @@ static int tcf_action_offload_add(struct tc_action *action,gotofl_err;}-err=tcf_action_offload_cmd(fl_action,&in_hw_count,extack);+err=tcf_action_offload_cmd(fl_action,&in_hw_count,cb,cb_priv);if(!err)-flow_action_hw_count_set(action,in_hw_count);+cb?flow_action_hw_count_inc(action,in_hw_count):+flow_action_hw_count_set(action,in_hw_count);if(skip_sw&&!tc_act_in_hw(action))err=-EINVAL;
@@ -1240,6 +1337,12 @@ static int tcf_action_offload_add(struct tc_action *action,returnerr;}+staticinttcf_action_offload_add(structtc_action*action,+structnetlink_ext_ack*extack)+{+returntcf_action_offload_add_ex(action,extack,NULL,NULL);+}+inttcf_action_update_hw_stats(structtc_action*action){structflow_offload_actionfl_act={};
@@ -1252,7 +1355,7 @@ int tcf_action_update_hw_stats(struct tc_action *action)if(err)gotoerr_out;-err=tcf_action_offload_cmd(&fl_act,NULL,NULL);+err=tcf_action_offload_cmd(&fl_act,NULL,NULL,NULL);if(!err&&fl_act.stats.lastused){preempt_disable();
@@ -1274,7 +1377,9 @@ int tcf_action_update_hw_stats(struct tc_action *action)}EXPORT_SYMBOL(tcf_action_update_hw_stats);-inttcf_action_offload_del(structtc_action*action)+staticinttcf_action_offload_del_ex(structtc_action*action,+flow_indr_block_bind_cb_t*cb,+void*cb_priv){structflow_offload_actionfl_act;u32in_hw_count=0;
@@ -1290,13 +1395,83 @@ int tcf_action_offload_del(struct tc_action *action)if(err)returnerr;-err=tcf_action_offload_cmd(&fl_act,&in_hw_count,NULL);-if(err)+err=tcf_action_offload_cmd(&fl_act,&in_hw_count,cb,cb_priv);+if(err<0)returnerr;-if(action->in_hw_count!=in_hw_count)+if(!cb&&action->in_hw_count!=in_hw_count)return-EINVAL;+/* do not need to update hw state when deleting action */+if(cb&&in_hw_count)+flow_action_hw_count_dec(action,in_hw_count);++return0;+}++inttcf_action_offload_del(structtc_action*action)+{+returntcf_action_offload_del_ex(action,NULL,NULL);+}++inttcf_action_reoffload_cb(flow_indr_block_bind_cb_t*cb,+void*cb_priv,booladd)+{+structtc_act_pernet_id*id_ptr;+structtcf_idrinfo*idrinfo;+structtc_action_net*tn;+structtc_action*p;+unsignedintact_id;+unsignedlongtmp;+unsignedlongid;+structidr*idr;+structnet*net;+intret;++if(!cb)+return-EINVAL;++down_read(&net_rwsem);+mutex_lock(&act_id_mutex);++for_each_net(net){+list_for_each_entry(id_ptr,&act_pernet_id_list,list){+act_id=id_ptr->id;+tn=net_generic(net,act_id);+if(!tn)+continue;+idrinfo=tn->idrinfo;+if(!idrinfo)+continue;++mutex_lock(&idrinfo->lock);+idr=&idrinfo->action_idr;+idr_for_each_entry_ul(idr,p,tmp,id){+if(IS_ERR(p)||tc_act_bind(p->tcfa_flags))+continue;+if(add){+tcf_action_offload_add_ex(p,NULL,cb,+cb_priv);+continue;+}++/* cb unregister to update hw count */+ret=tcf_action_offload_del_ex(p,cb,cb_priv);+if(ret<0)+continue;+if(tc_act_skip_sw(p->tcfa_flags)&&+!tc_act_in_hw(p)){+ret=tcf_idr_release_unsafe(p);+if(ret==ACT_P_DELETED)+module_put(p->ops->owner);+}+}+mutex_unlock(&idrinfo->lock);+}+}+mutex_unlock(&act_id_mutex);+up_read(&net_rwsem);+return0;}
From: Simon Horman <hidden> Date: 2021-10-28 11:07:25
From: Baowen Zheng <redacted>
Save full action flags and return user flags when return flags to
user space.
Save full action flags to distinguish if the action is created
independent from classifier.
We made this change mainly for further patch to reoffload tc actions.
Signed-off-by: Baowen Zheng <redacted>
Signed-off-by: Louis Peens <redacted>
Signed-off-by: Simon Horman <redacted>
---
net/sched/act_api.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
From: Simon Horman <hidden> Date: 2021-10-28 11:07:33
From: Baowen Zheng <redacted>
Add process to validate flags of filter and actions when adding
a tc filter.
We need to prevent adding filter with flags conflicts with its actions.
Signed-off-by: Baowen Zheng <redacted>
Signed-off-by: Louis Peens <redacted>
Signed-off-by: Simon Horman <redacted>
---
net/sched/cls_api.c | 26 ++++++++++++++++++++++++++
net/sched/cls_flower.c | 3 ++-
net/sched/cls_matchall.c | 4 ++--
net/sched/cls_u32.c | 7 ++++---
4 files changed, 34 insertions(+), 6 deletions(-)
From: Jamal Hadi Salim <jhs@mojatatu.com> Date: 2021-10-28 14:23:18
On 2021-10-28 07:06, Simon Horman wrote:
aowen Zheng says:
Allow use of flow_indr_dev_register/flow_indr_dev_setup_offload to offload
tc actions independent of flows.
The motivation for this work is to prepare for using TC police action
instances to provide hardware offload of OVS metering feature - which calls
for policers that may be used by multiple flows and whose lifecycle is
independent of any flows that use them.
This patch includes basic changes to offload drivers to return EOPNOTSUPP
if this feature is used - it is not yet supported by any driver.
Tc cli command to offload and quote an action:
tc qdisc del dev $DEV ingress && sleep 1 || true
tc actions delete action police index 99 || true
tc qdisc add dev $DEV ingress
tc qdisc show dev $DEV ingress
tc actions add action police index 99 rate 1mbit burst 100k skip_sw
tc actions list action police
tc filter add dev $DEV protocol ip parent ffff:
flower ip_proto tcp action police index 99
tc -s -d filter show dev $DEV protocol ip parent ffff:
tc filter add dev $DEV protocol ipv6 parent ffff:
flower skip_sw ip_proto tcp action police index 99
tc -s -d filter show dev $DEV protocol ipv6 parent ffff:
tc actions list action police
tc qdisc del dev $DEV ingress && sleep 1
tc actions delete action police index 99
tc actions list action police
It will be helpful to display the output of the show commands in the
cover letter....
cheers,
jamal
On Thu 28 Oct 2021 at 14:06, Simon Horman [off-list ref] wrote:
quoted hunk
From: Baowen Zheng <redacted>
Use flow_indr_dev_register/flow_indr_dev_setup_offload to
offload tc action.
We need to call tc_cleanup_flow_action to clean up tc action entry since
in tc_setup_action, some actions may hold dev refcnt, especially the mirror
action.
Signed-off-by: Baowen Zheng <redacted>
Signed-off-by: Louis Peens <redacted>
Signed-off-by: Simon Horman <redacted>
---
include/linux/netdevice.h | 1 +
include/net/act_api.h | 2 +-
include/net/flow_offload.h | 17 ++++
include/net/pkt_cls.h | 15 ++++
net/core/flow_offload.c | 43 ++++++++--
net/sched/act_api.c | 166 +++++++++++++++++++++++++++++++++++++
net/sched/cls_api.c | 29 ++++++-
7 files changed, 260 insertions(+), 13 deletions(-)
@@ -916,6 +916,7 @@ enum tc_setup_type {TC_SETUP_QDISC_TBF,TC_SETUP_QDISC_FIFO,TC_SETUP_QDISC_HTB,+TC_SETUP_ACT,};/* These structures hold the attributes of bpf state that are being passed
tcf_action_offload_del() and tcf_action_cleanup() seem to be always
called together. Consider moving the call to tcf_action_offload_del()
into tcf_action_cleanup().
@@ -1103,6 +1267,8 @@ int tcf_action_init(struct net *net, struct tcf_proto *tp, struct nlattr *nla, sz += tcf_action_fill_size(act); /* Start from index 0 */ actions[i - 1] = act;+ if (!(flags & TCA_ACT_FLAGS_BIND))+ tcf_action_offload_add(act, extack); } /* We have to commit them all together, because if any error happened in
On Thu 28 Oct 2021 at 14:06, Simon Horman [off-list ref] wrote:
From: Baowen Zheng <redacted>
When collecting stats for actions update them using both
both hardware and software counters.
Stats update process should not in context of preempt_disable.
@@ -1238,6 +1238,40 @@ static int tcf_action_offload_add(struct tc_action *action,returnerr;}+inttcf_action_update_hw_stats(structtc_action*action)+{+structflow_offload_actionfl_act={};+interr=0;++if(!tc_act_in_hw(action))+return-EOPNOTSUPP;++err=flow_action_init(&fl_act,action,FLOW_ACT_STATS,NULL);+if(err)+gotoerr_out;++err=tcf_action_offload_cmd(&fl_act,NULL,NULL);++if(!err&&fl_act.stats.lastused){+preempt_disable();+tcf_action_stats_update(action,fl_act.stats.bytes,+fl_act.stats.pkts,+fl_act.stats.drops,+fl_act.stats.lastused,+true);+preempt_enable();+action->used_hw_stats=fl_act.stats.used_hw_stats;+action->used_hw_stats_valid=true;+err=0;
Error handling here is slightly convoluted. This line assigns err=0
third time (it is initialized with zero and then we can only get here if
result of tcf_action_offload_cmd() assigned 'err' to zero again).
Considering that error handler in this function is empty we can just
return errors directly as soon as they happen and return zero at the end
of the function.
+ } else {
+ err = -EOPNOTSUPP;
Hmm the code can return error here when tcf_action_offload_cmd()
succeeded but 'lastused' is zero. Such behavior will cause
tcf_exts_stats_update() to update action with filter counter values. Is
this the desired behavior when, for example, in filter action list there
is and action that can drop packets followed by some shared action? In
such case 'lastused' can be zero if all packets that filter matched were
dropped by previous action and shared action will be assigned with
filter counter value that includes dropped packets/bytes.
@@ -1362,6 +1396,9 @@ int tcf_action_copy_stats(struct sk_buff *skb, struct tc_action *p, if (p == NULL) goto errout;+ /* update hw stats for this action */+ tcf_action_update_hw_stats(p);+ /* compat_mode being true specifies a call that is supposed * to add additional backward compatibility statistic TLVs. */
On Thu 28 Oct 2021 at 14:06, Simon Horman [off-list ref] wrote:
quoted hunk
From: Baowen Zheng <redacted>
Add reoffload process to update hw_count when driver
is inserted or removed.
When reoffloading actions, we still offload the actions
that are added independent of filters.
Signed-off-by: Baowen Zheng <redacted>
Signed-off-by: Louis Peens <redacted>
Signed-off-by: Simon Horman <redacted>
---
include/net/act_api.h | 24 +++++
include/net/pkt_cls.h | 5 +
net/core/flow_offload.c | 5 +
net/sched/act_api.c | 213 ++++++++++++++++++++++++++++++++++++----
4 files changed, 228 insertions(+), 19 deletions(-)
Don't know if it is a problem, but shouldn't tcf_action_reoffload_cb()
be called before flow_block_indr_notify(), which calls
flow_block_indr->cleanup() callbacks?
@@ -638,6 +638,59 @@ EXPORT_SYMBOL(tcf_idrinfo_destroy);staticLIST_HEAD(act_base);staticDEFINE_RWLOCK(act_mod_lock);+/* since act ops id is stored in pernet subsystem list,+*thenthereisnowaytowalkthroughonlyalltheaction+*subsystem,sowekeeptcactionpernetopsidfor+*reoffloadtowalkthrough.+*/+staticLIST_HEAD(act_pernet_id_list);+staticDEFINE_MUTEX(act_id_mutex);+structtc_act_pernet_id{+structlist_headlist;+unsignedintid;+};++staticinttcf_pernet_add_id_list(unsignedintid)+{+structtc_act_pernet_id*id_ptr;+intret=0;++mutex_lock(&act_id_mutex);+list_for_each_entry(id_ptr,&act_pernet_id_list,list){+if(id_ptr->id==id){+ret=-EEXIST;+gotoerr_out;+}+}++id_ptr=kzalloc(sizeof(*id_ptr),GFP_KERNEL);+if(!id_ptr){+ret=-ENOMEM;+gotoerr_out;+}+id_ptr->id=id;++list_add_tail(&id_ptr->list,&act_pernet_id_list);++err_out:+mutex_unlock(&act_id_mutex);+returnret;+}++staticvoidtcf_pernet_del_id_list(unsignedintid)+{+structtc_act_pernet_id*id_ptr;++mutex_lock(&act_id_mutex);+list_for_each_entry(id_ptr,&act_pernet_id_list,list){+if(id_ptr->id==id){+list_del(&id_ptr->list);+kfree(id_ptr);+break;+}+}+mutex_unlock(&act_id_mutex);+}inttcf_register_action(structtc_action_ops*act,structpernet_operations*ops)
@@ -656,18 +709,30 @@ int tcf_register_action(struct tc_action_ops *act,if(ret)returnret;+if(ops->id){+ret=tcf_pernet_add_id_list(*ops->id);+if(ret)+gotoid_err;+}+write_lock(&act_mod_lock);list_for_each_entry(a,&act_base,head){if(act->id==a->id||(strcmp(act->kind,a->kind)==0)){-write_unlock(&act_mod_lock);-unregister_pernet_subsys(ops);-return-EEXIST;+ret=-EEXIST;+gotoerr_out;}}list_add_tail(&act->head,&act_base);write_unlock(&act_mod_lock);return0;++err_out:+write_unlock(&act_mod_lock);+tcf_pernet_del_id_list(*ops->id);+id_err:+unregister_pernet_subsys(ops);+returnret;}EXPORT_SYMBOL(tcf_register_action);
@@ -686,8 +751,11 @@ int tcf_unregister_action(struct tc_action_ops *act,}}write_unlock(&act_mod_lock);-if(!err)+if(!err){unregister_pernet_subsys(ops);+if(ops->id)+tcf_pernet_del_id_list(*ops->id);+}returnerr;}EXPORT_SYMBOL(tcf_unregister_action);
@@ -1175,15 +1243,11 @@ static int flow_action_init(struct flow_offload_action *fl_action,return0;}-staticinttcf_action_offload_cmd(structflow_offload_action*fl_act,-u32*hw_count,-structnetlink_ext_ack*extack)+staticinttcf_action_offload_cmd_ex(structflow_offload_action*fl_act,+u32*hw_count){interr;-if(IS_ERR(fl_act))-returnPTR_ERR(fl_act);-err=flow_indr_dev_setup_offload(NULL,NULL,TC_SETUP_ACT,fl_act,NULL,NULL);if(err<0)
@@ -1195,9 +1259,41 @@ static int tcf_action_offload_cmd(struct flow_offload_action *fl_act,return0;}+staticinttcf_action_offload_cmd_cb_ex(structflow_offload_action*fl_act,+u32*hw_count,+flow_indr_block_bind_cb_t*cb,+void*cb_priv)+{+interr;++err=cb(NULL,NULL,cb_priv,TC_SETUP_ACT,NULL,fl_act,NULL);+if(err<0)+returnerr;++if(hw_count)+*hw_count=1;++return0;+}++staticinttcf_action_offload_cmd(structflow_offload_action*fl_act,+u32*hw_count,+flow_indr_block_bind_cb_t*cb,+void*cb_priv)+{+if(IS_ERR(fl_act))+returnPTR_ERR(fl_act);++returncb?tcf_action_offload_cmd_cb_ex(fl_act,hw_count,+cb,cb_priv):+tcf_action_offload_cmd_ex(fl_act,hw_count);+}+/* offload the tc command after inserted */-staticinttcf_action_offload_add(structtc_action*action,-structnetlink_ext_ack*extack)+staticinttcf_action_offload_add_ex(structtc_action*action,+structnetlink_ext_ack*extack,+flow_indr_block_bind_cb_t*cb,+void*cb_priv){boolskip_sw=tc_act_skip_sw(action->tcfa_flags);structtc_action*actions[TCA_ACT_MAX_PRIO]={
@@ -1225,9 +1321,10 @@ static int tcf_action_offload_add(struct tc_action *action,gotofl_err;}-err=tcf_action_offload_cmd(fl_action,&in_hw_count,extack);+err=tcf_action_offload_cmd(fl_action,&in_hw_count,cb,cb_priv);if(!err)-flow_action_hw_count_set(action,in_hw_count);+cb?flow_action_hw_count_inc(action,in_hw_count):+flow_action_hw_count_set(action,in_hw_count);if(skip_sw&&!tc_act_in_hw(action))err=-EINVAL;
@@ -1240,6 +1337,12 @@ static int tcf_action_offload_add(struct tc_action *action,returnerr;}+staticinttcf_action_offload_add(structtc_action*action,+structnetlink_ext_ack*extack)+{+returntcf_action_offload_add_ex(action,extack,NULL,NULL);+}+inttcf_action_update_hw_stats(structtc_action*action){structflow_offload_actionfl_act={};
@@ -1252,7 +1355,7 @@ int tcf_action_update_hw_stats(struct tc_action *action)if(err)gotoerr_out;-err=tcf_action_offload_cmd(&fl_act,NULL,NULL);+err=tcf_action_offload_cmd(&fl_act,NULL,NULL,NULL);if(!err&&fl_act.stats.lastused){preempt_disable();
@@ -1274,7 +1377,9 @@ int tcf_action_update_hw_stats(struct tc_action *action)}EXPORT_SYMBOL(tcf_action_update_hw_stats);-inttcf_action_offload_del(structtc_action*action)+staticinttcf_action_offload_del_ex(structtc_action*action,+flow_indr_block_bind_cb_t*cb,+void*cb_priv){structflow_offload_actionfl_act;u32in_hw_count=0;
@@ -1290,13 +1395,83 @@ int tcf_action_offload_del(struct tc_action *action)if(err)returnerr;-err=tcf_action_offload_cmd(&fl_act,&in_hw_count,NULL);-if(err)+err=tcf_action_offload_cmd(&fl_act,&in_hw_count,cb,cb_priv);+if(err<0)returnerr;-if(action->in_hw_count!=in_hw_count)+if(!cb&&action->in_hw_count!=in_hw_count)return-EINVAL;+/* do not need to update hw state when deleting action */+if(cb&&in_hw_count)+flow_action_hw_count_dec(action,in_hw_count);++return0;+}++inttcf_action_offload_del(structtc_action*action)+{+returntcf_action_offload_del_ex(action,NULL,NULL);+}++inttcf_action_reoffload_cb(flow_indr_block_bind_cb_t*cb,+void*cb_priv,booladd)+{+structtc_act_pernet_id*id_ptr;+structtcf_idrinfo*idrinfo;+structtc_action_net*tn;+structtc_action*p;+unsignedintact_id;+unsignedlongtmp;+unsignedlongid;+structidr*idr;+structnet*net;+intret;++if(!cb)+return-EINVAL;++down_read(&net_rwsem);+mutex_lock(&act_id_mutex);++for_each_net(net){+list_for_each_entry(id_ptr,&act_pernet_id_list,list){+act_id=id_ptr->id;+tn=net_generic(net,act_id);+if(!tn)+continue;+idrinfo=tn->idrinfo;+if(!idrinfo)+continue;++mutex_lock(&idrinfo->lock);+idr=&idrinfo->action_idr;+idr_for_each_entry_ul(idr,p,tmp,id){+if(IS_ERR(p)||tc_act_bind(p->tcfa_flags))+continue;+if(add){+tcf_action_offload_add_ex(p,NULL,cb,+cb_priv);+continue;+}++/* cb unregister to update hw count */+ret=tcf_action_offload_del_ex(p,cb,cb_priv);+if(ret<0)+continue;+if(tc_act_skip_sw(p->tcfa_flags)&&+!tc_act_in_hw(p)){+ret=tcf_idr_release_unsafe(p);+if(ret==ACT_P_DELETED)+module_put(p->ops->owner);+}+}+mutex_unlock(&idrinfo->lock);+}+}+mutex_unlock(&act_id_mutex);+up_read(&net_rwsem);+return0;}
On Thu 28 Oct 2021 at 14:06, Simon Horman [off-list ref] wrote:
quoted hunk
From: Baowen Zheng <redacted>
Add process to validate flags of filter and actions when adding
a tc filter.
We need to prevent adding filter with flags conflicts with its actions.
Signed-off-by: Baowen Zheng <redacted>
Signed-off-by: Louis Peens <redacted>
Signed-off-by: Simon Horman <redacted>
---
net/sched/cls_api.c | 26 ++++++++++++++++++++++++++
net/sched/cls_flower.c | 3 ++-
net/sched/cls_matchall.c | 4 ++--
net/sched/cls_u32.c | 7 ++++---
4 files changed, 34 insertions(+), 6 deletions(-)
I know Jamal suggested to have skip_sw for actions, but it complicates
the code and I'm still not entirely understand why it is necessary.
After all, action can only get applied to a packet if the packet has
been matched by some filter and filters already have skip sw/hw
controls. Forgoing action skip_sw flag would:
- Alleviate the need to validate that filter and action flags are
compatible. (trying to offload filter that points to existing skip_hw
action would just fail because the driver wouldn't find the action with
provided id in its tables)
- Remove the need to add more conditionals into TC software data path in
patch 4.
WDYT?
@@ -2035,7 +2035,8 @@ static int fl_change(struct net *net, struct sk_buff *in_skb,}err=fl_set_parms(net,tp,fnew,mask,base,tb,tca[TCA_RATE],-tp->chain->tmplt_priv,flags,extack);+tp->chain->tmplt_priv,flags|fnew->flags,+extack);
Aren't you or-ing flags from two different ranges (TCA_CLS_FLAGS_* and
TCA_ACT_FLAGS_*) that map to same bits, or am I missing something? This
isn't explained in commit message so it is hard for me to understand the
idea here.
From: Jamal Hadi Salim <jhs@mojatatu.com> Date: 2021-10-30 10:54:31
On 2021-10-29 14:01, Vlad Buslov wrote:
On Thu 28 Oct 2021 at 14:06, Simon Horman [off-list ref] wrote:
quoted
From: Baowen Zheng <redacted>
Add process to validate flags of filter and actions when adding
a tc filter.
We need to prevent adding filter with flags conflicts with its actions.
Signed-off-by: Baowen Zheng <redacted>
Signed-off-by: Louis Peens <redacted>
Signed-off-by: Simon Horman <redacted>
---
net/sched/cls_api.c | 26 ++++++++++++++++++++++++++
net/sched/cls_flower.c | 3 ++-
net/sched/cls_matchall.c | 4 ++--
net/sched/cls_u32.c | 7 ++++---
4 files changed, 34 insertions(+), 6 deletions(-)
I know Jamal suggested to have skip_sw for actions, but it complicates
the code and I'm still not entirely understand why it is necessary.
If the hardware can independently accept an action offload then
skip_sw per action makes total sense. BTW, my understanding is
_your_ hardware is capable as such at least for policers ;->
And such policers are then shared across filters.
Other than the architectural reason I may have missed something
because I dont see much complexity added as a result.
Are you more worried about slowing down the update rate?
cheers,
jamal
On Sat 30 Oct 2021 at 13:54, Jamal Hadi Salim [off-list ref] wrote:
On 2021-10-29 14:01, Vlad Buslov wrote:
quoted
On Thu 28 Oct 2021 at 14:06, Simon Horman [off-list ref] wrote:
quoted
From: Baowen Zheng <redacted>
Add process to validate flags of filter and actions when adding
a tc filter.
We need to prevent adding filter with flags conflicts with its actions.
Signed-off-by: Baowen Zheng <redacted>
Signed-off-by: Louis Peens <redacted>
Signed-off-by: Simon Horman <redacted>
---
net/sched/cls_api.c | 26 ++++++++++++++++++++++++++
net/sched/cls_flower.c | 3 ++-
net/sched/cls_matchall.c | 4 ++--
net/sched/cls_u32.c | 7 ++++---
4 files changed, 34 insertions(+), 6 deletions(-)
I know Jamal suggested to have skip_sw for actions, but it complicates
the code and I'm still not entirely understand why it is necessary.
If the hardware can independently accept an action offload then
skip_sw per action makes total sense. BTW, my understanding is
Example configuration that seems bizarre to me is when offloaded shared
action has skip_sw flag set but filter doesn't. Then behavior of
classifier that points to such action diverges between hardware and
software (different lists of actions are applied). We always try to make
offloaded TC data path behave exactly the same as software and, even
though here it would be explicit and deliberate, I don't see any
practical use-case for this.
_your_ hardware is capable as such at least for policers ;->
And such policers are then shared across filters.
True, but why do you need skip_sw action flag for that?
Other than the architectural reason I may have missed something
because I dont see much complexity added as a result.
Well, other part of my email was about how I don't understand what is
going on in the flags handling code here. This patch and parts of other
patches in the series would be unnecessary, if we forgo the action
skip_sw flag. I guess we can just make the code nicer by folding the
validation into tcf_action_init(), for example.
Are you more worried about slowing down the update rate?
I don't expect the validation code to significantly impact the update
rate.
From: Oz Shlomo <hidden> Date: 2021-10-31 09:50:22
On 10/28/2021 2:06 PM, Simon Horman wrote:
From: Baowen Zheng <redacted>
Use flow_indr_dev_register/flow_indr_dev_setup_offload to
offload tc action.
How will device drivers reference the offloaded actions when offloading a flow?
Perhaps the flow_action_entry structure should also include the action index.
quoted hunk
We need to call tc_cleanup_flow_action to clean up tc action entry since
in tc_setup_action, some actions may hold dev refcnt, especially the mirror
action.
Signed-off-by: Baowen Zheng <redacted>
Signed-off-by: Louis Peens <redacted>
Signed-off-by: Simon Horman <redacted>
---
include/linux/netdevice.h | 1 +
include/net/act_api.h | 2 +-
include/net/flow_offload.h | 17 ++++
include/net/pkt_cls.h | 15 ++++
net/core/flow_offload.c | 43 ++++++++--
net/sched/act_api.c | 166 +++++++++++++++++++++++++++++++++++++
net/sched/cls_api.c | 29 ++++++-
7 files changed, 260 insertions(+), 13 deletions(-)
@@ -916,6 +916,7 @@ enum tc_setup_type {TC_SETUP_QDISC_TBF,TC_SETUP_QDISC_FIFO,TC_SETUP_QDISC_HTB,+TC_SETUP_ACT,};/* These structures hold the attributes of bpf state that are being passed
@@ -1061,6 +1077,154 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,returnERR_PTR(err);}+staticintflow_action_init(structflow_offload_action*fl_action,+structtc_action*act,+enumflow_act_commandcmd,+structnetlink_ext_ack*extack)+{+if(!fl_action)+return-EINVAL;++fl_action->extack=extack;+fl_action->command=cmd;+fl_action->index=act->tcfa_index;++if(is_tcf_gact_ok(act)){+fl_action->id=FLOW_ACTION_ACCEPT;+}elseif(is_tcf_gact_shot(act)){+fl_action->id=FLOW_ACTION_DROP;+}elseif(is_tcf_gact_trap(act)){+fl_action->id=FLOW_ACTION_TRAP;+}elseif(is_tcf_gact_goto_chain(act)){+fl_action->id=FLOW_ACTION_GOTO;+}elseif(is_tcf_mirred_egress_redirect(act)){+fl_action->id=FLOW_ACTION_REDIRECT;+}elseif(is_tcf_mirred_egress_mirror(act)){+fl_action->id=FLOW_ACTION_MIRRED;+}elseif(is_tcf_mirred_ingress_redirect(act)){+fl_action->id=FLOW_ACTION_REDIRECT_INGRESS;+}elseif(is_tcf_mirred_ingress_mirror(act)){+fl_action->id=FLOW_ACTION_MIRRED_INGRESS;+}elseif(is_tcf_vlan(act)){+switch(tcf_vlan_action(act)){+caseTCA_VLAN_ACT_PUSH:+fl_action->id=FLOW_ACTION_VLAN_PUSH;+break;+caseTCA_VLAN_ACT_POP:+fl_action->id=FLOW_ACTION_VLAN_POP;+break;+caseTCA_VLAN_ACT_MODIFY:+fl_action->id=FLOW_ACTION_VLAN_MANGLE;+break;+default:+return-EOPNOTSUPP;+}+}elseif(is_tcf_tunnel_set(act)){+fl_action->id=FLOW_ACTION_TUNNEL_ENCAP;+}elseif(is_tcf_tunnel_release(act)){+fl_action->id=FLOW_ACTION_TUNNEL_DECAP;+}elseif(is_tcf_csum(act)){+fl_action->id=FLOW_ACTION_CSUM;+}elseif(is_tcf_skbedit_mark(act)){+fl_action->id=FLOW_ACTION_MARK;+}elseif(is_tcf_sample(act)){+fl_action->id=FLOW_ACTION_SAMPLE;+}elseif(is_tcf_police(act)){+fl_action->id=FLOW_ACTION_POLICE;+}elseif(is_tcf_ct(act)){+fl_action->id=FLOW_ACTION_CT;+}elseif(is_tcf_mpls(act)){+switch(tcf_mpls_action(act)){+caseTCA_MPLS_ACT_PUSH:+fl_action->id=FLOW_ACTION_MPLS_PUSH;+break;+caseTCA_MPLS_ACT_POP:+fl_action->id=FLOW_ACTION_MPLS_POP;+break;+caseTCA_MPLS_ACT_MODIFY:+fl_action->id=FLOW_ACTION_MPLS_MANGLE;+break;+default:+return-EOPNOTSUPP;+}+}elseif(is_tcf_skbedit_ptype(act)){+fl_action->id=FLOW_ACTION_PTYPE;+}elseif(is_tcf_skbedit_priority(act)){+fl_action->id=FLOW_ACTION_PRIORITY;+}elseif(is_tcf_gate(act)){+fl_action->id=FLOW_ACTION_GATE;+}else{+return-EOPNOTSUPP;+}++return0;+}++staticinttcf_action_offload_cmd(structflow_offload_action*fl_act,+structnetlink_ext_ack*extack)+{+interr;++if(IS_ERR(fl_act))+returnPTR_ERR(fl_act);++err=flow_indr_dev_setup_offload(NULL,NULL,TC_SETUP_ACT,+fl_act,NULL,NULL);+if(err<0)+returnerr;++return0;+}++/* offload the tc command after inserted */+staticinttcf_action_offload_add(structtc_action*action,+structnetlink_ext_ack*extack)+{+structtc_action*actions[TCA_ACT_MAX_PRIO]={+[0]=action,+};+structflow_offload_action*fl_action;+interr=0;++fl_action=flow_action_alloc(tcf_act_num_actions_single(action));+if(!fl_action)+return-EINVAL;++err=flow_action_init(fl_action,action,FLOW_ACT_REPLACE,extack);+if(err)+gotofl_err;++err=tc_setup_action(&fl_action->action,actions);+if(err){+NL_SET_ERR_MSG_MOD(extack,+"Failed to setup tc actions for offload\n");+gotofl_err;+}++err=tcf_action_offload_cmd(fl_action,extack);+tc_cleanup_flow_action(&fl_action->action);++fl_err:+kfree(fl_action);++returnerr;+}++inttcf_action_offload_del(structtc_action*action)+{+structflow_offload_actionfl_act;+interr=0;++if(!action)+return-EINVAL;++err=flow_action_init(&fl_act,action,FLOW_ACT_DESTROY,NULL);+if(err)+returnerr;++returntcf_action_offload_cmd(&fl_act,NULL);+}+/* Returns numbers of initialized actions or negative error. */inttcf_action_init(structnet*net,structtcf_proto*tp,structnlattr*nla,
@@ -1103,6 +1267,8 @@ int tcf_action_init(struct net *net, struct tcf_proto *tp, struct nlattr *nla,sz+=tcf_action_fill_size(act);/* Start from index 0 */actions[i-1]=act;+if(!(flags&TCA_ACT_FLAGS_BIND))+tcf_action_offload_add(act,extack);
Why is this restricted to actions created without the TCA_ACT_FLAGS_BIND flag?
How are actions instantiated by the filters different from those that are created by "tc actions"?
quoted hunk
}
/* We have to commit them all together, because if any error happened in
From: Oz Shlomo <hidden> Date: 2021-10-31 09:50:56
On 10/28/2021 2:06 PM, Simon Horman wrote:
Baowen Zheng says:
Allow use of flow_indr_dev_register/flow_indr_dev_setup_offload to offload
tc actions independent of flows.
The motivation for this work is to prepare for using TC police action
instances to provide hardware offload of OVS metering feature - which calls
for policers that may be used by multiple flows and whose lifecycle is
independent of any flows that use them.
This patch includes basic changes to offload drivers to return EOPNOTSUPP
if this feature is used - it is not yet supported by any driver.
Tc cli command to offload and quote an action:
tc qdisc del dev $DEV ingress && sleep 1 || true
tc actions delete action police index 99 || true
tc qdisc add dev $DEV ingress
tc qdisc show dev $DEV ingress
tc actions add action police index 99 rate 1mbit burst 100k skip_sw
tc actions list action police
tc filter add dev $DEV protocol ip parent ffff:
flower ip_proto tcp action police index 99
tc -s -d filter show dev $DEV protocol ip parent ffff:
tc filter add dev $DEV protocol ipv6 parent ffff:
flower skip_sw ip_proto tcp action police index 99
tc -s -d filter show dev $DEV protocol ipv6 parent ffff:
tc actions list action police
tc qdisc del dev $DEV ingress && sleep 1
tc actions delete action police index 99
tc actions list action police
Actions are also (implicitly) instantiated when filters are created.
In the following example the mirred action instance (created by the first filter) is shared by the
second filter:
tc filter add dev $DEV1 proto ip parent ffff: flower \
ip_proto tcp action mirred egress redirect dev $DEV3
tc filter add dev $DEV2 proto ip parent ffff: flower \
ip_proto tcp action mirred index 1
Changes compared to v2 patches:
* Made changes according to the review comments.
* Delete in_hw and not_in_hw flag and user can judge if the action is
offloaded to any hardware by in_hw_count.
* Split the main patch of the action offload to three single patch to
facilitate code review.
Posting this revision of the patchset as an RFC as while we feel it is
ready for review we would like an opportunity to conduct further testing
before acceptance into upstream.
Baowen Zheng (8):
flow_offload: fill flags to action structure
flow_offload: reject to offload tc actions in offload drivers
flow_offload: allow user to offload tc action to net device
flow_offload: add skip_hw and skip_sw to control if offload the action
flow_offload: add process to update action stats from hardware
net: sched: save full flags for tc action
flow_offload: add reoffload process to update hw_count
flow_offload: validate flags of filter and actions
drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c | 2 +-
.../ethernet/mellanox/mlx5/core/en/rep/tc.c | 3 +
.../ethernet/netronome/nfp/flower/offload.c | 3 +
include/linux/netdevice.h | 1 +
include/net/act_api.h | 34 +-
include/net/flow_offload.h | 17 +
include/net/pkt_cls.h | 61 ++-
include/uapi/linux/pkt_cls.h | 9 +-
net/core/flow_offload.c | 48 +-
net/sched/act_api.c | 440 +++++++++++++++++-
net/sched/act_bpf.c | 2 +-
net/sched/act_connmark.c | 2 +-
net/sched/act_ctinfo.c | 2 +-
net/sched/act_gate.c | 2 +-
net/sched/act_ife.c | 2 +-
net/sched/act_ipt.c | 2 +-
net/sched/act_mpls.c | 2 +-
net/sched/act_nat.c | 2 +-
net/sched/act_pedit.c | 2 +-
net/sched/act_police.c | 2 +-
net/sched/act_sample.c | 2 +-
net/sched/act_simple.c | 2 +-
net/sched/act_skbedit.c | 2 +-
net/sched/act_skbmod.c | 2 +-
net/sched/cls_api.c | 55 ++-
net/sched/cls_flower.c | 3 +-
net/sched/cls_matchall.c | 4 +-
net/sched/cls_u32.c | 7 +-
28 files changed, 661 insertions(+), 54 deletions(-)
From: Dave Taht <hidden> Date: 2021-10-31 12:03:34
On Sun, Oct 31, 2021 at 2:51 AM Oz Shlomo [off-list ref] wrote:
On 10/28/2021 2:06 PM, Simon Horman wrote:
quoted
Baowen Zheng says:
Allow use of flow_indr_dev_register/flow_indr_dev_setup_offload to offload
tc actions independent of flows.
The motivation for this work is to prepare for using TC police action
instances to provide hardware offload of OVS metering feature - which calls
for policers that may be used by multiple flows and whose lifecycle is
independent of any flows that use them.
This patch includes basic changes to offload drivers to return EOPNOTSUPP
if this feature is used - it is not yet supported by any driver.
Tc cli command to offload and quote an action:
tc qdisc del dev $DEV ingress && sleep 1 || true
tc actions delete action police index 99 || true
tc qdisc add dev $DEV ingress
tc qdisc show dev $DEV ingress
tc actions add action police index 99 rate 1mbit burst 100k skip_sw
tc actions list action police
tc filter add dev $DEV protocol ip parent ffff:
flower ip_proto tcp action police index 99
tc -s -d filter show dev $DEV protocol ip parent ffff:
tc filter add dev $DEV protocol ipv6 parent ffff:
flower skip_sw ip_proto tcp action police index 99
tc -s -d filter show dev $DEV protocol ipv6 parent ffff:
tc actions list action police
tc qdisc del dev $DEV ingress && sleep 1
tc actions delete action police index 99
tc actions list action police
Actions are also (implicitly) instantiated when filters are created.
In the following example the mirred action instance (created by the first filter) is shared by the
second filter:
tc filter add dev $DEV1 proto ip parent ffff: flower \
ip_proto tcp action mirred egress redirect dev $DEV3
tc filter add dev $DEV2 proto ip parent ffff: flower \
ip_proto tcp action mirred index 1
quoted
Changes compared to v2 patches:
* Made changes according to the review comments.
* Delete in_hw and not_in_hw flag and user can judge if the action is
offloaded to any hardware by in_hw_count.
* Split the main patch of the action offload to three single patch to
facilitate code review.
Posting this revision of the patchset as an RFC as while we feel it is
ready for review we would like an opportunity to conduct further testing
before acceptance into upstream.
Baowen Zheng (8):
flow_offload: fill flags to action structure
flow_offload: reject to offload tc actions in offload drivers
flow_offload: allow user to offload tc action to net device
flow_offload: add skip_hw and skip_sw to control if offload the action
flow_offload: add process to update action stats from hardware
net: sched: save full flags for tc action
flow_offload: add reoffload process to update hw_count
flow_offload: validate flags of filter and actions
drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c | 2 +-
.../ethernet/mellanox/mlx5/core/en/rep/tc.c | 3 +
.../ethernet/netronome/nfp/flower/offload.c | 3 +
include/linux/netdevice.h | 1 +
include/net/act_api.h | 34 +-
include/net/flow_offload.h | 17 +
include/net/pkt_cls.h | 61 ++-
include/uapi/linux/pkt_cls.h | 9 +-
net/core/flow_offload.c | 48 +-
net/sched/act_api.c | 440 +++++++++++++++++-
net/sched/act_bpf.c | 2 +-
net/sched/act_connmark.c | 2 +-
net/sched/act_ctinfo.c | 2 +-
net/sched/act_gate.c | 2 +-
net/sched/act_ife.c | 2 +-
net/sched/act_ipt.c | 2 +-
net/sched/act_mpls.c | 2 +-
net/sched/act_nat.c | 2 +-
net/sched/act_pedit.c | 2 +-
net/sched/act_police.c | 2 +-
net/sched/act_sample.c | 2 +-
net/sched/act_simple.c | 2 +-
net/sched/act_skbedit.c | 2 +-
net/sched/act_skbmod.c | 2 +-
net/sched/cls_api.c | 55 ++-
net/sched/cls_flower.c | 3 +-
net/sched/cls_matchall.c | 4 +-
net/sched/cls_u32.c | 7 +-
28 files changed, 661 insertions(+), 54 deletions(-)
Just as an on-going grump: It has been my hope that policing as a
technique would have died a horrible death by now. Seeing it come back
as an "easy to offload" operation here - fresh from the 1990s! does
not mean it's a good idea, and I'd rather like it if we were finding
ways to
offload newer things that work better, such as modern aqm, fair
queuing, and shaping technologies that are in pie, fq_codel, and cake.
policing leads to bursty loss, especially at higher rates, BBR has a
specific mode designed to defeat it, and I ripped it out of
wondershaper
long ago for very good reasons:
https://www.bufferbloat.net/projects/bloat/wiki/Wondershaper_Must_Die/
I did a long time ago start working on a better policing idea based on
some good aqm ideas like AFD, but dropped it figuring that policing
was going to vanish
from the planet. It's baaaaaack.
--
I tried to build a better future, a few times:
https://wayforward.archive.org/?site=https%3A%2F%2Fwww.icei.org
Dave Täht CEO, TekLibre, LLC
From: Jamal Hadi Salim <jhs@mojatatu.com> Date: 2021-10-31 13:40:20
On 2021-10-31 05:50, Oz Shlomo wrote:
On 10/28/2021 2:06 PM, Simon Horman wrote:
quoted
Baowen Zheng says:
Allow use of flow_indr_dev_register/flow_indr_dev_setup_offload to
offload
tc actions independent of flows.
The motivation for this work is to prepare for using TC police action
instances to provide hardware offload of OVS metering feature - which
calls
for policers that may be used by multiple flows and whose lifecycle is
independent of any flows that use them.
This patch includes basic changes to offload drivers to return EOPNOTSUPP
if this feature is used - it is not yet supported by any driver.
Tc cli command to offload and quote an action:
tc qdisc del dev $DEV ingress && sleep 1 || true
tc actions delete action police index 99 || true
tc qdisc add dev $DEV ingress
tc qdisc show dev $DEV ingress
tc actions add action police index 99 rate 1mbit burst 100k skip_sw
tc actions list action police
tc filter add dev $DEV protocol ip parent ffff:
flower ip_proto tcp action police index 99
tc -s -d filter show dev $DEV protocol ip parent ffff:
tc filter add dev $DEV protocol ipv6 parent ffff:
flower skip_sw ip_proto tcp action police index 99
tc -s -d filter show dev $DEV protocol ipv6 parent ffff:
tc actions list action police
tc qdisc del dev $DEV ingress && sleep 1
tc actions delete action police index 99
tc actions list action police
Actions are also (implicitly) instantiated when filters are created.
In the following example the mirred action instance (created by the
first filter) is shared by the second filter:
tc filter add dev $DEV1 proto ip parent ffff: flower \
ip_proto tcp action mirred egress redirect dev $DEV3
tc filter add dev $DEV2 proto ip parent ffff: flower \
ip_proto tcp action mirred index 1
I sure hope this is supported. At least the discussions so far
are a nod in that direction...
I know there is hardware that is not capable of achieving this
(little CPE type devices) but lets not make that the common case.
cheers,
jamal
From: Jamal Hadi Salim <jhs@mojatatu.com> Date: 2021-10-31 14:14:27
On 2021-10-31 08:03, Dave Taht wrote:
[..]
Just as an on-going grump: It has been my hope that policing as a
technique would have died a horrible death by now. Seeing it come back
as an "easy to offload" operation here - fresh from the 1990s! does
not mean it's a good idea, and I'd rather like it if we were finding
ways to
offload newer things that work better, such as modern aqm, fair
queuing, and shaping technologies that are in pie, fq_codel, and cake.
policing leads to bursty loss, especially at higher rates, BBR has a
specific mode designed to defeat it, and I ripped it out of
wondershaper
long ago for very good reasons:
https://www.bufferbloat.net/projects/bloat/wiki/Wondershaper_Must_Die/
I did a long time ago start working on a better policing idea based on
some good aqm ideas like AFD, but dropped it figuring that policing
was going to vanish
from the planet. It's baaaaaack.
A lot of enthusiasm for fq_codel in that link ;->
Root cause for burstiness is typically due to large transient queues
(which are sometimes not under your admin control) and of course if
you use a policer and dont have your double leaky buckets set properly
to compensate for both short and long term rates you will have bursts
of drops with the policer. It would be the same with shaper as well
if the packet burst shows up when the queue is full.
Intuitively it would feel, for non-work conserving approaches,
delaying a packet (as in shaping) is better than dropping (as in
policing) - but i have not a study which scientifically proves it.
Any pointers in that regard?
TCP would recover either way (either detecting sequence gaps or RTO).
In Linux kernel level i am not sure i see much difference in either
since we actually feedback an indicator to TCP to indicate a local
drop (as opposed to guessing when it is dropped in the network)
and the TCP code is smart enough to utilize that knowledge.
For hardware offload there is no such feedback for either of those
two approaches (so no difference with drop in the blackhole).
As to "policer must die" - not possible i am afraid;-> I mean there
has to be strong evidence that it is a bad idea and besides that
_a lot of hardware_ supports it;-> Ergo, we have to support it as well.
Note: RED for example has been proven almost impossible to configure
properly but we still support it and there's a good set of hardware
offload support for it. For RED - and i should say the policer as well -
if you configure properly, _it works_.
BTW, Some mellanox NICs offload HTB. See for example:
https://legacy.netdevconf.info/0x14/session.html?talk-hierarchical-QoS-hardware-offload
cheers,
jamal
From: Baowen Zheng <redacted>
Use flow_indr_dev_register/flow_indr_dev_setup_offload to offload tc
action.
How will device drivers reference the offloaded actions when offloading a
flow?
Perhaps the flow_action_entry structure should also include the action index.
We have set action index in flow_offload_action to offload the action, also there are
already some actions in flow_action_entry include index which we want to offload.
If the driver wants to support action that needs index, I think it can add the index later,
it may not include in this patch, WDYT?
quoted
We need to call tc_cleanup_flow_action to clean up tc action entry
since in tc_setup_action, some actions may hold dev refcnt, especially
the mirror action.
Signed-off-by: Baowen Zheng <redacted>
Signed-off-by: Louis Peens <redacted>
Signed-off-by: Simon Horman <redacted>
---
include/linux/netdevice.h | 1 +
include/net/act_api.h | 2 +-
include/net/flow_offload.h | 17 ++++
include/net/pkt_cls.h | 15 ++++
net/core/flow_offload.c | 43 ++++++++--
net/sched/act_api.c | 166 +++++++++++++++++++++++++++++++++++++
net/sched/cls_api.c | 29 ++++++-
7 files changed, 260 insertions(+), 13 deletions(-)
@@ -916,6 +916,7 @@ enum tc_setup_type {TC_SETUP_QDISC_TBF,TC_SETUP_QDISC_FIFO,TC_SETUP_QDISC_HTB,+TC_SETUP_ACT,};/* These structures hold the attributes of bpf state that are being
passed diff --git a/include/net/act_api.h b/include/net/act_api.h
index b5b624c7e488..9eb19188603c 100644
sz += tcf_action_fill_size(act);
/* Start from index 0 */
actions[i - 1] = act;
+ if (!(flags & TCA_ACT_FLAGS_BIND))
+ tcf_action_offload_add(act, extack);
Why is this restricted to actions created without the TCA_ACT_FLAGS_BIND
flag?
How are actions instantiated by the filters different from those that are
created by "tc actions"?
Our patch aims to offload tc action that is created independent of any flow. It is usually
offloaded when it is added or replaced.
This patch is to implement a process of reoffloading the actions when driver is
inserted or removed, so it will still offload the independent actions.
quoted
}
/* We have to commit them all together, because if any error
happened in diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 2ef8f5a6205a..351d93988b8b 100644
On Sun 31 Oct 2021 at 15:40, Jamal Hadi Salim [off-list ref] wrote:
On 2021-10-31 05:50, Oz Shlomo wrote:
quoted
On 10/28/2021 2:06 PM, Simon Horman wrote:
quoted
Baowen Zheng says:
Allow use of flow_indr_dev_register/flow_indr_dev_setup_offload to offload
tc actions independent of flows.
The motivation for this work is to prepare for using TC police action
instances to provide hardware offload of OVS metering feature - which calls
for policers that may be used by multiple flows and whose lifecycle is
independent of any flows that use them.
This patch includes basic changes to offload drivers to return EOPNOTSUPP
if this feature is used - it is not yet supported by any driver.
Tc cli command to offload and quote an action:
tc qdisc del dev $DEV ingress && sleep 1 || true
tc actions delete action police index 99 || true
tc qdisc add dev $DEV ingress
tc qdisc show dev $DEV ingress
tc actions add action police index 99 rate 1mbit burst 100k skip_sw
tc actions list action police
tc filter add dev $DEV protocol ip parent ffff:
flower ip_proto tcp action police index 99
tc -s -d filter show dev $DEV protocol ip parent ffff:
tc filter add dev $DEV protocol ipv6 parent ffff:
flower skip_sw ip_proto tcp action police index 99
tc -s -d filter show dev $DEV protocol ipv6 parent ffff:
tc actions list action police
tc qdisc del dev $DEV ingress && sleep 1
tc actions delete action police index 99
tc actions list action police
Actions are also (implicitly) instantiated when filters are created.
In the following example the mirred action instance (created by the first
filter) is shared by the second filter:
tc filter add dev $DEV1 proto ip parent ffff: flower \
ip_proto tcp action mirred egress redirect dev $DEV3
tc filter add dev $DEV2 proto ip parent ffff: flower \
ip_proto tcp action mirred index 1
I sure hope this is supported. At least the discussions so far
are a nod in that direction...
I know there is hardware that is not capable of achieving this
(little CPE type devices) but lets not make that the common case.
Looks like it isn't supported in this change since
tcf_action_offload_add() is only called by tcf_action_init() when BIND
flag is not set (the flag is always set when called from cls code).
Moreover, I don't think it is good idea to support such use-case because
that would require to increase number of calls to driver offload
infrastructure from 1 per filter to 1+number_of_actions, which would
significantly impact insertion rate.
Thanks for your review and sorry for delay in responding.
On October 30, 2021 12:59 AM, Vlad Buslov [off-list ref] wrote:
On Thu 28 Oct 2021 at 14:06, Simon Horman [off-list ref]
wrote:
quoted
From: Baowen Zheng <redacted>
Use flow_indr_dev_register/flow_indr_dev_setup_offload to offload tc
action.
We need to call tc_cleanup_flow_action to clean up tc action entry
since in tc_setup_action, some actions may hold dev refcnt, especially
the mirror action.
Signed-off-by: Baowen Zheng <redacted>
Signed-off-by: Louis Peens <redacted>
Signed-off-by: Simon Horman <redacted>
---
include/linux/netdevice.h | 1 +
include/net/act_api.h | 2 +-
include/net/flow_offload.h | 17 ++++
include/net/pkt_cls.h | 15 ++++
net/core/flow_offload.c | 43 ++++++++--
net/sched/act_api.c | 166
@@ -916,6 +916,7 @@ enum tc_setup_type {TC_SETUP_QDISC_TBF,TC_SETUP_QDISC_FIFO,TC_SETUP_QDISC_HTB,+TC_SETUP_ACT,};/* These structures hold the attributes of bpf state that are being
passed diff --git a/include/net/act_api.h b/include/net/act_api.h
index b5b624c7e488..9eb19188603c 100644
for (; 0; (void)(i), (void)(a), (void)(exts)) #endif
+#define tcf_act_for_each_action(i, a, actions) \
+ for (i = 0; i < TCA_ACT_MAX_PRIO && ((a) = actions[i]); i++)
+
static inline void
tcf_exts_stats_update(const struct tcf_exts *exts,
u64 bytes, u64 packets, u64 drops, u64 lastuse, @@ -532,8
+535,19 @@ tcf_match_indev(struct sk_buff *skb, int ifindex)
return ifindex == skb->skb_iif;
}
+#ifdef CONFIG_NET_CLS_ACT
int tc_setup_flow_action(struct flow_action *flow_action,
const struct tcf_exts *exts);
Why does existing cls_api function tc_setup_flow_action() now depend on
CONFIG_NET_CLS_ACT?
Originally the function tc_setup_flow_action deal with the dependence of CONFIG_NET_CLS_ACT
By calling the macro tcf_exts_for_each_action, now we change to call the function tc_setup_action
Then tc_setup_flow_action will refer to exts->actions, so it will depend on CONFIG_NET_CLS_ACT explicitly.
To fix this, we have to have the ifdef in tc_setup_flow_action declaration or in the implement in cls_api.c.
Do you think if it makes sense?
tcf_action_offload_del() and tcf_action_cleanup() seem to be always called
together. Consider moving the call to tcf_action_offload_del() into
tcf_action_cleanup().
Thanks, we will consider to move tcf_action_offload_del() inside of tcf_action_cleanup.
@@ -1061,6 +1077,154 @@ struct tc_action *tcf_action_init_1(struct net
*net, struct tcf_proto *tp,
quoted
return ERR_PTR(err);
}
...
quoted
+/* offload the tc command after inserted */ static int
+tcf_action_offload_add(struct tc_action *action,
+ struct netlink_ext_ack *extack) {
+ struct tc_action *actions[TCA_ACT_MAX_PRIO] = {
+ [0] = action,
+ };
+ struct flow_offload_action *fl_action;
+ int err = 0;
+
+ fl_action = flow_action_alloc(tcf_act_num_actions_single(action));
+ if (!fl_action)
+ return -EINVAL;
Failed alloc-like functions usually result -ENOMEM.
Thanks, we will fix this in V4 patch.
quoted
+
+ err = flow_action_init(fl_action, action, FLOW_ACT_REPLACE, extack);
+ if (err)
+ goto fl_err;
+
+ err = tc_setup_action(&fl_action->action, actions);
+ if (err) {
+ NL_SET_ERR_MSG_MOD(extack,
+ "Failed to setup tc actions for offload\n");
+ goto fl_err;
+ }
+
+ err = tcf_action_offload_cmd(fl_action, extack);
+ tc_cleanup_flow_action(&fl_action->action);
+
+fl_err:
+ kfree(fl_action);
+
+ return err;
+}
+
+int tcf_action_offload_del(struct tc_action *action) {
+ struct flow_offload_action fl_act;
+ int err = 0;
+
+ if (!action)
+ return -EINVAL;
+
+ err = flow_action_init(&fl_act, action, FLOW_ACT_DESTROY, NULL);
+ if (err)
+ return err;
+
+ return tcf_action_offload_cmd(&fl_act, NULL); }
+
/* Returns numbers of initialized actions or negative error. */
int tcf_action_init(struct net *net, struct tcf_proto *tp, struct
nlattr *nla, @@ -1103,6 +1267,8 @@ int tcf_action_init(struct net *net,
struct tcf_proto *tp, struct nlattr *nla,
quoted
sz += tcf_action_fill_size(act);
/* Start from index 0 */
actions[i - 1] = act;
+ if (!(flags & TCA_ACT_FLAGS_BIND))
+ tcf_action_offload_add(act, extack);
}
/* We have to commit them all together, because if any error
happened in diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 2ef8f5a6205a..351d93988b8b 100644
Maybe just move tc_setup_action() to act_api and ifdef its definition in
pkt_cls.h instead of existing tc_setup_flow_action()?
As explanation above, after the change, tc_setup_flow_action will call function of
tc_setup_action and refer to exts->actions, so just move tc_setup_action can not
fix this problem.
From: Oz Shlomo <hidden> Date: 2021-11-01 10:07:38
On 11/1/2021 4:30 AM, Baowen Zheng wrote:
On 10/31/2021 5:50 PM, Oz Shlomo wrote:
quoted
On 10/28/2021 2:06 PM, Simon Horman wrote:
quoted
From: Baowen Zheng <redacted>
Use flow_indr_dev_register/flow_indr_dev_setup_offload to offload tc
action.
How will device drivers reference the offloaded actions when offloading a
flow?
Perhaps the flow_action_entry structure should also include the action index.
We have set action index in flow_offload_action to offload the action, also there are > already some actions in flow_action_entry include index which we want to offload.
If the driver wants to support action that needs index, I think it can add the index later,
it may not include in this patch, WDYT?
What do you mean by "action that needs index"?
Currently only the police and gate actions have an action index parameter.
However, with this series the user can create any action using the tc action API and then reference
it from any filter.
Do you see a reason not to expose the action index as a flow_action_entry attribute?
quoted
quoted
We need to call tc_cleanup_flow_action to clean up tc action entry
since in tc_setup_action, some actions may hold dev refcnt, especially
the mirror action.
Signed-off-by: Baowen Zheng <redacted>
Signed-off-by: Louis Peens <redacted>
Signed-off-by: Simon Horman <redacted>
---
include/linux/netdevice.h | 1 +
include/net/act_api.h | 2 +-
include/net/flow_offload.h | 17 ++++
include/net/pkt_cls.h | 15 ++++
net/core/flow_offload.c | 43 ++++++++--
net/sched/act_api.c | 166 +++++++++++++++++++++++++++++++++++++
net/sched/cls_api.c | 29 ++++++-
7 files changed, 260 insertions(+), 13 deletions(-)
@@ -916,6 +916,7 @@ enum tc_setup_type {TC_SETUP_QDISC_TBF,TC_SETUP_QDISC_FIFO,TC_SETUP_QDISC_HTB,+TC_SETUP_ACT,};/* These structures hold the attributes of bpf state that are being
passed diff --git a/include/net/act_api.h b/include/net/act_api.h
index b5b624c7e488..9eb19188603c 100644
sz += tcf_action_fill_size(act);
/* Start from index 0 */
actions[i - 1] = act;
+ if (!(flags & TCA_ACT_FLAGS_BIND))
+ tcf_action_offload_add(act, extack);
Why is this restricted to actions created without the TCA_ACT_FLAGS_BIND
flag?
How are actions instantiated by the filters different from those that are
created by "tc actions"?
Our patch aims to offload tc action that is created independent of any flow. It is usually
offloaded when it is added or replaced.
This patch is to implement a process of reoffloading the actions when driver is
inserted or removed, so it will still offload the independent actions.
I see.
quoted
quoted
}
/* We have to commit them all together, because if any error
happened in diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 2ef8f5a6205a..351d93988b8b 100644
On October 30, 2021 1:11 AM, Vlad Buslov [off-list ref] wrote:
On Thu 28 Oct 2021 at 14:06, Simon Horman [off-list ref]
wrote:
quoted
From: Baowen Zheng <redacted>
When collecting stats for actions update them using both both hardware
and software counters.
Stats update process should not in context of preempt_disable.
Error handling here is slightly convoluted. This line assigns err=0 third time (it is
initialized with zero and then we can only get here if result of
tcf_action_offload_cmd() assigned 'err' to zero again).
Considering that error handler in this function is empty we can just return
errors directly as soon as they happen and return zero at the end of the
function.
Thanks, we will change as your suggestion.
quoted
+ } else {
+ err = -EOPNOTSUPP;
Hmm the code can return error here when tcf_action_offload_cmd()
succeeded but 'lastused' is zero. Such behavior will cause
tcf_exts_stats_update() to update action with filter counter values. Is this the
desired behavior when, for example, in filter action list there is and action that
can drop packets followed by some shared action? In such case 'lastused' can
be zero if all packets that filter matched were dropped by previous action and
shared action will be assigned with filter counter value that includes dropped
packets/bytes.
Thanks, we will consider if it make sense to only judge return value err from tcf_action_offload_cmd.
@@ -1362,6 +1396,9 @@ int tcf_action_copy_stats(struct sk_buff *skb,
struct tc_action *p,
quoted
if (p == NULL)
goto errout;
+ /* update hw stats for this action */
+ tcf_action_update_hw_stats(p);
+
/* compat_mode being true specifies a call that is supposed
* to add additional backward compatibility statistic TLVs.
*/
From: Baowen Zheng <redacted>
Use flow_indr_dev_register/flow_indr_dev_setup_offload to offload tc
action.
How will device drivers reference the offloaded actions when
offloading a flow?
Perhaps the flow_action_entry structure should also include the action
index.
quoted
quoted
We have set action index in flow_offload_action to offload the action, also
there are > already some actions in flow_action_entry include index which we
want to offload.
quoted
If the driver wants to support action that needs index, I think it can
add the index later, it may not include in this patch, WDYT?
What do you mean by "action that needs index"?
Currently only the police and gate actions have an action index parameter.
However, with this series the user can create any action using the tc action API
and then reference it from any filter.
Do you see a reason not to expose the action index as a flow_action_entry
attribute?
What I mean is currently the action is created along with the filter, then the index is not needed.
With this patch, we intend to offload the police action which already includes action index.
I think your suggestion makes sense to us, we will consider to move the index to the
flow_action_entry structure instead of current in single action structure, thanks.
quoted
quoted
quoted
We need to call tc_cleanup_flow_action to clean up tc action entry
since in tc_setup_action, some actions may hold dev refcnt, especially
the mirror action.
Signed-off-by: Baowen Zheng <redacted>
Signed-off-by: Louis Peens <redacted>
Signed-off-by: Simon Horman <redacted>
---
include/linux/netdevice.h | 1 +
include/net/act_api.h | 2 +-
include/net/flow_offload.h | 17 ++++
include/net/pkt_cls.h | 15 ++++
net/core/flow_offload.c | 43 ++++++++--
net/sched/act_api.c | 166
int tcf_action_init(struct net *net, struct tcf_proto *tp, struct
nlattr *nla, @@ -1103,6 +1267,8 @@ int tcf_action_init(struct net *net,
struct tcf_proto *tp, struct nlattr *nla,
quoted
sz += tcf_action_fill_size(act);
/* Start from index 0 */
actions[i - 1] = act;
+ if (!(flags & TCA_ACT_FLAGS_BIND))
+ tcf_action_offload_add(act, extack);
Why is this restricted to actions created without the
TCA_ACT_FLAGS_BIND
quoted
quoted
flag?
How are actions instantiated by the filters different from those that are
created by "tc actions"?
Our patch aims to offload tc action that is created independent of any flow.
It is usually
quoted
offloaded when it is added or replaced.
This patch is to implement a process of reoffloading the actions when driver
is
quoted
inserted or removed, so it will still offload the independent actions.
I see.
quoted
quoted
quoted
}
/* We have to commit them all together, because if any error
happened in diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 2ef8f5a6205a..351d93988b8b 100644
On Mon 01 Nov 2021 at 11:44, Baowen Zheng [off-list ref] wrote:
Thanks for your review and sorry for delay in responding.
On October 30, 2021 12:59 AM, Vlad Buslov [off-list ref] wrote:
quoted
On Thu 28 Oct 2021 at 14:06, Simon Horman [off-list ref]
wrote:
quoted
From: Baowen Zheng <redacted>
Use flow_indr_dev_register/flow_indr_dev_setup_offload to offload tc
action.
We need to call tc_cleanup_flow_action to clean up tc action entry
since in tc_setup_action, some actions may hold dev refcnt, especially
the mirror action.
Signed-off-by: Baowen Zheng <redacted>
Signed-off-by: Louis Peens <redacted>
Signed-off-by: Simon Horman <redacted>
---
include/linux/netdevice.h | 1 +
include/net/act_api.h | 2 +-
include/net/flow_offload.h | 17 ++++
include/net/pkt_cls.h | 15 ++++
net/core/flow_offload.c | 43 ++++++++--
net/sched/act_api.c | 166
@@ -916,6 +916,7 @@ enum tc_setup_type {TC_SETUP_QDISC_TBF,TC_SETUP_QDISC_FIFO,TC_SETUP_QDISC_HTB,+TC_SETUP_ACT,};/* These structures hold the attributes of bpf state that are being
passed diff --git a/include/net/act_api.h b/include/net/act_api.h
index b5b624c7e488..9eb19188603c 100644
for (; 0; (void)(i), (void)(a), (void)(exts)) #endif
+#define tcf_act_for_each_action(i, a, actions) \
+ for (i = 0; i < TCA_ACT_MAX_PRIO && ((a) = actions[i]); i++)
+
static inline void
tcf_exts_stats_update(const struct tcf_exts *exts,
u64 bytes, u64 packets, u64 drops, u64 lastuse, @@ -532,8
+535,19 @@ tcf_match_indev(struct sk_buff *skb, int ifindex)
return ifindex == skb->skb_iif;
}
+#ifdef CONFIG_NET_CLS_ACT
int tc_setup_flow_action(struct flow_action *flow_action,
const struct tcf_exts *exts);
Why does existing cls_api function tc_setup_flow_action() now depend on
CONFIG_NET_CLS_ACT?
Originally the function tc_setup_flow_action deal with the dependence of CONFIG_NET_CLS_ACT
By calling the macro tcf_exts_for_each_action, now we change to call the function tc_setup_action
Then tc_setup_flow_action will refer to exts->actions, so it will depend on CONFIG_NET_CLS_ACT explicitly.
To fix this, we have to have the ifdef in tc_setup_flow_action declaration or in the implement in cls_api.c.
Do you think if it makes sense?
Since we already have multiple of such ifdefs in cls_api I don't think
having more is an issue, but I also don't think we need to ifdef this
function in both pkt_cls.h and cls_api.c. Unless I'm missing something
you can either:
- Make tc_setup_flow_action() inline in pkt_cls.h and remove its
definition from cls_api.c since tc_setup_action() is also exported.
- Move ifdef check inside function definition in cls_api.c (return 0, if
config is not defined), which will allows you to remove ifdef from
pkt_cls.h.
WDYT?
tcf_action_offload_del() and tcf_action_cleanup() seem to be always called
together. Consider moving the call to tcf_action_offload_del() into
tcf_action_cleanup().
Thanks, we will consider to move tcf_action_offload_del() inside of tcf_action_cleanup.
@@ -1061,6 +1077,154 @@ struct tc_action *tcf_action_init_1(struct net
*net, struct tcf_proto *tp,
quoted
return ERR_PTR(err);
}
...
quoted
quoted
+/* offload the tc command after inserted */ static int
+tcf_action_offload_add(struct tc_action *action,
+ struct netlink_ext_ack *extack) {
+ struct tc_action *actions[TCA_ACT_MAX_PRIO] = {
+ [0] = action,
+ };
+ struct flow_offload_action *fl_action;
+ int err = 0;
+
+ fl_action = flow_action_alloc(tcf_act_num_actions_single(action));
+ if (!fl_action)
+ return -EINVAL;
Failed alloc-like functions usually result -ENOMEM.
Thanks, we will fix this in V4 patch.
quoted
quoted
+
+ err = flow_action_init(fl_action, action, FLOW_ACT_REPLACE, extack);
+ if (err)
+ goto fl_err;
+
+ err = tc_setup_action(&fl_action->action, actions);
+ if (err) {
+ NL_SET_ERR_MSG_MOD(extack,
+ "Failed to setup tc actions for offload\n");
+ goto fl_err;
+ }
+
+ err = tcf_action_offload_cmd(fl_action, extack);
+ tc_cleanup_flow_action(&fl_action->action);
+
+fl_err:
+ kfree(fl_action);
+
+ return err;
+}
+
+int tcf_action_offload_del(struct tc_action *action) {
+ struct flow_offload_action fl_act;
+ int err = 0;
+
+ if (!action)
+ return -EINVAL;
+
+ err = flow_action_init(&fl_act, action, FLOW_ACT_DESTROY, NULL);
+ if (err)
+ return err;
+
+ return tcf_action_offload_cmd(&fl_act, NULL); }
+
/* Returns numbers of initialized actions or negative error. */
int tcf_action_init(struct net *net, struct tcf_proto *tp, struct
nlattr *nla, @@ -1103,6 +1267,8 @@ int tcf_action_init(struct net *net,
struct tcf_proto *tp, struct nlattr *nla,
quoted
sz += tcf_action_fill_size(act);
/* Start from index 0 */
actions[i - 1] = act;
+ if (!(flags & TCA_ACT_FLAGS_BIND))
+ tcf_action_offload_add(act, extack);
}
/* We have to commit them all together, because if any error
happened in diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 2ef8f5a6205a..351d93988b8b 100644
Maybe just move tc_setup_action() to act_api and ifdef its definition in
pkt_cls.h instead of existing tc_setup_flow_action()?
As explanation above, after the change, tc_setup_flow_action will call function of
tc_setup_action and refer to exts->actions, so just move tc_setup_action can not
fix this problem.
From: Dave Taht <hidden> Date: 2021-11-01 14:28:18
On Sun, Oct 31, 2021 at 7:14 AM Jamal Hadi Salim [off-list ref] wrote:
On 2021-10-31 08:03, Dave Taht wrote:
[..]
quoted
Just as an on-going grump: It has been my hope that policing as a
technique would have died a horrible death by now. Seeing it come back
as an "easy to offload" operation here - fresh from the 1990s! does
not mean it's a good idea, and I'd rather like it if we were finding
ways to
offload newer things that work better, such as modern aqm, fair
queuing, and shaping technologies that are in pie, fq_codel, and cake.
policing leads to bursty loss, especially at higher rates, BBR has a
specific mode designed to defeat it, and I ripped it out of
wondershaper
long ago for very good reasons:
https://www.bufferbloat.net/projects/bloat/wiki/Wondershaper_Must_Die/
I did a long time ago start working on a better policing idea based on
some good aqm ideas like AFD, but dropped it figuring that policing
was going to vanish
from the planet. It's baaaaaack.
A lot of enthusiasm for fq_codel in that link ;->
Wrote that in 2013. It's not every day you solve tcp global synchronization,
achieve a queue depth of 5ms no matter the rate, develop something that
has zero latency for sparse packets, only shoots at the fat flows, drops from
head so there's always an immediate signal of congestion from the packet
just behind, makes opus's PLC and simpler forms of FEC "just work", and
requires near zero configuration.
The plots at the end made a very convincing case for abandoning policing.
Root cause for burstiness is typically due to large transient queues
(which are sometimes not under your admin control) and of course if
you use a policer and dont have your double leaky buckets set properly
to compensate for both short and long term rates you will have bursts
of drops with the policer.
I would really like to see a good configuration guide for policing at
multiple real-world bandwidths and at real-world workloads.
It would be the same with shaper as well
if the packet burst shows up when the queue is full.
Queues are shock absorbers as Van always says. We do drop packets
still, on the rx ring. The default queue depth of codel is 32MB. It takes
a really really really large burst to overwhelm that.
I wonder where all the userspace wireguard vpns are dropping packets nowdays.
Intuitively it would feel, for non-work conserving approaches,
delaying a packet (as in shaping) is better than dropping (as in
It's shaping + flow queueing that's the win, if you are going to
queue. It gets all
the flows statistically multiplexed and in flow balance orders of
magnitude faster
than a policer could. (flow queuing is different from classic fair queuing)
The tiny flows pass through untouched at zero delay also.
At the time, I was considering applying a codel-like technique to policing -
I'd called it "bobbie", where once you exceed the rate, a virtual clock moves
forward as to how long you would have delayed packet delivery if you were
queueing and then starts shooting at packets once your burst tolerance is
exceeded until
But inbound fq+shaping did wonders faster, and selfishly I didn't feel
like abandoning
floating point to work with in the kernel.
That said, it's taken breaking the qdisc lock and xpf to make inbound
shaping scale
decently (see: https://github.com/rchac/LibreQoS#how-do-cake-and-fq_codel-work )
policing) - but i have not a study which scientifically proves it.
Any pointers in that regard?
Neither do I. Matt Mathis has ranted about it, and certainly the workarounds
in BBRv1 to defeat others desperate attempts to control their bandwidth with
a policer is obvious from their data.
If there really is a resurgence of interest in policing, a good paper
would compare
a classic 3 color policer to bobbie, and to shaping vs a vs BBR and cubic.
I'm low on students at the moment...
TCP would recover either way (either detecting sequence gaps or RTO).
Yes, it does. But policing is often devastating to voip and videoconferencing
traffic.
In Linux kernel level i am not sure i see much difference in either
since we actually feedback an indicator to TCP to indicate a local
drop (as opposed to guessing when it is dropped in the network)
and the TCP code is smart enough to utilize that knowledge.
For hardware offload there is no such feedback for either of those
two approaches (so no difference with drop in the blackhole).
Yes, now you've built a *router* and lost the local control loop.
TSQ, sch_fq's pacing, and other host optimizations no longer work.
I encourage more folk to regularly take packet
captures of the end results of offloads vs a vs network latency.
Look! MORE BANDWIDTH for a single flow! Wait! There's
600ms of latency and new flows can't even get started!
As to "policer must die" - not possible i am afraid;-> I mean there
has to be strong evidence that it is a bad idea and besides that
_a lot of hardware_ supports it;-> Ergo, we have to support it as well.
I agree that supporting hardware features is good. I merely wish that
certain other software features were making it into modern hardware.
I'm encouraged by this work in p4, at least.
https://arxiv.org/pdf/2010.04528.pdf
Note: RED for example has been proven almost impossible to configure
properly but we still support it and there's a good set of hardware
offload support for it. For RED - and i should say the policer as well -
if you configure properly, _it works_.
I have no idea how often RED is used nowadays. The *only* requests
for offloading it Ive heard is for configuring it as a brick wall ecn marking
tool, which does indeed work for dctcp.
The hope was with pie, being similar in construction, would end up
implemented in hardware, however it's so far turned out that codel
was easier to implement in hw and more effective.
On November 1, 2021 8:06 PM, Vlad Buslov [off-list ref] wrote:
On Mon 01 Nov 2021 at 11:44, Baowen Zheng [off-list ref]
wrote:
quoted
Thanks for your review and sorry for delay in responding.
On October 30, 2021 12:59 AM, Vlad Buslov [off-list ref] wrote:
quoted
On Thu 28 Oct 2021 at 14:06, Simon Horman [off-list ref]
wrote:
quoted
From: Baowen Zheng <redacted>
Use flow_indr_dev_register/flow_indr_dev_setup_offload to offload tc
action.
We need to call tc_cleanup_flow_action to clean up tc action entry
since in tc_setup_action, some actions may hold dev refcnt,
especially the mirror action.
Signed-off-by: Baowen Zheng <redacted>
Signed-off-by: Louis Peens <redacted>
Signed-off-by: Simon Horman <redacted>
---
include/linux/netdevice.h | 1 +
include/net/act_api.h | 2 +-
include/net/flow_offload.h | 17 ++++
include/net/pkt_cls.h | 15 ++++
net/core/flow_offload.c | 43 ++++++++--
net/sched/act_api.c | 166
@@ -916,6 +916,7 @@ enum tc_setup_type {TC_SETUP_QDISC_TBF,TC_SETUP_QDISC_FIFO,TC_SETUP_QDISC_HTB,+TC_SETUP_ACT,};/* These structures hold the attributes of bpf state that are being
passed diff --git a/include/net/act_api.h b/include/net/act_api.h
index b5b624c7e488..9eb19188603c 100644
for (; 0; (void)(i), (void)(a), (void)(exts)) #endif
+#define tcf_act_for_each_action(i, a, actions) \
+ for (i = 0; i < TCA_ACT_MAX_PRIO && ((a) = actions[i]); i++)
+
static inline void
tcf_exts_stats_update(const struct tcf_exts *exts,
u64 bytes, u64 packets, u64 drops, u64 lastuse, @@ -532,8
+535,19 @@ tcf_match_indev(struct sk_buff *skb, int ifindex)
return ifindex == skb->skb_iif;
}
+#ifdef CONFIG_NET_CLS_ACT
int tc_setup_flow_action(struct flow_action *flow_action,
const struct tcf_exts *exts);
Why does existing cls_api function tc_setup_flow_action() now depend
on CONFIG_NET_CLS_ACT?
Originally the function tc_setup_flow_action deal with the dependence
of CONFIG_NET_CLS_ACT By calling the macro tcf_exts_for_each_action,
now we change to call the function tc_setup_action Then
tc_setup_flow_action will refer to exts->actions, so it will depend on
CONFIG_NET_CLS_ACT explicitly.
quoted
To fix this, we have to have the ifdef in tc_setup_flow_action declaration or
in the implement in cls_api.c.
quoted
Do you think if it makes sense?
Since we already have multiple of such ifdefs in cls_api I don't think having
more is an issue, but I also don't think we need to ifdef this function in both
pkt_cls.h and cls_api.c. Unless I'm missing something you can either:
- Make tc_setup_flow_action() inline in pkt_cls.h and remove its definition
from cls_api.c since tc_setup_action() is also exported.
- Move ifdef check inside function definition in cls_api.c (return 0, if config is
not defined), which will allows you to remove ifdef from pkt_cls.h.
WDYT?
Thanks, I think it makes sense to us. We will make the change according to the second option.
tcf_action_offload_del() and tcf_action_cleanup() seem to be always
called together. Consider moving the call to tcf_action_offload_del()
into tcf_action_cleanup().
Thanks, we will consider to move tcf_action_offload_del() inside of
+/* offload the tc command after inserted */ static int
+tcf_action_offload_add(struct tc_action *action,
+ struct netlink_ext_ack *extack) {
+ struct tc_action *actions[TCA_ACT_MAX_PRIO] = {
+ [0] = action,
+ };
+ struct flow_offload_action *fl_action;
+ int err = 0;
+
+ fl_action = flow_action_alloc(tcf_act_num_actions_single(action));
+ if (!fl_action)
+ return -EINVAL;
Failed alloc-like functions usually result -ENOMEM.
Thanks, we will fix this in V4 patch.
quoted
quoted
+
+ err = flow_action_init(fl_action, action, FLOW_ACT_REPLACE, extack);
+ if (err)
+ goto fl_err;
+
+ err = tc_setup_action(&fl_action->action, actions);
+ if (err) {
+ NL_SET_ERR_MSG_MOD(extack,
+ "Failed to setup tc actions for offload\n");
+ goto fl_err;
+ }
+
+ err = tcf_action_offload_cmd(fl_action, extack);
+ tc_cleanup_flow_action(&fl_action->action);
+
+fl_err:
+ kfree(fl_action);
+
+ return err;
+}
+
+int tcf_action_offload_del(struct tc_action *action) {
+ struct flow_offload_action fl_act;
+ int err = 0;
+
+ if (!action)
+ return -EINVAL;
+
+ err = flow_action_init(&fl_act, action, FLOW_ACT_DESTROY, NULL);
+ if (err)
+ return err;
+
+ return tcf_action_offload_cmd(&fl_act, NULL); }
+
/* Returns numbers of initialized actions or negative error. */
int tcf_action_init(struct net *net, struct tcf_proto *tp, struct
nlattr *nla, @@ -1103,6 +1267,8 @@ int tcf_action_init(struct net
*net,
struct tcf_proto *tp, struct nlattr *nla,
quoted
sz += tcf_action_fill_size(act);
/* Start from index 0 */
actions[i - 1] = act;
+ if (!(flags & TCA_ACT_FLAGS_BIND))
+ tcf_action_offload_add(act, extack);
}
/* We have to commit them all together, because if any error
happened in diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 2ef8f5a6205a..351d93988b8b 100644
Maybe just move tc_setup_action() to act_api and ifdef its definition
in pkt_cls.h instead of existing tc_setup_flow_action()?
As explanation above, after the change, tc_setup_flow_action will call
function of tc_setup_action and refer to exts->actions, so just move
tc_setup_action can not fix this problem.
On October 30, 2021 1:31 AM, Vlad Buslov [off-list ref] wrote:
On Thu 28 Oct 2021 at 14:06, Simon Horman [off-list ref]
wrote:
quoted
From: Baowen Zheng <redacted>
Add reoffload process to update hw_count when driver is inserted or
removed.
When reoffloading actions, we still offload the actions that are added
independent of filters.
Signed-off-by: Baowen Zheng <redacted>
Signed-off-by: Louis Peens <redacted>
Signed-off-by: Simon Horman <redacted>
---
include/net/act_api.h | 24 +++++
include/net/pkt_cls.h | 5 +
net/core/flow_offload.c | 5 +
net/sched/act_api.c | 213
Don't know if it is a problem, but shouldn't tcf_action_reoffload_cb() be called
before flow_block_indr_notify(), which calls
flow_block_indr->cleanup() callbacks?
Thanks for bring this issue to us. I think it totally make sense to us.
Although we did not find problem as current tests.
We will make the change according to our review.
quoted
}
EXPORT_SYMBOL(flow_indr_dev_unregister);
diff --git a/net/sched/act_api.c b/net/sched/act_api.c index
@@ -638,6 +638,59 @@ EXPORT_SYMBOL(tcf_idrinfo_destroy);staticLIST_HEAD(act_base);staticDEFINE_RWLOCK(act_mod_lock);+/* since act ops id is stored in pernet subsystem list,+*thenthereisnowaytowalkthroughonlyalltheaction+*subsystem,sowekeeptcactionpernetopsidfor+*reoffloadtowalkthrough.+*/+staticLIST_HEAD(act_pernet_id_list);static+DEFINE_MUTEX(act_id_mutex);structtc_act_pernet_id{+structlist_headlist;+unsignedintid;+};++staticinttcf_pernet_add_id_list(unsignedintid){+structtc_act_pernet_id*id_ptr;+intret=0;++mutex_lock(&act_id_mutex);+list_for_each_entry(id_ptr,&act_pernet_id_list,list){+if(id_ptr->id==id){+ret=-EEXIST;+gotoerr_out;+}+}++id_ptr=kzalloc(sizeof(*id_ptr),GFP_KERNEL);+if(!id_ptr){+ret=-ENOMEM;+gotoerr_out;+}+id_ptr->id=id;++list_add_tail(&id_ptr->list,&act_pernet_id_list);++err_out:+mutex_unlock(&act_id_mutex);+returnret;+}++staticvoidtcf_pernet_del_id_list(unsignedintid){+structtc_act_pernet_id*id_ptr;++mutex_lock(&act_id_mutex);+list_for_each_entry(id_ptr,&act_pernet_id_list,list){+if(id_ptr->id==id){+list_del(&id_ptr->list);+kfree(id_ptr);+break;+}+}+mutex_unlock(&act_id_mutex);+}inttcf_register_action(structtc_action_ops*act,structpernet_operations*ops)
@@ -656,18 +709,30 @@ int tcf_register_action(struct tc_action_ops *act,if(ret)returnret;+if(ops->id){+ret=tcf_pernet_add_id_list(*ops->id);+if(ret)+gotoid_err;+}+write_lock(&act_mod_lock);list_for_each_entry(a,&act_base,head){if(act->id==a->id||(strcmp(act->kind,a->kind)==0)){-write_unlock(&act_mod_lock);-unregister_pernet_subsys(ops);-return-EEXIST;+ret=-EEXIST;+gotoerr_out;}}list_add_tail(&act->head,&act_base);write_unlock(&act_mod_lock);return0;++err_out:+write_unlock(&act_mod_lock);+tcf_pernet_del_id_list(*ops->id);+id_err:+unregister_pernet_subsys(ops);+returnret;}EXPORT_SYMBOL(tcf_register_action);
@@ -686,8 +751,11 @@ int tcf_unregister_action(struct tc_action_ops *act,}}write_unlock(&act_mod_lock);-if(!err)+if(!err){unregister_pernet_subsys(ops);+if(ops->id)+tcf_pernet_del_id_list(*ops->id);+}returnerr;}EXPORT_SYMBOL(tcf_unregister_action);
@@ -1175,15 +1243,11 @@ static int flow_action_init(struct
flow_offload_action *fl_action,
quoted
return 0;
}
-static int tcf_action_offload_cmd(struct flow_offload_action *fl_act,
- u32 *hw_count,
- struct netlink_ext_ack *extack)
+static int tcf_action_offload_cmd_ex(struct flow_offload_action *fl_act,
+ u32 *hw_count)
{
int err;
- if (IS_ERR(fl_act))
- return PTR_ERR(fl_act);
-
err = flow_indr_dev_setup_offload(NULL, NULL, TC_SETUP_ACT,
fl_act, NULL, NULL);
if (err < 0)
@@ -1195,9 +1259,41 @@ static int tcf_action_offload_cmd(struct
flow_offload_action *fl_act,
quoted
return 0;
}
+static int tcf_action_offload_cmd_cb_ex(struct flow_offload_action
From: Simon Horman <hidden> Date: 2021-11-02 12:51:29
On Mon, Nov 01, 2021 at 10:01:28AM +0200, Vlad Buslov wrote:
On Sun 31 Oct 2021 at 15:40, Jamal Hadi Salim [off-list ref] wrote:
quoted
On 2021-10-31 05:50, Oz Shlomo wrote:
quoted
On 10/28/2021 2:06 PM, Simon Horman wrote:
...
quoted
quoted
Actions are also (implicitly) instantiated when filters are created.
In the following example the mirred action instance (created by the first
filter) is shared by the second filter:
tc filter add dev $DEV1 proto ip parent ffff: flower \
ip_proto tcp action mirred egress redirect dev $DEV3
tc filter add dev $DEV2 proto ip parent ffff: flower \
ip_proto tcp action mirred index 1
I sure hope this is supported. At least the discussions so far
are a nod in that direction...
I know there is hardware that is not capable of achieving this
(little CPE type devices) but lets not make that the common case.
Looks like it isn't supported in this change since
tcf_action_offload_add() is only called by tcf_action_init() when BIND
flag is not set (the flag is always set when called from cls code).
Moreover, I don't think it is good idea to support such use-case because
that would require to increase number of calls to driver offload
infrastructure from 1 per filter to 1+number_of_actions, which would
significantly impact insertion rate.
Hi,
I feel that I am missing some very obvious point here.
But from my perspective the use case described by Oz is supported
by existing offload of the flower classifier (since ~4.13 IIRC).
On Tue 02 Nov 2021 at 14:51, Simon Horman [off-list ref] wrote:
On Mon, Nov 01, 2021 at 10:01:28AM +0200, Vlad Buslov wrote:
quoted
On Sun 31 Oct 2021 at 15:40, Jamal Hadi Salim [off-list ref] wrote:
quoted
On 2021-10-31 05:50, Oz Shlomo wrote:
quoted
On 10/28/2021 2:06 PM, Simon Horman wrote:
...
quoted
quoted
quoted
Actions are also (implicitly) instantiated when filters are created.
In the following example the mirred action instance (created by the first
filter) is shared by the second filter:
tc filter add dev $DEV1 proto ip parent ffff: flower \
ip_proto tcp action mirred egress redirect dev $DEV3
tc filter add dev $DEV2 proto ip parent ffff: flower \
ip_proto tcp action mirred index 1
I sure hope this is supported. At least the discussions so far
are a nod in that direction...
I know there is hardware that is not capable of achieving this
(little CPE type devices) but lets not make that the common case.
Looks like it isn't supported in this change since
tcf_action_offload_add() is only called by tcf_action_init() when BIND
flag is not set (the flag is always set when called from cls code).
Moreover, I don't think it is good idea to support such use-case because
that would require to increase number of calls to driver offload
infrastructure from 1 per filter to 1+number_of_actions, which would
significantly impact insertion rate.
Hi,
I feel that I am missing some very obvious point here.
But from my perspective the use case described by Oz is supported
by existing offload of the flower classifier (since ~4.13 IIRC).
Mlx5 driver can't support such case without infrastructure change in
kernel for following reasons:
- Action index is not provided by flow_action offload infrastructure for
most of the actions, so there is no way for driver to determine
whether the action is shared.
- If we extend the infrastructure to always provide tcfa_index (a
trivial change), there would be not much use for it because there is
no way to properly update shared action counters without
infrastructure code similar to what you implemented as part of this
series.
How do you support shared actions created through cls_api in your
driver, considering described limitations?
From: Simon Horman <hidden> Date: 2021-11-02 16:51:53
On Tue, Nov 02, 2021 at 05:33:14PM +0200, Vlad Buslov wrote:
On Tue 02 Nov 2021 at 14:51, Simon Horman [off-list ref] wrote:
quoted
On Mon, Nov 01, 2021 at 10:01:28AM +0200, Vlad Buslov wrote:
quoted
On Sun 31 Oct 2021 at 15:40, Jamal Hadi Salim [off-list ref] wrote:
quoted
On 2021-10-31 05:50, Oz Shlomo wrote:
quoted
On 10/28/2021 2:06 PM, Simon Horman wrote:
...
quoted
quoted
quoted
Actions are also (implicitly) instantiated when filters are created.
In the following example the mirred action instance (created by the first
filter) is shared by the second filter:
tc filter add dev $DEV1 proto ip parent ffff: flower \
ip_proto tcp action mirred egress redirect dev $DEV3
tc filter add dev $DEV2 proto ip parent ffff: flower \
ip_proto tcp action mirred index 1
I sure hope this is supported. At least the discussions so far
are a nod in that direction...
I know there is hardware that is not capable of achieving this
(little CPE type devices) but lets not make that the common case.
Looks like it isn't supported in this change since
tcf_action_offload_add() is only called by tcf_action_init() when BIND
flag is not set (the flag is always set when called from cls code).
Moreover, I don't think it is good idea to support such use-case because
that would require to increase number of calls to driver offload
infrastructure from 1 per filter to 1+number_of_actions, which would
significantly impact insertion rate.
Hi,
I feel that I am missing some very obvious point here.
But from my perspective the use case described by Oz is supported
by existing offload of the flower classifier (since ~4.13 IIRC).
Mlx5 driver can't support such case without infrastructure change in
kernel for following reasons:
- Action index is not provided by flow_action offload infrastructure for
most of the actions, so there is no way for driver to determine
whether the action is shared.
- If we extend the infrastructure to always provide tcfa_index (a
trivial change), there would be not much use for it because there is
no way to properly update shared action counters without
infrastructure code similar to what you implemented as part of this
series.
How do you support shared actions created through cls_api in your
driver, considering described limitations?
Thanks,
I misread the use case described by Oz, but I believe I understand it now.
I agree that the case described is neither currently supported, nor
supported by this patchset (to be honest I for one had not considered it).
So, I think the question is: does upporting this use-case make sense - from
implementation, use-case, and consistency perspectives - in the context of
this patchset?
Am I on the right track?
From: Oz Shlomo <hidden> Date: 2021-11-03 10:56:19
On 11/2/2021 6:15 PM, Simon Horman wrote:
On Tue, Nov 02, 2021 at 05:33:14PM +0200, Vlad Buslov wrote:
quoted
On Tue 02 Nov 2021 at 14:51, Simon Horman [off-list ref] wrote:
quoted
On Mon, Nov 01, 2021 at 10:01:28AM +0200, Vlad Buslov wrote:
quoted
On Sun 31 Oct 2021 at 15:40, Jamal Hadi Salim [off-list ref] wrote:
quoted
On 2021-10-31 05:50, Oz Shlomo wrote:
quoted
On 10/28/2021 2:06 PM, Simon Horman wrote:
...
quoted
quoted
quoted
Actions are also (implicitly) instantiated when filters are created.
In the following example the mirred action instance (created by the first
filter) is shared by the second filter:
tc filter add dev $DEV1 proto ip parent ffff: flower \
ip_proto tcp action mirred egress redirect dev $DEV3
tc filter add dev $DEV2 proto ip parent ffff: flower \
ip_proto tcp action mirred index 1
I sure hope this is supported. At least the discussions so far
are a nod in that direction...
I know there is hardware that is not capable of achieving this
(little CPE type devices) but lets not make that the common case.
Looks like it isn't supported in this change since
tcf_action_offload_add() is only called by tcf_action_init() when BIND
flag is not set (the flag is always set when called from cls code).
Moreover, I don't think it is good idea to support such use-case because
that would require to increase number of calls to driver offload
infrastructure from 1 per filter to 1+number_of_actions, which would
significantly impact insertion rate.
Hi,
I feel that I am missing some very obvious point here.
But from my perspective the use case described by Oz is supported
by existing offload of the flower classifier (since ~4.13 IIRC).
Mlx5 driver can't support such case without infrastructure change in
kernel for following reasons:
- Action index is not provided by flow_action offload infrastructure for
most of the actions, so there is no way for driver to determine
whether the action is shared.
- If we extend the infrastructure to always provide tcfa_index (a
trivial change), there would be not much use for it because there is
no way to properly update shared action counters without
infrastructure code similar to what you implemented as part of this
series.
How do you support shared actions created through cls_api in your
driver, considering described limitations?
Thanks,
I misread the use case described by Oz, but I believe I understand it now.
I agree that the case described is neither currently supported, nor
supported by this patchset (to be honest I for one had not considered it).
So, I think the question is: does upporting this use-case make sense - from
implementation, use-case, and consistency perspectives - in the context of
this patchset?
Am I on the right track?
Currently we don't have a specific application use case for sharing actions that were created by tc
filters. However, we do have future use case in mind.
We could add such functionality on top of this series when a use case will materialize.
Perhaps, at that point, we can also introduce a control flag in order to avoid unnecessary insertion
rate performance degradation.
Thanks for your review and sorry for delay in responding.
On October 30, 2021 2:01 AM, Vlad Buslov wrote:
On Thu 28 Oct 2021 at 14:06, Simon Horman [off-list ref]
wrote:
quoted
From: Baowen Zheng <redacted>
Add process to validate flags of filter and actions when adding a tc
filter.
We need to prevent adding filter with flags conflicts with its actions.
Signed-off-by: Baowen Zheng <redacted>
Signed-off-by: Louis Peens <redacted>
Signed-off-by: Simon Horman <redacted>
---
net/sched/cls_api.c | 26 ++++++++++++++++++++++++++
net/sched/cls_flower.c | 3 ++-
net/sched/cls_matchall.c | 4 ++--
net/sched/cls_u32.c | 7 ++++---
4 files changed, 34 insertions(+), 6 deletions(-)
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c index
I know Jamal suggested to have skip_sw for actions, but it complicates the
code and I'm still not entirely understand why it is necessary.
After all, action can only get applied to a packet if the packet has been
matched by some filter and filters already have skip sw/hw controls. Forgoing
action skip_sw flag would:
- Alleviate the need to validate that filter and action flags are compatible.
(trying to offload filter that points to existing skip_hw action would just fail
because the driver wouldn't find the action with provided id in its tables)
- Remove the need to add more conditionals into TC software data path in
patch 4.
WDYT?
As we discussed with Jamal, we will keep the flag of skip_sw and we need to make
exactly match for the actions with flags and the filter specific action with index.
Aren't you or-ing flags from two different ranges (TCA_CLS_FLAGS_* and
TCA_ACT_FLAGS_*) that map to same bits, or am I missing something? This
isn't explained in commit message so it is hard for me to understand the idea
here.
Yes, as you said we use TCA_CLS_FLAGS_* or TCA_ACT_FLAGS_* flags to validate the action flags.
As you know, the TCA_ACT_FLAGS_* in flags are system flags(in high 16 bits) and the TCA_CLS_FLAGS_*
are user flags(in low 16 bits), so they will not be conflict.
But I think you suggestion also makes sense to us, do you think we need to pass a single filter flag
to make the process more clear?
quoted
if (err)
goto errout;
diff --git a/net/sched/cls_matchall.c b/net/sched/cls_matchall.c index
Sorry for reply this message again.
On November 4, 2021 10:31 AM, Baowen Zheng wrote:
Thanks for your review and sorry for delay in responding.
On October 30, 2021 2:01 AM, Vlad Buslov wrote:
quoted
On Thu 28 Oct 2021 at 14:06, Simon Horman [off-list ref]
wrote:
quoted
From: Baowen Zheng <redacted>
Add process to validate flags of filter and actions when adding a tc
filter.
We need to prevent adding filter with flags conflicts with its actions.
Signed-off-by: Baowen Zheng <redacted>
Signed-off-by: Louis Peens <redacted>
Signed-off-by: Simon Horman <redacted>
---
net/sched/cls_api.c | 26 ++++++++++++++++++++++++++
net/sched/cls_flower.c | 3 ++-
net/sched/cls_matchall.c | 4 ++--
net/sched/cls_u32.c | 7 ++++---
4 files changed, 34 insertions(+), 6 deletions(-)
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c index
I know Jamal suggested to have skip_sw for actions, but it complicates
the code and I'm still not entirely understand why it is necessary.
After all, action can only get applied to a packet if the packet has
been matched by some filter and filters already have skip sw/hw
controls. Forgoing action skip_sw flag would:
- Alleviate the need to validate that filter and action flags are compatible.
(trying to offload filter that points to existing skip_hw action would
just fail because the driver wouldn't find the action with provided id
in its tables)
- Remove the need to add more conditionals into TC software data path
in patch 4.
WDYT?
As we discussed with Jamal, we will keep the flag of skip_sw and we need to
make exactly match for the actions with flags and the filter specific action with
index.
quoted
quoted
int tcf_exts_validate(struct net *net, struct tcf_proto *tp, struct nlattr
Aren't you or-ing flags from two different ranges (TCA_CLS_FLAGS_* and
TCA_ACT_FLAGS_*) that map to same bits, or am I missing something? This
isn't explained in commit message so it is hard for me to understand
the idea here.
Yes, as you said we use TCA_CLS_FLAGS_* or TCA_ACT_FLAGS_* flags to
validate the action flags.
As you know, the TCA_ACT_FLAGS_* in flags are system flags(in high 16 bits)
and the TCA_CLS_FLAGS_* are user flags(in low 16 bits), so they will not be
conflict.
But I think you suggestion also makes sense to us, do you think we need to
pass a single filter flag to make the process more clear?
After consideration, I think it is better to separate CLS flags and ACT flags.
So we will pass CLS flags as a separate flags, thanks.
On Thu 04 Nov 2021 at 07:51, Baowen Zheng [off-list ref] wrote:
Sorry for reply this message again.
On November 4, 2021 10:31 AM, Baowen Zheng wrote:
quoted
Thanks for your review and sorry for delay in responding.
On October 30, 2021 2:01 AM, Vlad Buslov wrote:
quoted
On Thu 28 Oct 2021 at 14:06, Simon Horman [off-list ref]
wrote:
quoted
From: Baowen Zheng <redacted>
Add process to validate flags of filter and actions when adding a tc
filter.
We need to prevent adding filter with flags conflicts with its actions.
Signed-off-by: Baowen Zheng <redacted>
Signed-off-by: Louis Peens <redacted>
Signed-off-by: Simon Horman <redacted>
---
net/sched/cls_api.c | 26 ++++++++++++++++++++++++++
net/sched/cls_flower.c | 3 ++-
net/sched/cls_matchall.c | 4 ++--
net/sched/cls_u32.c | 7 ++++---
4 files changed, 34 insertions(+), 6 deletions(-)
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c index
I know Jamal suggested to have skip_sw for actions, but it complicates
the code and I'm still not entirely understand why it is necessary.
After all, action can only get applied to a packet if the packet has
been matched by some filter and filters already have skip sw/hw
controls. Forgoing action skip_sw flag would:
- Alleviate the need to validate that filter and action flags are compatible.
(trying to offload filter that points to existing skip_hw action would
just fail because the driver wouldn't find the action with provided id
in its tables)
- Remove the need to add more conditionals into TC software data path
in patch 4.
WDYT?
As we discussed with Jamal, we will keep the flag of skip_sw and we need to
make exactly match for the actions with flags and the filter specific action with
index.
quoted
quoted
int tcf_exts_validate(struct net *net, struct tcf_proto *tp, struct nlattr
Aren't you or-ing flags from two different ranges (TCA_CLS_FLAGS_* and
TCA_ACT_FLAGS_*) that map to same bits, or am I missing something? This
isn't explained in commit message so it is hard for me to understand
the idea here.
Yes, as you said we use TCA_CLS_FLAGS_* or TCA_ACT_FLAGS_* flags to
validate the action flags.
As you know, the TCA_ACT_FLAGS_* in flags are system flags(in high 16 bits)
and the TCA_CLS_FLAGS_* are user flags(in low 16 bits), so they will not be
conflict.
Indeed, currently available TCA_CLS_FLAGS_* fit into first 16 bits, but
the field itself is 32 bits and with addition of more flags in the
future higher bits may start to be used since TCA_CLS_FLAGS_* and
TCA_ACT_FLAGS_* are independent sets.
quoted
But I think you suggestion also makes sense to us, do you think we need to
pass a single filter flag to make the process more clear?
After consideration, I think it is better to separate CLS flags and ACT flags.
So we will pass CLS flags as a separate flags, thanks.
Please also validate inside tcf_action_init() instead of creating new
tcf_exts_validate_actions() function, if possible. I think this will
lead to cleaner and more simple code.
On Thu 04 Nov 2021 at 07:51, Baowen Zheng [off-list ref]
wrote:
quoted
Sorry for reply this message again.
On November 4, 2021 10:31 AM, Baowen Zheng wrote:
quoted
Thanks for your review and sorry for delay in responding.
On October 30, 2021 2:01 AM, Vlad Buslov wrote:
quoted
On Thu 28 Oct 2021 at 14:06, Simon Horman
[off-list ref]
quoted
quoted
quoted
wrote:
quoted
From: Baowen Zheng <redacted>
Add process to validate flags of filter and actions when adding a
tc filter.
We need to prevent adding filter with flags conflicts with its actions.
Signed-off-by: Baowen Zheng <redacted>
Signed-off-by: Louis Peens <redacted>
Signed-off-by: Simon Horman <redacted>
---
net/sched/cls_api.c | 26 ++++++++++++++++++++++++++
net/sched/cls_flower.c | 3 ++-
net/sched/cls_matchall.c | 4 ++--
net/sched/cls_u32.c | 7 ++++---
4 files changed, 34 insertions(+), 6 deletions(-)
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c index
I know Jamal suggested to have skip_sw for actions, but it
complicates the code and I'm still not entirely understand why it is
necessary.
quoted
quoted
quoted
After all, action can only get applied to a packet if the packet has
been matched by some filter and filters already have skip sw/hw
controls. Forgoing action skip_sw flag would:
- Alleviate the need to validate that filter and action flags are compatible.
(trying to offload filter that points to existing skip_hw action
would just fail because the driver wouldn't find the action with
provided id in its tables)
- Remove the need to add more conditionals into TC software data path
in patch 4.
WDYT?
As we discussed with Jamal, we will keep the flag of skip_sw and we
need to make exactly match for the actions with flags and the filter
specific action with index.
quoted
quoted
int tcf_exts_validate(struct net *net, struct tcf_proto *tp,
struct nlattr
Aren't you or-ing flags from two different ranges (TCA_CLS_FLAGS_*
and
TCA_ACT_FLAGS_*) that map to same bits, or am I missing something?
This isn't explained in commit message so it is hard for me to
understand the idea here.
Yes, as you said we use TCA_CLS_FLAGS_* or TCA_ACT_FLAGS_* flags to
validate the action flags.
As you know, the TCA_ACT_FLAGS_* in flags are system flags(in high 16
bits) and the TCA_CLS_FLAGS_* are user flags(in low 16 bits), so they
will not be conflict.
Indeed, currently available TCA_CLS_FLAGS_* fit into first 16 bits, but the field
itself is 32 bits and with addition of more flags in the future higher bits may
start to be used since TCA_CLS_FLAGS_* and
TCA_ACT_FLAGS_* are independent sets.
Thanks, we will use a single parameter as the filter flag.
quoted
quoted
But I think you suggestion also makes sense to us, do you think we
need to pass a single filter flag to make the process more clear?
After consideration, I think it is better to separate CLS flags and ACT flags.
So we will pass CLS flags as a separate flags, thanks.
Please also validate inside tcf_action_init() instead of creating new
tcf_exts_validate_actions() function, if possible. I think this will lead to cleaner
and more simple code.
Thanks, we will consider to implement the validation inside tcf_action_init().