| Message ID | 20230206174403.32733-2-paulb@nvidia.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net/sched: cls_api: Support hardware miss to tc action | expand |
On Mon, Feb 06, 2023 at 07:43:57PM +0200, Paul Blakey wrote: > For drivers to support partial offload of a filter's action list, > add support for action miss to specify an action instance to > continue from in sw. > > CT action in particular can't be fully offloaded, as new connections > need to be handled in software. This imposes other limitations on > the actions that can be offloaded together with the CT action, such > as packet modifications. > > Assign each action on a filter's action list a unique miss_cookie > which drivers can then use to fill action_miss part of the tc skb > extension. On getting back this miss_cookie, find the action > instance with relevant cookie and continue classifying from there. > > Signed-off-by: Paul Blakey <paulb@nvidia.com> > Reviewed-by: Jiri Pirko <jiri@nvidia.com> > Reviewed-by: Simon Horman <simon.horman@corigine.com> > Acked-by: Jamal Hadi Salim <jhs@mojatatu.com> > --- > include/linux/skbuff.h | 6 +- > include/net/flow_offload.h | 1 + > include/net/pkt_cls.h | 34 +++--- > include/net/sch_generic.h | 2 + > net/openvswitch/flow.c | 3 +- > net/sched/act_api.c | 2 +- > net/sched/cls_api.c | 213 +++++++++++++++++++++++++++++++++++-- > 7 files changed, 234 insertions(+), 27 deletions(-) > > diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h > index 1fa95b916342..9b9aa854068f 100644 > --- a/include/linux/skbuff.h > +++ b/include/linux/skbuff.h > @@ -311,12 +311,16 @@ struct nf_bridge_info { > * and read by ovs to recirc_id. > */ > struct tc_skb_ext { > - __u32 chain; > + union { > + u64 act_miss_cookie; > + __u32 chain; > + }; > __u16 mru; > __u16 zone; > u8 post_ct:1; > u8 post_ct_snat:1; > u8 post_ct_dnat:1; > + u8 act_miss:1; /* Set if act_miss_cookie is used */ > }; > #endif > > diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h > index 0400a0ac8a29..88db7346eb7a 100644 > --- a/include/net/flow_offload.h > +++ b/include/net/flow_offload.h > @@ -228,6 +228,7 @@ void flow_action_cookie_destroy(struct flow_action_cookie *cookie); > struct flow_action_entry { > enum flow_action_id id; > u32 hw_index; > + u64 miss_cookie; The per-action stats patchset is adding a cookie for the actions as well, and exactly on this struct: @@ -228,6 +228,7 @@ struct flow_action_cookie *flow_action_cookie_create(void *data, struct flow_action_entry { enum flow_action_id id; u32 hw_index; + unsigned long act_cookie; enum flow_action_hw_stats hw_stats; action_destr destructor; void *destructor_priv; There, it is a simple value: the act pointer itself. Here, it is already more complex. Can them be merged into only one maybe? If not, perhaps act_cookie should be renamed to stats_cookie then. > enum flow_action_hw_stats hw_stats; > action_destr destructor; > void *destructor_priv; > diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h > index cd410a87517b..e395f2a84ed2 100644 > --- a/include/net/pkt_cls.h > +++ b/include/net/pkt_cls.h > @@ -59,6 +59,8 @@ int tcf_block_get_ext(struct tcf_block **p_block, struct Qdisc *q, > void tcf_block_put(struct tcf_block *block); > void tcf_block_put_ext(struct tcf_block *block, struct Qdisc *q, > struct tcf_block_ext_info *ei); > +int tcf_exts_init_ex(struct tcf_exts *exts, struct net *net, int action, > + int police, struct tcf_proto *tp, u32 handle, bool used_action_miss); > > static inline bool tcf_block_shared(struct tcf_block *block) > { > @@ -229,6 +231,7 @@ struct tcf_exts { > struct tc_action **actions; > struct net *net; > netns_tracker ns_tracker; > + struct tcf_exts_miss_cookie_node *miss_cookie_node; > #endif > /* Map to export classifier specific extension TLV types to the > * generic extensions API. Unsupported extensions must be set to 0. > @@ -240,21 +243,11 @@ struct tcf_exts { > static inline int tcf_exts_init(struct tcf_exts *exts, struct net *net, > int action, int police) > { > -#ifdef CONFIG_NET_CLS_ACT > - exts->type = 0; > - exts->nr_actions = 0; > - /* Note: we do not own yet a reference on net. > - * This reference might be taken later from tcf_exts_get_net(). > - */ > - exts->net = net; > - exts->actions = kcalloc(TCA_ACT_MAX_PRIO, sizeof(struct tc_action *), > - GFP_KERNEL); > - if (!exts->actions) > - return -ENOMEM; > +#ifdef CONFIG_NET_CLS > + return tcf_exts_init_ex(exts, net, action, police, NULL, 0, false); > +#else > + return -EOPNOTSUPP; > #endif > - exts->action = action; > - exts->police = police; > - return 0; > } > > /* Return false if the netns is being destroyed in cleanup_net(). Callers > @@ -353,6 +346,18 @@ tcf_exts_exec(struct sk_buff *skb, struct tcf_exts *exts, > return TC_ACT_OK; > } > > +static inline int > +tcf_exts_exec_ex(struct sk_buff *skb, struct tcf_exts *exts, int act_index, > + struct tcf_result *res) > +{ > +#ifdef CONFIG_NET_CLS_ACT > + return tcf_action_exec(skb, exts->actions + act_index, > + exts->nr_actions - act_index, res); > +#else > + return TC_ACT_OK; > +#endif > +} > + > int tcf_exts_validate(struct net *net, struct tcf_proto *tp, > struct nlattr **tb, struct nlattr *rate_tlv, > struct tcf_exts *exts, u32 flags, > @@ -577,6 +582,7 @@ int tc_setup_offload_action(struct flow_action *flow_action, > void tc_cleanup_offload_action(struct flow_action *flow_action); > int tc_setup_action(struct flow_action *flow_action, > struct tc_action *actions[], > + u32 miss_cookie_base, > struct netlink_ext_ack *extack); > > int tc_setup_cb_call(struct tcf_block *block, enum tc_setup_type type, > diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h > index af4aa66aaa4e..fab5ba3e61b7 100644 > --- a/include/net/sch_generic.h > +++ b/include/net/sch_generic.h > @@ -369,6 +369,8 @@ struct tcf_proto_ops { > struct nlattr **tca, > struct netlink_ext_ack *extack); > void (*tmplt_destroy)(void *tmplt_priv); > + struct tcf_exts * (*get_exts)(const struct tcf_proto *tp, > + u32 handle); > > /* rtnetlink specific */ > int (*dump)(struct net*, struct tcf_proto*, void *, > diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c > index e20d1a973417..69f91460a55c 100644 > --- a/net/openvswitch/flow.c > +++ b/net/openvswitch/flow.c > @@ -1038,7 +1038,8 @@ int ovs_flow_key_extract(const struct ip_tunnel_info *tun_info, > #if IS_ENABLED(CONFIG_NET_TC_SKB_EXT) > if (tc_skb_ext_tc_enabled()) { > tc_ext = skb_ext_find(skb, TC_SKB_EXT); > - key->recirc_id = tc_ext ? tc_ext->chain : 0; > + key->recirc_id = tc_ext && !tc_ext->act_miss ? > + tc_ext->chain : 0; > OVS_CB(skb)->mru = tc_ext ? tc_ext->mru : 0; > post_ct = tc_ext ? tc_ext->post_ct : false; > post_ct_snat = post_ct ? tc_ext->post_ct_snat : false; > diff --git a/net/sched/act_api.c b/net/sched/act_api.c > index cd09ef49df22..16fd3d30eb12 100644 > --- a/net/sched/act_api.c > +++ b/net/sched/act_api.c > @@ -272,7 +272,7 @@ static int tcf_action_offload_add_ex(struct tc_action *action, > if (err) > goto fl_err; > > - err = tc_setup_action(&fl_action->action, actions, extack); > + err = tc_setup_action(&fl_action->action, actions, 0, extack); > if (err) { > NL_SET_ERR_MSG_MOD(extack, > "Failed to setup tc actions for offload"); > diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c > index 5b4a95e8a1ee..8ff9530fef68 100644 > --- a/net/sched/cls_api.c > +++ b/net/sched/cls_api.c > @@ -22,6 +22,7 @@ > #include <linux/idr.h> > #include <linux/jhash.h> > #include <linux/rculist.h> > +#include <linux/rhashtable.h> > #include <net/net_namespace.h> > #include <net/sock.h> > #include <net/netlink.h> > @@ -50,6 +51,109 @@ static LIST_HEAD(tcf_proto_base); > /* Protects list of registered TC modules. It is pure SMP lock. */ > static DEFINE_RWLOCK(cls_mod_lock); > > +static struct xarray tcf_exts_miss_cookies_xa; > +struct tcf_exts_miss_cookie_node { > + const struct tcf_chain *chain; > + const struct tcf_proto *tp; > + const struct tcf_exts *exts; > + u32 chain_index; > + u32 tp_prio; > + u32 handle; > + u32 miss_cookie_base; > + struct rcu_head rcu; > +}; > + > +/* Each tc action entry cookie will be comprised of 32bit miss_cookie_base + > + * action index in the exts tc actions array. > + */ > +union tcf_exts_miss_cookie { > + struct { > + u32 miss_cookie_base; > + u32 act_index; > + }; > + u64 miss_cookie; > +}; > + > +#if IS_ENABLED(CONFIG_NET_TC_SKB_EXT) > +static int > +tcf_exts_miss_cookie_base_alloc(struct tcf_exts *exts, struct tcf_proto *tp, > + u32 handle) > +{ > + struct tcf_exts_miss_cookie_node *n; > + static u32 next; What protects this static variable from concurrent access? > + int err; > + > + if (WARN_ON(!handle || !tp->ops->get_exts)) > + return -EINVAL; > + > + n = kzalloc(sizeof(*n), GFP_KERNEL); > + if (!n) > + return -ENOMEM; > + > + n->chain_index = tp->chain->index; > + n->chain = tp->chain; > + n->tp_prio = tp->prio; > + n->tp = tp; > + n->exts = exts; > + n->handle = handle; > + > + err = xa_alloc_cyclic(&tcf_exts_miss_cookies_xa, &n->miss_cookie_base, > + n, xa_limit_32b, &next, GFP_KERNEL); > + if (err) > + goto err_xa_alloc; > + > + exts->miss_cookie_node = n; > + return 0; > + > +err_xa_alloc: > + kfree(n); > + return err; > +} > + > +static void tcf_exts_miss_cookie_base_destroy(struct tcf_exts *exts) > +{ > + struct tcf_exts_miss_cookie_node *n; > + > + if (!exts->miss_cookie_node) > + return; > + > + n = exts->miss_cookie_node; > + xa_erase(&tcf_exts_miss_cookies_xa, n->miss_cookie_base); > + kfree_rcu(n, rcu); > +} > + > +static struct tcf_exts_miss_cookie_node * > +tcf_exts_miss_cookie_lookup(u64 miss_cookie, int *act_index) > +{ > + union tcf_exts_miss_cookie mc = { .miss_cookie = miss_cookie, }; > + > + *act_index = mc.act_index; > + return xa_load(&tcf_exts_miss_cookies_xa, mc.miss_cookie_base); > +} > +#else /* IS_ENABLED(CONFIG_NET_TC_SKB_EXT) */ > +static int > +tcf_exts_miss_cookie_base_alloc(struct tcf_exts *exts, struct tcf_proto *tp, > + u32 handle) > +{ > + return 0; > +} > + > +static void tcf_exts_miss_cookie_base_destroy(struct tcf_exts *exts) > +{ > +} > +#endif /* IS_ENABLED(CONFIG_NET_TC_SKB_EXT) */ > + > +static u64 tcf_exts_miss_cookie_get(u32 miss_cookie_base, int act_index) > +{ > + union tcf_exts_miss_cookie mc = { .act_index = act_index, }; > + > + if (!miss_cookie_base) > + return 0; > + > + mc.miss_cookie_base = miss_cookie_base; > + return mc.miss_cookie; > +} > + > #ifdef CONFIG_NET_CLS_ACT > DEFINE_STATIC_KEY_FALSE(tc_skb_ext_tc); > EXPORT_SYMBOL(tc_skb_ext_tc); > @@ -1549,6 +1653,8 @@ static inline int __tcf_classify(struct sk_buff *skb, > const struct tcf_proto *orig_tp, > struct tcf_result *res, > bool compat_mode, > + struct tcf_exts_miss_cookie_node *n, > + int act_index, > u32 *last_executed_chain) > { > #ifdef CONFIG_NET_CLS_ACT > @@ -1560,13 +1666,36 @@ static inline int __tcf_classify(struct sk_buff *skb, > #endif > for (; tp; tp = rcu_dereference_bh(tp->next)) { > __be16 protocol = skb_protocol(skb, false); > - int err; > + int err = 0; > > - if (tp->protocol != protocol && > - tp->protocol != htons(ETH_P_ALL)) > - continue; > + if (n) { > + struct tcf_exts *exts; > + > + if (n->tp_prio != tp->prio) > + continue; > + > + /* We re-lookup the tp and chain based on index instead > + * of having hard refs and locks to them, so do a sanity > + * check if any of tp,chain,exts was replaced by the > + * time we got here with a cookie from hardware. > + */ > + if (unlikely(n->tp != tp || n->tp->chain != n->chain || > + !tp->ops->get_exts)) > + return TC_ACT_SHOT; > + > + exts = tp->ops->get_exts(tp, n->handle); > + if (unlikely(!exts || n->exts != exts)) > + return TC_ACT_SHOT; > > - err = tc_classify(skb, tp, res); > + n = NULL; > + err = tcf_exts_exec_ex(skb, exts, act_index, res); > + } else { > + if (tp->protocol != protocol && > + tp->protocol != htons(ETH_P_ALL)) > + continue; > + > + err = tc_classify(skb, tp, res); > + } > #ifdef CONFIG_NET_CLS_ACT > if (unlikely(err == TC_ACT_RECLASSIFY && !compat_mode)) { > first_tp = orig_tp; > @@ -1582,6 +1711,9 @@ static inline int __tcf_classify(struct sk_buff *skb, > return err; > } > > + if (unlikely(n)) > + return TC_ACT_SHOT; > + > return TC_ACT_UNSPEC; /* signal: continue lookup */ > #ifdef CONFIG_NET_CLS_ACT > reset: > @@ -1606,21 +1738,33 @@ int tcf_classify(struct sk_buff *skb, > #if !IS_ENABLED(CONFIG_NET_TC_SKB_EXT) > u32 last_executed_chain = 0; > > - return __tcf_classify(skb, tp, tp, res, compat_mode, > + return __tcf_classify(skb, tp, tp, res, compat_mode, NULL, 0, > &last_executed_chain); > #else > u32 last_executed_chain = tp ? tp->chain->index : 0; > + struct tcf_exts_miss_cookie_node *n = NULL; > const struct tcf_proto *orig_tp = tp; > struct tc_skb_ext *ext; > + int act_index = 0; > int ret; > > if (block) { > ext = skb_ext_find(skb, TC_SKB_EXT); > > - if (ext && ext->chain) { > + if (ext && (ext->chain || ext->act_miss)) { > struct tcf_chain *fchain; > + u32 chain = ext->chain; IMHO it would be nice to avoid this assignment here, because there is nothing above saying that the union value can be read as a chain. For C, yes, it is okay. The worry is about ensuring that whatever is reading it, is reading what it is expecting it to be. Perhaps just declare it here, and then: > > - fchain = tcf_chain_lookup_rcu(block, ext->chain); > + if (ext->act_miss) { > + n = tcf_exts_miss_cookie_lookup(ext->act_miss_cookie, > + &act_index); > + if (!n) > + return TC_ACT_SHOT; > + > + chain = n->chain_index; > + } else { chain = ext->chain; } > + > + fchain = tcf_chain_lookup_rcu(block, chain); > if (!fchain) > return TC_ACT_SHOT; > > @@ -1632,7 +1776,7 @@ int tcf_classify(struct sk_buff *skb, > } > } > > - ret = __tcf_classify(skb, tp, orig_tp, res, compat_mode, > + ret = __tcf_classify(skb, tp, orig_tp, res, compat_mode, n, act_index, > &last_executed_chain); > > if (tc_skb_ext_tc_enabled()) { > @@ -3056,9 +3200,48 @@ static int tc_dump_chain(struct sk_buff *skb, struct netlink_callback *cb) > return skb->len; > } > > +int tcf_exts_init_ex(struct tcf_exts *exts, struct net *net, int action, > + int police, struct tcf_proto *tp, u32 handle, > + bool use_action_miss) > +{ > + int err = 0; > + > +#ifdef CONFIG_NET_CLS_ACT > + exts->type = 0; > + exts->nr_actions = 0; > + /* Note: we do not own yet a reference on net. > + * This reference might be taken later from tcf_exts_get_net(). > + */ > + exts->net = net; > + exts->actions = kcalloc(TCA_ACT_MAX_PRIO, sizeof(struct tc_action *), > + GFP_KERNEL); > + if (!exts->actions) > + return -ENOMEM; > +#endif > + > + exts->action = action; > + exts->police = police; > + > + if (!use_action_miss) > + return 0; > + > + err = tcf_exts_miss_cookie_base_alloc(exts, tp, handle); > + if (err) > + goto err_miss_alloc; > + > + return 0; > + > +err_miss_alloc: > + tcf_exts_destroy(exts); > + return err; > +} > +EXPORT_SYMBOL(tcf_exts_init_ex); > + > void tcf_exts_destroy(struct tcf_exts *exts) > { > #ifdef CONFIG_NET_CLS_ACT > + tcf_exts_miss_cookie_base_destroy(exts); > + > if (exts->actions) { > tcf_action_destroy(exts->actions, TCA_ACT_UNBIND); > kfree(exts->actions); > @@ -3547,6 +3730,7 @@ static int tc_setup_offload_act(struct tc_action *act, > > int tc_setup_action(struct flow_action *flow_action, > struct tc_action *actions[], > + u32 miss_cookie_base, > struct netlink_ext_ack *extack) > { > int i, j, k, index, err = 0; > @@ -3577,6 +3761,8 @@ int tc_setup_action(struct flow_action *flow_action, > for (k = 0; k < index ; k++) { > entry[k].hw_stats = tc_act_hw_stats(act->hw_stats); > entry[k].hw_index = act->tcfa_index; > + entry[k].miss_cookie = > + tcf_exts_miss_cookie_get(miss_cookie_base, i); > } > > j += index; > @@ -3599,10 +3785,15 @@ int tc_setup_offload_action(struct flow_action *flow_action, > struct netlink_ext_ack *extack) > { > #ifdef CONFIG_NET_CLS_ACT > + u32 miss_cookie_base; > + > if (!exts) > return 0; > > - return tc_setup_action(flow_action, exts->actions, extack); > + miss_cookie_base = exts->miss_cookie_node ? > + exts->miss_cookie_node->miss_cookie_base : 0; > + return tc_setup_action(flow_action, exts->actions, miss_cookie_base, > + extack); > #else > return 0; > #endif > @@ -3770,6 +3961,8 @@ static int __init tc_filter_init(void) > if (err) > goto err_register_pernet_subsys; > > + xa_init_flags(&tcf_exts_miss_cookies_xa, XA_FLAGS_ALLOC1); > + > rtnl_register(PF_UNSPEC, RTM_NEWTFILTER, tc_new_tfilter, NULL, > RTNL_FLAG_DOIT_UNLOCKED); > rtnl_register(PF_UNSPEC, RTM_DELTFILTER, tc_del_tfilter, NULL, > -- > 2.30.1 >
On 10/02/2023 04:21, Marcelo Ricardo Leitner wrote: > On Mon, Feb 06, 2023 at 07:43:57PM +0200, Paul Blakey wrote: >> For drivers to support partial offload of a filter's action list, >> add support for action miss to specify an action instance to >> continue from in sw. >> >> CT action in particular can't be fully offloaded, as new connections >> need to be handled in software. This imposes other limitations on >> the actions that can be offloaded together with the CT action, such >> as packet modifications. >> >> Assign each action on a filter's action list a unique miss_cookie >> which drivers can then use to fill action_miss part of the tc skb >> extension. On getting back this miss_cookie, find the action >> instance with relevant cookie and continue classifying from there. >> >> Signed-off-by: Paul Blakey <paulb@nvidia.com> >> Reviewed-by: Jiri Pirko <jiri@nvidia.com> >> Reviewed-by: Simon Horman <simon.horman@corigine.com> >> Acked-by: Jamal Hadi Salim <jhs@mojatatu.com> >> --- >> include/linux/skbuff.h | 6 +- >> include/net/flow_offload.h | 1 + >> include/net/pkt_cls.h | 34 +++--- >> include/net/sch_generic.h | 2 + >> net/openvswitch/flow.c | 3 +- >> net/sched/act_api.c | 2 +- >> net/sched/cls_api.c | 213 +++++++++++++++++++++++++++++++++++-- >> 7 files changed, 234 insertions(+), 27 deletions(-) >> >> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h >> index 1fa95b916342..9b9aa854068f 100644 >> --- a/include/linux/skbuff.h >> +++ b/include/linux/skbuff.h >> @@ -311,12 +311,16 @@ struct nf_bridge_info { >> * and read by ovs to recirc_id. >> */ >> struct tc_skb_ext { >> - __u32 chain; >> + union { >> + u64 act_miss_cookie; >> + __u32 chain; >> + }; >> __u16 mru; >> __u16 zone; >> u8 post_ct:1; >> u8 post_ct_snat:1; >> u8 post_ct_dnat:1; >> + u8 act_miss:1; /* Set if act_miss_cookie is used */ >> }; >> #endif >> >> diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h >> index 0400a0ac8a29..88db7346eb7a 100644 >> --- a/include/net/flow_offload.h >> +++ b/include/net/flow_offload.h >> @@ -228,6 +228,7 @@ void flow_action_cookie_destroy(struct flow_action_cookie *cookie); >> struct flow_action_entry { >> enum flow_action_id id; >> u32 hw_index; >> + u64 miss_cookie; > > The per-action stats patchset is adding a cookie for the actions as > well, and exactly on this struct: > > @@ -228,6 +228,7 @@ struct flow_action_cookie *flow_action_cookie_create(void *data, > struct flow_action_entry { > enum flow_action_id id; > u32 hw_index; > + unsigned long act_cookie; > enum flow_action_hw_stats hw_stats; > action_destr destructor; > void *destructor_priv; > > There, it is a simple value: the act pointer itself. Here, it is already more > complex. Can them be merged into only one maybe? > If not, perhaps act_cookie should be renamed to stats_cookie then. I don't think it can be shared, actions can be shared between multiple filters, while the miss cookie would be different for each used instance (takes the filter in to account). So I'll rename it. > >> enum flow_action_hw_stats hw_stats; >> action_destr destructor; >> void *destructor_priv; >> diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h >> index cd410a87517b..e395f2a84ed2 100644 >> --- a/include/net/pkt_cls.h >> +++ b/include/net/pkt_cls.h >> @@ -59,6 +59,8 @@ int tcf_block_get_ext(struct tcf_block **p_block, struct Qdisc *q, >> void tcf_block_put(struct tcf_block *block); >> void tcf_block_put_ext(struct tcf_block *block, struct Qdisc *q, >> struct tcf_block_ext_info *ei); >> +int tcf_exts_init_ex(struct tcf_exts *exts, struct net *net, int action, >> + int police, struct tcf_proto *tp, u32 handle, bool used_action_miss); >> >> static inline bool tcf_block_shared(struct tcf_block *block) >> { >> @@ -229,6 +231,7 @@ struct tcf_exts { >> struct tc_action **actions; >> struct net *net; >> netns_tracker ns_tracker; >> + struct tcf_exts_miss_cookie_node *miss_cookie_node; >> #endif >> /* Map to export classifier specific extension TLV types to the >> * generic extensions API. Unsupported extensions must be set to 0. >> @@ -240,21 +243,11 @@ struct tcf_exts { >> static inline int tcf_exts_init(struct tcf_exts *exts, struct net *net, >> int action, int police) >> { >> -#ifdef CONFIG_NET_CLS_ACT >> - exts->type = 0; >> - exts->nr_actions = 0; >> - /* Note: we do not own yet a reference on net. >> - * This reference might be taken later from tcf_exts_get_net(). >> - */ >> - exts->net = net; >> - exts->actions = kcalloc(TCA_ACT_MAX_PRIO, sizeof(struct tc_action *), >> - GFP_KERNEL); >> - if (!exts->actions) >> - return -ENOMEM; >> +#ifdef CONFIG_NET_CLS >> + return tcf_exts_init_ex(exts, net, action, police, NULL, 0, false); >> +#else >> + return -EOPNOTSUPP; >> #endif >> - exts->action = action; >> - exts->police = police; >> - return 0; >> } >> >> /* Return false if the netns is being destroyed in cleanup_net(). Callers >> @@ -353,6 +346,18 @@ tcf_exts_exec(struct sk_buff *skb, struct tcf_exts *exts, >> return TC_ACT_OK; >> } >> >> +static inline int >> +tcf_exts_exec_ex(struct sk_buff *skb, struct tcf_exts *exts, int act_index, >> + struct tcf_result *res) >> +{ >> +#ifdef CONFIG_NET_CLS_ACT >> + return tcf_action_exec(skb, exts->actions + act_index, >> + exts->nr_actions - act_index, res); >> +#else >> + return TC_ACT_OK; >> +#endif >> +} >> + >> int tcf_exts_validate(struct net *net, struct tcf_proto *tp, >> struct nlattr **tb, struct nlattr *rate_tlv, >> struct tcf_exts *exts, u32 flags, >> @@ -577,6 +582,7 @@ int tc_setup_offload_action(struct flow_action *flow_action, >> void tc_cleanup_offload_action(struct flow_action *flow_action); >> int tc_setup_action(struct flow_action *flow_action, >> struct tc_action *actions[], >> + u32 miss_cookie_base, >> struct netlink_ext_ack *extack); >> >> int tc_setup_cb_call(struct tcf_block *block, enum tc_setup_type type, >> diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h >> index af4aa66aaa4e..fab5ba3e61b7 100644 >> --- a/include/net/sch_generic.h >> +++ b/include/net/sch_generic.h >> @@ -369,6 +369,8 @@ struct tcf_proto_ops { >> struct nlattr **tca, >> struct netlink_ext_ack *extack); >> void (*tmplt_destroy)(void *tmplt_priv); >> + struct tcf_exts * (*get_exts)(const struct tcf_proto *tp, >> + u32 handle); >> >> /* rtnetlink specific */ >> int (*dump)(struct net*, struct tcf_proto*, void *, >> diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c >> index e20d1a973417..69f91460a55c 100644 >> --- a/net/openvswitch/flow.c >> +++ b/net/openvswitch/flow.c >> @@ -1038,7 +1038,8 @@ int ovs_flow_key_extract(const struct ip_tunnel_info *tun_info, >> #if IS_ENABLED(CONFIG_NET_TC_SKB_EXT) >> if (tc_skb_ext_tc_enabled()) { >> tc_ext = skb_ext_find(skb, TC_SKB_EXT); >> - key->recirc_id = tc_ext ? tc_ext->chain : 0; >> + key->recirc_id = tc_ext && !tc_ext->act_miss ? >> + tc_ext->chain : 0; >> OVS_CB(skb)->mru = tc_ext ? tc_ext->mru : 0; >> post_ct = tc_ext ? tc_ext->post_ct : false; >> post_ct_snat = post_ct ? tc_ext->post_ct_snat : false; >> diff --git a/net/sched/act_api.c b/net/sched/act_api.c >> index cd09ef49df22..16fd3d30eb12 100644 >> --- a/net/sched/act_api.c >> +++ b/net/sched/act_api.c >> @@ -272,7 +272,7 @@ static int tcf_action_offload_add_ex(struct tc_action *action, >> if (err) >> goto fl_err; >> >> - err = tc_setup_action(&fl_action->action, actions, extack); >> + err = tc_setup_action(&fl_action->action, actions, 0, extack); >> if (err) { >> NL_SET_ERR_MSG_MOD(extack, >> "Failed to setup tc actions for offload"); >> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c >> index 5b4a95e8a1ee..8ff9530fef68 100644 >> --- a/net/sched/cls_api.c >> +++ b/net/sched/cls_api.c >> @@ -22,6 +22,7 @@ >> #include <linux/idr.h> >> #include <linux/jhash.h> >> #include <linux/rculist.h> >> +#include <linux/rhashtable.h> >> #include <net/net_namespace.h> >> #include <net/sock.h> >> #include <net/netlink.h> >> @@ -50,6 +51,109 @@ static LIST_HEAD(tcf_proto_base); >> /* Protects list of registered TC modules. It is pure SMP lock. */ >> static DEFINE_RWLOCK(cls_mod_lock); >> >> +static struct xarray tcf_exts_miss_cookies_xa; >> +struct tcf_exts_miss_cookie_node { >> + const struct tcf_chain *chain; >> + const struct tcf_proto *tp; >> + const struct tcf_exts *exts; >> + u32 chain_index; >> + u32 tp_prio; >> + u32 handle; >> + u32 miss_cookie_base; >> + struct rcu_head rcu; >> +}; >> + >> +/* Each tc action entry cookie will be comprised of 32bit miss_cookie_base + >> + * action index in the exts tc actions array. >> + */ >> +union tcf_exts_miss_cookie { >> + struct { >> + u32 miss_cookie_base; >> + u32 act_index; >> + }; >> + u64 miss_cookie; >> +}; >> + >> +#if IS_ENABLED(CONFIG_NET_TC_SKB_EXT) >> +static int >> +tcf_exts_miss_cookie_base_alloc(struct tcf_exts *exts, struct tcf_proto *tp, >> + u32 handle) >> +{ >> + struct tcf_exts_miss_cookie_node *n; >> + static u32 next; > > What protects this static variable from concurrent access? Nothing, but it acts as a suggestion for xa_alloc, so I don't think syncronzation is needed here. > >> + int err; >> + >> + if (WARN_ON(!handle || !tp->ops->get_exts)) >> + return -EINVAL; >> + >> + n = kzalloc(sizeof(*n), GFP_KERNEL); >> + if (!n) >> + return -ENOMEM; >> + >> + n->chain_index = tp->chain->index; >> + n->chain = tp->chain; >> + n->tp_prio = tp->prio; >> + n->tp = tp; >> + n->exts = exts; >> + n->handle = handle; >> + >> + err = xa_alloc_cyclic(&tcf_exts_miss_cookies_xa, &n->miss_cookie_base, >> + n, xa_limit_32b, &next, GFP_KERNEL); >> + if (err) >> + goto err_xa_alloc; >> + >> + exts->miss_cookie_node = n; >> + return 0; >> + >> +err_xa_alloc: >> + kfree(n); >> + return err; >> +} >> + >> +static void tcf_exts_miss_cookie_base_destroy(struct tcf_exts *exts) >> +{ >> + struct tcf_exts_miss_cookie_node *n; >> + >> + if (!exts->miss_cookie_node) >> + return; >> + >> + n = exts->miss_cookie_node; >> + xa_erase(&tcf_exts_miss_cookies_xa, n->miss_cookie_base); >> + kfree_rcu(n, rcu); >> +} >> + >> +static struct tcf_exts_miss_cookie_node * >> +tcf_exts_miss_cookie_lookup(u64 miss_cookie, int *act_index) >> +{ >> + union tcf_exts_miss_cookie mc = { .miss_cookie = miss_cookie, }; >> + >> + *act_index = mc.act_index; >> + return xa_load(&tcf_exts_miss_cookies_xa, mc.miss_cookie_base); >> +} >> +#else /* IS_ENABLED(CONFIG_NET_TC_SKB_EXT) */ >> +static int >> +tcf_exts_miss_cookie_base_alloc(struct tcf_exts *exts, struct tcf_proto *tp, >> + u32 handle) >> +{ >> + return 0; >> +} >> + >> +static void tcf_exts_miss_cookie_base_destroy(struct tcf_exts *exts) >> +{ >> +} >> +#endif /* IS_ENABLED(CONFIG_NET_TC_SKB_EXT) */ >> + >> +static u64 tcf_exts_miss_cookie_get(u32 miss_cookie_base, int act_index) >> +{ >> + union tcf_exts_miss_cookie mc = { .act_index = act_index, }; >> + >> + if (!miss_cookie_base) >> + return 0; >> + >> + mc.miss_cookie_base = miss_cookie_base; >> + return mc.miss_cookie; >> +} >> + >> #ifdef CONFIG_NET_CLS_ACT >> DEFINE_STATIC_KEY_FALSE(tc_skb_ext_tc); >> EXPORT_SYMBOL(tc_skb_ext_tc); >> @@ -1549,6 +1653,8 @@ static inline int __tcf_classify(struct sk_buff *skb, >> const struct tcf_proto *orig_tp, >> struct tcf_result *res, >> bool compat_mode, >> + struct tcf_exts_miss_cookie_node *n, >> + int act_index, >> u32 *last_executed_chain) >> { >> #ifdef CONFIG_NET_CLS_ACT >> @@ -1560,13 +1666,36 @@ static inline int __tcf_classify(struct sk_buff *skb, >> #endif >> for (; tp; tp = rcu_dereference_bh(tp->next)) { >> __be16 protocol = skb_protocol(skb, false); >> - int err; >> + int err = 0; >> >> - if (tp->protocol != protocol && >> - tp->protocol != htons(ETH_P_ALL)) >> - continue; >> + if (n) { >> + struct tcf_exts *exts; >> + >> + if (n->tp_prio != tp->prio) >> + continue; >> + >> + /* We re-lookup the tp and chain based on index instead >> + * of having hard refs and locks to them, so do a sanity >> + * check if any of tp,chain,exts was replaced by the >> + * time we got here with a cookie from hardware. >> + */ >> + if (unlikely(n->tp != tp || n->tp->chain != n->chain || >> + !tp->ops->get_exts)) >> + return TC_ACT_SHOT; >> + >> + exts = tp->ops->get_exts(tp, n->handle); >> + if (unlikely(!exts || n->exts != exts)) >> + return TC_ACT_SHOT; >> >> - err = tc_classify(skb, tp, res); >> + n = NULL; >> + err = tcf_exts_exec_ex(skb, exts, act_index, res); >> + } else { >> + if (tp->protocol != protocol && >> + tp->protocol != htons(ETH_P_ALL)) >> + continue; >> + >> + err = tc_classify(skb, tp, res); >> + } >> #ifdef CONFIG_NET_CLS_ACT >> if (unlikely(err == TC_ACT_RECLASSIFY && !compat_mode)) { >> first_tp = orig_tp; >> @@ -1582,6 +1711,9 @@ static inline int __tcf_classify(struct sk_buff *skb, >> return err; >> } >> >> + if (unlikely(n)) >> + return TC_ACT_SHOT; >> + >> return TC_ACT_UNSPEC; /* signal: continue lookup */ >> #ifdef CONFIG_NET_CLS_ACT >> reset: >> @@ -1606,21 +1738,33 @@ int tcf_classify(struct sk_buff *skb, >> #if !IS_ENABLED(CONFIG_NET_TC_SKB_EXT) >> u32 last_executed_chain = 0; >> >> - return __tcf_classify(skb, tp, tp, res, compat_mode, >> + return __tcf_classify(skb, tp, tp, res, compat_mode, NULL, 0, >> &last_executed_chain); >> #else >> u32 last_executed_chain = tp ? tp->chain->index : 0; >> + struct tcf_exts_miss_cookie_node *n = NULL; >> const struct tcf_proto *orig_tp = tp; >> struct tc_skb_ext *ext; >> + int act_index = 0; >> int ret; >> >> if (block) { >> ext = skb_ext_find(skb, TC_SKB_EXT); >> >> - if (ext && ext->chain) { >> + if (ext && (ext->chain || ext->act_miss)) { >> struct tcf_chain *fchain; >> + u32 chain = ext->chain; > > IMHO it would be nice to avoid this assignment here, because there is > nothing above saying that the union value can be read as a chain. For > C, yes, it is okay. The worry is about ensuring that whatever is > reading it, is reading what it is expecting it to be. > Perhaps just declare it here, and then: Right, not nice, will fix. > >> >> - fchain = tcf_chain_lookup_rcu(block, ext->chain); >> + if (ext->act_miss) { >> + n = tcf_exts_miss_cookie_lookup(ext->act_miss_cookie, >> + &act_index); >> + if (!n) >> + return TC_ACT_SHOT; >> + >> + chain = n->chain_index; >> + } > > else { > chain = ext->chain; > } > >> + >> + fchain = tcf_chain_lookup_rcu(block, chain); >> if (!fchain) >> return TC_ACT_SHOT; >> >> @@ -1632,7 +1776,7 @@ int tcf_classify(struct sk_buff *skb, >> } >> } >> >> - ret = __tcf_classify(skb, tp, orig_tp, res, compat_mode, >> + ret = __tcf_classify(skb, tp, orig_tp, res, compat_mode, n, act_index, >> &last_executed_chain); >> >> if (tc_skb_ext_tc_enabled()) { >> @@ -3056,9 +3200,48 @@ static int tc_dump_chain(struct sk_buff *skb, struct netlink_callback *cb) >> return skb->len; >> } >> >> +int tcf_exts_init_ex(struct tcf_exts *exts, struct net *net, int action, >> + int police, struct tcf_proto *tp, u32 handle, >> + bool use_action_miss) >> +{ >> + int err = 0; >> + >> +#ifdef CONFIG_NET_CLS_ACT >> + exts->type = 0; >> + exts->nr_actions = 0; >> + /* Note: we do not own yet a reference on net. >> + * This reference might be taken later from tcf_exts_get_net(). >> + */ >> + exts->net = net; >> + exts->actions = kcalloc(TCA_ACT_MAX_PRIO, sizeof(struct tc_action *), >> + GFP_KERNEL); >> + if (!exts->actions) >> + return -ENOMEM; >> +#endif >> + >> + exts->action = action; >> + exts->police = police; >> + >> + if (!use_action_miss) >> + return 0; >> + >> + err = tcf_exts_miss_cookie_base_alloc(exts, tp, handle); >> + if (err) >> + goto err_miss_alloc; >> + >> + return 0; >> + >> +err_miss_alloc: >> + tcf_exts_destroy(exts); >> + return err; >> +} >> +EXPORT_SYMBOL(tcf_exts_init_ex); >> + >> void tcf_exts_destroy(struct tcf_exts *exts) >> { >> #ifdef CONFIG_NET_CLS_ACT >> + tcf_exts_miss_cookie_base_destroy(exts); >> + >> if (exts->actions) { >> tcf_action_destroy(exts->actions, TCA_ACT_UNBIND); >> kfree(exts->actions); >> @@ -3547,6 +3730,7 @@ static int tc_setup_offload_act(struct tc_action *act, >> >> int tc_setup_action(struct flow_action *flow_action, >> struct tc_action *actions[], >> + u32 miss_cookie_base, >> struct netlink_ext_ack *extack) >> { >> int i, j, k, index, err = 0; >> @@ -3577,6 +3761,8 @@ int tc_setup_action(struct flow_action *flow_action, >> for (k = 0; k < index ; k++) { >> entry[k].hw_stats = tc_act_hw_stats(act->hw_stats); >> entry[k].hw_index = act->tcfa_index; >> + entry[k].miss_cookie = >> + tcf_exts_miss_cookie_get(miss_cookie_base, i); >> } >> >> j += index; >> @@ -3599,10 +3785,15 @@ int tc_setup_offload_action(struct flow_action *flow_action, >> struct netlink_ext_ack *extack) >> { >> #ifdef CONFIG_NET_CLS_ACT >> + u32 miss_cookie_base; >> + >> if (!exts) >> return 0; >> >> - return tc_setup_action(flow_action, exts->actions, extack); >> + miss_cookie_base = exts->miss_cookie_node ? >> + exts->miss_cookie_node->miss_cookie_base : 0; >> + return tc_setup_action(flow_action, exts->actions, miss_cookie_base, >> + extack); >> #else >> return 0; >> #endif >> @@ -3770,6 +3961,8 @@ static int __init tc_filter_init(void) >> if (err) >> goto err_register_pernet_subsys; >> >> + xa_init_flags(&tcf_exts_miss_cookies_xa, XA_FLAGS_ALLOC1); >> + >> rtnl_register(PF_UNSPEC, RTM_NEWTFILTER, tc_new_tfilter, NULL, >> RTNL_FLAG_DOIT_UNLOCKED); >> rtnl_register(PF_UNSPEC, RTM_DELTFILTER, tc_del_tfilter, NULL, >> -- >> 2.30.1 >>
On Mon, Feb 13, 2023 at 06:13:34PM +0200, Paul Blakey wrote: > > > On 10/02/2023 04:21, Marcelo Ricardo Leitner wrote: > > On Mon, Feb 06, 2023 at 07:43:57PM +0200, Paul Blakey wrote: > > > For drivers to support partial offload of a filter's action list, > > > add support for action miss to specify an action instance to > > > continue from in sw. > > > > > > CT action in particular can't be fully offloaded, as new connections > > > need to be handled in software. This imposes other limitations on > > > the actions that can be offloaded together with the CT action, such > > > as packet modifications. > > > > > > Assign each action on a filter's action list a unique miss_cookie > > > which drivers can then use to fill action_miss part of the tc skb > > > extension. On getting back this miss_cookie, find the action > > > instance with relevant cookie and continue classifying from there. > > > > > > Signed-off-by: Paul Blakey <paulb@nvidia.com> > > > Reviewed-by: Jiri Pirko <jiri@nvidia.com> > > > Reviewed-by: Simon Horman <simon.horman@corigine.com> > > > Acked-by: Jamal Hadi Salim <jhs@mojatatu.com> > > > --- > > > include/linux/skbuff.h | 6 +- > > > include/net/flow_offload.h | 1 + > > > include/net/pkt_cls.h | 34 +++--- > > > include/net/sch_generic.h | 2 + > > > net/openvswitch/flow.c | 3 +- > > > net/sched/act_api.c | 2 +- > > > net/sched/cls_api.c | 213 +++++++++++++++++++++++++++++++++++-- > > > 7 files changed, 234 insertions(+), 27 deletions(-) > > > > > > diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h > > > index 1fa95b916342..9b9aa854068f 100644 > > > --- a/include/linux/skbuff.h > > > +++ b/include/linux/skbuff.h > > > @@ -311,12 +311,16 @@ struct nf_bridge_info { > > > * and read by ovs to recirc_id. > > > */ > > > struct tc_skb_ext { > > > - __u32 chain; > > > + union { > > > + u64 act_miss_cookie; > > > + __u32 chain; > > > + }; > > > __u16 mru; > > > __u16 zone; > > > u8 post_ct:1; > > > u8 post_ct_snat:1; > > > u8 post_ct_dnat:1; > > > + u8 act_miss:1; /* Set if act_miss_cookie is used */ > > > }; > > > #endif > > > diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h > > > index 0400a0ac8a29..88db7346eb7a 100644 > > > --- a/include/net/flow_offload.h > > > +++ b/include/net/flow_offload.h > > > @@ -228,6 +228,7 @@ void flow_action_cookie_destroy(struct flow_action_cookie *cookie); > > > struct flow_action_entry { > > > enum flow_action_id id; > > > u32 hw_index; > > > + u64 miss_cookie; > > > > The per-action stats patchset is adding a cookie for the actions as > > well, and exactly on this struct: > > > > @@ -228,6 +228,7 @@ struct flow_action_cookie *flow_action_cookie_create(void *data, > > struct flow_action_entry { > > enum flow_action_id id; > > u32 hw_index; > > + unsigned long act_cookie; > > enum flow_action_hw_stats hw_stats; > > action_destr destructor; > > void *destructor_priv; > > > > There, it is a simple value: the act pointer itself. Here, it is already more > > complex. Can them be merged into only one maybe? > > If not, perhaps act_cookie should be renamed to stats_cookie then. > > I don't think it can be shared, actions can be shared between multiple > filters, while the miss cookie would be different for each used instance > (takes the filter in to account). Good point. So it would at best be a masked value that part A works for the miss here and part B for the stats, which is pretty much what the two cookies are giving, just without having to do bit gymnasics, yes. > > So I'll rename it. > > > > > > enum flow_action_hw_stats hw_stats; > > > action_destr destructor; > > > void *destructor_priv; > > > diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h > > > index cd410a87517b..e395f2a84ed2 100644 > > > --- a/include/net/pkt_cls.h > > > +++ b/include/net/pkt_cls.h > > > @@ -59,6 +59,8 @@ int tcf_block_get_ext(struct tcf_block **p_block, struct Qdisc *q, > > > void tcf_block_put(struct tcf_block *block); > > > void tcf_block_put_ext(struct tcf_block *block, struct Qdisc *q, > > > struct tcf_block_ext_info *ei); > > > +int tcf_exts_init_ex(struct tcf_exts *exts, struct net *net, int action, > > > + int police, struct tcf_proto *tp, u32 handle, bool used_action_miss); > > > static inline bool tcf_block_shared(struct tcf_block *block) > > > { > > > @@ -229,6 +231,7 @@ struct tcf_exts { > > > struct tc_action **actions; > > > struct net *net; > > > netns_tracker ns_tracker; > > > + struct tcf_exts_miss_cookie_node *miss_cookie_node; > > > #endif > > > /* Map to export classifier specific extension TLV types to the > > > * generic extensions API. Unsupported extensions must be set to 0. > > > @@ -240,21 +243,11 @@ struct tcf_exts { > > > static inline int tcf_exts_init(struct tcf_exts *exts, struct net *net, > > > int action, int police) > > > { > > > -#ifdef CONFIG_NET_CLS_ACT > > > - exts->type = 0; > > > - exts->nr_actions = 0; > > > - /* Note: we do not own yet a reference on net. > > > - * This reference might be taken later from tcf_exts_get_net(). > > > - */ > > > - exts->net = net; > > > - exts->actions = kcalloc(TCA_ACT_MAX_PRIO, sizeof(struct tc_action *), > > > - GFP_KERNEL); > > > - if (!exts->actions) > > > - return -ENOMEM; > > > +#ifdef CONFIG_NET_CLS > > > + return tcf_exts_init_ex(exts, net, action, police, NULL, 0, false); > > > +#else > > > + return -EOPNOTSUPP; > > > #endif > > > - exts->action = action; > > > - exts->police = police; > > > - return 0; > > > } > > > /* Return false if the netns is being destroyed in cleanup_net(). Callers > > > @@ -353,6 +346,18 @@ tcf_exts_exec(struct sk_buff *skb, struct tcf_exts *exts, > > > return TC_ACT_OK; > > > } > > > +static inline int > > > +tcf_exts_exec_ex(struct sk_buff *skb, struct tcf_exts *exts, int act_index, > > > + struct tcf_result *res) > > > +{ > > > +#ifdef CONFIG_NET_CLS_ACT > > > + return tcf_action_exec(skb, exts->actions + act_index, > > > + exts->nr_actions - act_index, res); > > > +#else > > > + return TC_ACT_OK; > > > +#endif > > > +} > > > + > > > int tcf_exts_validate(struct net *net, struct tcf_proto *tp, > > > struct nlattr **tb, struct nlattr *rate_tlv, > > > struct tcf_exts *exts, u32 flags, > > > @@ -577,6 +582,7 @@ int tc_setup_offload_action(struct flow_action *flow_action, > > > void tc_cleanup_offload_action(struct flow_action *flow_action); > > > int tc_setup_action(struct flow_action *flow_action, > > > struct tc_action *actions[], > > > + u32 miss_cookie_base, > > > struct netlink_ext_ack *extack); > > > int tc_setup_cb_call(struct tcf_block *block, enum tc_setup_type type, > > > diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h > > > index af4aa66aaa4e..fab5ba3e61b7 100644 > > > --- a/include/net/sch_generic.h > > > +++ b/include/net/sch_generic.h > > > @@ -369,6 +369,8 @@ struct tcf_proto_ops { > > > struct nlattr **tca, > > > struct netlink_ext_ack *extack); > > > void (*tmplt_destroy)(void *tmplt_priv); > > > + struct tcf_exts * (*get_exts)(const struct tcf_proto *tp, > > > + u32 handle); > > > /* rtnetlink specific */ > > > int (*dump)(struct net*, struct tcf_proto*, void *, > > > diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c > > > index e20d1a973417..69f91460a55c 100644 > > > --- a/net/openvswitch/flow.c > > > +++ b/net/openvswitch/flow.c > > > @@ -1038,7 +1038,8 @@ int ovs_flow_key_extract(const struct ip_tunnel_info *tun_info, > > > #if IS_ENABLED(CONFIG_NET_TC_SKB_EXT) > > > if (tc_skb_ext_tc_enabled()) { > > > tc_ext = skb_ext_find(skb, TC_SKB_EXT); > > > - key->recirc_id = tc_ext ? tc_ext->chain : 0; > > > + key->recirc_id = tc_ext && !tc_ext->act_miss ? > > > + tc_ext->chain : 0; > > > OVS_CB(skb)->mru = tc_ext ? tc_ext->mru : 0; > > > post_ct = tc_ext ? tc_ext->post_ct : false; > > > post_ct_snat = post_ct ? tc_ext->post_ct_snat : false; > > > diff --git a/net/sched/act_api.c b/net/sched/act_api.c > > > index cd09ef49df22..16fd3d30eb12 100644 > > > --- a/net/sched/act_api.c > > > +++ b/net/sched/act_api.c > > > @@ -272,7 +272,7 @@ static int tcf_action_offload_add_ex(struct tc_action *action, > > > if (err) > > > goto fl_err; > > > - err = tc_setup_action(&fl_action->action, actions, extack); > > > + err = tc_setup_action(&fl_action->action, actions, 0, extack); > > > if (err) { > > > NL_SET_ERR_MSG_MOD(extack, > > > "Failed to setup tc actions for offload"); > > > diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c > > > index 5b4a95e8a1ee..8ff9530fef68 100644 > > > --- a/net/sched/cls_api.c > > > +++ b/net/sched/cls_api.c > > > @@ -22,6 +22,7 @@ > > > #include <linux/idr.h> > > > #include <linux/jhash.h> > > > #include <linux/rculist.h> > > > +#include <linux/rhashtable.h> > > > #include <net/net_namespace.h> > > > #include <net/sock.h> > > > #include <net/netlink.h> > > > @@ -50,6 +51,109 @@ static LIST_HEAD(tcf_proto_base); > > > /* Protects list of registered TC modules. It is pure SMP lock. */ > > > static DEFINE_RWLOCK(cls_mod_lock); > > > +static struct xarray tcf_exts_miss_cookies_xa; > > > +struct tcf_exts_miss_cookie_node { > > > + const struct tcf_chain *chain; > > > + const struct tcf_proto *tp; > > > + const struct tcf_exts *exts; > > > + u32 chain_index; > > > + u32 tp_prio; > > > + u32 handle; > > > + u32 miss_cookie_base; > > > + struct rcu_head rcu; > > > +}; > > > + > > > +/* Each tc action entry cookie will be comprised of 32bit miss_cookie_base + > > > + * action index in the exts tc actions array. > > > + */ > > > +union tcf_exts_miss_cookie { > > > + struct { > > > + u32 miss_cookie_base; > > > + u32 act_index; > > > + }; > > > + u64 miss_cookie; > > > +}; > > > + > > > +#if IS_ENABLED(CONFIG_NET_TC_SKB_EXT) > > > +static int > > > +tcf_exts_miss_cookie_base_alloc(struct tcf_exts *exts, struct tcf_proto *tp, > > > + u32 handle) > > > +{ > > > + struct tcf_exts_miss_cookie_node *n; > > > + static u32 next; > > > > What protects this static variable from concurrent access? > > > Nothing, but it acts as a suggestion for xa_alloc, so I don't > think syncronzation is needed here. > > > > > > > + int err; > > > + > > > + if (WARN_ON(!handle || !tp->ops->get_exts)) > > > + return -EINVAL; > > > + > > > + n = kzalloc(sizeof(*n), GFP_KERNEL); > > > + if (!n) > > > + return -ENOMEM; > > > + > > > + n->chain_index = tp->chain->index; > > > + n->chain = tp->chain; > > > + n->tp_prio = tp->prio; > > > + n->tp = tp; > > > + n->exts = exts; > > > + n->handle = handle; > > > + > > > + err = xa_alloc_cyclic(&tcf_exts_miss_cookies_xa, &n->miss_cookie_base, > > > + n, xa_limit_32b, &next, GFP_KERNEL); > > > + if (err) > > > + goto err_xa_alloc; > > > + > > > + exts->miss_cookie_node = n; > > > + return 0; > > > + > > > +err_xa_alloc: > > > + kfree(n); > > > + return err; > > > +} > > > + > > > +static void tcf_exts_miss_cookie_base_destroy(struct tcf_exts *exts) > > > +{ > > > + struct tcf_exts_miss_cookie_node *n; > > > + > > > + if (!exts->miss_cookie_node) > > > + return; > > > + > > > + n = exts->miss_cookie_node; > > > + xa_erase(&tcf_exts_miss_cookies_xa, n->miss_cookie_base); > > > + kfree_rcu(n, rcu); > > > +} > > > + > > > +static struct tcf_exts_miss_cookie_node * > > > +tcf_exts_miss_cookie_lookup(u64 miss_cookie, int *act_index) > > > +{ > > > + union tcf_exts_miss_cookie mc = { .miss_cookie = miss_cookie, }; > > > + > > > + *act_index = mc.act_index; > > > + return xa_load(&tcf_exts_miss_cookies_xa, mc.miss_cookie_base); > > > +} > > > +#else /* IS_ENABLED(CONFIG_NET_TC_SKB_EXT) */ > > > +static int > > > +tcf_exts_miss_cookie_base_alloc(struct tcf_exts *exts, struct tcf_proto *tp, > > > + u32 handle) > > > +{ > > > + return 0; > > > +} > > > + > > > +static void tcf_exts_miss_cookie_base_destroy(struct tcf_exts *exts) > > > +{ > > > +} > > > +#endif /* IS_ENABLED(CONFIG_NET_TC_SKB_EXT) */ > > > + > > > +static u64 tcf_exts_miss_cookie_get(u32 miss_cookie_base, int act_index) > > > +{ > > > + union tcf_exts_miss_cookie mc = { .act_index = act_index, }; > > > + > > > + if (!miss_cookie_base) > > > + return 0; > > > + > > > + mc.miss_cookie_base = miss_cookie_base; > > > + return mc.miss_cookie; > > > +} > > > + > > > #ifdef CONFIG_NET_CLS_ACT > > > DEFINE_STATIC_KEY_FALSE(tc_skb_ext_tc); > > > EXPORT_SYMBOL(tc_skb_ext_tc); > > > @@ -1549,6 +1653,8 @@ static inline int __tcf_classify(struct sk_buff *skb, > > > const struct tcf_proto *orig_tp, > > > struct tcf_result *res, > > > bool compat_mode, > > > + struct tcf_exts_miss_cookie_node *n, > > > + int act_index, > > > u32 *last_executed_chain) > > > { > > > #ifdef CONFIG_NET_CLS_ACT > > > @@ -1560,13 +1666,36 @@ static inline int __tcf_classify(struct sk_buff *skb, > > > #endif > > > for (; tp; tp = rcu_dereference_bh(tp->next)) { > > > __be16 protocol = skb_protocol(skb, false); > > > - int err; > > > + int err = 0; > > > - if (tp->protocol != protocol && > > > - tp->protocol != htons(ETH_P_ALL)) > > > - continue; > > > + if (n) { > > > + struct tcf_exts *exts; > > > + > > > + if (n->tp_prio != tp->prio) > > > + continue; > > > + > > > + /* We re-lookup the tp and chain based on index instead > > > + * of having hard refs and locks to them, so do a sanity > > > + * check if any of tp,chain,exts was replaced by the > > > + * time we got here with a cookie from hardware. > > > + */ > > > + if (unlikely(n->tp != tp || n->tp->chain != n->chain || > > > + !tp->ops->get_exts)) > > > + return TC_ACT_SHOT; > > > + > > > + exts = tp->ops->get_exts(tp, n->handle); > > > + if (unlikely(!exts || n->exts != exts)) > > > + return TC_ACT_SHOT; > > > - err = tc_classify(skb, tp, res); > > > + n = NULL; > > > + err = tcf_exts_exec_ex(skb, exts, act_index, res); > > > + } else { > > > + if (tp->protocol != protocol && > > > + tp->protocol != htons(ETH_P_ALL)) > > > + continue; > > > + > > > + err = tc_classify(skb, tp, res); > > > + } > > > #ifdef CONFIG_NET_CLS_ACT > > > if (unlikely(err == TC_ACT_RECLASSIFY && !compat_mode)) { > > > first_tp = orig_tp; > > > @@ -1582,6 +1711,9 @@ static inline int __tcf_classify(struct sk_buff *skb, > > > return err; > > > } > > > + if (unlikely(n)) > > > + return TC_ACT_SHOT; > > > + > > > return TC_ACT_UNSPEC; /* signal: continue lookup */ > > > #ifdef CONFIG_NET_CLS_ACT > > > reset: > > > @@ -1606,21 +1738,33 @@ int tcf_classify(struct sk_buff *skb, > > > #if !IS_ENABLED(CONFIG_NET_TC_SKB_EXT) > > > u32 last_executed_chain = 0; > > > - return __tcf_classify(skb, tp, tp, res, compat_mode, > > > + return __tcf_classify(skb, tp, tp, res, compat_mode, NULL, 0, > > > &last_executed_chain); > > > #else > > > u32 last_executed_chain = tp ? tp->chain->index : 0; > > > + struct tcf_exts_miss_cookie_node *n = NULL; > > > const struct tcf_proto *orig_tp = tp; > > > struct tc_skb_ext *ext; > > > + int act_index = 0; > > > int ret; > > > if (block) { > > > ext = skb_ext_find(skb, TC_SKB_EXT); > > > - if (ext && ext->chain) { > > > + if (ext && (ext->chain || ext->act_miss)) { > > > struct tcf_chain *fchain; > > > + u32 chain = ext->chain; > > > > IMHO it would be nice to avoid this assignment here, because there is > > nothing above saying that the union value can be read as a chain. For > > C, yes, it is okay. The worry is about ensuring that whatever is > > reading it, is reading what it is expecting it to be. > > Perhaps just declare it here, and then: > > Right, not nice, will fix. > > > > > > - fchain = tcf_chain_lookup_rcu(block, ext->chain); > > > + if (ext->act_miss) { > > > + n = tcf_exts_miss_cookie_lookup(ext->act_miss_cookie, > > > + &act_index); > > > + if (!n) > > > + return TC_ACT_SHOT; > > > + > > > + chain = n->chain_index; > > > + } > > > > else { > > chain = ext->chain; > > } > > > > > + > > > + fchain = tcf_chain_lookup_rcu(block, chain); > > > if (!fchain) > > > return TC_ACT_SHOT; > > > @@ -1632,7 +1776,7 @@ int tcf_classify(struct sk_buff *skb, > > > } > > > } > > > - ret = __tcf_classify(skb, tp, orig_tp, res, compat_mode, > > > + ret = __tcf_classify(skb, tp, orig_tp, res, compat_mode, n, act_index, > > > &last_executed_chain); > > > if (tc_skb_ext_tc_enabled()) { > > > @@ -3056,9 +3200,48 @@ static int tc_dump_chain(struct sk_buff *skb, struct netlink_callback *cb) > > > return skb->len; > > > } > > > +int tcf_exts_init_ex(struct tcf_exts *exts, struct net *net, int action, > > > + int police, struct tcf_proto *tp, u32 handle, > > > + bool use_action_miss) > > > +{ > > > + int err = 0; > > > + > > > +#ifdef CONFIG_NET_CLS_ACT > > > + exts->type = 0; > > > + exts->nr_actions = 0; > > > + /* Note: we do not own yet a reference on net. > > > + * This reference might be taken later from tcf_exts_get_net(). > > > + */ > > > + exts->net = net; > > > + exts->actions = kcalloc(TCA_ACT_MAX_PRIO, sizeof(struct tc_action *), > > > + GFP_KERNEL); > > > + if (!exts->actions) > > > + return -ENOMEM; > > > +#endif > > > + > > > + exts->action = action; > > > + exts->police = police; > > > + > > > + if (!use_action_miss) > > > + return 0; > > > + > > > + err = tcf_exts_miss_cookie_base_alloc(exts, tp, handle); > > > + if (err) > > > + goto err_miss_alloc; > > > + > > > + return 0; > > > + > > > +err_miss_alloc: > > > + tcf_exts_destroy(exts); > > > + return err; > > > +} > > > +EXPORT_SYMBOL(tcf_exts_init_ex); > > > + > > > void tcf_exts_destroy(struct tcf_exts *exts) > > > { > > > #ifdef CONFIG_NET_CLS_ACT > > > + tcf_exts_miss_cookie_base_destroy(exts); > > > + > > > if (exts->actions) { > > > tcf_action_destroy(exts->actions, TCA_ACT_UNBIND); > > > kfree(exts->actions); > > > @@ -3547,6 +3730,7 @@ static int tc_setup_offload_act(struct tc_action *act, > > > int tc_setup_action(struct flow_action *flow_action, > > > struct tc_action *actions[], > > > + u32 miss_cookie_base, > > > struct netlink_ext_ack *extack) > > > { > > > int i, j, k, index, err = 0; > > > @@ -3577,6 +3761,8 @@ int tc_setup_action(struct flow_action *flow_action, > > > for (k = 0; k < index ; k++) { > > > entry[k].hw_stats = tc_act_hw_stats(act->hw_stats); > > > entry[k].hw_index = act->tcfa_index; > > > + entry[k].miss_cookie = > > > + tcf_exts_miss_cookie_get(miss_cookie_base, i); > > > } > > > j += index; > > > @@ -3599,10 +3785,15 @@ int tc_setup_offload_action(struct flow_action *flow_action, > > > struct netlink_ext_ack *extack) > > > { > > > #ifdef CONFIG_NET_CLS_ACT > > > + u32 miss_cookie_base; > > > + > > > if (!exts) > > > return 0; > > > - return tc_setup_action(flow_action, exts->actions, extack); > > > + miss_cookie_base = exts->miss_cookie_node ? > > > + exts->miss_cookie_node->miss_cookie_base : 0; > > > + return tc_setup_action(flow_action, exts->actions, miss_cookie_base, > > > + extack); > > > #else > > > return 0; > > > #endif > > > @@ -3770,6 +3961,8 @@ static int __init tc_filter_init(void) > > > if (err) > > > goto err_register_pernet_subsys; > > > + xa_init_flags(&tcf_exts_miss_cookies_xa, XA_FLAGS_ALLOC1); > > > + > > > rtnl_register(PF_UNSPEC, RTM_NEWTFILTER, tc_new_tfilter, NULL, > > > RTNL_FLAG_DOIT_UNLOCKED); > > > rtnl_register(PF_UNSPEC, RTM_DELTFILTER, tc_del_tfilter, NULL, > > > -- > > > 2.30.1 > > >
On 13/02/2023 20:43, Marcelo Ricardo Leitner wrote: > On Mon, Feb 13, 2023 at 06:13:34PM +0200, Paul Blakey wrote: >> >> On 10/02/2023 04:21, Marcelo Ricardo Leitner wrote: >>> On Mon, Feb 06, 2023 at 07:43:57PM +0200, Paul Blakey wrote: >>>> For drivers to support partial offload of a filter's action list, >>>> add support for action miss to specify an action instance to >>>> continue from in sw. >>>> >>>> CT action in particular can't be fully offloaded, as new connections >>>> need to be handled in software. This imposes other limitations on >>>> the actions that can be offloaded together with the CT action, such >>>> as packet modifications. >>>> >>>> Assign each action on a filter's action list a unique miss_cookie >>>> which drivers can then use to fill action_miss part of the tc skb >>>> extension. On getting back this miss_cookie, find the action >>>> instance with relevant cookie and continue classifying from there. >>>> >>>> Signed-off-by: Paul Blakey <paulb@nvidia.com> >>>> Reviewed-by: Jiri Pirko <jiri@nvidia.com> >>>> Reviewed-by: Simon Horman <simon.horman@corigine.com> >>>> Acked-by: Jamal Hadi Salim <jhs@mojatatu.com> >>>> --- >>>> include/linux/skbuff.h | 6 +- >>>> include/net/flow_offload.h | 1 + >>>> include/net/pkt_cls.h | 34 +++--- >>>> include/net/sch_generic.h | 2 + >>>> net/openvswitch/flow.c | 3 +- >>>> net/sched/act_api.c | 2 +- >>>> net/sched/cls_api.c | 213 +++++++++++++++++++++++++++++++++++-- >>>> 7 files changed, 234 insertions(+), 27 deletions(-) >>>> >>>> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h >>>> index 1fa95b916342..9b9aa854068f 100644 >>>> --- a/include/linux/skbuff.h >>>> +++ b/include/linux/skbuff.h >>>> @@ -311,12 +311,16 @@ struct nf_bridge_info { >>>> * and read by ovs to recirc_id. >>>> */ >>>> struct tc_skb_ext { >>>> - __u32 chain; >>>> + union { >>>> + u64 act_miss_cookie; >>>> + __u32 chain; >>>> + }; >>>> __u16 mru; >>>> __u16 zone; >>>> u8 post_ct:1; >>>> u8 post_ct_snat:1; >>>> u8 post_ct_dnat:1; >>>> + u8 act_miss:1; /* Set if act_miss_cookie is used */ >>>> }; >>>> #endif >>>> diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h >>>> index 0400a0ac8a29..88db7346eb7a 100644 >>>> --- a/include/net/flow_offload.h >>>> +++ b/include/net/flow_offload.h >>>> @@ -228,6 +228,7 @@ void flow_action_cookie_destroy(struct flow_action_cookie *cookie); >>>> struct flow_action_entry { >>>> enum flow_action_id id; >>>> u32 hw_index; >>>> + u64 miss_cookie; >>> The per-action stats patchset is adding a cookie for the actions as >>> well, and exactly on this struct: >>> >>> @@ -228,6 +228,7 @@ struct flow_action_cookie *flow_action_cookie_create(void *data, >>> struct flow_action_entry { >>> enum flow_action_id id; >>> u32 hw_index; >>> + unsigned long act_cookie; >>> enum flow_action_hw_stats hw_stats; >>> action_destr destructor; >>> void *destructor_priv; >>> >>> There, it is a simple value: the act pointer itself. Here, it is already more >>> complex. Can them be merged into only one maybe? >>> If not, perhaps act_cookie should be renamed to stats_cookie then. >> I don't think it can be shared, actions can be shared between multiple >> filters, while the miss cookie would be different for each used instance >> (takes the filter in to account). > Good point. So it would at best be a masked value that part A works > for the miss here and part B for the stats, which is pretty much what > the two cookies are giving, just without having to do bit gymnasics, > yes. act cookie is using 64 bits (to store the pointer and void a mapping), and I'm using at least 32bits, so there is not simple type that will contain both. So I'll rename the act_cookie to stats_cookie once I rebase.
On 14/02/2023 14:14, Paul Blakey wrote: > On 13/02/2023 20:43, Marcelo Ricardo Leitner wrote: >> On Mon, Feb 13, 2023 at 06:13:34PM +0200, Paul Blakey wrote: >>> On 10/02/2023 04:21, Marcelo Ricardo Leitner wrote: >>>> On Mon, Feb 06, 2023 at 07:43:57PM +0200, Paul Blakey wrote: >>>>> For drivers to support partial offload of a filter's action list, >>>>> add support for action miss to specify an action instance to >>>>> continue from in sw. >>>>> >>>>> CT action in particular can't be fully offloaded, as new connections >>>>> need to be handled in software. This imposes other limitations on >>>>> the actions that can be offloaded together with the CT action, such >>>>> as packet modifications. >>>>> >>>>> Assign each action on a filter's action list a unique miss_cookie >>>>> which drivers can then use to fill action_miss part of the tc skb >>>>> extension. On getting back this miss_cookie, find the action >>>>> instance with relevant cookie and continue classifying from there. >>>>> >>>>> Signed-off-by: Paul Blakey <paulb@nvidia.com> >>>>> Reviewed-by: Jiri Pirko <jiri@nvidia.com> >>>>> Reviewed-by: Simon Horman <simon.horman@corigine.com> >>>>> Acked-by: Jamal Hadi Salim <jhs@mojatatu.com> >>>>> --- >>>>> include/linux/skbuff.h | 6 +- >>>>> include/net/flow_offload.h | 1 + >>>>> include/net/pkt_cls.h | 34 +++--- >>>>> include/net/sch_generic.h | 2 + >>>>> net/openvswitch/flow.c | 3 +- >>>>> net/sched/act_api.c | 2 +- >>>>> net/sched/cls_api.c | 213 +++++++++++++++++++++++++++++++++++-- >>>>> 7 files changed, 234 insertions(+), 27 deletions(-) >>>>> >>>>> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h >>>>> index 1fa95b916342..9b9aa854068f 100644 >>>>> --- a/include/linux/skbuff.h >>>>> +++ b/include/linux/skbuff.h >>>>> @@ -311,12 +311,16 @@ struct nf_bridge_info { >>>>> * and read by ovs to recirc_id. >>>>> */ >>>>> struct tc_skb_ext { >>>>> - __u32 chain; >>>>> + union { >>>>> + u64 act_miss_cookie; >>>>> + __u32 chain; >>>>> + }; >>>>> __u16 mru; >>>>> __u16 zone; >>>>> u8 post_ct:1; >>>>> u8 post_ct_snat:1; >>>>> u8 post_ct_dnat:1; >>>>> + u8 act_miss:1; /* Set if act_miss_cookie is used */ >>>>> }; >>>>> #endif >>>>> diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h >>>>> index 0400a0ac8a29..88db7346eb7a 100644 >>>>> --- a/include/net/flow_offload.h >>>>> +++ b/include/net/flow_offload.h >>>>> @@ -228,6 +228,7 @@ void flow_action_cookie_destroy(struct flow_action_cookie *cookie); >>>>> struct flow_action_entry { >>>>> enum flow_action_id id; >>>>> u32 hw_index; >>>>> + u64 miss_cookie; >>>> The per-action stats patchset is adding a cookie for the actions as >>>> well, and exactly on this struct: >>>> >>>> @@ -228,6 +228,7 @@ struct flow_action_cookie *flow_action_cookie_create(void *data, >>>> struct flow_action_entry { >>>> enum flow_action_id id; >>>> u32 hw_index; >>>> + unsigned long act_cookie; >>>> enum flow_action_hw_stats hw_stats; >>>> action_destr destructor; >>>> void *destructor_priv; >>>> >>>> There, it is a simple value: the act pointer itself. Here, it is already more >>>> complex. Can them be merged into only one maybe? >>>> If not, perhaps act_cookie should be renamed to stats_cookie then. >>> I don't think it can be shared, actions can be shared between multiple >>> filters, while the miss cookie would be different for each used instance >>> (takes the filter in to account). >> Good point. So it would at best be a masked value that part A works >> for the miss here and part B for the stats, which is pretty much what >> the two cookies are giving, just without having to do bit gymnasics, >> yes. > act cookie is using 64 bits (to store the pointer and void a mapping), and I'm using at least > > 32bits, so there is not simple type that will contain both. > > So I'll rename the act_cookie to stats_cookie once I rebase. > The current act_cookie uniquely identifies the action instance. I think it might be used in other use cases and not just for stats. Actually, I think the current naming scheme of act_cookie and miss_cookie makes sense.
On Tue, Feb 14, 2023 at 02:31:06PM +0200, Oz Shlomo wrote: > > On 14/02/2023 14:14, Paul Blakey wrote: > > On 13/02/2023 20:43, Marcelo Ricardo Leitner wrote: > > > On Mon, Feb 13, 2023 at 06:13:34PM +0200, Paul Blakey wrote: > > > > On 10/02/2023 04:21, Marcelo Ricardo Leitner wrote: > > > > > On Mon, Feb 06, 2023 at 07:43:57PM +0200, Paul Blakey wrote: > > > > > > For drivers to support partial offload of a filter's action list, > > > > > > add support for action miss to specify an action instance to > > > > > > continue from in sw. > > > > > > > > > > > > CT action in particular can't be fully offloaded, as new connections > > > > > > need to be handled in software. This imposes other limitations on > > > > > > the actions that can be offloaded together with the CT action, such > > > > > > as packet modifications. > > > > > > > > > > > > Assign each action on a filter's action list a unique miss_cookie > > > > > > which drivers can then use to fill action_miss part of the tc skb > > > > > > extension. On getting back this miss_cookie, find the action > > > > > > instance with relevant cookie and continue classifying from there. > > > > > > > > > > > > Signed-off-by: Paul Blakey <paulb@nvidia.com> > > > > > > Reviewed-by: Jiri Pirko <jiri@nvidia.com> > > > > > > Reviewed-by: Simon Horman <simon.horman@corigine.com> > > > > > > Acked-by: Jamal Hadi Salim <jhs@mojatatu.com> > > > > > > --- > > > > > > include/linux/skbuff.h | 6 +- > > > > > > include/net/flow_offload.h | 1 + > > > > > > include/net/pkt_cls.h | 34 +++--- > > > > > > include/net/sch_generic.h | 2 + > > > > > > net/openvswitch/flow.c | 3 +- > > > > > > net/sched/act_api.c | 2 +- > > > > > > net/sched/cls_api.c | 213 +++++++++++++++++++++++++++++++++++-- > > > > > > 7 files changed, 234 insertions(+), 27 deletions(-) > > > > > > > > > > > > diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h > > > > > > index 1fa95b916342..9b9aa854068f 100644 > > > > > > --- a/include/linux/skbuff.h > > > > > > +++ b/include/linux/skbuff.h > > > > > > @@ -311,12 +311,16 @@ struct nf_bridge_info { > > > > > > * and read by ovs to recirc_id. > > > > > > */ > > > > > > struct tc_skb_ext { > > > > > > - __u32 chain; > > > > > > + union { > > > > > > + u64 act_miss_cookie; > > > > > > + __u32 chain; > > > > > > + }; > > > > > > __u16 mru; > > > > > > __u16 zone; > > > > > > u8 post_ct:1; > > > > > > u8 post_ct_snat:1; > > > > > > u8 post_ct_dnat:1; > > > > > > + u8 act_miss:1; /* Set if act_miss_cookie is used */ > > > > > > }; > > > > > > #endif > > > > > > diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h > > > > > > index 0400a0ac8a29..88db7346eb7a 100644 > > > > > > --- a/include/net/flow_offload.h > > > > > > +++ b/include/net/flow_offload.h > > > > > > @@ -228,6 +228,7 @@ void flow_action_cookie_destroy(struct flow_action_cookie *cookie); > > > > > > struct flow_action_entry { > > > > > > enum flow_action_id id; > > > > > > u32 hw_index; > > > > > > + u64 miss_cookie; > > > > > The per-action stats patchset is adding a cookie for the actions as > > > > > well, and exactly on this struct: > > > > > > > > > > @@ -228,6 +228,7 @@ struct flow_action_cookie *flow_action_cookie_create(void *data, > > > > > struct flow_action_entry { > > > > > enum flow_action_id id; > > > > > u32 hw_index; > > > > > + unsigned long act_cookie; > > > > > enum flow_action_hw_stats hw_stats; > > > > > action_destr destructor; > > > > > void *destructor_priv; > > > > > > > > > > There, it is a simple value: the act pointer itself. Here, it is already more > > > > > complex. Can them be merged into only one maybe? > > > > > If not, perhaps act_cookie should be renamed to stats_cookie then. > > > > I don't think it can be shared, actions can be shared between multiple > > > > filters, while the miss cookie would be different for each used instance > > > > (takes the filter in to account). > > > Good point. So it would at best be a masked value that part A works > > > for the miss here and part B for the stats, which is pretty much what > > > the two cookies are giving, just without having to do bit gymnasics, > > > yes. > > act cookie is using 64 bits (to store the pointer and void a mapping), and I'm using at least > > > > 32bits, so there is not simple type that will contain both. > > > > So I'll rename the act_cookie to stats_cookie once I rebase. > > > The current act_cookie uniquely identifies the action instance. > While miss_cookie also uniquely identifies the action, within a filter list. Or, miss_cookie uniquely identifies an action configuration. > I think it might be used in other use cases and not just for stats. I don't disagree. > > Actually, I think the current naming scheme of act_cookie and miss_cookie > makes sense. act_cookie is actually already used (and differently) in tc_action itself here: 1045ba77a596 ("net sched actions: Add support for user cookies") With this, IMO it is tough to know which one takes precedence where. Then perhaps, act_cookie here -> instance_cookie miss_cookie -> config_cookie Sorry for the bikeshedding, btw, but these cookies are getting confusing. We need them to taste nice :-}
On 14/02/2023 18:48, Marcelo Ricardo Leitner wrote: > On Tue, Feb 14, 2023 at 02:31:06PM +0200, Oz Shlomo wrote: >> Actually, I think the current naming scheme of act_cookie and miss_cookie >> makes sense. > > Then perhaps, > act_cookie here -> instance_cookie > miss_cookie -> config_cookie > > Sorry for the bikeshedding, btw, but these cookies are getting > confusing. We need them to taste nice :-} I'm with Oz, keep the current name for act_cookie. (In my ideal world, it'd just be called cookie, and the existing cookie in struct flow_action_entry would be renamed user_cookie. Because act_cookie is the same thing conceptually as flow_cls_offload.cookie. Though I wonder if that means it belongs in struct flow_offload_action instead?) -ed
On 14/02/2023 21:24, Edward Cree wrote: > On 14/02/2023 18:48, Marcelo Ricardo Leitner wrote: >> On Tue, Feb 14, 2023 at 02:31:06PM +0200, Oz Shlomo wrote: >>> Actually, I think the current naming scheme of act_cookie and miss_cookie >>> makes sense. >> >> Then perhaps, >> act_cookie here -> instance_cookie >> miss_cookie -> config_cookie >> >> Sorry for the bikeshedding, btw, but these cookies are getting >> confusing. We need them to taste nice :-} > > I'm with Oz, keep the current name for act_cookie. > > (In my ideal world, it'd just be called cookie, and the existing > cookie in struct flow_action_entry would be renamed user_cookie. > Because act_cookie is the same thing conceptually as > flow_cls_offload.cookie. Though I wonder if that means it > belongs in struct flow_offload_action instead?) > > -ed Ok so I want to add this patch to the series: From 326938812758dbd2591b221452708504911ca419 Mon Sep 17 00:00:00 2001 From: Paul Blakey <paulb@nvidia.com> Date: Wed, 15 Feb 2023 10:57:40 +0200 Subject: [PATCH] net: sched: Rename user cookie and act cookie struct tc_action->act_cookie is a user defined cookie, and the related struct flow_action_entry->act_cookie is used as an handle similar to struct flow_cls_offload->cookie. Rename tc_action->act_cookie to user_cookie, and flow_action_entry->act_cookie to cookie so their names would better fit their usage. Issue: 3226890 Change-Id: I3cfff2323f50234250e510062fd27307b6aa1896 Signed-off-by: Paul Blakey <paulb@nvidia.com> --- diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c index 2d06b44..208809a 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c @@ -4180,7 +4180,7 @@ parse_state->actions |= attr->action; if (!tc_act->stats_action) - attr->tc_act_cookies[attr->tc_act_cookies_count++] = act->act_cookie; + attr->tc_act_cookies[attr->tc_act_cookies_count++] = act->cookie; /* Split attr for multi table act if not the last act. */ if (jump_state.jump_target || diff --git a/include/net/act_api.h b/include/net/act_api.h index 2a6f443..4ae0580 100644 --- a/include/net/act_api.h +++ b/include/net/act_api.h @@ -39,7 +39,7 @@ struct gnet_stats_basic_sync __percpu *cpu_bstats; struct gnet_stats_basic_sync __percpu *cpu_bstats_hw; struct gnet_stats_queue __percpu *cpu_qstats; - struct tc_cookie __rcu *act_cookie; + struct tc_cookie __rcu *user_cookie; struct tcf_chain __rcu *goto_chain; u32 tcfa_flags; u8 hw_stats; diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h index 8c05455..9c5cb12 100644 --- a/include/net/flow_offload.h +++ b/include/net/flow_offload.h @@ -228,7 +228,7 @@ struct flow_action_entry { enum flow_action_id id; u32 hw_index; - unsigned long act_cookie; + unsigned long cookie; enum flow_action_hw_stats hw_stats; action_destr destructor; void *destructor_priv; @@ -321,7 +321,7 @@ u16 sid; } pppoe; }; - struct flow_action_cookie *cookie; /* user defined action cookie */ + struct flow_action_cookie *user_cookie; /* user defined action cookie */ }; struct flow_action { diff --git a/net/sched/act_api.c b/net/sched/act_api.c index eda58b7..e67ebc9 100644 --- a/net/sched/act_api.c +++ b/net/sched/act_api.c @@ -125,7 +125,7 @@ free_percpu(p->cpu_bstats_hw); free_percpu(p->cpu_qstats); - tcf_set_action_cookie(&p->act_cookie, NULL); + tcf_set_action_cookie(&p->user_cookie, NULL); if (chain) tcf_chain_put_by_act(chain); @@ -431,14 +431,14 @@ static size_t tcf_action_shared_attrs_size(const struct tc_action *act) { - struct tc_cookie *act_cookie; + struct tc_cookie *user_cookie; u32 cookie_len = 0; rcu_read_lock(); - act_cookie = rcu_dereference(act->act_cookie); + user_cookie = rcu_dereference(act->user_cookie); - if (act_cookie) - cookie_len = nla_total_size(act_cookie->len); + if (user_cookie) + cookie_len = nla_total_size(user_cookie->len); rcu_read_unlock(); return nla_total_size(0) /* action number nested */ @@ -488,7 +488,7 @@ goto nla_put_failure; rcu_read_lock(); - cookie = rcu_dereference(a->act_cookie); + cookie = rcu_dereference(a->user_cookie); if (cookie) { if (nla_put(skb, TCA_ACT_COOKIE, cookie->len, cookie->data)) { rcu_read_unlock(); @@ -1362,9 +1362,9 @@ { bool police = flags & TCA_ACT_FLAGS_POLICE; struct nla_bitfield32 userflags = { 0, 0 }; + struct tc_cookie *user_cookie = NULL; u8 hw_stats = TCA_ACT_HW_STATS_ANY; struct nlattr *tb[TCA_ACT_MAX + 1]; - struct tc_cookie *cookie = NULL; struct tc_action *a; int err; @@ -1375,8 +1375,8 @@ if (err < 0) return ERR_PTR(err); if (tb[TCA_ACT_COOKIE]) { - cookie = nla_memdup_cookie(tb); - if (!cookie) { + user_cookie = nla_memdup_cookie(tb); + if (!user_cookie) { NL_SET_ERR_MSG(extack, "No memory to generate TC cookie"); err = -ENOMEM; goto err_out; @@ -1402,7 +1402,7 @@ *init_res = err; if (!police && tb[TCA_ACT_COOKIE]) - tcf_set_action_cookie(&a->act_cookie, cookie); + tcf_set_action_cookie(&a->user_cookie, user_cookie); if (!police) a->hw_stats = hw_stats; @@ -1410,9 +1410,9 @@ return a; err_out: - if (cookie) { - kfree(cookie->data); - kfree(cookie); + if (user_cookie) { + kfree(user_cookie->data); + kfree(user_cookie); } return ERR_PTR(err); } diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c index bfabc9c..656049e 100644 --- a/net/sched/cls_api.c +++ b/net/sched/cls_api.c @@ -3490,28 +3490,28 @@ } EXPORT_SYMBOL(tc_setup_cb_reoffload); -static int tcf_act_get_cookie(struct flow_action_entry *entry, - const struct tc_action *act) +static int tcf_act_get_user_cookie(struct flow_action_entry *entry, + const struct tc_action *act) { - struct tc_cookie *cookie; + struct tc_cookie *user_cookie; int err = 0; rcu_read_lock(); - cookie = rcu_dereference(act->act_cookie); - if (cookie) { - entry->cookie = flow_action_cookie_create(cookie->data, - cookie->len, - GFP_ATOMIC); - if (!entry->cookie) + user_cookie = rcu_dereference(act->user_cookie); + if (user_cookie) { + entry->user_cookie = flow_action_cookie_create(user_cookie->data, + user_cookie->len, + GFP_ATOMIC); + if (!entry->user_cookie) err = -ENOMEM; } rcu_read_unlock(); return err; } -static void tcf_act_put_cookie(struct flow_action_entry *entry) +static void tcf_act_put_user_cookie(struct flow_action_entry *entry) { - flow_action_cookie_destroy(entry->cookie); + flow_action_cookie_destroy(entry->user_cookie); } void tc_cleanup_offload_action(struct flow_action *flow_action) @@ -3520,7 +3520,7 @@ int i; flow_action_for_each(i, entry, flow_action) { - tcf_act_put_cookie(entry); + tcf_act_put_user_cookie(entry); if (entry->destructor) entry->destructor(entry->destructor_priv); } @@ -3565,7 +3565,7 @@ entry = &flow_action->entries[j]; spin_lock_bh(&act->tcfa_lock); - err = tcf_act_get_cookie(entry, act); + err = tcf_act_get_user_cookie(entry, act); if (err) goto err_out_locked; @@ -3577,7 +3577,7 @@ for (k = 0; k < index ; k++) { entry[k].hw_stats = tc_act_hw_stats(act->hw_stats); entry[k].hw_index = act->tcfa_index; - entry[k].act_cookie = (unsigned long)act; + entry[k].cookie = (unsigned long)act; } j += index;
On Wed, Feb 15, 2023 at 12:09:51PM +0200, Paul Blakey wrote: > > > On 14/02/2023 21:24, Edward Cree wrote: > > On 14/02/2023 18:48, Marcelo Ricardo Leitner wrote: > > > On Tue, Feb 14, 2023 at 02:31:06PM +0200, Oz Shlomo wrote: > > > > Actually, I think the current naming scheme of act_cookie and miss_cookie > > > > makes sense. > > > > > > Then perhaps, > > > act_cookie here -> instance_cookie > > > miss_cookie -> config_cookie > > > > > > Sorry for the bikeshedding, btw, but these cookies are getting > > > confusing. We need them to taste nice :-} > > > > I'm with Oz, keep the current name for act_cookie. > > > > (In my ideal world, it'd just be called cookie, and the existing > > cookie in struct flow_action_entry would be renamed user_cookie. > > Because act_cookie is the same thing conceptually as > > flow_cls_offload.cookie. Though I wonder if that means it > > belongs in struct flow_offload_action instead?) > > > > -ed > > > > > Ok so I want to add this patch to the series: > > > From 326938812758dbd2591b221452708504911ca419 Mon Sep 17 00:00:00 2001 > From: Paul Blakey <paulb@nvidia.com> > Date: Wed, 15 Feb 2023 10:57:40 +0200 > Subject: [PATCH] net: sched: Rename user cookie and act cookie > > struct tc_action->act_cookie is a user defined cookie, > and the related struct flow_action_entry->act_cookie is > used as an handle similar to struct flow_cls_offload->cookie. > > Rename tc_action->act_cookie to user_cookie, and > flow_action_entry->act_cookie to cookie so their names > would better fit their usage. Makes sense. This helps a lot already. (I didn't review the patch carefully yet, but it seems good) Thanks!
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h index 1fa95b916342..9b9aa854068f 100644 --- a/include/linux/skbuff.h +++ b/include/linux/skbuff.h @@ -311,12 +311,16 @@ struct nf_bridge_info { * and read by ovs to recirc_id. */ struct tc_skb_ext { - __u32 chain; + union { + u64 act_miss_cookie; + __u32 chain; + }; __u16 mru; __u16 zone; u8 post_ct:1; u8 post_ct_snat:1; u8 post_ct_dnat:1; + u8 act_miss:1; /* Set if act_miss_cookie is used */ }; #endif diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h index 0400a0ac8a29..88db7346eb7a 100644 --- a/include/net/flow_offload.h +++ b/include/net/flow_offload.h @@ -228,6 +228,7 @@ void flow_action_cookie_destroy(struct flow_action_cookie *cookie); struct flow_action_entry { enum flow_action_id id; u32 hw_index; + u64 miss_cookie; enum flow_action_hw_stats hw_stats; action_destr destructor; void *destructor_priv; diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h index cd410a87517b..e395f2a84ed2 100644 --- a/include/net/pkt_cls.h +++ b/include/net/pkt_cls.h @@ -59,6 +59,8 @@ int tcf_block_get_ext(struct tcf_block **p_block, struct Qdisc *q, void tcf_block_put(struct tcf_block *block); void tcf_block_put_ext(struct tcf_block *block, struct Qdisc *q, struct tcf_block_ext_info *ei); +int tcf_exts_init_ex(struct tcf_exts *exts, struct net *net, int action, + int police, struct tcf_proto *tp, u32 handle, bool used_action_miss); static inline bool tcf_block_shared(struct tcf_block *block) { @@ -229,6 +231,7 @@ struct tcf_exts { struct tc_action **actions; struct net *net; netns_tracker ns_tracker; + struct tcf_exts_miss_cookie_node *miss_cookie_node; #endif /* Map to export classifier specific extension TLV types to the * generic extensions API. Unsupported extensions must be set to 0. @@ -240,21 +243,11 @@ struct tcf_exts { static inline int tcf_exts_init(struct tcf_exts *exts, struct net *net, int action, int police) { -#ifdef CONFIG_NET_CLS_ACT - exts->type = 0; - exts->nr_actions = 0; - /* Note: we do not own yet a reference on net. - * This reference might be taken later from tcf_exts_get_net(). - */ - exts->net = net; - exts->actions = kcalloc(TCA_ACT_MAX_PRIO, sizeof(struct tc_action *), - GFP_KERNEL); - if (!exts->actions) - return -ENOMEM; +#ifdef CONFIG_NET_CLS + return tcf_exts_init_ex(exts, net, action, police, NULL, 0, false); +#else + return -EOPNOTSUPP; #endif - exts->action = action; - exts->police = police; - return 0; } /* Return false if the netns is being destroyed in cleanup_net(). Callers @@ -353,6 +346,18 @@ tcf_exts_exec(struct sk_buff *skb, struct tcf_exts *exts, return TC_ACT_OK; } +static inline int +tcf_exts_exec_ex(struct sk_buff *skb, struct tcf_exts *exts, int act_index, + struct tcf_result *res) +{ +#ifdef CONFIG_NET_CLS_ACT + return tcf_action_exec(skb, exts->actions + act_index, + exts->nr_actions - act_index, res); +#else + return TC_ACT_OK; +#endif +} + int tcf_exts_validate(struct net *net, struct tcf_proto *tp, struct nlattr **tb, struct nlattr *rate_tlv, struct tcf_exts *exts, u32 flags, @@ -577,6 +582,7 @@ int tc_setup_offload_action(struct flow_action *flow_action, void tc_cleanup_offload_action(struct flow_action *flow_action); int tc_setup_action(struct flow_action *flow_action, struct tc_action *actions[], + u32 miss_cookie_base, struct netlink_ext_ack *extack); int tc_setup_cb_call(struct tcf_block *block, enum tc_setup_type type, diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h index af4aa66aaa4e..fab5ba3e61b7 100644 --- a/include/net/sch_generic.h +++ b/include/net/sch_generic.h @@ -369,6 +369,8 @@ struct tcf_proto_ops { struct nlattr **tca, struct netlink_ext_ack *extack); void (*tmplt_destroy)(void *tmplt_priv); + struct tcf_exts * (*get_exts)(const struct tcf_proto *tp, + u32 handle); /* rtnetlink specific */ int (*dump)(struct net*, struct tcf_proto*, void *, diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c index e20d1a973417..69f91460a55c 100644 --- a/net/openvswitch/flow.c +++ b/net/openvswitch/flow.c @@ -1038,7 +1038,8 @@ int ovs_flow_key_extract(const struct ip_tunnel_info *tun_info, #if IS_ENABLED(CONFIG_NET_TC_SKB_EXT) if (tc_skb_ext_tc_enabled()) { tc_ext = skb_ext_find(skb, TC_SKB_EXT); - key->recirc_id = tc_ext ? tc_ext->chain : 0; + key->recirc_id = tc_ext && !tc_ext->act_miss ? + tc_ext->chain : 0; OVS_CB(skb)->mru = tc_ext ? tc_ext->mru : 0; post_ct = tc_ext ? tc_ext->post_ct : false; post_ct_snat = post_ct ? tc_ext->post_ct_snat : false; diff --git a/net/sched/act_api.c b/net/sched/act_api.c index cd09ef49df22..16fd3d30eb12 100644 --- a/net/sched/act_api.c +++ b/net/sched/act_api.c @@ -272,7 +272,7 @@ static int tcf_action_offload_add_ex(struct tc_action *action, if (err) goto fl_err; - err = tc_setup_action(&fl_action->action, actions, extack); + err = tc_setup_action(&fl_action->action, actions, 0, extack); if (err) { NL_SET_ERR_MSG_MOD(extack, "Failed to setup tc actions for offload"); diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c index 5b4a95e8a1ee..8ff9530fef68 100644 --- a/net/sched/cls_api.c +++ b/net/sched/cls_api.c @@ -22,6 +22,7 @@ #include <linux/idr.h> #include <linux/jhash.h> #include <linux/rculist.h> +#include <linux/rhashtable.h> #include <net/net_namespace.h> #include <net/sock.h> #include <net/netlink.h> @@ -50,6 +51,109 @@ static LIST_HEAD(tcf_proto_base); /* Protects list of registered TC modules. It is pure SMP lock. */ static DEFINE_RWLOCK(cls_mod_lock); +static struct xarray tcf_exts_miss_cookies_xa; +struct tcf_exts_miss_cookie_node { + const struct tcf_chain *chain; + const struct tcf_proto *tp; + const struct tcf_exts *exts; + u32 chain_index; + u32 tp_prio; + u32 handle; + u32 miss_cookie_base; + struct rcu_head rcu; +}; + +/* Each tc action entry cookie will be comprised of 32bit miss_cookie_base + + * action index in the exts tc actions array. + */ +union tcf_exts_miss_cookie { + struct { + u32 miss_cookie_base; + u32 act_index; + }; + u64 miss_cookie; +}; + +#if IS_ENABLED(CONFIG_NET_TC_SKB_EXT) +static int +tcf_exts_miss_cookie_base_alloc(struct tcf_exts *exts, struct tcf_proto *tp, + u32 handle) +{ + struct tcf_exts_miss_cookie_node *n; + static u32 next; + int err; + + if (WARN_ON(!handle || !tp->ops->get_exts)) + return -EINVAL; + + n = kzalloc(sizeof(*n), GFP_KERNEL); + if (!n) + return -ENOMEM; + + n->chain_index = tp->chain->index; + n->chain = tp->chain; + n->tp_prio = tp->prio; + n->tp = tp; + n->exts = exts; + n->handle = handle; + + err = xa_alloc_cyclic(&tcf_exts_miss_cookies_xa, &n->miss_cookie_base, + n, xa_limit_32b, &next, GFP_KERNEL); + if (err) + goto err_xa_alloc; + + exts->miss_cookie_node = n; + return 0; + +err_xa_alloc: + kfree(n); + return err; +} + +static void tcf_exts_miss_cookie_base_destroy(struct tcf_exts *exts) +{ + struct tcf_exts_miss_cookie_node *n; + + if (!exts->miss_cookie_node) + return; + + n = exts->miss_cookie_node; + xa_erase(&tcf_exts_miss_cookies_xa, n->miss_cookie_base); + kfree_rcu(n, rcu); +} + +static struct tcf_exts_miss_cookie_node * +tcf_exts_miss_cookie_lookup(u64 miss_cookie, int *act_index) +{ + union tcf_exts_miss_cookie mc = { .miss_cookie = miss_cookie, }; + + *act_index = mc.act_index; + return xa_load(&tcf_exts_miss_cookies_xa, mc.miss_cookie_base); +} +#else /* IS_ENABLED(CONFIG_NET_TC_SKB_EXT) */ +static int +tcf_exts_miss_cookie_base_alloc(struct tcf_exts *exts, struct tcf_proto *tp, + u32 handle) +{ + return 0; +} + +static void tcf_exts_miss_cookie_base_destroy(struct tcf_exts *exts) +{ +} +#endif /* IS_ENABLED(CONFIG_NET_TC_SKB_EXT) */ + +static u64 tcf_exts_miss_cookie_get(u32 miss_cookie_base, int act_index) +{ + union tcf_exts_miss_cookie mc = { .act_index = act_index, }; + + if (!miss_cookie_base) + return 0; + + mc.miss_cookie_base = miss_cookie_base; + return mc.miss_cookie; +} + #ifdef CONFIG_NET_CLS_ACT DEFINE_STATIC_KEY_FALSE(tc_skb_ext_tc); EXPORT_SYMBOL(tc_skb_ext_tc); @@ -1549,6 +1653,8 @@ static inline int __tcf_classify(struct sk_buff *skb, const struct tcf_proto *orig_tp, struct tcf_result *res, bool compat_mode, + struct tcf_exts_miss_cookie_node *n, + int act_index, u32 *last_executed_chain) { #ifdef CONFIG_NET_CLS_ACT @@ -1560,13 +1666,36 @@ static inline int __tcf_classify(struct sk_buff *skb, #endif for (; tp; tp = rcu_dereference_bh(tp->next)) { __be16 protocol = skb_protocol(skb, false); - int err; + int err = 0; - if (tp->protocol != protocol && - tp->protocol != htons(ETH_P_ALL)) - continue; + if (n) { + struct tcf_exts *exts; + + if (n->tp_prio != tp->prio) + continue; + + /* We re-lookup the tp and chain based on index instead + * of having hard refs and locks to them, so do a sanity + * check if any of tp,chain,exts was replaced by the + * time we got here with a cookie from hardware. + */ + if (unlikely(n->tp != tp || n->tp->chain != n->chain || + !tp->ops->get_exts)) + return TC_ACT_SHOT; + + exts = tp->ops->get_exts(tp, n->handle); + if (unlikely(!exts || n->exts != exts)) + return TC_ACT_SHOT; - err = tc_classify(skb, tp, res); + n = NULL; + err = tcf_exts_exec_ex(skb, exts, act_index, res); + } else { + if (tp->protocol != protocol && + tp->protocol != htons(ETH_P_ALL)) + continue; + + err = tc_classify(skb, tp, res); + } #ifdef CONFIG_NET_CLS_ACT if (unlikely(err == TC_ACT_RECLASSIFY && !compat_mode)) { first_tp = orig_tp; @@ -1582,6 +1711,9 @@ static inline int __tcf_classify(struct sk_buff *skb, return err; } + if (unlikely(n)) + return TC_ACT_SHOT; + return TC_ACT_UNSPEC; /* signal: continue lookup */ #ifdef CONFIG_NET_CLS_ACT reset: @@ -1606,21 +1738,33 @@ int tcf_classify(struct sk_buff *skb, #if !IS_ENABLED(CONFIG_NET_TC_SKB_EXT) u32 last_executed_chain = 0; - return __tcf_classify(skb, tp, tp, res, compat_mode, + return __tcf_classify(skb, tp, tp, res, compat_mode, NULL, 0, &last_executed_chain); #else u32 last_executed_chain = tp ? tp->chain->index : 0; + struct tcf_exts_miss_cookie_node *n = NULL; const struct tcf_proto *orig_tp = tp; struct tc_skb_ext *ext; + int act_index = 0; int ret; if (block) { ext = skb_ext_find(skb, TC_SKB_EXT); - if (ext && ext->chain) { + if (ext && (ext->chain || ext->act_miss)) { struct tcf_chain *fchain; + u32 chain = ext->chain; - fchain = tcf_chain_lookup_rcu(block, ext->chain); + if (ext->act_miss) { + n = tcf_exts_miss_cookie_lookup(ext->act_miss_cookie, + &act_index); + if (!n) + return TC_ACT_SHOT; + + chain = n->chain_index; + } + + fchain = tcf_chain_lookup_rcu(block, chain); if (!fchain) return TC_ACT_SHOT; @@ -1632,7 +1776,7 @@ int tcf_classify(struct sk_buff *skb, } } - ret = __tcf_classify(skb, tp, orig_tp, res, compat_mode, + ret = __tcf_classify(skb, tp, orig_tp, res, compat_mode, n, act_index, &last_executed_chain); if (tc_skb_ext_tc_enabled()) { @@ -3056,9 +3200,48 @@ static int tc_dump_chain(struct sk_buff *skb, struct netlink_callback *cb) return skb->len; } +int tcf_exts_init_ex(struct tcf_exts *exts, struct net *net, int action, + int police, struct tcf_proto *tp, u32 handle, + bool use_action_miss) +{ + int err = 0; + +#ifdef CONFIG_NET_CLS_ACT + exts->type = 0; + exts->nr_actions = 0; + /* Note: we do not own yet a reference on net. + * This reference might be taken later from tcf_exts_get_net(). + */ + exts->net = net; + exts->actions = kcalloc(TCA_ACT_MAX_PRIO, sizeof(struct tc_action *), + GFP_KERNEL); + if (!exts->actions) + return -ENOMEM; +#endif + + exts->action = action; + exts->police = police; + + if (!use_action_miss) + return 0; + + err = tcf_exts_miss_cookie_base_alloc(exts, tp, handle); + if (err) + goto err_miss_alloc; + + return 0; + +err_miss_alloc: + tcf_exts_destroy(exts); + return err; +} +EXPORT_SYMBOL(tcf_exts_init_ex); + void tcf_exts_destroy(struct tcf_exts *exts) { #ifdef CONFIG_NET_CLS_ACT + tcf_exts_miss_cookie_base_destroy(exts); + if (exts->actions) { tcf_action_destroy(exts->actions, TCA_ACT_UNBIND); kfree(exts->actions); @@ -3547,6 +3730,7 @@ static int tc_setup_offload_act(struct tc_action *act, int tc_setup_action(struct flow_action *flow_action, struct tc_action *actions[], + u32 miss_cookie_base, struct netlink_ext_ack *extack) { int i, j, k, index, err = 0; @@ -3577,6 +3761,8 @@ int tc_setup_action(struct flow_action *flow_action, for (k = 0; k < index ; k++) { entry[k].hw_stats = tc_act_hw_stats(act->hw_stats); entry[k].hw_index = act->tcfa_index; + entry[k].miss_cookie = + tcf_exts_miss_cookie_get(miss_cookie_base, i); } j += index; @@ -3599,10 +3785,15 @@ int tc_setup_offload_action(struct flow_action *flow_action, struct netlink_ext_ack *extack) { #ifdef CONFIG_NET_CLS_ACT + u32 miss_cookie_base; + if (!exts) return 0; - return tc_setup_action(flow_action, exts->actions, extack); + miss_cookie_base = exts->miss_cookie_node ? + exts->miss_cookie_node->miss_cookie_base : 0; + return tc_setup_action(flow_action, exts->actions, miss_cookie_base, + extack); #else return 0; #endif @@ -3770,6 +3961,8 @@ static int __init tc_filter_init(void) if (err) goto err_register_pernet_subsys; + xa_init_flags(&tcf_exts_miss_cookies_xa, XA_FLAGS_ALLOC1); + rtnl_register(PF_UNSPEC, RTM_NEWTFILTER, tc_new_tfilter, NULL, RTNL_FLAG_DOIT_UNLOCKED); rtnl_register(PF_UNSPEC, RTM_DELTFILTER, tc_del_tfilter, NULL,