From: Jamal Hadi Salim <jhs@mojatatu.com>
To: Cong Wang <xiyou.wangcong@gmail.com>, netdev@vger.kernel.org
Cc: Thomas Graf <tgraf@suug.ch>, "David S. Miller" <davem@davemloft.net>
Subject: Re: [Patch net-next 6/7] net_sched: cls: move allocation in ->init to generic layer
Date: Sun, 12 Jan 2014 08:07:46 -0500 [thread overview]
Message-ID: <52D293A2.2070703@mojatatu.com> (raw)
In-Reply-To: <1389312845-10304-7-git-send-email-xiyou.wangcong@gmail.com>
On 01/09/14 19:14, Cong Wang wrote:
> Most of the filters need allocation of tp->root in ->init()
> and free it in ->destroy(), make this generic.
>
> Also we could reduce the use of tcf_tree_lock a bit.
>
> Cc: Thomas Graf <tgraf@suug.ch>
> Cc: David S. Miller <davem@davemloft.net>
> Cc: Jamal Hadi Salim <jhs@mojatatu.com>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
Hrm. This one worries me a little.
I dont see how just pre-allocing the private head of the classifier
magically allows you to get rid of locks. Have you tested against those
classifiers you changed?
If those locks are useless - then that is a separate patch to kill
them (sorry, dont have time to test myself right now).
cheers,
jamal
> ---
> include/net/sch_generic.h | 1 +
> net/sched/cls_api.c | 7 +++++++
> net/sched/cls_basic.c | 8 ++------
> net/sched/cls_bpf.c | 11 ++---------
> net/sched/cls_cgroup.c | 21 +++++++--------------
> net/sched/cls_flow.c | 8 ++------
> net/sched/cls_fw.c | 14 ++++----------
> net/sched/cls_route.c | 15 ++-------------
> net/sched/cls_rsvp.h | 10 ++--------
> net/sched/cls_tcindex.c | 13 ++-----------
> net/sched/cls_u32.c | 9 ++-------
> net/sched/sch_api.c | 1 +
> 12 files changed, 34 insertions(+), 84 deletions(-)
>
> diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
> index d062f81..819dc1d 100644
> --- a/include/net/sch_generic.h
> +++ b/include/net/sch_generic.h
> @@ -208,6 +208,7 @@ struct tcf_proto_ops {
> struct sk_buff *skb, struct tcmsg*);
>
> struct module *owner;
> + size_t root_size;
> };
>
> struct tcf_proto {
> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
> index 29a30a1..8460c75 100644
> --- a/net/sched/cls_api.c
> +++ b/net/sched/cls_api.c
> @@ -262,6 +262,13 @@ replay:
> tp->q = q;
> tp->classify = tp_ops->classify;
> tp->classid = parent;
> + tp->root = kzalloc(tp_ops->root_size, GFP_KERNEL);
> + if (!tp->root) {
> + err = -ENOBUFS;
> + module_put(tp_ops->owner);
> + kfree(tp);
> + goto errout;
> + }
>
> err = tp_ops->init(tp);
> if (err != 0) {
> diff --git a/net/sched/cls_basic.c b/net/sched/cls_basic.c
> index e98ca99..318f672 100644
> --- a/net/sched/cls_basic.c
> +++ b/net/sched/cls_basic.c
> @@ -75,13 +75,9 @@ static void basic_put(struct tcf_proto *tp, unsigned long f)
>
> static int basic_init(struct tcf_proto *tp)
> {
> - struct basic_head *head;
> + struct basic_head *head = tp->root;
>
> - head = kzalloc(sizeof(*head), GFP_KERNEL);
> - if (head == NULL)
> - return -ENOBUFS;
> INIT_LIST_HEAD(&head->flist);
> - tp->root = head;
> return 0;
> }
>
> @@ -102,7 +98,6 @@ static void basic_destroy(struct tcf_proto *tp)
> list_del(&f->link);
> basic_delete_filter(tp, f);
> }
> - kfree(head);
> }
>
> static int basic_delete(struct tcf_proto *tp, unsigned long arg)
> @@ -288,6 +283,7 @@ static struct tcf_proto_ops cls_basic_ops __read_mostly = {
> .walk = basic_walk,
> .dump = basic_dump,
> .owner = THIS_MODULE,
> + .root_size = sizeof(struct basic_head),
> };
>
> static int __init init_basic(void)
> diff --git a/net/sched/cls_bpf.c b/net/sched/cls_bpf.c
> index 8e3cf49..eedd296 100644
> --- a/net/sched/cls_bpf.c
> +++ b/net/sched/cls_bpf.c
> @@ -75,15 +75,9 @@ static int cls_bpf_classify(struct sk_buff *skb, const struct tcf_proto *tp,
>
> static int cls_bpf_init(struct tcf_proto *tp)
> {
> - struct cls_bpf_head *head;
> -
> - head = kzalloc(sizeof(*head), GFP_KERNEL);
> - if (head == NULL)
> - return -ENOBUFS;
> + struct cls_bpf_head *head = tp->root;
>
> INIT_LIST_HEAD(&head->plist);
> - tp->root = head;
> -
> return 0;
> }
>
> @@ -126,8 +120,6 @@ static void cls_bpf_destroy(struct tcf_proto *tp)
> list_del(&prog->link);
> cls_bpf_delete_prog(tp, prog);
> }
> -
> - kfree(head);
> }
>
> static unsigned long cls_bpf_get(struct tcf_proto *tp, u32 handle)
> @@ -366,6 +358,7 @@ static struct tcf_proto_ops cls_bpf_ops __read_mostly = {
> .delete = cls_bpf_delete,
> .walk = cls_bpf_walk,
> .dump = cls_bpf_dump,
> + .root_size = sizeof(struct cls_bpf_head),
> };
>
> static int __init cls_bpf_init_mod(void)
> diff --git a/net/sched/cls_cgroup.c b/net/sched/cls_cgroup.c
> index 8e2158a..4b7e083 100644
> --- a/net/sched/cls_cgroup.c
> +++ b/net/sched/cls_cgroup.c
> @@ -22,6 +22,7 @@ struct cls_cgroup_head {
> u32 handle;
> struct tcf_exts exts;
> struct tcf_ematch_tree ematches;
> + bool init;
> };
>
> static int cls_cgroup_classify(struct sk_buff *skb, const struct tcf_proto *tp,
> @@ -73,6 +74,9 @@ static void cls_cgroup_put(struct tcf_proto *tp, unsigned long f)
>
> static int cls_cgroup_init(struct tcf_proto *tp)
> {
> + struct cls_cgroup_head *head = tp->root;
> +
> + tcf_exts_init(&head->exts, TCA_CGROUP_ACT, TCA_CGROUP_POLICE);
> return 0;
> }
>
> @@ -94,20 +98,9 @@ static int cls_cgroup_change(struct net *net, struct sk_buff *in_skb,
> if (!tca[TCA_OPTIONS])
> return -EINVAL;
>
> - if (head == NULL) {
> - if (!handle)
> - return -EINVAL;
> -
> - head = kzalloc(sizeof(*head), GFP_KERNEL);
> - if (head == NULL)
> - return -ENOBUFS;
> -
> - tcf_exts_init(&head->exts, TCA_CGROUP_ACT, TCA_CGROUP_POLICE);
> + if (!head->init) {
> head->handle = handle;
> -
> - tcf_tree_lock(tp);
> - tp->root = head;
> - tcf_tree_unlock(tp);
> + head->init = true;
> }
>
> if (handle != head->handle)
> @@ -140,7 +133,6 @@ static void cls_cgroup_destroy(struct tcf_proto *tp)
> if (head) {
> tcf_exts_destroy(tp, &head->exts);
> tcf_em_tree_destroy(tp, &head->ematches);
> - kfree(head);
> }
> }
>
> @@ -205,6 +197,7 @@ static struct tcf_proto_ops cls_cgroup_ops __read_mostly = {
> .walk = cls_cgroup_walk,
> .dump = cls_cgroup_dump,
> .owner = THIS_MODULE,
> + .root_size = sizeof(struct cls_cgroup_head),
> };
>
> static int __init init_cgroup_cls(void)
> diff --git a/net/sched/cls_flow.c b/net/sched/cls_flow.c
> index 257029c..b39080a 100644
> --- a/net/sched/cls_flow.c
> +++ b/net/sched/cls_flow.c
> @@ -526,13 +526,9 @@ static int flow_delete(struct tcf_proto *tp, unsigned long arg)
>
> static int flow_init(struct tcf_proto *tp)
> {
> - struct flow_head *head;
> + struct flow_head *head = tp->root;
>
> - head = kzalloc(sizeof(*head), GFP_KERNEL);
> - if (head == NULL)
> - return -ENOBUFS;
> INIT_LIST_HEAD(&head->filters);
> - tp->root = head;
> return 0;
> }
>
> @@ -545,7 +541,6 @@ static void flow_destroy(struct tcf_proto *tp)
> list_del(&f->list);
> flow_destroy_filter(tp, f);
> }
> - kfree(head);
> }
>
> static unsigned long flow_get(struct tcf_proto *tp, u32 handle)
> @@ -653,6 +648,7 @@ static struct tcf_proto_ops cls_flow_ops __read_mostly = {
> .dump = flow_dump,
> .walk = flow_walk,
> .owner = THIS_MODULE,
> + .root_size = sizeof(struct flow_head),
> };
>
> static int __init cls_flow_init(void)
> diff --git a/net/sched/cls_fw.c b/net/sched/cls_fw.c
> index ed00e8c..73cd277 100644
> --- a/net/sched/cls_fw.c
> +++ b/net/sched/cls_fw.c
> @@ -34,6 +34,7 @@
> struct fw_head {
> struct fw_filter *ht[HTSIZE];
> u32 mask;
> + bool init;
> };
>
> struct fw_filter {
> @@ -155,7 +156,6 @@ static void fw_destroy(struct tcf_proto *tp)
> fw_delete_filter(tp, f);
> }
> }
> - kfree(head);
> }
>
> static int fw_delete(struct tcf_proto *tp, unsigned long arg)
> @@ -259,19 +259,12 @@ static int fw_change(struct net *net, struct sk_buff *in_skb,
> if (!handle)
> return -EINVAL;
>
> - if (head == NULL) {
> + if (!head->init) {
> u32 mask = 0xFFFFFFFF;
> if (tb[TCA_FW_MASK])
> mask = nla_get_u32(tb[TCA_FW_MASK]);
> -
> - head = kzalloc(sizeof(struct fw_head), GFP_KERNEL);
> - if (head == NULL)
> - return -ENOBUFS;
> head->mask = mask;
> -
> - tcf_tree_lock(tp);
> - tp->root = head;
> - tcf_tree_unlock(tp);
> + head->init = true;
> }
>
> f = kzalloc(sizeof(struct fw_filter), GFP_KERNEL);
> @@ -388,6 +381,7 @@ static struct tcf_proto_ops cls_fw_ops __read_mostly = {
> .walk = fw_walk,
> .dump = fw_dump,
> .owner = THIS_MODULE,
> + .root_size = sizeof(struct fw_head),
> };
>
> static int __init init_fw(void)
> diff --git a/net/sched/cls_route.c b/net/sched/cls_route.c
> index 1ad3068..038f35f 100644
> --- a/net/sched/cls_route.c
> +++ b/net/sched/cls_route.c
> @@ -279,7 +279,6 @@ static void route4_destroy(struct tcf_proto *tp)
> kfree(b);
> }
> }
> - kfree(head);
> }
>
> static int route4_delete(struct tcf_proto *tp, unsigned long arg)
> @@ -462,20 +461,9 @@ static int route4_change(struct net *net, struct sk_buff *in_skb,
> goto reinsert;
> }
>
> - err = -ENOBUFS;
> - if (head == NULL) {
> - head = kzalloc(sizeof(struct route4_head), GFP_KERNEL);
> - if (head == NULL)
> - goto errout;
> -
> - tcf_tree_lock(tp);
> - tp->root = head;
> - tcf_tree_unlock(tp);
> - }
> -
> f = kzalloc(sizeof(struct route4_filter), GFP_KERNEL);
> if (f == NULL)
> - goto errout;
> + return -ENOBUFS;
>
> tcf_exts_init(&f->exts, TCA_ROUTE4_ACT, TCA_ROUTE4_POLICE);
> err = route4_set_parms(net, tp, base, f, handle, head, tb,
> @@ -613,6 +601,7 @@ static struct tcf_proto_ops cls_route4_ops __read_mostly = {
> .walk = route4_walk,
> .dump = route4_dump,
> .owner = THIS_MODULE,
> + .root_size = sizeof(struct route4_head),
> };
>
> static int __init init_route4(void)
> diff --git a/net/sched/cls_rsvp.h b/net/sched/cls_rsvp.h
> index 19f8e5d..47930bc 100644
> --- a/net/sched/cls_rsvp.h
> +++ b/net/sched/cls_rsvp.h
> @@ -242,14 +242,7 @@ static void rsvp_put(struct tcf_proto *tp, unsigned long f)
>
> static int rsvp_init(struct tcf_proto *tp)
> {
> - struct rsvp_head *data;
> -
> - data = kzalloc(sizeof(struct rsvp_head), GFP_KERNEL);
> - if (data) {
> - tp->root = data;
> - return 0;
> - }
> - return -ENOBUFS;
> + return 0;
> }
>
> static void
> @@ -656,6 +649,7 @@ static struct tcf_proto_ops RSVP_OPS __read_mostly = {
> .walk = rsvp_walk,
> .dump = rsvp_dump,
> .owner = THIS_MODULE,
> + .root_size = sizeof(struct rsvp_head),
> };
>
> static int __init init_rsvp(void)
> diff --git a/net/sched/cls_tcindex.c b/net/sched/cls_tcindex.c
> index eed8404..6454158 100644
> --- a/net/sched/cls_tcindex.c
> +++ b/net/sched/cls_tcindex.c
> @@ -118,18 +118,11 @@ static void tcindex_put(struct tcf_proto *tp, unsigned long f)
>
> static int tcindex_init(struct tcf_proto *tp)
> {
> - struct tcindex_data *p;
> -
> - pr_debug("tcindex_init(tp %p)\n", tp);
> - p = kzalloc(sizeof(struct tcindex_data), GFP_KERNEL);
> - if (!p)
> - return -ENOMEM;
> + struct tcindex_data *p = tp->root;
>
> p->mask = 0xffff;
> p->hash = DEFAULT_HASH_SIZE;
> p->fall_through = 1;
> -
> - tp->root = p;
> return 0;
> }
>
> @@ -407,15 +400,12 @@ static void tcindex_destroy(struct tcf_proto *tp)
> struct tcindex_data *p = tp->root;
> struct tcf_walker walker;
>
> - pr_debug("tcindex_destroy(tp %p),p %p\n", tp, p);
> walker.count = 0;
> walker.skip = 0;
> walker.fn = &tcindex_destroy_element;
> tcindex_walk(tp, &walker);
> kfree(p->perfect);
> kfree(p->h);
> - kfree(p);
> - tp->root = NULL;
> }
>
>
> @@ -491,6 +481,7 @@ static struct tcf_proto_ops cls_tcindex_ops __read_mostly = {
> .walk = tcindex_walk,
> .dump = tcindex_dump,
> .owner = THIS_MODULE,
> + .root_size = sizeof(struct tcindex_data),
> };
>
> static int __init init_tcindex(void)
> diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
> index 84c28da..678c2d72 100644
> --- a/net/sched/cls_u32.c
> +++ b/net/sched/cls_u32.c
> @@ -300,15 +300,11 @@ static u32 gen_new_htid(struct tc_u_common *tp_c)
>
> static int u32_init(struct tcf_proto *tp)
> {
> - struct tc_u_hnode *root_ht;
> + struct tc_u_hnode *root_ht = tp->root;
> struct tc_u_common *tp_c;
>
> tp_c = tp->q->u32_node;
>
> - root_ht = kzalloc(sizeof(*root_ht), GFP_KERNEL);
> - if (root_ht == NULL)
> - return -ENOBUFS;
> -
> root_ht->divisor = 0;
> root_ht->refcnt++;
> root_ht->handle = tp_c ? gen_new_htid(tp_c) : 0x80000000;
> @@ -329,7 +325,6 @@ static int u32_init(struct tcf_proto *tp)
> tp_c->hlist = root_ht;
> root_ht->tp_c = tp_c;
>
> - tp->root = root_ht;
> tp->data = tp_c;
> return 0;
> }
> @@ -394,7 +389,6 @@ static int u32_destroy_hnode(struct tcf_proto *tp, struct tc_u_hnode *ht)
> for (hn = &tp_c->hlist; *hn; hn = &(*hn)->next) {
> if (*hn == ht) {
> *hn = ht->next;
> - kfree(ht);
> return 0;
> }
> }
> @@ -801,6 +795,7 @@ static struct tcf_proto_ops cls_u32_ops __read_mostly = {
> .walk = u32_walk,
> .dump = u32_dump,
> .owner = THIS_MODULE,
> + .root_size = sizeof(struct tc_u_hnode),
> };
>
> static int __init init_u32(void)
> diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
> index 1313145..5fef7f4 100644
> --- a/net/sched/sch_api.c
> +++ b/net/sched/sch_api.c
> @@ -1829,6 +1829,7 @@ EXPORT_SYMBOL(tc_classify);
> void tcf_destroy(struct tcf_proto *tp)
> {
> tp->ops->destroy(tp);
> + kfree(tp->root);
> module_put(tp->ops->owner);
> kfree(tp);
> }
>
next prev parent reply other threads:[~2014-01-12 13:07 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-01-10 0:13 [Patch net-next 0/7] net_sched: some more cleanup and improvements Cong Wang
2014-01-10 0:13 ` [Patch net-next 1/7] net_sched: act: move idx_gen into struct tcf_hashinfo Cong Wang
2014-01-12 12:34 ` Jamal Hadi Salim
2014-01-10 0:14 ` [Patch net-next 2/7] net_sched: act: clean up notification functions Cong Wang
2014-01-12 12:38 ` Jamal Hadi Salim
2014-01-10 0:14 ` [Patch net-next 3/7] net_sched: add struct net pointer to tcf_proto_ops->dump Cong Wang
2014-01-12 12:46 ` Jamal Hadi Salim
2014-01-10 0:14 ` [Patch net-next 4/7] net_sched: optimize tcf_match_indev() Cong Wang
2014-01-12 12:50 ` Jamal Hadi Salim
2014-01-10 0:14 ` [Patch net-next 5/7] net_sched: avoid casting void pointer Cong Wang
2014-01-12 12:55 ` Jamal Hadi Salim
2014-01-10 0:14 ` [Patch net-next 6/7] net_sched: cls: move allocation in ->init to generic layer Cong Wang
2014-01-12 13:07 ` Jamal Hadi Salim [this message]
2014-01-14 18:56 ` Cong Wang
2014-01-13 19:49 ` David Miller
2014-01-14 18:57 ` Cong Wang
2014-01-10 0:14 ` [Patch net-next 7/7] net_sched: act: remove struct tcf_act_hdr Cong Wang
2014-01-12 13:13 ` Jamal Hadi Salim
2014-01-12 12:32 ` [Patch net-next 0/7] net_sched: some more cleanup and improvements Jamal Hadi Salim
2014-01-14 18:54 ` Cong Wang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=52D293A2.2070703@mojatatu.com \
--to=jhs@mojatatu.com \
--cc=davem@davemloft.net \
--cc=netdev@vger.kernel.org \
--cc=tgraf@suug.ch \
--cc=xiyou.wangcong@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.