[Patch net-next 0/6] net_sched: act: more cleanup and improvement

[Patch net-next 0/6] net_sched: act: more cleanup and improvement

[Cong Wang]

Patch 1-5 are cleanup's for the structures of tc actions.
Patch 6 is a small improvement.
See each patch for details.

Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: David S. Miller <davem@davemloft.net>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>

Cong Wang (6):
  net_sched: act: fetch hinfo from a->ops->hinfo
  net_sched: act: export tcf_hash_search() instead of tcf_hash_lookup()
  net_sched: act: hide struct tcf_common from API
  net_sched: act: refactor cleanup ops
  net_sched: act: move tcf_hashinfo_init() into tcf_register_action()
  net_sched: act: refuse to remove bound action outside

 include/net/act_api.h           |  28 +++++------
 include/net/tc_act/tc_csum.h    |   4 +-
 include/net/tc_act/tc_defact.h  |   4 +-
 include/net/tc_act/tc_gact.h    |   4 +-
 include/net/tc_act/tc_ipt.h     |   4 +-
 include/net/tc_act/tc_mirred.h  |   4 +-
 include/net/tc_act/tc_nat.h     |   4 +-
 include/net/tc_act/tc_pedit.h   |   4 +-
 include/net/tc_act/tc_skbedit.h |   4 +-
 net/sched/act_api.c             | 107 +++++++++++++++++++++++++++-------------
 net/sched/act_csum.c            |  32 +++---------
 net/sched/act_gact.c            |  35 +++----------
 net/sched/act_ipt.c             |  69 +++++++-------------------
 net/sched/act_mirred.c          |  57 ++++++---------------
 net/sched/act_nat.c             |  34 +++----------
 net/sched/act_pedit.c           |  46 +++++------------
 net/sched/act_police.c          |  44 +++++------------
 net/sched/act_simple.c          |  65 ++++++------------------
 net/sched/act_skbedit.c         |  37 ++++----------
 19 files changed, 207 insertions(+), 379 deletions(-)

-- 
1.8.3.1

[Patch net-next 1/6] net_sched: act: fetch hinfo from a->ops->hinfo

[Cong Wang]

Every action ops has a pointer to hash info, so we don't need to
hard-code it in each module.

Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: David S. Miller <davem@davemloft.net>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 include/net/act_api.h   |  4 ++--
 net/sched/act_api.c     | 23 +++++++++++------------
 net/sched/act_csum.c    |  9 ++++-----
 net/sched/act_gact.c    | 11 +++++------
 net/sched/act_ipt.c     |  7 +++----
 net/sched/act_mirred.c  |  7 +++----
 net/sched/act_nat.c     |  9 ++++-----
 net/sched/act_pedit.c   |  9 ++++-----
 net/sched/act_police.c  | 18 ++++++++++--------
 net/sched/act_simple.c  |  7 +++----
 net/sched/act_skbedit.c |  9 ++++-----
 11 files changed, 53 insertions(+), 60 deletions(-)

diff --git a/include/net/act_api.h b/include/net/act_api.h
index e171387..ca1dcf5 100644
--- a/include/net/act_api.h
+++ b/include/net/act_api.h
@@ -105,10 +105,10 @@ int tcf_hash_release(struct tcf_common *p, int bind,
 		     struct tcf_hashinfo *hinfo);
 u32 tcf_hash_new_index(struct tcf_hashinfo *hinfo);
 struct tcf_common *tcf_hash_check(u32 index, struct tc_action *a,
-				  int bind, struct tcf_hashinfo *hinfo);
+				  int bind);
 struct tcf_common *tcf_hash_create(u32 index, struct nlattr *est,
 				   struct tc_action *a, int size,
-				   int bind, struct tcf_hashinfo *hinfo);
+				   int bind);
 void tcf_hash_insert(struct tcf_common *p, struct tcf_hashinfo *hinfo);
 
 int tcf_register_action(struct tc_action_ops *a);
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 35f89e9..b948253 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -62,8 +62,9 @@ int tcf_hash_release(struct tcf_common *p, int bind,
 EXPORT_SYMBOL(tcf_hash_release);
 
 static int tcf_dump_walker(struct sk_buff *skb, struct netlink_callback *cb,
-			   struct tc_action *a, struct tcf_hashinfo *hinfo)
+			   struct tc_action *a)
 {
+	struct tcf_hashinfo *hinfo = a->ops->hinfo;
 	struct hlist_head *head;
 	struct tcf_common *p;
 	int err = 0, index = -1, i = 0, s_i = 0, n_i = 0;
@@ -109,9 +110,9 @@ nla_put_failure:
 	goto done;
 }
 
