net_sched: use idr to allocate bpf filter handles
Subsystems (5)
PATCH v2 1/1 net_sched: use idr to allocate bpf filter handles
Instead of calling cls_bpf_get() in a loop to find a unused handle, just switch to idr API to allocate new handles.
--- net/sched/cls_bpf.c | 57 ++++++++++++++++++++++++++--------------------------- 1 file changed, 28 insertions(+), 29 deletions(-)
net/sched/cls_bpf.c +28 -29
@@ -17,6 +17,7 @@#include <linux/skbuff.h> #include <linux/filter.h> #include <linux/bpf.h> +#include <linux/idr.h> #include <net/rtnetlink.h> #include <net/pkt_cls.h>
@@ -32,7 +33,7 @@ MODULE_DESCRIPTION("TC BPF based classifier");struct cls_bpf_head {struct list_head plist; -u32 hgen; +struct idr handle_idr;struct rcu_head rcu; };
@@ -238,6 +239,7 @@ static int cls_bpf_init(struct tcf_proto *tp)return -ENOBUFS ;INIT_LIST_HEAD_RCU (&head->plist); +idr_init (&head->handle_idr);rcu_assign_pointer (tp->root, head);return 0 ;
@@ -264,6 +266,9 @@ static void cls_bpf_delete_prog_rcu(struct rcu_head *rcu)static void __cls_bpf_delete (struct tcf_proto *tp,struct cls_bpf_prog *prog) { +struct cls_bpf_head *head =rtnl_dereference (tp->root); + +idr_remove_ext (&head->handle_idr, prog->handle);cls_bpf_stop_offload (tp, prog);list_del_rcu (&prog->link);tcf_unbind_filter (tp, &prog->res);
@@ -287,6 +292,7 @@ static void cls_bpf_destroy(struct tcf_proto *tp)list_for_each_entry_safe (prog, tmp, &head->plist, link)__cls_bpf_delete (tp, prog); +idr_destroy (&head->handle_idr);kfree_rcu (head, rcu); }
@@ -421,27 +427,6 @@ static int cls_bpf_set_parms(struct net *net, struct tcf_proto *tp,return 0 ; } -static u32 cls_bpf_grab_new_handle (struct tcf_proto *tp, -struct cls_bpf_head *head) -{ -unsigned int i =0x80000000 ; -u32 handle; - -do { -if (++head->hgen ==0x7FFFFFFF ) - head->hgen =1 ; - }while (--i >0 &&cls_bpf_get (tp, head->hgen)); - -if (unlikely (i ==0 )) { -pr_err ("Insufficient number of handles\n" ); - handle =0 ; - }else { - handle = head->hgen; - } - -return handle; -} - static int cls_bpf_change(struct net *net, struct sk_buff *in_skb, struct tcf_proto *tp, unsigned long base, u32 handle, struct nlattr **tca,
@@ -451,6 +436,7 @@ static int cls_bpf_change(struct net *net, struct sk_buff *in_skb,struct cls_bpf_prog *oldprog = *arg;struct nlattr *tb[TCA_BPF_MAX +1 ];struct cls_bpf_prog *prog; +unsigned long idr_index;int ret;if (tca[TCA_OPTIONS ] ==NULL )
@@ -476,21 +462,30 @@ static int cls_bpf_change(struct net *net, struct sk_buff *in_skb, } } -if (handle ==0 ) - prog->handle =cls_bpf_grab_new_handle (tp, head); -else +if (handle ==0 ) { + ret =idr_alloc_ext (&head->handle_idr, prog, &idr_index, +1 ,0x7FFFFFFF ,GFP_KERNEL ); +if (ret) + goto errout; + prog->handle = idr_index; + }else { +if (!oldprog) { + ret =idr_alloc_ext (&head->handle_idr, prog, &idr_index, + handle, handle +1 ,GFP_KERNEL ); +if (ret) + goto errout; + } prog->handle = handle; -if (prog->handle ==0 ) { - ret = -EINVAL ; - goto errout; } ret =cls_bpf_set_parms (net, tp, prog, base, tb, tca[TCA_RATE ], ovr);if (ret <0 ) - goto errout; + goto errout_idr; ret =cls_bpf_offload (tp, prog, oldprog);if (ret) { +if (!oldprog) +idr_remove_ext (&head->handle_idr, prog->handle);__cls_bpf_delete_prog (prog);return ret; }
@@ -499,6 +494,7 @@ static int cls_bpf_change(struct net *net, struct sk_buff *in_skb, prog->gen_flags |=TCA_CLS_FLAGS_NOT_IN_HW ;if (oldprog) { +idr_replace_ext (&head->handle_idr, prog, handle);list_replace_rcu (&oldprog->link, &prog->link);tcf_unbind_filter (tp, &oldprog->res);call_rcu (&oldprog->rcu, cls_bpf_delete_prog_rcu);
@@ -509,6 +505,9 @@ static int cls_bpf_change(struct net *net, struct sk_buff *in_skb, *arg = prog;return 0 ; +errout_idr: +if (!oldprog) +idr_remove_ext (&head->handle_idr, prog->handle); errout:tcf_exts_destroy (&prog->exts);kfree (prog);
Signature · 1 line
2.13.0
Daniel Borkmann
maintainerQuoted 8 lines · Cong Wang
Instead of calling
cls_bpf_get()in a loop to find a unused handle, just switch to idr API to allocate new handles.Cc Daniel Borkmann <[email protected]>Cc Chris Mi <[email protected]>Cc Jamal Hadi Salim <[email protected]>Signed-off-by Cong Wang <[email protected]>
[...]
Quoted 25 lines
net/sched/cls_bpf.c +13
-6@@ -476,21 +462,30 @@ static int cls_bpf_change(struct net *net, struct sk_buff *in_skb, } } -if (handle ==0 ) - prog->handle =cls_bpf_grab_new_handle (tp, head); -else +if (handle ==0 ) { + ret =idr_alloc_ext (&head->handle_idr, prog, &idr_index, +1 ,0x7FFFFFFF ,GFP_KERNEL ); +if (ret) + goto errout; + prog->handle = idr_index; + }else { +if (!oldprog) { + ret =idr_alloc_ext (&head->handle_idr, prog, &idr_index, + handle, handle +1 ,GFP_KERNEL ); +if (ret) + goto errout; + } prog->handle = handle; -if (prog->handle ==0 ) { - ret = -EINVAL ; - goto errout; }
net/sched/cls_bpf.c static int cls_bpf_change(struct net *net, struct sk_buff *in_skb, · line 488
Quoted 10 lines
net/sched/cls_bpf.c +3
-1ret =cls_bpf_set_parms (net, tp, prog, base, tb, tca[TCA_RATE ], ovr);if (ret <0 ) - goto errout; + goto errout_idr; ret =cls_bpf_offload (tp, prog, oldprog);if (ret) { +if (!oldprog) +idr_remove_ext (&head->handle_idr, prog->handle);
Shouldn't we also call idr_remove_ext() when there was an oldprog, but we didn't care about reusing the same handle, so it was handle == 0 initially?
There's this condition in the code before above idr allocations, I think also in other classifiers:
if (oldprog) {
if (handle && oldprog->handle != handle) {
ret = -EINVAL;
goto errout;
}
}net/sched/cls_bpf.c static int cls_bpf_change(struct net *net, struct sk_buff *in_skb, · line 497
Quoted 8 lines
net/sched/cls_bpf.c +1
-0__cls_bpf_delete_prog (prog);return ret; }@@ -499,6 +494,7 @@ static int cls_bpf_change(struct net *net, struct sk_buff *in_skb, prog->gen_flags |=TCA_CLS_FLAGS_NOT_IN_HW ;if (oldprog) { +idr_replace_ext (&head->handle_idr, prog, handle);
And here, we should probably use prog->handle for the above mentioned case as well, no?
Would be great if all this (and e.g. the fact that we use idr itself) could optionally be hidden behind some handle generator api given we could reuse that api also for cls_basic and cls_u32. Could also be followed-up perhaps.
net/sched/cls_bpf.c static int cls_bpf_change(struct net *net, struct sk_buff *in_skb, · line 510
Quoted 10 lines
net/sched/cls_bpf.c +3
-0list_replace_rcu (&oldprog->link, &prog->link);tcf_unbind_filter (tp, &oldprog->res);call_rcu (&oldprog->rcu, cls_bpf_delete_prog_rcu);@@ -509,6 +505,9 @@ static int cls_bpf_change(struct net *net, struct sk_buff *in_skb, *arg = prog;return 0 ; +errout_idr: +if (!oldprog) +idr_remove_ext (&head->handle_idr, prog->handle);
(Likewise as the failing cls_bpf_offload().)
Quoted 4 lines
net/sched/cls_bpf.c +0
-0errout:tcf_exts_destroy (&prog->exts);kfree (prog);
Quoted 10 lines · Daniel Borkmann
Review
Quoted 4 lines · Cong Wang
ret = cls_bpf_offload(tp, prog, oldprog); if (ret) {(quoted diff) +2
-0+ if (!oldprog) + idr_remove_ext(&head->handle_idr, prog->handle);Shouldn't we also call
idr_remove_ext()when there was an oldprog, but we didn't care about reusing the same handle, so it was handle == 0 initially?
When oldprog is non-NULL, we are replacing the oldprog with a new one, therefore we should call idr_replace_ext() which happens after this. So no need to call idr_remove_ext() at this point.
Quoted 10 lines
There's this condition in the code before above idr allocations, I think also in other classifiers:
if (oldprog) { if (handle && oldprog->handle != handle) { ret = -EINVAL; goto errout; } }
Sure. If we use handle to find oldprog, it should have the same handle. cls_bpf_get() guarantees it. This check is redundant.
Quoted 8 lines
Quoted 8 lines
__cls_bpf_delete_prog(prog); return ret; }net/sched/cls_bpf.c +0
-0@@ -499,6 +494,7 @@ static int cls_bpf_change(struct net *net, struct sk_buff *in_skb, prog->gen_flags |=TCA_CLS_FLAGS_NOT_IN_HW ;
Quoted 6 lines
net/sched/cls_bpf.c static int cls_bpf_change(struct net *net, struct sk_buff *in_skb, · line 497
Quoted 2 lines
net/sched/cls_bpf.c +1
-0if (oldprog) { +idr_replace_ext (&head->handle_idr, prog, handle);And here, we should probably use
prog->handlefor the above mentioned case as well, no?
Since are replacing oldprog with a new one, prog->handle is same with handle.
Quoted 6 lines
Would be great if all this (and e.g. the fact that we use idr itself) could optionally be hidden behind some handle generator api given we could reuse that api also for
cls_basicandcls_u32. Could also be followed-up perhaps.
Yeah, the idr_alloc_ext(.., handle, handle+1,) is ugly. Ideally we should specify the range during initialization rather than in each idr_alloc_ext(). Commit c15ab236d69d already did the same thing. We can refactor this later.
Daniel Borkmann
maintainerQuoted 2 lines · Cong Wang
Quoted 1 line · Daniel Borkmann
On 09/25/2017 07:13 PM, Cong Wang wrote:
[...]
Quoted 10 lines
Quoted 9 lines
There's this condition in the code before above idr allocations, I think also in other classifiers:
if (oldprog) { if (handle && oldprog->handle != handle) { ret = -EINVAL; goto errout; } }
Quoted 3 lines
Sure. If we use handle to find oldprog, it should have the same handle.
cls_bpf_get()guarantees it. This check is redundant.
Good point, we should just test for 'oldprog && oldprog->handle != handle' and bail out then, otherwise it's just irritating. I can see to fix this up later. Seems fine to me then, thanks!
From: Cong Wang <[email protected]>
Date: Mon, 25 Sep 2017 10:13:49 -0700
Quoted 8 lines
Instead of calling
cls_bpf_get()in a loop to find a unused handle, just switch to idr API to allocate new handles.Cc Daniel Borkmann <[email protected]>Cc Chris Mi <[email protected]>Cc Jamal Hadi Salim <[email protected]>Signed-off-by Cong Wang <[email protected]>
Applied.