| Message ID | 20190504114628.14755-11-jakub.kicinski@netronome.com |
|---|---|
| State | Accepted |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: act_police offload support | expand |
Sat, May 04, 2019 at 01:46:25PM CEST, jakub.kicinski@netronome.com wrote: >From: Pieter Jansen van Vuuren <pieter.jansenvanvuuren@netronome.com> > >Some actions like the police action are stateful and could share state >between devices. This is incompatible with offloading to multiple devices >and drivers might want to test for shared blocks when offloading. >Store a pointer to the tcf_block structure in the tc_cls_common_offload >structure to allow drivers to determine when offloads apply to a shared >block. I don't this this is good idea. If your driver supports shared blocks, you should register the callback accordingly. See: mlxsw_sp_setup_tc_block_flower_bind() where tcf_block_cb_lookup() and __tcf_block_cb_register() are used to achieve that.
On Sat, 4 May 2019 15:16:54 +0200, Jiri Pirko wrote: > Sat, May 04, 2019 at 01:46:25PM CEST, jakub.kicinski@netronome.com wrote: > >From: Pieter Jansen van Vuuren <pieter.jansenvanvuuren@netronome.com> > > > >Some actions like the police action are stateful and could share state > >between devices. This is incompatible with offloading to multiple devices > >and drivers might want to test for shared blocks when offloading. > >Store a pointer to the tcf_block structure in the tc_cls_common_offload > >structure to allow drivers to determine when offloads apply to a shared > >block. > > I don't this this is good idea. If your driver supports shared blocks, > you should register the callback accordingly. See: > mlxsw_sp_setup_tc_block_flower_bind() where tcf_block_cb_lookup() and > __tcf_block_cb_register() are used to achieve that. Right, in some ways. Unfortunately we don't support shared blocks fully, i.e. we register multiple callbacks and get the rules replicated. It's a FW limitation, but I don't think we have shared blocks on the roadmap, since rule storage is not an issue for our HW. But even if we did support sharing blocks, we'd have to teach TC that some rules can only be offloaded if there is only a single callback registered, right? In case the block is shared between different ASICs.
Sun, May 05, 2019 at 07:34:32PM CEST, jakub.kicinski@netronome.com wrote: >On Sat, 4 May 2019 15:16:54 +0200, Jiri Pirko wrote: >> Sat, May 04, 2019 at 01:46:25PM CEST, jakub.kicinski@netronome.com wrote: >> >From: Pieter Jansen van Vuuren <pieter.jansenvanvuuren@netronome.com> >> > >> >Some actions like the police action are stateful and could share state >> >between devices. This is incompatible with offloading to multiple devices >> >and drivers might want to test for shared blocks when offloading. >> >Store a pointer to the tcf_block structure in the tc_cls_common_offload >> >structure to allow drivers to determine when offloads apply to a shared >> >block. >> >> I don't this this is good idea. If your driver supports shared blocks, >> you should register the callback accordingly. See: >> mlxsw_sp_setup_tc_block_flower_bind() where tcf_block_cb_lookup() and >> __tcf_block_cb_register() are used to achieve that. > >Right, in some ways. Unfortunately we don't support shared blocks >fully, i.e. we register multiple callbacks and get the rules >replicated. It's a FW limitation, but I don't think we have shared >blocks on the roadmap, since rule storage is not an issue for our HW. > >But even if we did support sharing blocks, we'd have to teach TC that >some rules can only be offloaded if there is only a single callback >registered, right? In case the block is shared between different ASICs. I don't see why sharing block between different ASICs is a problem. The sharing implementation is totally up to the driver. It can duplicate the rules even within one ASIC. According to that, it registers one or more callbacks. In this patchset, you use the block only to see if it is shared or not. When TC calls the driver to bind, it provides the block struct: ndo_setup_tc type == TC_SETUP_BLOCK f->command == TC_BLOCK_BIND You can check for sharing there and remember it for the future check in filter insertion. I would like to avoid passing block pointer during filter insertion. It is misleading and I'm pretty sure it would lead to misuse by drivers. I see that Dave already applied this patchset. Could you please send follow-up removing the block pointer from filter offload struct? Thanks!
On Mon, 6 May 2019 08:16:31 +0200, Jiri Pirko wrote: > Sun, May 05, 2019 at 07:34:32PM CEST, jakub.kicinski@netronome.com wrote: > >On Sat, 4 May 2019 15:16:54 +0200, Jiri Pirko wrote: > >> Sat, May 04, 2019 at 01:46:25PM CEST, jakub.kicinski@netronome.com wrote: > >> >From: Pieter Jansen van Vuuren <pieter.jansenvanvuuren@netronome.com> > >> > > >> >Some actions like the police action are stateful and could share state > >> >between devices. This is incompatible with offloading to multiple devices > >> >and drivers might want to test for shared blocks when offloading. > >> >Store a pointer to the tcf_block structure in the tc_cls_common_offload > >> >structure to allow drivers to determine when offloads apply to a shared > >> >block. > >> > >> I don't this this is good idea. If your driver supports shared blocks, > >> you should register the callback accordingly. See: > >> mlxsw_sp_setup_tc_block_flower_bind() where tcf_block_cb_lookup() and > >> __tcf_block_cb_register() are used to achieve that. > > > >Right, in some ways. Unfortunately we don't support shared blocks > >fully, i.e. we register multiple callbacks and get the rules > >replicated. It's a FW limitation, but I don't think we have shared > >blocks on the roadmap, since rule storage is not an issue for our HW. > > > >But even if we did support sharing blocks, we'd have to teach TC that > >some rules can only be offloaded if there is only a single callback > >registered, right? In case the block is shared between different ASICs. > > I don't see why sharing block between different ASICs is a problem. The > sharing implementation is totally up to the driver. It can duplicate the > rules even within one ASIC. According to that, it registers one or more > callbacks. If we want to replicate software semantics for act_police all ports sharing the port should count against the same rate limit. This is pretty much impossible unless the rule is offloaded to a single ASIC and the ASIC/FW supports proper block/action sharing. > In this patchset, you use the block only to see if it is shared or not. > When TC calls the driver to bind, it provides the block struct: > ndo_setup_tc > type == TC_SETUP_BLOCK > f->command == TC_BLOCK_BIND > You can check for sharing there and remember it for the future check in > filter insertion. > > I would like to avoid passing block pointer during filter insertion. It > is misleading and I'm pretty sure it would lead to misuse by drivers. > > I see that Dave already applied this patchset. Could you please send > follow-up removing the block pointer from filter offload struct? Makes sense, we'll follow up shortly!
diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h index 161fcf8516ac..eed98f8fcb5e 100644 --- a/include/net/pkt_cls.h +++ b/include/net/pkt_cls.h @@ -100,6 +100,11 @@ int tcf_classify(struct sk_buff *skb, const struct tcf_proto *tp, struct tcf_result *res, bool compat_mode); #else +static inline bool tcf_block_shared(struct tcf_block *block) +{ + return false; +} + static inline int tcf_block_get(struct tcf_block **p_block, struct tcf_proto __rcu **p_filter_chain, struct Qdisc *q, @@ -624,6 +629,7 @@ struct tc_cls_common_offload { u32 chain_index; __be16 protocol; u32 prio; + struct tcf_block *block; struct netlink_ext_ack *extack; }; @@ -725,11 +731,13 @@ static inline bool tc_in_hw(u32 flags) static inline void tc_cls_common_offload_init(struct tc_cls_common_offload *cls_common, const struct tcf_proto *tp, u32 flags, + struct tcf_block *block, struct netlink_ext_ack *extack) { cls_common->chain_index = tp->chain->index; cls_common->protocol = tp->protocol; cls_common->prio = tp->prio; + cls_common->block = block; if (tc_skip_sw(flags) || flags & TCA_CLS_FLAGS_VERBOSE) cls_common->extack = extack; } diff --git a/net/sched/cls_bpf.c b/net/sched/cls_bpf.c index 9bcf499cce0c..ce7ff286ccb8 100644 --- a/net/sched/cls_bpf.c +++ b/net/sched/cls_bpf.c @@ -157,7 +157,7 @@ static int cls_bpf_offload_cmd(struct tcf_proto *tp, struct cls_bpf_prog *prog, skip_sw = prog && tc_skip_sw(prog->gen_flags); obj = prog ?: oldprog; - tc_cls_common_offload_init(&cls_bpf.common, tp, obj->gen_flags, + tc_cls_common_offload_init(&cls_bpf.common, tp, obj->gen_flags, block, extack); cls_bpf.command = TC_CLSBPF_OFFLOAD; cls_bpf.exts = &obj->exts; @@ -227,7 +227,8 @@ static void cls_bpf_offload_update_stats(struct tcf_proto *tp, struct tcf_block *block = tp->chain->block; struct tc_cls_bpf_offload cls_bpf = {}; - tc_cls_common_offload_init(&cls_bpf.common, tp, prog->gen_flags, NULL); + tc_cls_common_offload_init(&cls_bpf.common, tp, prog->gen_flags, block, + NULL); cls_bpf.command = TC_CLSBPF_STATS; cls_bpf.exts = &prog->exts; cls_bpf.prog = prog->filter; @@ -669,7 +670,7 @@ static int cls_bpf_reoffload(struct tcf_proto *tp, bool add, tc_setup_cb_t *cb, continue; tc_cls_common_offload_init(&cls_bpf.common, tp, prog->gen_flags, - extack); + block, extack); cls_bpf.command = TC_CLSBPF_OFFLOAD; cls_bpf.exts = &prog->exts; cls_bpf.prog = add ? prog->filter : NULL; diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c index f6685fc53119..3cb372b0e933 100644 --- a/net/sched/cls_flower.c +++ b/net/sched/cls_flower.c @@ -389,7 +389,8 @@ static void fl_hw_destroy_filter(struct tcf_proto *tp, struct cls_fl_filter *f, if (!rtnl_held) rtnl_lock(); - tc_cls_common_offload_init(&cls_flower.common, tp, f->flags, extack); + tc_cls_common_offload_init(&cls_flower.common, tp, f->flags, block, + extack); cls_flower.command = TC_CLSFLOWER_DESTROY; cls_flower.cookie = (unsigned long) f; @@ -422,7 +423,8 @@ static int fl_hw_replace_filter(struct tcf_proto *tp, goto errout; } - tc_cls_common_offload_init(&cls_flower.common, tp, f->flags, extack); + tc_cls_common_offload_init(&cls_flower.common, tp, f->flags, block, + extack); cls_flower.command = TC_CLSFLOWER_REPLACE; cls_flower.cookie = (unsigned long) f; cls_flower.rule->match.dissector = &f->mask->dissector; @@ -478,7 +480,8 @@ static void fl_hw_update_stats(struct tcf_proto *tp, struct cls_fl_filter *f, if (!rtnl_held) rtnl_lock(); - tc_cls_common_offload_init(&cls_flower.common, tp, f->flags, NULL); + tc_cls_common_offload_init(&cls_flower.common, tp, f->flags, block, + NULL); cls_flower.command = TC_CLSFLOWER_STATS; cls_flower.cookie = (unsigned long) f; cls_flower.classid = f->res.classid; @@ -1757,7 +1760,7 @@ static int fl_reoffload(struct tcf_proto *tp, bool add, tc_setup_cb_t *cb, } tc_cls_common_offload_init(&cls_flower.common, tp, f->flags, - extack); + block, extack); cls_flower.command = add ? TC_CLSFLOWER_REPLACE : TC_CLSFLOWER_DESTROY; cls_flower.cookie = (unsigned long)f; diff --git a/net/sched/cls_matchall.c b/net/sched/cls_matchall.c index da916f39b719..820938fa09ed 100644 --- a/net/sched/cls_matchall.c +++ b/net/sched/cls_matchall.c @@ -71,7 +71,8 @@ static void mall_destroy_hw_filter(struct tcf_proto *tp, struct tc_cls_matchall_offload cls_mall = {}; struct tcf_block *block = tp->chain->block; - tc_cls_common_offload_init(&cls_mall.common, tp, head->flags, extack); + tc_cls_common_offload_init(&cls_mall.common, tp, head->flags, block, + extack); cls_mall.command = TC_CLSMATCHALL_DESTROY; cls_mall.cookie = cookie; @@ -93,7 +94,8 @@ static int mall_replace_hw_filter(struct tcf_proto *tp, if (!cls_mall.rule) return -ENOMEM; - tc_cls_common_offload_init(&cls_mall.common, tp, head->flags, extack); + tc_cls_common_offload_init(&cls_mall.common, tp, head->flags, block, + extack); cls_mall.command = TC_CLSMATCHALL_REPLACE; cls_mall.cookie = cookie; @@ -293,7 +295,8 @@ static int mall_reoffload(struct tcf_proto *tp, bool add, tc_setup_cb_t *cb, if (!cls_mall.rule) return -ENOMEM; - tc_cls_common_offload_init(&cls_mall.common, tp, head->flags, extack); + tc_cls_common_offload_init(&cls_mall.common, tp, head->flags, block, + extack); cls_mall.command = add ? TC_CLSMATCHALL_REPLACE : TC_CLSMATCHALL_DESTROY; cls_mall.cookie = (unsigned long)head; @@ -328,7 +331,8 @@ static void mall_stats_hw_filter(struct tcf_proto *tp, struct tc_cls_matchall_offload cls_mall = {}; struct tcf_block *block = tp->chain->block; - tc_cls_common_offload_init(&cls_mall.common, tp, head->flags, NULL); + tc_cls_common_offload_init(&cls_mall.common, tp, head->flags, block, + NULL); cls_mall.command = TC_CLSMATCHALL_STATS; cls_mall.cookie = cookie; diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c index 4b8710a266cc..2feed0ffa269 100644 --- a/net/sched/cls_u32.c +++ b/net/sched/cls_u32.c @@ -485,7 +485,8 @@ static void u32_clear_hw_hnode(struct tcf_proto *tp, struct tc_u_hnode *h, struct tcf_block *block = tp->chain->block; struct tc_cls_u32_offload cls_u32 = {}; - tc_cls_common_offload_init(&cls_u32.common, tp, h->flags, extack); + tc_cls_common_offload_init(&cls_u32.common, tp, h->flags, block, + extack); cls_u32.command = TC_CLSU32_DELETE_HNODE; cls_u32.hnode.divisor = h->divisor; cls_u32.hnode.handle = h->handle; @@ -503,7 +504,7 @@ static int u32_replace_hw_hnode(struct tcf_proto *tp, struct tc_u_hnode *h, bool offloaded = false; int err; - tc_cls_common_offload_init(&cls_u32.common, tp, flags, extack); + tc_cls_common_offload_init(&cls_u32.common, tp, flags, block, extack); cls_u32.command = TC_CLSU32_NEW_HNODE; cls_u32.hnode.divisor = h->divisor; cls_u32.hnode.handle = h->handle; @@ -529,7 +530,8 @@ static void u32_remove_hw_knode(struct tcf_proto *tp, struct tc_u_knode *n, struct tcf_block *block = tp->chain->block; struct tc_cls_u32_offload cls_u32 = {}; - tc_cls_common_offload_init(&cls_u32.common, tp, n->flags, extack); + tc_cls_common_offload_init(&cls_u32.common, tp, n->flags, block, + extack); cls_u32.command = TC_CLSU32_DELETE_KNODE; cls_u32.knode.handle = n->handle; @@ -546,7 +548,7 @@ static int u32_replace_hw_knode(struct tcf_proto *tp, struct tc_u_knode *n, bool skip_sw = tc_skip_sw(flags); int err; - tc_cls_common_offload_init(&cls_u32.common, tp, flags, extack); + tc_cls_common_offload_init(&cls_u32.common, tp, flags, block, extack); cls_u32.command = TC_CLSU32_REPLACE_KNODE; cls_u32.knode.handle = n->handle; cls_u32.knode.fshift = n->fshift; @@ -1170,10 +1172,12 @@ static int u32_reoffload_hnode(struct tcf_proto *tp, struct tc_u_hnode *ht, bool add, tc_setup_cb_t *cb, void *cb_priv, struct netlink_ext_ack *extack) { + struct tcf_block *block = tp->chain->block; struct tc_cls_u32_offload cls_u32 = {}; int err; - tc_cls_common_offload_init(&cls_u32.common, tp, ht->flags, extack); + tc_cls_common_offload_init(&cls_u32.common, tp, ht->flags, block, + extack); cls_u32.command = add ? TC_CLSU32_NEW_HNODE : TC_CLSU32_DELETE_HNODE; cls_u32.hnode.divisor = ht->divisor; cls_u32.hnode.handle = ht->handle; @@ -1195,7 +1199,8 @@ static int u32_reoffload_knode(struct tcf_proto *tp, struct tc_u_knode *n, struct tc_cls_u32_offload cls_u32 = {}; int err; - tc_cls_common_offload_init(&cls_u32.common, tp, n->flags, extack); + tc_cls_common_offload_init(&cls_u32.common, tp, n->flags, block, + extack); cls_u32.command = add ? TC_CLSU32_REPLACE_KNODE : TC_CLSU32_DELETE_KNODE; cls_u32.knode.handle = n->handle;