| Message ID | 20211118130805.23897-5-simon.horman@corigine.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | allow user to offload tc action to net device | expand |
On Thu 18 Nov 2021 at 15:07, Simon Horman <simon.horman@corigine.com> wrote: > From: Baowen Zheng <baowen.zheng@corigine.com> > > Use flow_indr_dev_register/flow_indr_dev_setup_offload to > offload tc action. > > We need to call tc_cleanup_flow_action to clean up tc action entry since > in tc_setup_action, some actions may hold dev refcnt, especially the mirror > action. > > Signed-off-by: Baowen Zheng <baowen.zheng@corigine.com> > Signed-off-by: Louis Peens <louis.peens@corigine.com> > Signed-off-by: Simon Horman <simon.horman@corigine.com> > --- > include/linux/netdevice.h | 1 + > include/net/flow_offload.h | 17 ++++ > include/net/pkt_cls.h | 12 +++ > net/core/flow_offload.c | 43 ++++++++-- > net/sched/act_api.c | 164 +++++++++++++++++++++++++++++++++++++ > net/sched/cls_api.c | 31 ++++++- > 6 files changed, 256 insertions(+), 12 deletions(-) > > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h > index 4f4a299e92de..ae189fcff3c6 100644 > --- a/include/linux/netdevice.h > +++ b/include/linux/netdevice.h > @@ -916,6 +916,7 @@ enum tc_setup_type { > TC_SETUP_QDISC_TBF, > TC_SETUP_QDISC_FIFO, > TC_SETUP_QDISC_HTB, > + TC_SETUP_ACT, > }; > > /* These structures hold the attributes of bpf state that are being passed > diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h > index f6970213497a..15662cad5bca 100644 > --- a/include/net/flow_offload.h > +++ b/include/net/flow_offload.h > @@ -551,6 +551,23 @@ struct flow_cls_offload { > u32 classid; > }; > > +enum flow_act_command { > + FLOW_ACT_REPLACE, > + FLOW_ACT_DESTROY, > + FLOW_ACT_STATS, > +}; > + > +struct flow_offload_action { > + struct netlink_ext_ack *extack; /* NULL in FLOW_ACT_STATS process*/ > + enum flow_act_command command; > + enum flow_action_id id; > + u32 index; > + struct flow_stats stats; > + struct flow_action action; > +}; > + > +struct flow_offload_action *flow_action_alloc(unsigned int num_actions); > + > static inline struct flow_rule * > flow_cls_offload_flow_rule(struct flow_cls_offload *flow_cmd) > { > diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h > index 193f88ebf629..14d098a887d0 100644 > --- a/include/net/pkt_cls.h > +++ b/include/net/pkt_cls.h > @@ -258,6 +258,14 @@ static inline void tcf_exts_put_net(struct tcf_exts *exts) > for (; 0; (void)(i), (void)(a), (void)(exts)) > #endif > > +#define tcf_act_for_each_action(i, a, actions) \ > + for (i = 0; i < TCA_ACT_MAX_PRIO && ((a) = actions[i]); i++) > + > +static inline bool tc_act_bind(u32 flags) > +{ > + return !!(flags & TCA_ACT_FLAGS_BIND); > +} > + > static inline void > tcf_exts_stats_update(const struct tcf_exts *exts, > u64 bytes, u64 packets, u64 drops, u64 lastuse, > @@ -534,6 +542,9 @@ tcf_match_indev(struct sk_buff *skb, int ifindex) > > int tc_setup_flow_action(struct flow_action *flow_action, > const struct tcf_exts *exts); > + > +int tc_setup_action(struct flow_action *flow_action, > + struct tc_action *actions[]); > void tc_cleanup_flow_action(struct flow_action *flow_action); > > int tc_setup_cb_call(struct tcf_block *block, enum tc_setup_type type, > @@ -554,6 +565,7 @@ int tc_setup_cb_reoffload(struct tcf_block *block, struct tcf_proto *tp, > enum tc_setup_type type, void *type_data, > void *cb_priv, u32 *flags, unsigned int *in_hw_count); > unsigned int tcf_exts_num_actions(struct tcf_exts *exts); > +unsigned int tcf_act_num_actions_single(struct tc_action *act); > > #ifdef CONFIG_NET_CLS_ACT > int tcf_qevent_init(struct tcf_qevent *qe, struct Qdisc *sch, > diff --git a/net/core/flow_offload.c b/net/core/flow_offload.c > index 6beaea13564a..6676431733ef 100644 > --- a/net/core/flow_offload.c > +++ b/net/core/flow_offload.c > @@ -27,6 +27,27 @@ struct flow_rule *flow_rule_alloc(unsigned int num_actions) > } > EXPORT_SYMBOL(flow_rule_alloc); > > +struct flow_offload_action *flow_action_alloc(unsigned int num_actions) > +{ > + struct flow_offload_action *fl_action; > + int i; > + > + fl_action = kzalloc(struct_size(fl_action, action.entries, num_actions), > + GFP_KERNEL); > + if (!fl_action) > + return NULL; > + > + fl_action->action.num_entries = num_actions; > + /* Pre-fill each action hw_stats with DONT_CARE. > + * Caller can override this if it wants stats for a given action. > + */ > + for (i = 0; i < num_actions; i++) > + fl_action->action.entries[i].hw_stats = FLOW_ACTION_HW_STATS_DONT_CARE; > + > + return fl_action; > +} > +EXPORT_SYMBOL(flow_action_alloc); > + > #define FLOW_DISSECTOR_MATCH(__rule, __type, __out) \ > const struct flow_match *__m = &(__rule)->match; \ > struct flow_dissector *__d = (__m)->dissector; \ > @@ -549,19 +570,25 @@ int flow_indr_dev_setup_offload(struct net_device *dev, struct Qdisc *sch, > void (*cleanup)(struct flow_block_cb *block_cb)) > { > struct flow_indr_dev *this; > + u32 count = 0; > + int err; > > mutex_lock(&flow_indr_block_lock); > + if (bo) { > + if (bo->command == FLOW_BLOCK_BIND) > + indir_dev_add(data, dev, sch, type, cleanup, bo); > + else if (bo->command == FLOW_BLOCK_UNBIND) > + indir_dev_remove(data); > + } > > - if (bo->command == FLOW_BLOCK_BIND) > - indir_dev_add(data, dev, sch, type, cleanup, bo); > - else if (bo->command == FLOW_BLOCK_UNBIND) > - indir_dev_remove(data); > - > - list_for_each_entry(this, &flow_block_indr_dev_list, list) > - this->cb(dev, sch, this->cb_priv, type, bo, data, cleanup); > + list_for_each_entry(this, &flow_block_indr_dev_list, list) { > + err = this->cb(dev, sch, this->cb_priv, type, bo, data, cleanup); > + if (!err) > + count++; > + } > > mutex_unlock(&flow_indr_block_lock); > > - return list_empty(&bo->cb_list) ? -EOPNOTSUPP : 0; > + return (bo && list_empty(&bo->cb_list)) ? -EOPNOTSUPP : count; > } > EXPORT_SYMBOL(flow_indr_dev_setup_offload); > diff --git a/net/sched/act_api.c b/net/sched/act_api.c > index 3258da3d5bed..c3d08b710661 100644 > --- a/net/sched/act_api.c > +++ b/net/sched/act_api.c > @@ -21,6 +21,19 @@ > #include <net/pkt_cls.h> > #include <net/act_api.h> > #include <net/netlink.h> > +#include <net/tc_act/tc_pedit.h> > +#include <net/tc_act/tc_mirred.h> > +#include <net/tc_act/tc_vlan.h> > +#include <net/tc_act/tc_tunnel_key.h> > +#include <net/tc_act/tc_csum.h> > +#include <net/tc_act/tc_gact.h> > +#include <net/tc_act/tc_police.h> > +#include <net/tc_act/tc_sample.h> > +#include <net/tc_act/tc_skbedit.h> > +#include <net/tc_act/tc_ct.h> > +#include <net/tc_act/tc_mpls.h> > +#include <net/tc_act/tc_gate.h> > +#include <net/flow_offload.h> > > #ifdef CONFIG_INET > DEFINE_STATIC_KEY_FALSE(tcf_frag_xmit_count); > @@ -129,8 +142,157 @@ static void free_tcf(struct tc_action *p) > kfree(p); > } > > +static int flow_action_init(struct flow_offload_action *fl_action, > + struct tc_action *act, > + enum flow_act_command cmd, > + struct netlink_ext_ack *extack) > +{ > + if (!fl_action) > + return -EINVAL; Multiple functions in this patch verify action argument that seemingly can't be NULL neither in this change nor in the following ones in series. Any particular motivation for this? > + > + fl_action->extack = extack; > + fl_action->command = cmd; > + fl_action->index = act->tcfa_index; > + > + if (is_tcf_gact_ok(act)) { > + fl_action->id = FLOW_ACTION_ACCEPT; > + } else if (is_tcf_gact_shot(act)) { > + fl_action->id = FLOW_ACTION_DROP; > + } else if (is_tcf_gact_trap(act)) { > + fl_action->id = FLOW_ACTION_TRAP; > + } else if (is_tcf_gact_goto_chain(act)) { > + fl_action->id = FLOW_ACTION_GOTO; > + } else if (is_tcf_mirred_egress_redirect(act)) { > + fl_action->id = FLOW_ACTION_REDIRECT; > + } else if (is_tcf_mirred_egress_mirror(act)) { > + fl_action->id = FLOW_ACTION_MIRRED; > + } else if (is_tcf_mirred_ingress_redirect(act)) { > + fl_action->id = FLOW_ACTION_REDIRECT_INGRESS; > + } else if (is_tcf_mirred_ingress_mirror(act)) { > + fl_action->id = FLOW_ACTION_MIRRED_INGRESS; > + } else if (is_tcf_vlan(act)) { > + switch (tcf_vlan_action(act)) { > + case TCA_VLAN_ACT_PUSH: > + fl_action->id = FLOW_ACTION_VLAN_PUSH; > + break; > + case TCA_VLAN_ACT_POP: > + fl_action->id = FLOW_ACTION_VLAN_POP; > + break; > + case TCA_VLAN_ACT_MODIFY: > + fl_action->id = FLOW_ACTION_VLAN_MANGLE; > + break; > + default: > + return -EOPNOTSUPP; > + } > + } else if (is_tcf_tunnel_set(act)) { > + fl_action->id = FLOW_ACTION_TUNNEL_ENCAP; > + } else if (is_tcf_tunnel_release(act)) { > + fl_action->id = FLOW_ACTION_TUNNEL_DECAP; > + } else if (is_tcf_csum(act)) { > + fl_action->id = FLOW_ACTION_CSUM; > + } else if (is_tcf_skbedit_mark(act)) { > + fl_action->id = FLOW_ACTION_MARK; > + } else if (is_tcf_sample(act)) { > + fl_action->id = FLOW_ACTION_SAMPLE; > + } else if (is_tcf_police(act)) { > + fl_action->id = FLOW_ACTION_POLICE; > + } else if (is_tcf_ct(act)) { > + fl_action->id = FLOW_ACTION_CT; > + } else if (is_tcf_mpls(act)) { > + switch (tcf_mpls_action(act)) { > + case TCA_MPLS_ACT_PUSH: > + fl_action->id = FLOW_ACTION_MPLS_PUSH; > + break; > + case TCA_MPLS_ACT_POP: > + fl_action->id = FLOW_ACTION_MPLS_POP; > + break; > + case TCA_MPLS_ACT_MODIFY: > + fl_action->id = FLOW_ACTION_MPLS_MANGLE; > + break; > + default: > + return -EOPNOTSUPP; > + } > + } else if (is_tcf_skbedit_ptype(act)) { > + fl_action->id = FLOW_ACTION_PTYPE; > + } else if (is_tcf_skbedit_priority(act)) { > + fl_action->id = FLOW_ACTION_PRIORITY; > + } else if (is_tcf_gate(act)) { > + fl_action->id = FLOW_ACTION_GATE; > + } else { > + return -EOPNOTSUPP; > + } > + > + return 0; > +} > + > +static int tcf_action_offload_cmd(struct flow_offload_action *fl_act, > + struct netlink_ext_ack *extack) > +{ > + int err; > + > + if (IS_ERR(fl_act)) > + return PTR_ERR(fl_act); Ditto. > + > + err = flow_indr_dev_setup_offload(NULL, NULL, TC_SETUP_ACT, > + fl_act, NULL, NULL); > + if (err < 0) > + return err; > + > + return 0; > +} > + > +/* offload the tc command after inserted */ > +static int tcf_action_offload_add(struct tc_action *action, > + struct netlink_ext_ack *extack) > +{ > + struct tc_action *actions[TCA_ACT_MAX_PRIO] = { > + [0] = action, > + }; > + struct flow_offload_action *fl_action; > + int err = 0; > + > + fl_action = flow_action_alloc(tcf_act_num_actions_single(action)); > + if (!fl_action) > + return -ENOMEM; > + > + err = flow_action_init(fl_action, action, FLOW_ACT_REPLACE, extack); > + if (err) > + goto fl_err; > + > + err = tc_setup_action(&fl_action->action, actions); > + if (err) { > + NL_SET_ERR_MSG_MOD(extack, > + "Failed to setup tc actions for offload\n"); > + goto fl_err; > + } > + > + err = tcf_action_offload_cmd(fl_action, extack); > + tc_cleanup_flow_action(&fl_action->action); > + > +fl_err: > + kfree(fl_action); > + > + return err; > +} > + > +static int tcf_action_offload_del(struct tc_action *action) > +{ > + struct flow_offload_action fl_act; This is the only usage of uninitialized flow_offload_action instance. Since there is no reference driver implementation, it is not clear to me whether this is intentional because flow_action_init() is guaranteed to initialize all data that is necessary for delete, or just an omission. > + int err = 0; > + > + if (!action) > + return -EINVAL; Seemingly redundant check again. > + > + err = flow_action_init(&fl_act, action, FLOW_ACT_DESTROY, NULL); > + if (err) > + return err; > + > + return tcf_action_offload_cmd(&fl_act, NULL); > +} > + > static void tcf_action_cleanup(struct tc_action *p) > { > + tcf_action_offload_del(p); > if (p->ops->cleanup) > p->ops->cleanup(p); > > @@ -1103,6 +1265,8 @@ int tcf_action_init(struct net *net, struct tcf_proto *tp, struct nlattr *nla, > sz += tcf_action_fill_size(act); > /* Start from index 0 */ > actions[i - 1] = act; > + if (!tc_act_bind(flags)) > + tcf_action_offload_add(act, extack); > } > > /* We have to commit them all together, because if any error happened in > diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c > index d9d6ff0bf361..55fa48999d43 100644 > --- a/net/sched/cls_api.c > +++ b/net/sched/cls_api.c > @@ -3544,8 +3544,8 @@ static enum flow_action_hw_stats tc_act_hw_stats(u8 hw_stats) > return hw_stats; > } > > -int tc_setup_flow_action(struct flow_action *flow_action, > - const struct tcf_exts *exts) > +int tc_setup_action(struct flow_action *flow_action, > + struct tc_action *actions[]) > { > struct tc_action *act; > int i, j, k, err = 0; > @@ -3554,11 +3554,11 @@ int tc_setup_flow_action(struct flow_action *flow_action, > BUILD_BUG_ON(TCA_ACT_HW_STATS_IMMEDIATE != FLOW_ACTION_HW_STATS_IMMEDIATE); > BUILD_BUG_ON(TCA_ACT_HW_STATS_DELAYED != FLOW_ACTION_HW_STATS_DELAYED); > > - if (!exts) > + if (!actions) > return 0; > > j = 0; > - tcf_exts_for_each_action(i, act, exts) { > + tcf_act_for_each_action(i, act, actions) { > struct flow_action_entry *entry; > > entry = &flow_action->entries[j]; > @@ -3724,6 +3724,20 @@ int tc_setup_flow_action(struct flow_action *flow_action, > spin_unlock_bh(&act->tcfa_lock); > goto err_out; > } > +EXPORT_SYMBOL(tc_setup_action); > + > +int tc_setup_flow_action(struct flow_action *flow_action, > + const struct tcf_exts *exts) > +{ > +#ifdef CONFIG_NET_CLS_ACT > + if (!exts) > + return 0; > + > + return tc_setup_action(flow_action, exts->actions); > +#else > + return 0; > +#endif > +} > EXPORT_SYMBOL(tc_setup_flow_action); > > unsigned int tcf_exts_num_actions(struct tcf_exts *exts) > @@ -3742,6 +3756,15 @@ unsigned int tcf_exts_num_actions(struct tcf_exts *exts) > } > EXPORT_SYMBOL(tcf_exts_num_actions); > > +unsigned int tcf_act_num_actions_single(struct tc_action *act) > +{ > + if (is_tcf_pedit(act)) > + return tcf_pedit_nkeys(act); > + else > + return 1; > +} > +EXPORT_SYMBOL(tcf_act_num_actions_single); > + > #ifdef CONFIG_NET_CLS_ACT > static int tcf_qevent_parse_block_index(struct nlattr *block_index_attr, > u32 *p_block_index,
On November 20, 2021 3:06 AM, Vlad Buslov wrote: >On Thu 18 Nov 2021 at 15:07, Simon Horman <simon.horman@corigine.com> >wrote: >> From: Baowen Zheng <baowen.zheng@corigine.com> >> >> Use flow_indr_dev_register/flow_indr_dev_setup_offload to offload tc >> action. >> >> We need to call tc_cleanup_flow_action to clean up tc action entry >> since in tc_setup_action, some actions may hold dev refcnt, especially >> the mirror action. >> >> Signed-off-by: Baowen Zheng <baowen.zheng@corigine.com> >> Signed-off-by: Louis Peens <louis.peens@corigine.com> >> Signed-off-by: Simon Horman <simon.horman@corigine.com> >> --- >> include/linux/netdevice.h | 1 + >> include/net/flow_offload.h | 17 ++++ >> include/net/pkt_cls.h | 12 +++ >> net/core/flow_offload.c | 43 ++++++++-- >> net/sched/act_api.c | 164 +++++++++++++++++++++++++++++++++++++ >> net/sched/cls_api.c | 31 ++++++- >> 6 files changed, 256 insertions(+), 12 deletions(-) >> >> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h >> index 4f4a299e92de..ae189fcff3c6 100644 >> --- a/include/linux/netdevice.h >> +++ b/include/linux/netdevice.h >> @@ -916,6 +916,7 @@ enum tc_setup_type { >> TC_SETUP_QDISC_TBF, >> TC_SETUP_QDISC_FIFO, >> TC_SETUP_QDISC_HTB, >> + TC_SETUP_ACT, >> }; >> >> /* These structures hold the attributes of bpf state that are being >> passed diff --git a/include/net/flow_offload.h >> b/include/net/flow_offload.h index f6970213497a..15662cad5bca 100644 >> --- a/include/net/flow_offload.h >> +++ b/include/net/flow_offload.h >> @@ -551,6 +551,23 @@ struct flow_cls_offload { >> u32 classid; >> }; >> >> +enum flow_act_command { >> + FLOW_ACT_REPLACE, >> + FLOW_ACT_DESTROY, >> + FLOW_ACT_STATS, >> +}; >> + >> +struct flow_offload_action { >> + struct netlink_ext_ack *extack; /* NULL in FLOW_ACT_STATS >process*/ >> + enum flow_act_command command; >> + enum flow_action_id id; >> + u32 index; >> + struct flow_stats stats; >> + struct flow_action action; >> +}; >> + >> +struct flow_offload_action *flow_action_alloc(unsigned int >> +num_actions); >> + >> static inline struct flow_rule * >> flow_cls_offload_flow_rule(struct flow_cls_offload *flow_cmd) { diff >> --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h index >> 193f88ebf629..14d098a887d0 100644 >> --- a/include/net/pkt_cls.h >> +++ b/include/net/pkt_cls.h >> @@ -258,6 +258,14 @@ static inline void tcf_exts_put_net(struct tcf_exts >*exts) >> for (; 0; (void)(i), (void)(a), (void)(exts)) #endif >> >> +#define tcf_act_for_each_action(i, a, actions) \ >> + for (i = 0; i < TCA_ACT_MAX_PRIO && ((a) = actions[i]); i++) >> + >> +static inline bool tc_act_bind(u32 flags) { >> + return !!(flags & TCA_ACT_FLAGS_BIND); } >> + >> static inline void >> tcf_exts_stats_update(const struct tcf_exts *exts, >> u64 bytes, u64 packets, u64 drops, u64 lastuse, @@ -534,6 >> +542,9 @@ tcf_match_indev(struct sk_buff *skb, int ifindex) >> >> int tc_setup_flow_action(struct flow_action *flow_action, >> const struct tcf_exts *exts); >> + >> +int tc_setup_action(struct flow_action *flow_action, >> + struct tc_action *actions[]); >> void tc_cleanup_flow_action(struct flow_action *flow_action); >> >> int tc_setup_cb_call(struct tcf_block *block, enum tc_setup_type >> type, @@ -554,6 +565,7 @@ int tc_setup_cb_reoffload(struct tcf_block >*block, struct tcf_proto *tp, >> enum tc_setup_type type, void *type_data, >> void *cb_priv, u32 *flags, unsigned int >*in_hw_count); unsigned >> int tcf_exts_num_actions(struct tcf_exts *exts); >> +unsigned int tcf_act_num_actions_single(struct tc_action *act); >> >> #ifdef CONFIG_NET_CLS_ACT >> int tcf_qevent_init(struct tcf_qevent *qe, struct Qdisc *sch, diff >> --git a/net/core/flow_offload.c b/net/core/flow_offload.c index >> 6beaea13564a..6676431733ef 100644 >> --- a/net/core/flow_offload.c >> +++ b/net/core/flow_offload.c >> @@ -27,6 +27,27 @@ struct flow_rule *flow_rule_alloc(unsigned int >> num_actions) } EXPORT_SYMBOL(flow_rule_alloc); >> >> +struct flow_offload_action *flow_action_alloc(unsigned int >> +num_actions) { >> + struct flow_offload_action *fl_action; >> + int i; >> + >> + fl_action = kzalloc(struct_size(fl_action, action.entries, num_actions), >> + GFP_KERNEL); >> + if (!fl_action) >> + return NULL; >> + >> + fl_action->action.num_entries = num_actions; >> + /* Pre-fill each action hw_stats with DONT_CARE. >> + * Caller can override this if it wants stats for a given action. >> + */ >> + for (i = 0; i < num_actions; i++) >> + fl_action->action.entries[i].hw_stats = >> +FLOW_ACTION_HW_STATS_DONT_CARE; >> + >> + return fl_action; >> +} >> +EXPORT_SYMBOL(flow_action_alloc); >> + >> #define FLOW_DISSECTOR_MATCH(__rule, __type, __out) > \ >> const struct flow_match *__m = &(__rule)->match; > \ >> struct flow_dissector *__d = (__m)->dissector; > \ >> @@ -549,19 +570,25 @@ int flow_indr_dev_setup_offload(struct >net_device *dev, struct Qdisc *sch, >> void (*cleanup)(struct flow_block_cb >*block_cb)) { >> struct flow_indr_dev *this; >> + u32 count = 0; >> + int err; >> >> mutex_lock(&flow_indr_block_lock); >> + if (bo) { >> + if (bo->command == FLOW_BLOCK_BIND) >> + indir_dev_add(data, dev, sch, type, cleanup, bo); >> + else if (bo->command == FLOW_BLOCK_UNBIND) >> + indir_dev_remove(data); >> + } >> >> - if (bo->command == FLOW_BLOCK_BIND) >> - indir_dev_add(data, dev, sch, type, cleanup, bo); >> - else if (bo->command == FLOW_BLOCK_UNBIND) >> - indir_dev_remove(data); >> - >> - list_for_each_entry(this, &flow_block_indr_dev_list, list) >> - this->cb(dev, sch, this->cb_priv, type, bo, data, cleanup); >> + list_for_each_entry(this, &flow_block_indr_dev_list, list) { >> + err = this->cb(dev, sch, this->cb_priv, type, bo, data, cleanup); >> + if (!err) >> + count++; >> + } >> >> mutex_unlock(&flow_indr_block_lock); >> >> - return list_empty(&bo->cb_list) ? -EOPNOTSUPP : 0; >> + return (bo && list_empty(&bo->cb_list)) ? -EOPNOTSUPP : count; >> } >> EXPORT_SYMBOL(flow_indr_dev_setup_offload); >> diff --git a/net/sched/act_api.c b/net/sched/act_api.c index >> 3258da3d5bed..c3d08b710661 100644 >> --- a/net/sched/act_api.c >> +++ b/net/sched/act_api.c >> @@ -21,6 +21,19 @@ >> #include <net/pkt_cls.h> >> #include <net/act_api.h> >> #include <net/netlink.h> >> +#include <net/tc_act/tc_pedit.h> >> +#include <net/tc_act/tc_mirred.h> >> +#include <net/tc_act/tc_vlan.h> >> +#include <net/tc_act/tc_tunnel_key.h> #include <net/tc_act/tc_csum.h> >> +#include <net/tc_act/tc_gact.h> #include <net/tc_act/tc_police.h> >> +#include <net/tc_act/tc_sample.h> #include <net/tc_act/tc_skbedit.h> >> +#include <net/tc_act/tc_ct.h> #include <net/tc_act/tc_mpls.h> >> +#include <net/tc_act/tc_gate.h> #include <net/flow_offload.h> >> >> #ifdef CONFIG_INET >> DEFINE_STATIC_KEY_FALSE(tcf_frag_xmit_count); >> @@ -129,8 +142,157 @@ static void free_tcf(struct tc_action *p) >> kfree(p); >> } >> >> +static int flow_action_init(struct flow_offload_action *fl_action, >> + struct tc_action *act, >> + enum flow_act_command cmd, >> + struct netlink_ext_ack *extack) { >> + if (!fl_action) >> + return -EINVAL; > >Multiple functions in this patch verify action argument that seemingly can't be >NULL neither in this change nor in the following ones in series. Any particular >motivation for this? > We will make a check and delete this check if it is redundant. >> + >> + fl_action->extack = extack; >> + fl_action->command = cmd; >> + fl_action->index = act->tcfa_index; >> + >> + if (is_tcf_gact_ok(act)) { >> + fl_action->id = FLOW_ACTION_ACCEPT; >> + } else if (is_tcf_gact_shot(act)) { >> + fl_action->id = FLOW_ACTION_DROP; >> + } else if (is_tcf_gact_trap(act)) { >> + fl_action->id = FLOW_ACTION_TRAP; >> + } else if (is_tcf_gact_goto_chain(act)) { >> + fl_action->id = FLOW_ACTION_GOTO; >> + } else if (is_tcf_mirred_egress_redirect(act)) { >> + fl_action->id = FLOW_ACTION_REDIRECT; >> + } else if (is_tcf_mirred_egress_mirror(act)) { >> + fl_action->id = FLOW_ACTION_MIRRED; >> + } else if (is_tcf_mirred_ingress_redirect(act)) { >> + fl_action->id = FLOW_ACTION_REDIRECT_INGRESS; >> + } else if (is_tcf_mirred_ingress_mirror(act)) { >> + fl_action->id = FLOW_ACTION_MIRRED_INGRESS; >> + } else if (is_tcf_vlan(act)) { >> + switch (tcf_vlan_action(act)) { >> + case TCA_VLAN_ACT_PUSH: >> + fl_action->id = FLOW_ACTION_VLAN_PUSH; >> + break; >> + case TCA_VLAN_ACT_POP: >> + fl_action->id = FLOW_ACTION_VLAN_POP; >> + break; >> + case TCA_VLAN_ACT_MODIFY: >> + fl_action->id = FLOW_ACTION_VLAN_MANGLE; >> + break; >> + default: >> + return -EOPNOTSUPP; >> + } >> + } else if (is_tcf_tunnel_set(act)) { >> + fl_action->id = FLOW_ACTION_TUNNEL_ENCAP; >> + } else if (is_tcf_tunnel_release(act)) { >> + fl_action->id = FLOW_ACTION_TUNNEL_DECAP; >> + } else if (is_tcf_csum(act)) { >> + fl_action->id = FLOW_ACTION_CSUM; >> + } else if (is_tcf_skbedit_mark(act)) { >> + fl_action->id = FLOW_ACTION_MARK; >> + } else if (is_tcf_sample(act)) { >> + fl_action->id = FLOW_ACTION_SAMPLE; >> + } else if (is_tcf_police(act)) { >> + fl_action->id = FLOW_ACTION_POLICE; >> + } else if (is_tcf_ct(act)) { >> + fl_action->id = FLOW_ACTION_CT; >> + } else if (is_tcf_mpls(act)) { >> + switch (tcf_mpls_action(act)) { >> + case TCA_MPLS_ACT_PUSH: >> + fl_action->id = FLOW_ACTION_MPLS_PUSH; >> + break; >> + case TCA_MPLS_ACT_POP: >> + fl_action->id = FLOW_ACTION_MPLS_POP; >> + break; >> + case TCA_MPLS_ACT_MODIFY: >> + fl_action->id = FLOW_ACTION_MPLS_MANGLE; >> + break; >> + default: >> + return -EOPNOTSUPP; >> + } >> + } else if (is_tcf_skbedit_ptype(act)) { >> + fl_action->id = FLOW_ACTION_PTYPE; >> + } else if (is_tcf_skbedit_priority(act)) { >> + fl_action->id = FLOW_ACTION_PRIORITY; >> + } else if (is_tcf_gate(act)) { >> + fl_action->id = FLOW_ACTION_GATE; >> + } else { >> + return -EOPNOTSUPP; >> + } >> + >> + return 0; >> +} >> + >> +static int tcf_action_offload_cmd(struct flow_offload_action *fl_act, >> + struct netlink_ext_ack *extack) { >> + int err; >> + >> + if (IS_ERR(fl_act)) >> + return PTR_ERR(fl_act); > >Ditto. > We will make a check and delete this check if it is redundant. >> + >> + err = flow_indr_dev_setup_offload(NULL, NULL, TC_SETUP_ACT, >> + fl_act, NULL, NULL); >> + if (err < 0) >> + return err; >> + >> + return 0; >> +} >> + >> +/* offload the tc command after inserted */ static int >> +tcf_action_offload_add(struct tc_action *action, >> + struct netlink_ext_ack *extack) { >> + struct tc_action *actions[TCA_ACT_MAX_PRIO] = { >> + [0] = action, >> + }; >> + struct flow_offload_action *fl_action; >> + int err = 0; >> + >> + fl_action = flow_action_alloc(tcf_act_num_actions_single(action)); >> + if (!fl_action) >> + return -ENOMEM; >> + >> + err = flow_action_init(fl_action, action, FLOW_ACT_REPLACE, extack); >> + if (err) >> + goto fl_err; >> + >> + err = tc_setup_action(&fl_action->action, actions); >> + if (err) { >> + NL_SET_ERR_MSG_MOD(extack, >> + "Failed to setup tc actions for offload\n"); >> + goto fl_err; >> + } >> + >> + err = tcf_action_offload_cmd(fl_action, extack); >> + tc_cleanup_flow_action(&fl_action->action); >> + >> +fl_err: >> + kfree(fl_action); >> + >> + return err; >> +} >> + >> +static int tcf_action_offload_del(struct tc_action *action) { >> + struct flow_offload_action fl_act; > >This is the only usage of uninitialized flow_offload_action instance. >Since there is no reference driver implementation, it is not clear to me >whether this is intentional because flow_action_init() is guaranteed to >initialize all data that is necessary for delete, or just an omission. > We will add the initialization. >> + int err = 0; >> + >> + if (!action) >> + return -EINVAL; > >Seemingly redundant check again. > Will delete the redundant check. >> + >> + err = flow_action_init(&fl_act, action, FLOW_ACT_DESTROY, NULL); >> + if (err) >> + return err; >> + >> + return tcf_action_offload_cmd(&fl_act, NULL); } >> + >> static void tcf_action_cleanup(struct tc_action *p) { >> + tcf_action_offload_del(p); >> if (p->ops->cleanup) >> p->ops->cleanup(p); >> >> @@ -1103,6 +1265,8 @@ int tcf_action_init(struct net *net, struct tcf_proto >*tp, struct nlattr *nla, >> sz += tcf_action_fill_size(act); >> /* Start from index 0 */ >> actions[i - 1] = act; >> + if (!tc_act_bind(flags)) >> + tcf_action_offload_add(act, extack); >> } >> >> /* We have to commit them all together, because if any error >> happened in diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c >> index d9d6ff0bf361..55fa48999d43 100644 >> --- a/net/sched/cls_api.c >> +++ b/net/sched/cls_api.c >> @@ -3544,8 +3544,8 @@ static enum flow_action_hw_stats >tc_act_hw_stats(u8 hw_stats) >> return hw_stats; >> } >> >> -int tc_setup_flow_action(struct flow_action *flow_action, >> - const struct tcf_exts *exts) >> +int tc_setup_action(struct flow_action *flow_action, >> + struct tc_action *actions[]) >> { >> struct tc_action *act; >> int i, j, k, err = 0; >> @@ -3554,11 +3554,11 @@ int tc_setup_flow_action(struct flow_action >*flow_action, >> BUILD_BUG_ON(TCA_ACT_HW_STATS_IMMEDIATE != >FLOW_ACTION_HW_STATS_IMMEDIATE); >> BUILD_BUG_ON(TCA_ACT_HW_STATS_DELAYED != >> FLOW_ACTION_HW_STATS_DELAYED); >> >> - if (!exts) >> + if (!actions) >> return 0; >> >> j = 0; >> - tcf_exts_for_each_action(i, act, exts) { >> + tcf_act_for_each_action(i, act, actions) { >> struct flow_action_entry *entry; >> >> entry = &flow_action->entries[j]; >> @@ -3724,6 +3724,20 @@ int tc_setup_flow_action(struct flow_action >*flow_action, >> spin_unlock_bh(&act->tcfa_lock); >> goto err_out; >> } >> +EXPORT_SYMBOL(tc_setup_action); >> + >> +int tc_setup_flow_action(struct flow_action *flow_action, >> + const struct tcf_exts *exts) >> +{ >> +#ifdef CONFIG_NET_CLS_ACT >> + if (!exts) >> + return 0; >> + >> + return tc_setup_action(flow_action, exts->actions); #else >> + return 0; >> +#endif >> +} >> EXPORT_SYMBOL(tc_setup_flow_action); >> >> unsigned int tcf_exts_num_actions(struct tcf_exts *exts) @@ -3742,6 >> +3756,15 @@ unsigned int tcf_exts_num_actions(struct tcf_exts *exts) >> } EXPORT_SYMBOL(tcf_exts_num_actions); >> >> +unsigned int tcf_act_num_actions_single(struct tc_action *act) { >> + if (is_tcf_pedit(act)) >> + return tcf_pedit_nkeys(act); >> + else >> + return 1; >> +} >> +EXPORT_SYMBOL(tcf_act_num_actions_single); >> + >> #ifdef CONFIG_NET_CLS_ACT >> static int tcf_qevent_parse_block_index(struct nlattr *block_index_attr, >> u32 *p_block_index,
On 2021-11-18 08:07, Simon Horman wrote: [..] > --- a/net/sched/act_api.c > +++ b/net/sched/act_api.c > @@ -21,6 +21,19 @@ > +#include <net/tc_act/tc_pedit.h> > +#include <net/tc_act/tc_mirred.h> > +#include <net/tc_act/tc_vlan.h> > +#include <net/tc_act/tc_tunnel_key.h> > +#include <net/tc_act/tc_csum.h> > +#include <net/tc_act/tc_gact.h> > +#include <net/tc_act/tc_police.h> > +#include <net/tc_act/tc_sample.h> > +#include <net/tc_act/tc_skbedit.h> > +#include <net/tc_act/tc_ct.h> > +#include <net/tc_act/tc_mpls.h> > +#include <net/tc_act/tc_gate.h> > +#include <net/flow_offload.h> > > #ifdef CONFIG_INET > DEFINE_STATIC_KEY_FALSE(tcf_frag_xmit_count); > @@ -129,8 +142,157 @@ static void free_tcf(struct tc_action *p) > kfree(p); > } > > +static int flow_action_init(struct flow_offload_action *fl_action, > + struct tc_action *act, > + enum flow_act_command cmd, > + struct netlink_ext_ack *extack) > +{ > + if (!fl_action) > + return -EINVAL; > + > + fl_action->extack = extack; > + fl_action->command = cmd; > + fl_action->index = act->tcfa_index; > + > + if (is_tcf_gact_ok(act)) { > + fl_action->id = FLOW_ACTION_ACCEPT; > + } else if (is_tcf_gact_shot(act)) { > + fl_action->id = FLOW_ACTION_DROP; > + } else if (is_tcf_gact_trap(act)) { > + fl_action->id = FLOW_ACTION_TRAP; > + } else if (is_tcf_gact_goto_chain(act)) { > + fl_action->id = FLOW_ACTION_GOTO; > + } else if (is_tcf_mirred_egress_redirect(act)) { > + fl_action->id = FLOW_ACTION_REDIRECT; > + } else if (is_tcf_mirred_egress_mirror(act)) { > + fl_action->id = FLOW_ACTION_MIRRED; > + } else if (is_tcf_mirred_ingress_redirect(act)) { > + fl_action->id = FLOW_ACTION_REDIRECT_INGRESS; > + } else if (is_tcf_mirred_ingress_mirror(act)) { > + fl_action->id = FLOW_ACTION_MIRRED_INGRESS; > + } else if (is_tcf_vlan(act)) { > + switch (tcf_vlan_action(act)) { > + case TCA_VLAN_ACT_PUSH: > + fl_action->id = FLOW_ACTION_VLAN_PUSH; > + break; > + case TCA_VLAN_ACT_POP: > + fl_action->id = FLOW_ACTION_VLAN_POP; > + break; > + case TCA_VLAN_ACT_MODIFY: > + fl_action->id = FLOW_ACTION_VLAN_MANGLE; > + break; > + default: > + return -EOPNOTSUPP; > + } > + } else if (is_tcf_tunnel_set(act)) { > + fl_action->id = FLOW_ACTION_TUNNEL_ENCAP; > + } else if (is_tcf_tunnel_release(act)) { > + fl_action->id = FLOW_ACTION_TUNNEL_DECAP; > + } else if (is_tcf_csum(act)) { > + fl_action->id = FLOW_ACTION_CSUM; > + } else if (is_tcf_skbedit_mark(act)) { > + fl_action->id = FLOW_ACTION_MARK; > + } else if (is_tcf_sample(act)) { > + fl_action->id = FLOW_ACTION_SAMPLE; > + } else if (is_tcf_police(act)) { > + fl_action->id = FLOW_ACTION_POLICE; > + } else if (is_tcf_ct(act)) { > + fl_action->id = FLOW_ACTION_CT; > + } else if (is_tcf_mpls(act)) { > + switch (tcf_mpls_action(act)) { > + case TCA_MPLS_ACT_PUSH: > + fl_action->id = FLOW_ACTION_MPLS_PUSH; > + break; > + case TCA_MPLS_ACT_POP: > + fl_action->id = FLOW_ACTION_MPLS_POP; > + break; > + case TCA_MPLS_ACT_MODIFY: > + fl_action->id = FLOW_ACTION_MPLS_MANGLE; > + break; > + default: > + return -EOPNOTSUPP; > + } > + } else if (is_tcf_skbedit_ptype(act)) { > + fl_action->id = FLOW_ACTION_PTYPE; > + } else if (is_tcf_skbedit_priority(act)) { > + fl_action->id = FLOW_ACTION_PRIORITY; > + } else if (is_tcf_gate(act)) { > + fl_action->id = FLOW_ACTION_GATE; > + } else { > + return -EOPNOTSUPP; > + } > + > + return 0; > +} > + The challenge with this is now it is impossible to write an action as a standalone module (which works today). One resolution to this is to either reuse or introduce a new ops in struct tc_action_ops. Then flow_action_init() would just invoke this act->ops() which will do action specific setup. cheers, jamal
On November 22, 2021 8:25 PM, Jamal Hadi Salim wrote: >On 2021-11-18 08:07, Simon Horman wrote: > >[..] > > > --- a/net/sched/act_api.c > > +++ b/net/sched/act_api.c > > @@ -21,6 +21,19 @@ >> +#include <net/tc_act/tc_pedit.h> >> +#include <net/tc_act/tc_mirred.h> >> +#include <net/tc_act/tc_vlan.h> >> +#include <net/tc_act/tc_tunnel_key.h> #include <net/tc_act/tc_csum.h> >> +#include <net/tc_act/tc_gact.h> #include <net/tc_act/tc_police.h> >> +#include <net/tc_act/tc_sample.h> #include <net/tc_act/tc_skbedit.h> >> +#include <net/tc_act/tc_ct.h> #include <net/tc_act/tc_mpls.h> >> +#include <net/tc_act/tc_gate.h> #include <net/flow_offload.h> >> >> #ifdef CONFIG_INET >> DEFINE_STATIC_KEY_FALSE(tcf_frag_xmit_count); >> @@ -129,8 +142,157 @@ static void free_tcf(struct tc_action *p) >> kfree(p); >> } >> >> +static int flow_action_init(struct flow_offload_action *fl_action, >> + struct tc_action *act, >> + enum flow_act_command cmd, >> + struct netlink_ext_ack *extack) { >> + if (!fl_action) >> + return -EINVAL; >> + >> + fl_action->extack = extack; >> + fl_action->command = cmd; >> + fl_action->index = act->tcfa_index; >> + >> + if (is_tcf_gact_ok(act)) { >> + fl_action->id = FLOW_ACTION_ACCEPT; >> + } else if (is_tcf_gact_shot(act)) { >> + fl_action->id = FLOW_ACTION_DROP; >> + } else if (is_tcf_gact_trap(act)) { >> + fl_action->id = FLOW_ACTION_TRAP; >> + } else if (is_tcf_gact_goto_chain(act)) { >> + fl_action->id = FLOW_ACTION_GOTO; >> + } else if (is_tcf_mirred_egress_redirect(act)) { >> + fl_action->id = FLOW_ACTION_REDIRECT; >> + } else if (is_tcf_mirred_egress_mirror(act)) { >> + fl_action->id = FLOW_ACTION_MIRRED; >> + } else if (is_tcf_mirred_ingress_redirect(act)) { >> + fl_action->id = FLOW_ACTION_REDIRECT_INGRESS; >> + } else if (is_tcf_mirred_ingress_mirror(act)) { >> + fl_action->id = FLOW_ACTION_MIRRED_INGRESS; >> + } else if (is_tcf_vlan(act)) { >> + switch (tcf_vlan_action(act)) { >> + case TCA_VLAN_ACT_PUSH: >> + fl_action->id = FLOW_ACTION_VLAN_PUSH; >> + break; >> + case TCA_VLAN_ACT_POP: >> + fl_action->id = FLOW_ACTION_VLAN_POP; >> + break; >> + case TCA_VLAN_ACT_MODIFY: >> + fl_action->id = FLOW_ACTION_VLAN_MANGLE; >> + break; >> + default: >> + return -EOPNOTSUPP; >> + } >> + } else if (is_tcf_tunnel_set(act)) { >> + fl_action->id = FLOW_ACTION_TUNNEL_ENCAP; >> + } else if (is_tcf_tunnel_release(act)) { >> + fl_action->id = FLOW_ACTION_TUNNEL_DECAP; >> + } else if (is_tcf_csum(act)) { >> + fl_action->id = FLOW_ACTION_CSUM; >> + } else if (is_tcf_skbedit_mark(act)) { >> + fl_action->id = FLOW_ACTION_MARK; >> + } else if (is_tcf_sample(act)) { >> + fl_action->id = FLOW_ACTION_SAMPLE; >> + } else if (is_tcf_police(act)) { >> + fl_action->id = FLOW_ACTION_POLICE; >> + } else if (is_tcf_ct(act)) { >> + fl_action->id = FLOW_ACTION_CT; >> + } else if (is_tcf_mpls(act)) { >> + switch (tcf_mpls_action(act)) { >> + case TCA_MPLS_ACT_PUSH: >> + fl_action->id = FLOW_ACTION_MPLS_PUSH; >> + break; >> + case TCA_MPLS_ACT_POP: >> + fl_action->id = FLOW_ACTION_MPLS_POP; >> + break; >> + case TCA_MPLS_ACT_MODIFY: >> + fl_action->id = FLOW_ACTION_MPLS_MANGLE; >> + break; >> + default: >> + return -EOPNOTSUPP; >> + } >> + } else if (is_tcf_skbedit_ptype(act)) { >> + fl_action->id = FLOW_ACTION_PTYPE; >> + } else if (is_tcf_skbedit_priority(act)) { >> + fl_action->id = FLOW_ACTION_PRIORITY; >> + } else if (is_tcf_gate(act)) { >> + fl_action->id = FLOW_ACTION_GATE; >> + } else { >> + return -EOPNOTSUPP; >> + } >> + >> + return 0; >> +} >> + > >The challenge with this is now it is impossible to write an action as a >standalone module (which works today). >One resolution to this is to either reuse or introduce a new ops in struct >tc_action_ops. >Then flow_action_init() would just invoke this act->ops() which will do action >specific setup. > Thanks for bringing this to us. As my understanding, for this issue, we are facing the same fact with What we do in function tc_setup_flow_action. If we add a filter with a new added action, we will also fail to offload the filter. For a new added action, if we aim to offload the action to hardware, then we definitely need a init fction and setup function for action/filter offload. We can add a ops for the new added action to init or setup the action. Do you think it is proper to include this implement in our patch series or we can delivery a new patch for this? >cheers, >jamal
On 2021-11-23 03:23, Baowen Zheng wrote: > On November 22, 2021 8:25 PM, Jamal Hadi Salim wrote: >> On 2021-11-18 08:07, Simon Horman wrote: >> >> [..] >> >>> --- a/net/sched/act_api.c >>> +++ b/net/sched/act_api.c >>> @@ -21,6 +21,19 @@ >>> +#include <net/tc_act/tc_pedit.h> >>> +#include <net/tc_act/tc_mirred.h> >>> +#include <net/tc_act/tc_vlan.h> >>> +#include <net/tc_act/tc_tunnel_key.h> #include <net/tc_act/tc_csum.h> >>> +#include <net/tc_act/tc_gact.h> #include <net/tc_act/tc_police.h> >>> +#include <net/tc_act/tc_sample.h> #include <net/tc_act/tc_skbedit.h> >>> +#include <net/tc_act/tc_ct.h> #include <net/tc_act/tc_mpls.h> >>> +#include <net/tc_act/tc_gate.h> #include <net/flow_offload.h> >>> >>> #ifdef CONFIG_INET >>> DEFINE_STATIC_KEY_FALSE(tcf_frag_xmit_count); >>> @@ -129,8 +142,157 @@ static void free_tcf(struct tc_action *p) >>> kfree(p); >>> } >>> >>> +static int flow_action_init(struct flow_offload_action *fl_action, >>> + struct tc_action *act, >>> + enum flow_act_command cmd, >>> + struct netlink_ext_ack *extack) { >>> + if (!fl_action) >>> + return -EINVAL; >>> + >>> + fl_action->extack = extack; >>> + fl_action->command = cmd; >>> + fl_action->index = act->tcfa_index; >>> + >>> + if (is_tcf_gact_ok(act)) { >>> + fl_action->id = FLOW_ACTION_ACCEPT; >>> + } else if (is_tcf_gact_shot(act)) { >>> + fl_action->id = FLOW_ACTION_DROP; >>> + } else if (is_tcf_gact_trap(act)) { >>> + fl_action->id = FLOW_ACTION_TRAP; >>> + } else if (is_tcf_gact_goto_chain(act)) { >>> + fl_action->id = FLOW_ACTION_GOTO; >>> + } else if (is_tcf_mirred_egress_redirect(act)) { >>> + fl_action->id = FLOW_ACTION_REDIRECT; >>> + } else if (is_tcf_mirred_egress_mirror(act)) { >>> + fl_action->id = FLOW_ACTION_MIRRED; >>> + } else if (is_tcf_mirred_ingress_redirect(act)) { >>> + fl_action->id = FLOW_ACTION_REDIRECT_INGRESS; >>> + } else if (is_tcf_mirred_ingress_mirror(act)) { >>> + fl_action->id = FLOW_ACTION_MIRRED_INGRESS; >>> + } else if (is_tcf_vlan(act)) { >>> + switch (tcf_vlan_action(act)) { >>> + case TCA_VLAN_ACT_PUSH: >>> + fl_action->id = FLOW_ACTION_VLAN_PUSH; >>> + break; >>> + case TCA_VLAN_ACT_POP: >>> + fl_action->id = FLOW_ACTION_VLAN_POP; >>> + break; >>> + case TCA_VLAN_ACT_MODIFY: >>> + fl_action->id = FLOW_ACTION_VLAN_MANGLE; >>> + break; >>> + default: >>> + return -EOPNOTSUPP; >>> + } >>> + } else if (is_tcf_tunnel_set(act)) { >>> + fl_action->id = FLOW_ACTION_TUNNEL_ENCAP; >>> + } else if (is_tcf_tunnel_release(act)) { >>> + fl_action->id = FLOW_ACTION_TUNNEL_DECAP; >>> + } else if (is_tcf_csum(act)) { >>> + fl_action->id = FLOW_ACTION_CSUM; >>> + } else if (is_tcf_skbedit_mark(act)) { >>> + fl_action->id = FLOW_ACTION_MARK; >>> + } else if (is_tcf_sample(act)) { >>> + fl_action->id = FLOW_ACTION_SAMPLE; >>> + } else if (is_tcf_police(act)) { >>> + fl_action->id = FLOW_ACTION_POLICE; >>> + } else if (is_tcf_ct(act)) { >>> + fl_action->id = FLOW_ACTION_CT; >>> + } else if (is_tcf_mpls(act)) { >>> + switch (tcf_mpls_action(act)) { >>> + case TCA_MPLS_ACT_PUSH: >>> + fl_action->id = FLOW_ACTION_MPLS_PUSH; >>> + break; >>> + case TCA_MPLS_ACT_POP: >>> + fl_action->id = FLOW_ACTION_MPLS_POP; >>> + break; >>> + case TCA_MPLS_ACT_MODIFY: >>> + fl_action->id = FLOW_ACTION_MPLS_MANGLE; >>> + break; >>> + default: >>> + return -EOPNOTSUPP; >>> + } >>> + } else if (is_tcf_skbedit_ptype(act)) { >>> + fl_action->id = FLOW_ACTION_PTYPE; >>> + } else if (is_tcf_skbedit_priority(act)) { >>> + fl_action->id = FLOW_ACTION_PRIORITY; >>> + } else if (is_tcf_gate(act)) { >>> + fl_action->id = FLOW_ACTION_GATE; >>> + } else { >>> + return -EOPNOTSUPP; >>> + } >>> + >>> + return 0; >>> +} >>> + >> >> The challenge with this is now it is impossible to write an action as a >> standalone module (which works today). >> One resolution to this is to either reuse or introduce a new ops in struct >> tc_action_ops. >> Then flow_action_init() would just invoke this act->ops() which will do action >> specific setup. >> > Thanks for bringing this to us. > As my understanding, for this issue, we are facing the same fact with What we do in function tc_setup_flow_action. > If we add a filter with a new added action, we will also fail to offload the filter. > For a new added action, if we aim to offload the action to hardware, then we definitely need a > init fction and setup function for action/filter offload. We can add a ops for the new added action to init or setup the action. > The simplest approach seems to be adding a field in ops struct and call it hw_id (we already have id which represents the s/w side). All your code in flow_action_init() then becomes something like: if (!fl_action) return -EINVAL; fl_action->extack = extack; fl_action->command = cmd; fl_action->index = act->tcfa_index; fl_action->id = act->hwid; And modules continue to work. Did i miss something? > Do you think it is proper to include this implement in our patch series or we can delivery a new patch for this? Unless I am missing something basic, I dont see this as hard to do as explained above in this patch series. BTW: shouldnt extack be used here instead of returning just -EINVAL? I didnt stare long enough but it seems extack is not passed when deleting from hardware? I saw a NULL being passed in one of the patches. cheers, jamal
On November 24, 2021 3:04 AM, Jamal Hadi Salim wrote: >On 2021-11-23 03:23, Baowen Zheng wrote: >> On November 22, 2021 8:25 PM, Jamal Hadi Salim wrote: >>> On 2021-11-18 08:07, Simon Horman wrote: >>> >>> [..] >>> >>>> --- a/net/sched/act_api.c >>>> +++ b/net/sched/act_api.c >>>> @@ -21,6 +21,19 @@ >>>> +#include <net/tc_act/tc_pedit.h> >>>> +#include <net/tc_act/tc_mirred.h> >>>> +#include <net/tc_act/tc_vlan.h> >>>> +#include <net/tc_act/tc_tunnel_key.h> #include >>>> +<net/tc_act/tc_csum.h> #include <net/tc_act/tc_gact.h> #include >>>> +<net/tc_act/tc_police.h> #include <net/tc_act/tc_sample.h> #include >>>> +<net/tc_act/tc_skbedit.h> #include <net/tc_act/tc_ct.h> #include >>>> +<net/tc_act/tc_mpls.h> #include <net/tc_act/tc_gate.h> #include >>>> +<net/flow_offload.h> >>>> >>>> #ifdef CONFIG_INET >>>> DEFINE_STATIC_KEY_FALSE(tcf_frag_xmit_count); >>>> @@ -129,8 +142,157 @@ static void free_tcf(struct tc_action *p) >>>> kfree(p); >>>> } >>>> >>>> +static int flow_action_init(struct flow_offload_action *fl_action, >>>> + struct tc_action *act, >>>> + enum flow_act_command cmd, >>>> + struct netlink_ext_ack *extack) { >>>> + if (!fl_action) >>>> + return -EINVAL; >>>> + >>>> + fl_action->extack = extack; >>>> + fl_action->command = cmd; >>>> + fl_action->index = act->tcfa_index; >>>> + >>>> + if (is_tcf_gact_ok(act)) { >>>> + fl_action->id = FLOW_ACTION_ACCEPT; >>>> + } else if (is_tcf_gact_shot(act)) { >>>> + fl_action->id = FLOW_ACTION_DROP; >>>> + } else if (is_tcf_gact_trap(act)) { >>>> + fl_action->id = FLOW_ACTION_TRAP; >>>> + } else if (is_tcf_gact_goto_chain(act)) { >>>> + fl_action->id = FLOW_ACTION_GOTO; >>>> + } else if (is_tcf_mirred_egress_redirect(act)) { >>>> + fl_action->id = FLOW_ACTION_REDIRECT; >>>> + } else if (is_tcf_mirred_egress_mirror(act)) { >>>> + fl_action->id = FLOW_ACTION_MIRRED; >>>> + } else if (is_tcf_mirred_ingress_redirect(act)) { >>>> + fl_action->id = FLOW_ACTION_REDIRECT_INGRESS; >>>> + } else if (is_tcf_mirred_ingress_mirror(act)) { >>>> + fl_action->id = FLOW_ACTION_MIRRED_INGRESS; >>>> + } else if (is_tcf_vlan(act)) { >>>> + switch (tcf_vlan_action(act)) { >>>> + case TCA_VLAN_ACT_PUSH: >>>> + fl_action->id = FLOW_ACTION_VLAN_PUSH; >>>> + break; >>>> + case TCA_VLAN_ACT_POP: >>>> + fl_action->id = FLOW_ACTION_VLAN_POP; >>>> + break; >>>> + case TCA_VLAN_ACT_MODIFY: >>>> + fl_action->id = FLOW_ACTION_VLAN_MANGLE; >>>> + break; >>>> + default: >>>> + return -EOPNOTSUPP; >>>> + } >>>> + } else if (is_tcf_tunnel_set(act)) { >>>> + fl_action->id = FLOW_ACTION_TUNNEL_ENCAP; >>>> + } else if (is_tcf_tunnel_release(act)) { >>>> + fl_action->id = FLOW_ACTION_TUNNEL_DECAP; >>>> + } else if (is_tcf_csum(act)) { >>>> + fl_action->id = FLOW_ACTION_CSUM; >>>> + } else if (is_tcf_skbedit_mark(act)) { >>>> + fl_action->id = FLOW_ACTION_MARK; >>>> + } else if (is_tcf_sample(act)) { >>>> + fl_action->id = FLOW_ACTION_SAMPLE; >>>> + } else if (is_tcf_police(act)) { >>>> + fl_action->id = FLOW_ACTION_POLICE; >>>> + } else if (is_tcf_ct(act)) { >>>> + fl_action->id = FLOW_ACTION_CT; >>>> + } else if (is_tcf_mpls(act)) { >>>> + switch (tcf_mpls_action(act)) { >>>> + case TCA_MPLS_ACT_PUSH: >>>> + fl_action->id = FLOW_ACTION_MPLS_PUSH; >>>> + break; >>>> + case TCA_MPLS_ACT_POP: >>>> + fl_action->id = FLOW_ACTION_MPLS_POP; >>>> + break; >>>> + case TCA_MPLS_ACT_MODIFY: >>>> + fl_action->id = FLOW_ACTION_MPLS_MANGLE; >>>> + break; >>>> + default: >>>> + return -EOPNOTSUPP; >>>> + } >>>> + } else if (is_tcf_skbedit_ptype(act)) { >>>> + fl_action->id = FLOW_ACTION_PTYPE; >>>> + } else if (is_tcf_skbedit_priority(act)) { >>>> + fl_action->id = FLOW_ACTION_PRIORITY; >>>> + } else if (is_tcf_gate(act)) { >>>> + fl_action->id = FLOW_ACTION_GATE; >>>> + } else { >>>> + return -EOPNOTSUPP; >>>> + } >>>> + >>>> + return 0; >>>> +} >>>> + >>> >>> The challenge with this is now it is impossible to write an action as >>> a standalone module (which works today). >>> One resolution to this is to either reuse or introduce a new ops in >>> struct tc_action_ops. >>> Then flow_action_init() would just invoke this act->ops() which will >>> do action specific setup. >>> >> Thanks for bringing this to us. >> As my understanding, for this issue, we are facing the same fact with What >we do in function tc_setup_flow_action. >> If we add a filter with a new added action, we will also fail to offload the >filter. >> For a new added action, if we aim to offload the action to hardware, >> then we definitely need a init fction and setup function for action/filter >offload. We can add a ops for the new added action to init or setup the action. >> > >The simplest approach seems to be adding a field in ops struct and call it >hw_id (we already have id which represents the s/w side). >All your code in flow_action_init() then becomes something like: > > if (!fl_action) > return -EINVAL; > > fl_action->extack = extack; > fl_action->command = cmd; > fl_action->index = act->tcfa_index; > > > fl_action->id = act->hwid; > >And modules continue to work. Did i miss something? > Hi Jamal, for your suggestion, I think it will work for most of the case. But there maybe some kind of actions that will be assigned different hw_id in different case, such as the gact, we need to think about this case. So I will prefer to add a callback in action ops struct to implement the flow_action_init function for the new added Standalone action. WDYT? >> Do you think it is proper to include this implement in our patch series or we >can delivery a new patch for this? > >Unless I am missing something basic, I dont see this as hard to do as explained >above in this patch series. I did not mean it is difficult. Since as my understanding, we will have the same problem in function of tc_setup_flow_action to Setup the actions for a to be offloaded flower. So my proposal is to add a callback in action ops to implement Both the function of flow_act_init and tc_setup_flow_action with a flag(maybe bind?) as a distinguish. What is your opinion? > >BTW: shouldnt extack be used here instead of returning just -EINVAL? >I didnt stare long enough but it seems extack is not passed when deleting from >hardware? I saw a NULL being passed in one of the patches. Ok we will consider to delete the extack parameter. >cheers, >jamal
Sorry for reply this message again. On November 24, 2021 10:11 AM, Baowen Zheng wrote: >On November 24, 2021 3:04 AM, Jamal Hadi Salim wrote: >>On 2021-11-23 03:23, Baowen Zheng wrote: >>> On November 22, 2021 8:25 PM, Jamal Hadi Salim wrote: >>>> On 2021-11-18 08:07, Simon Horman wrote: >>>> >>>> [..] >>>> >>>>> --- a/net/sched/act_api.c >>>>> +++ b/net/sched/act_api.c >>>>> @@ -21,6 +21,19 @@ >>>>> +#include <net/tc_act/tc_pedit.h> >>>>> +#include <net/tc_act/tc_mirred.h> >>>>> +#include <net/tc_act/tc_vlan.h> >>>>> +#include <net/tc_act/tc_tunnel_key.h> #include >>>>> +<net/tc_act/tc_csum.h> #include <net/tc_act/tc_gact.h> #include >>>>> +<net/tc_act/tc_police.h> #include <net/tc_act/tc_sample.h> >>>>> +#include <net/tc_act/tc_skbedit.h> #include <net/tc_act/tc_ct.h> >>>>> +#include <net/tc_act/tc_mpls.h> #include <net/tc_act/tc_gate.h> >>>>> +#include <net/flow_offload.h> >>>>> >>>>> #ifdef CONFIG_INET >>>>> DEFINE_STATIC_KEY_FALSE(tcf_frag_xmit_count); >>>>> @@ -129,8 +142,157 @@ static void free_tcf(struct tc_action *p) >>>>> kfree(p); >>>>> } >>>>> >>>>> +static int flow_action_init(struct flow_offload_action *fl_action, >>>>> + struct tc_action *act, >>>>> + enum flow_act_command cmd, >>>>> + struct netlink_ext_ack *extack) { >>>>> + if (!fl_action) >>>>> + return -EINVAL; >>>>> + >>>>> + fl_action->extack = extack; >>>>> + fl_action->command = cmd; >>>>> + fl_action->index = act->tcfa_index; >>>>> + >>>>> + if (is_tcf_gact_ok(act)) { >>>>> + fl_action->id = FLOW_ACTION_ACCEPT; >>>>> + } else if (is_tcf_gact_shot(act)) { >>>>> + fl_action->id = FLOW_ACTION_DROP; >>>>> + } else if (is_tcf_gact_trap(act)) { >>>>> + fl_action->id = FLOW_ACTION_TRAP; >>>>> + } else if (is_tcf_gact_goto_chain(act)) { >>>>> + fl_action->id = FLOW_ACTION_GOTO; >>>>> + } else if (is_tcf_mirred_egress_redirect(act)) { >>>>> + fl_action->id = FLOW_ACTION_REDIRECT; >>>>> + } else if (is_tcf_mirred_egress_mirror(act)) { >>>>> + fl_action->id = FLOW_ACTION_MIRRED; >>>>> + } else if (is_tcf_mirred_ingress_redirect(act)) { >>>>> + fl_action->id = FLOW_ACTION_REDIRECT_INGRESS; >>>>> + } else if (is_tcf_mirred_ingress_mirror(act)) { >>>>> + fl_action->id = FLOW_ACTION_MIRRED_INGRESS; >>>>> + } else if (is_tcf_vlan(act)) { >>>>> + switch (tcf_vlan_action(act)) { >>>>> + case TCA_VLAN_ACT_PUSH: >>>>> + fl_action->id = FLOW_ACTION_VLAN_PUSH; >>>>> + break; >>>>> + case TCA_VLAN_ACT_POP: >>>>> + fl_action->id = FLOW_ACTION_VLAN_POP; >>>>> + break; >>>>> + case TCA_VLAN_ACT_MODIFY: >>>>> + fl_action->id = FLOW_ACTION_VLAN_MANGLE; >>>>> + break; >>>>> + default: >>>>> + return -EOPNOTSUPP; >>>>> + } >>>>> + } else if (is_tcf_tunnel_set(act)) { >>>>> + fl_action->id = FLOW_ACTION_TUNNEL_ENCAP; >>>>> + } else if (is_tcf_tunnel_release(act)) { >>>>> + fl_action->id = FLOW_ACTION_TUNNEL_DECAP; >>>>> + } else if (is_tcf_csum(act)) { >>>>> + fl_action->id = FLOW_ACTION_CSUM; >>>>> + } else if (is_tcf_skbedit_mark(act)) { >>>>> + fl_action->id = FLOW_ACTION_MARK; >>>>> + } else if (is_tcf_sample(act)) { >>>>> + fl_action->id = FLOW_ACTION_SAMPLE; >>>>> + } else if (is_tcf_police(act)) { >>>>> + fl_action->id = FLOW_ACTION_POLICE; >>>>> + } else if (is_tcf_ct(act)) { >>>>> + fl_action->id = FLOW_ACTION_CT; >>>>> + } else if (is_tcf_mpls(act)) { >>>>> + switch (tcf_mpls_action(act)) { >>>>> + case TCA_MPLS_ACT_PUSH: >>>>> + fl_action->id = FLOW_ACTION_MPLS_PUSH; >>>>> + break; >>>>> + case TCA_MPLS_ACT_POP: >>>>> + fl_action->id = FLOW_ACTION_MPLS_POP; >>>>> + break; >>>>> + case TCA_MPLS_ACT_MODIFY: >>>>> + fl_action->id = FLOW_ACTION_MPLS_MANGLE; >>>>> + break; >>>>> + default: >>>>> + return -EOPNOTSUPP; >>>>> + } >>>>> + } else if (is_tcf_skbedit_ptype(act)) { >>>>> + fl_action->id = FLOW_ACTION_PTYPE; >>>>> + } else if (is_tcf_skbedit_priority(act)) { >>>>> + fl_action->id = FLOW_ACTION_PRIORITY; >>>>> + } else if (is_tcf_gate(act)) { >>>>> + fl_action->id = FLOW_ACTION_GATE; >>>>> + } else { >>>>> + return -EOPNOTSUPP; >>>>> + } >>>>> + >>>>> + return 0; >>>>> +} >>>>> + >>>> >>>> The challenge with this is now it is impossible to write an action >>>> as a standalone module (which works today). >>>> One resolution to this is to either reuse or introduce a new ops in >>>> struct tc_action_ops. >>>> Then flow_action_init() would just invoke this act->ops() which will >>>> do action specific setup. >>>> >>> Thanks for bringing this to us. >>> As my understanding, for this issue, we are facing the same fact with >>> What >>we do in function tc_setup_flow_action. >>> If we add a filter with a new added action, we will also fail to >>> offload the >>filter. >>> For a new added action, if we aim to offload the action to hardware, >>> then we definitely need a init fction and setup function for >>> action/filter >>offload. We can add a ops for the new added action to init or setup the >action. >>> >> >>The simplest approach seems to be adding a field in ops struct and call >>it hw_id (we already have id which represents the s/w side). >>All your code in flow_action_init() then becomes something like: >> >> if (!fl_action) >> return -EINVAL; >> >> fl_action->extack = extack; >> fl_action->command = cmd; >> fl_action->index = act->tcfa_index; >> >> >> fl_action->id = act->hwid; >> >>And modules continue to work. Did i miss something? >> >Hi Jamal, for your suggestion, I think it will work for most of the case. But >there maybe some kind of actions that will be assigned different hw_id in >different case, such as the gact, we need to think about this case. >So I will prefer to add a callback in action ops struct to implement the >flow_action_init function for the new added Standalone action. >WDYT? > >>> Do you think it is proper to include this implement in our patch >>> series or we >>can delivery a new patch for this? >> >>Unless I am missing something basic, I dont see this as hard to do as >>explained above in this patch series. >I did not mean it is difficult. >Since as my understanding, we will have the same problem in function of >tc_setup_flow_action to Setup the actions for a to be offloaded flower. So my >proposal is to add a callback in action ops to implement Both the function of >flow_act_init and tc_setup_flow_action with a flag(maybe bind?) as a >distinguish. >What is your opinion? >> >>BTW: shouldnt extack be used here instead of returning just -EINVAL? >>I didnt stare long enough but it seems extack is not passed when >>deleting from hardware? I saw a NULL being passed in one of the patches. Maybe I misunderstand what you mean previously, when I look through the implement in flow_action_init, I did not found we use the extack to make a log before return -EINVAL. So could you please figure it out? Maybe I miss something or misunderstand again. >>cheers, >>jamal
On 2021-11-23 21:11, Baowen Zheng wrote: > On November 24, 2021 3:04 AM, Jamal Hadi Salim wrote: [..] >> The simplest approach seems to be adding a field in ops struct and call it >> hw_id (we already have id which represents the s/w side). >> All your code in flow_action_init() then becomes something like: >> >> if (!fl_action) >> return -EINVAL; >> >> fl_action->extack = extack; >> fl_action->command = cmd; >> fl_action->index = act->tcfa_index; >> >> >> fl_action->id = act->hwid; >> >> And modules continue to work. Did i miss something? >> > Hi Jamal, for your suggestion, I think it will work for most of the case. But there maybe some kind of actions > that will be assigned different hw_id in different case, such as the gact, we need to think about this case. > So I will prefer to add a callback in action ops struct to implement the flow_action_init function for the new added > Standalone action. > WDYT? > Yes, the callback makes sense. I imagine this would be needed also if you offload mirred (selecting whether to mirror or redirect). >>> Do you think it is proper to include this implement in our patch series or we >> can delivery a new patch for this? >> >> Unless I am missing something basic, I dont see this as hard to do as explained >> above in this patch series. > I did not mean it is difficult. > Since as my understanding, we will have the same problem in function of tc_setup_flow_action to > Setup the actions for a to be offloaded flower. So my proposal is to add a callback in action ops to implement > Both the function of flow_act_init and tc_setup_flow_action with a flag(maybe bind?) as a distinguish. > What is your opinion? See above. I agree with your suggestion. cheers, jamal
On 2021-11-24 06:10, Jamal Hadi Salim wrote: > On 2021-11-23 21:11, Baowen Zheng wrote: >> On November 24, 2021 3:04 AM, Jamal Hadi Salim wrote: > > [..] > >>> The simplest approach seems to be adding a field in ops struct and >>> call it >>> hw_id (we already have id which represents the s/w side). >>> All your code in flow_action_init() then becomes something like: >>> >>> if (!fl_action) >>> return -EINVAL; >>> >>> fl_action->extack = extack; >>> fl_action->command = cmd; >>> fl_action->index = act->tcfa_index; >>> >>> >>> fl_action->id = act->hwid; >>> >>> And modules continue to work. Did i miss something? >>> >> Hi Jamal, for your suggestion, I think it will work for most of the >> case. But there maybe some kind of actions >> that will be assigned different hw_id in different case, such as the >> gact, we need to think about this case. >> So I will prefer to add a callback in action ops struct to implement >> the flow_action_init function for the new added >> Standalone action. >> WDYT? >> > > Yes, the callback makes sense. I imagine this would be needed also > if you offload mirred (selecting whether to mirror or redirect). > BTW, I think i am able to parse your earlier message better. There is an equivalent piece of code in cls_api.c. I didnt realize you had cutnpasted from that code. So this callback change has to be a separate patch. i.e patchset 1 to 1) add the callback 2) simplify cls_api.c code patchset 2: Your patchset that then uses the cb. I am also wondering why that code is in the cls_api.c to begin with... cheers, jamal I think if you add the action callback then you can also simplify that. Unfortunately that is now a separate patch given tha
On 2021-11-23 21:59, Baowen Zheng wrote: > Sorry for reply this message again. > On November 24, 2021 10:11 AM, Baowen Zheng wrote: >> On November 24, 2021 3:04 AM, Jamal Hadi Salim wrote: [..] >>> >>> BTW: shouldnt extack be used here instead of returning just -EINVAL? >>> I didnt stare long enough but it seems extack is not passed when >>> deleting from hardware? I saw a NULL being passed in one of the patches. > Maybe I misunderstand what you mean previously, when I look through the implement in > flow_action_init, I did not found we use the extack to make a log before return -EINVAL. > So could you please figure it out? Maybe I miss something or misunderstand again. I mean there are maybe 1-2 places where you called that function flow_action_init() with extack being NULL but the others with legitimate extack. I pointed to offload delete as an example. This may have existed before your changes (but it is hard to tell from just eyeballing patches); regardless it is a problem for debugging incase some delete offload fails, no? BTW: now that i am looking at the patches again - small details: struct flow_offload_action is sometimes initialized and sometimes not (and sometimes allocated and sometimes off the stack). Maybe to be consistent pick one style and stick with it. cheers, jamal
On November 24, 2021 7:40 PM, Jamal Hadi Salim wrote: >On 2021-11-23 21:59, Baowen Zheng wrote: >> Sorry for reply this message again. >> On November 24, 2021 10:11 AM, Baowen Zheng wrote: >>> On November 24, 2021 3:04 AM, Jamal Hadi Salim wrote: > >[..] > >>>> >>>> BTW: shouldnt extack be used here instead of returning just -EINVAL? >>>> I didnt stare long enough but it seems extack is not passed when >>>> deleting from hardware? I saw a NULL being passed in one of the patches. >> Maybe I misunderstand what you mean previously, when I look through >> the implement in flow_action_init, I did not found we use the extack to >make a log before return -EINVAL. >> So could you please figure it out? Maybe I miss something or misunderstand >again. > >I mean there are maybe 1-2 places where you called that function >flow_action_init() with extack being NULL but the others with legitimate extack. >I pointed to offload delete as an example. This may have existed before your >changes (but it is hard to tell from just eyeballing patches); regardless it is a >problem for debugging incase some delete offload fails, no? Yes, you are right, for the most of the delete scenario, the extack is NULL since The original implement to delete the action does not include an extack, so we will Use extack when it is available. > >BTW: >now that i am looking at the patches again - small details: >struct flow_offload_action is sometimes initialized and sometimes not (and >sometimes allocated and sometimes off the stack). Maybe to be consistent >pick one style and stick with it. For this implement, it is because for the action offload process, we need items of flow_action_entry in the flow_offload_action, then the size of flow_offload_action is dependent on the action to be offloaded. But for delete case, we just need a pure flow_offload_action, so we take it in stack. You can refer to the implement in cls_flower, it is similar with our case. Do you think if it make sense to us? >cheers, >jamal
On 2021-11-24 08:47, Baowen Zheng wrote: > On November 24, 2021 7:40 PM, Jamal Hadi Salim wrote: >> On 2021-11-23 21:59, Baowen Zheng wrote: >>> Sorry for reply this message again. >>> On November 24, 2021 10:11 AM, Baowen Zheng wrote: >>>> On November 24, 2021 3:04 AM, Jamal Hadi Salim wrote: >> >> [..] >> >>>>> >>>>> BTW: shouldnt extack be used here instead of returning just -EINVAL? >>>>> I didnt stare long enough but it seems extack is not passed when >>>>> deleting from hardware? I saw a NULL being passed in one of the patches. >>> Maybe I misunderstand what you mean previously, when I look through >>> the implement in flow_action_init, I did not found we use the extack to >> make a log before return -EINVAL. >>> So could you please figure it out? Maybe I miss something or misunderstand >> again. >> >> I mean there are maybe 1-2 places where you called that function >> flow_action_init() with extack being NULL but the others with legitimate extack. >> I pointed to offload delete as an example. This may have existed before your >> changes (but it is hard to tell from just eyeballing patches); regardless it is a >> problem for debugging incase some delete offload fails, no? > Yes, you are right, for the most of the delete scenario, the extack is NULL since > The original implement to delete the action does not include an extack, so we will > Use extack when it is available. You may have to go deeper in the code for this to work. I think if it is tricky to do consider doing it as a followup patch. >> >> BTW: >> now that i am looking at the patches again - small details: >> struct flow_offload_action is sometimes initialized and sometimes not (and >> sometimes allocated and sometimes off the stack). Maybe to be consistent >> pick one style and stick with it. > For this implement, it is because for the action offload process, we need items of > flow_action_entry in the flow_offload_action, then the size of flow_offload_action is > dependent on the action to be offloaded. But for delete case, we just need a pure > flow_offload_action, so we take it in stack. You can refer to the implement in cls_flower, > it is similar with our case. > Do you think if it make sense to us? I think it does. Let me see if i can explain it in my words: For delete you dont have any attributes to pass but for create you need to pass attributes which may be variable sized depending on the action. And for that reason for create you need to allocate (but for delete you can use a variable on the stack). If yes, then at least make sure you are consistent on the stack values; I think i saw some cases you initialize and in some you didnt. cheers, jamal
On November 24, 2021 10:59 PM, Jamal Hadi Salim wrote: >On 2021-11-24 08:47, Baowen Zheng wrote: >> On November 24, 2021 7:40 PM, Jamal Hadi Salim wrote: >>> On 2021-11-23 21:59, Baowen Zheng wrote: >>>> Sorry for reply this message again. >>>> On November 24, 2021 10:11 AM, Baowen Zheng wrote: >>>>> On November 24, 2021 3:04 AM, Jamal Hadi Salim wrote: >>> >>> [..] >>> >>>>>> >>>>>> BTW: shouldnt extack be used here instead of returning just -EINVAL? >>>>>> I didnt stare long enough but it seems extack is not passed when >>>>>> deleting from hardware? I saw a NULL being passed in one of the >patches. >>>> Maybe I misunderstand what you mean previously, when I look through >>>> the implement in flow_action_init, I did not found we use the extack >>>> to >>> make a log before return -EINVAL. >>>> So could you please figure it out? Maybe I miss something or >>>> misunderstand >>> again. >>> >>> I mean there are maybe 1-2 places where you called that function >>> flow_action_init() with extack being NULL but the others with legitimate >extack. >>> I pointed to offload delete as an example. This may have existed >>> before your changes (but it is hard to tell from just eyeballing >>> patches); regardless it is a problem for debugging incase some delete >offload fails, no? >> Yes, you are right, for the most of the delete scenario, the extack is >> NULL since The original implement to delete the action does not >> include an extack, so we will Use extack when it is available. > >You may have to go deeper in the code for this to work. I think if it is tricky to >do consider doing it as a followup patch. > >>> >>> BTW: >>> now that i am looking at the patches again - small details: >>> struct flow_offload_action is sometimes initialized and sometimes not >>> (and sometimes allocated and sometimes off the stack). Maybe to be >>> consistent pick one style and stick with it. >> For this implement, it is because for the action offload process, we >> need items of flow_action_entry in the flow_offload_action, then the >> size of flow_offload_action is dependent on the action to be >> offloaded. But for delete case, we just need a pure >> flow_offload_action, so we take it in stack. You can refer to the implement >in cls_flower, it is similar with our case. >> Do you think if it make sense to us? > >I think it does. Let me see if i can explain it in my words: >For delete you dont have any attributes to pass but for create you need to >pass attributes which may be variable sized depending on the action. >And for that reason for create you need to allocate (but for delete you can use >a variable on the stack). >If yes, then at least make sure you are consistent on the stack values; I think i >saw some cases you initialize and in some you didnt. For the initialization, Vald mentioned this in another message, we will add the Initialization in action delete, thanks. >cheers, >jamal
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h index 4f4a299e92de..ae189fcff3c6 100644 --- a/include/linux/netdevice.h +++ b/include/linux/netdevice.h @@ -916,6 +916,7 @@ enum tc_setup_type { TC_SETUP_QDISC_TBF, TC_SETUP_QDISC_FIFO, TC_SETUP_QDISC_HTB, + TC_SETUP_ACT, }; /* These structures hold the attributes of bpf state that are being passed diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h index f6970213497a..15662cad5bca 100644 --- a/include/net/flow_offload.h +++ b/include/net/flow_offload.h @@ -551,6 +551,23 @@ struct flow_cls_offload { u32 classid; }; +enum flow_act_command { + FLOW_ACT_REPLACE, + FLOW_ACT_DESTROY, + FLOW_ACT_STATS, +}; + +struct flow_offload_action { + struct netlink_ext_ack *extack; /* NULL in FLOW_ACT_STATS process*/ + enum flow_act_command command; + enum flow_action_id id; + u32 index; + struct flow_stats stats; + struct flow_action action; +}; + +struct flow_offload_action *flow_action_alloc(unsigned int num_actions); + static inline struct flow_rule * flow_cls_offload_flow_rule(struct flow_cls_offload *flow_cmd) { diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h index 193f88ebf629..14d098a887d0 100644 --- a/include/net/pkt_cls.h +++ b/include/net/pkt_cls.h @@ -258,6 +258,14 @@ static inline void tcf_exts_put_net(struct tcf_exts *exts) for (; 0; (void)(i), (void)(a), (void)(exts)) #endif +#define tcf_act_for_each_action(i, a, actions) \ + for (i = 0; i < TCA_ACT_MAX_PRIO && ((a) = actions[i]); i++) + +static inline bool tc_act_bind(u32 flags) +{ + return !!(flags & TCA_ACT_FLAGS_BIND); +} + static inline void tcf_exts_stats_update(const struct tcf_exts *exts, u64 bytes, u64 packets, u64 drops, u64 lastuse, @@ -534,6 +542,9 @@ tcf_match_indev(struct sk_buff *skb, int ifindex) int tc_setup_flow_action(struct flow_action *flow_action, const struct tcf_exts *exts); + +int tc_setup_action(struct flow_action *flow_action, + struct tc_action *actions[]); void tc_cleanup_flow_action(struct flow_action *flow_action); int tc_setup_cb_call(struct tcf_block *block, enum tc_setup_type type, @@ -554,6 +565,7 @@ int tc_setup_cb_reoffload(struct tcf_block *block, struct tcf_proto *tp, enum tc_setup_type type, void *type_data, void *cb_priv, u32 *flags, unsigned int *in_hw_count); unsigned int tcf_exts_num_actions(struct tcf_exts *exts); +unsigned int tcf_act_num_actions_single(struct tc_action *act); #ifdef CONFIG_NET_CLS_ACT int tcf_qevent_init(struct tcf_qevent *qe, struct Qdisc *sch, diff --git a/net/core/flow_offload.c b/net/core/flow_offload.c index 6beaea13564a..6676431733ef 100644 --- a/net/core/flow_offload.c +++ b/net/core/flow_offload.c @@ -27,6 +27,27 @@ struct flow_rule *flow_rule_alloc(unsigned int num_actions) } EXPORT_SYMBOL(flow_rule_alloc); +struct flow_offload_action *flow_action_alloc(unsigned int num_actions) +{ + struct flow_offload_action *fl_action; + int i; + + fl_action = kzalloc(struct_size(fl_action, action.entries, num_actions), + GFP_KERNEL); + if (!fl_action) + return NULL; + + fl_action->action.num_entries = num_actions; + /* Pre-fill each action hw_stats with DONT_CARE. + * Caller can override this if it wants stats for a given action. + */ + for (i = 0; i < num_actions; i++) + fl_action->action.entries[i].hw_stats = FLOW_ACTION_HW_STATS_DONT_CARE; + + return fl_action; +} +EXPORT_SYMBOL(flow_action_alloc); + #define FLOW_DISSECTOR_MATCH(__rule, __type, __out) \ const struct flow_match *__m = &(__rule)->match; \ struct flow_dissector *__d = (__m)->dissector; \ @@ -549,19 +570,25 @@ int flow_indr_dev_setup_offload(struct net_device *dev, struct Qdisc *sch, void (*cleanup)(struct flow_block_cb *block_cb)) { struct flow_indr_dev *this; + u32 count = 0; + int err; mutex_lock(&flow_indr_block_lock); + if (bo) { + if (bo->command == FLOW_BLOCK_BIND) + indir_dev_add(data, dev, sch, type, cleanup, bo); + else if (bo->command == FLOW_BLOCK_UNBIND) + indir_dev_remove(data); + } - if (bo->command == FLOW_BLOCK_BIND) - indir_dev_add(data, dev, sch, type, cleanup, bo); - else if (bo->command == FLOW_BLOCK_UNBIND) - indir_dev_remove(data); - - list_for_each_entry(this, &flow_block_indr_dev_list, list) - this->cb(dev, sch, this->cb_priv, type, bo, data, cleanup); + list_for_each_entry(this, &flow_block_indr_dev_list, list) { + err = this->cb(dev, sch, this->cb_priv, type, bo, data, cleanup); + if (!err) + count++; + } mutex_unlock(&flow_indr_block_lock); - return list_empty(&bo->cb_list) ? -EOPNOTSUPP : 0; + return (bo && list_empty(&bo->cb_list)) ? -EOPNOTSUPP : count; } EXPORT_SYMBOL(flow_indr_dev_setup_offload); diff --git a/net/sched/act_api.c b/net/sched/act_api.c index 3258da3d5bed..c3d08b710661 100644 --- a/net/sched/act_api.c +++ b/net/sched/act_api.c @@ -21,6 +21,19 @@ #include <net/pkt_cls.h> #include <net/act_api.h> #include <net/netlink.h> +#include <net/tc_act/tc_pedit.h> +#include <net/tc_act/tc_mirred.h> +#include <net/tc_act/tc_vlan.h> +#include <net/tc_act/tc_tunnel_key.h> +#include <net/tc_act/tc_csum.h> +#include <net/tc_act/tc_gact.h> +#include <net/tc_act/tc_police.h> +#include <net/tc_act/tc_sample.h> +#include <net/tc_act/tc_skbedit.h> +#include <net/tc_act/tc_ct.h> +#include <net/tc_act/tc_mpls.h> +#include <net/tc_act/tc_gate.h> +#include <net/flow_offload.h> #ifdef CONFIG_INET DEFINE_STATIC_KEY_FALSE(tcf_frag_xmit_count); @@ -129,8 +142,157 @@ static void free_tcf(struct tc_action *p) kfree(p); } +static int flow_action_init(struct flow_offload_action *fl_action, + struct tc_action *act, + enum flow_act_command cmd, + struct netlink_ext_ack *extack) +{ + if (!fl_action) + return -EINVAL; + + fl_action->extack = extack; + fl_action->command = cmd; + fl_action->index = act->tcfa_index; + + if (is_tcf_gact_ok(act)) { + fl_action->id = FLOW_ACTION_ACCEPT; + } else if (is_tcf_gact_shot(act)) { + fl_action->id = FLOW_ACTION_DROP; + } else if (is_tcf_gact_trap(act)) { + fl_action->id = FLOW_ACTION_TRAP; + } else if (is_tcf_gact_goto_chain(act)) { + fl_action->id = FLOW_ACTION_GOTO; + } else if (is_tcf_mirred_egress_redirect(act)) { + fl_action->id = FLOW_ACTION_REDIRECT; + } else if (is_tcf_mirred_egress_mirror(act)) { + fl_action->id = FLOW_ACTION_MIRRED; + } else if (is_tcf_mirred_ingress_redirect(act)) { + fl_action->id = FLOW_ACTION_REDIRECT_INGRESS; + } else if (is_tcf_mirred_ingress_mirror(act)) { + fl_action->id = FLOW_ACTION_MIRRED_INGRESS; + } else if (is_tcf_vlan(act)) { + switch (tcf_vlan_action(act)) { + case TCA_VLAN_ACT_PUSH: + fl_action->id = FLOW_ACTION_VLAN_PUSH; + break; + case TCA_VLAN_ACT_POP: + fl_action->id = FLOW_ACTION_VLAN_POP; + break; + case TCA_VLAN_ACT_MODIFY: + fl_action->id = FLOW_ACTION_VLAN_MANGLE; + break; + default: + return -EOPNOTSUPP; + } + } else if (is_tcf_tunnel_set(act)) { + fl_action->id = FLOW_ACTION_TUNNEL_ENCAP; + } else if (is_tcf_tunnel_release(act)) { + fl_action->id = FLOW_ACTION_TUNNEL_DECAP; + } else if (is_tcf_csum(act)) { + fl_action->id = FLOW_ACTION_CSUM; + } else if (is_tcf_skbedit_mark(act)) { + fl_action->id = FLOW_ACTION_MARK; + } else if (is_tcf_sample(act)) { + fl_action->id = FLOW_ACTION_SAMPLE; + } else if (is_tcf_police(act)) { + fl_action->id = FLOW_ACTION_POLICE; + } else if (is_tcf_ct(act)) { + fl_action->id = FLOW_ACTION_CT; + } else if (is_tcf_mpls(act)) { + switch (tcf_mpls_action(act)) { + case TCA_MPLS_ACT_PUSH: + fl_action->id = FLOW_ACTION_MPLS_PUSH; + break; + case TCA_MPLS_ACT_POP: + fl_action->id = FLOW_ACTION_MPLS_POP; + break; + case TCA_MPLS_ACT_MODIFY: + fl_action->id = FLOW_ACTION_MPLS_MANGLE; + break; + default: + return -EOPNOTSUPP; + } + } else if (is_tcf_skbedit_ptype(act)) { + fl_action->id = FLOW_ACTION_PTYPE; + } else if (is_tcf_skbedit_priority(act)) { + fl_action->id = FLOW_ACTION_PRIORITY; + } else if (is_tcf_gate(act)) { + fl_action->id = FLOW_ACTION_GATE; + } else { + return -EOPNOTSUPP; + } + + return 0; +} + +static int tcf_action_offload_cmd(struct flow_offload_action *fl_act, + struct netlink_ext_ack *extack) +{ + int err; + + if (IS_ERR(fl_act)) + return PTR_ERR(fl_act); + + err = flow_indr_dev_setup_offload(NULL, NULL, TC_SETUP_ACT, + fl_act, NULL, NULL); + if (err < 0) + return err; + + return 0; +} + +/* offload the tc command after inserted */ +static int tcf_action_offload_add(struct tc_action *action, + struct netlink_ext_ack *extack) +{ + struct tc_action *actions[TCA_ACT_MAX_PRIO] = { + [0] = action, + }; + struct flow_offload_action *fl_action; + int err = 0; + + fl_action = flow_action_alloc(tcf_act_num_actions_single(action)); + if (!fl_action) + return -ENOMEM; + + err = flow_action_init(fl_action, action, FLOW_ACT_REPLACE, extack); + if (err) + goto fl_err; + + err = tc_setup_action(&fl_action->action, actions); + if (err) { + NL_SET_ERR_MSG_MOD(extack, + "Failed to setup tc actions for offload\n"); + goto fl_err; + } + + err = tcf_action_offload_cmd(fl_action, extack); + tc_cleanup_flow_action(&fl_action->action); + +fl_err: + kfree(fl_action); + + return err; +} + +static int tcf_action_offload_del(struct tc_action *action) +{ + struct flow_offload_action fl_act; + int err = 0; + + if (!action) + return -EINVAL; + + err = flow_action_init(&fl_act, action, FLOW_ACT_DESTROY, NULL); + if (err) + return err; + + return tcf_action_offload_cmd(&fl_act, NULL); +} + static void tcf_action_cleanup(struct tc_action *p) { + tcf_action_offload_del(p); if (p->ops->cleanup) p->ops->cleanup(p); @@ -1103,6 +1265,8 @@ int tcf_action_init(struct net *net, struct tcf_proto *tp, struct nlattr *nla, sz += tcf_action_fill_size(act); /* Start from index 0 */ actions[i - 1] = act; + if (!tc_act_bind(flags)) + tcf_action_offload_add(act, extack); } /* We have to commit them all together, because if any error happened in diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c index d9d6ff0bf361..55fa48999d43 100644 --- a/net/sched/cls_api.c +++ b/net/sched/cls_api.c @@ -3544,8 +3544,8 @@ static enum flow_action_hw_stats tc_act_hw_stats(u8 hw_stats) return hw_stats; } -int tc_setup_flow_action(struct flow_action *flow_action, - const struct tcf_exts *exts) +int tc_setup_action(struct flow_action *flow_action, + struct tc_action *actions[]) { struct tc_action *act; int i, j, k, err = 0; @@ -3554,11 +3554,11 @@ int tc_setup_flow_action(struct flow_action *flow_action, BUILD_BUG_ON(TCA_ACT_HW_STATS_IMMEDIATE != FLOW_ACTION_HW_STATS_IMMEDIATE); BUILD_BUG_ON(TCA_ACT_HW_STATS_DELAYED != FLOW_ACTION_HW_STATS_DELAYED); - if (!exts) + if (!actions) return 0; j = 0; - tcf_exts_for_each_action(i, act, exts) { + tcf_act_for_each_action(i, act, actions) { struct flow_action_entry *entry; entry = &flow_action->entries[j]; @@ -3724,6 +3724,20 @@ int tc_setup_flow_action(struct flow_action *flow_action, spin_unlock_bh(&act->tcfa_lock); goto err_out; } +EXPORT_SYMBOL(tc_setup_action); + +int tc_setup_flow_action(struct flow_action *flow_action, + const struct tcf_exts *exts) +{ +#ifdef CONFIG_NET_CLS_ACT + if (!exts) + return 0; + + return tc_setup_action(flow_action, exts->actions); +#else + return 0; +#endif +} EXPORT_SYMBOL(tc_setup_flow_action); unsigned int tcf_exts_num_actions(struct tcf_exts *exts) @@ -3742,6 +3756,15 @@ unsigned int tcf_exts_num_actions(struct tcf_exts *exts) } EXPORT_SYMBOL(tcf_exts_num_actions); +unsigned int tcf_act_num_actions_single(struct tc_action *act) +{ + if (is_tcf_pedit(act)) + return tcf_pedit_nkeys(act); + else + return 1; +} +EXPORT_SYMBOL(tcf_act_num_actions_single); + #ifdef CONFIG_NET_CLS_ACT static int tcf_qevent_parse_block_index(struct nlattr *block_index_attr, u32 *p_block_index,