-static int tcf_del_walker(struct sk_buff *skb, struct tc_action *a,
-			  struct tcf_hashinfo *hinfo)
+static int tcf_del_walker(struct sk_buff *skb, struct tc_action *a)
 {
+	struct tcf_hashinfo *hinfo = a->ops->hinfo;
 	struct hlist_head *head;
 	struct hlist_node *n;
 	struct tcf_common *p;
@@ -145,12 +146,10 @@ nla_put_failure:
 static int tcf_generic_walker(struct sk_buff *skb, struct netlink_callback *cb,
 			      int type, struct tc_action *a)
 {
-	struct tcf_hashinfo *hinfo = a->ops->hinfo;
-
 	if (type == RTM_DELACTION) {
-		return tcf_del_walker(skb, a, hinfo);
+		return tcf_del_walker(skb, a);
 	} else if (type == RTM_GETACTION) {
-		return tcf_dump_walker(skb, cb, a, hinfo);
+		return tcf_dump_walker(skb, cb, a);
 	} else {
 		WARN(1, "tcf_generic_walker: unknown action %d\n", type);
 		return -EINVAL;
@@ -199,9 +198,9 @@ static int tcf_hash_search(struct tc_action *a, u32 index)
 	return 0;
 }
 
-struct tcf_common *tcf_hash_check(u32 index, struct tc_action *a, int bind,
-				  struct tcf_hashinfo *hinfo)
+struct tcf_common *tcf_hash_check(u32 index, struct tc_action *a, int bind)
 {
+	struct tcf_hashinfo *hinfo = a->ops->hinfo;
 	struct tcf_common *p = NULL;
 	if (index && (p = tcf_hash_lookup(index, hinfo)) != NULL) {
 		if (bind)
@@ -214,9 +213,9 @@ struct tcf_common *tcf_hash_check(u32 index, struct tc_action *a, int bind,
 EXPORT_SYMBOL(tcf_hash_check);
 
 struct tcf_common *tcf_hash_create(u32 index, struct nlattr *est,
-				   struct tc_action *a, int size, int bind,
-				   struct tcf_hashinfo *hinfo)
+				   struct tc_action *a, int size, int bind)
 {
+	struct tcf_hashinfo *hinfo = a->ops->hinfo;
 	struct tcf_common *p = kzalloc(size, GFP_KERNEL);
 
 	if (unlikely(!p))
@@ -495,6 +494,7 @@ struct tc_action *tcf_action_init_1(struct net *net, struct nlattr *nla,
 	if (a == NULL)
 		goto err_mod;
 
+	a->ops = a_o;
 	INIT_LIST_HEAD(&a->list);
 	/* backward compatibility for policer */
 	if (name == NULL)
@@ -510,7 +510,6 @@ struct tc_action *tcf_action_init_1(struct net *net, struct nlattr *nla,
 	 */
 	if (err != ACT_P_CREATED)
 		module_put(a_o->owner);
-	a->ops = a_o;
 
 	return a;
 
diff --git a/net/sched/act_csum.c b/net/sched/act_csum.c
index ee28e1c..a9a3185 100644
--- a/net/sched/act_csum.c
+++ b/net/sched/act_csum.c
@@ -63,17 +63,16 @@ static int tcf_csum_init(struct net *n, struct nlattr *nla, struct nlattr *est,
 		return -EINVAL;
 	parm = nla_data(tb[TCA_CSUM_PARMS]);
 
-	pc = tcf_hash_check(parm->index, a, bind, &csum_hash_info);
+	pc = tcf_hash_check(parm->index, a, bind);
 	if (!pc) {
-		pc = tcf_hash_create(parm->index, est, a, sizeof(*p), bind,
-				     &csum_hash_info);
+		pc = tcf_hash_create(parm->index, est, a, sizeof(*p), bind);
 		if (IS_ERR(pc))
 			return PTR_ERR(pc);
 		ret = ACT_P_CREATED;
 	} else {
 		if (bind)/* dont override defaults */
 			return 0;
-		tcf_hash_release(pc, bind, &csum_hash_info);
+		tcf_hash_release(pc, bind, a->ops->hinfo);
 		if (!ovr)
 			return -EEXIST;
 	}
@@ -85,7 +84,7 @@ static int tcf_csum_init(struct net *n, struct nlattr *nla, struct nlattr *est,
 	spin_unlock_bh(&p->tcf_lock);
 
 	if (ret == ACT_P_CREATED)
-		tcf_hash_insert(pc, &csum_hash_info);
+		tcf_hash_insert(pc, a->ops->hinfo);
 
 	return ret;
 }
diff --git a/net/sched/act_gact.c b/net/sched/act_gact.c
index 3133307..5f05ecf 100644
--- a/net/sched/act_gact.c
+++ b/net/sched/act_gact.c
@@ -86,17 +86,16 @@ static int tcf_gact_init(struct net *net, struct nlattr *nla,
 	}
 #endif
 
-	pc = tcf_hash_check(parm->index, a, bind, &gact_hash_info);
+	pc = tcf_hash_check(parm->index, a, bind);
 	if (!pc) {
-		pc = tcf_hash_create(parm->index, est, a, sizeof(*gact),
-				     bind, &gact_hash_info);
+		pc = tcf_hash_create(parm->index, est, a, sizeof(*gact), bind);
 		if (IS_ERR(pc))
 			return PTR_ERR(pc);
 		ret = ACT_P_CREATED;
 	} else {
 		if (bind)/* dont override defaults */
 			return 0;
-		tcf_hash_release(pc, bind, &gact_hash_info);
+		tcf_hash_release(pc, bind, a->ops->hinfo);
 		if (!ovr)
 			return -EEXIST;
 	}
@@ -114,7 +113,7 @@ static int tcf_gact_init(struct net *net, struct nlattr *nla,
 #endif
 	spin_unlock_bh(&gact->tcf_lock);
 	if (ret == ACT_P_CREATED)
-		tcf_hash_insert(pc, &gact_hash_info);
+		tcf_hash_insert(pc, a->ops->hinfo);
 	return ret;
 }
 
@@ -123,7 +122,7 @@ static int tcf_gact_cleanup(struct tc_action *a, int bind)
 	struct tcf_gact *gact = a->priv;
 
 	if (gact)
-		return tcf_hash_release(&gact->common, bind, &gact_hash_info);
+		return tcf_hash_release(&gact->common, bind, a->ops->hinfo);
 	return 0;
 }
 
diff --git a/net/sched/act_ipt.c b/net/sched/act_ipt.c
index bc9f498..a22602b 100644
--- a/net/sched/act_ipt.c
+++ b/net/sched/act_ipt.c
@@ -125,10 +125,9 @@ static int tcf_ipt_init(struct net *net, struct nlattr *nla, struct nlattr *est,
 	if (tb[TCA_IPT_INDEX] != NULL)
 		index = nla_get_u32(tb[TCA_IPT_INDEX]);
 
-	pc = tcf_hash_check(index, a, bind, &ipt_hash_info);
+	pc = tcf_hash_check(index, a, bind);
 	if (!pc) {
-		pc = tcf_hash_create(index, est, a, sizeof(*ipt), bind,
-				     &ipt_hash_info);
+		pc = tcf_hash_create(index, est, a, sizeof(*ipt), bind);
 		if (IS_ERR(pc))
 			return PTR_ERR(pc);
 		ret = ACT_P_CREATED;
@@ -171,7 +170,7 @@ static int tcf_ipt_init(struct net *net, struct nlattr *nla, struct nlattr *est,
 	ipt->tcfi_hook  = hook;
 	spin_unlock_bh(&ipt->tcf_lock);
 	if (ret == ACT_P_CREATED)
-		tcf_hash_insert(pc, &ipt_hash_info);
+		tcf_hash_insert(pc, a->ops->hinfo);
 	return ret;
 
 err3:
diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
index 5d05b57..f1fb5bc 100644
--- a/net/sched/act_mirred.c
+++ b/net/sched/act_mirred.c
@@ -101,12 +101,11 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
 		dev = NULL;
 	}
 
-	pc = tcf_hash_check(parm->index, a, bind, &mirred_hash_info);
+	pc = tcf_hash_check(parm->index, a, bind);
 	if (!pc) {
 		if (dev == NULL)
 			return -EINVAL;
-		pc = tcf_hash_create(parm->index, est, a, sizeof(*m), bind,
-				     &mirred_hash_info);
+		pc = tcf_hash_create(parm->index, est, a, sizeof(*m), bind);
 		if (IS_ERR(pc))
 			return PTR_ERR(pc);
 		ret = ACT_P_CREATED;
@@ -132,7 +131,7 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
 	spin_unlock_bh(&m->tcf_lock);
 	if (ret == ACT_P_CREATED) {
 		list_add(&m->tcfm_list, &mirred_list);
-		tcf_hash_insert(pc, &mirred_hash_info);
+		tcf_hash_insert(pc, a->ops->hinfo);
 	}
 
 	return ret;
diff --git a/net/sched/act_nat.c b/net/sched/act_nat.c
index a49fa23..5fc1364 100644
--- a/net/sched/act_nat.c
+++ b/net/sched/act_nat.c
@@ -57,17 +57,16 @@ static int tcf_nat_init(struct net *net, struct nlattr *nla, struct nlattr *est,
 		return -EINVAL;
 	parm = nla_data(tb[TCA_NAT_PARMS]);
 
-	pc = tcf_hash_check(parm->index, a, bind, &nat_hash_info);
+	pc = tcf_hash_check(parm->index, a, bind);
 	if (!pc) {
-		pc = tcf_hash_create(parm->index, est, a, sizeof(*p), bind,
-				     &nat_hash_info);
+		pc = tcf_hash_create(parm->index, est, a, sizeof(*p), bind);
 		if (IS_ERR(pc))
 			return PTR_ERR(pc);
 		ret = ACT_P_CREATED;
 	} else {
 		if (bind)
 			return 0;
-		tcf_hash_release(pc, bind, &nat_hash_info);
+		tcf_hash_release(pc, bind, a->ops->hinfo);
 		if (!ovr)
 			return -EEXIST;
 	}
@@ -83,7 +82,7 @@ static int tcf_nat_init(struct net *net, struct nlattr *nla, struct nlattr *est,
 	spin_unlock_bh(&p->tcf_lock);
 
 	if (ret == ACT_P_CREATED)
-		tcf_hash_insert(pc, &nat_hash_info);
+		tcf_hash_insert(pc, a->ops->hinfo);
 
 	return ret;
 }
diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
index f361e4e..142ff73 100644
--- a/net/sched/act_pedit.c
+++ b/net/sched/act_pedit.c
@@ -57,12 +57,11 @@ static int tcf_pedit_init(struct net *net, struct nlattr *nla,
 	if (nla_len(tb[TCA_PEDIT_PARMS]) < sizeof(*parm) + ksize)
 		return -EINVAL;
 
-	pc = tcf_hash_check(parm->index, a, bind, &pedit_hash_info);
+	pc = tcf_hash_check(parm->index, a, bind);
 	if (!pc) {
 		if (!parm->nkeys)
 			return -EINVAL;
-		pc = tcf_hash_create(parm->index, est, a, sizeof(*p), bind,
-				     &pedit_hash_info);
+		pc = tcf_hash_create(parm->index, est, a, sizeof(*p), bind);
 		if (IS_ERR(pc))
 			return PTR_ERR(pc);
 		p = to_pedit(pc);
@@ -77,7 +76,7 @@ static int tcf_pedit_init(struct net *net, struct nlattr *nla,
 		ret = ACT_P_CREATED;
 	} else {
 		p = to_pedit(pc);
-		tcf_hash_release(pc, bind, &pedit_hash_info);
+		tcf_hash_release(pc, bind, a->ops->hinfo);
 		if (bind)
 			return 0;
 		if (!ovr)
@@ -101,7 +100,7 @@ static int tcf_pedit_init(struct net *net, struct nlattr *nla,
 	memcpy(p->tcfp_keys, parm->keys, ksize);
 	spin_unlock_bh(&p->tcf_lock);
 	if (ret == ACT_P_CREATED)
-		tcf_hash_insert(pc, &pedit_hash_info);
+		tcf_hash_insert(pc, a->ops->hinfo);
 	return ret;
 }
 
diff --git a/net/sched/act_police.c b/net/sched/act_police.c
index 5ba467b..30d4f4b 100644
--- a/net/sched/act_police.c
+++ b/net/sched/act_police.c
@@ -59,17 +59,18 @@ struct tc_police_compat {
 static int tcf_act_police_walker(struct sk_buff *skb, struct netlink_callback *cb,
 			      int type, struct tc_action *a)
 {
+	struct tcf_hashinfo *hinfo = a->ops->hinfo;
 	struct hlist_head *head;
 	struct tcf_common *p;
 	int err = 0, index = -1, i = 0, s_i = 0, n_i = 0;
 	struct nlattr *nest;
 
-	spin_lock_bh(&police_hash_info.lock);
+	spin_lock_bh(&hinfo->lock);
 
 	s_i = cb->args[0];
 
 	for (i = 0; i < (POL_TAB_MASK + 1); i++) {
-		head = &police_hash_info.htab[tcf_hash(i, POL_TAB_MASK)];
+		head = &hinfo->htab[tcf_hash(i, POL_TAB_MASK)];
 
 		hlist_for_each_entry_rcu(p, head, tcfc_head) {
 			index++;
@@ -94,7 +95,7 @@ static int tcf_act_police_walker(struct sk_buff *skb, struct netlink_callback *c
 		}
 	}
 done:
-	spin_unlock_bh(&police_hash_info.lock);
+	spin_unlock_bh(&hinfo->lock);
 	if (n_i)
 		cb->args[0] += n_i;
 	return n_i;
@@ -121,6 +122,7 @@ static int tcf_act_police_locate(struct net *net, struct nlattr *nla,
 	struct tc_police *parm;
 	struct tcf_police *police;
 	struct qdisc_rate_table *R_tab = NULL, *P_tab = NULL;
+	struct tcf_hashinfo *hinfo = a->ops->hinfo;
 	int size;
 
 	if (nla == NULL)
@@ -140,7 +142,7 @@ static int tcf_act_police_locate(struct net *net, struct nlattr *nla,
 	if (parm->index) {
 		struct tcf_common *pc;
 
-		pc = tcf_hash_lookup(parm->index, &police_hash_info);
+		pc = tcf_hash_lookup(parm->index, hinfo);
 		if (pc != NULL) {
 			a->priv = pc;
 			police = to_police(pc);
@@ -236,11 +238,11 @@ override:
 
 	police->tcfp_t_c = ktime_to_ns(ktime_get());
 	police->tcf_index = parm->index ? parm->index :
-		tcf_hash_new_index(&police_hash_info);
+		tcf_hash_new_index(a->ops->hinfo);
 	h = tcf_hash(police->tcf_index, POL_TAB_MASK);
-	spin_lock_bh(&police_hash_info.lock);
-	hlist_add_head(&police->tcf_head, &police_hash_info.htab[h]);
-	spin_unlock_bh(&police_hash_info.lock);
+	spin_lock_bh(&hinfo->lock);
+	hlist_add_head(&police->tcf_head, &hinfo->htab[h]);
+	spin_unlock_bh(&hinfo->lock);
 
 	a->priv = police;
 	return ret;
diff --git a/net/sched/act_simple.c b/net/sched/act_simple.c
index f7d5406..745d0d5 100644
--- a/net/sched/act_simple.c
+++ b/net/sched/act_simple.c
@@ -114,10 +114,9 @@ static int tcf_simp_init(struct net *net, struct nlattr *nla,
 	parm = nla_data(tb[TCA_DEF_PARMS]);
 	defdata = nla_data(tb[TCA_DEF_DATA]);
 
-	pc = tcf_hash_check(parm->index, a, bind, &simp_hash_info);
+	pc = tcf_hash_check(parm->index, a, bind);
 	if (!pc) {
-		pc = tcf_hash_create(parm->index, est, a, sizeof(*d), bind,
-				     &simp_hash_info);
+		pc = tcf_hash_create(parm->index, est, a, sizeof(*d), bind);
 		if (IS_ERR(pc))
 			return PTR_ERR(pc);
 
@@ -145,7 +144,7 @@ static int tcf_simp_init(struct net *net, struct nlattr *nla,
 	}
 
 	if (ret == ACT_P_CREATED)
-		tcf_hash_insert(pc, &simp_hash_info);
+		tcf_hash_insert(pc, a->ops->hinfo);
 	return ret;
 }
 
diff --git a/net/sched/act_skbedit.c b/net/sched/act_skbedit.c
index 74af461..bedbe5b 100644
--- a/net/sched/act_skbedit.c
+++ b/net/sched/act_skbedit.c
@@ -100,10 +100,9 @@ static int tcf_skbedit_init(struct net *net, struct nlattr *nla,
 
 	parm = nla_data(tb[TCA_SKBEDIT_PARMS]);
 
-	pc = tcf_hash_check(parm->index, a, bind, &skbedit_hash_info);
+	pc = tcf_hash_check(parm->index, a, bind);
 	if (!pc) {
-		pc = tcf_hash_create(parm->index, est, a, sizeof(*d), bind,
-				     &skbedit_hash_info);
+		pc = tcf_hash_create(parm->index, est, a, sizeof(*d), bind);
 		if (IS_ERR(pc))
 			return PTR_ERR(pc);
 
@@ -113,7 +112,7 @@ static int tcf_skbedit_init(struct net *net, struct nlattr *nla,
 		d = to_skbedit(pc);
 		if (bind)
 			return 0;
-		tcf_hash_release(pc, bind, &skbedit_hash_info);
+		tcf_hash_release(pc, bind, a->ops->hinfo);
 		if (!ovr)
 			return -EEXIST;
 	}
@@ -133,7 +132,7 @@ static int tcf_skbedit_init(struct net *net, struct nlattr *nla,
 	spin_unlock_bh(&d->tcf_lock);
 
 	if (ret == ACT_P_CREATED)
-		tcf_hash_insert(pc, &skbedit_hash_info);
+		tcf_hash_insert(pc, a->ops->hinfo);
 	return ret;
 }
 
-- 
1.8.3.1

Re: [Patch net-next 1/6] net_sched: act: fetch hinfo from a->ops->hinfo

[Jamal Hadi Salim]

On 01/17/14 14:37, Cong Wang wrote:
> Every action ops has a pointer to hash info, so we don't need to
> hard-code it in each module.
>
> Cc: Jamal Hadi Salim <jhs@mojatatu.com>
> Cc: David S. Miller <davem@davemloft.net>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>

Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>

Although changes like the one below (which are in a few places)
are necessary i.e it was more readable before.


> diff --git a/net/sched/act_csum.c b/net/sched/act_csum.c
> index ee28e1c..a9a3185 100644
> --- a/net/sched/act_csum.c
> +++ b/net/sched/act_csum.c

> -		tcf_hash_release(pc, bind, &csum_hash_info);
> +		tcf_hash_release(pc, bind, a->ops->hinfo);
>   		if (!ovr)

cheers,
jamal

Re: [Patch net-next 1/6] net_sched: act: fetch hinfo from a->ops->hinfo

[David Miller]

From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Fri, 17 Jan 2014 11:37:02 -0800

> Every action ops has a pointer to hash info, so we don't need to
> hard-code it in each module.
> 
> Cc: Jamal Hadi Salim <jhs@mojatatu.com>
> Cc: David S. Miller <davem@davemloft.net>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>

Applied.

[Patch net-next 2/6] net_sched: act: export tcf_hash_search() instead of tcf_hash_lookup()

[Cong Wang]

So that we will not expose struct tcf_common to modules.

Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: David S. Miller <davem@davemloft.net>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 include/net/act_api.h  | 2 +-
 net/sched/act_api.c    | 6 +++---
 net/sched/act_police.c | 8 ++------
 3 files changed, 6 insertions(+), 10 deletions(-)

diff --git a/include/net/act_api.h b/include/net/act_api.h
index ca1dcf5..3b76a30 100644
--- a/include/net/act_api.h
+++ b/include/net/act_api.h
@@ -99,7 +99,7 @@ struct tc_action_ops {
 	int     (*walk)(struct sk_buff *, struct netlink_callback *, int, struct tc_action *);
 };
 
-struct tcf_common *tcf_hash_lookup(u32 index, struct tcf_hashinfo *hinfo);
+int tcf_hash_search(struct tc_action *a, u32 index);
 void tcf_hash_destroy(struct tcf_common *p, struct tcf_hashinfo *hinfo);
 int tcf_hash_release(struct tcf_common *p, int bind,
 		     struct tcf_hashinfo *hinfo);
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index b948253..72bdc71 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -156,7 +156,7 @@ static int tcf_generic_walker(struct sk_buff *skb, struct netlink_callback *cb,
 	}
 }
 
-struct tcf_common *tcf_hash_lookup(u32 index, struct tcf_hashinfo *hinfo)
+static struct tcf_common *tcf_hash_lookup(u32 index, struct tcf_hashinfo *hinfo)
 {
 	struct tcf_common *p = NULL;
 	struct hlist_head *head;
@@ -170,7 +170,6 @@ struct tcf_common *tcf_hash_lookup(u32 index, struct tcf_hashinfo *hinfo)
 
 	return p;
 }
-EXPORT_SYMBOL(tcf_hash_lookup);
 
 u32 tcf_hash_new_index(struct tcf_hashinfo *hinfo)
 {
@@ -186,7 +185,7 @@ u32 tcf_hash_new_index(struct tcf_hashinfo *hinfo)
 }
 EXPORT_SYMBOL(tcf_hash_new_index);
 
-static int tcf_hash_search(struct tc_action *a, u32 index)
+int tcf_hash_search(struct tc_action *a, u32 index)
 {
 	struct tcf_hashinfo *hinfo = a->ops->hinfo;
 	struct tcf_common *p = tcf_hash_lookup(index, hinfo);
@@ -197,6 +196,7 @@ static int tcf_hash_search(struct tc_action *a, u32 index)
 	}
 	return 0;
 }
+EXPORT_SYMBOL(tcf_hash_search);
 
 struct tcf_common *tcf_hash_check(u32 index, struct tc_action *a, int bind)
 {
diff --git a/net/sched/act_police.c b/net/sched/act_police.c
index 30d4f4b..e51f7d9 100644
--- a/net/sched/act_police.c
+++ b/net/sched/act_police.c
@@ -140,12 +140,8 @@ static int tcf_act_police_locate(struct net *net, struct nlattr *nla,
 	parm = nla_data(tb[TCA_POLICE_TBF]);
 
 	if (parm->index) {
-		struct tcf_common *pc;
-
-		pc = tcf_hash_lookup(parm->index, hinfo);
-		if (pc != NULL) {
-			a->priv = pc;
-			police = to_police(pc);
+		if (tcf_hash_search(a, parm->index)) {
+			police = to_police(a->priv);
 			if (bind) {
 				police->tcf_bindcnt += 1;
 				police->tcf_refcnt += 1;
-- 
1.8.3.1

Re: [Patch net-next 2/6] net_sched: act: export tcf_hash_search() instead of tcf_hash_lookup()

[Jamal Hadi Salim]

On 01/17/14 14:37, Cong Wang wrote:
> So that we will not expose struct tcf_common to modules.
>
> Cc: Jamal Hadi Salim <jhs@mojatatu.com>
> Cc: David S. Miller <davem@davemloft.net>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
>


Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>


cheers,
jamal

Re: [Patch net-next 2/6] net_sched: act: export tcf_hash_search() instead of tcf_hash_lookup()

[David Miller]

From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Fri, 17 Jan 2014 11:37:03 -0800

> So that we will not expose struct tcf_common to modules.
> 
> Cc: Jamal Hadi Salim <jhs@mojatatu.com>
> Cc: David S. Miller <davem@davemloft.net>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>

Applied.

[Patch net-next 3/6] net_sched: act: hide struct tcf_common from API

[Cong Wang]

Now we can totally hide it from modules. tcf_hash_*() API's
will operate on struct tc_action, modules don't need to care about
the details.

Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: David S. Miller <davem@davemloft.net>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 include/net/act_api.h           | 16 +++++++---------
 include/net/tc_act/tc_csum.h    |  4 ++--
 include/net/tc_act/tc_defact.h  |  4 ++--
 include/net/tc_act/tc_gact.h    |  4 ++--
 include/net/tc_act/tc_ipt.h     |  4 ++--
 include/net/tc_act/tc_mirred.h  |  4 ++--
 include/net/tc_act/tc_nat.h     |  4 ++--
 include/net/tc_act/tc_pedit.h   |  4 ++--
 include/net/tc_act/tc_skbedit.h |  4 ++--
 net/sched/act_api.c             | 42 ++++++++++++++++++++++++++++-------------
 net/sched/act_csum.c            | 24 ++++++++---------------
 net/sched/act_gact.c            | 27 ++++++++------------------
 net/sched/act_ipt.c             | 39 ++++++++++++++------------------------
 net/sched/act_mirred.c          | 32 +++++++++++--------------------
 net/sched/act_nat.c             | 25 ++++++++----------------
 net/sched/act_pedit.c           | 25 ++++++++++--------------
 net/sched/act_simple.c          | 39 +++++++++++++-------------------------
 net/sched/act_skbedit.c         | 29 +++++++++-------------------
 18 files changed, 133 insertions(+), 197 deletions(-)

diff --git a/include/net/act_api.h b/include/net/act_api.h
index 3b76a30..f2b5cb3 100644
--- a/include/net/act_api.h
+++ b/include/net/act_api.h
@@ -100,16 +100,14 @@ struct tc_action_ops {
 };
 
 int tcf_hash_search(struct tc_action *a, u32 index);
-void tcf_hash_destroy(struct tcf_common *p, struct tcf_hashinfo *hinfo);
-int tcf_hash_release(struct tcf_common *p, int bind,
-		     struct tcf_hashinfo *hinfo);
+void tcf_hash_destroy(struct tc_action *a);
+int tcf_hash_release(struct tc_action *a, int bind);
 u32 tcf_hash_new_index(struct tcf_hashinfo *hinfo);
-struct tcf_common *tcf_hash_check(u32 index, struct tc_action *a,
-				  int bind);
-struct tcf_common *tcf_hash_create(u32 index, struct nlattr *est,
-				   struct tc_action *a, int size,
-				   int bind);
-void tcf_hash_insert(struct tcf_common *p, struct tcf_hashinfo *hinfo);
+int tcf_hash_check(u32 index, struct tc_action *a, int bind);
+int tcf_hash_create(u32 index, struct nlattr *est, struct tc_action *a,
+		    int size, int bind);
+void tcf_hash_cleanup(struct tc_action *a, struct nlattr *est);
+void tcf_hash_insert(struct tc_action *a);
 
 int tcf_register_action(struct tc_action_ops *a);
 int tcf_unregister_action(struct tc_action_ops *a);
diff --git a/include/net/tc_act/tc_csum.h b/include/net/tc_act/tc_csum.h
index 9e8710b..fa8f5fa 100644
--- a/include/net/tc_act/tc_csum.h
+++ b/include/net/tc_act/tc_csum.h
@@ -9,7 +9,7 @@ struct tcf_csum {
 
 	u32 update_flags;
 };
-#define to_tcf_csum(pc) \
-	container_of(pc,struct tcf_csum,common)
+#define to_tcf_csum(a) \
+	container_of(a->priv,struct tcf_csum,common)
 
 #endif /* __NET_TC_CSUM_H */
diff --git a/include/net/tc_act/tc_defact.h b/include/net/tc_act/tc_defact.h
index 65f024b..9763dcb 100644
--- a/include/net/tc_act/tc_defact.h
+++ b/include/net/tc_act/tc_defact.h
@@ -8,7 +8,7 @@ struct tcf_defact {
 	u32     		tcfd_datalen;
 	void    		*tcfd_defdata;
 };
-#define to_defact(pc) \
-	container_of(pc, struct tcf_defact, common)
+#define to_defact(a) \
+	container_of(a->priv, struct tcf_defact, common)
 
 #endif /* __NET_TC_DEF_H */
diff --git a/include/net/tc_act/tc_gact.h b/include/net/tc_act/tc_gact.h
index 9e3f676..9fc9b57 100644
--- a/include/net/tc_act/tc_gact.h
+++ b/include/net/tc_act/tc_gact.h
@@ -11,7 +11,7 @@ struct tcf_gact {
         int			tcfg_paction;
 #endif
 };
-#define to_gact(pc) \
-	container_of(pc, struct tcf_gact, common)
+#define to_gact(a) \
+	container_of(a->priv, struct tcf_gact, common)
 
 #endif /* __NET_TC_GACT_H */
diff --git a/include/net/tc_act/tc_ipt.h b/include/net/tc_act/tc_ipt.h
index f7d25df..c0f4193 100644
--- a/include/net/tc_act/tc_ipt.h
+++ b/include/net/tc_act/tc_ipt.h
@@ -11,7 +11,7 @@ struct tcf_ipt {
 	char			*tcfi_tname;
 	struct xt_entry_target	*tcfi_t;
 };
-#define to_ipt(pc) \
-	container_of(pc, struct tcf_ipt, common)
+#define to_ipt(a) \
+	container_of(a->priv, struct tcf_ipt, common)
 
 #endif /* __NET_TC_IPT_H */
diff --git a/include/net/tc_act/tc_mirred.h b/include/net/tc_act/tc_mirred.h
index cfe2943..4dd77a1 100644
--- a/include/net/tc_act/tc_mirred.h
+++ b/include/net/tc_act/tc_mirred.h
@@ -11,7 +11,7 @@ struct tcf_mirred {
 	struct net_device	*tcfm_dev;
 	struct list_head	tcfm_list;
 };
-#define to_mirred(pc) \
-	container_of(pc, struct tcf_mirred, common)
+#define to_mirred(a) \
+	container_of(a->priv, struct tcf_mirred, common)
 
 #endif /* __NET_TC_MIR_H */
diff --git a/include/net/tc_act/tc_nat.h b/include/net/tc_act/tc_nat.h
index 4a691f3..63d8e9c 100644
--- a/include/net/tc_act/tc_nat.h
+++ b/include/net/tc_act/tc_nat.h
@@ -13,9 +13,9 @@ struct tcf_nat {
 	u32 flags;
 };
 
-static inline struct tcf_nat *to_tcf_nat(struct tcf_common *pc)
+static inline struct tcf_nat *to_tcf_nat(struct tc_action *a)
 {
-	return container_of(pc, struct tcf_nat, common);
+	return container_of(a->priv, struct tcf_nat, common);
 }
 
 #endif /* __NET_TC_NAT_H */
diff --git a/include/net/tc_act/tc_pedit.h b/include/net/tc_act/tc_pedit.h
index e6f6e15..5b80998 100644
--- a/include/net/tc_act/tc_pedit.h
+++ b/include/net/tc_act/tc_pedit.h
@@ -9,7 +9,7 @@ struct tcf_pedit {
 	unsigned char		tcfp_flags;
 	struct tc_pedit_key	*tcfp_keys;
 };
-#define to_pedit(pc) \
-	container_of(pc, struct tcf_pedit, common)
+#define to_pedit(a) \
+	container_of(a->priv, struct tcf_pedit, common)
 
 #endif /* __NET_TC_PED_H */
diff --git a/include/net/tc_act/tc_skbedit.h b/include/net/tc_act/tc_skbedit.h
index dd5d86f..0df9a0d 100644
--- a/include/net/tc_act/tc_skbedit.h
+++ b/include/net/tc_act/tc_skbedit.h
@@ -29,7 +29,7 @@ struct tcf_skbedit {
 	u16			queue_mapping;
 	/* XXX: 16-bit pad here? */
 };
-#define to_skbedit(pc) \
-	container_of(pc, struct tcf_skbedit, common)
+#define to_skbedit(a) \
+	container_of(a->priv, struct tcf_skbedit, common)
 
 #endif /* __NET_TC_SKBEDIT_H */
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 72bdc71..91b489e 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -27,8 +27,11 @@
 #include <net/act_api.h>
 #include <net/netlink.h>
 
-void tcf_hash_destroy(struct tcf_common *p, struct tcf_hashinfo *hinfo)
+void tcf_hash_destroy(struct tc_action *a)
 {
+	struct tcf_common *p = a->priv;
+	struct tcf_hashinfo *hinfo = a->ops->hinfo;
+
 	spin_lock_bh(&hinfo->lock);
 	hlist_del(&p->tcfc_head);
 	spin_unlock_bh(&hinfo->lock);
@@ -42,9 +45,9 @@ void tcf_hash_destroy(struct tcf_common *p, struct tcf_hashinfo *hinfo)
 }
 EXPORT_SYMBOL(tcf_hash_destroy);
 
-int tcf_hash_release(struct tcf_common *p, int bind,
-		     struct tcf_hashinfo *hinfo)
+int tcf_hash_release(struct tc_action *a, int bind)
 {
+	struct tcf_common *p = a->priv;
 	int ret = 0;
 
 	if (p) {
@@ -53,7 +56,7 @@ int tcf_hash_release(struct tcf_common *p, int bind,
 
 		p->tcfc_refcnt--;
 		if (p->tcfc_bindcnt <= 0 && p->tcfc_refcnt <= 0) {
-			tcf_hash_destroy(p, hinfo);
+			tcf_hash_destroy(a);
 			ret = 1;
 		}
 	}
@@ -127,7 +130,7 @@ static int tcf_del_walker(struct sk_buff *skb, struct tc_action *a)
 	for (i = 0; i < (hinfo->hmask + 1); i++) {
 		head = &hinfo->htab[tcf_hash(i, hinfo->hmask)];
 		hlist_for_each_entry_safe(p, n, head, tcfc_head) {
-			if (ACT_P_DELETED == tcf_hash_release(p, 0, hinfo)) {
+			if (ACT_P_DELETED == tcf_hash_release(a, 0)) {
 				module_put(a->ops->owner);
 				n_i++;
 			}
@@ -198,7 +201,7 @@ int tcf_hash_search(struct tc_action *a, u32 index)
 }
 EXPORT_SYMBOL(tcf_hash_search);
 
-struct tcf_common *tcf_hash_check(u32 index, struct tc_action *a, int bind)
+int tcf_hash_check(u32 index, struct tc_action *a, int bind)
 {
 	struct tcf_hashinfo *hinfo = a->ops->hinfo;
 	struct tcf_common *p = NULL;
@@ -207,19 +210,30 @@ struct tcf_common *tcf_hash_check(u32 index, struct tc_action *a, int bind)
 			p->tcfc_bindcnt++;
 		p->tcfc_refcnt++;
 		a->priv = p;
+		return 1;
 	}
-	return p;
+	return 0;
 }
 EXPORT_SYMBOL(tcf_hash_check);
 
-struct tcf_common *tcf_hash_create(u32 index, struct nlattr *est,
-				   struct tc_action *a, int size, int bind)
+void tcf_hash_cleanup(struct tc_action *a, struct nlattr *est)
+{
+	struct tcf_common *pc = a->priv;
+	if (est)
+		gen_kill_estimator(&pc->tcfc_bstats,
+				   &pc->tcfc_rate_est);
+	kfree_rcu(pc, tcfc_rcu);
+}
+EXPORT_SYMBOL(tcf_hash_cleanup);
+
+int tcf_hash_create(u32 index, struct nlattr *est, struct tc_action *a,
+		    int size, int bind)
 {
 	struct tcf_hashinfo *hinfo = a->ops->hinfo;
 	struct tcf_common *p = kzalloc(size, GFP_KERNEL);
 
 	if (unlikely(!p))
-		return ERR_PTR(-ENOMEM);
+		return -ENOMEM;
 	p->tcfc_refcnt = 1;
 	if (bind)
 		p->tcfc_bindcnt = 1;
@@ -234,17 +248,19 @@ struct tcf_common *tcf_hash_create(u32 index, struct nlattr *est,
 					    &p->tcfc_lock, est);
 		if (err) {
 			kfree(p);
-			return ERR_PTR(err);
+			return err;
 		}
 	}
 
 	a->priv = (void *) p;
-	return p;
+	return 0;
 }
 EXPORT_SYMBOL(tcf_hash_create);
 
-void tcf_hash_insert(struct tcf_common *p, struct tcf_hashinfo *hinfo)
+void tcf_hash_insert(struct tc_action *a)
 {
+	struct tcf_common *p = a->priv;
+	struct tcf_hashinfo *hinfo = a->ops->hinfo;
 	unsigned int h = tcf_hash(p->tcfc_index, hinfo->hmask);
 
 	spin_lock_bh(&hinfo->lock);
diff --git a/net/sched/act_csum.c b/net/sched/act_csum.c
index a9a3185..8e75a08 100644
--- a/net/sched/act_csum.c
+++ b/net/sched/act_csum.c
@@ -48,7 +48,6 @@ static int tcf_csum_init(struct net *n, struct nlattr *nla, struct nlattr *est,
 {
 	struct nlattr *tb[TCA_CSUM_MAX + 1];
 	struct tc_csum *parm;
-	struct tcf_common *pc;
 	struct tcf_csum *p;
 	int ret = 0, err;
 
@@ -63,38 +62,31 @@ static int tcf_csum_init(struct net *n, struct nlattr *nla, struct nlattr *est,
 		return -EINVAL;
 	parm = nla_data(tb[TCA_CSUM_PARMS]);
 
-	pc = tcf_hash_check(parm->index, a, bind);
-	if (!pc) {
-		pc = tcf_hash_create(parm->index, est, a, sizeof(*p), bind);
-		if (IS_ERR(pc))
-			return PTR_ERR(pc);
+	if (!tcf_hash_check(parm->index, a, bind)) {
+		ret = tcf_hash_create(parm->index, est, a, sizeof(*p), bind);
+		if (ret)
+			return ret;
 		ret = ACT_P_CREATED;
 	} else {
 		if (bind)/* dont override defaults */
 			return 0;
-		tcf_hash_release(pc, bind, a->ops->hinfo);
+		tcf_hash_release(a, bind);
 		if (!ovr)
 			return -EEXIST;
 	}
 
-	p = to_tcf_csum(pc);
+	p = to_tcf_csum(a);
 	spin_lock_bh(&p->tcf_lock);
 	p->tcf_action = parm->action;
 	p->update_flags = parm->update_flags;
 	spin_unlock_bh(&p->tcf_lock);
 
 	if (ret == ACT_P_CREATED)
-		tcf_hash_insert(pc, a->ops->hinfo);
+		tcf_hash_insert(a);
 
 	return ret;
 }
 
-static int tcf_csum_cleanup(struct tc_action *a, int bind)
-{
-	struct tcf_csum *p = a->priv;
-	return tcf_hash_release(&p->common, bind, &csum_hash_info);
-}
-
 /**
  * tcf_csum_skb_nextlayer - Get next layer pointer
  * @skb: sk_buff to use
@@ -575,7 +567,7 @@ static struct tc_action_ops act_csum_ops = {
 	.owner		= THIS_MODULE,
 	.act		= tcf_csum,
 	.dump		= tcf_csum_dump,
-	.cleanup	= tcf_csum_cleanup,
+	.cleanup	= tcf_hash_release,
 	.init		= tcf_csum_init,
 };
 
diff --git a/net/sched/act_gact.c b/net/sched/act_gact.c
index 5f05ecf..47148fa 100644
--- a/net/sched/act_gact.c
+++ b/net/sched/act_gact.c
@@ -57,7 +57,6 @@ static int tcf_gact_init(struct net *net, struct nlattr *nla,
 	struct nlattr *tb[TCA_GACT_MAX + 1];
 	struct tc_gact *parm;
 	struct tcf_gact *gact;
-	struct tcf_common *pc;
 	int ret = 0;
 	int err;
 #ifdef CONFIG_GACT_PROB
@@ -86,21 +85,20 @@ static int tcf_gact_init(struct net *net, struct nlattr *nla,
 	}
 #endif
 
-	pc = tcf_hash_check(parm->index, a, bind);
-	if (!pc) {
-		pc = tcf_hash_create(parm->index, est, a, sizeof(*gact), bind);
-		if (IS_ERR(pc))
-			return PTR_ERR(pc);
+	if (!tcf_hash_check(parm->index, a, bind)) {
+		ret = tcf_hash_create(parm->index, est, a, sizeof(*gact), bind);
+		if (ret)
+			return ret;
 		ret = ACT_P_CREATED;
 	} else {
 		if (bind)/* dont override defaults */
 			return 0;
-		tcf_hash_release(pc, bind, a->ops->hinfo);
+		tcf_hash_release(a, bind);
 		if (!ovr)
 			return -EEXIST;
 	}
 
-	gact = to_gact(pc);
+	gact = to_gact(a);
 
 	spin_lock_bh(&gact->tcf_lock);
 	gact->tcf_action = parm->action;
@@ -113,19 +111,10 @@ static int tcf_gact_init(struct net *net, struct nlattr *nla,
 #endif
 	spin_unlock_bh(&gact->tcf_lock);
 	if (ret == ACT_P_CREATED)
-		tcf_hash_insert(pc, a->ops->hinfo);
+		tcf_hash_insert(a);
 	return ret;
 }
 
-static int tcf_gact_cleanup(struct tc_action *a, int bind)
-{
-	struct tcf_gact *gact = a->priv;
-
-	if (gact)
-		return tcf_hash_release(&gact->common, bind, a->ops->hinfo);
-	return 0;
-}
-
 static int tcf_gact(struct sk_buff *skb, const struct tc_action *a,
 		    struct tcf_result *res)
 {
@@ -197,7 +186,7 @@ static struct tc_action_ops act_gact_ops = {
 	.owner		=	THIS_MODULE,
 	.act		=	tcf_gact,
 	.dump		=	tcf_gact_dump,
-	.cleanup	=	tcf_gact_cleanup,
+	.cleanup	=	tcf_hash_release,
 	.init		=	tcf_gact_init,
 };
 
diff --git a/net/sched/act_ipt.c b/net/sched/act_ipt.c
index a22602b..f0545fb 100644
--- a/net/sched/act_ipt.c
+++ b/net/sched/act_ipt.c
@@ -69,8 +69,9 @@ static void ipt_destroy_target(struct xt_entry_target *t)
 	module_put(par.target->me);
 }
 
-static int tcf_ipt_release(struct tcf_ipt *ipt, int bind)
+static int tcf_ipt_release(struct tc_action *a, int bind)
 {
+	struct tcf_ipt *ipt = to_ipt(a);
 	int ret = 0;
 	if (ipt) {
 		if (bind)
@@ -80,7 +81,7 @@ static int tcf_ipt_release(struct tcf_ipt *ipt, int bind)
 			ipt_destroy_target(ipt->tcfi_t);
 			kfree(ipt->tcfi_tname);
 			kfree(ipt->tcfi_t);
-			tcf_hash_destroy(&ipt->common, &ipt_hash_info);
+			tcf_hash_destroy(a);
 			ret = ACT_P_DELETED;
 		}
 	}
@@ -99,7 +100,6 @@ static int tcf_ipt_init(struct net *net, struct nlattr *nla, struct nlattr *est,
 {
 	struct nlattr *tb[TCA_IPT_MAX + 1];
 	struct tcf_ipt *ipt;
-	struct tcf_common *pc;
 	struct xt_entry_target *td, *t;
 	char *tname;
 	int ret = 0, err;
@@ -125,21 +125,20 @@ static int tcf_ipt_init(struct net *net, struct nlattr *nla, struct nlattr *est,
 	if (tb[TCA_IPT_INDEX] != NULL)
 		index = nla_get_u32(tb[TCA_IPT_INDEX]);
 
-	pc = tcf_hash_check(index, a, bind);
-	if (!pc) {
-		pc = tcf_hash_create(index, est, a, sizeof(*ipt), bind);
-		if (IS_ERR(pc))
-			return PTR_ERR(pc);
+	if (!tcf_hash_check(index, a, bind) ) {
+		ret = tcf_hash_create(index, est, a, sizeof(*ipt), bind);
+		if (ret)
+			return ret;
 		ret = ACT_P_CREATED;
 	} else {
 		if (bind)/* dont override defaults */
 			return 0;
-		tcf_ipt_release(to_ipt(pc), bind);
+		tcf_ipt_release(a, bind);
 
 		if (!ovr)
 			return -EEXIST;
 	}
-	ipt = to_ipt(pc);
+	ipt = to_ipt(a);
 
 	hook = nla_get_u32(tb[TCA_IPT_HOOK]);
 
@@ -170,7 +169,7 @@ static int tcf_ipt_init(struct net *net, struct nlattr *nla, struct nlattr *est,
 	ipt->tcfi_hook  = hook;
 	spin_unlock_bh(&ipt->tcf_lock);
 	if (ret == ACT_P_CREATED)
-		tcf_hash_insert(pc, a->ops->hinfo);
+		tcf_hash_insert(a);
 	return ret;
 
 err3:
@@ -178,21 +177,11 @@ err3:
 err2:
 	kfree(tname);
 err1:
-	if (ret == ACT_P_CREATED) {
-		if (est)
-			gen_kill_estimator(&pc->tcfc_bstats,
-					   &pc->tcfc_rate_est);
-		kfree_rcu(pc, tcfc_rcu);
-	}
+	if (ret == ACT_P_CREATED)
+		tcf_hash_cleanup(a, est);
 	return err;
 }
 
-static int tcf_ipt_cleanup(struct tc_action *a, int bind)
-{
-	struct tcf_ipt *ipt = a->priv;
-	return tcf_ipt_release(ipt, bind);
-}
-
 static int tcf_ipt(struct sk_buff *skb, const struct tc_action *a,
 		   struct tcf_result *res)
 {
@@ -290,7 +279,7 @@ static struct tc_action_ops act_ipt_ops = {
 	.owner		=	THIS_MODULE,
 	.act		=	tcf_ipt,
 	.dump		=	tcf_ipt_dump,
-	.cleanup	=	tcf_ipt_cleanup,
+	.cleanup	=	tcf_ipt_release,
 	.init		=	tcf_ipt_init,
 };
 
@@ -302,7 +291,7 @@ static struct tc_action_ops act_xt_ops = {
 	.owner		=	THIS_MODULE,
 	.act		=	tcf_ipt,
 	.dump		=	tcf_ipt_dump,
-	.cleanup	=	tcf_ipt_cleanup,
+	.cleanup	=	tcf_ipt_release,
 	.init		=	tcf_ipt_init,
 };
 
diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
index f1fb5bc..f94a58f 100644
--- a/net/sched/act_mirred.c
+++ b/net/sched/act_mirred.c
@@ -33,8 +33,9 @@
 static LIST_HEAD(mirred_list);
 static struct tcf_hashinfo mirred_hash_info;
 
-static int tcf_mirred_release(struct tcf_mirred *m, int bind)
+static int tcf_mirred_release(struct tc_action *a, int bind)
 {
+	struct tcf_mirred *m = to_mirred(a);
 	if (m) {
 		if (bind)
 			m->tcf_bindcnt--;
@@ -43,7 +44,7 @@ static int tcf_mirred_release(struct tcf_mirred *m, int bind)
 			list_del(&m->tcfm_list);
 			if (m->tcfm_dev)
 				dev_put(m->tcfm_dev);
-			tcf_hash_destroy(&m->common, &mirred_hash_info);
+			tcf_hash_destroy(a);
 			return 1;
 		}
 	}
@@ -61,7 +62,6 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
 	struct nlattr *tb[TCA_MIRRED_MAX + 1];
 	struct tc_mirred *parm;
 	struct tcf_mirred *m;
-	struct tcf_common *pc;
 	struct net_device *dev;
 	int ret, ok_push = 0;
 
@@ -101,21 +101,20 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
 		dev = NULL;
 	}
 
-	pc = tcf_hash_check(parm->index, a, bind);
-	if (!pc) {
+	if (!tcf_hash_check(parm->index, a, bind)) {
 		if (dev == NULL)
 			return -EINVAL;
-		pc = tcf_hash_create(parm->index, est, a, sizeof(*m), bind);
-		if (IS_ERR(pc))
-			return PTR_ERR(pc);
+		ret = tcf_hash_create(parm->index, est, a, sizeof(*m), bind);
+		if (ret)
+			return ret;
 		ret = ACT_P_CREATED;
 	} else {
 		if (!ovr) {
-			tcf_mirred_release(to_mirred(pc), bind);
+			tcf_mirred_release(a, bind);
 			return -EEXIST;
 		}
 	}
-	m = to_mirred(pc);
+	m = to_mirred(a);
 
 	spin_lock_bh(&m->tcf_lock);
 	m->tcf_action = parm->action;
@@ -131,21 +130,12 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
 	spin_unlock_bh(&m->tcf_lock);
 	if (ret == ACT_P_CREATED) {
 		list_add(&m->tcfm_list, &mirred_list);
-		tcf_hash_insert(pc, a->ops->hinfo);
+		tcf_hash_insert(a);
 	}
 
 	return ret;
 }
 
-static int tcf_mirred_cleanup(struct tc_action *a, int bind)
-{
-	struct tcf_mirred *m = a->priv;
-
-	if (m)
-		return tcf_mirred_release(m, bind);
-	return 0;
-}
-
 static int tcf_mirred(struct sk_buff *skb, const struct tc_action *a,
 		      struct tcf_result *res)
 {
@@ -260,7 +250,7 @@ static struct tc_action_ops act_mirred_ops = {
 	.owner		=	THIS_MODULE,
 	.act		=	tcf_mirred,
 	.dump		=	tcf_mirred_dump,
-	.cleanup	=	tcf_mirred_cleanup,
+	.cleanup	=	tcf_mirred_release,
 	.init		=	tcf_mirred_init,
 };
 
diff --git a/net/sched/act_nat.c b/net/sched/act_nat.c
index 5fc1364..c8d1843 100644
--- a/net/sched/act_nat.c
+++ b/net/sched/act_nat.c
@@ -44,7 +44,6 @@ static int tcf_nat_init(struct net *net, struct nlattr *nla, struct nlattr *est,
 	struct tc_nat *parm;
 	int ret = 0, err;
 	struct tcf_nat *p;
-	struct tcf_common *pc;
 
 	if (nla == NULL)
 		return -EINVAL;
@@ -57,20 +56,19 @@ static int tcf_nat_init(struct net *net, struct nlattr *nla, struct nlattr *est,
 		return -EINVAL;
 	parm = nla_data(tb[TCA_NAT_PARMS]);
 
-	pc = tcf_hash_check(parm->index, a, bind);
-	if (!pc) {
-		pc = tcf_hash_create(parm->index, est, a, sizeof(*p), bind);
-		if (IS_ERR(pc))
-			return PTR_ERR(pc);
+	if (!tcf_hash_check(parm->index, a, bind)) {
+		ret = tcf_hash_create(parm->index, est, a, sizeof(*p), bind);
+		if (ret)
+			return ret;
 		ret = ACT_P_CREATED;
 	} else {
 		if (bind)
 			return 0;
-		tcf_hash_release(pc, bind, a->ops->hinfo);
+		tcf_hash_release(a, bind);
 		if (!ovr)
 			return -EEXIST;
 	}
-	p = to_tcf_nat(pc);
+	p = to_tcf_nat(a);
 
 	spin_lock_bh(&p->tcf_lock);
 	p->old_addr = parm->old_addr;
@@ -82,18 +80,11 @@ static int tcf_nat_init(struct net *net, struct nlattr *nla, struct nlattr *est,
 	spin_unlock_bh(&p->tcf_lock);
 
 	if (ret == ACT_P_CREATED)
-		tcf_hash_insert(pc, a->ops->hinfo);
+		tcf_hash_insert(a);
 
 	return ret;
 }
 
-static int tcf_nat_cleanup(struct tc_action *a, int bind)
-{
-	struct tcf_nat *p = a->priv;
-
-	return tcf_hash_release(&p->common, bind, &nat_hash_info);
-}
-
 static int tcf_nat(struct sk_buff *skb, const struct tc_action *a,
 		   struct tcf_result *res)
 {
@@ -299,7 +290,7 @@ static struct tc_action_ops act_nat_ops = {
 	.owner		=	THIS_MODULE,
 	.act		=	tcf_nat,
 	.dump		=	tcf_nat_dump,
-	.cleanup	=	tcf_nat_cleanup,
+	.cleanup	=	tcf_hash_release,
 	.init		=	tcf_nat_init,
 };
 
diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
index 142ff73..75f08e4 100644
--- a/net/sched/act_pedit.c
+++ b/net/sched/act_pedit.c
@@ -39,7 +39,6 @@ static int tcf_pedit_init(struct net *net, struct nlattr *nla,
 	struct tc_pedit *parm;
 	int ret = 0, err;
 	struct tcf_pedit *p;
-	struct tcf_common *pc;
 	struct tc_pedit_key *keys = NULL;
 	int ksize;
 
@@ -57,26 +56,22 @@ static int tcf_pedit_init(struct net *net, struct nlattr *nla,
 	if (nla_len(tb[TCA_PEDIT_PARMS]) < sizeof(*parm) + ksize)
 		return -EINVAL;
 
-	pc = tcf_hash_check(parm->index, a, bind);
-	if (!pc) {
+	if (!tcf_hash_check(parm->index, a, bind)) {
 		if (!parm->nkeys)
 			return -EINVAL;
-		pc = tcf_hash_create(parm->index, est, a, sizeof(*p), bind);
-		if (IS_ERR(pc))
-			return PTR_ERR(pc);
-		p = to_pedit(pc);
+		ret = tcf_hash_create(parm->index, est, a, sizeof(*p), bind);
+		if (ret)
+			return ret;
+		p = to_pedit(a);
 		keys = kmalloc(ksize, GFP_KERNEL);
 		if (keys == NULL) {
-			if (est)
-				gen_kill_estimator(&pc->tcfc_bstats,
-						   &pc->tcfc_rate_est);
-			kfree_rcu(pc, tcfc_rcu);
+			tcf_hash_cleanup(a, est);
 			return -ENOMEM;
 		}
 		ret = ACT_P_CREATED;
 	} else {
-		p = to_pedit(pc);
-		tcf_hash_release(pc, bind, a->ops->hinfo);
+		p = to_pedit(a);
+		tcf_hash_release(a, bind);
 		if (bind)
 			return 0;
 		if (!ovr)
@@ -100,7 +95,7 @@ static int tcf_pedit_init(struct net *net, struct nlattr *nla,
 	memcpy(p->tcfp_keys, parm->keys, ksize);
 	spin_unlock_bh(&p->tcf_lock);
 	if (ret == ACT_P_CREATED)
-		tcf_hash_insert(pc, a->ops->hinfo);
+		tcf_hash_insert(a);
 	return ret;
 }
 
@@ -110,7 +105,7 @@ static int tcf_pedit_cleanup(struct tc_action *a, int bind)
 
 	if (p) {
 		struct tc_pedit_key *keys = p->tcfp_keys;
-		if (tcf_hash_release(&p->common, bind, &pedit_hash_info)) {
+		if (tcf_hash_release(a, bind)) {
 			kfree(keys);
 			return 1;
 		}
diff --git a/net/sched/act_simple.c b/net/sched/act_simple.c
index 745d0d5..e52ee06 100644
--- a/net/sched/act_simple.c
+++ b/net/sched/act_simple.c
@@ -47,8 +47,9 @@ static int tcf_simp(struct sk_buff *skb, const struct tc_action *a,
 	return d->tcf_action;
 }
 
-static int tcf_simp_release(struct tcf_defact *d, int bind)
+static int tcf_simp_release(struct tc_action *a, int bind)
 {
+	struct tcf_defact *d = to_defact(a);
 	int ret = 0;
 	if (d) {
 		if (bind)
@@ -56,7 +57,7 @@ static int tcf_simp_release(struct tcf_defact *d, int bind)
 		d->tcf_refcnt--;
 		if (d->tcf_bindcnt <= 0 && d->tcf_refcnt <= 0) {
 			kfree(d->tcfd_defdata);
-			tcf_hash_destroy(&d->common, &simp_hash_info);
+			tcf_hash_destroy(a);
 			ret = 1;
 		}
 	}
@@ -94,7 +95,6 @@ static int tcf_simp_init(struct net *net, struct nlattr *nla,
 	struct nlattr *tb[TCA_DEF_MAX + 1];
 	struct tc_defact *parm;
 	struct tcf_defact *d;
-	struct tcf_common *pc;
 	char *defdata;
 	int ret = 0, err;
 
@@ -114,29 +114,25 @@ static int tcf_simp_init(struct net *net, struct nlattr *nla,
 	parm = nla_data(tb[TCA_DEF_PARMS]);
 	defdata = nla_data(tb[TCA_DEF_DATA]);
 
-	pc = tcf_hash_check(parm->index, a, bind);
-	if (!pc) {
-		pc = tcf_hash_create(parm->index, est, a, sizeof(*d), bind);
-		if (IS_ERR(pc))
-			return PTR_ERR(pc);
+	if (!tcf_hash_check(parm->index, a, bind)) {
+		ret = tcf_hash_create(parm->index, est, a, sizeof(*d), bind);
+		if (ret)
+			return ret;
 
-		d = to_defact(pc);
+		d = to_defact(a);
 		ret = alloc_defdata(d, defdata);
 		if (ret < 0) {
-			if (est)
-				gen_kill_estimator(&pc->tcfc_bstats,
-						   &pc->tcfc_rate_est);
-			kfree_rcu(pc, tcfc_rcu);
+			tcf_hash_cleanup(a, est);
 			return ret;
 		}
 		d->tcf_action = parm->action;
 		ret = ACT_P_CREATED;
 	} else {
-		d = to_defact(pc);
+		d = to_defact(a);
 
 		if (bind)
 			return 0;
-		tcf_simp_release(d, bind);
+		tcf_simp_release(a, bind);
 		if (!ovr)
 			return -EEXIST;
 
@@ -144,19 +140,10 @@ static int tcf_simp_init(struct net *net, struct nlattr *nla,
 	}
 
 	if (ret == ACT_P_CREATED)
-		tcf_hash_insert(pc, a->ops->hinfo);
+		tcf_hash_insert(a);
 	return ret;
 }
 
-static int tcf_simp_cleanup(struct tc_action *a, int bind)
-{
-	struct tcf_defact *d = a->priv;
-
-	if (d)
-		return tcf_simp_release(d, bind);
-	return 0;
-}
-
 static int tcf_simp_dump(struct sk_buff *skb, struct tc_action *a,
 			 int bind, int ref)
 {
@@ -193,7 +180,7 @@ static struct tc_action_ops act_simp_ops = {
 	.owner		=	THIS_MODULE,
 	.act		=	tcf_simp,
 	.dump		=	tcf_simp_dump,
-	.cleanup	=	tcf_simp_cleanup,
+	.cleanup	=	tcf_simp_release,
 	.init		=	tcf_simp_init,
 };
 
diff --git a/net/sched/act_skbedit.c b/net/sched/act_skbedit.c
index bedbe5b..68ab57b 100644
--- a/net/sched/act_skbedit.c
+++ b/net/sched/act_skbedit.c
@@ -65,7 +65,6 @@ static int tcf_skbedit_init(struct net *net, struct nlattr *nla,
 	struct nlattr *tb[TCA_SKBEDIT_MAX + 1];
 	struct tc_skbedit *parm;
 	struct tcf_skbedit *d;
-	struct tcf_common *pc;
 	u32 flags = 0, *priority = NULL, *mark = NULL;
 	u16 *queue_mapping = NULL;
 	int ret = 0, err;
@@ -100,19 +99,18 @@ static int tcf_skbedit_init(struct net *net, struct nlattr *nla,
 
 	parm = nla_data(tb[TCA_SKBEDIT_PARMS]);
 
-	pc = tcf_hash_check(parm->index, a, bind);
-	if (!pc) {
-		pc = tcf_hash_create(parm->index, est, a, sizeof(*d), bind);
-		if (IS_ERR(pc))
-			return PTR_ERR(pc);
+	if (!tcf_hash_check(parm->index, a, bind)) {
+		ret = tcf_hash_create(parm->index, est, a, sizeof(*d), bind);
+		if (ret)
+			return ret;
 
-		d = to_skbedit(pc);
+		d = to_skbedit(a);
 		ret = ACT_P_CREATED;
 	} else {
-		d = to_skbedit(pc);
+		d = to_skbedit(a);
 		if (bind)
 			return 0;
-		tcf_hash_release(pc, bind, a->ops->hinfo);
+		tcf_hash_release(a, bind);
 		if (!ovr)
 			return -EEXIST;
 	}
@@ -132,19 +130,10 @@ static int tcf_skbedit_init(struct net *net, struct nlattr *nla,
 	spin_unlock_bh(&d->tcf_lock);
 
 	if (ret == ACT_P_CREATED)
-		tcf_hash_insert(pc, a->ops->hinfo);
+		tcf_hash_insert(a);
 	return ret;
 }
 
-static int tcf_skbedit_cleanup(struct tc_action *a, int bind)
-{
-	struct tcf_skbedit *d = a->priv;
-
-	if (d)
-		return tcf_hash_release(&d->common, bind, &skbedit_hash_info);
-	return 0;
-}
-
 static int tcf_skbedit_dump(struct sk_buff *skb, struct tc_action *a,
 			    int bind, int ref)
 {
@@ -192,7 +181,7 @@ static struct tc_action_ops act_skbedit_ops = {
 	.owner		=	THIS_MODULE,
 	.act		=	tcf_skbedit,
 	.dump		=	tcf_skbedit_dump,
-	.cleanup	=	tcf_skbedit_cleanup,
+	.cleanup	=	tcf_hash_release,
 	.init		=	tcf_skbedit_init,
 };
 
-- 
1.8.3.1

Re: [Patch net-next 3/6] net_sched: act: hide struct tcf_common from API

[Jamal Hadi Salim]

On 01/17/14 14:37, Cong Wang wrote:
> Now we can totally hide it from modules. tcf_hash_*() API's
> will operate on struct tc_action, modules don't need to care about
> the details.
>
> Cc: Jamal Hadi Salim <jhs@mojatatu.com>
> Cc: David S. Miller <davem@davemloft.net>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>


Looks good.
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>

Re: [Patch net-next 3/6] net_sched: act: hide struct tcf_common from API

[Jamal Hadi Salim]

On 01/17/14 14:37, Cong Wang wrote:
> Now we can totally hide it from modules. tcf_hash_*() API's
> will operate on struct tc_action, modules don't need to care about
> the details.
>
> Cc: Jamal Hadi Salim <jhs@mojatatu.com>
> Cc: David S. Miller <davem@davemloft.net>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>

Had to stare at this a bit longer - I am afraid
this and rest look a little suspect.
Can you run some tests for me after your patches?
I could do it in about 1 day if you dont have time.

---
#add a couple of tests
sudo tc actions add action drop index 10
sudo tc actions add action drop index 12
# now list them - should see both.
sudo tc actions ls action gact
#now flush them
sudo tc actions flush action gact
# now list them - should see them gone
sudo tc actions ls action gact
---

And for the last patch, in particular
---
#add two actions by index
sudo tc actions add action drop index 10
sudo tc actions add action ok index 12
# we need an ingress qdisc to attach filter to
sudo tc qdisc del dev lo parent ffff:
sudo tc qdisc add dev lo ingress
#use existing action index 10
sudo tc filter add dev lo parent ffff: protocol ip prio 8 \
u32 match ip dst 127.0.0.8/32 flowid 1:10 action gact index 10
#double bind
sudo tc filter add dev lo parent ffff: protocol ip prio 7 \
u32 match ip src 127.0.0.10/32 flowid 1:11 action gact index 10
# now lets see the filters..
sudo tc filter ls dev lo parent ffff: protocol ip
#display the actions and pay attention to the bind count
sudo tc actions ls action gact
#try to readd an existing action
sudo tc actions add action ok index 12
#it should be rejected - now list it and make sure refcnt doesnt go up
sudo tc actions ls action gact
#delete action index 12 (which is not bound)
sudo tc actions del action gact index 12
#list and make sure index 12 is gone
sudo tc actions ls action gact
#delete action index 10 (which is bound)
sudo tc actions del action gact index 10
#display to see it is still there ..
sudo tc actions ls action gact
#Repeat above two steps several times and make sure action 10 stays
# action should not be deleted...
#
# delete qdisc - which should delete all filters but not
# action that were not created by filters
sudo tc qdisc del dev lo parent ffff:
#ok now that filter is gone, lets see the actions ..
#pay attention to binds and references
sudo tc actions ls action gact
#
#delete action index 10 (which is no longer bound)
sudo tc actions del action gact index 10
#display to see it is gone
sudo tc actions ls action gact


cheers,
jamal

Re: [Patch net-next 3/6] net_sched: act: hide struct tcf_common from API

[Jamal Hadi Salim]

Cong,

I applied the patches and run the tests below.
The first one is broken for sure.
The second one has different behavior - but then I rembered
i had an offline discussion with you and i think this is fine.

Please chase whatever these issues are and fix; if the tests pass
you could go ahead and add my signed-off.

Much thanks for your efforts to make this better.

cheers,
jamal

On 01/20/14 08:01, Jamal Hadi Salim wrote:
> On 01/17/14 14:37, Cong Wang wrote:
>> Now we can totally hide it from modules. tcf_hash_*() API's
>> will operate on struct tc_action, modules don't need to care about
>> the details.
>>
>> Cc: Jamal Hadi Salim <jhs@mojatatu.com>
>> Cc: David S. Miller <davem@davemloft.net>
>> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
>
> Had to stare at this a bit longer - I am afraid
> this and rest look a little suspect.
> Can you run some tests for me after your patches?
> I could do it in about 1 day if you dont have time.
>
> ---
> #add a couple of tests
> sudo tc actions add action drop index 10
> sudo tc actions add action drop index 12
> # now list them - should see both.
> sudo tc actions ls action gact
> #now flush them
> sudo tc actions flush action gact
> # now list them - should see them gone
> sudo tc actions ls action gact
> ---
>
> And for the last patch, in particular
> ---
> #add two actions by index
> sudo tc actions add action drop index 10
> sudo tc actions add action ok index 12
> # we need an ingress qdisc to attach filter to
> sudo tc qdisc del dev lo parent ffff:
> sudo tc qdisc add dev lo ingress
> #use existing action index 10
> sudo tc filter add dev lo parent ffff: protocol ip prio 8 \
> u32 match ip dst 127.0.0.8/32 flowid 1:10 action gact index 10
> #double bind
> sudo tc filter add dev lo parent ffff: protocol ip prio 7 \
> u32 match ip src 127.0.0.10/32 flowid 1:11 action gact index 10
> # now lets see the filters..
> sudo tc filter ls dev lo parent ffff: protocol ip
> #display the actions and pay attention to the bind count
> sudo tc actions ls action gact
> #try to readd an existing action
> sudo tc actions add action ok index 12
> #it should be rejected - now list it and make sure refcnt doesnt go up
> sudo tc actions ls action gact
> #delete action index 12 (which is not bound)
> sudo tc actions del action gact index 12
> #list and make sure index 12 is gone
> sudo tc actions ls action gact
> #delete action index 10 (which is bound)
> sudo tc actions del action gact index 10
> #display to see it is still there ..
> sudo tc actions ls action gact
> #Repeat above two steps several times and make sure action 10 stays
> # action should not be deleted...
> #
> # delete qdisc - which should delete all filters but not
> # action that were not created by filters
> sudo tc qdisc del dev lo parent ffff:
> #ok now that filter is gone, lets see the actions ..
> #pay attention to binds and references
> sudo tc actions ls action gact
> #
> #delete action index 10 (which is no longer bound)
> sudo tc actions del action gact index 10
> #display to see it is gone
> sudo tc actions ls action gact
>
>
> cheers,
> jamal
>
>
>
>
>
>
>
>
>
>
>

Re: [Patch net-next 3/6] net_sched: act: hide struct tcf_common from API

[Cong Wang]

On Wed, Jan 22, 2014 at 4:44 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> Cong,
>
> I applied the patches and run the tests below.
> The first one is broken for sure.
> The second one has different behavior - but then I rembered
> i had an offline discussion with you and i think this is fine.
>
> Please chase whatever these issues are and fix; if the tests pass
> you could go ahead and add my signed-off.
>

Hi, Jamal

I had similar tests locally, I will check why the first of your test
failed tomorrow.

For the second, yeah actions bound to a filter refuse to be removed
after my patches, this is intentional.

Thanks!

Re: [Patch net-next 3/6] net_sched: act: hide struct tcf_common from API

[Cong Wang]

On Wed, Jan 22, 2014 at 11:10 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>
> Hi, Jamal
>
> I had similar tests locally, I will check why the first of your test
> failed tomorrow.

I see, something was missed:

diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index fd9042b..4640ad3 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -135,6 +135,7 @@ static int tcf_del_walker(struct sk_buff *skb,
struct tc_action *a)
        for (i = 0; i < (hinfo->hmask + 1); i++) {
                head = &hinfo->htab[tcf_hash(i, hinfo->hmask)];
                hlist_for_each_entry_safe(p, n, head, tcfc_head) {
+                       a->priv = p;
                        ret = tcf_hash_release(a, 0);
                        if (ret == ACT_P_DELETED) {
                                module_put(a->ops->owner);

I will update and resend the last 4 patches in this series.

[Patch net-next 4/6] net_sched: act: refactor cleanup ops

[Cong Wang]

For bindcnt and refcnt etc., they are common for all actions,
not need to repeat such operations for their own, they can be unified
now. Actions just need to do its specific cleanup if needed.

Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: David S. Miller <davem@davemloft.net>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 include/net/act_api.h   |  2 +-
 net/sched/act_api.c     |  8 +++++---
 net/sched/act_csum.c    |  1 -
 net/sched/act_gact.c    |  1 -
 net/sched/act_ipt.c     | 21 +++++----------------
 net/sched/act_mirred.c  | 20 +++++---------------
 net/sched/act_nat.c     |  1 -
 net/sched/act_pedit.c   | 13 +++----------
 net/sched/act_police.c  |  9 ---------
 net/sched/act_simple.c  | 17 +++--------------
 net/sched/act_skbedit.c |  1 -
 11 files changed, 22 insertions(+), 72 deletions(-)

diff --git a/include/net/act_api.h b/include/net/act_api.h
index f2b5cb3..9263ace 100644
--- a/include/net/act_api.h
+++ b/include/net/act_api.h
@@ -91,7 +91,7 @@ struct tc_action_ops {
 	struct module		*owner;
 	int     (*act)(struct sk_buff *, const struct tc_action *, struct tcf_result *);
 	int     (*dump)(struct sk_buff *, struct tc_action *, int, int);
-	int     (*cleanup)(struct tc_action *, int bind);
+	void	(*cleanup)(struct tc_action *, int bind);
 	int     (*lookup)(struct tc_action *, u32);
 	int     (*init)(struct net *net, struct nlattr *nla,
 			struct nlattr *est, struct tc_action *act, int ovr,
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 91b489e..453a47e 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -56,6 +56,8 @@ int tcf_hash_release(struct tc_action *a, int bind)
 
 		p->tcfc_refcnt--;
 		if (p->tcfc_bindcnt <= 0 && p->tcfc_refcnt <= 0) {
+			if (a->ops->cleanup)
+				a->ops->cleanup(a, bind);
 			tcf_hash_destroy(a);
 			ret = 1;
 		}
@@ -276,8 +278,8 @@ int tcf_register_action(struct tc_action_ops *act)
 {
 	struct tc_action_ops *a;
 
-	/* Must supply act, dump, cleanup and init */
-	if (!act->act || !act->dump || !act->cleanup || !act->init)
+	/* Must supply act, dump and init */
+	if (!act->act || !act->dump || !act->init)
 		return -EINVAL;
 
 	/* Supply defaults */
@@ -389,7 +391,7 @@ void tcf_action_destroy(struct list_head *actions, int bind)
 	struct tc_action *a, *tmp;
 
 	list_for_each_entry_safe(a, tmp, actions, list) {
-		if (a->ops->cleanup(a, bind) == ACT_P_DELETED)
+		if (tcf_hash_release(a, bind) == ACT_P_DELETED)
 			module_put(a->ops->owner);
 		list_del(&a->list);
 		kfree(a);
diff --git a/net/sched/act_csum.c b/net/sched/act_csum.c
index 8e75a08..15e6515 100644
--- a/net/sched/act_csum.c
+++ b/net/sched/act_csum.c
@@ -567,7 +567,6 @@ static struct tc_action_ops act_csum_ops = {
 	.owner		= THIS_MODULE,
 	.act		= tcf_csum,
 	.dump		= tcf_csum_dump,
-	.cleanup	= tcf_hash_release,
 	.init		= tcf_csum_init,
 };
 
diff --git a/net/sched/act_gact.c b/net/sched/act_gact.c
index 47148fa..e09b6e5 100644
--- a/net/sched/act_gact.c
+++ b/net/sched/act_gact.c
@@ -186,7 +186,6 @@ static struct tc_action_ops act_gact_ops = {
 	.owner		=	THIS_MODULE,
 	.act		=	tcf_gact,
 	.dump		=	tcf_gact_dump,
-	.cleanup	=	tcf_hash_release,
 	.init		=	tcf_gact_init,
 };
 
diff --git a/net/sched/act_ipt.c b/net/sched/act_ipt.c
index f0545fb..6096310 100644
--- a/net/sched/act_ipt.c
+++ b/net/sched/act_ipt.c
@@ -69,23 +69,12 @@ static void ipt_destroy_target(struct xt_entry_target *t)
 	module_put(par.target->me);
 }
 
-static int tcf_ipt_release(struct tc_action *a, int bind)
+static void tcf_ipt_release(struct tc_action *a, int bind)
 {
 	struct tcf_ipt *ipt = to_ipt(a);
-	int ret = 0;
-	if (ipt) {
-		if (bind)
-			ipt->tcf_bindcnt--;
-		ipt->tcf_refcnt--;
-		if (ipt->tcf_bindcnt <= 0 && ipt->tcf_refcnt <= 0) {
-			ipt_destroy_target(ipt->tcfi_t);
-			kfree(ipt->tcfi_tname);
-			kfree(ipt->tcfi_t);
-			tcf_hash_destroy(a);
-			ret = ACT_P_DELETED;
-		}
-	}
-	return ret;
+	ipt_destroy_target(ipt->tcfi_t);
+	kfree(ipt->tcfi_tname);
+	kfree(ipt->tcfi_t);
 }
 
 static const struct nla_policy ipt_policy[TCA_IPT_MAX + 1] = {
@@ -133,7 +122,7 @@ static int tcf_ipt_init(struct net *net, struct nlattr *nla, struct nlattr *est,
 	} else {
 		if (bind)/* dont override defaults */
 			return 0;
-		tcf_ipt_release(a, bind);
+		tcf_hash_release(a, bind);
 
 		if (!ovr)
 			return -EEXIST;
diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
index f94a58f..93e5209 100644
--- a/net/sched/act_mirred.c
+++ b/net/sched/act_mirred.c
@@ -33,22 +33,12 @@
 static LIST_HEAD(mirred_list);
 static struct tcf_hashinfo mirred_hash_info;
 
-static int tcf_mirred_release(struct tc_action *a, int bind)
+static void tcf_mirred_release(struct tc_action *a, int bind)
 {
 	struct tcf_mirred *m = to_mirred(a);
-	if (m) {
-		if (bind)
-			m->tcf_bindcnt--;
-		m->tcf_refcnt--;
-		if (!m->tcf_bindcnt && m->tcf_refcnt <= 0) {
-			list_del(&m->tcfm_list);
-			if (m->tcfm_dev)
-				dev_put(m->tcfm_dev);
-			tcf_hash_destroy(a);
-			return 1;
-		}
-	}
-	return 0;
+	list_del(&m->tcfm_list);
+	if (m->tcfm_dev)
+		dev_put(m->tcfm_dev);
 }
 
 static const struct nla_policy mirred_policy[TCA_MIRRED_MAX + 1] = {
@@ -110,7 +100,7 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
 		ret = ACT_P_CREATED;
 	} else {
 		if (!ovr) {
-			tcf_mirred_release(a, bind);
+			tcf_hash_release(a, bind);
 			return -EEXIST;
 		}
 	}
diff --git a/net/sched/act_nat.c b/net/sched/act_nat.c
index c8d1843..e1ae2e5 100644
--- a/net/sched/act_nat.c
+++ b/net/sched/act_nat.c
@@ -290,7 +290,6 @@ static struct tc_action_ops act_nat_ops = {
 	.owner		=	THIS_MODULE,
 	.act		=	tcf_nat,
 	.dump		=	tcf_nat_dump,
-	.cleanup	=	tcf_hash_release,
 	.init		=	tcf_nat_init,
 };
 
diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
index 75f08e4..cd08b3b 100644
--- a/net/sched/act_pedit.c
+++ b/net/sched/act_pedit.c
@@ -99,18 +99,11 @@ static int tcf_pedit_init(struct net *net, struct nlattr *nla,
 	return ret;
 }
 
-static int tcf_pedit_cleanup(struct tc_action *a, int bind)
+static void tcf_pedit_cleanup(struct tc_action *a, int bind)
 {
 	struct tcf_pedit *p = a->priv;
-
-	if (p) {
-		struct tc_pedit_key *keys = p->tcfp_keys;
-		if (tcf_hash_release(a, bind)) {
-			kfree(keys);
-			return 1;
-		}
-	}
-	return 0;
+	struct tc_pedit_key *keys = p->tcfp_keys;
+	kfree(keys);
 }
 
 static int tcf_pedit(struct sk_buff *skb, const struct tc_action *a,
diff --git a/net/sched/act_police.c b/net/sched/act_police.c
index e51f7d9..143af47 100644
--- a/net/sched/act_police.c
+++ b/net/sched/act_police.c
@@ -253,14 +253,6 @@ failure:
 	return err;
 }
 
-static int tcf_act_police_cleanup(struct tc_action *a, int bind)
-{
-	struct tcf_police *p = a->priv;
-	if (p)
-		return tcf_hash_release(&p->common, bind, &police_hash_info);
-	return 0;
-}
-
 static int tcf_act_police(struct sk_buff *skb, const struct tc_action *a,
 			  struct tcf_result *res)
 {
@@ -363,7 +355,6 @@ static struct tc_action_ops act_police_ops = {
 	.owner		=	THIS_MODULE,
 	.act		=	tcf_act_police,
 	.dump		=	tcf_act_police_dump,
-	.cleanup	=	tcf_act_police_cleanup,
 	.init		=	tcf_act_police_locate,
 	.walk		=	tcf_act_police_walker
 };
diff --git a/net/sched/act_simple.c b/net/sched/act_simple.c
index e52ee06..fbdc6eb 100644
--- a/net/sched/act_simple.c
+++ b/net/sched/act_simple.c
@@ -47,21 +47,10 @@ static int tcf_simp(struct sk_buff *skb, const struct tc_action *a,
 	return d->tcf_action;
 }
 
-static int tcf_simp_release(struct tc_action *a, int bind)
+static void tcf_simp_release(struct tc_action *a, int bind)
 {
 	struct tcf_defact *d = to_defact(a);
-	int ret = 0;
-	if (d) {
-		if (bind)
-			d->tcf_bindcnt--;
-		d->tcf_refcnt--;
-		if (d->tcf_bindcnt <= 0 && d->tcf_refcnt <= 0) {
-			kfree(d->tcfd_defdata);
-			tcf_hash_destroy(a);
-			ret = 1;
-		}
-	}
-	return ret;
+	kfree(d->tcfd_defdata);
 }
 
 static int alloc_defdata(struct tcf_defact *d, char *defdata)
@@ -132,7 +121,7 @@ static int tcf_simp_init(struct net *net, struct nlattr *nla,
 
 		if (bind)
 			return 0;
-		tcf_simp_release(a, bind);
+		tcf_hash_release(a, bind);
 		if (!ovr)
 			return -EEXIST;
 
diff --git a/net/sched/act_skbedit.c b/net/sched/act_skbedit.c
index 68ab57b..2965484 100644
--- a/net/sched/act_skbedit.c
+++ b/net/sched/act_skbedit.c
@@ -181,7 +181,6 @@ static struct tc_action_ops act_skbedit_ops = {
 	.owner		=	THIS_MODULE,
 	.act		=	tcf_skbedit,
 	.dump		=	tcf_skbedit_dump,
-	.cleanup	=	tcf_hash_release,
 	.init		=	tcf_skbedit_init,
 };
 
-- 
1.8.3.1

[Patch net-next 5/6] net_sched: act: move tcf_hashinfo_init() into tcf_register_action()

[Cong Wang]

Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: David S. Miller <davem@davemloft.net>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 include/net/act_api.h   |  6 +++---
 net/sched/act_api.c     | 22 ++++++++++++++--------
 net/sched/act_csum.c    |  8 +-------
 net/sched/act_gact.c    |  8 +-------
 net/sched/act_ipt.c     | 14 +++-----------
 net/sched/act_mirred.c  | 10 +---------
 net/sched/act_nat.c     |  9 +--------
 net/sched/act_pedit.c   |  9 +--------
 net/sched/act_police.c  | 17 ++++-------------
 net/sched/act_simple.c  | 14 ++------------
 net/sched/act_skbedit.c |  8 +-------
 11 files changed, 32 insertions(+), 93 deletions(-)

diff --git a/include/net/act_api.h b/include/net/act_api.h
index 9263ace..3198b0a 100644
--- a/include/net/act_api.h
+++ b/include/net/act_api.h
@@ -75,7 +75,7 @@ static inline void tcf_hashinfo_destroy(struct tcf_hashinfo *hf)
 
 struct tc_action {
 	void			*priv;
-	const struct tc_action_ops	*ops;
+	struct tc_action_ops	*ops;
 	__u32			type; /* for backward compat(TCA_OLD_COMPAT) */
 	__u32			order;
 	struct list_head	list;
@@ -84,7 +84,7 @@ struct tc_action {
 #define TCA_CAP_NONE 0
 struct tc_action_ops {
 	struct list_head head;
-	struct tcf_hashinfo *hinfo;
+	struct tcf_hashinfo hinfo;
 	char    kind[IFNAMSIZ];
 	__u32   type; /* TBD to match kind */
 	__u32 	capab;  /* capabilities includes 4 bit version */
@@ -109,7 +109,7 @@ int tcf_hash_create(u32 index, struct nlattr *est, struct tc_action *a,
 void tcf_hash_cleanup(struct tc_action *a, struct nlattr *est);
 void tcf_hash_insert(struct tc_action *a);
 
-int tcf_register_action(struct tc_action_ops *a);
+int tcf_register_action(struct tc_action_ops *a, unsigned int mask);
 int tcf_unregister_action(struct tc_action_ops *a);
 void tcf_action_destroy(struct list_head *actions, int bind);
 int tcf_action_exec(struct sk_buff *skb, const struct list_head *actions,
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 453a47e..5ac0392 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -30,7 +30,7 @@
 void tcf_hash_destroy(struct tc_action *a)
 {
 	struct tcf_common *p = a->priv;
-	struct tcf_hashinfo *hinfo = a->ops->hinfo;
+	struct tcf_hashinfo *hinfo = &a->ops->hinfo;
 
 	spin_lock_bh(&hinfo->lock);
 	hlist_del(&p->tcfc_head);
@@ -69,7 +69,7 @@ EXPORT_SYMBOL(tcf_hash_release);
 static int tcf_dump_walker(struct sk_buff *skb, struct netlink_callback *cb,
 			   struct tc_action *a)
 {
-	struct tcf_hashinfo *hinfo = a->ops->hinfo;
+	struct tcf_hashinfo *hinfo = &a->ops->hinfo;
 	struct hlist_head *head;
 	struct tcf_common *p;
 	int err = 0, index = -1, i = 0, s_i = 0, n_i = 0;
@@ -117,7 +117,7 @@ nla_put_failure:
 
 static int tcf_del_walker(struct sk_buff *skb, struct tc_action *a)
 {
-	struct tcf_hashinfo *hinfo = a->ops->hinfo;
+	struct tcf_hashinfo *hinfo = &a->ops->hinfo;
 	struct hlist_head *head;
 	struct hlist_node *n;
 	struct tcf_common *p;
@@ -192,7 +192,7 @@ EXPORT_SYMBOL(tcf_hash_new_index);
 
 int tcf_hash_search(struct tc_action *a, u32 index)
 {
-	struct tcf_hashinfo *hinfo = a->ops->hinfo;
+	struct tcf_hashinfo *hinfo = &a->ops->hinfo;
 	struct tcf_common *p = tcf_hash_lookup(index, hinfo);
 
 	if (p) {
@@ -205,7 +205,7 @@ EXPORT_SYMBOL(tcf_hash_search);
 
 int tcf_hash_check(u32 index, struct tc_action *a, int bind)
 {
-	struct tcf_hashinfo *hinfo = a->ops->hinfo;
+	struct tcf_hashinfo *hinfo = &a->ops->hinfo;
 	struct tcf_common *p = NULL;
 	if (index && (p = tcf_hash_lookup(index, hinfo)) != NULL) {
 		if (bind)
@@ -231,7 +231,7 @@ EXPORT_SYMBOL(tcf_hash_cleanup);
 int tcf_hash_create(u32 index, struct nlattr *est, struct tc_action *a,
 		    int size, int bind)
 {
-	struct tcf_hashinfo *hinfo = a->ops->hinfo;
+	struct tcf_hashinfo *hinfo = &a->ops->hinfo;
 	struct tcf_common *p = kzalloc(size, GFP_KERNEL);
 
 	if (unlikely(!p))
@@ -262,7 +262,7 @@ EXPORT_SYMBOL(tcf_hash_create);
 void tcf_hash_insert(struct tc_action *a)
 {
 	struct tcf_common *p = a->priv;
-	struct tcf_hashinfo *hinfo = a->ops->hinfo;
+	struct tcf_hashinfo *hinfo = &a->ops->hinfo;
 	unsigned int h = tcf_hash(p->tcfc_index, hinfo->hmask);
 
 	spin_lock_bh(&hinfo->lock);
@@ -274,9 +274,10 @@ EXPORT_SYMBOL(tcf_hash_insert);
 static LIST_HEAD(act_base);
 static DEFINE_RWLOCK(act_mod_lock);
 
-int tcf_register_action(struct tc_action_ops *act)
+int tcf_register_action(struct tc_action_ops *act, unsigned int mask)
 {
 	struct tc_action_ops *a;
+	int err;
 
 	/* Must supply act, dump and init */
 	if (!act->act || !act->dump || !act->init)
@@ -288,6 +289,10 @@ int tcf_register_action(struct tc_action_ops *act)
 	if (!act->walk)
 		act->walk = tcf_generic_walker;
 
+	err = tcf_hashinfo_init(&act->hinfo, mask);
+	if (err)
+		return err;
+
 	write_lock(&act_mod_lock);
 	list_for_each_entry(a, &act_base, head) {
 		if (act->type == a->type || (strcmp(act->kind, a->kind) == 0)) {
@@ -310,6 +315,7 @@ int tcf_unregister_action(struct tc_action_ops *act)
 	list_for_each_entry(a, &act_base, head) {
 		if (a == act) {
 			list_del(&act->head);
+			tcf_hashinfo_destroy(&act->hinfo);
 			err = 0;
 			break;
 		}
diff --git a/net/sched/act_csum.c b/net/sched/act_csum.c
index 15e6515..93394d5 100644
--- a/net/sched/act_csum.c
+++ b/net/sched/act_csum.c
@@ -37,7 +37,6 @@
 #include <net/tc_act/tc_csum.h>
 
 #define CSUM_TAB_MASK 15
-static struct tcf_hashinfo csum_hash_info;
 
 static const struct nla_policy csum_policy[TCA_CSUM_MAX + 1] = {
 	[TCA_CSUM_PARMS] = { .len = sizeof(struct tc_csum), },
@@ -561,7 +560,6 @@ nla_put_failure:
 
 static struct tc_action_ops act_csum_ops = {
 	.kind		= "csum",
-	.hinfo		= &csum_hash_info,
 	.type		= TCA_ACT_CSUM,
 	.capab		= TCA_CAP_NONE,
 	.owner		= THIS_MODULE,
@@ -575,11 +573,7 @@ MODULE_LICENSE("GPL");
 
 static int __init csum_init_module(void)
 {
-	int err = tcf_hashinfo_init(&csum_hash_info, CSUM_TAB_MASK);
-	if (err)
-		return err;
-
-	return tcf_register_action(&act_csum_ops);
+	return tcf_register_action(&act_csum_ops, CSUM_TAB_MASK);
 }
 
 static void __exit csum_cleanup_module(void)
diff --git a/net/sched/act_gact.c b/net/sched/act_gact.c
index e09b6e5..0002a12 100644
--- a/net/sched/act_gact.c
+++ b/net/sched/act_gact.c
@@ -24,7 +24,6 @@
 #include <net/tc_act/tc_gact.h>
 
 #define GACT_TAB_MASK	15
-static struct tcf_hashinfo gact_hash_info;
 
 #ifdef CONFIG_GACT_PROB
 static int gact_net_rand(struct tcf_gact *gact)
@@ -180,7 +179,6 @@ nla_put_failure:
 
 static struct tc_action_ops act_gact_ops = {
 	.kind		=	"gact",
-	.hinfo		=	&gact_hash_info,
 	.type		=	TCA_ACT_GACT,
 	.capab		=	TCA_CAP_NONE,
 	.owner		=	THIS_MODULE,
@@ -195,21 +193,17 @@ MODULE_LICENSE("GPL");
 
 static int __init gact_init_module(void)
 {
-	int err = tcf_hashinfo_init(&gact_hash_info, GACT_TAB_MASK);
-	if (err)
-		return err;
 #ifdef CONFIG_GACT_PROB
 	pr_info("GACT probability on\n");
 #else
 	pr_info("GACT probability NOT on\n");
 #endif
-	return tcf_register_action(&act_gact_ops);
+	return tcf_register_action(&act_gact_ops, GACT_TAB_MASK);
 }
 
 static void __exit gact_cleanup_module(void)
 {
 	tcf_unregister_action(&act_gact_ops);
-	tcf_hashinfo_destroy(&gact_hash_info);
 }
 
 module_init(gact_init_module);
diff --git a/net/sched/act_ipt.c b/net/sched/act_ipt.c
index 6096310..a00a2cb 100644
--- a/net/sched/act_ipt.c
+++ b/net/sched/act_ipt.c
@@ -29,7 +29,6 @@
 
 
 #define IPT_TAB_MASK     15
-static struct tcf_hashinfo ipt_hash_info;
 
 static int ipt_init_target(struct xt_entry_target *t, char *table, unsigned int hook)
 {
@@ -262,7 +261,6 @@ nla_put_failure:
 
 static struct tc_action_ops act_ipt_ops = {
 	.kind		=	"ipt",
-	.hinfo		=	&ipt_hash_info,
 	.type		=	TCA_ACT_IPT,
 	.capab		=	TCA_CAP_NONE,
 	.owner		=	THIS_MODULE,
@@ -274,7 +272,6 @@ static struct tc_action_ops act_ipt_ops = {
 
 static struct tc_action_ops act_xt_ops = {
 	.kind		=	"xt",
-	.hinfo		=	&ipt_hash_info,
 	.type		=	TCA_ACT_XT,
 	.capab		=	TCA_CAP_NONE,
 	.owner		=	THIS_MODULE,
@@ -291,20 +288,16 @@ MODULE_ALIAS("act_xt");
 
 static int __init ipt_init_module(void)
 {
-	int ret1, ret2, err;
-	err = tcf_hashinfo_init(&ipt_hash_info, IPT_TAB_MASK);
-	if (err)
-		return err;
+	int ret1, ret2;
 
-	ret1 = tcf_register_action(&act_xt_ops);
+	ret1 = tcf_register_action(&act_xt_ops, IPT_TAB_MASK);
 	if (ret1 < 0)
 		printk("Failed to load xt action\n");
-	ret2 = tcf_register_action(&act_ipt_ops);
+	ret2 = tcf_register_action(&act_ipt_ops, IPT_TAB_MASK);
 	if (ret2 < 0)
 		printk("Failed to load ipt action\n");
 
 	if (ret1 < 0 && ret2 < 0) {
-		tcf_hashinfo_destroy(&ipt_hash_info);
 		return ret1;
 	} else
 		return 0;
@@ -314,7 +307,6 @@ static void __exit ipt_cleanup_module(void)
 {
 	tcf_unregister_action(&act_xt_ops);
 	tcf_unregister_action(&act_ipt_ops);
-	tcf_hashinfo_destroy(&ipt_hash_info);
 }
 
 module_init(ipt_init_module);
diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
index 93e5209..e7c4e0a 100644
--- a/net/sched/act_mirred.c
+++ b/net/sched/act_mirred.c
@@ -31,7 +31,6 @@
 
 #define MIRRED_TAB_MASK     7
 static LIST_HEAD(mirred_list);
-static struct tcf_hashinfo mirred_hash_info;
 
 static void tcf_mirred_release(struct tc_action *a, int bind)
 {
@@ -234,7 +233,6 @@ static struct notifier_block mirred_device_notifier = {
 
 static struct tc_action_ops act_mirred_ops = {
 	.kind		=	"mirred",
-	.hinfo		=	&mirred_hash_info,
 	.type		=	TCA_ACT_MIRRED,
 	.capab		=	TCA_CAP_NONE,
 	.owner		=	THIS_MODULE,
@@ -254,19 +252,13 @@ static int __init mirred_init_module(void)
 	if (err)
 		return err;
 
-	err = tcf_hashinfo_init(&mirred_hash_info, MIRRED_TAB_MASK);
-	if (err) {
-		unregister_netdevice_notifier(&mirred_device_notifier);
-		return err;
-	}
 	pr_info("Mirror/redirect action on\n");
-	return tcf_register_action(&act_mirred_ops);
+	return tcf_register_action(&act_mirred_ops, MIRRED_TAB_MASK);
 }
 
 static void __exit mirred_cleanup_module(void)
 {
 	tcf_unregister_action(&act_mirred_ops);
-	tcf_hashinfo_destroy(&mirred_hash_info);
 	unregister_netdevice_notifier(&mirred_device_notifier);
 }
 
diff --git a/net/sched/act_nat.c b/net/sched/act_nat.c
index e1ae2e5..6523980 100644
--- a/net/sched/act_nat.c
+++ b/net/sched/act_nat.c
@@ -31,8 +31,6 @@
 
 #define NAT_TAB_MASK	15
 
-static struct tcf_hashinfo nat_hash_info;
-
 static const struct nla_policy nat_policy[TCA_NAT_MAX + 1] = {
 	[TCA_NAT_PARMS]	= { .len = sizeof(struct tc_nat) },
 };
@@ -284,7 +282,6 @@ nla_put_failure:
 
 static struct tc_action_ops act_nat_ops = {
 	.kind		=	"nat",
-	.hinfo		=	&nat_hash_info,
 	.type		=	TCA_ACT_NAT,
 	.capab		=	TCA_CAP_NONE,
 	.owner		=	THIS_MODULE,
@@ -298,16 +295,12 @@ MODULE_LICENSE("GPL");
 
 static int __init nat_init_module(void)
 {
-	int err = tcf_hashinfo_init(&nat_hash_info, NAT_TAB_MASK);
-	if (err)
-		return err;
-	return tcf_register_action(&act_nat_ops);
+	return tcf_register_action(&act_nat_ops, NAT_TAB_MASK);
 }
 
 static void __exit nat_cleanup_module(void)
 {
 	tcf_unregister_action(&act_nat_ops);
-	tcf_hashinfo_destroy(&nat_hash_info);
 }
 
 module_init(nat_init_module);
diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
index cd08b3b..22cf016 100644
--- a/net/sched/act_pedit.c
+++ b/net/sched/act_pedit.c
@@ -25,8 +25,6 @@
 
 #define PEDIT_TAB_MASK	15
 
-static struct tcf_hashinfo pedit_hash_info;
-
 static const struct nla_policy pedit_policy[TCA_PEDIT_MAX + 1] = {
 	[TCA_PEDIT_PARMS]	= { .len = sizeof(struct tc_pedit) },
 };
@@ -218,7 +216,6 @@ nla_put_failure:
 
 static struct tc_action_ops act_pedit_ops = {
 	.kind		=	"pedit",
-	.hinfo		=	&pedit_hash_info,
 	.type		=	TCA_ACT_PEDIT,
 	.capab		=	TCA_CAP_NONE,
 	.owner		=	THIS_MODULE,
@@ -234,15 +231,11 @@ MODULE_LICENSE("GPL");
 
 static int __init pedit_init_module(void)
 {
-	int err = tcf_hashinfo_init(&pedit_hash_info, PEDIT_TAB_MASK);
-	if (err)
-		return err;
-	return tcf_register_action(&act_pedit_ops);
+	return tcf_register_action(&act_pedit_ops, PEDIT_TAB_MASK);
 }
 
 static void __exit pedit_cleanup_module(void)
 {
-	tcf_hashinfo_destroy(&pedit_hash_info);
 	tcf_unregister_action(&act_pedit_ops);
 }
 
diff --git a/net/sched/act_police.c b/net/sched/act_police.c
index 143af47..da724b9 100644
--- a/net/sched/act_police.c
+++ b/net/sched/act_police.c
@@ -41,7 +41,6 @@ struct tcf_police {
 	container_of(pc, struct tcf_police, common)
 
 #define POL_TAB_MASK     15
-static struct tcf_hashinfo police_hash_info;
 
 /* old policer structure from before tc actions */
 struct tc_police_compat {
@@ -59,7 +58,7 @@ struct tc_police_compat {
 static int tcf_act_police_walker(struct sk_buff *skb, struct netlink_callback *cb,
 			      int type, struct tc_action *a)
 {
-	struct tcf_hashinfo *hinfo = a->ops->hinfo;
+	struct tcf_hashinfo *hinfo = &a->ops->hinfo;
 	struct hlist_head *head;
 	struct tcf_common *p;
 	int err = 0, index = -1, i = 0, s_i = 0, n_i = 0;
@@ -122,7 +121,7 @@ static int tcf_act_police_locate(struct net *net, struct nlattr *nla,
 	struct tc_police *parm;
 	struct tcf_police *police;
 	struct qdisc_rate_table *R_tab = NULL, *P_tab = NULL;
-	struct tcf_hashinfo *hinfo = a->ops->hinfo;
+	struct tcf_hashinfo *hinfo = &a->ops->hinfo;
 	int size;
 
 	if (nla == NULL)
@@ -234,7 +233,7 @@ override:
 
 	police->tcfp_t_c = ktime_to_ns(ktime_get());
 	police->tcf_index = parm->index ? parm->index :
-		tcf_hash_new_index(a->ops->hinfo);
+		tcf_hash_new_index(hinfo);
 	h = tcf_hash(police->tcf_index, POL_TAB_MASK);
 	spin_lock_bh(&hinfo->lock);
 	hlist_add_head(&police->tcf_head, &hinfo->htab[h]);
@@ -349,7 +348,6 @@ MODULE_LICENSE("GPL");
 
 static struct tc_action_ops act_police_ops = {
 	.kind		=	"police",
-	.hinfo		=	&police_hash_info,
 	.type		=	TCA_ID_POLICE,
 	.capab		=	TCA_CAP_NONE,
 	.owner		=	THIS_MODULE,
@@ -362,19 +360,12 @@ static struct tc_action_ops act_police_ops = {
 static int __init
 police_init_module(void)
 {
-	int err = tcf_hashinfo_init(&police_hash_info, POL_TAB_MASK);
-	if (err)
-		return err;
-	err = tcf_register_action(&act_police_ops);
-	if (err)
-		tcf_hashinfo_destroy(&police_hash_info);
-	return err;
+	return tcf_register_action(&act_police_ops, POL_TAB_MASK);
 }
 
 static void __exit
 police_cleanup_module(void)
 {
-	tcf_hashinfo_destroy(&police_hash_info);
 	tcf_unregister_action(&act_police_ops);
 }
 
diff --git a/net/sched/act_simple.c b/net/sched/act_simple.c
index fbdc6eb..29cffff 100644
--- a/net/sched/act_simple.c
+++ b/net/sched/act_simple.c
@@ -25,7 +25,6 @@
 #include <net/tc_act/tc_defact.h>
 
 #define SIMP_TAB_MASK     7
-static struct tcf_hashinfo simp_hash_info;
 
 #define SIMP_MAX_DATA	32
 static int tcf_simp(struct sk_buff *skb, const struct tc_action *a,
@@ -163,7 +162,6 @@ nla_put_failure:
 
 static struct tc_action_ops act_simp_ops = {
 	.kind		=	"simple",
-	.hinfo		=	&simp_hash_info,
 	.type		=	TCA_ACT_SIMP,
 	.capab		=	TCA_CAP_NONE,
 	.owner		=	THIS_MODULE,
@@ -179,23 +177,15 @@ MODULE_LICENSE("GPL");
 
 static int __init simp_init_module(void)
 {
-	int err, ret;
-	err = tcf_hashinfo_init(&simp_hash_info, SIMP_TAB_MASK);
-	if (err)
-		return err;
-
-	ret = tcf_register_action(&act_simp_ops);
+	int ret;
+	ret = tcf_register_action(&act_simp_ops, SIMP_TAB_MASK);
 	if (!ret)
 		pr_info("Simple TC action Loaded\n");
-	else
-		tcf_hashinfo_destroy(&simp_hash_info);
-
 	return ret;
 }
 
 static void __exit simp_cleanup_module(void)
 {
-	tcf_hashinfo_destroy(&simp_hash_info);
 	tcf_unregister_action(&act_simp_ops);
 }
 
diff --git a/net/sched/act_skbedit.c b/net/sched/act_skbedit.c
index 2965484..67dfce8 100644
--- a/net/sched/act_skbedit.c
+++ b/net/sched/act_skbedit.c
@@ -28,7 +28,6 @@
 #include <net/tc_act/tc_skbedit.h>
 
 #define SKBEDIT_TAB_MASK     15
-static struct tcf_hashinfo skbedit_hash_info;
 
 static int tcf_skbedit(struct sk_buff *skb, const struct tc_action *a,
 		       struct tcf_result *res)
@@ -175,7 +174,6 @@ nla_put_failure:
 
 static struct tc_action_ops act_skbedit_ops = {
 	.kind		=	"skbedit",
-	.hinfo		=	&skbedit_hash_info,
 	.type		=	TCA_ACT_SKBEDIT,
 	.capab		=	TCA_CAP_NONE,
 	.owner		=	THIS_MODULE,
@@ -190,15 +188,11 @@ MODULE_LICENSE("GPL");
 
 static int __init skbedit_init_module(void)
 {
-	int err = tcf_hashinfo_init(&skbedit_hash_info, SKBEDIT_TAB_MASK);
-	if (err)
-		return err;
-	return tcf_register_action(&act_skbedit_ops);
+	return tcf_register_action(&act_skbedit_ops, SKBEDIT_TAB_MASK);
 }
 
 static void __exit skbedit_cleanup_module(void)
 {
-	tcf_hashinfo_destroy(&skbedit_hash_info);
 	tcf_unregister_action(&act_skbedit_ops);
 }
 
-- 
1.8.3.1

[Patch net-next 6/6] net_sched: act: refuse to remove bound action outside

[Cong Wang]

When an action is bonnd to a filter, there is no point to
remove it outside. Currently we just silently decrease the refcnt,
we should reject this explicitly with EPERM.

Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: David S. Miller <davem@davemloft.net>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 include/net/act_api.h |  2 +-
 net/sched/act_api.c   | 26 ++++++++++++++++++++------
 2 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/include/net/act_api.h b/include/net/act_api.h
index 3198b0a..982d528 100644
--- a/include/net/act_api.h
+++ b/include/net/act_api.h
@@ -111,7 +111,7 @@ void tcf_hash_insert(struct tc_action *a);
 
 int tcf_register_action(struct tc_action_ops *a, unsigned int mask);
 int tcf_unregister_action(struct tc_action_ops *a);
-void tcf_action_destroy(struct list_head *actions, int bind);
+int tcf_action_destroy(struct list_head *actions, int bind);
 int tcf_action_exec(struct sk_buff *skb, const struct list_head *actions,
 		    struct tcf_result *res);
 int tcf_action_init(struct net *net, struct nlattr *nla,
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 5ac0392..fd9042b 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -53,6 +53,8 @@ int tcf_hash_release(struct tc_action *a, int bind)
 	if (p) {
 		if (bind)
 			p->tcfc_bindcnt--;
+		else if (p->tcfc_bindcnt > 0)
+			return -EPERM;
 
 		p->tcfc_refcnt--;
 		if (p->tcfc_bindcnt <= 0 && p->tcfc_refcnt <= 0) {
@@ -123,6 +125,7 @@ static int tcf_del_walker(struct sk_buff *skb, struct tc_action *a)
 	struct tcf_common *p;
 	struct nlattr *nest;
 	int i = 0, n_i = 0;
+	int ret = -EINVAL;
 
 	nest = nla_nest_start(skb, a->order);
 	if (nest == NULL)
@@ -132,10 +135,12 @@ static int tcf_del_walker(struct sk_buff *skb, struct tc_action *a)
 	for (i = 0; i < (hinfo->hmask + 1); i++) {
 		head = &hinfo->htab[tcf_hash(i, hinfo->hmask)];
 		hlist_for_each_entry_safe(p, n, head, tcfc_head) {
-			if (ACT_P_DELETED == tcf_hash_release(a, 0)) {
+			ret = tcf_hash_release(a, 0);
+			if (ret == ACT_P_DELETED) {
 				module_put(a->ops->owner);
 				n_i++;
-			}
+			} else if (ret < 0)
+				goto nla_put_failure;
 		}
 	}
 	if (nla_put_u32(skb, TCA_FCNT, n_i))
@@ -145,7 +150,7 @@ static int tcf_del_walker(struct sk_buff *skb, struct tc_action *a)
 	return n_i;
 nla_put_failure:
 	nla_nest_cancel(skb, nest);
-	return -EINVAL;
+	return ret;
 }
 
 static int tcf_generic_walker(struct sk_buff *skb, struct netlink_callback *cb,
@@ -392,16 +397,21 @@ exec_done:
 }
 EXPORT_SYMBOL(tcf_action_exec);
 
-void tcf_action_destroy(struct list_head *actions, int bind)
+int tcf_action_destroy(struct list_head *actions, int bind)
 {
 	struct tc_action *a, *tmp;
+	int ret = 0;
 
 	list_for_each_entry_safe(a, tmp, actions, list) {
-		if (tcf_hash_release(a, bind) == ACT_P_DELETED)
+		ret = tcf_hash_release(a, bind);
+		if (ret == ACT_P_DELETED)
 			module_put(a->ops->owner);
+		else if (ret < 0)
+			return ret;
 		list_del(&a->list);
 		kfree(a);
 	}
+	return ret;
 }
 
 int
@@ -829,7 +839,11 @@ tcf_del_notify(struct net *net, struct nlmsghdr *n, struct list_head *actions,
 	}
 
 	/* now do the delete */
-	tcf_action_destroy(actions, 0);
+	ret = tcf_action_destroy(actions, 0);
+	if (ret < 0) {
+		kfree_skb(skb);
+		return ret;
+	}
 
 	ret = rtnetlink_send(skb, net, portid, RTNLGRP_TC,
 			     n->nlmsg_flags & NLM_F_ECHO);
-- 
1.8.3.1