From: Simon Horman <hidden> Date: 2021-07-22 09:19:57
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.
Changes since RFC:
- Fix robot test failure.
- Change actions offload process in action add function rather than action
init.
- Change actions offload delete process after tcf_del_notify to keep
undeleted actions.
- Add process to update actions stats from hardware.
Baowen Zheng (3):
flow_offload: allow user to offload tc action to net device
flow_offload: add process to delete offloaded actions from net device
flow_offload: add process to update action stats from hardware
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 | 1 +
include/net/flow_offload.h | 15 ++
include/net/pkt_cls.h | 20 +++
net/core/flow_offload.c | 26 ++-
net/sched/act_api.c | 162 +++++++++++++++++-
net/sched/cls_api.c | 42 ++++-
10 files changed, 264 insertions(+), 11 deletions(-)
--
2.20.1
From: Simon Horman <hidden> Date: 2021-07-22 09:19:59
From: Baowen Zheng <redacted>
Use flow_indr_dev_register/flow_indr_dev_setup_offload to
offload tc action.
We offload the tc action mainly for ovs meter configuration.
Make some basic changes for different vendors to return EOPNOTSUPP.
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.
As per review from the RFC, the kernel test robot will fail to run, so
we add CONFIG_NET_CLS_ACT control for the action offload.
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 +-
.../ethernet/mellanox/mlx5/core/en/rep/tc.c | 3 ++
.../ethernet/netronome/nfp/flower/offload.c | 3 ++
include/linux/netdevice.h | 1 +
include/net/flow_offload.h | 15 ++++++++
include/net/pkt_cls.h | 15 ++++++++
net/core/flow_offload.c | 26 +++++++++++++-
net/sched/act_api.c | 33 +++++++++++++++++
net/sched/cls_api.c | 36 ++++++++++++++++---
9 files changed, 128 insertions(+), 6 deletions(-)
@@ -923,6 +923,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
@@ -1060,6 +1060,36 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,returnERR_PTR(err);}+/* offload the tc command after inserted */+inttcf_action_offload_cmd(structtc_action*actions[],+structnetlink_ext_ack*extack)+{+structflow_offload_action*fl_act;+interr=0;++fl_act=flow_action_alloc(tcf_act_num_actions(actions));+if(!fl_act)+return-ENOMEM;++fl_act->extack=extack;+err=tc_setup_action(&fl_act->action,actions);+if(err){+NL_SET_ERR_MSG_MOD(extack,+"Failed to setup tc actions for offload\n");+gotoerr_out;+}+fl_act->command=FLOW_ACT_REPLACE;++flow_indr_dev_setup_offload(NULL,NULL,TC_SETUP_ACT,fl_act,NULL,NULL);++tc_cleanup_flow_action(&fl_act->action);++err_out:+kfree(fl_act);+returnerr;+}+EXPORT_SYMBOL(tcf_action_offload_cmd);+/* Returns numbers of initialized actions or negative error. */inttcf_action_init(structnet*net,structtcf_proto*tp,structnlattr*nla,
@@ -1514,6 +1544,9 @@ static int tcf_action_add(struct net *net, struct nlattr *nla,returnret;ret=tcf_add_notify(net,n,actions,portid,attr_size,extack);+/* offload actions to hardware if possible */+tcf_action_offload_cmd(actions,extack);+/* only put existing actions */for(i=0;i<TCA_ACT_MAX_PRIO;i++)if(init_res[i]==ACT_P_CREATED)
From: Simon Horman <hidden> Date: 2021-07-22 09:20:01
From: Baowen Zheng <redacted>
Add a basic process to delete offloaded actions from net device.
Should not remove the offloaded action entries if the action
fails to delete in tcf_del_notify.
Signed-off-by: Baowen Zheng <redacted>
Signed-off-by: Louis Peens <redacted>
Signed-off-by: Simon Horman <redacted>
---
include/net/pkt_cls.h | 1 +
net/sched/act_api.c | 112 +++++++++++++++++++++++++++++++++++-------
net/sched/cls_api.c | 14 ++++--
3 files changed, 106 insertions(+), 21 deletions(-)
@@ -1060,36 +1060,109 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,returnERR_PTR(err);}-/* offload the tc command after inserted */-inttcf_action_offload_cmd(structtc_action*actions[],-structnetlink_ext_ack*extack)+inttcf_action_offload_cmd_pre(structtc_action*actions[],+enumflow_act_commandcmd,+structnetlink_ext_ack*extack,+structflow_offload_action**fl_act){-structflow_offload_action*fl_act;+structflow_offload_action*fl_act_p;interr=0;-fl_act=flow_action_alloc(tcf_act_num_actions(actions));-if(!fl_act)+fl_act_p=flow_action_alloc(tcf_act_num_actions(actions));+if(!fl_act_p)return-ENOMEM;-fl_act->extack=extack;-err=tc_setup_action(&fl_act->action,actions);+fl_act_p->extack=extack;+fl_act_p->command=cmd;+err=tc_setup_action(&fl_act_p->action,actions);if(err){NL_SET_ERR_MSG_MOD(extack,"Failed to setup tc actions for offload\n");gotoerr_out;}-fl_act->command=FLOW_ACT_REPLACE;+*fl_act=fl_act_p;+return0;+err_out:+kfree(fl_act_p);+returnerr;+}+EXPORT_SYMBOL(tcf_action_offload_cmd_pre);++inttcf_action_offload_cmd_post(structflow_offload_action*fl_act,+structnetlink_ext_ack*extack)+{+if(IS_ERR(fl_act))+returnPTR_ERR(fl_act);flow_indr_dev_setup_offload(NULL,NULL,TC_SETUP_ACT,fl_act,NULL,NULL);tc_cleanup_flow_action(&fl_act->action);--err_out:kfree(fl_act);-returnerr;+return0;+}++/* offload the tc command after inserted */+inttcf_action_offload_cmd(structtc_action*actions[],+structnetlink_ext_ack*extack)+{+structflow_offload_action*fl_act;+interr=0;++err=tcf_action_offload_cmd_pre(actions,+FLOW_ACT_REPLACE,+extack,+&fl_act);+if(err)+returnerr;++returntcf_action_offload_cmd_post(fl_act,extack);}EXPORT_SYMBOL(tcf_action_offload_cmd);+/* offload the tc command after deleted */+inttcf_action_offload_del_post(structflow_offload_action*fl_act,+structtc_action*actions[],+structnetlink_ext_ack*extack,+intfallback_num)+{+intfallback_entries=0;+structtc_action*act;+inttotal_entries=0;+inti;++if(!fl_act)+return-EINVAL;++if(fallback_num){+/* for each the actions to fallback the action entries remain in the actions */+for(i=0;i<TCA_ACT_MAX_PRIO;i++){+act=actions[i];+if(!act)+continue;++fallback_entries+=tcf_act_num_actions_single(act);+}+fallback_entries+=fallback_num;+}+total_entries=fl_act->action.num_entries;+if(total_entries>fallback_entries){+/* just offload the actions that is not fallback and start with the actions */+fl_act->action.num_entries-=fallback_entries;+flow_indr_dev_setup_offload(NULL,NULL,TC_SETUP_ACT,fl_act,NULL,NULL);++/* recovery num_entries for cleanup */+fl_act->action.num_entries=total_entries;+}else{+NL_SET_ERR_MSG(extack,"no entries to offload when deleting the tc actions");+}++tc_cleanup_flow_action(&fl_act->action);++kfree(fl_act);+return0;+}+EXPORT_SYMBOL(tcf_action_offload_del_post);+/* Returns numbers of initialized actions or negative error. */inttcf_action_init(structnet*net,structtcf_proto*tp,structnlattr*nla,
@@ -1393,7 +1466,7 @@ static int tca_action_flush(struct net *net, struct nlattr *nla,returnerr;}-staticinttcf_action_delete(structnet*net,structtc_action*actions[])+staticinttcf_action_delete(structnet*net,structtc_action*actions[],int*fallbacknum){inti;
@@ -1407,6 +1480,7 @@ static int tcf_action_delete(struct net *net, struct tc_action *actions[])u32act_index=a->tcfa_index;actions[i]=NULL;+*fallbacknum=tcf_act_num_actions_single(a);if(tcf_action_put(a)){/* last reference, action was deleted concurrently */module_put(ops->owner);
@@ -1419,12 +1493,13 @@ static int tcf_action_delete(struct net *net, struct tc_action *actions[])returnret;}}+*fallbacknum=0;return0;}staticinttcf_del_notify(structnet*net,structnlmsghdr*n,structtc_action*actions[],-u32portid,size_tattr_size,structnetlink_ext_ack*extack)+u32portid,size_tattr_size,structnetlink_ext_ack*extack,int*fallbacknum){intret;structsk_buff*skb;
@@ -1442,7 +1517,7 @@ tcf_del_notify(struct net *net, struct nlmsghdr *n, struct tc_action *actions[],}/* now do the delete */-ret=tcf_action_delete(net,actions);+ret=tcf_action_delete(net,actions,fallbacknum);if(ret<0){NL_SET_ERR_MSG(extack,"Failed to delete TC action");kfree_skb(skb);
@@ -282,6 +282,10 @@ tcf_exts_stats_update(const struct tcf_exts *exts,for(i=0;i<exts->nr_actions;i++){structtc_action*a=exts->actions[i];+/* if stats from hw, just skip */+if(!tcf_action_update_hw_stats(a))+continue;+tcf_action_stats_update(a,bytes,packets,drops,lastuse,true);a->used_hw_stats=used_hw_stats;
@@ -1089,15 +1089,18 @@ int tcf_action_offload_cmd_pre(struct tc_action *actions[],EXPORT_SYMBOL(tcf_action_offload_cmd_pre);inttcf_action_offload_cmd_post(structflow_offload_action*fl_act,-structnetlink_ext_ack*extack)+structnetlink_ext_ack*extack,+boolkeep_fl_act){if(IS_ERR(fl_act))returnPTR_ERR(fl_act);flow_indr_dev_setup_offload(NULL,NULL,TC_SETUP_ACT,fl_act,NULL,NULL);-tc_cleanup_flow_action(&fl_act->action);-kfree(fl_act);+if(!keep_fl_act){+tc_cleanup_flow_action(&fl_act->action);+kfree(fl_act);+}return0;}
@@ -1115,10 +1118,45 @@ int tcf_action_offload_cmd(struct tc_action *actions[],if(err)returnerr;-returntcf_action_offload_cmd_post(fl_act,extack);+returntcf_action_offload_cmd_post(fl_act,extack,false);}EXPORT_SYMBOL(tcf_action_offload_cmd);+inttcf_action_update_hw_stats(structtc_action*action)+{+structtc_action*actions[TCA_ACT_MAX_PRIO]={+[0]=action,+};+structflow_offload_action*fl_act;+interr=0;++err=tcf_action_offload_cmd_pre(actions,+FLOW_ACT_STATS,+NULL,+&fl_act);+if(err)+gotoerr_out;++err=tcf_action_offload_cmd_post(fl_act,NULL,true);++if(fl_act->stats.lastused){+tcf_action_stats_update(action,fl_act->stats.bytes,+fl_act->stats.pkts,+fl_act->stats.drops,+fl_act->stats.lastused,+true);+err=0;+}else{+err=-EOPNOTSUPP;+}+tc_cleanup_flow_action(&fl_act->action);+kfree(fl_act);++err_out:+returnerr;+}+EXPORT_SYMBOL(tcf_action_update_hw_stats);+/* offload the tc command after deleted */inttcf_action_offload_del_post(structflow_offload_action*fl_act,structtc_action*actions[],
@@ -1255,6 +1293,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: Roi Dayan <hidden> Date: 2021-07-22 12:24:16
On 2021-07-22 12:19 PM, Simon Horman wrote:
quoted hunk
From: Baowen Zheng <redacted>
Use flow_indr_dev_register/flow_indr_dev_setup_offload to
offload tc action.
We offload the tc action mainly for ovs meter configuration.
Make some basic changes for different vendors to return EOPNOTSUPP.
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.
As per review from the RFC, the kernel test robot will fail to run, so
we add CONFIG_NET_CLS_ACT control for the action offload.
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 +-
.../ethernet/mellanox/mlx5/core/en/rep/tc.c | 3 ++
.../ethernet/netronome/nfp/flower/offload.c | 3 ++
include/linux/netdevice.h | 1 +
include/net/flow_offload.h | 15 ++++++++
include/net/pkt_cls.h | 15 ++++++++
net/core/flow_offload.c | 26 +++++++++++++-
net/sched/act_api.c | 33 +++++++++++++++++
net/sched/cls_api.c | 36 ++++++++++++++++---
9 files changed, 128 insertions(+), 6 deletions(-)
@@ -923,6 +923,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
@@ -1060,6 +1060,36 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,returnERR_PTR(err);}+/* offload the tc command after inserted */+inttcf_action_offload_cmd(structtc_action*actions[],+structnetlink_ext_ack*extack)+{+structflow_offload_action*fl_act;+interr=0;++fl_act=flow_action_alloc(tcf_act_num_actions(actions));+if(!fl_act)+return-ENOMEM;++fl_act->extack=extack;+err=tc_setup_action(&fl_act->action,actions);+if(err){+NL_SET_ERR_MSG_MOD(extack,+"Failed to setup tc actions for offload\n");+gotoerr_out;+}+fl_act->command=FLOW_ACT_REPLACE;++flow_indr_dev_setup_offload(NULL,NULL,TC_SETUP_ACT,fl_act,NULL,NULL);++tc_cleanup_flow_action(&fl_act->action);++err_out:+kfree(fl_act);+returnerr;+}+EXPORT_SYMBOL(tcf_action_offload_cmd);+/* Returns numbers of initialized actions or negative error. */inttcf_action_init(structnet*net,structtcf_proto*tp,structnlattr*nla,
@@ -1514,6 +1544,9 @@ static int tcf_action_add(struct net *net, struct nlattr *nla,returnret;ret=tcf_add_notify(net,n,actions,portid,attr_size,extack);+/* offload actions to hardware if possible */+tcf_action_offload_cmd(actions,extack);+/* only put existing actions */for(i=0;i<TCA_ACT_MAX_PRIO;i++)if(init_res[i]==ACT_P_CREATED)
From: Simon Horman <hidden> Date: 2021-07-22 13:19:22
On Thu, Jul 22, 2021 at 03:24:07PM +0300, Roi Dayan wrote:
On 2021-07-22 12:19 PM, Simon Horman wrote:
quoted
From: Baowen Zheng <redacted>
Use flow_indr_dev_register/flow_indr_dev_setup_offload to
offload tc action.
We offload the tc action mainly for ovs meter configuration.
Make some basic changes for different vendors to return EOPNOTSUPP.
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.
As per review from the RFC, the kernel test robot will fail to run, so
we add CONFIG_NET_CLS_ACT control for the action offload.
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 +-
.../ethernet/mellanox/mlx5/core/en/rep/tc.c | 3 ++
.../ethernet/netronome/nfp/flower/offload.c | 3 ++
include/linux/netdevice.h | 1 +
include/net/flow_offload.h | 15 ++++++++
include/net/pkt_cls.h | 15 ++++++++
net/core/flow_offload.c | 26 +++++++++++++-
net/sched/act_api.c | 33 +++++++++++++++++
net/sched/cls_api.c | 36 ++++++++++++++++---
9 files changed, 128 insertions(+), 6 deletions(-)
@@ -923,6 +923,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
@@ -27,6 +27,27 @@ struct flow_rule *flow_rule_alloc(unsigned int num_actions)}EXPORT_SYMBOL(flow_rule_alloc);+structflow_offload_action*flow_action_alloc(unsignedintnum_actions)+{+structflow_offload_action*fl_action;+inti;++fl_action=kzalloc(struct_size(fl_action,action.entries,num_actions),+GFP_KERNEL);+if(!fl_action)+returnNULL;
Hi Simon,
Our automatic tests got a trace from flow_action_alloc()
introduced in this series.
I don't have specific commands right now but maybe its easy
to reproduce with option CONFIG_DEBUG_ATOMIC_SLEEP=y
fl_dump->fl_hw_update_stats->fl_hw_update_stats->tcf_exts_stats_update
->tcf_action_update_hw_stats->tcf_action_offload_cmd_pre->
->flow_action_alloc
Thanks,
Roi
On Thu 22 Jul 2021 at 12:19, 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 offload the tc action mainly for ovs meter configuration.
Make some basic changes for different vendors to return EOPNOTSUPP.
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.
As per review from the RFC, the kernel test robot will fail to run, so
we add CONFIG_NET_CLS_ACT control for the action offload.
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 +-
.../ethernet/mellanox/mlx5/core/en/rep/tc.c | 3 ++
.../ethernet/netronome/nfp/flower/offload.c | 3 ++
include/linux/netdevice.h | 1 +
include/net/flow_offload.h | 15 ++++++++
include/net/pkt_cls.h | 15 ++++++++
net/core/flow_offload.c | 26 +++++++++++++-
net/sched/act_api.c | 33 +++++++++++++++++
net/sched/cls_api.c | 36 ++++++++++++++++---
9 files changed, 128 insertions(+), 6 deletions(-)
@@ -923,6 +923,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
@@ -1060,6 +1060,36 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,returnERR_PTR(err);}+/* offload the tc command after inserted */+inttcf_action_offload_cmd(structtc_action*actions[],+structnetlink_ext_ack*extack)+{+structflow_offload_action*fl_act;+interr=0;++fl_act=flow_action_alloc(tcf_act_num_actions(actions));+if(!fl_act)+return-ENOMEM;++fl_act->extack=extack;+err=tc_setup_action(&fl_act->action,actions);+if(err){+NL_SET_ERR_MSG_MOD(extack,+"Failed to setup tc actions for offload\n");+gotoerr_out;+}+fl_act->command=FLOW_ACT_REPLACE;++flow_indr_dev_setup_offload(NULL,NULL,TC_SETUP_ACT,fl_act,NULL,NULL);++tc_cleanup_flow_action(&fl_act->action);++err_out:+kfree(fl_act);+returnerr;+}+EXPORT_SYMBOL(tcf_action_offload_cmd);+/* Returns numbers of initialized actions or negative error. */inttcf_action_init(structnet*net,structtcf_proto*tp,structnlattr*nla,
@@ -1514,6 +1544,9 @@ static int tcf_action_add(struct net *net, struct nlattr *nla,returnret;ret=tcf_add_notify(net,n,actions,portid,attr_size,extack);+/* offload actions to hardware if possible */+tcf_action_offload_cmd(actions,extack);+
I think this has already been suggested for RFC, but some sort of
visibility for offload status of action would be extremely welcome.
Perhaps "IN_HW" flag and counter, similar to what we have for offloaded
filters.
quoted hunk
/* only put existing actions */
for (i = 0; i < TCA_ACT_MAX_PRIO; i++)
if (init_res[i] == ACT_P_CREATED)
From: Jamal Hadi Salim <jhs@mojatatu.com> Date: 2021-07-22 13:33:18
On 2021-07-22 9:29 a.m., Vlad Buslov wrote:
On Thu 22 Jul 2021 at 12:19, 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 offload the tc action mainly for ovs meter configuration.
Make some basic changes for different vendors to return EOPNOTSUPP.
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.
As per review from the RFC, the kernel test robot will fail to run, so
we add CONFIG_NET_CLS_ACT control for the action offload.
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 +-
.../ethernet/mellanox/mlx5/core/en/rep/tc.c | 3 ++
+ /* offload actions to hardware if possible */
+ tcf_action_offload_cmd(actions, extack);
+
I think this has already been suggested for RFC, but some sort of
visibility for offload status of action would be extremely welcome.
Perhaps "IN_HW" flag and counter, similar to what we have for offloaded
filters.
Also showing a tc command line in the cover letter on how one would
ask for a specific action to be offloaded.
cheers,
jamal
On Thu 22 Jul 2021 at 12:19, Simon Horman [off-list ref] wrote:
quoted hunk
From: Baowen Zheng <redacted>
Add a basic process to delete offloaded actions from net device.
Should not remove the offloaded action entries if the action
fails to delete in tcf_del_notify.
Signed-off-by: Baowen Zheng <redacted>
Signed-off-by: Louis Peens <redacted>
Signed-off-by: Simon Horman <redacted>
---
include/net/pkt_cls.h | 1 +
net/sched/act_api.c | 112 +++++++++++++++++++++++++++++++++++-------
net/sched/cls_api.c | 14 ++++--
3 files changed, 106 insertions(+), 21 deletions(-)
@@ -1060,36 +1060,109 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,returnERR_PTR(err);}-/* offload the tc command after inserted */-inttcf_action_offload_cmd(structtc_action*actions[],-structnetlink_ext_ack*extack)+inttcf_action_offload_cmd_pre(structtc_action*actions[],+enumflow_act_commandcmd,+structnetlink_ext_ack*extack,+structflow_offload_action**fl_act){-structflow_offload_action*fl_act;+structflow_offload_action*fl_act_p;interr=0;-fl_act=flow_action_alloc(tcf_act_num_actions(actions));-if(!fl_act)+fl_act_p=flow_action_alloc(tcf_act_num_actions(actions));+if(!fl_act_p)return-ENOMEM;-fl_act->extack=extack;-err=tc_setup_action(&fl_act->action,actions);+fl_act_p->extack=extack;+fl_act_p->command=cmd;+err=tc_setup_action(&fl_act_p->action,actions);if(err){NL_SET_ERR_MSG_MOD(extack,"Failed to setup tc actions for offload\n");gotoerr_out;}-fl_act->command=FLOW_ACT_REPLACE;+*fl_act=fl_act_p;+return0;+err_out:+kfree(fl_act_p);+returnerr;+}+EXPORT_SYMBOL(tcf_action_offload_cmd_pre);
This doesn't seem be used anywhere outside this file.
+
+/* offload the tc command after inserted */
+int tcf_action_offload_cmd(struct tc_action *actions[],
+ struct netlink_ext_ack *extack)
+{
+ struct flow_offload_action *fl_act;
+ int err = 0;
+
+ err = tcf_action_offload_cmd_pre(actions,
+ FLOW_ACT_REPLACE,
+ extack,
+ &fl_act);
+ if (err)
+ return err;
+
+ return tcf_action_offload_cmd_post(fl_act, extack);
}
EXPORT_SYMBOL(tcf_action_offload_cmd);
+/* offload the tc command after deleted */
+int tcf_action_offload_del_post(struct flow_offload_action *fl_act,
+ struct tc_action *actions[],
+ struct netlink_ext_ack *extack,
+ int fallback_num)
+{
+ int fallback_entries = 0;
+ struct tc_action *act;
+ int total_entries = 0;
+ int i;
+
+ if (!fl_act)
+ return -EINVAL;
+
+ if (fallback_num) {
+ /* for each the actions to fallback the action entries remain in the actions */
+ for (i = 0; i < TCA_ACT_MAX_PRIO; i++) {
+ act = actions[i];
+ if (!act)
+ continue;
+
+ fallback_entries += tcf_act_num_actions_single(act);
+ }
+ fallback_entries += fallback_num;
+ }
+ total_entries = fl_act->action.num_entries;
+ if (total_entries > fallback_entries) {
+ /* just offload the actions that is not fallback and start with the actions */
+ fl_act->action.num_entries -= fallback_entries;
+ flow_indr_dev_setup_offload(NULL, NULL, TC_SETUP_ACT, fl_act, NULL, NULL);
+
+ /* recovery num_entries for cleanup */
+ fl_act->action.num_entries = total_entries;
+ } else {
+ NL_SET_ERR_MSG(extack, "no entries to offload when deleting the tc actions");
+ }
+
+ tc_cleanup_flow_action(&fl_act->action);
+
+ kfree(fl_act);
+ return 0;
+}
+EXPORT_SYMBOL(tcf_action_offload_del_post);
+
/* Returns numbers of initialized actions or negative error. */
int tcf_action_init(struct net *net, struct tcf_proto *tp, struct nlattr *nla,
@@ -1393,7 +1466,7 @@ static int tca_action_flush(struct net *net, struct nlattr *nla, return err; }-static int tcf_action_delete(struct net *net, struct tc_action *actions[])+static int tcf_action_delete(struct net *net, struct tc_action *actions[], int *fallbacknum) { int i;
@@ -1407,6 +1480,7 @@ static int tcf_action_delete(struct net *net, struct tc_action *actions[]) u32 act_index = a->tcfa_index; actions[i] = NULL;+ *fallbacknum = tcf_act_num_actions_single(a); if (tcf_action_put(a)) { /* last reference, action was deleted concurrently */ module_put(ops->owner);
@@ -1419,12 +1493,13 @@ static int tcf_action_delete(struct net *net, struct tc_action *actions[]) return ret; } }+ *fallbacknum = 0; return 0; } static int tcf_del_notify(struct net *net, struct nlmsghdr *n, struct tc_action *actions[],- u32 portid, size_t attr_size, struct netlink_ext_ack *extack)+ u32 portid, size_t attr_size, struct netlink_ext_ack *extack, int *fallbacknum) { int ret; struct sk_buff *skb;
@@ -1442,7 +1517,7 @@ tcf_del_notify(struct net *net, struct nlmsghdr *n, struct tc_action *actions[], } /* now do the delete */- ret = tcf_action_delete(net, actions);+ ret = tcf_action_delete(net, actions, fallbacknum); if (ret < 0) { NL_SET_ERR_MSG(extack, "Failed to delete TC action"); kfree_skb(skb);
@@ -1458,11 +1533,12 @@ static int tca_action_gd(struct net *net, struct nlattr *nla, struct nlmsghdr *n, u32 portid, int event, struct netlink_ext_ack *extack) {- int i, ret; struct nlattr *tb[TCA_ACT_MAX_PRIO + 1]; struct tc_action *act; size_t attr_size = 0; struct tc_action *actions[TCA_ACT_MAX_PRIO] = {};+ struct flow_offload_action *fl_act;+ int i, ret, fallback_num; ret = nla_parse_nested_deprecated(tb, TCA_ACT_MAX_PRIO, nla, NULL, extack);
@@ -1492,7 +1568,9 @@ tca_action_gd(struct net *net, struct nlattr *nla, struct nlmsghdr *n, if (event == RTM_GETACTION) ret = tcf_get_notify(net, portid, n, actions, event, extack); else { /* delete */- ret = tcf_del_notify(net, n, actions, portid, attr_size, extack);+ tcf_action_offload_cmd_pre(actions, FLOW_ACT_DESTROY, extack, &fl_act);+ ret = tcf_del_notify(net, n, actions, portid, attr_size, extack, &fallback_num);+ tcf_action_offload_del_post(fl_act, actions, extack, fallback_num);
This tcf_action_offload_cmd_{pre|post}() approach looks slightly
complicated, especially with fallback_num calculations. I would suggest
to simplify it by only initializing action cookies in
flow_action->entries[] (I assume you don't really need all the action
data just to delete it, right?) for DEL/STATS and do one of the
following:
- Unoffload actions one-by-one after every deletion in
tcf_actions_delete(), perhaps reusing the same flow_offload_action of
size 1 by only changing the cookie on each iteration.
- If you really want to send the whole batch to the driver, save cookies
for all successfully deleted actions in an array and initialize
compound flow_offload_action from the array.
This would remove the need for whole pre/post thing, which otherwise
gets even more complicated in following patch by 'keep_fl_act' arg.
@@ -282,6 +282,10 @@ tcf_exts_stats_update(const struct tcf_exts *exts,for(i=0;i<exts->nr_actions;i++){structtc_action*a=exts->actions[i];+/* if stats from hw, just skip */+if(!tcf_action_update_hw_stats(a))+continue;+
Is it okay to call this inside preempt disable section?
@@ -1089,15 +1089,18 @@ int tcf_action_offload_cmd_pre(struct tc_action *actions[],EXPORT_SYMBOL(tcf_action_offload_cmd_pre);inttcf_action_offload_cmd_post(structflow_offload_action*fl_act,-structnetlink_ext_ack*extack)+structnetlink_ext_ack*extack,+boolkeep_fl_act){if(IS_ERR(fl_act))returnPTR_ERR(fl_act);flow_indr_dev_setup_offload(NULL,NULL,TC_SETUP_ACT,fl_act,NULL,NULL);-tc_cleanup_flow_action(&fl_act->action);-kfree(fl_act);+if(!keep_fl_act){+tc_cleanup_flow_action(&fl_act->action);+kfree(fl_act);+}return0;}
@@ -1115,10 +1118,45 @@ int tcf_action_offload_cmd(struct tc_action *actions[],if(err)returnerr;-returntcf_action_offload_cmd_post(fl_act,extack);+returntcf_action_offload_cmd_post(fl_act,extack,false);}EXPORT_SYMBOL(tcf_action_offload_cmd);+inttcf_action_update_hw_stats(structtc_action*action)+{+structtc_action*actions[TCA_ACT_MAX_PRIO]={+[0]=action,+};+structflow_offload_action*fl_act;+interr=0;+
Having some way to distinguish offloaded actions would also be useful
here to skip this function. I wonder how this affects dump rate when
executed for every single action, even when none of them were offloaded
through action API.
@@ -1255,6 +1293,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. */
From: kernel test robot <hidden> Date: 2021-07-22 15:31:58
Hi Simon,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on net-next/master]
url: https://github.com/0day-ci/linux/commits/Simon-Horman/flow_offload-hardware-offload-of-TC-actions/20210722-172229
base: https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git c2255ff47768c94a0ebc3968f007928bb47ea43b
config: powerpc-randconfig-r016-20210722 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 9625ca5b602616b2f5584e8a49ba93c52c141e40)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install powerpc cross compiling tool for clang build
# apt-get install binutils-powerpc-linux-gnu
# https://github.com/0day-ci/linux/commit/9228a8efdbf7a736354b87c0db3260dd7d2c4abd
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Simon-Horman/flow_offload-hardware-offload-of-TC-actions/20210722-172229
git checkout 9228a8efdbf7a736354b87c0db3260dd7d2c4abd
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=powerpc
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <redacted>
All warnings (new ones prefixed by >>):
In file included from include/linux/dma-mapping.h:10:
In file included from include/linux/scatterlist.h:9:
In file included from arch/powerpc/include/asm/io.h:619:
arch/powerpc/include/asm/io-defs.h:43:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
DEF_PCI_AC_NORET(insb, (unsigned long p, void *b, unsigned long c),
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
arch/powerpc/include/asm/io.h:616:3: note: expanded from macro 'DEF_PCI_AC_NORET'
__do_##name al; \
^~~~~~~~~~~~~~
<scratch space>:238:1: note: expanded from here
__do_insb
^
arch/powerpc/include/asm/io.h:556:56: note: expanded from macro '__do_insb'
#define __do_insb(p, b, n) readsb((PCI_IO_ADDR)_IO_BASE+(p), (b), (n))
~~~~~~~~~~~~~~~~~~~~~^
In file included from net/sched/act_api.c:13:
In file included from include/linux/skbuff.h:31:
In file included from include/linux/dma-mapping.h:10:
In file included from include/linux/scatterlist.h:9:
In file included from arch/powerpc/include/asm/io.h:619:
arch/powerpc/include/asm/io-defs.h:45:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
DEF_PCI_AC_NORET(insw, (unsigned long p, void *b, unsigned long c),
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
arch/powerpc/include/asm/io.h:616:3: note: expanded from macro 'DEF_PCI_AC_NORET'
__do_##name al; \
^~~~~~~~~~~~~~
<scratch space>:2:1: note: expanded from here
__do_insw
^
arch/powerpc/include/asm/io.h:557:56: note: expanded from macro '__do_insw'
#define __do_insw(p, b, n) readsw((PCI_IO_ADDR)_IO_BASE+(p), (b), (n))
~~~~~~~~~~~~~~~~~~~~~^
In file included from net/sched/act_api.c:13:
In file included from include/linux/skbuff.h:31:
In file included from include/linux/dma-mapping.h:10:
In file included from include/linux/scatterlist.h:9:
In file included from arch/powerpc/include/asm/io.h:619:
arch/powerpc/include/asm/io-defs.h:47:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
DEF_PCI_AC_NORET(insl, (unsigned long p, void *b, unsigned long c),
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
arch/powerpc/include/asm/io.h:616:3: note: expanded from macro 'DEF_PCI_AC_NORET'
__do_##name al; \
^~~~~~~~~~~~~~
<scratch space>:4:1: note: expanded from here
__do_insl
^
arch/powerpc/include/asm/io.h:558:56: note: expanded from macro '__do_insl'
#define __do_insl(p, b, n) readsl((PCI_IO_ADDR)_IO_BASE+(p), (b), (n))
~~~~~~~~~~~~~~~~~~~~~^
In file included from net/sched/act_api.c:13:
In file included from include/linux/skbuff.h:31:
In file included from include/linux/dma-mapping.h:10:
In file included from include/linux/scatterlist.h:9:
In file included from arch/powerpc/include/asm/io.h:619:
arch/powerpc/include/asm/io-defs.h:49:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
DEF_PCI_AC_NORET(outsb, (unsigned long p, const void *b, unsigned long c),
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
arch/powerpc/include/asm/io.h:616:3: note: expanded from macro 'DEF_PCI_AC_NORET'
__do_##name al; \
^~~~~~~~~~~~~~
<scratch space>:6:1: note: expanded from here
__do_outsb
^
arch/powerpc/include/asm/io.h:559:58: note: expanded from macro '__do_outsb'
#define __do_outsb(p, b, n) writesb((PCI_IO_ADDR)_IO_BASE+(p),(b),(n))
~~~~~~~~~~~~~~~~~~~~~^
In file included from net/sched/act_api.c:13:
In file included from include/linux/skbuff.h:31:
In file included from include/linux/dma-mapping.h:10:
In file included from include/linux/scatterlist.h:9:
In file included from arch/powerpc/include/asm/io.h:619:
arch/powerpc/include/asm/io-defs.h:51:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
DEF_PCI_AC_NORET(outsw, (unsigned long p, const void *b, unsigned long c),
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
arch/powerpc/include/asm/io.h:616:3: note: expanded from macro 'DEF_PCI_AC_NORET'
__do_##name al; \
^~~~~~~~~~~~~~
<scratch space>:8:1: note: expanded from here
__do_outsw
^
arch/powerpc/include/asm/io.h:560:58: note: expanded from macro '__do_outsw'
#define __do_outsw(p, b, n) writesw((PCI_IO_ADDR)_IO_BASE+(p),(b),(n))
~~~~~~~~~~~~~~~~~~~~~^
In file included from net/sched/act_api.c:13:
In file included from include/linux/skbuff.h:31:
In file included from include/linux/dma-mapping.h:10:
In file included from include/linux/scatterlist.h:9:
In file included from arch/powerpc/include/asm/io.h:619:
arch/powerpc/include/asm/io-defs.h:53:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
DEF_PCI_AC_NORET(outsl, (unsigned long p, const void *b, unsigned long c),
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
arch/powerpc/include/asm/io.h:616:3: note: expanded from macro 'DEF_PCI_AC_NORET'
__do_##name al; \
^~~~~~~~~~~~~~
<scratch space>:10:1: note: expanded from here
__do_outsl
^
arch/powerpc/include/asm/io.h:561:58: note: expanded from macro '__do_outsl'
#define __do_outsl(p, b, n) writesl((PCI_IO_ADDR)_IO_BASE+(p),(b),(n))
~~~~~~~~~~~~~~~~~~~~~^
quoted
net/sched/act_api.c:1064:5: warning: no previous prototype for function 'tcf_action_offload_cmd' [-Wmissing-prototypes]
int tcf_action_offload_cmd(struct tc_action *actions[],
^
net/sched/act_api.c:1064:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
int tcf_action_offload_cmd(struct tc_action *actions[],
^
static
13 warnings generated.
vim +/tcf_action_offload_cmd +1064 net/sched/act_api.c
1062
1063 /* offload the tc command after inserted */
1064 int tcf_action_offload_cmd(struct tc_action *actions[],
From: kernel test robot <hidden> Date: 2021-07-22 17:08:18
Hi Simon,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on net-next/master]
url: https://github.com/0day-ci/linux/commits/Simon-Horman/flow_offload-hardware-offload-of-TC-actions/20210722-172229
base: https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git c2255ff47768c94a0ebc3968f007928bb47ea43b
config: powerpc-randconfig-r016-20210722 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 9625ca5b602616b2f5584e8a49ba93c52c141e40)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install powerpc cross compiling tool for clang build
# apt-get install binutils-powerpc-linux-gnu
# https://github.com/0day-ci/linux/commit/a8e2d0acfc98c127ab0b5189f7635049515c43f3
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Simon-Horman/flow_offload-hardware-offload-of-TC-actions/20210722-172229
git checkout a8e2d0acfc98c127ab0b5189f7635049515c43f3
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=powerpc
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <redacted>
All warnings (new ones prefixed by >>):
In file included from include/linux/dma-mapping.h:10:
In file included from include/linux/scatterlist.h:9:
In file included from arch/powerpc/include/asm/io.h:619:
arch/powerpc/include/asm/io-defs.h:43:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
DEF_PCI_AC_NORET(insb, (unsigned long p, void *b, unsigned long c),
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
arch/powerpc/include/asm/io.h:616:3: note: expanded from macro 'DEF_PCI_AC_NORET'
__do_##name al; \
^~~~~~~~~~~~~~
<scratch space>:238:1: note: expanded from here
__do_insb
^
arch/powerpc/include/asm/io.h:556:56: note: expanded from macro '__do_insb'
#define __do_insb(p, b, n) readsb((PCI_IO_ADDR)_IO_BASE+(p), (b), (n))
~~~~~~~~~~~~~~~~~~~~~^
In file included from net/sched/act_api.c:13:
In file included from include/linux/skbuff.h:31:
In file included from include/linux/dma-mapping.h:10:
In file included from include/linux/scatterlist.h:9:
In file included from arch/powerpc/include/asm/io.h:619:
arch/powerpc/include/asm/io-defs.h:45:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
DEF_PCI_AC_NORET(insw, (unsigned long p, void *b, unsigned long c),
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
arch/powerpc/include/asm/io.h:616:3: note: expanded from macro 'DEF_PCI_AC_NORET'
__do_##name al; \
^~~~~~~~~~~~~~
<scratch space>:2:1: note: expanded from here
__do_insw
^
arch/powerpc/include/asm/io.h:557:56: note: expanded from macro '__do_insw'
#define __do_insw(p, b, n) readsw((PCI_IO_ADDR)_IO_BASE+(p), (b), (n))
~~~~~~~~~~~~~~~~~~~~~^
In file included from net/sched/act_api.c:13:
In file included from include/linux/skbuff.h:31:
In file included from include/linux/dma-mapping.h:10:
In file included from include/linux/scatterlist.h:9:
In file included from arch/powerpc/include/asm/io.h:619:
arch/powerpc/include/asm/io-defs.h:47:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
DEF_PCI_AC_NORET(insl, (unsigned long p, void *b, unsigned long c),
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
arch/powerpc/include/asm/io.h:616:3: note: expanded from macro 'DEF_PCI_AC_NORET'
__do_##name al; \
^~~~~~~~~~~~~~
<scratch space>:4:1: note: expanded from here
__do_insl
^
arch/powerpc/include/asm/io.h:558:56: note: expanded from macro '__do_insl'
#define __do_insl(p, b, n) readsl((PCI_IO_ADDR)_IO_BASE+(p), (b), (n))
~~~~~~~~~~~~~~~~~~~~~^
In file included from net/sched/act_api.c:13:
In file included from include/linux/skbuff.h:31:
In file included from include/linux/dma-mapping.h:10:
In file included from include/linux/scatterlist.h:9:
In file included from arch/powerpc/include/asm/io.h:619:
arch/powerpc/include/asm/io-defs.h:49:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
DEF_PCI_AC_NORET(outsb, (unsigned long p, const void *b, unsigned long c),
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
arch/powerpc/include/asm/io.h:616:3: note: expanded from macro 'DEF_PCI_AC_NORET'
__do_##name al; \
^~~~~~~~~~~~~~
<scratch space>:6:1: note: expanded from here
__do_outsb
^
arch/powerpc/include/asm/io.h:559:58: note: expanded from macro '__do_outsb'
#define __do_outsb(p, b, n) writesb((PCI_IO_ADDR)_IO_BASE+(p),(b),(n))
~~~~~~~~~~~~~~~~~~~~~^
In file included from net/sched/act_api.c:13:
In file included from include/linux/skbuff.h:31:
In file included from include/linux/dma-mapping.h:10:
In file included from include/linux/scatterlist.h:9:
In file included from arch/powerpc/include/asm/io.h:619:
arch/powerpc/include/asm/io-defs.h:51:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
DEF_PCI_AC_NORET(outsw, (unsigned long p, const void *b, unsigned long c),
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
arch/powerpc/include/asm/io.h:616:3: note: expanded from macro 'DEF_PCI_AC_NORET'
__do_##name al; \
^~~~~~~~~~~~~~
<scratch space>:8:1: note: expanded from here
__do_outsw
^
arch/powerpc/include/asm/io.h:560:58: note: expanded from macro '__do_outsw'
#define __do_outsw(p, b, n) writesw((PCI_IO_ADDR)_IO_BASE+(p),(b),(n))
~~~~~~~~~~~~~~~~~~~~~^
In file included from net/sched/act_api.c:13:
In file included from include/linux/skbuff.h:31:
In file included from include/linux/dma-mapping.h:10:
In file included from include/linux/scatterlist.h:9:
In file included from arch/powerpc/include/asm/io.h:619:
arch/powerpc/include/asm/io-defs.h:53:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
DEF_PCI_AC_NORET(outsl, (unsigned long p, const void *b, unsigned long c),
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
arch/powerpc/include/asm/io.h:616:3: note: expanded from macro 'DEF_PCI_AC_NORET'
__do_##name al; \
^~~~~~~~~~~~~~
<scratch space>:10:1: note: expanded from here
__do_outsl
^
arch/powerpc/include/asm/io.h:561:58: note: expanded from macro '__do_outsl'
#define __do_outsl(p, b, n) writesl((PCI_IO_ADDR)_IO_BASE+(p),(b),(n))
~~~~~~~~~~~~~~~~~~~~~^
quoted
net/sched/act_api.c:1063:5: warning: no previous prototype for function 'tcf_action_offload_cmd_pre' [-Wmissing-prototypes]
int tcf_action_offload_cmd_pre(struct tc_action *actions[],
^
net/sched/act_api.c:1063:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
int tcf_action_offload_cmd_pre(struct tc_action *actions[],
^
static
quoted
net/sched/act_api.c:1091:5: warning: no previous prototype for function 'tcf_action_offload_cmd_post' [-Wmissing-prototypes]
int tcf_action_offload_cmd_post(struct flow_offload_action *fl_act,
^
net/sched/act_api.c:1091:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
int tcf_action_offload_cmd_post(struct flow_offload_action *fl_act,
^
static
net/sched/act_api.c:1105:5: warning: no previous prototype for function 'tcf_action_offload_cmd' [-Wmissing-prototypes]
int tcf_action_offload_cmd(struct tc_action *actions[],
^
net/sched/act_api.c:1105:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
int tcf_action_offload_cmd(struct tc_action *actions[],
^
static
quoted
net/sched/act_api.c:1123:5: warning: no previous prototype for function 'tcf_action_offload_del_post' [-Wmissing-prototypes]
int tcf_action_offload_del_post(struct flow_offload_action *fl_act,
^
net/sched/act_api.c:1123:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
int tcf_action_offload_del_post(struct flow_offload_action *fl_act,
^
static
16 warnings generated.
vim +/tcf_action_offload_cmd_pre +1063 net/sched/act_api.c
1062
1063 int tcf_action_offload_cmd_pre(struct tc_action *actions[],
From: Simon Horman <hidden> Date: 2021-07-27 13:04:36
On Thu, Jul 22, 2021 at 09:33:09AM -0400, Jamal Hadi Salim wrote:
On 2021-07-22 9:29 a.m., Vlad Buslov wrote:
quoted
On Thu 22 Jul 2021 at 12:19, 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 offload the tc action mainly for ovs meter configuration.
Make some basic changes for different vendors to return EOPNOTSUPP.
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.
As per review from the RFC, the kernel test robot will fail to run, so
we add CONFIG_NET_CLS_ACT control for the action offload.
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 +-
.../ethernet/mellanox/mlx5/core/en/rep/tc.c | 3 ++
+ /* offload actions to hardware if possible */
+ tcf_action_offload_cmd(actions, extack);
+
I think this has already been suggested for RFC, but some sort of
visibility for offload status of action would be extremely welcome.
Perhaps "IN_HW" flag and counter, similar to what we have for offloaded
filters.
Also showing a tc command line in the cover letter on how one would
ask for a specific action to be offloaded.
In practice actions are offloaded when a flow using them is offloaded.
So I think we need to consider what the meaning of IN_HW is.
Is it that:
* The driver (and potentially hardware, though not in our current
implementation) has accepted the action for offload;
* That a classifier that uses the action has bee offloaded;
* Or something else?
With regards to a counter, I'm not quite sure what this would be:
* The number of devices where the action has been offloaded (which ties
into the question of what we mean by IN_HW)
* The number of offloaded classifier instances using the action
* Something else
Regarding a flag to control offload:
* For classifiers (at least the flower classifier) there is the skip_sw and
skip_hw flags, which allow control of placement of a classifier in SW and
HW.
* We could add similar flags for actions, which at least in my
world view would have the net-effect of controlling which classifiers can
be added to sw and hw - f.e. a classifier that uses an action marked
skip_hw could not be added to HW.
* Doing so would add some extra complexity and its not immediately apparent
to me what the use-case would be given that there are already flags for
classifiers.
On Tue 27 Jul 2021 at 16:04, Simon Horman [off-list ref] wrote:
On Thu, Jul 22, 2021 at 09:33:09AM -0400, Jamal Hadi Salim wrote:
quoted
On 2021-07-22 9:29 a.m., Vlad Buslov wrote:
quoted
On Thu 22 Jul 2021 at 12:19, 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 offload the tc action mainly for ovs meter configuration.
Make some basic changes for different vendors to return EOPNOTSUPP.
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.
As per review from the RFC, the kernel test robot will fail to run, so
we add CONFIG_NET_CLS_ACT control for the action offload.
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 +-
.../ethernet/mellanox/mlx5/core/en/rep/tc.c | 3 ++
+ /* offload actions to hardware if possible */
+ tcf_action_offload_cmd(actions, extack);
+
I think this has already been suggested for RFC, but some sort of
visibility for offload status of action would be extremely welcome.
Perhaps "IN_HW" flag and counter, similar to what we have for offloaded
filters.
Also showing a tc command line in the cover letter on how one would
ask for a specific action to be offloaded.
In practice actions are offloaded when a flow using them is offloaded.
So I think we need to consider what the meaning of IN_HW is.
Is it that:
* The driver (and potentially hardware, though not in our current
implementation) has accepted the action for offload;
* That a classifier that uses the action has bee offloaded;
* Or something else?
I think we have the same issue with filters - they might not be in
hardware after driver callback returned "success" (due to neigh state
being invalid for tunnel_key encap, for example).
With regards to a counter, I'm not quite sure what this would be:
* The number of devices where the action has been offloaded (which ties
into the question of what we mean by IN_HW)
* The number of offloaded classifier instances using the action
* Something else
I would prefer to have semantics similar to filters:
1. Count number of driver callbacks that returned "success".
2. If count > 0, then set in_hw flag.
3. Set in_hw_count to success count.
This would allow user to immediately determine whether action passed
driver validation.
Regarding a flag to control offload:
* For classifiers (at least the flower classifier) there is the skip_sw and
skip_hw flags, which allow control of placement of a classifier in SW and
HW.
* We could add similar flags for actions, which at least in my
world view would have the net-effect of controlling which classifiers can
be added to sw and hw - f.e. a classifier that uses an action marked
skip_hw could not be added to HW.
* Doing so would add some extra complexity and its not immediately apparent
to me what the use-case would be given that there are already flags for
classifiers.
Yeah, adding such flag for action offload seems to complicate things.
Also, "skip_sw" flag doesn't even make much sense for actions. I thought
that "skip_hw" flag would be nice to have for users that would like to
avoid "spamming" their NIC drivers (potentially causing higher latency
and resource consumption) for filters/actions they have no intention to
offload to hardware, but I'm not sure how useful is that option really
is.
From: Jamal Hadi Salim <jhs@mojatatu.com> Date: 2021-07-27 16:13:45
On 2021-07-27 10:38 a.m., Vlad Buslov wrote:
On Tue 27 Jul 2021 at 16:04, Simon Horman [off-list ref] wrote:
quoted
quoted
Also showing a tc command line in the cover letter on how one would
ask for a specific action to be offloaded.
In practice actions are offloaded when a flow using them is offloaded.
So I think we need to consider what the meaning of IN_HW is.
Is it that:
* The driver (and potentially hardware, though not in our current
implementation) has accepted the action for offload;
* That a classifier that uses the action has bee offloaded;
* Or something else?
I think we have the same issue with filters - they might not be in
hardware after driver callback returned "success" (due to neigh state
being invalid for tunnel_key encap, for example).
Sounds like we need another state for this. Otherwise, how do you debug
that something is sitting in the driver and not in hardware after you
issued a command to offload it? How do i tell today?
Also knowing reason why something is sitting in the driver would be
helpful.
quoted
With regards to a counter, I'm not quite sure what this would be:
* The number of devices where the action has been offloaded (which ties
into the question of what we mean by IN_HW)
* The number of offloaded classifier instances using the action
* Something else
I would prefer to have semantics similar to filters:
1. Count number of driver callbacks that returned "success".
2. If count > 0, then set in_hw flag.
3. Set in_hw_count to success count.
This would allow user to immediately determine whether action passed
driver validation.
I didnt follow this:
Are we refering to the the "block" semantics (where a filter for
example applies to multiple devices)?
quoted
Regarding a flag to control offload:
* For classifiers (at least the flower classifier) there is the skip_sw and
skip_hw flags, which allow control of placement of a classifier in SW and
HW.
* We could add similar flags for actions, which at least in my
world view would have the net-effect of controlling which classifiers can
be added to sw and hw - f.e. a classifier that uses an action marked
skip_hw could not be added to HW.
I guess it depends on the hardware implementation.
In S/W we have two modes:
Approach A: create an action and then 2) bind it to a filter.
Approach B: Create a filter and then bind it to an action.
And #2A can be repeated multiple times for the same action
(would require some index as a reference for the action)
To Simon's comment above that would mean allowing
"a classifier that uses an action marked skip_hw to be added to HW"
i.e
Some hardware is capable of doing both option #A and #B.
Todays offload assumes #B - in which both filter and action are assumed
offloaded.
I am hoping whatever approach we end up agreeing on doesnt limit
either mode.
quoted
* Doing so would add some extra complexity and its not immediately apparent
to me what the use-case would be given that there are already flags for
classifiers.
Yeah, adding such flag for action offload seems to complicate things.
Also, "skip_sw" flag doesn't even make much sense for actions. I thought
that "skip_hw" flag would be nice to have for users that would like to
avoid "spamming" their NIC drivers (potentially causing higher latency
and resource consumption) for filters/actions they have no intention to
offload to hardware, but I'm not sure how useful is that option really
is.
Hold on Vlad.
So you are looking at this mostly as an optimization to speed up h/w
control updates? ;->
I was looking at it more as a (currently missing) feature improvement.
We already have a use case that is implemented by s/w today. The feature
mimics it in h/w.
At minimal all existing NICs should be able to support the counters
as mapped to simple actions like drop. I understand for example if some
cant support adding separately offloading of tunnels for example.
So the syntax is something along the lines of:
tc actions add action drop index 15 skip_sw
tc filter add dev ...parent ... protocol ip prio X ..\
u32/flower skip_sw match ... flowid 1:10 action gact index 15
You get an error if counter index 15 is not offloaded or
if skip_sw was left out..
And then later on, if you support sharing of actions:
tc filter add dev ...parent ... protocol ip prio X2 ..\
u32/flower skip_sw match ... flowid 1:10 action gact index 15
cheers,
jamal
On Tue 27 Jul 2021 at 19:13, Jamal Hadi Salim [off-list ref] wrote:
On 2021-07-27 10:38 a.m., Vlad Buslov wrote:
quoted
On Tue 27 Jul 2021 at 16:04, Simon Horman [off-list ref] wrote:
quoted
quoted
quoted
Also showing a tc command line in the cover letter on how one would
ask for a specific action to be offloaded.
In practice actions are offloaded when a flow using them is offloaded.
So I think we need to consider what the meaning of IN_HW is.
Is it that:
* The driver (and potentially hardware, though not in our current
implementation) has accepted the action for offload;
* That a classifier that uses the action has bee offloaded;
* Or something else?
I think we have the same issue with filters - they might not be in
hardware after driver callback returned "success" (due to neigh state
being invalid for tunnel_key encap, for example).
Sounds like we need another state for this. Otherwise, how do you debug
that something is sitting in the driver and not in hardware after you
issued a command to offload it? How do i tell today?
Also knowing reason why something is sitting in the driver would be
helpful.
It is not about just adding another state. The issue is that there is no
way for drivers to change the state of software filter dynamically.
quoted
quoted
With regards to a counter, I'm not quite sure what this would be:
* The number of devices where the action has been offloaded (which ties
into the question of what we mean by IN_HW)
* The number of offloaded classifier instances using the action
* Something else
I would prefer to have semantics similar to filters:
1. Count number of driver callbacks that returned "success".
2. If count > 0, then set in_hw flag.
3. Set in_hw_count to success count.
This would allow user to immediately determine whether action passed
driver validation.
I didnt follow this:
Are we refering to the the "block" semantics (where a filter for
example applies to multiple devices)?
This uses indirect offload infrastructure, which means all drivers
in flow_block_indr_dev_list will receive action offload requests.
quoted
quoted
Regarding a flag to control offload:
* For classifiers (at least the flower classifier) there is the skip_sw and
skip_hw flags, which allow control of placement of a classifier in SW and
HW.
* We could add similar flags for actions, which at least in my
world view would have the net-effect of controlling which classifiers can
be added to sw and hw - f.e. a classifier that uses an action marked
skip_hw could not be added to HW.
I guess it depends on the hardware implementation.
In S/W we have two modes:
Approach A: create an action and then 2) bind it to a filter.
Approach B: Create a filter and then bind it to an action.
And #2A can be repeated multiple times for the same action
(would require some index as a reference for the action)
To Simon's comment above that would mean allowing
"a classifier that uses an action marked skip_hw to be added to HW"
i.e
Some hardware is capable of doing both option #A and #B.
Todays offload assumes #B - in which both filter and action are assumed
offloaded.
I am hoping whatever approach we end up agreeing on doesnt limit
either mode.
quoted
quoted
* Doing so would add some extra complexity and its not immediately apparent
to me what the use-case would be given that there are already flags for
classifiers.
Yeah, adding such flag for action offload seems to complicate things.
Also, "skip_sw" flag doesn't even make much sense for actions. I thought
that "skip_hw" flag would be nice to have for users that would like to
avoid "spamming" their NIC drivers (potentially causing higher latency
and resource consumption) for filters/actions they have no intention to
offload to hardware, but I'm not sure how useful is that option really
is.
Hold on Vlad.
So you are looking at this mostly as an optimization to speed up h/w
control updates? ;->
No. How would adding more flags improve h/w update rate? I was just
thinking that it is strange that users that are not interested in
offloads would suddenly have higher memory usage for their actions just
because they happen to have offload-capable driver loaded. But it is not
a major concern for me.
I was looking at it more as a (currently missing) feature improvement.
We already have a use case that is implemented by s/w today. The feature
mimics it in h/w.
At minimal all existing NICs should be able to support the counters
as mapped to simple actions like drop. I understand for example if some
cant support adding separately offloading of tunnels for example.
So the syntax is something along the lines of:
tc actions add action drop index 15 skip_sw
tc filter add dev ...parent ... protocol ip prio X ..\
u32/flower skip_sw match ... flowid 1:10 action gact index 15
You get an error if counter index 15 is not offloaded or
if skip_sw was left out..
And then later on, if you support sharing of actions:
tc filter add dev ...parent ... protocol ip prio X2 ..\
u32/flower skip_sw match ... flowid 1:10 action gact index 15
cheers,
jamal
From: Simon Horman <hidden> Date: 2021-07-28 07:46:39
On Tue, Jul 27, 2021 at 07:47:43PM +0300, Vlad Buslov wrote:
On Tue 27 Jul 2021 at 19:13, Jamal Hadi Salim [off-list ref] wrote:
quoted
On 2021-07-27 10:38 a.m., Vlad Buslov wrote:
quoted
On Tue 27 Jul 2021 at 16:04, Simon Horman [off-list ref] wrote:
quoted
quoted
quoted
Also showing a tc command line in the cover letter on how one would
ask for a specific action to be offloaded.
In practice actions are offloaded when a flow using them is offloaded.
So I think we need to consider what the meaning of IN_HW is.
Is it that:
* The driver (and potentially hardware, though not in our current
implementation) has accepted the action for offload;
* That a classifier that uses the action has bee offloaded;
* Or something else?
I think we have the same issue with filters - they might not be in
hardware after driver callback returned "success" (due to neigh state
being invalid for tunnel_key encap, for example).
Sounds like we need another state for this. Otherwise, how do you debug
that something is sitting in the driver and not in hardware after you
issued a command to offload it? How do i tell today?
Also knowing reason why something is sitting in the driver would be
helpful.
It is not about just adding another state. The issue is that there is no
way for drivers to change the state of software filter dynamically.
I think it might be worth considering enhancing things at some point.
But I agree that its more than a matter of adding an extra flag. And
I think it's reasonable to implement something similar to the classifier
current offload handling of IN_HW now and consider enhancements separately.
quoted
quoted
quoted
With regards to a counter, I'm not quite sure what this would be:
* The number of devices where the action has been offloaded (which ties
into the question of what we mean by IN_HW)
* The number of offloaded classifier instances using the action
* Something else
I would prefer to have semantics similar to filters:
1. Count number of driver callbacks that returned "success".
2. If count > 0, then set in_hw flag.
3. Set in_hw_count to success count.
This would allow user to immediately determine whether action passed
driver validation.
Thanks, that makes sense to me.
quoted
I didnt follow this:
Are we refering to the the "block" semantics (where a filter for
example applies to multiple devices)?
This uses indirect offload infrastructure, which means all drivers
in flow_block_indr_dev_list will receive action offload requests.
quoted
quoted
quoted
Regarding a flag to control offload:
* For classifiers (at least the flower classifier) there is the skip_sw and
skip_hw flags, which allow control of placement of a classifier in SW and
HW.
* We could add similar flags for actions, which at least in my
world view would have the net-effect of controlling which classifiers can
be added to sw and hw - f.e. a classifier that uses an action marked
skip_hw could not be added to HW.
I guess it depends on the hardware implementation.
In S/W we have two modes:
Approach A: create an action and then 2) bind it to a filter.
Approach B: Create a filter and then bind it to an action.
And #2A can be repeated multiple times for the same action
(would require some index as a reference for the action)
To Simon's comment above that would mean allowing
"a classifier that uses an action marked skip_hw to be added to HW"
i.e
Some hardware is capable of doing both option #A and #B.
Todays offload assumes #B - in which both filter and action are assumed
offloaded.
I am hoping whatever approach we end up agreeing on doesnt limit
either mode.
quoted
quoted
* Doing so would add some extra complexity and its not immediately apparent
to me what the use-case would be given that there are already flags for
classifiers.
Yeah, adding such flag for action offload seems to complicate things.
Also, "skip_sw" flag doesn't even make much sense for actions. I thought
that "skip_hw" flag would be nice to have for users that would like to
avoid "spamming" their NIC drivers (potentially causing higher latency
and resource consumption) for filters/actions they have no intention to
offload to hardware, but I'm not sure how useful is that option really
is.
Hold on Vlad.
So you are looking at this mostly as an optimization to speed up h/w
control updates? ;->
No. How would adding more flags improve h/w update rate? I was just
thinking that it is strange that users that are not interested in
offloads would suddenly have higher memory usage for their actions just
because they happen to have offload-capable driver loaded. But it is not
a major concern for me.
In that case can we rely on the global tc-offload on/off flag
provided by ethtool? (I understand its not the same, but perhaps
it is sufficient in practice.)
quoted
I was looking at it more as a (currently missing) feature improvement.
We already have a use case that is implemented by s/w today. The feature
mimics it in h/w.
At minimal all existing NICs should be able to support the counters
as mapped to simple actions like drop. I understand for example if some
cant support adding separately offloading of tunnels for example.
So the syntax is something along the lines of:
tc actions add action drop index 15 skip_sw
tc filter add dev ...parent ... protocol ip prio X ..\
u32/flower skip_sw match ... flowid 1:10 action gact index 15
You get an error if counter index 15 is not offloaded or
if skip_sw was left out..
And then later on, if you support sharing of actions:
tc filter add dev ...parent ... protocol ip prio X2 ..\
u32/flower skip_sw match ... flowid 1:10 action gact index 15
Right, I understand that makes sense and is internally consistent.
But I think that in practice it only makes a difference "Approach B"
implementations, none of which currently exist.
I would suggest we can add this when the need arises, rather than
speculatively without hw/driver support. Its not precluded by the current
model AFAIK.
On Wed 28 Jul 2021 at 10:46, Simon Horman [off-list ref] wrote:
On Tue, Jul 27, 2021 at 07:47:43PM +0300, Vlad Buslov wrote:
quoted
On Tue 27 Jul 2021 at 19:13, Jamal Hadi Salim [off-list ref] wrote:
quoted
On 2021-07-27 10:38 a.m., Vlad Buslov wrote:
quoted
On Tue 27 Jul 2021 at 16:04, Simon Horman [off-list ref] wrote:
quoted
quoted
quoted
Also showing a tc command line in the cover letter on how one would
ask for a specific action to be offloaded.
In practice actions are offloaded when a flow using them is offloaded.
So I think we need to consider what the meaning of IN_HW is.
Is it that:
* The driver (and potentially hardware, though not in our current
implementation) has accepted the action for offload;
* That a classifier that uses the action has bee offloaded;
* Or something else?
I think we have the same issue with filters - they might not be in
hardware after driver callback returned "success" (due to neigh state
being invalid for tunnel_key encap, for example).
Sounds like we need another state for this. Otherwise, how do you debug
that something is sitting in the driver and not in hardware after you
issued a command to offload it? How do i tell today?
Also knowing reason why something is sitting in the driver would be
helpful.
It is not about just adding another state. The issue is that there is no
way for drivers to change the state of software filter dynamically.
I think it might be worth considering enhancing things at some point.
But I agree that its more than a matter of adding an extra flag. And
I think it's reasonable to implement something similar to the classifier
current offload handling of IN_HW now and consider enhancements separately.
quoted
quoted
quoted
quoted
With regards to a counter, I'm not quite sure what this would be:
* The number of devices where the action has been offloaded (which ties
into the question of what we mean by IN_HW)
* The number of offloaded classifier instances using the action
* Something else
I would prefer to have semantics similar to filters:
1. Count number of driver callbacks that returned "success".
2. If count > 0, then set in_hw flag.
3. Set in_hw_count to success count.
This would allow user to immediately determine whether action passed
driver validation.
Thanks, that makes sense to me.
quoted
quoted
I didnt follow this:
Are we refering to the the "block" semantics (where a filter for
example applies to multiple devices)?
This uses indirect offload infrastructure, which means all drivers
in flow_block_indr_dev_list will receive action offload requests.
quoted
quoted
quoted
Regarding a flag to control offload:
* For classifiers (at least the flower classifier) there is the skip_sw and
skip_hw flags, which allow control of placement of a classifier in SW and
HW.
* We could add similar flags for actions, which at least in my
world view would have the net-effect of controlling which classifiers can
be added to sw and hw - f.e. a classifier that uses an action marked
skip_hw could not be added to HW.
I guess it depends on the hardware implementation.
In S/W we have two modes:
Approach A: create an action and then 2) bind it to a filter.
Approach B: Create a filter and then bind it to an action.
And #2A can be repeated multiple times for the same action
(would require some index as a reference for the action)
To Simon's comment above that would mean allowing
"a classifier that uses an action marked skip_hw to be added to HW"
i.e
Some hardware is capable of doing both option #A and #B.
Todays offload assumes #B - in which both filter and action are assumed
offloaded.
I am hoping whatever approach we end up agreeing on doesnt limit
either mode.
quoted
quoted
* Doing so would add some extra complexity and its not immediately apparent
to me what the use-case would be given that there are already flags for
classifiers.
Yeah, adding such flag for action offload seems to complicate things.
Also, "skip_sw" flag doesn't even make much sense for actions. I thought
that "skip_hw" flag would be nice to have for users that would like to
avoid "spamming" their NIC drivers (potentially causing higher latency
and resource consumption) for filters/actions they have no intention to
offload to hardware, but I'm not sure how useful is that option really
is.
Hold on Vlad.
So you are looking at this mostly as an optimization to speed up h/w
control updates? ;->
No. How would adding more flags improve h/w update rate? I was just
thinking that it is strange that users that are not interested in
offloads would suddenly have higher memory usage for their actions just
because they happen to have offload-capable driver loaded. But it is not
a major concern for me.
In that case can we rely on the global tc-offload on/off flag
provided by ethtool? (I understand its not the same, but perhaps
it is sufficient in practice.)
Yes, the ethtool should be sufficient. Didn't think about it initially.
Thanks!
quoted
quoted
I was looking at it more as a (currently missing) feature improvement.
We already have a use case that is implemented by s/w today. The feature
mimics it in h/w.
At minimal all existing NICs should be able to support the counters
as mapped to simple actions like drop. I understand for example if some
cant support adding separately offloading of tunnels for example.
So the syntax is something along the lines of:
tc actions add action drop index 15 skip_sw
tc filter add dev ...parent ... protocol ip prio X ..\
u32/flower skip_sw match ... flowid 1:10 action gact index 15
You get an error if counter index 15 is not offloaded or
if skip_sw was left out..
And then later on, if you support sharing of actions:
tc filter add dev ...parent ... protocol ip prio X2 ..\
u32/flower skip_sw match ... flowid 1:10 action gact index 15
Right, I understand that makes sense and is internally consistent.
But I think that in practice it only makes a difference "Approach B"
implementations, none of which currently exist.
I would suggest we can add this when the need arises, rather than
speculatively without hw/driver support. Its not precluded by the current
model AFAIK.
From: Jamal Hadi Salim <jhs@mojatatu.com> Date: 2021-07-28 13:53:10
On 2021-07-28 3:46 a.m., Simon Horman wrote:
On Tue, Jul 27, 2021 at 07:47:43PM +0300, Vlad Buslov wrote:
quoted
On Tue 27 Jul 2021 at 19:13, Jamal Hadi Salim [off-list ref] wrote:
quoted
On 2021-07-27 10:38 a.m., Vlad Buslov wrote:
quoted
On Tue 27 Jul 2021 at 16:04, Simon Horman [off-list ref] wrote:
[..]
quoted
quoted
quoted
I think we have the same issue with filters - they might not be in
hardware after driver callback returned "success" (due to neigh state
being invalid for tunnel_key encap, for example).
Sounds like we need another state for this. Otherwise, how do you debug
that something is sitting in the driver and not in hardware after you
issued a command to offload it? How do i tell today?
Also knowing reason why something is sitting in the driver would be
helpful.
It is not about just adding another state. The issue is that there is no
way for drivers to change the state of software filter dynamically.
I think it might be worth considering enhancing things at some point.
But I agree that its more than a matter of adding an extra flag. And
I think it's reasonable to implement something similar to the classifier
current offload handling of IN_HW now and consider enhancements separately.
Debugability is very important. If we have such gotchas we need to have
the admin at least be able to tell if the driver returns "success"
and the request is still sitting in the driver for whatever reason
At minimal there needs to be some indicator somewhere which say
"inprogress" or "waiting for resolution" etc.
If the control plane(user space app) starts making other decisions
based on assumptions that filter was successfully installed i.e
packets are being treated in the hardware then there could be
consequences when this assumption is wrong.
So if i undestood the challenge correctly it is: how do you relay
this info back so it is reflected in the filter details. Yes that
would require some mechanism to exist and possibly mapping state
between whats in the driver and in the cls layer.
If i am not mistaken, the switchdev folks handle this asynchronicty?
+Cc Ido, Jiri, Roopa
And it should be noted that: Yes, the filters have this
pre-existing condition but doesnt mean given the opportunity
to do actions we should replicate what they do.
[..]
quoted
quoted
I didnt follow this:
Are we refering to the the "block" semantics (where a filter for
example applies to multiple devices)?
This uses indirect offload infrastructure, which means all drivers
in flow_block_indr_dev_list will receive action offload requests.
Ok, understood.
[..]
quoted
No. How would adding more flags improve h/w update rate? I was just
thinking that it is strange that users that are not interested in
offloads would suddenly have higher memory usage for their actions just
because they happen to have offload-capable driver loaded. But it is not
a major concern for me.
In that case can we rely on the global tc-offload on/off flag
provided by ethtool? (I understand its not the same, but perhaps
it is sufficient in practice.)
ok.
So: I think i have seen this what is probably the spamming refered
with the intel (800?) driver ;-> Basically driver was reacting to
all filters regardless of need to offload or not.
I thought it was an oversight on their part and the driver needed
fixing. Are we invoking the offload regardless of whether h/w offload
is requested? In my naive view - at least when i looked at the intel
code - it didnt seem hard to avoid the spamming.
quoted
quoted
I was looking at it more as a (currently missing) feature improvement.
We already have a use case that is implemented by s/w today. The feature
mimics it in h/w.
At minimal all existing NICs should be able to support the counters
as mapped to simple actions like drop. I understand for example if some
cant support adding separately offloading of tunnels for example.
So the syntax is something along the lines of:
tc actions add action drop index 15 skip_sw
tc filter add dev ...parent ... protocol ip prio X ..\
u32/flower skip_sw match ... flowid 1:10 action gact index 15
You get an error if counter index 15 is not offloaded or
if skip_sw was left out..
And then later on, if you support sharing of actions:
tc filter add dev ...parent ... protocol ip prio X2 ..\
u32/flower skip_sw match ... flowid 1:10 action gact index 15
Right, I understand that makes sense and is internally consistent.
But I think that in practice it only makes a difference "Approach B"
implementations, none of which currently exist.
At minimal:
Shouldnt counters (easily correlated to basic actions like drop or
accept) fit the scenario of:
tc actions add action drop index 15 skip_sw
tc filter add dev ...parent ... protocol ip prio X .. \
u32/flower skip_sw match ... flowid 1:10 action gact index 15
?
I would suggest we can add this when the need arises, rather than
speculatively without hw/driver support. Its not precluded by the current
model AFAIK.
We are going to work on a driver that would have the "B" approach.
I am hoping - whatever the consensus here - it doesnt require a
surgery afterwards to make that work.
cheers,
jamal
From: Simon Horman <hidden> Date: 2021-07-28 14:46:39
On Wed, Jul 28, 2021 at 09:51:00AM -0400, Jamal Hadi Salim wrote:
On 2021-07-28 3:46 a.m., Simon Horman wrote:
quoted
On Tue, Jul 27, 2021 at 07:47:43PM +0300, Vlad Buslov wrote:
quoted
On Tue 27 Jul 2021 at 19:13, Jamal Hadi Salim [off-list ref] wrote:
quoted
On 2021-07-27 10:38 a.m., Vlad Buslov wrote:
quoted
On Tue 27 Jul 2021 at 16:04, Simon Horman [off-list ref] wrote:
[..]
quoted
quoted
quoted
quoted
I think we have the same issue with filters - they might not be in
hardware after driver callback returned "success" (due to neigh state
being invalid for tunnel_key encap, for example).
Sounds like we need another state for this. Otherwise, how do you debug
that something is sitting in the driver and not in hardware after you
issued a command to offload it? How do i tell today?
Also knowing reason why something is sitting in the driver would be
helpful.
It is not about just adding another state. The issue is that there is no
way for drivers to change the state of software filter dynamically.
I think it might be worth considering enhancing things at some point.
But I agree that its more than a matter of adding an extra flag. And
I think it's reasonable to implement something similar to the classifier
current offload handling of IN_HW now and consider enhancements separately.
Debugability is very important. If we have such gotchas we need to have
the admin at least be able to tell if the driver returns "success"
and the request is still sitting in the driver for whatever reason
At minimal there needs to be some indicator somewhere which say
"inprogress" or "waiting for resolution" etc.
If the control plane(user space app) starts making other decisions
based on assumptions that filter was successfully installed i.e
packets are being treated in the hardware then there could be
consequences when this assumption is wrong.
So if i undestood the challenge correctly it is: how do you relay
this info back so it is reflected in the filter details. Yes that
would require some mechanism to exist and possibly mapping state
between whats in the driver and in the cls layer.
If i am not mistaken, the switchdev folks handle this asynchronicty?
+Cc Ido, Jiri, Roopa
And it should be noted that: Yes, the filters have this
pre-existing condition but doesnt mean given the opportunity
to do actions we should replicate what they do.
I'd prefer symmetry between the use of IN_HW for filters and actions,
which I believe is what Vlad has suggested.
If we wish to enhance things - f.e. for debugging, which I
agree is important - then I think that is a separate topic.
[..]
quoted
quoted
quoted
I didnt follow this:
Are we refering to the the "block" semantics (where a filter for
example applies to multiple devices)?
This uses indirect offload infrastructure, which means all drivers
in flow_block_indr_dev_list will receive action offload requests.
Ok, understood.
[..]
quoted
quoted
No. How would adding more flags improve h/w update rate? I was just
thinking that it is strange that users that are not interested in
offloads would suddenly have higher memory usage for their actions just
because they happen to have offload-capable driver loaded. But it is not
a major concern for me.
In that case can we rely on the global tc-offload on/off flag
provided by ethtool? (I understand its not the same, but perhaps
it is sufficient in practice.)
ok.
So: I think i have seen this what is probably the spamming refered
with the intel (800?) driver ;-> Basically driver was reacting to
all filters regardless of need to offload or not.
I thought it was an oversight on their part and the driver needed
fixing. Are we invoking the offload regardless of whether h/w offload
is requested? In my naive view - at least when i looked at the intel
code - it didnt seem hard to avoid the spamming.
There is a per-netdev (not global as I wrote above) flag to enable and
disable offload. And there is per-classifier skip_hw flag. I can dig
through the code as easily as you can but I'd be surprised if the
driver is seeing offload requests if either of those settings
are in effect.
quoted
quoted
quoted
I was looking at it more as a (currently missing) feature improvement.
We already have a use case that is implemented by s/w today. The feature
mimics it in h/w.
At minimal all existing NICs should be able to support the counters
as mapped to simple actions like drop. I understand for example if some
cant support adding separately offloading of tunnels for example.
So the syntax is something along the lines of:
tc actions add action drop index 15 skip_sw
tc filter add dev ...parent ... protocol ip prio X ..\
u32/flower skip_sw match ... flowid 1:10 action gact index 15
You get an error if counter index 15 is not offloaded or
if skip_sw was left out..
And then later on, if you support sharing of actions:
tc filter add dev ...parent ... protocol ip prio X2 ..\
u32/flower skip_sw match ... flowid 1:10 action gact index 15
Right, I understand that makes sense and is internally consistent.
But I think that in practice it only makes a difference "Approach B"
implementations, none of which currently exist.
At minimal:
Shouldnt counters (easily correlated to basic actions like drop or
accept) fit the scenario of:
tc actions add action drop index 15 skip_sw
tc filter add dev ...parent ... protocol ip prio X .. \
u32/flower skip_sw match ... flowid 1:10 action gact index 15
?
quoted
I would suggest we can add this when the need arises, rather than
speculatively without hw/driver support. Its not precluded by the current
model AFAIK.
We are going to work on a driver that would have the "B" approach.
I am hoping - whatever the consensus here - it doesnt require a
surgery afterwards to make that work.
You should be able to build on the work proposed here to add what you
suggest into the framework to meet these requirements for your driver work.
From: Jamal Hadi Salim <jhs@mojatatu.com> Date: 2021-07-30 10:17:23
On 2021-07-28 10:46 a.m., Simon Horman wrote:
On Wed, Jul 28, 2021 at 09:51:00AM -0400, Jamal Hadi Salim wrote:
quoted
On 2021-07-28 3:46 a.m., Simon Horman wrote:
quoted
On Tue, Jul 27, 2021 at 07:47:43PM +0300, Vlad Buslov wrote:
quoted
On Tue 27 Jul 2021 at 19:13, Jamal Hadi Salim [off-list ref] wrote:
quoted
On 2021-07-27 10:38 a.m., Vlad Buslov wrote:
quoted
On Tue 27 Jul 2021 at 16:04, Simon Horman [off-list ref] wrote:
[..]
quoted
quoted
quoted
quoted
I think we have the same issue with filters - they might not be in
hardware after driver callback returned "success" (due to neigh state
being invalid for tunnel_key encap, for example).
Sounds like we need another state for this. Otherwise, how do you debug
that something is sitting in the driver and not in hardware after you
issued a command to offload it? How do i tell today?
Also knowing reason why something is sitting in the driver would be
helpful.
It is not about just adding another state. The issue is that there is no
way for drivers to change the state of software filter dynamically.
I think it might be worth considering enhancing things at some point.
But I agree that its more than a matter of adding an extra flag. And
I think it's reasonable to implement something similar to the classifier
current offload handling of IN_HW now and consider enhancements separately.
Debugability is very important. If we have such gotchas we need to have
the admin at least be able to tell if the driver returns "success"
and the request is still sitting in the driver for whatever reason
At minimal there needs to be some indicator somewhere which say
"inprogress" or "waiting for resolution" etc.
If the control plane(user space app) starts making other decisions
based on assumptions that filter was successfully installed i.e
packets are being treated in the hardware then there could be
consequences when this assumption is wrong.
So if i undestood the challenge correctly it is: how do you relay
this info back so it is reflected in the filter details. Yes that
would require some mechanism to exist and possibly mapping state
between whats in the driver and in the cls layer.
If i am not mistaken, the switchdev folks handle this asynchronicty?
+Cc Ido, Jiri, Roopa
And it should be noted that: Yes, the filters have this
pre-existing condition but doesnt mean given the opportunity
to do actions we should replicate what they do.
I'd prefer symmetry between the use of IN_HW for filters and actions,
which I believe is what Vlad has suggested.
It still not clear to me what it means from a command line pov.
How do i add a rule and when i dump it what does it show?
If we wish to enhance things - f.e. for debugging, which I
agree is important - then I think that is a separate topic.
My only concern is not to repeat mistakes that are in filters
just for the sake of symmetry. Example the fact that something
went wrong with insertion or insertion is still in progress
and you get an indication that all went well.
Looking at mlnx (NIC) ndrivers it does seem that in the normal case
the insertion into hw is synchronous (for anything that is not sw
only). I didnt quiet see what Vlad was referring to.
We have spent literally hours debugging issues where rules are being
offloaded thinking it was the driver so any extra info helps.
quoted
quoted
quoted
No. How would adding more flags improve h/w update rate? I was just
thinking that it is strange that users that are not interested in
offloads would suddenly have higher memory usage for their actions just
because they happen to have offload-capable driver loaded. But it is not
a major concern for me.
In that case can we rely on the global tc-offload on/off flag
provided by ethtool? (I understand its not the same, but perhaps
it is sufficient in practice.)
ok.
So: I think i have seen this what is probably the spamming refered
with the intel (800?) driver ;-> Basically driver was reacting to
all filters regardless of need to offload or not.
I thought it was an oversight on their part and the driver needed
fixing. Are we invoking the offload regardless of whether h/w offload
is requested? In my naive view - at least when i looked at the intel
code - it didnt seem hard to avoid the spamming.
There is a per-netdev (not global as I wrote above) flag to enable and
disable offload. And there is per-classifier skip_hw flag. I can dig
through the code as easily as you can but I'd be surprised if the
driver is seeing offload requests if either of those settings
are in effect.
For sure it was the 800 (Ice driver if you want to dig into the code).
I think the ethtool flag was turned on but not skip_sw in the policy. I
dont have the card installed in any board right now - either will go and
dig into the logs (because it spews the message into the logs).
Again not clear if this is what Vlad was calling spam.
quoted
quoted
quoted
quoted
I was looking at it more as a (currently missing) feature improvement.
We already have a use case that is implemented by s/w today. The feature
mimics it in h/w.
At minimal all existing NICs should be able to support the counters
as mapped to simple actions like drop. I understand for example if some
cant support adding separately offloading of tunnels for example.
So the syntax is something along the lines of:
tc actions add action drop index 15 skip_sw
tc filter add dev ...parent ... protocol ip prio X ..\
u32/flower skip_sw match ... flowid 1:10 action gact index 15
You get an error if counter index 15 is not offloaded or
if skip_sw was left out..
And then later on, if you support sharing of actions:
tc filter add dev ...parent ... protocol ip prio X2 ..\
u32/flower skip_sw match ... flowid 1:10 action gact index 15
Right, I understand that makes sense and is internally consistent.
But I think that in practice it only makes a difference "Approach B"
implementations, none of which currently exist.
At minimal:
Shouldnt counters (easily correlated to basic actions like drop or
accept) fit the scenario of:
tc actions add action drop index 15 skip_sw
tc filter add dev ...parent ... protocol ip prio X .. \
u32/flower skip_sw match ... flowid 1:10 action gact index 15
?
quoted
I would suggest we can add this when the need arises, rather than
speculatively without hw/driver support. Its not precluded by the current
model AFAIK.
We are going to work on a driver that would have the "B" approach.
I am hoping - whatever the consensus here - it doesnt require a
surgery afterwards to make that work.
You should be able to build on the work proposed here to add what you
suggest into the framework to meet these requirements for your driver work.
Then we are good. These are the same patches you have here?
cheers,
jamal
On Fri 30 Jul 2021 at 13:17, Jamal Hadi Salim [off-list ref] wrote:
On 2021-07-28 10:46 a.m., Simon Horman wrote:
quoted
On Wed, Jul 28, 2021 at 09:51:00AM -0400, Jamal Hadi Salim wrote:
quoted
On 2021-07-28 3:46 a.m., Simon Horman wrote:
quoted
On Tue, Jul 27, 2021 at 07:47:43PM +0300, Vlad Buslov wrote:
quoted
On Tue 27 Jul 2021 at 19:13, Jamal Hadi Salim [off-list ref] wrote:
quoted
On 2021-07-27 10:38 a.m., Vlad Buslov wrote:
quoted
On Tue 27 Jul 2021 at 16:04, Simon Horman [off-list ref] wrote:
[..]
quoted
quoted
quoted
quoted
I think we have the same issue with filters - they might not be in
hardware after driver callback returned "success" (due to neigh state
being invalid for tunnel_key encap, for example).
Sounds like we need another state for this. Otherwise, how do you debug
that something is sitting in the driver and not in hardware after you
issued a command to offload it? How do i tell today?
Also knowing reason why something is sitting in the driver would be
helpful.
It is not about just adding another state. The issue is that there is no
way for drivers to change the state of software filter dynamically.
I think it might be worth considering enhancing things at some point.
But I agree that its more than a matter of adding an extra flag. And
I think it's reasonable to implement something similar to the classifier
current offload handling of IN_HW now and consider enhancements separately.
Debugability is very important. If we have such gotchas we need to have
the admin at least be able to tell if the driver returns "success"
and the request is still sitting in the driver for whatever reason
At minimal there needs to be some indicator somewhere which say
"inprogress" or "waiting for resolution" etc.
If the control plane(user space app) starts making other decisions
based on assumptions that filter was successfully installed i.e
packets are being treated in the hardware then there could be
consequences when this assumption is wrong.
So if i undestood the challenge correctly it is: how do you relay
this info back so it is reflected in the filter details. Yes that
would require some mechanism to exist and possibly mapping state
between whats in the driver and in the cls layer.
If i am not mistaken, the switchdev folks handle this asynchronicty?
+Cc Ido, Jiri, Roopa
And it should be noted that: Yes, the filters have this
pre-existing condition but doesnt mean given the opportunity
to do actions we should replicate what they do.
I'd prefer symmetry between the use of IN_HW for filters and actions,
which I believe is what Vlad has suggested.
It still not clear to me what it means from a command line pov.
How do i add a rule and when i dump it what does it show?
quoted
If we wish to enhance things - f.e. for debugging, which I
agree is important - then I think that is a separate topic.
My only concern is not to repeat mistakes that are in filters
just for the sake of symmetry. Example the fact that something
went wrong with insertion or insertion is still in progress
and you get an indication that all went well.
Looking at mlnx (NIC) ndrivers it does seem that in the normal case
the insertion into hw is synchronous (for anything that is not sw
only). I didnt quiet see what Vlad was referring to.
Filters with tunnel_key encap actions can be offloaded/unoffloaded
dynamically based on neigh state (see mlx5e_rep_neigh_update()) and fib
events (see mlx5e_tc_fib_event_work()).
[...]
From: Simon Horman <hidden> Date: 2021-07-30 13:20:16
On Fri, Jul 30, 2021 at 06:17:18AM -0400, Jamal Hadi Salim wrote:
On 2021-07-28 10:46 a.m., Simon Horman wrote:
quoted
On Wed, Jul 28, 2021 at 09:51:00AM -0400, Jamal Hadi Salim wrote:
quoted
On 2021-07-28 3:46 a.m., Simon Horman wrote:
quoted
On Tue, Jul 27, 2021 at 07:47:43PM +0300, Vlad Buslov wrote:
quoted
On Tue 27 Jul 2021 at 19:13, Jamal Hadi Salim [off-list ref] wrote:
quoted
On 2021-07-27 10:38 a.m., Vlad Buslov wrote:
quoted
On Tue 27 Jul 2021 at 16:04, Simon Horman [off-list ref] wrote:
[..]
quoted
quoted
quoted
quoted
I think we have the same issue with filters - they might not be in
hardware after driver callback returned "success" (due to neigh state
being invalid for tunnel_key encap, for example).
Sounds like we need another state for this. Otherwise, how do you debug
that something is sitting in the driver and not in hardware after you
issued a command to offload it? How do i tell today?
Also knowing reason why something is sitting in the driver would be
helpful.
It is not about just adding another state. The issue is that there is no
way for drivers to change the state of software filter dynamically.
I think it might be worth considering enhancing things at some point.
But I agree that its more than a matter of adding an extra flag. And
I think it's reasonable to implement something similar to the classifier
current offload handling of IN_HW now and consider enhancements separately.
Debugability is very important. If we have such gotchas we need to have
the admin at least be able to tell if the driver returns "success"
and the request is still sitting in the driver for whatever reason
At minimal there needs to be some indicator somewhere which say
"inprogress" or "waiting for resolution" etc.
If the control plane(user space app) starts making other decisions
based on assumptions that filter was successfully installed i.e
packets are being treated in the hardware then there could be
consequences when this assumption is wrong.
So if i undestood the challenge correctly it is: how do you relay
this info back so it is reflected in the filter details. Yes that
would require some mechanism to exist and possibly mapping state
between whats in the driver and in the cls layer.
If i am not mistaken, the switchdev folks handle this asynchronicty?
+Cc Ido, Jiri, Roopa
And it should be noted that: Yes, the filters have this
pre-existing condition but doesnt mean given the opportunity
to do actions we should replicate what they do.
I'd prefer symmetry between the use of IN_HW for filters and actions,
which I believe is what Vlad has suggested.
It still not clear to me what it means from a command line pov.
How do i add a rule and when i dump it what does it show?
How about we confirm that once we've implemented the feature.
But I would assume that:
* Existing methods for adding rules work as before
* When one dumps an action (in a sufficiently verbose
way) the in_hw and in_hw_counter fields are displayed as they are for
filters.
Does that help?
quoted
If we wish to enhance things - f.e. for debugging, which I
agree is important - then I think that is a separate topic.
My only concern is not to repeat mistakes that are in filters
just for the sake of symmetry. Example the fact that something
went wrong with insertion or insertion is still in progress
and you get an indication that all went well.
Looking at mlnx (NIC) ndrivers it does seem that in the normal case
the insertion into hw is synchronous (for anything that is not sw
only). I didnt quiet see what Vlad was referring to.
We have spent literally hours debugging issues where rules are being
offloaded thinking it was the driver so any extra info helps.
I do think there is a value to symmetry between the APIs.
And I don't think doing so moves things in a bad direction.
But rather a separate discussion is needed to discuss how to
improve debuggability.
...
quoted
quoted
quoted
quoted
quoted
I was looking at it more as a (currently missing) feature improvement.
We already have a use case that is implemented by s/w today. The feature
mimics it in h/w.
At minimal all existing NICs should be able to support the counters
as mapped to simple actions like drop. I understand for example if some
cant support adding separately offloading of tunnels for example.
So the syntax is something along the lines of:
tc actions add action drop index 15 skip_sw
tc filter add dev ...parent ... protocol ip prio X ..\
u32/flower skip_sw match ... flowid 1:10 action gact index 15
You get an error if counter index 15 is not offloaded or
if skip_sw was left out..
And then later on, if you support sharing of actions:
tc filter add dev ...parent ... protocol ip prio X2 ..\
u32/flower skip_sw match ... flowid 1:10 action gact index 15
Right, I understand that makes sense and is internally consistent.
But I think that in practice it only makes a difference "Approach B"
implementations, none of which currently exist.
At minimal:
Shouldnt counters (easily correlated to basic actions like drop or
accept) fit the scenario of:
tc actions add action drop index 15 skip_sw
tc filter add dev ...parent ... protocol ip prio X .. \
u32/flower skip_sw match ... flowid 1:10 action gact index 15
?
quoted
I would suggest we can add this when the need arises, rather than
speculatively without hw/driver support. Its not precluded by the current
model AFAIK.
We are going to work on a driver that would have the "B" approach.
I am hoping - whatever the consensus here - it doesnt require a
surgery afterwards to make that work.
You should be able to build on the work proposed here to add what you
suggest into the framework to meet these requirements for your driver work.
Then we are good. These are the same patches you have here?
From: Jamal Hadi Salim <jhs@mojatatu.com> Date: 2021-08-03 09:58:00
On 2021-07-30 7:40 a.m., Vlad Buslov wrote:
On Fri 30 Jul 2021 at 13:17, Jamal Hadi Salim [off-list ref] wrote:
quoted
On 2021-07-28 10:46 a.m., Simon Horman wrote:
Filters with tunnel_key encap actions can be offloaded/unoffloaded
dynamically based on neigh state (see mlx5e_rep_neigh_update()) and fib
events (see mlx5e_tc_fib_event_work()).
Thanks. Will look and compare against the FIB case.
cheers,
jamal
From: Jamal Hadi Salim <jhs@mojatatu.com> Date: 2021-08-03 10:14:13
On 2021-07-30 9:20 a.m., Simon Horman wrote:
On Fri, Jul 30, 2021 at 06:17:18AM -0400, Jamal Hadi Salim wrote:
quoted
On 2021-07-28 10:46 a.m., Simon Horman wrote:
[..]
quoted
It still not clear to me what it means from a command line pov.
How do i add a rule and when i dump it what does it show?
How about we confirm that once we've implemented the feature.
But I would assume that:
* Existing methods for adding rules work as before
* When one dumps an action (in a sufficiently verbose
way) the in_hw and in_hw_counter fields are displayed as they are for
filters.
Does that help?
I think it would help a lot more to say explicitly what it actually
means in the cover letter from a tc cli pov since the subject
is about offloading actions _independently_ of filters.
I am assuming you have some more patches on top of these that
actually will actually work for that.
Example of something you could show was adding a policer,
like so:
tc actions add action ... skip_sw...
then show get or dump showing things in h/w.
And del..
And i certainly hope that the above works and it is
not meant just for the consumption of some OVS use
case.
quoted
quoted
If we wish to enhance things - f.e. for debugging, which I
agree is important - then I think that is a separate topic.
My only concern is not to repeat mistakes that are in filters
just for the sake of symmetry. Example the fact that something
went wrong with insertion or insertion is still in progress
and you get an indication that all went well.
Looking at mlnx (NIC) ndrivers it does seem that in the normal case
the insertion into hw is synchronous (for anything that is not sw
only). I didnt quiet see what Vlad was referring to.
We have spent literally hours debugging issues where rules are being
offloaded thinking it was the driver so any extra info helps.
I do think there is a value to symmetry between the APIs.
And I don't think doing so moves things in a bad direction.
But rather a separate discussion is needed to discuss how to
improve debuggability.
Fair enough - lets have a separate discussion on debug-ability.
Maybe a new thread after reviewing Vlad's pointer.
quoted
quoted
You should be able to build on the work proposed here to add what you
suggest into the framework to meet these requirements for your driver work.
Then we are good. These are the same patches you have here?
Yes.
Thanks - will review with that in mind.
cheers,
jamal
From: Jamal Hadi Salim <jhs@mojatatu.com> Date: 2021-08-03 10:51:09
On 2021-07-22 5:19 a.m., Simon Horman wrote:
From: Baowen Zheng <redacted>
Use flow_indr_dev_register/flow_indr_dev_setup_offload to
offload tc action.
We offload the tc action mainly for ovs meter configuration.
Make some basic changes for different vendors to return EOPNOTSUPP.
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.
@@ -1060,6 +1060,36 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,returnERR_PTR(err);}+/* offload the tc command after inserted */+inttcf_action_offload_cmd(structtc_action*actions[],+structnetlink_ext_ack*extack)+{+structflow_offload_action*fl_act;+interr=0;++fl_act=flow_action_alloc(tcf_act_num_actions(actions));+if(!fl_act)+return-ENOMEM;++fl_act->extack=extack;+err=tc_setup_action(&fl_act->action,actions);+if(err){+NL_SET_ERR_MSG_MOD(extack,+"Failed to setup tc actions for offload\n");+gotoerr_out;+}+fl_act->command=FLOW_ACT_REPLACE;+
The fn name is a bit misleading with _cmd suffix when it is
only targeting one command: REPLACE (and not the other two).
cheers,
jamal
From: Jamal Hadi Salim <jhs@mojatatu.com> Date: 2021-08-03 10:59:19
On 2021-07-22 5:19 a.m., Simon Horman wrote:
[..]
quoted hunk
tca_action_gd(struct net *net, struct nlattr *nla, struct nlmsghdr *n,
u32 portid, int event, struct netlink_ext_ack *extack)
{
- int i, ret;
struct nlattr *tb[TCA_ACT_MAX_PRIO + 1];
struct tc_action *act;
size_t attr_size = 0;
struct tc_action *actions[TCA_ACT_MAX_PRIO] = {};
+ struct flow_offload_action *fl_act;
+ int i, ret, fallback_num;
ret = nla_parse_nested_deprecated(tb, TCA_ACT_MAX_PRIO, nla, NULL,
extack);
@@ -1492,7 +1568,9 @@ tca_action_gd(struct net *net, struct nlattr *nla, struct nlmsghdr *n, if (event == RTM_GETACTION) ret = tcf_get_notify(net, portid, n, actions, event, extack); else { /* delete */- ret = tcf_del_notify(net, n, actions, portid, attr_size, extack);+ tcf_action_offload_cmd_pre(actions, FLOW_ACT_DESTROY, extack, &fl_act);+ ret = tcf_del_notify(net, n, actions, portid, attr_size, extack, &fallback_num);+ tcf_action_offload_del_post(fl_act, actions, extack, fallback_num); if (ret) goto err;
It is hard to read from a patch context, but iiuc:
if the hardware update fails in tcf_action_offload_del_post() then
user space would still have been notified that it succeeded via
tcf_del_notify()... and there is no remediation after the fact.
cheers,
jamal
From: Jamal Hadi Salim <jhs@mojatatu.com> Date: 2021-08-03 11:05:57
On 2021-07-22 5:19 a.m., Simon Horman wrote:
Triggered by my observation on 2/3 went back and looked at this again to
see if we have same problem with notification on REPLACE case (I think
we do) but here's another comment:
quoted hunk
+EXPORT_SYMBOL(tcf_action_offload_cmd);
+
/* Returns numbers of initialized actions or negative error. */
int tcf_action_init(struct net *net, struct tcf_proto *tp, struct nlattr *nla,
@@ -1514,6 +1544,9 @@ static int tcf_action_add(struct net *net, struct nlattr *nla, return ret; ret = tcf_add_notify(net, n, actions, portid, attr_size, extack);+ /* offload actions to hardware if possible */+ tcf_action_offload_cmd(actions, extack);+
Above seems to be unconditional whether hw update is requested or not?
The comment says the right thing ("if possible") but the code
should have checked some sort of skip_sw check?
cheers,
jamal
From: Jamal Hadi Salim <jhs@mojatatu.com> Date: 2021-08-03 11:24:51
On 2021-07-22 5:19 a.m., Simon Horman wrote:
[..]
quoted hunk
/* offload the tc command after deleted */
int tcf_action_offload_del_post(struct flow_offload_action *fl_act,
struct tc_action *actions[],
@@ -1255,6 +1293,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);
This is more a curiosity than a comment on the patch: Is the
driver polling for these stats synchronously and what we get here
is the last update or do we end up invoking beyond
the driver when requesting for the stats?
Overall commentary from looking at the patch set:
I believe your patches will support the individual tc actions
add/del/get/dump command line requests.
What is missing is an example usage all the way to the driver. I am sure
you have additional patches that put this to good use. My suggestion
is to test that cli with that pov against your overall patches and
show this in your commit logs - even if those patches are to follow
later.
cheers,
jamal
From: Simon Horman <hidden> Date: 2021-08-03 11:31:11
On Tue, Aug 03, 2021 at 07:05:52AM -0400, Jamal Hadi Salim wrote:
On 2021-07-22 5:19 a.m., Simon Horman wrote:
Triggered by my observation on 2/3 went back and looked at this again to
see if we have same problem with notification on REPLACE case (I think
we do) but here's another comment:
quoted
+EXPORT_SYMBOL(tcf_action_offload_cmd);
+
/* Returns numbers of initialized actions or negative error. */
int tcf_action_init(struct net *net, struct tcf_proto *tp, struct nlattr *nla,
@@ -1514,6 +1544,9 @@ static int tcf_action_add(struct net *net, struct nlattr *nla, return ret; ret = tcf_add_notify(net, n, actions, portid, attr_size, extack);+ /* offload actions to hardware if possible */+ tcf_action_offload_cmd(actions, extack);+
Above seems to be unconditional whether hw update is requested or not?
The comment says the right thing ("if possible") but the code
should have checked some sort of skip_sw check?
Jamal, we are going around in circles.
As we have already discussed, this patchset does not add support for
skip_sw (or skip_hw) for actions.
From: Simon Horman <hidden> Date: 2021-08-03 11:35:35
On Tue, Aug 03, 2021 at 07:24:47AM -0400, Jamal Hadi Salim wrote:
On 2021-07-22 5:19 a.m., Simon Horman wrote:
[..]
quoted
/* offload the tc command after deleted */
int tcf_action_offload_del_post(struct flow_offload_action *fl_act,
struct tc_action *actions[],
@@ -1255,6 +1293,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);
This is more a curiosity than a comment on the patch: Is the
driver polling for these stats synchronously and what we get here
is the last update or do we end up invoking beyond
the driver when requesting for the stats?
I would have to double check but I believe the driver will report
back stats already received from the HW, rather than going all the way
to HW when the above call is made.
Overall commentary from looking at the patch set:
I believe your patches will support the individual tc actions
add/del/get/dump command line requests.
Yes, that is the aim of this patchset.
What is missing is an example usage all the way to the driver. I am sure
you have additional patches that put this to good use. My suggestion
is to test that cli with that pov against your overall patches and
show this in your commit logs - even if those patches are to follow
later.
Thanks, I'll see about making that so.
Just to be clear. We do have patches for the driver. And we do plan to post
them for inclusion in mainline. But I do believe that from a review
perspective its easier if one thing follows another.
From: Simon Horman <hidden> Date: 2021-08-03 11:37:12
On Tue, Aug 03, 2021 at 06:14:08AM -0400, Jamal Hadi Salim wrote:
On 2021-07-30 9:20 a.m., Simon Horman wrote:
quoted
On Fri, Jul 30, 2021 at 06:17:18AM -0400, Jamal Hadi Salim wrote:
quoted
On 2021-07-28 10:46 a.m., Simon Horman wrote:
[..]
quoted
quoted
It still not clear to me what it means from a command line pov.
How do i add a rule and when i dump it what does it show?
How about we confirm that once we've implemented the feature.
But I would assume that:
* Existing methods for adding rules work as before
* When one dumps an action (in a sufficiently verbose
way) the in_hw and in_hw_counter fields are displayed as they are for
filters.
Does that help?
I think it would help a lot more to say explicitly what it actually
means in the cover letter from a tc cli pov since the subject
is about offloading actions _independently_ of filters.
I am assuming you have some more patches on top of these that
actually will actually work for that.
Example of something you could show was adding a policer,
like so:
tc actions add action ... skip_sw...
then show get or dump showing things in h/w.
And del..
And i certainly hope that the above works and it is
not meant just for the consumption of some OVS use
case.
I agree it would be useful to include a tc cli example in the cover letter.
I'll see about making that so in v2.
From: Jamal Hadi Salim <jhs@mojatatu.com> Date: 2021-08-03 11:47:40
On 2021-08-03 7:36 a.m., Simon Horman wrote:
On Tue, Aug 03, 2021 at 06:14:08AM -0400, Jamal Hadi Salim wrote:
quoted
On 2021-07-30 9:20 a.m., Simon Horman wrote:
[..]
quoted
Example of something you could show was adding a policer,
like so:
tc actions add action ... skip_sw...
then show get or dump showing things in h/w.
And del..
And i certainly hope that the above works and it is
not meant just for the consumption of some OVS use
case.
I agree it would be useful to include a tc cli example in the cover letter.
I'll see about making that so in v2.
Thanks. That will remove 95% of my commentary.
Check my other comment on the user space notification.
I was just looking at the way classifiers handle things from this
perspective and they dont return to cls_api until completion so
the notification gets sent after both h/w and software succeed...
cheers,
jamal
From: Jamal Hadi Salim <jhs@mojatatu.com> Date: 2021-08-03 12:02:32
I just changed the subject line..
On 2021-08-03 5:57 a.m., Jamal Hadi Salim wrote:
On 2021-07-30 7:40 a.m., Vlad Buslov wrote:
quoted
On Fri 30 Jul 2021 at 13:17, Jamal Hadi Salim [off-list ref] wrote:
quoted
On 2021-07-28 10:46 a.m., Simon Horman wrote:
quoted
Filters with tunnel_key encap actions can be offloaded/unoffloaded
dynamically based on neigh state (see mlx5e_rep_neigh_update()) and fib
events (see mlx5e_tc_fib_event_work()).
Thanks. Will look and compare against the FIB case.
So unless i am mistaken Vlad:
a) there is no way to reflect the details when someone dumps the rules.
b) No notifications sent to the control plane (user space) when the
neighbor updates are offloaded.
My comments earlier are inspired by debugging tc offload and by this:
https://patches.linaro.org/cover/378345/
cheers,
jamal
On Tue 03 Aug 2021 at 15:02, Jamal Hadi Salim [off-list ref] wrote:
I just changed the subject line..
On 2021-08-03 5:57 a.m., Jamal Hadi Salim wrote:
quoted
On 2021-07-30 7:40 a.m., Vlad Buslov wrote:
quoted
On Fri 30 Jul 2021 at 13:17, Jamal Hadi Salim [off-list ref] wrote:
quoted
On 2021-07-28 10:46 a.m., Simon Horman wrote:
quoted
Filters with tunnel_key encap actions can be offloaded/unoffloaded
dynamically based on neigh state (see mlx5e_rep_neigh_update()) and fib
events (see mlx5e_tc_fib_event_work()).
Thanks. Will look and compare against the FIB case.
So unless i am mistaken Vlad:
a) there is no way to reflect the details when someone dumps the rules.
b) No notifications sent to the control plane (user space) when the
neighbor updates are offloaded.
From: Simon Horman <hidden> Date: 2021-08-03 12:32:09
On Tue, Aug 03, 2021 at 07:45:13AM -0400, Jamal Hadi Salim wrote:
On 2021-08-03 7:36 a.m., Simon Horman wrote:
quoted
On Tue, Aug 03, 2021 at 06:14:08AM -0400, Jamal Hadi Salim wrote:
quoted
On 2021-07-30 9:20 a.m., Simon Horman wrote:
[..]
quoted
quoted
Example of something you could show was adding a policer,
like so:
tc actions add action ... skip_sw...
then show get or dump showing things in h/w.
And del..
And i certainly hope that the above works and it is
not meant just for the consumption of some OVS use
case.
I agree it would be useful to include a tc cli example in the cover letter.
I'll see about making that so in v2.
Thanks. That will remove 95% of my commentary.
:)
Check my other comment on the user space notification.
I was just looking at the way classifiers handle things from this
perspective and they dont return to cls_api until completion so
the notification gets sent after both h/w and software succeed...
Thanks, I will look into this. But it would make my life slightly easier if
a) You could be more specific about what portion of cls_api you are
referring to.
b) Constrained comments to a topic to a single sub-thread.
From: Jamal Hadi Salim <jhs@mojatatu.com> Date: 2021-08-03 12:50:32
On 2021-08-03 8:14 a.m., Vlad Buslov wrote:
On Tue 03 Aug 2021 at 15:02, Jamal Hadi Salim [off-list ref] wrote:
[..]
quoted
So unless i am mistaken Vlad:
a) there is no way to reflect the details when someone dumps the rules.
b) No notifications sent to the control plane (user space) when the
neighbor updates are offloaded.
Correct.
Feels like we can adopt the same mechanics. Although, unless i am
misreading, it seems Ido's patches cover a slightly different use
case: not totally synchronous in successfully pushing the rule to
hardware i.e could be sitting somewhere in firmware on its way to
the ASIC (and at least your connectx driver seems to only be
relinquishing control after confirming the update succeeded).
Am i mistaken Ido?
cheers,
jamal
From: Jamal Hadi Salim <jhs@mojatatu.com> Date: 2021-08-03 13:01:50
On 2021-08-03 8:31 a.m., Simon Horman wrote:
On Tue, Aug 03, 2021 at 07:45:13AM -0400, Jamal Hadi Salim wrote:
Thanks, I will look into this. But it would make my life slightly easier if
a) You could be more specific about what portion of cls_api you are
referring to.
b) Constrained comments to a topic to a single sub-thread.
Context, this was on the comment i made on 2/3 here:
-----
- ret = tcf_del_notify(net, n, actions, portid, attr_size, extack);
+ tcf_action_offload_cmd_pre(actions, FLOW_ACT_DESTROY, extack,
&fl_act);
+ ret = tcf_del_notify(net, n, actions, portid, attr_size,
extack, &fallback_num);
+ tcf_action_offload_del_post(fl_act, actions, extack, fallback_num);
if (ret)
goto err;
----
where a notification goes to user space to say "success" but hardware
update fails.
If you look at fl_change() which does the offload you'll see that it
returns err on any of sw or hw failure (depending on request).
Notification of success is done in cls_api.c - example for
creating/replacing with this snippet:
---
err = tp->ops->change(net, skb, tp, cl, t->tcm_handle, tca, &fh,
flags, extack);
if (err == 0) {
tfilter_notify(net, skb, n, tp, block, q, parent, fh,
RTM_NEWTFILTER, false, rtnl_held);
tfilter_put(tp, fh);
/* q pointer is NULL for shared blocks */
if (q)
q->flags &= ~TCQ_F_CAN_BYPASS;
}
---
cheers,
jamal
On Tue, Aug 03, 2021 at 08:50:27AM -0400, Jamal Hadi Salim wrote:
On 2021-08-03 8:14 a.m., Vlad Buslov wrote:
quoted
On Tue 03 Aug 2021 at 15:02, Jamal Hadi Salim [off-list ref] wrote:
[..]
quoted
quoted
So unless i am mistaken Vlad:
a) there is no way to reflect the details when someone dumps the rules.
b) No notifications sent to the control plane (user space) when the
neighbor updates are offloaded.
Correct.
Feels like we can adopt the same mechanics. Although, unless i am
misreading, it seems Ido's patches cover a slightly different use
case: not totally synchronous in successfully pushing the rule to
hardware i.e could be sitting somewhere in firmware on its way to
the ASIC (and at least your connectx driver seems to only be
relinquishing control after confirming the update succeeded).
Am i mistaken Ido?
It is simply that all routes are notified from an atomic context, which
means installation to hardware needs to be deferred. But even if we
solve this case, there are other cases that cannot be solved.
For example, in IPv6 routes can be installed in response to RA packets
from softIRQ context. As another example, you can have several routes
with the same key already installed in the kernel, but only the one with
the lowest metric will be installed in hardware. Once it is deleted (for
example, in response to a netdev event), the kernel will try to install
the route with the higher metric. As a user, you want to get a
notification if this operation failed.
The same problem exists with tc actions, not sure about classifiers. For
example, we can mirror to a gretap to emulate ERSPAN. The hardware
expects all the headers to be specified, which means the path towards
the remote IP must be resolved in the kernel. Due to FIB/neigh/FDB
events it can obviously change, which means that the validity of the
action in hardware (not the classifier) changes over time with zero
visibility to user space.
So I believe that we need to be able to notify user space about the
state of the action. Whether it is in hardware / not in hardware /
failed to be installed to hardware.
From: Simon Horman <hidden> Date: 2021-08-03 14:47:59
On Tue, Aug 03, 2021 at 09:01:45AM -0400, Jamal Hadi Salim wrote:
On 2021-08-03 8:31 a.m., Simon Horman wrote:
quoted
On Tue, Aug 03, 2021 at 07:45:13AM -0400, Jamal Hadi Salim wrote:
quoted
Thanks, I will look into this. But it would make my life slightly easier if
a) You could be more specific about what portion of cls_api you are
referring to.
b) Constrained comments to a topic to a single sub-thread.
Context, this was on the comment i made on 2/3 here:
-----
- ret = tcf_del_notify(net, n, actions, portid, attr_size, extack);
+ tcf_action_offload_cmd_pre(actions, FLOW_ACT_DESTROY, extack,
&fl_act);
+ ret = tcf_del_notify(net, n, actions, portid, attr_size, extack,
&fallback_num);
+ tcf_action_offload_del_post(fl_act, actions, extack, fallback_num);
if (ret)
goto err;
----
where a notification goes to user space to say "success" but hardware
update fails.
If you look at fl_change() which does the offload you'll see that it
returns err on any of sw or hw failure (depending on request).
Notification of success is done in cls_api.c - example for
creating/replacing with this snippet:
---
err = tp->ops->change(net, skb, tp, cl, t->tcm_handle, tca, &fh,
flags, extack);
if (err == 0) {
tfilter_notify(net, skb, n, tp, block, q, parent, fh,
RTM_NEWTFILTER, false, rtnl_held);
tfilter_put(tp, fh);
/* q pointer is NULL for shared blocks */
if (q)
q->flags &= ~TCQ_F_CAN_BYPASS;
}
---