[PATCH net-next v4 0/8] net_sched: some cleanup and improvments

[PATCH net-next v4 0/8] net_sched: some cleanup and improvments

[Cong Wang]

Here are some cleanup and improvements for tc filters
and tc actions.

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

v3 -> v4:
* fix tcf_exts_is_predicative()
* rename 'head' to 'actions'
* use spinlock instead of spinlock+RCU
* add two more patches

v2 -> v3:
* fix a typo introduced during rebase

v1 -> v2:
* fix a smatch warning and a checkpatch warning
* add a cover letter
* add a missing synchronize_rcu()

Cong Wang (8):
  net_sched: remove get_stats from tc_action_ops
  net_sched: act: use standard struct list_head
  net_sched: mirred: remove action when the target device is gone
  net_sched: cls: refactor out struct tcf_ext_map
  net_sched: init struct tcf_hashinfo at register time
  net_sched: convert tcf_hashinfo to hlist and use spinlock
  net_sched: convert tc_action_ops to use struct list_head
  net_sched: convert tcf_proto_ops to use struct list_head

 include/net/act_api.h          |  43 ++++++---
 include/net/pkt_cls.h          |  37 ++++----
 include/net/sch_generic.h      |   2 +-
 include/net/tc_act/tc_mirred.h |   4 +-
 net/sched/act_api.c            | 198 +++++++++++++++++------------------------
 net/sched/act_csum.c           |  13 ++-
 net/sched/act_gact.c           |  13 ++-
 net/sched/act_ipt.c            |  21 +++--
 net/sched/act_mirred.c         |  31 +++----
 net/sched/act_nat.c            |  12 ++-
 net/sched/act_pedit.c          |  12 ++-
 net/sched/act_police.c         |  63 ++++++-------
 net/sched/act_simple.c         |  20 +++--
 net/sched/act_skbedit.c        |  13 ++-
 net/sched/cls_api.c            |  99 ++++++++++-----------
 net/sched/cls_basic.c          |  13 ++-
 net/sched/cls_bpf.c            |  13 ++-
 net/sched/cls_cgroup.c         |  14 ++-
 net/sched/cls_flow.c           |  13 ++-
 net/sched/cls_fw.c             |  13 ++-
 net/sched/cls_route.c          |  13 ++-
 net/sched/cls_rsvp.h           |  13 ++-
 net/sched/cls_tcindex.c        |  17 ++--
 net/sched/cls_u32.c            |  13 ++-
 24 files changed, 320 insertions(+), 383 deletions(-)

-- 
1.8.1.4

[PATCH net-next v4 1/8] net_sched: remove get_stats from tc_action_ops

[Cong Wang]

It is not used.

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 | 1 -
 net/sched/act_api.c   | 4 ----
 2 files changed, 5 deletions(-)

diff --git a/include/net/act_api.h b/include/net/act_api.h
index 9e90fdf..04c6825 100644
--- a/include/net/act_api.h
+++ b/include/net/act_api.h
@@ -72,7 +72,6 @@ struct tc_action_ops {
 	__u32 	capab;  /* capabilities includes 4 bit version */
 	struct module		*owner;
 	int     (*act)(struct sk_buff *, const struct tc_action *, struct tcf_result *);
-	int     (*get_stats)(struct sk_buff *, struct tc_action *);
 	int     (*dump)(struct sk_buff *, struct tc_action *, int, int);
 	int     (*cleanup)(struct tc_action *, int bind);
 	int     (*lookup)(struct tc_action *, u32);
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 4adbce8..51e28f7 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -637,10 +637,6 @@ int tcf_action_copy_stats(struct sk_buff *skb, struct tc_action *a,
 	if (err < 0)
 		goto errout;
 
-	if (a->ops != NULL && a->ops->get_stats != NULL)
-		if (a->ops->get_stats(skb, a) < 0)
-			goto errout;
-
 	if (gnet_stats_copy_basic(&d, &h->tcf_bstats) < 0 ||
 	    gnet_stats_copy_rate_est(&d, &h->tcf_bstats,
 				     &h->tcf_rate_est) < 0 ||
-- 
1.8.1.4

Re: [PATCH net-next v4 1/8] net_sched: remove get_stats from tc_action_ops

[Jamal Hadi Salim]

On 12/15/13 23:15, Cong Wang wrote:
> It is not used.
>
> 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

> ---
>   include/net/act_api.h | 1 -
>   net/sched/act_api.c   | 4 ----
>   2 files changed, 5 deletions(-)
>
> diff --git a/include/net/act_api.h b/include/net/act_api.h
> index 9e90fdf..04c6825 100644
> --- a/include/net/act_api.h
> +++ b/include/net/act_api.h
> @@ -72,7 +72,6 @@ struct tc_action_ops {
>   	__u32 	capab;  /* capabilities includes 4 bit version */
>   	struct module		*owner;
>   	int     (*act)(struct sk_buff *, const struct tc_action *, struct tcf_result *);
> -	int     (*get_stats)(struct sk_buff *, struct tc_action *);
>   	int     (*dump)(struct sk_buff *, struct tc_action *, int, int);
>   	int     (*cleanup)(struct tc_action *, int bind);
>   	int     (*lookup)(struct tc_action *, u32);
> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
> index 4adbce8..51e28f7 100644
> --- a/net/sched/act_api.c
> +++ b/net/sched/act_api.c
> @@ -637,10 +637,6 @@ int tcf_action_copy_stats(struct sk_buff *skb, struct tc_action *a,
>   	if (err < 0)
>   		goto errout;
>
> -	if (a->ops != NULL && a->ops->get_stats != NULL)
> -		if (a->ops->get_stats(skb, a) < 0)
> -			goto errout;
> -
>   	if (gnet_stats_copy_basic(&d, &h->tcf_bstats) < 0 ||
>   	    gnet_stats_copy_rate_est(&d, &h->tcf_bstats,
>   				     &h->tcf_rate_est) < 0 ||
>

[PATCH net-next v4 2/8] net_sched: act: use standard struct list_head

[Cong Wang]

Currently actions are chained by a singly linked list,
therefore it is a bit hard to add and remove a specific
entry. Convert it to struct list_head so that in the
latter patch we can remove an action without finding
its head.

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   |  12 +++---
 include/net/pkt_cls.h   |  16 ++++++--
 net/sched/act_api.c     | 105 +++++++++++++++++++++---------------------------
 net/sched/cls_api.c     |  54 ++++++++++++-------------
 net/sched/cls_basic.c   |   1 +
 net/sched/cls_bpf.c     |   1 +
 net/sched/cls_cgroup.c  |   1 +
 net/sched/cls_flow.c    |   1 +
 net/sched/cls_fw.c      |   1 +
 net/sched/cls_route.c   |   1 +
 net/sched/cls_rsvp.h    |   1 +
 net/sched/cls_tcindex.c |   5 ++-
 net/sched/cls_u32.c     |   1 +
 13 files changed, 101 insertions(+), 99 deletions(-)

diff --git a/include/net/act_api.h b/include/net/act_api.h
index 04c6825..a726426 100644
--- a/include/net/act_api.h
+++ b/include/net/act_api.h
@@ -60,7 +60,7 @@ struct tc_action {
 	const struct tc_action_ops	*ops;
 	__u32			type; /* for backward compat(TCA_OLD_COMPAT) */
 	__u32			order;
-	struct tc_action	*next;
+	struct list_head	list;
 };
 
 #define TCA_CAP_NONE 0
@@ -99,16 +99,16 @@ void tcf_hash_insert(struct tcf_common *p, struct tcf_hashinfo *hinfo);
 
 int tcf_register_action(struct tc_action_ops *a);
 int tcf_unregister_action(struct tc_action_ops *a);
-void tcf_action_destroy(struct tc_action *a, int bind);
-int tcf_action_exec(struct sk_buff *skb, const struct tc_action *a,
+void 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);
-struct tc_action *tcf_action_init(struct net *net, struct nlattr *nla,
+int tcf_action_init(struct net *net, struct nlattr *nla,
 				  struct nlattr *est, char *n, int ovr,
-				  int bind);
+				  int bind, struct list_head *);
 struct tc_action *tcf_action_init_1(struct net *net, struct nlattr *nla,
 				    struct nlattr *est, char *n, int ovr,
 				    int bind);
-int tcf_action_dump(struct sk_buff *skb, struct tc_action *a, int, int);
+int tcf_action_dump(struct sk_buff *skb, struct list_head *, int, int);
 int tcf_action_dump_old(struct sk_buff *skb, struct tc_action *a, int, int);
 int tcf_action_dump_1(struct sk_buff *skb, struct tc_action *a, int, int);
 int tcf_action_copy_stats(struct sk_buff *, struct tc_action *, int);
diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
index 2ebef77..34fe693d 100644
--- a/include/net/pkt_cls.h
+++ b/include/net/pkt_cls.h
@@ -62,7 +62,8 @@ tcf_unbind_filter(struct tcf_proto *tp, struct tcf_result *r)
 
 struct tcf_exts {
 #ifdef CONFIG_NET_CLS_ACT
-	struct tc_action *action;
+	__u32	type; /* for backward compat(TCA_OLD_COMPAT) */
+	struct list_head actions;
 #endif
 };
 
@@ -74,6 +75,13 @@ struct tcf_ext_map {
 	int police;
 };
 
+static inline void tcf_exts_init(struct tcf_exts *exts)
+{
+#ifdef CONFIG_NET_CLS_ACT
+	INIT_LIST_HEAD(&exts->actions);
+#endif
+}
+
 /**
  * tcf_exts_is_predicative - check if a predicative extension is present
  * @exts: tc filter extensions handle
@@ -85,7 +93,7 @@ static inline int
 tcf_exts_is_predicative(struct tcf_exts *exts)
 {
 #ifdef CONFIG_NET_CLS_ACT
-	return !!exts->action;
+	return !list_empty(&exts->actions);
 #else
 	return 0;
 #endif
@@ -120,8 +128,8 @@ tcf_exts_exec(struct sk_buff *skb, struct tcf_exts *exts,
 	       struct tcf_result *res)
 {
 #ifdef CONFIG_NET_CLS_ACT
-	if (exts->action)
-		return tcf_action_exec(skb, exts->action, res);
+	if (!list_empty(&exts->actions))
+		return tcf_action_exec(skb, &exts->actions, res);
 #endif
 	return 0;
 }
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 51e28f7..7d84183 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -379,7 +379,7 @@ static struct tc_action_ops *tc_lookup_action_id(u32 type)
 }
 #endif
 
-int tcf_action_exec(struct sk_buff *skb, const struct tc_action *act,
+int tcf_action_exec(struct sk_buff *skb, const struct list_head *actions,
 		    struct tcf_result *res)
 {
 	const struct tc_action *a;
@@ -390,7 +390,7 @@ int tcf_action_exec(struct sk_buff *skb, const struct tc_action *act,
 		ret = TC_ACT_OK;
 		goto exec_done;
 	}
-	while ((a = act) != NULL) {
+	list_for_each_entry(a, actions, list) {
 repeat:
 		if (a->ops) {
 			ret = a->ops->act(skb, a, res);
@@ -404,27 +404,26 @@ repeat:
 			if (ret != TC_ACT_PIPE)
 				goto exec_done;
 		}
-		act = a->next;
 	}
 exec_done:
 	return ret;
 }
 EXPORT_SYMBOL(tcf_action_exec);
 
-void tcf_action_destroy(struct tc_action *act, int bind)
+void tcf_action_destroy(struct list_head *actions, int bind)
 {
-	struct tc_action *a;
+	struct tc_action *a, *tmp;
 
-	for (a = act; a; a = act) {
+	list_for_each_entry_safe(a, tmp, actions, list) {
 		if (a->ops) {
 			if (a->ops->cleanup(a, bind) == ACT_P_DELETED)
 				module_put(a->ops->owner);
-			act = act->next;
+			list_del(&a->list);
 			kfree(a);
 		} else {
 			/*FIXME: Remove later - catch insertion bugs*/
 			WARN(1, "tcf_action_destroy: BUG? destroying NULL ops\n");
-			act = act->next;
+			list_del(&a->list);
 			kfree(a);
 		}
 	}
@@ -470,14 +469,13 @@ nla_put_failure:
 EXPORT_SYMBOL(tcf_action_dump_1);
 
 int
-tcf_action_dump(struct sk_buff *skb, struct tc_action *act, int bind, int ref)
+tcf_action_dump(struct sk_buff *skb, struct list_head *actions, int bind, int ref)
 {
 	struct tc_action *a;
 	int err = -EINVAL;
 	struct nlattr *nest;
 
-	while ((a = act) != NULL) {
-		act = a->next;
+	list_for_each_entry(a, actions, list) {
 		nest = nla_nest_start(skb, a->order);
 		if (nest == NULL)
 			goto nla_put_failure;
@@ -552,6 +550,7 @@ struct tc_action *tcf_action_init_1(struct net *net, struct nlattr *nla,
 	if (a == NULL)
 		goto err_mod;
 
+	INIT_LIST_HEAD(&a->list);
 	/* backward compatibility for policer */
 	if (name == NULL)
 		err = a_o->init(net, tb[TCA_ACT_OPTIONS], est, a, ovr, bind);
@@ -578,37 +577,33 @@ err_out:
 	return ERR_PTR(err);
 }
 
-struct tc_action *tcf_action_init(struct net *net, struct nlattr *nla,
+int tcf_action_init(struct net *net, struct nlattr *nla,
 				  struct nlattr *est, char *name, int ovr,
-				  int bind)
+				  int bind, struct list_head *actions)
 {
 	struct nlattr *tb[TCA_ACT_MAX_PRIO + 1];
-	struct tc_action *head = NULL, *act, *act_prev = NULL;
+	struct tc_action *act;
 	int err;
 	int i;
 
 	err = nla_parse_nested(tb, TCA_ACT_MAX_PRIO, nla, NULL);
 	if (err < 0)
-		return ERR_PTR(err);
+		return err;
 
 	for (i = 1; i <= TCA_ACT_MAX_PRIO && tb[i]; i++) {
 		act = tcf_action_init_1(net, tb[i], est, name, ovr, bind);
-		if (IS_ERR(act))
+		if (IS_ERR(act)) {
+			err = PTR_ERR(act);
 			goto err;
+		}
 		act->order = i;
-
-		if (head == NULL)
-			head = act;
-		else
-			act_prev->next = act;
-		act_prev = act;
+		list_add_tail(&act->list, actions);
 	}
-	return head;
+	return 0;
 
 err:
-	if (head != NULL)
-		tcf_action_destroy(head, bind);
-	return act;
+	tcf_action_destroy(actions, bind);
+	return err;
 }
 
 int tcf_action_copy_stats(struct sk_buff *skb, struct tc_action *a,
@@ -653,7 +648,7 @@ errout:
 }
 
 static int
-tca_get_fill(struct sk_buff *skb, struct tc_action *a, u32 portid, u32 seq,
+tca_get_fill(struct sk_buff *skb, struct list_head *actions, u32 portid, u32 seq,
 	     u16 flags, int event, int bind, int ref)
 {
 	struct tcamsg *t;
@@ -673,7 +668,7 @@ tca_get_fill(struct sk_buff *skb, struct tc_action *a, u32 portid, u32 seq,
 	if (nest == NULL)
 		goto out_nlmsg_trim;
 
-	if (tcf_action_dump(skb, a, bind, ref) < 0)
+	if (tcf_action_dump(skb, actions, bind, ref) < 0)
 		goto out_nlmsg_trim;
 
 	nla_nest_end(skb, nest);
@@ -688,14 +683,14 @@ out_nlmsg_trim:
 
 static int
 act_get_notify(struct net *net, u32 portid, struct nlmsghdr *n,
-	       struct tc_action *a, int event)
+	       struct list_head *actions, int event)
 {
 	struct sk_buff *skb;
 
 	skb = alloc_skb(NLMSG_GOODSIZE, GFP_KERNEL);
 	if (!skb)
 		return -ENOBUFS;
-	if (tca_get_fill(skb, a, portid, n->nlmsg_seq, 0, event, 0, 0) <= 0) {
+	if (tca_get_fill(skb, actions, portid, n->nlmsg_seq, 0, event, 0, 0) <= 0) {
 		kfree_skb(skb);
 		return -EINVAL;
 	}
@@ -726,6 +721,7 @@ tcf_action_get_1(struct nlattr *nla, struct nlmsghdr *n, u32 portid)
 	if (a == NULL)
 		goto err_out;
 
+	INIT_LIST_HEAD(&a->list);
 	err = -EINVAL;
 	a->ops = tc_lookup_action(tb[TCA_ACT_KIND]);
 	if (a->ops == NULL)
@@ -745,12 +741,12 @@ err_out:
 	return ERR_PTR(err);
 }
 
-static void cleanup_a(struct tc_action *act)
+static void cleanup_a(struct list_head *actions)
 {
-	struct tc_action *a;
+	struct tc_action *a, *tmp;
 
-	for (a = act; a; a = act) {
-		act = a->next;
+	list_for_each_entry_safe(a, tmp, actions, list) {
+		list_del(&a->list);
 		kfree(a);
 	}
 }
@@ -765,6 +761,7 @@ static struct tc_action *create_a(int i)
 		return NULL;
 	}
 	act->order = i;
+	INIT_LIST_HEAD(&act->list);
 	return act;
 }
 
@@ -852,7 +849,8 @@ tca_action_gd(struct net *net, struct nlattr *nla, struct nlmsghdr *n,
 {
 	int i, ret;
 	struct nlattr *tb[TCA_ACT_MAX_PRIO + 1];
-	struct tc_action *head = NULL, *act, *act_prev = NULL;
+	struct tc_action *act;
+	LIST_HEAD(actions);
 
 	ret = nla_parse_nested(tb, TCA_ACT_MAX_PRIO, nla, NULL);
 	if (ret < 0)
@@ -872,16 +870,11 @@ tca_action_gd(struct net *net, struct nlattr *nla, struct nlmsghdr *n,
 			goto err;
 		}
 		act->order = i;
-
-		if (head == NULL)
-			head = act;
-		else
-			act_prev->next = act;
-		act_prev = act;
+		list_add_tail(&act->list, &actions);
 	}
 
 	if (event == RTM_GETACTION)
-		ret = act_get_notify(net, portid, n, head, event);
+		ret = act_get_notify(net, portid, n, &actions, event);
 	else { /* delete */
 		struct sk_buff *skb;
 
@@ -891,7 +884,7 @@ tca_action_gd(struct net *net, struct nlattr *nla, struct nlmsghdr *n,
 			goto err;
 		}
 
-		if (tca_get_fill(skb, head, portid, n->nlmsg_seq, 0, event,
+		if (tca_get_fill(skb, &actions, portid, n->nlmsg_seq, 0, event,
 				 0, 1) <= 0) {
 			kfree_skb(skb);
 			ret = -EINVAL;
@@ -899,7 +892,7 @@ tca_action_gd(struct net *net, struct nlattr *nla, struct nlmsghdr *n,
 		}
 
 		/* now do the delete */
-		tcf_action_destroy(head, 0);
+		tcf_action_destroy(&actions, 0);
 		ret = rtnetlink_send(skb, net, portid, RTNLGRP_TC,
 				     n->nlmsg_flags & NLM_F_ECHO);
 		if (ret > 0)
@@ -907,11 +900,11 @@ tca_action_gd(struct net *net, struct nlattr *nla, struct nlmsghdr *n,
 		return ret;
 	}
 err:
-	cleanup_a(head);
+	cleanup_a(&actions);
 	return ret;
 }
 
-static int tcf_add_notify(struct net *net, struct tc_action *a,
+static int tcf_add_notify(struct net *net, struct list_head *actions,
 			  u32 portid, u32 seq, int event, u16 flags)
 {
 	struct tcamsg *t;
@@ -939,7 +932,7 @@ static int tcf_add_notify(struct net *net, struct tc_action *a,
 	if (nest == NULL)
 		goto out_kfree_skb;
 
-	if (tcf_action_dump(skb, a, 0, 0) < 0)
+	if (tcf_action_dump(skb, actions, 0, 0) < 0)
 		goto out_kfree_skb;
 
 	nla_nest_end(skb, nest);
@@ -963,26 +956,18 @@ tcf_action_add(struct net *net, struct nlattr *nla, struct nlmsghdr *n,
 	       u32 portid, int ovr)
 {
 	int ret = 0;
-	struct tc_action *act;
-	struct tc_action *a;
+	LIST_HEAD(actions);
 	u32 seq = n->nlmsg_seq;
 
-	act = tcf_action_init(net, nla, NULL, NULL, ovr, 0);
-	if (act == NULL)
-		goto done;
-	if (IS_ERR(act)) {
-		ret = PTR_ERR(act);
+	ret = tcf_action_init(net, nla, NULL, NULL, ovr, 0, &actions);
+	if (ret)
 		goto done;
-	}
 
 	/* dump then free all the actions after update; inserted policy
 	 * stays intact
 	 */
-	ret = tcf_add_notify(net, act, portid, seq, RTM_NEWACTION, n->nlmsg_flags);
-	for (a = act; a; a = act) {
-		act = a->next;
-		kfree(a);
-	}
+	ret = tcf_add_notify(net, &actions, portid, seq, RTM_NEWACTION, n->nlmsg_flags);
+	cleanup_a(&actions);
 done:
 	return ret;
 }
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 8e118af..3c056d7 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -500,10 +500,8 @@ out:
 void tcf_exts_destroy(struct tcf_proto *tp, struct tcf_exts *exts)
 {
 #ifdef CONFIG_NET_CLS_ACT
-	if (exts->action) {
-		tcf_action_destroy(exts->action, TCA_ACT_UNBIND);
-		exts->action = NULL;
-	}
+	tcf_action_destroy(&exts->actions, TCA_ACT_UNBIND);
+	INIT_LIST_HEAD(&exts->actions);
 #endif
 }
 EXPORT_SYMBOL(tcf_exts_destroy);
@@ -518,6 +516,7 @@ int tcf_exts_validate(struct net *net, struct tcf_proto *tp, struct nlattr **tb,
 	{
 		struct tc_action *act;
 
+		INIT_LIST_HEAD(&exts->actions);
 		if (map->police && tb[map->police]) {
 			act = tcf_action_init_1(net, tb[map->police], rate_tlv,
 						"police", TCA_ACT_NOREPLACE,
@@ -525,16 +524,15 @@ int tcf_exts_validate(struct net *net, struct tcf_proto *tp, struct nlattr **tb,
 			if (IS_ERR(act))
 				return PTR_ERR(act);
 
-			act->type = TCA_OLD_COMPAT;
-			exts->action = act;
+			act->type = exts->type = TCA_OLD_COMPAT;
+			list_add(&act->list, &exts->actions);
 		} else if (map->action && tb[map->action]) {
-			act = tcf_action_init(net, tb[map->action], rate_tlv,
+			int err;
+			err = tcf_action_init(net, tb[map->action], rate_tlv,
 					      NULL, TCA_ACT_NOREPLACE,
-					      TCA_ACT_BIND);
-			if (IS_ERR(act))
-				return PTR_ERR(act);
-
-			exts->action = act;
+					      TCA_ACT_BIND, &exts->actions);
+			if (err)
+				return err;
 		}
 	}
 #else
@@ -551,43 +549,45 @@ void tcf_exts_change(struct tcf_proto *tp, struct tcf_exts *dst,
 		     struct tcf_exts *src)
 {
 #ifdef CONFIG_NET_CLS_ACT
-	if (src->action) {
-		struct tc_action *act;
+	if (!list_empty(&src->actions)) {
+		LIST_HEAD(tmp);
 		tcf_tree_lock(tp);
-		act = dst->action;
-		dst->action = src->action;
+		list_splice_init(&dst->actions, &tmp);
+		list_splice(&src->actions, &dst->actions);
 		tcf_tree_unlock(tp);
-		if (act)
-			tcf_action_destroy(act, TCA_ACT_UNBIND);
+		tcf_action_destroy(&tmp, TCA_ACT_UNBIND);
 	}
 #endif
 }
 EXPORT_SYMBOL(tcf_exts_change);
 
+#define tcf_exts_first_act(ext) \
+		list_first_entry(&(exts)->actions, struct tc_action, list)
+
 int tcf_exts_dump(struct sk_buff *skb, struct tcf_exts *exts,
 		  const struct tcf_ext_map *map)
 {
 #ifdef CONFIG_NET_CLS_ACT
-	if (map->action && exts->action) {
+	if (map->action && !list_empty(&exts->actions)) {
 		/*
 		 * again for backward compatible mode - we want
 		 * to work with both old and new modes of entering
 		 * tc data even if iproute2  was newer - jhs
 		 */
 		struct nlattr *nest;
-
-		if (exts->action->type != TCA_OLD_COMPAT) {
+		if (exts->type != TCA_OLD_COMPAT) {
 			nest = nla_nest_start(skb, map->action);
 			if (nest == NULL)
 				goto nla_put_failure;
-			if (tcf_action_dump(skb, exts->action, 0, 0) < 0)
+			if (tcf_action_dump(skb, &exts->actions, 0, 0) < 0)
 				goto nla_put_failure;
 			nla_nest_end(skb, nest);
 		} else if (map->police) {
+			struct tc_action *act = tcf_exts_first_act(exts);
 			nest = nla_nest_start(skb, map->police);
 			if (nest == NULL)
 				goto nla_put_failure;
-			if (tcf_action_dump_old(skb, exts->action, 0, 0) < 0)
+			if (tcf_action_dump_old(skb, act, 0, 0) < 0)
 				goto nla_put_failure;
 			nla_nest_end(skb, nest);
 		}
@@ -604,13 +604,11 @@ int tcf_exts_dump_stats(struct sk_buff *skb, struct tcf_exts *exts,
 			const struct tcf_ext_map *map)
 {
 #ifdef CONFIG_NET_CLS_ACT
-	if (exts->action)
-		if (tcf_action_copy_stats(skb, exts->action, 1) < 0)
-			goto nla_put_failure;
+	struct tc_action *a = tcf_exts_first_act(exts);
+	if (tcf_action_copy_stats(skb, a, 1) < 0)
+		return -1;
 #endif
 	return 0;
-nla_put_failure: __attribute__ ((unused))
-	return -1;
 }
 EXPORT_SYMBOL(tcf_exts_dump_stats);
 
diff --git a/net/sched/cls_basic.c b/net/sched/cls_basic.c
index 636d913..7b9b460 100644
--- a/net/sched/cls_basic.c
+++ b/net/sched/cls_basic.c
@@ -191,6 +191,7 @@ static int basic_change(struct net *net, struct sk_buff *in_skb,
 	if (f == NULL)
 		goto errout;
 
+	tcf_exts_init(&f->exts);
 	err = -EINVAL;
 	if (handle)
 		f->handle = handle;
diff --git a/net/sched/cls_bpf.c b/net/sched/cls_bpf.c
index d7c72be..90fe275 100644
--- a/net/sched/cls_bpf.c
+++ b/net/sched/cls_bpf.c
@@ -271,6 +271,7 @@ static int cls_bpf_change(struct net *net, struct sk_buff *in_skb,
 	if (prog == NULL)
 		return -ENOBUFS;
 
+	tcf_exts_init(&prog->exts);
 	if (handle == 0)
 		prog->handle = cls_bpf_grab_new_handle(tp, head);
 	else
diff --git a/net/sched/cls_cgroup.c b/net/sched/cls_cgroup.c
index 16006c9..e4fae03 100644
--- a/net/sched/cls_cgroup.c
+++ b/net/sched/cls_cgroup.c
@@ -203,6 +203,7 @@ static int cls_cgroup_change(struct net *net, struct sk_buff *in_skb,
 		if (head == NULL)
 			return -ENOBUFS;
 
+		tcf_exts_init(&head->exts);
 		head->handle = handle;
 
 		tcf_tree_lock(tp);
diff --git a/net/sched/cls_flow.c b/net/sched/cls_flow.c
index 7881e2f..411e0d8 100644
--- a/net/sched/cls_flow.c
+++ b/net/sched/cls_flow.c
@@ -455,6 +455,7 @@ static int flow_change(struct net *net, struct sk_buff *in_skb,
 
 		f->handle = handle;
 		f->mask	  = ~0U;
+		tcf_exts_init(&f->exts);
 
 		get_random_bytes(&f->hashrnd, 4);
 		f->perturb_timer.function = flow_perturbation;
diff --git a/net/sched/cls_fw.c b/net/sched/cls_fw.c
index 9b97172..d1cebad 100644
--- a/net/sched/cls_fw.c
+++ b/net/sched/cls_fw.c
@@ -280,6 +280,7 @@ static int fw_change(struct net *net, struct sk_buff *in_skb,
 	if (f == NULL)
 		return -ENOBUFS;
 
+	tcf_exts_init(&f->exts);
 	f->id = handle;
 
 	err = fw_change_attrs(net, tp, f, tb, tca, base);
diff --git a/net/sched/cls_route.c b/net/sched/cls_route.c
index 37da567..f1f1dfd 100644
--- a/net/sched/cls_route.c
+++ b/net/sched/cls_route.c
@@ -481,6 +481,7 @@ static int route4_change(struct net *net, struct sk_buff *in_skb,
 	if (f == NULL)
 		goto errout;
 
+	tcf_exts_init(&f->exts);
 	err = route4_set_parms(net, tp, base, f, handle, head, tb,
 		tca[TCA_RATE], 1);
 	if (err < 0)
diff --git a/net/sched/cls_rsvp.h b/net/sched/cls_rsvp.h
index 252d8b0..b1d3ce5 100644
--- a/net/sched/cls_rsvp.h
+++ b/net/sched/cls_rsvp.h
@@ -471,6 +471,7 @@ static int rsvp_change(struct net *net, struct sk_buff *in_skb,
 	if (f == NULL)
 		goto errout2;
 
+	tcf_exts_init(&f->exts);
 	h2 = 16;
 	if (tb[TCA_RSVP_SRC]) {
 		memcpy(f->src, nla_data(tb[TCA_RSVP_SRC]), sizeof(f->src));
diff --git a/net/sched/cls_tcindex.c b/net/sched/cls_tcindex.c
index b86535a..c39bbfc 100644
--- a/net/sched/cls_tcindex.c
+++ b/net/sched/cls_tcindex.c
@@ -215,11 +215,14 @@ tcindex_set_parms(struct net *net, struct tcf_proto *tp, unsigned long base,
 
 	memcpy(&cp, p, sizeof(cp));
 	memset(&new_filter_result, 0, sizeof(new_filter_result));
+	tcf_exts_init(&new_filter_result.exts);
 
 	if (old_r)
 		memcpy(&cr, r, sizeof(cr));
-	else
+	else {
 		memset(&cr, 0, sizeof(cr));
+		tcf_exts_init(&cr.exts);
+	}
 
 	if (tb[TCA_TCINDEX_HASH])
 		cp.hash = nla_get_u32(tb[TCA_TCINDEX_HASH]);
diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
index 59e546c..492d9a6 100644
--- a/net/sched/cls_u32.c
+++ b/net/sched/cls_u32.c
@@ -646,6 +646,7 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
 	n->ht_up = ht;
 	n->handle = handle;
 	n->fshift = s->hmask ? ffs(ntohl(s->hmask)) - 1 : 0;
+	tcf_exts_init(&n->exts);
 
 #ifdef CONFIG_CLS_U32_MARK
 	if (tb[TCA_U32_MARK]) {
-- 
1.8.1.4

Re: [PATCH net-next v4 2/8] net_sched: act: use standard struct list_head

[Jamal Hadi Salim]

On 12/15/13 23:15, Cong Wang wrote:
> Currently actions are chained by a singly linked list,
> therefore it is a bit hard to add and remove a specific
> entry. Convert it to struct list_head so that in the
> latter patch we can remove an action without finding
> its head.
>
> Cc: Jamal Hadi Salim <jhs@mojatatu.com>
> Cc: David S. Miller <davem@davemloft.net>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>


Look good, thanks.

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

cheers,
jamal

[PATCH net-next v4 3/8] net_sched: mirred: remove action when the target device is gone

[Cong Wang]

When the target device is removed, the mirred action is
still there but with the dev pointer setting to NULL.
This makes the output from 'tc filter' ugly. There is no
reason to keep it.

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/tc_act/tc_mirred.h |  4 +++-
 net/sched/act_mirred.c         | 15 +++++++--------
 2 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/include/net/tc_act/tc_mirred.h b/include/net/tc_act/tc_mirred.h
index cfe2943..2026cf6 100644
--- a/include/net/tc_act/tc_mirred.h
+++ b/include/net/tc_act/tc_mirred.h
@@ -7,9 +7,11 @@ struct tcf_mirred {
 	struct tcf_common	common;
 	int			tcfm_eaction;
 	int			tcfm_ifindex;
-	int			tcfm_ok_push;
+	unsigned int		tcfm_ok_push:1;
+	unsigned int		tcfm_bind:1;
 	struct net_device	*tcfm_dev;
 	struct list_head	tcfm_list;
+	struct tc_action	*tcfm_act;
 };
 #define to_mirred(pc) \
 	container_of(pc, struct tcf_mirred, common)
diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
index 2523781..c26bfe5 100644
--- a/net/sched/act_mirred.c
+++ b/net/sched/act_mirred.c
@@ -136,6 +136,8 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
 		dev_hold(dev);
 		m->tcfm_dev = dev;
 		m->tcfm_ok_push = ok_push;
+		m->tcfm_bind = bind;
+		m->tcfm_act = a;
 	}
 	spin_unlock_bh(&m->tcf_lock);
 	if (ret == ACT_P_CREATED) {
@@ -169,10 +171,6 @@ static int tcf_mirred(struct sk_buff *skb, const struct tc_action *a,
 	bstats_update(&m->tcf_bstats, skb);
 
 	dev = m->tcfm_dev;
-	if (!dev) {
-		printk_once(KERN_NOTICE "tc mirred: target device is gone\n");
-		goto out;
-	}
 
 	if (!(dev->flags & IFF_UP)) {
 		net_notice_ratelimited("tc mirred to Houston: device %s is down\n",
@@ -244,13 +242,14 @@ static int mirred_device_event(struct notifier_block *unused,
 			       unsigned long event, void *ptr)
 {
 	struct net_device *dev = netdev_notifier_info_to_dev(ptr);
-	struct tcf_mirred *m;
+	struct tcf_mirred *m, *tmp;
 
 	if (event == NETDEV_UNREGISTER)
-		list_for_each_entry(m, &mirred_list, tcfm_list) {
+		list_for_each_entry_safe(m, tmp, &mirred_list, tcfm_list) {
 			if (m->tcfm_dev == dev) {
-				dev_put(dev);
-				m->tcfm_dev = NULL;
+				list_del(&m->tcfm_act->list);
+				tcf_mirred_cleanup(m->tcfm_act, m->tcfm_bind);
+				kfree(m->tcfm_act);
 			}
 		}
 
-- 
1.8.1.4

Re: [PATCH net-next v4 3/8] net_sched: mirred: remove action when the target device is gone

[Jamal Hadi Salim]

On 12/15/13 23:15, Cong Wang wrote:
> When the target device is removed, the mirred action is
> still there but with the dev pointer setting to NULL.
> This makes the output from 'tc filter' ugly. There is no
> reason to keep it.
>

Sorry - this one i have problems with.
actions may be referenced from multiple filters,
you cant just delete it (that would leave other users
pointing to it dangling). Destroying would eventually
delete it when the refcount goes to 0.
[And when we delete actions we send netlink events to announce
that]. The proper solution would require i.e tag it as to be
deleted and implement some form of garbage collection.
Do you mind leaving this one out for this series?

cheers,
jamal

> 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/tc_act/tc_mirred.h |  4 +++-
>   net/sched/act_mirred.c         | 15 +++++++--------
>   2 files changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/include/net/tc_act/tc_mirred.h b/include/net/tc_act/tc_mirred.h
> index cfe2943..2026cf6 100644
> --- a/include/net/tc_act/tc_mirred.h
> +++ b/include/net/tc_act/tc_mirred.h
> @@ -7,9 +7,11 @@ struct tcf_mirred {
>   	struct tcf_common	common;
>   	int			tcfm_eaction;
>   	int			tcfm_ifindex;
> -	int			tcfm_ok_push;
> +	unsigned int		tcfm_ok_push:1;
> +	unsigned int		tcfm_bind:1;
>   	struct net_device	*tcfm_dev;
>   	struct list_head	tcfm_list;
> +	struct tc_action	*tcfm_act;
>   };
>   #define to_mirred(pc) \
>   	container_of(pc, struct tcf_mirred, common)
> diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
> index 2523781..c26bfe5 100644
> --- a/net/sched/act_mirred.c
> +++ b/net/sched/act_mirred.c
> @@ -136,6 +136,8 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
>   		dev_hold(dev);
>   		m->tcfm_dev = dev;
>   		m->tcfm_ok_push = ok_push;
> +		m->tcfm_bind = bind;
> +		m->tcfm_act = a;
>   	}
>   	spin_unlock_bh(&m->tcf_lock);
>   	if (ret == ACT_P_CREATED) {
> @@ -169,10 +171,6 @@ static int tcf_mirred(struct sk_buff *skb, const struct tc_action *a,
>   	bstats_update(&m->tcf_bstats, skb);
>
>   	dev = m->tcfm_dev;
> -	if (!dev) {
> -		printk_once(KERN_NOTICE "tc mirred: target device is gone\n");
> -		goto out;
> -	}
>
>   	if (!(dev->flags & IFF_UP)) {
>   		net_notice_ratelimited("tc mirred to Houston: device %s is down\n",
> @@ -244,13 +242,14 @@ static int mirred_device_event(struct notifier_block *unused,
>   			       unsigned long event, void *ptr)
>   {
>   	struct net_device *dev = netdev_notifier_info_to_dev(ptr);
> -	struct tcf_mirred *m;
> +	struct tcf_mirred *m, *tmp;
>
>   	if (event == NETDEV_UNREGISTER)
> -		list_for_each_entry(m, &mirred_list, tcfm_list) {
> +		list_for_each_entry_safe(m, tmp, &mirred_list, tcfm_list) {
>   			if (m->tcfm_dev == dev) {
> -				dev_put(dev);
> -				m->tcfm_dev = NULL;
> +				list_del(&m->tcfm_act->list);
> +				tcf_mirred_cleanup(m->tcfm_act, m->tcfm_bind);
> +				kfree(m->tcfm_act);
>   			}
>   		}
>
>

Re: [PATCH net-next v4 3/8] net_sched: mirred: remove action when the target device is gone

[Cong Wang]

On Wed, Dec 18, 2013 at 6:31 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> On 12/15/13 23:15, Cong Wang wrote:
>>
>> When the target device is removed, the mirred action is
>> still there but with the dev pointer setting to NULL.
>> This makes the output from 'tc filter' ugly. There is no
>> reason to keep it.
>>
>
> Sorry - this one i have problems with.
> actions may be referenced from multiple filters,
> you cant just delete it (that would leave other users
> pointing to it dangling). Destroying would eventually
> delete it when the refcount goes to 0.

How? tcf_action_init() always allocates a new action,
it doesn't even look for an existing one.

> [And when we delete actions we send netlink events to announce
> that]. The proper solution would require i.e tag it as to be
> deleted and implement some form of garbage collection.

It doesn't worth to have a GC for this...

Even if what you said is true, we should just make a copy
for each of the filters. I just tried to create two different filters
with same and different mirred actions, my patch works
perfectly.

Re: [PATCH net-next v4 3/8] net_sched: mirred: remove action when the target device is gone

[David Miller]

From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Wed, 18 Dec 2013 10:36:06 -0800

> On Wed, Dec 18, 2013 at 6:31 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>> On 12/15/13 23:15, Cong Wang wrote:
>>>
>>> When the target device is removed, the mirred action is
>>> still there but with the dev pointer setting to NULL.
>>> This makes the output from 'tc filter' ugly. There is no
>>> reason to keep it.
>>>
>>
>> Sorry - this one i have problems with.
>> actions may be referenced from multiple filters,
>> you cant just delete it (that would leave other users
>> pointing to it dangling). Destroying would eventually
>> delete it when the refcount goes to 0.
> 
> How? tcf_action_init() always allocates a new action,
> it doesn't even look for an existing one.
> 
>> [And when we delete actions we send netlink events to announce
>> that]. The proper solution would require i.e tag it as to be
>> deleted and implement some form of garbage collection.
> 
> It doesn't worth to have a GC for this...
> 
> Even if what you said is true, we should just make a copy
> for each of the filters. I just tried to create two different filters
> with same and different mirred actions, my patch works
> perfectly.

Indeed, I think Jamal is confusing how he intended the code to work
vs. how it actually does :-)

Re: [PATCH net-next v4 3/8] net_sched: mirred: remove action when the target device is gone

[Jamal Hadi Salim]

On 12/18/13 13:36, Cong Wang wrote:
> On Wed, Dec 18, 2013 at 6:31 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>> On 12/15/13 23:15, Cong Wang wrote:
>>>
>>> When the target device is removed, the mirred action is
>>> still there but with the dev pointer setting to NULL.
>>> This makes the output from 'tc filter' ugly. There is no
>>> reason to keep it.
>>>
>>
>> Sorry - this one i have problems with.
>> actions may be referenced from multiple filters,
>> you cant just delete it (that would leave other users
>> pointing to it dangling). Destroying would eventually
>> delete it when the refcount goes to 0.
>
> How? tcf_action_init() always allocates a new action,
> it doesn't even look for an existing one.
>


tc action blah index 123
tc action filter goo action blah index 123
tc action filter gah action blah index 123

Very useful for example for multiple flows to
share the same policer.


>> [And when we delete actions we send netlink events to announce
>> that]. The proper solution would require i.e tag it as to be
>> deleted and implement some form of garbage collection.
>
> It doesn't worth to have a GC for this...
>
> Even if what you said is true, we should just make a copy
> for each of the filters. I just tried to create two different filters
> with same and different mirred actions, my patch works
> perfectly.
>

A copy wont work. We want sharing.

cheers,
jamal

Re: [PATCH net-next v4 3/8] net_sched: mirred: remove action when the target device is gone

[Cong Wang]

On Wed, Dec 18, 2013 at 11:50 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> On 12/18/13 13:36, Cong Wang wrote:
>>
>> On Wed, Dec 18, 2013 at 6:31 AM, Jamal Hadi Salim <jhs@mojatatu.com>
>> wrote:
>>>
>>> On 12/15/13 23:15, Cong Wang wrote:
>>>>
>>>>
>>>> When the target device is removed, the mirred action is
>>>> still there but with the dev pointer setting to NULL.
>>>> This makes the output from 'tc filter' ugly. There is no
>>>> reason to keep it.
>>>>
>>>
>>> Sorry - this one i have problems with.
>>> actions may be referenced from multiple filters,
>>> you cant just delete it (that would leave other users
>>> pointing to it dangling). Destroying would eventually
>>> delete it when the refcount goes to 0.
>>
>>
>> How? tcf_action_init() always allocates a new action,
>> it doesn't even look for an existing one.
>>
>
>
> tc action blah index 123
> tc action filter goo action blah index 123
> tc action filter gah action blah index 123
>
> Very useful for example for multiple flows to
> share the same policer.
>

Ah, I see. I will create a test case for this
and rework on the mirred patch.

Re: [PATCH net-next v4 3/8] net_sched: mirred: remove action when the target device is gone

[Jamal Hadi Salim]

[-- Attachment #1: Type: text/plain, Size: 1773 bytes --]



I am sorry Cong - I will still object to this change. I dont want
to even bother testing it.
You are making some serious policy decisions in the kernel.
Such policy decisions should be made by user space not the kernel.
Whoever made the idiotic decision of removing the device should
modify or delete the flow rule - at minimal they
may deserve some warning. Deleting the action is wrong. It is simple
graph theory.
Slightly related is this - look at the attached test (I went out of
my way to annotate it) and imagine you have mirred where the policers
are ....

cheers,
jamal

On 12/18/13 16:42, Cong Wang wrote:
> On Wed, Dec 18, 2013 at 11:50 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>> On 12/18/13 13:36, Cong Wang wrote:
>>>
>>> On Wed, Dec 18, 2013 at 6:31 AM, Jamal Hadi Salim <jhs@mojatatu.com>
>>> wrote:
>>>>
>>>> On 12/15/13 23:15, Cong Wang wrote:
>>>>>
>>>>>
>>>>> When the target device is removed, the mirred action is
>>>>> still there but with the dev pointer setting to NULL.
>>>>> This makes the output from 'tc filter' ugly. There is no
>>>>> reason to keep it.
>>>>>
>>>>
>>>> Sorry - this one i have problems with.
>>>> actions may be referenced from multiple filters,
>>>> you cant just delete it (that would leave other users
>>>> pointing to it dangling). Destroying would eventually
>>>> delete it when the refcount goes to 0.
>>>
>>>
>>> How? tcf_action_init() always allocates a new action,
>>> it doesn't even look for an existing one.
>>>
>>
>>
>> tc action blah index 123
>> tc action filter goo action blah index 123
>> tc action filter gah action blah index 123
>>
>> Very useful for example for multiple flows to
>> share the same policer.
>>
>
> Ah, I see. I will create a test case for this
> and rework on the mirred patch.
>


[-- Attachment #2: shared-act-tst1 --]
[-- Type: text/plain, Size: 1346 bytes --]

TC="/sbin/tc"
#
sudo ifconfig dummy0 up
sudo ifconfig dummy1 up
DEV1="eth0"
DEV2="eth1"
#
#
#
sudo $TC qdisc del dev $DEV1 ingress
sudo $TC qdisc add dev $DEV1 ingress
sudo $TC qdisc del dev $DEV2 ingress
sudo $TC qdisc add dev $DEV2 ingress
#Note - older tc did not pipe for skbedit (add "pipe" if this is rejected)
sudo $TC filter add dev $DEV1 parent ffff: protocol ip pref 10 \
u32 match ip protocol 1 0xff flowid 1:10 \
action skbedit mark 11 \
action police rate 10kbit burst 10k pipe index 1 \
action skbedit mark 12 \
action police rate 20kbit burst 20k pipe index 2 \
action action mirred egress mirror dev dummy0
#note sharing of the two policers by two flows
#across multiple devices
sudo $TC filter add dev $DEV2 parent ffff: protocol ip pref 10 \
u32 match ip protocol 1 0xff flowid 1:10 \
action skbedit mark 21 \
action police rate 10kbit burst 10k pipe index 1 \
action skbedit mark 22 \
action police rate 20kbit burst 20k pipe index 2 \
action action mirred egress mirror dev dummy1
#
#
# now do a ping -f to somewhere, for more exciting results
# do a -s 2048 or so
# sudo ping -f 192.168.1.100 -c 10000
# sudo ping -f 127.0.0.1 -c 10000
#
sudo tc -s filter ls dev $DEV1 parent ffff: protocol ip 
sudo tc -s filter ls dev $DEV2 parent ffff: protocol ip 
sudo tc -s actions ls action police
sudo tc -s actions ls action skbedit

Re: [PATCH net-next v4 3/8] net_sched: mirred: remove action when the target device is gone

[Cong Wang]

On Sun, Dec 22, 2013 at 8:15 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>
>
> I am sorry Cong - I will still object to this change. I dont want
> to even bother testing it.
> You are making some serious policy decisions in the kernel.
> Such policy decisions should be made by user space not the kernel.

You know qdiscs and filters are removed too when the device
is gone, right? So isn't that also a policy you are talking about?

This doesn't make any sense to me, if it did, you should remove
all net device notifications in kernel, right?

> Whoever made the idiotic decision of removing the device should
> modify or delete the flow rule - at minimal they
> may deserve some warning. Deleting the action is wrong. It is simple
> graph theory.

Have you ever thought about how hard it is to remove a mirred action
upon the device removal? Even with libnl, we still have to:

1) monitor the device removal notification
2) search the action cache to get the action matches the target device
3) search for the filters that contains such actions
4) remove the action by changing the filters it is attached

Try it and see how much more work you will do, compare it with
this kernel patch. It would not be hard to conclude which is easier.

Re: [PATCH net-next v4 3/8] net_sched: mirred: remove action when the target device is gone

[Jamal Hadi Salim]

On 12/22/13 14:42, Cong Wang wrote:
> On Sun, Dec 22, 2013 at 8:15 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:

> You know qdiscs and filters are removed too when the device
> is gone, right? So isn't that also a policy you are talking about?
>

That is an easy optimization that made sense to make - we
deleted the root of a graph. No need to spam the policy manager.
What we are talking about is deleting a faulty node shared
by multiple graphs and deleting the vertex but then leaving
dangling edges around. Doesnt make sense.
No action that is shared between two flows or devices is EVER
going to be removed because you deleted a qdisc or a netdev.

> This doesn't make any sense to me, if it did, you should remove
> all net device notifications in kernel, right?
>

Refer to above.

> Have you ever thought about how hard it is to remove a mirred action
> upon the device removal? Even with libnl, we still have to:
>
> 1) monitor the device removal notification
> 2) search the action cache to get the action matches the target device
> 3) search for the filters that contains such actions
> 4) remove the action by changing the filters it is attached
>
> Try it and see how much more work you will do, compare it with
> this kernel patch. It would not be hard to conclude which is easier.
>

It is not about ease - it is about correctness.
You are making a macroscopic decision with a microscopic view. This is
why we have control planes. It is like the legs making decisions on
behalf of the brain.


cheers,
jamal

Re: [PATCH net-next v4 3/8] net_sched: mirred: remove action when the target device is gone

[Cong Wang]

On Sun, Dec 22, 2013 at 12:31 PM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> On 12/22/13 14:42, Cong Wang wrote:
>>
>> On Sun, Dec 22, 2013 at 8:15 AM, Jamal Hadi Salim <jhs@mojatatu.com>
>> wrote:
>
>
>> You know qdiscs and filters are removed too when the device
>> is gone, right? So isn't that also a policy you are talking about?
>>
>
> That is an easy optimization that made sense to make - we
> deleted the root of a graph. No need to spam the policy manager.
> What we are talking about is deleting a faulty node shared
> by multiple graphs and deleting the vertex but then leaving
> dangling edges around. Doesnt make sense.
> No action that is shared between two flows or devices is EVER
> going to be removed because you deleted a qdisc or a netdev.

NOTHING you talked about here is relevant to the policy or
mechanism you talked previously. Just correctness.

If removing an shared action is impossible, what about non-shared
ones? Actually for my _own_ use case, I even don't use nor care
about shared one...

Re: [PATCH net-next v4 3/8] net_sched: mirred: remove action when the target device is gone

[Jamal Hadi Salim]

On 12/22/13 16:11, Cong Wang wrote:

> NOTHING you talked about here is relevant to the policy or
> mechanism you talked previously. Just correctness.
>

It is about _correct policy_
If you have a packets going:
->A-->B-->C-->D-->E

That is the policy (decided by someone/thing in user space).
A to E are mechanisms.
If something goes wrong with mechanism D, you dont go deleting
D because it is affecting other things in the graph. You have
no clue what it would affect - and if you try to build that in
it is not cheap for no good reason.
You let the thing/person who set it know and do the deletion.

You compared it to something going wrong with A(when A is netdev)
and how we delete everything underneath. They are not the same,
in that case, we know precisely that is the begining of the graph
and we can unreference everything underneath.

Does that make more sense?

> If removing an shared action is impossible, what about non-shared
> ones? Actually for my _own_ use case, I even don't use nor care
> about shared one...
>


Refer to above.

cheers,
jamal

Re: [PATCH net-next v4 3/8] net_sched: mirred: remove action when the target device is gone

[Cong Wang]

On Mon, Dec 23, 2013 at 4:41 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> On 12/22/13 16:11, Cong Wang wrote:
>
>> NOTHING you talked about here is relevant to the policy or
>> mechanism you talked previously. Just correctness.
>>
>
> It is about _correct policy_
> If you have a packets going:
> ->A-->B-->C-->D-->E
>
> That is the policy (decided by someone/thing in user space).
> A to E are mechanisms.

Apparently you have a different definition of "policy" and "mechanism"
with me, probably with others too...

I assume you mean actions here, since we don't care about others
in this thread.

> If something goes wrong with mechanism D, you dont go deleting
> D because it is affecting other things in the graph. You have
> no clue what it would affect - and if you try to build that in
> it is not cheap for no good reason.

Since they are chained by a singly or doubly linked list in
non-shared case, where are "other things in the graph"?

Please do explain how can they be in a graph in non-shared
case, it is not obvious to me at all.

> You let the thing/person who set it know and do the deletion.
>
> You compared it to something going wrong with A(when A is netdev)
> and how we delete everything underneath. They are not the same,
> in that case, we know precisely that is the begining of the graph
> and we can unreference everything underneath.

Nope, I only want to remove A from the chain by list_del() for non-shared
case.

>
> Does that make more sense?
>

Nope, you continue to mention graph, but don't explain why there is
a graph in non-shared case. Nor I think you use "policy" or "mechanism"
correctly. :)

Jamal, please don't be abstract, just talk about actions. We DO NOT
care about others in this thread.

Re: [PATCH net-next v4 3/8] net_sched: mirred: remove action when the target device is gone

[Jamal Hadi Salim]

On 12/23/13 13:50, Cong Wang wrote:
> On Mon, Dec 23, 2013 at 4:41 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:

>>
>> It is about _correct policy_
>> If you have a packets going:
>> ->A-->B-->C-->D-->E
>>
>> That is the policy (decided by someone/thing in user space).
>> A to E are mechanisms.
>
> Apparently you have a different definition of "policy" and "mechanism"
> with me, probably with others too...
>

I think only with you ;->

> I assume you mean actions here, since we don't care about others
> in this thread.
>
[..]
>
> Since they are chained by a singly or doubly linked list in
> non-shared case, where are "other things in the graph"?
>

Nothing to do with implementation - think conceptually.

> Please do explain how can they be in a graph in non-shared
> case, it is not obvious to me at all.
>

[..]
> Nope, I only want to remove A from the chain by list_del() for non-shared
> case.
>

[..]

>
> Nope, you continue to mention graph, but don't explain why there is
> a graph in non-shared case. Nor I think you use "policy" or "mechanism"
> correctly. :)
>
> Jamal, please don't be abstract, just talk about actions. We DO NOT
> care about others in this thread.
>

It is not just actions that constitute the graph. It starts at netdev.
Actions are more complex because you have a real DAG. i.e you can
have branches etc.
So how best to explain this?
Ive gone the avenue of showing you shared actions and failed. So i am
trying to simplify it to a single basic graph;->
I will try again to use the graph concept:

If i or some program decide that a packet coming on is going to
traverse:
  -->netdevX->qdiscA->classifierB->{ActionC->Action D->ActionE}

That is a policy. It tells different parts of the kernel(the mechanisms)
how to treat a packet that traverses them in order to create some
resultant service.
What i drew above is a graph (as noted above {} is more complex
because you could have branches etc, eg i could go on Action C to do
sometimes but skip to E other times based on state, actually you
could have that with classifiers - but lets ignore that for now).
Each of the things preceeded by "->" is a graph node, otherwise known as 
a vertex (eg netdevX).
Each of the "->" is an known as a graph edge.
Each of the nodes in a graph is a mechanism.
If this still doesnt make sense to you, I can describe it differently
without using concept of graphs.

Now if you have such a graph:
User space can delete the node netdevX and then you (kernel)
can dereference all that is underneath it. That is a good optimization
since user just deleted the root; we can then notify user only about 
netdevX. It doesnt matter if Action D is shared; reference count will
decrease and if it hits zero, real destroy will happen.
User can delete the qdiscA and then kernel can dereference everything
underneath. That is fair optimization as above.
User can delete the filter and kernel can dereference
everything underneath it. That is also fair optimization as above.
What you cannot do is, on your own as kernel, decide you want to delete
one action that is _bound_ to a filter because one of its attributes is
gone berserk. It doesnt matter whether such an action is mirred or foo
or whether D and E dont exist. You can put a big hole where the town
used to be and leave roads leading to the town.
It will still be referenced by things preeceding it (primarily the 
classifier which keeps an action chain intact), which is bad when the
next packet arrives.
You let the user/control program which is monitoring things do that
and "reroute" around the problem. If you insist on putting a little part
of the medula oblangata in the stomach, then:
the only correct way to do it is delete the filter.
And you start doing that you are making some serious policy decisions
in the kernel and adding lots of complexity.

Ok, does it make sense now?;->

cheers,
jamal

Re: [PATCH net-next v4 3/8] net_sched: mirred: remove action when the target device is gone

[Cong Wang]

On Mon, Dec 23, 2013 at 12:41 PM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>
> Now if you have such a graph:

Making such an abstraction only helps misunderstanding, really...

> What you cannot do is, on your own as kernel, decide you want to delete
> one action that is _bound_ to a filter because one of its attributes is
> gone berserk. It doesnt matter whether such an action is mirred or foo
> or whether D and E dont exist. You can put a big hole where the town
> used to be and leave roads leading to the town.

Again, since (non-shared) actions attached to a filter are chained by
a doubly linked list, why not?

> It will still be referenced by things preeceding it (primarily the
> classifier which keeps an action chain intact), which is bad when the
> next packet arrives.

You know, there is a head for such linked list for the filter to refer,
so what's the problem with deleting a node from a linked list if
we lock it properly? In non-shared case, no filter can refer to any
action without the head of the chain, right? So what is the problem
of "referenced by things preeceding it" when they are linked and locked
properly?

> You let the user/control program which is monitoring things do that
> and "reroute" around the problem. If you insist on putting a little part
> of the medula oblangata in the stomach, then:
> the only correct way to do it is delete the filter.

Apparent not, different actions can attached to the same filter
and only mirred action has a target dev.

> And you start doing that you are making some serious policy decisions
> in the kernel and adding lots of complexity.
>

Why removing a filter when the netdev is removed is not policy?
Actually it is, ONLY that it is not be able to shared with current
implementation.

Since you love mechanism _so much_, why not make filters shared
by other qdisc's and stop removing them when netdev is gone in kernel?

Be realistic, Jamal, user-space is hard, you can't simply let user-space
decide everything, especially when you don't provide a simple way to do it.
Netlink is already hard to use, even with libnl, since it enforces a cache
layer. Sit down and spend some time to write some libnl code, compare it
with this patch.

Re: [PATCH net-next v4 3/8] net_sched: mirred: remove action when the target device is gone

[Jamal Hadi Salim]

On 12/23/13 17:14, Cong Wang wrote:
> On Mon, Dec 23, 2013 at 12:41 PM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>>
>> Now if you have such a graph:
>
> Making such an abstraction only helps misunderstanding, really...
>

So I used a graph that maps to a netdevice, qdisc, filter and actions
like you asked to - there is still a misunderstanding?

>> What you cannot do is, on your own as kernel, decide you want to delete
>> one action that is _bound_ to a filter because one of its attributes is
>> gone berserk. It doesnt matter whether such an action is mirred or foo
>> or whether D and E dont exist. You can put a big hole where the town
>> used to be and leave roads leading to the town.
>
> Again, since (non-shared) actions attached to a filter are chained by
> a doubly linked list, why not?
>
>> It will still be referenced by things preeceding it (primarily the
>> classifier which keeps an action chain intact), which is bad when the
>> next packet arrives.
>
> You know, there is a head for such linked list for the filter to refer,
> so what's the problem with deleting a node from a linked list if
> we lock it properly? In non-shared case, no filter can refer to any
> action without the head of the chain, right? So what is the problem
> of "referenced by things preeceding it" when they are linked and locked
> properly?
>

Huh? This has nothing to do with whether you are capable to remove a
node from a list. It is about you making a decision that the node should
be removed. You dont have sufficient information to make such a decision.
Remember earlier i said you are making a macroscopic decision by looking
at microscope? I just saw a white blood cell, let me chop that toe.

>> You let the user/control program which is monitoring things do that
>> and "reroute" around the problem. If you insist on putting a little part
>> of the medula oblangata in the stomach, then:
>> the only correct way to do it is delete the filter.
>
> Apparent not, different actions can attached to the same filter
> and only mirred action has a target dev.
>

Parse error.

>> And you start doing that you are making some serious policy decisions
>> in the kernel and adding lots of complexity.
>>
>
> Why removing a filter when the netdev is removed is not policy?
> Actually it is, ONLY that it is not be able to shared with current
> implementation.
>

Of course it is. Since you understand that why do you think removing
an action which is part of that policy is a good idea?

> Since you love mechanism _so much_, why not make filters shared
> by other qdisc's and stop removing them when netdev is gone in kernel?
>
> Be realistic, Jamal, user-space is hard, you can't simply let user-space
> decide everything, especially when you don't provide a simple way to do it.
> Netlink is already hard to use, even with libnl, since it enforces a cache
> layer. Sit down and spend some time to write some libnl code, compare it
> with this patch.
>

You gotta be joking. I live through this every ither day.
Frankly, I think i will have no progress if i tried to convince you the
world is round. I am dropping from this discussion.

cheers,
jamal

Re: [PATCH net-next v4 3/8] net_sched: mirred: remove action when the target device is gone

[Cong Wang]

On Mon, Dec 23, 2013 at 2:30 PM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>
> Huh? This has nothing to do with whether you are capable to remove a
> node from a list. It is about you making a decision that the node should
> be removed. You dont have sufficient information to make such a decision.

Then which lacks?

>> Why removing a filter when the netdev is removed is not policy?
>> Actually it is, ONLY that it is not be able to shared with current
>> implementation.
>>
>
> Of course it is. Since you understand that why do you think removing
> an action which is part of that policy is a good idea?
>

Well, I think doing such policy decision for both filters and actions in
kernel IS fine, simply because it is harder for user-space. We already
have many of such policies, they fit well.


>
>> Since you love mechanism _so much_, why not make filters shared
>> by other qdisc's and stop removing them when netdev is gone in kernel?
>>
>> Be realistic, Jamal, user-space is hard, you can't simply let user-space
>> decide everything, especially when you don't provide a simple way to do
>> it.
>> Netlink is already hard to use, even with libnl, since it enforces a cache
>> layer. Sit down and spend some time to write some libnl code, compare it
>> with this patch.
>>
>
> You gotta be joking. I live through this every ither day.

Great! Then show me how to do remove a mirred action upon
the target device is gone in user-space, please?

> Frankly, I think i will have no progress if i tried to convince you the
> world is round. I am dropping from this discussion.
>

Your replies show you care about policy vs mechanism very much,
but you even don't do any thing with either 1) try to show me how
the user-space implements it or 2) go head to make same mechanism
for filters (and qdisc) as well.

Do something to prove that you said. :) Otherwise, I think you are kidding
me.

Re: [PATCH net-next v4 3/8] net_sched: mirred: remove action when the target device is gone

[Jamal Hadi Salim]

On 12/23/13 17:51, Cong Wang wrote:

[..]

> Great! Then show me how to do remove a mirred action upon
> the target device is gone in user-space, please?

[..]

> Your replies show you care about policy vs mechanism very much,
> but you even don't do any thing with either 1) try to show me how
> the user-space implements it or 2) go head to make same mechanism
> for filters (and qdisc) as well.
>
> Do something to prove that you said. :) Otherwise, I think you are kidding
> me.
>

No disrespect intended Cong - but lets just end this discussion agreeing
to disagree. On my side I violently disagree. I hope not to spend any
more energy on this.

cheers,
jamal

Re: [PATCH net-next v4 3/8] net_sched: mirred: remove action when the target device is gone

[Cong Wang]

On Mon, Dec 23, 2013 at 3:05 PM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>
> No disrespect intended Cong - but lets just end this discussion agreeing
> to disagree. On my side I violently disagree. I hope not to spend any
> more energy on this.
>

Sorry, I think your argument on policy vs mechanism doesn't make
any sense at all. Therefore I don't accept it.

[PATCH net-next v4 4/8] net_sched: cls: refactor out struct tcf_ext_map

[Cong Wang]

These information can be saved in tcf_exts, and this will
simplify the code.

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/pkt_cls.h   | 23 ++++++++++-------------
 net/sched/cls_api.c     | 31 +++++++++++++------------------
 net/sched/cls_basic.c   | 14 +++++---------
 net/sched/cls_bpf.c     | 14 +++++---------
 net/sched/cls_cgroup.c  | 15 +++++----------
 net/sched/cls_flow.c    | 14 +++++---------
 net/sched/cls_fw.c      | 14 +++++---------
 net/sched/cls_route.c   | 14 +++++---------
 net/sched/cls_rsvp.h    | 14 +++++---------
 net/sched/cls_tcindex.c | 16 ++++++----------
 net/sched/cls_u32.c     | 14 +++++---------
 11 files changed, 69 insertions(+), 114 deletions(-)

diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
index 34fe693d..50ea079 100644
--- a/include/net/pkt_cls.h
+++ b/include/net/pkt_cls.h
@@ -65,21 +65,21 @@ struct tcf_exts {
 	__u32	type; /* for backward compat(TCA_OLD_COMPAT) */
 	struct list_head actions;
 #endif
-};
-
-/* Map to export classifier specific extension TLV types to the
- * generic extensions API. Unsupported extensions must be set to 0.
- */
-struct tcf_ext_map {
+	/* Map to export classifier specific extension TLV types to the
+	 * generic extensions API. Unsupported extensions must be set to 0.
+	 */
 	int action;
 	int police;
 };
 
-static inline void tcf_exts_init(struct tcf_exts *exts)
+static inline void tcf_exts_init(struct tcf_exts *exts, int action, int police)
 {
 #ifdef CONFIG_NET_CLS_ACT
+	exts->type = 0;
 	INIT_LIST_HEAD(&exts->actions);
 #endif
+	exts->action = action;
+	exts->police = police;
 }
 
 /**
@@ -136,15 +136,12 @@ tcf_exts_exec(struct sk_buff *skb, struct tcf_exts *exts,
 
 int tcf_exts_validate(struct net *net, struct tcf_proto *tp,
 		      struct nlattr **tb, struct nlattr *rate_tlv,
-		      struct tcf_exts *exts,
-		      const struct tcf_ext_map *map);
+		      struct tcf_exts *exts);
 void tcf_exts_destroy(struct tcf_proto *tp, struct tcf_exts *exts);
 void tcf_exts_change(struct tcf_proto *tp, struct tcf_exts *dst,
 		     struct tcf_exts *src);
-int tcf_exts_dump(struct sk_buff *skb, struct tcf_exts *exts,
-		  const struct tcf_ext_map *map);
-int tcf_exts_dump_stats(struct sk_buff *skb, struct tcf_exts *exts,
-			const struct tcf_ext_map *map);
+int tcf_exts_dump(struct sk_buff *skb, struct tcf_exts *exts);
+int tcf_exts_dump_stats(struct sk_buff *skb, struct tcf_exts *exts);
 
 /**
  * struct tcf_pkt_info - packet information
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 3c056d7..028c980 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -507,18 +507,15 @@ void tcf_exts_destroy(struct tcf_proto *tp, struct tcf_exts *exts)
 EXPORT_SYMBOL(tcf_exts_destroy);
 
 int tcf_exts_validate(struct net *net, struct tcf_proto *tp, struct nlattr **tb,
-		  struct nlattr *rate_tlv, struct tcf_exts *exts,
-		  const struct tcf_ext_map *map)
+		  struct nlattr *rate_tlv, struct tcf_exts *exts)
 {
-	memset(exts, 0, sizeof(*exts));
-
 #ifdef CONFIG_NET_CLS_ACT
 	{
 		struct tc_action *act;
 
 		INIT_LIST_HEAD(&exts->actions);
-		if (map->police && tb[map->police]) {
-			act = tcf_action_init_1(net, tb[map->police], rate_tlv,
+		if (exts->police && tb[exts->police]) {
+			act = tcf_action_init_1(net, tb[exts->police], rate_tlv,
 						"police", TCA_ACT_NOREPLACE,
 						TCA_ACT_BIND);
 			if (IS_ERR(act))
@@ -526,9 +523,9 @@ int tcf_exts_validate(struct net *net, struct tcf_proto *tp, struct nlattr **tb,
 
 			act->type = exts->type = TCA_OLD_COMPAT;
 			list_add(&act->list, &exts->actions);
-		} else if (map->action && tb[map->action]) {
+		} else if (exts->action && tb[exts->action]) {
 			int err;
-			err = tcf_action_init(net, tb[map->action], rate_tlv,
+			err = tcf_action_init(net, tb[exts->action], rate_tlv,
 					      NULL, TCA_ACT_NOREPLACE,
 					      TCA_ACT_BIND, &exts->actions);
 			if (err)
@@ -536,8 +533,8 @@ int tcf_exts_validate(struct net *net, struct tcf_proto *tp, struct nlattr **tb,
 		}
 	}
 #else
-	if ((map->action && tb[map->action]) ||
-	    (map->police && tb[map->police]))
+	if ((exts->action && tb[exts->action]) ||
+	    (exts->police && tb[exts->police]))
 		return -EOPNOTSUPP;
 #endif
 
@@ -564,11 +561,10 @@ EXPORT_SYMBOL(tcf_exts_change);
 #define tcf_exts_first_act(ext) \
 		list_first_entry(&(exts)->actions, struct tc_action, list)
 
-int tcf_exts_dump(struct sk_buff *skb, struct tcf_exts *exts,
-		  const struct tcf_ext_map *map)
+int tcf_exts_dump(struct sk_buff *skb, struct tcf_exts *exts)
 {
 #ifdef CONFIG_NET_CLS_ACT
-	if (map->action && !list_empty(&exts->actions)) {
+	if (exts->action && !list_empty(&exts->actions)) {
 		/*
 		 * again for backward compatible mode - we want
 		 * to work with both old and new modes of entering
@@ -576,15 +572,15 @@ int tcf_exts_dump(struct sk_buff *skb, struct tcf_exts *exts,
 		 */
 		struct nlattr *nest;
 		if (exts->type != TCA_OLD_COMPAT) {
-			nest = nla_nest_start(skb, map->action);
+			nest = nla_nest_start(skb, exts->action);
 			if (nest == NULL)
 				goto nla_put_failure;
 			if (tcf_action_dump(skb, &exts->actions, 0, 0) < 0)
 				goto nla_put_failure;
 			nla_nest_end(skb, nest);
-		} else if (map->police) {
+		} else if (exts->police) {
 			struct tc_action *act = tcf_exts_first_act(exts);
-			nest = nla_nest_start(skb, map->police);
+			nest = nla_nest_start(skb, exts->police);
 			if (nest == NULL)
 				goto nla_put_failure;
 			if (tcf_action_dump_old(skb, act, 0, 0) < 0)
@@ -600,8 +596,7 @@ nla_put_failure: __attribute__ ((unused))
 EXPORT_SYMBOL(tcf_exts_dump);
 
 
-int tcf_exts_dump_stats(struct sk_buff *skb, struct tcf_exts *exts,
-			const struct tcf_ext_map *map)
+int tcf_exts_dump_stats(struct sk_buff *skb, struct tcf_exts *exts)
 {
 #ifdef CONFIG_NET_CLS_ACT
 	struct tc_action *a = tcf_exts_first_act(exts);
diff --git a/net/sched/cls_basic.c b/net/sched/cls_basic.c
index 7b9b460..b655203 100644
--- a/net/sched/cls_basic.c
+++ b/net/sched/cls_basic.c
@@ -34,11 +34,6 @@ struct basic_filter {
 	struct list_head	link;
 };
 
-static const struct tcf_ext_map basic_ext_map = {
-	.action = TCA_BASIC_ACT,
-	.police = TCA_BASIC_POLICE
-};
-
 static int basic_classify(struct sk_buff *skb, const struct tcf_proto *tp,
 			  struct tcf_result *res)
 {
@@ -141,7 +136,8 @@ static int basic_set_parms(struct net *net, struct tcf_proto *tp,
 	struct tcf_exts e;
 	struct tcf_ematch_tree t;
 
-	err = tcf_exts_validate(net, tp, tb, est, &e, &basic_ext_map);
+	tcf_exts_init(&e, TCA_BASIC_ACT, TCA_BASIC_POLICE);
+	err = tcf_exts_validate(net, tp, tb, est, &e);
 	if (err < 0)
 		return err;
 
@@ -191,7 +187,7 @@ static int basic_change(struct net *net, struct sk_buff *in_skb,
 	if (f == NULL)
 		goto errout;
 
-	tcf_exts_init(&f->exts);
+	tcf_exts_init(&f->exts, TCA_BASIC_ACT, TCA_BASIC_POLICE);
 	err = -EINVAL;
 	if (handle)
 		f->handle = handle;
@@ -264,13 +260,13 @@ static int basic_dump(struct tcf_proto *tp, unsigned long fh,
 	    nla_put_u32(skb, TCA_BASIC_CLASSID, f->res.classid))
 		goto nla_put_failure;
 
-	if (tcf_exts_dump(skb, &f->exts, &basic_ext_map) < 0 ||
+	if (tcf_exts_dump(skb, &f->exts) < 0 ||
 	    tcf_em_tree_dump(skb, &f->ematches, TCA_BASIC_EMATCHES) < 0)
 		goto nla_put_failure;
 
 	nla_nest_end(skb, nest);
 
-	if (tcf_exts_dump_stats(skb, &f->exts, &basic_ext_map) < 0)
+	if (tcf_exts_dump_stats(skb, &f->exts) < 0)
 		goto nla_put_failure;
 
 	return skb->len;
diff --git a/net/sched/cls_bpf.c b/net/sched/cls_bpf.c
index 90fe275..00a5a58 100644
--- a/net/sched/cls_bpf.c
+++ b/net/sched/cls_bpf.c
@@ -46,11 +46,6 @@ static const struct nla_policy bpf_policy[TCA_BPF_MAX + 1] = {
 				    .len = sizeof(struct sock_filter) * BPF_MAXINSNS },
 };
 
-static const struct tcf_ext_map bpf_ext_map = {
-	.action = TCA_BPF_ACT,
-	.police = TCA_BPF_POLICE,
-};
-
 static int cls_bpf_classify(struct sk_buff *skb, const struct tcf_proto *tp,
 			    struct tcf_result *res)
 {
@@ -174,7 +169,8 @@ static int cls_bpf_modify_existing(struct net *net, struct tcf_proto *tp,
 	if (!tb[TCA_BPF_OPS_LEN] || !tb[TCA_BPF_OPS] || !tb[TCA_BPF_CLASSID])
 		return -EINVAL;
 
-	ret = tcf_exts_validate(net, tp, tb, est, &exts, &bpf_ext_map);
+	tcf_exts_init(&exts, TCA_BPF_ACT, TCA_BPF_POLICE);
+	ret = tcf_exts_validate(net, tp, tb, est, &exts);
 	if (ret < 0)
 		return ret;
 
@@ -271,7 +267,7 @@ static int cls_bpf_change(struct net *net, struct sk_buff *in_skb,
 	if (prog == NULL)
 		return -ENOBUFS;
 
-	tcf_exts_init(&prog->exts);
+	tcf_exts_init(&prog->exts, TCA_BPF_ACT, TCA_BPF_POLICE);
 	if (handle == 0)
 		prog->handle = cls_bpf_grab_new_handle(tp, head);
 	else
@@ -326,12 +322,12 @@ static int cls_bpf_dump(struct tcf_proto *tp, unsigned long fh,
 
 	memcpy(nla_data(nla), prog->bpf_ops, nla_len(nla));
 
-	if (tcf_exts_dump(skb, &prog->exts, &bpf_ext_map) < 0)
+	if (tcf_exts_dump(skb, &prog->exts) < 0)
 		goto nla_put_failure;
 
 	nla_nest_end(skb, nest);
 
-	if (tcf_exts_dump_stats(skb, &prog->exts, &bpf_ext_map) < 0)
+	if (tcf_exts_dump_stats(skb, &prog->exts) < 0)
 		goto nla_put_failure;
 
 	return skb->len;
diff --git a/net/sched/cls_cgroup.c b/net/sched/cls_cgroup.c
index e4fae03..f9d21258 100644
--- a/net/sched/cls_cgroup.c
+++ b/net/sched/cls_cgroup.c
@@ -172,11 +172,6 @@ static int cls_cgroup_init(struct tcf_proto *tp)
 	return 0;
 }
 
-static const struct tcf_ext_map cgroup_ext_map = {
-	.action = TCA_CGROUP_ACT,
-	.police = TCA_CGROUP_POLICE,
-};
-
 static const struct nla_policy cgroup_policy[TCA_CGROUP_MAX + 1] = {
 	[TCA_CGROUP_EMATCHES]	= { .type = NLA_NESTED },
 };
@@ -203,7 +198,7 @@ static int cls_cgroup_change(struct net *net, struct sk_buff *in_skb,
 		if (head == NULL)
 			return -ENOBUFS;
 
-		tcf_exts_init(&head->exts);
+		tcf_exts_init(&head->exts, TCA_CGROUP_ACT, TCA_CGROUP_POLICE);
 		head->handle = handle;
 
 		tcf_tree_lock(tp);
@@ -219,8 +214,8 @@ static int cls_cgroup_change(struct net *net, struct sk_buff *in_skb,
 	if (err < 0)
 		return err;
 
-	err = tcf_exts_validate(net, tp, tb, tca[TCA_RATE], &e,
-				&cgroup_ext_map);
+	tcf_exts_init(&e, TCA_CGROUP_ACT, TCA_CGROUP_POLICE);
+	err = tcf_exts_validate(net, tp, tb, tca[TCA_RATE], &e);
 	if (err < 0)
 		return err;
 
@@ -278,13 +273,13 @@ static int cls_cgroup_dump(struct tcf_proto *tp, unsigned long fh,
 	if (nest == NULL)
 		goto nla_put_failure;
 
-	if (tcf_exts_dump(skb, &head->exts, &cgroup_ext_map) < 0 ||
+	if (tcf_exts_dump(skb, &head->exts) < 0 ||
 	    tcf_em_tree_dump(skb, &head->ematches, TCA_CGROUP_EMATCHES) < 0)
 		goto nla_put_failure;
 
 	nla_nest_end(skb, nest);
 
-	if (tcf_exts_dump_stats(skb, &head->exts, &cgroup_ext_map) < 0)
+	if (tcf_exts_dump_stats(skb, &head->exts) < 0)
 		goto nla_put_failure;
 
 	return skb->len;
diff --git a/net/sched/cls_flow.c b/net/sched/cls_flow.c
index 411e0d8..f9c8b05 100644
--- a/net/sched/cls_flow.c
+++ b/net/sched/cls_flow.c
@@ -56,11 +56,6 @@ struct flow_filter {
 	u32			hashrnd;
 };
 
-static const struct tcf_ext_map flow_ext_map = {
-	.action	= TCA_FLOW_ACT,
-	.police	= TCA_FLOW_POLICE,
-};
-
 static inline u32 addr_fold(void *addr)
 {
 	unsigned long a = (unsigned long)addr;
@@ -397,7 +392,8 @@ static int flow_change(struct net *net, struct sk_buff *in_skb,
 			return -EOPNOTSUPP;
 	}
 
-	err = tcf_exts_validate(net, tp, tb, tca[TCA_RATE], &e, &flow_ext_map);
+	tcf_exts_init(&e, TCA_FLOW_ACT, TCA_FLOW_POLICE);
+	err = tcf_exts_validate(net, tp, tb, tca[TCA_RATE], &e);
 	if (err < 0)
 		return err;
 
@@ -455,7 +451,7 @@ static int flow_change(struct net *net, struct sk_buff *in_skb,
 
 		f->handle = handle;
 		f->mask	  = ~0U;
-		tcf_exts_init(&f->exts);
+		tcf_exts_init(&f->exts, TCA_FLOW_ACT, TCA_FLOW_POLICE);
 
 		get_random_bytes(&f->hashrnd, 4);
 		f->perturb_timer.function = flow_perturbation;
@@ -609,7 +605,7 @@ static int flow_dump(struct tcf_proto *tp, unsigned long fh,
 	    nla_put_u32(skb, TCA_FLOW_PERTURB, f->perturb_period / HZ))
 		goto nla_put_failure;
 
-	if (tcf_exts_dump(skb, &f->exts, &flow_ext_map) < 0)
+	if (tcf_exts_dump(skb, &f->exts) < 0)
 		goto nla_put_failure;
 #ifdef CONFIG_NET_EMATCH
 	if (f->ematches.hdr.nmatches &&
@@ -618,7 +614,7 @@ static int flow_dump(struct tcf_proto *tp, unsigned long fh,
 #endif
 	nla_nest_end(skb, nest);
 
-	if (tcf_exts_dump_stats(skb, &f->exts, &flow_ext_map) < 0)
+	if (tcf_exts_dump_stats(skb, &f->exts) < 0)
 		goto nla_put_failure;
 
 	return skb->len;
diff --git a/net/sched/cls_fw.c b/net/sched/cls_fw.c
index d1cebad..3f9cece 100644
--- a/net/sched/cls_fw.c
+++ b/net/sched/cls_fw.c
@@ -46,11 +46,6 @@ struct fw_filter {
 	struct tcf_exts		exts;
 };
 
-static const struct tcf_ext_map fw_ext_map = {
-	.action = TCA_FW_ACT,
-	.police = TCA_FW_POLICE
-};
-
 static inline int fw_hash(u32 handle)
 {
 	if (HTSIZE == 4096)
@@ -200,7 +195,8 @@ fw_change_attrs(struct net *net, struct tcf_proto *tp, struct fw_filter *f,
 	u32 mask;
 	int err;
 
-	err = tcf_exts_validate(net, tp, tb, tca[TCA_RATE], &e, &fw_ext_map);
+	tcf_exts_init(&e, TCA_FW_ACT, TCA_FW_POLICE);
+	err = tcf_exts_validate(net, tp, tb, tca[TCA_RATE], &e);
 	if (err < 0)
 		return err;
 
@@ -280,7 +276,7 @@ static int fw_change(struct net *net, struct sk_buff *in_skb,
 	if (f == NULL)
 		return -ENOBUFS;
 
-	tcf_exts_init(&f->exts);
+	tcf_exts_init(&f->exts, TCA_FW_ACT, TCA_FW_POLICE);
 	f->id = handle;
 
 	err = fw_change_attrs(net, tp, f, tb, tca, base);
@@ -360,12 +356,12 @@ static int fw_dump(struct tcf_proto *tp, unsigned long fh,
 	    nla_put_u32(skb, TCA_FW_MASK, head->mask))
 		goto nla_put_failure;
 
-	if (tcf_exts_dump(skb, &f->exts, &fw_ext_map) < 0)
+	if (tcf_exts_dump(skb, &f->exts) < 0)
 		goto nla_put_failure;
 
 	nla_nest_end(skb, nest);
 
-	if (tcf_exts_dump_stats(skb, &f->exts, &fw_ext_map) < 0)
+	if (tcf_exts_dump_stats(skb, &f->exts) < 0)
 		goto nla_put_failure;
 
 	return skb->len;
diff --git a/net/sched/cls_route.c b/net/sched/cls_route.c
index f1f1dfd..2473953 100644
--- a/net/sched/cls_route.c
+++ b/net/sched/cls_route.c
@@ -59,11 +59,6 @@ struct route4_filter {
 
 #define ROUTE4_FAILURE ((struct route4_filter *)(-1L))
 
-static const struct tcf_ext_map route_ext_map = {
-	.police = TCA_ROUTE4_POLICE,
-	.action = TCA_ROUTE4_ACT
-};
-
 static inline int route4_fastmap_hash(u32 id, int iif)
 {
 	return id & 0xF;
@@ -347,7 +342,8 @@ static int route4_set_parms(struct net *net, struct tcf_proto *tp,
 	struct route4_bucket *b;
 	struct tcf_exts e;
 
-	err = tcf_exts_validate(net, tp, tb, est, &e, &route_ext_map);
+	tcf_exts_init(&e, TCA_ROUTE4_ACT, TCA_ROUTE4_POLICE);
+	err = tcf_exts_validate(net, tp, tb, est, &e);
 	if (err < 0)
 		return err;
 
@@ -481,7 +477,7 @@ static int route4_change(struct net *net, struct sk_buff *in_skb,
 	if (f == NULL)
 		goto errout;
 
-	tcf_exts_init(&f->exts);
+	tcf_exts_init(&f->exts, TCA_ROUTE4_ACT, TCA_ROUTE4_POLICE);
 	err = route4_set_parms(net, tp, base, f, handle, head, tb,
 		tca[TCA_RATE], 1);
 	if (err < 0)
@@ -590,12 +586,12 @@ static int route4_dump(struct tcf_proto *tp, unsigned long fh,
 	    nla_put_u32(skb, TCA_ROUTE4_CLASSID, f->res.classid))
 		goto nla_put_failure;
 
-	if (tcf_exts_dump(skb, &f->exts, &route_ext_map) < 0)
+	if (tcf_exts_dump(skb, &f->exts) < 0)
 		goto nla_put_failure;
 
 	nla_nest_end(skb, nest);
 
-	if (tcf_exts_dump_stats(skb, &f->exts, &route_ext_map) < 0)
+	if (tcf_exts_dump_stats(skb, &f->exts) < 0)
 		goto nla_put_failure;
 
 	return skb->len;
diff --git a/net/sched/cls_rsvp.h b/net/sched/cls_rsvp.h
index b1d3ce5..4f25c2a 100644
--- a/net/sched/cls_rsvp.h
+++ b/net/sched/cls_rsvp.h
@@ -116,11 +116,6 @@ static inline unsigned int hash_src(__be32 *src)
 	return h & 0xF;
 }
 
-static struct tcf_ext_map rsvp_ext_map = {
-	.police = TCA_RSVP_POLICE,
-	.action = TCA_RSVP_ACT
-};
-
 #define RSVP_APPLY_RESULT()				\
 {							\
 	int r = tcf_exts_exec(skb, &f->exts, res);	\
@@ -440,7 +435,8 @@ static int rsvp_change(struct net *net, struct sk_buff *in_skb,
 	if (err < 0)
 		return err;
 
-	err = tcf_exts_validate(net, tp, tb, tca[TCA_RATE], &e, &rsvp_ext_map);
+	tcf_exts_init(&e, TCA_RSVP_ACT, TCA_RSVP_POLICE);
+	err = tcf_exts_validate(net, tp, tb, tca[TCA_RATE], &e);
 	if (err < 0)
 		return err;
 
@@ -471,7 +467,7 @@ static int rsvp_change(struct net *net, struct sk_buff *in_skb,
 	if (f == NULL)
 		goto errout2;
 
-	tcf_exts_init(&f->exts);
+	tcf_exts_init(&f->exts, TCA_RSVP_ACT, TCA_RSVP_POLICE);
 	h2 = 16;
 	if (tb[TCA_RSVP_SRC]) {
 		memcpy(f->src, nla_data(tb[TCA_RSVP_SRC]), sizeof(f->src));
@@ -634,12 +630,12 @@ static int rsvp_dump(struct tcf_proto *tp, unsigned long fh,
 	    nla_put(skb, TCA_RSVP_SRC, sizeof(f->src), f->src))
 		goto nla_put_failure;
 
-	if (tcf_exts_dump(skb, &f->exts, &rsvp_ext_map) < 0)
+	if (tcf_exts_dump(skb, &f->exts) < 0)
 		goto nla_put_failure;
 
 	nla_nest_end(skb, nest);
 
-	if (tcf_exts_dump_stats(skb, &f->exts, &rsvp_ext_map) < 0)
+	if (tcf_exts_dump_stats(skb, &f->exts) < 0)
 		goto nla_put_failure;
 	return skb->len;
 
diff --git a/net/sched/cls_tcindex.c b/net/sched/cls_tcindex.c
index c39bbfc..ffad187 100644
--- a/net/sched/cls_tcindex.c
+++ b/net/sched/cls_tcindex.c
@@ -50,11 +50,6 @@ struct tcindex_data {
 	int fall_through;	/* 0: only classify if explicit match */
 };
 
-static const struct tcf_ext_map tcindex_ext_map = {
-	.police = TCA_TCINDEX_POLICE,
-	.action = TCA_TCINDEX_ACT
-};
-
 static inline int
 tcindex_filter_is_set(struct tcindex_filter_result *r)
 {
@@ -209,19 +204,20 @@ tcindex_set_parms(struct net *net, struct tcf_proto *tp, unsigned long base,
 	struct tcindex_filter *f = NULL; /* make gcc behave */
 	struct tcf_exts e;
 
-	err = tcf_exts_validate(net, tp, tb, est, &e, &tcindex_ext_map);
+	tcf_exts_init(&e, TCA_TCINDEX_ACT, TCA_TCINDEX_POLICE);
+	err = tcf_exts_validate(net, tp, tb, est, &e);
 	if (err < 0)
 		return err;
 
 	memcpy(&cp, p, sizeof(cp));
 	memset(&new_filter_result, 0, sizeof(new_filter_result));
-	tcf_exts_init(&new_filter_result.exts);
+	tcf_exts_init(&new_filter_result.exts, TCA_TCINDEX_ACT, TCA_TCINDEX_POLICE);
 
 	if (old_r)
 		memcpy(&cr, r, sizeof(cr));
 	else {
 		memset(&cr, 0, sizeof(cr));
-		tcf_exts_init(&cr.exts);
+		tcf_exts_init(&cr.exts, TCA_TCINDEX_ACT, TCA_TCINDEX_POLICE);
 	}
 
 	if (tb[TCA_TCINDEX_HASH])
@@ -471,11 +467,11 @@ static int tcindex_dump(struct tcf_proto *tp, unsigned long fh,
 		    nla_put_u32(skb, TCA_TCINDEX_CLASSID, r->res.classid))
 			goto nla_put_failure;
 
-		if (tcf_exts_dump(skb, &r->exts, &tcindex_ext_map) < 0)
+		if (tcf_exts_dump(skb, &r->exts) < 0)
 			goto nla_put_failure;
 		nla_nest_end(skb, nest);
 
-		if (tcf_exts_dump_stats(skb, &r->exts, &tcindex_ext_map) < 0)
+		if (tcf_exts_dump_stats(skb, &r->exts) < 0)
 			goto nla_put_failure;
 	}
 
diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
index 492d9a6..20f2fb7 100644
--- a/net/sched/cls_u32.c
+++ b/net/sched/cls_u32.c
@@ -79,11 +79,6 @@ struct tc_u_common {
 	u32			hgenerator;
 };
 
-static const struct tcf_ext_map u32_ext_map = {
-	.action = TCA_U32_ACT,
-	.police = TCA_U32_POLICE
-};
-
 static inline unsigned int u32_hash_fold(__be32 key,
 					 const struct tc_u32_sel *sel,
 					 u8 fshift)
@@ -496,7 +491,8 @@ static int u32_set_parms(struct net *net, struct tcf_proto *tp,
 	int err;
 	struct tcf_exts e;
 
-	err = tcf_exts_validate(net, tp, tb, est, &e, &u32_ext_map);
+	tcf_exts_init(&e, TCA_U32_ACT, TCA_U32_POLICE);
+	err = tcf_exts_validate(net, tp, tb, est, &e);
 	if (err < 0)
 		return err;
 
@@ -646,7 +642,7 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
 	n->ht_up = ht;
 	n->handle = handle;
 	n->fshift = s->hmask ? ffs(ntohl(s->hmask)) - 1 : 0;
-	tcf_exts_init(&n->exts);
+	tcf_exts_init(&n->exts, TCA_U32_ACT, TCA_U32_POLICE);
 
 #ifdef CONFIG_CLS_U32_MARK
 	if (tb[TCA_U32_MARK]) {
@@ -760,7 +756,7 @@ static int u32_dump(struct tcf_proto *tp, unsigned long fh,
 			goto nla_put_failure;
 #endif
 
-		if (tcf_exts_dump(skb, &n->exts, &u32_ext_map) < 0)
+		if (tcf_exts_dump(skb, &n->exts) < 0)
 			goto nla_put_failure;
 
 #ifdef CONFIG_NET_CLS_IND
@@ -779,7 +775,7 @@ static int u32_dump(struct tcf_proto *tp, unsigned long fh,
 	nla_nest_end(skb, nest);
 
 	if (TC_U32_KEY(n->handle))
-		if (tcf_exts_dump_stats(skb, &n->exts, &u32_ext_map) < 0)
+		if (tcf_exts_dump_stats(skb, &n->exts) < 0)
 			goto nla_put_failure;
 	return skb->len;
 
-- 
1.8.1.4

Re: [PATCH net-next v4 4/8] net_sched: cls: refactor out struct tcf_ext_map

[Jamal Hadi Salim]

On 12/15/13 23:15, Cong Wang wrote:
> These information can be saved in tcf_exts, and this will
> simplify the code.
>
> 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, thanks.

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

cheers,
jamal

[PATCH net-next v4 5/8] net_sched: init struct tcf_hashinfo at register time

[Cong Wang]

It looks weird to store the lock out of the struct but
still points to a static variable. Just move them into the struct.

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   | 18 +++++++++++++++++-
 net/sched/act_api.c     | 16 ++++++++--------
 net/sched/act_csum.c    | 13 +++++--------
 net/sched/act_gact.c    | 13 +++++--------
 net/sched/act_ipt.c     | 21 ++++++++++-----------
 net/sched/act_mirred.c  | 16 +++++++---------
 net/sched/act_nat.c     | 12 +++++-------
 net/sched/act_pedit.c   | 12 +++++-------
 net/sched/act_police.c  | 38 +++++++++++++++++++-------------------
 net/sched/act_simple.c  | 20 +++++++++++---------
 net/sched/act_skbedit.c | 13 +++++--------
 11 files changed, 97 insertions(+), 95 deletions(-)

diff --git a/include/net/act_api.h b/include/net/act_api.h
index a726426..2b5ec5a 100644
--- a/include/net/act_api.h
+++ b/include/net/act_api.h
@@ -38,7 +38,7 @@ struct tcf_common {
 struct tcf_hashinfo {
 	struct tcf_common	**htab;
 	unsigned int		hmask;
-	rwlock_t		*lock;
+	rwlock_t		lock;
 };
 
 static inline unsigned int tcf_hash(u32 index, unsigned int hmask)
@@ -46,6 +46,22 @@ static inline unsigned int tcf_hash(u32 index, unsigned int hmask)
 	return index & hmask;
 }
 
+static inline int tcf_hashinfo_init(struct tcf_hashinfo *hf, unsigned int mask)
+{
+	rwlock_init(&hf->lock);
+	hf->hmask = mask;
+	hf->htab = kzalloc((mask + 1) * sizeof(struct tcf_common *),
+			   GFP_KERNEL);
+	if (!hf->htab)
+		return -ENOMEM;
+	return 0;
+}
+
+static inline void tcf_hashinfo_destroy(struct tcf_hashinfo *hf)
+{
+	kfree(hf->htab);
+}
+
 #ifdef CONFIG_NET_CLS_ACT
 
 #define ACT_P_CREATED 1
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 7d84183..125673d 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -34,9 +34,9 @@ void tcf_hash_destroy(struct tcf_common *p, struct tcf_hashinfo *hinfo)
 
 	for (p1p = &hinfo->htab[h]; *p1p; p1p = &(*p1p)->tcfc_next) {
 		if (*p1p == p) {
-			write_lock_bh(hinfo->lock);
+			write_lock_bh(&hinfo->lock);
 			*p1p = p->tcfc_next;
-			write_unlock_bh(hinfo->lock);
+			write_unlock_bh(&hinfo->lock);
 			gen_kill_estimator(&p->tcfc_bstats,
 					   &p->tcfc_rate_est);
 			/*
@@ -77,7 +77,7 @@ static int tcf_dump_walker(struct sk_buff *skb, struct netlink_callback *cb,
 	int err = 0, index = -1, i = 0, s_i = 0, n_i = 0;
 	struct nlattr *nest;
 
-	read_lock_bh(hinfo->lock);
+	read_lock_bh(&hinfo->lock);
 
 	s_i = cb->args[0];
 
@@ -107,7 +107,7 @@ static int tcf_dump_walker(struct sk_buff *skb, struct netlink_callback *cb,
 		}
 	}
 done:
-	read_unlock_bh(hinfo->lock);
+	read_unlock_bh(&hinfo->lock);
 	if (n_i)
 		cb->args[0] += n_i;
 	return n_i;
@@ -170,13 +170,13 @@ struct tcf_common *tcf_hash_lookup(u32 index, struct tcf_hashinfo *hinfo)
 {
 	struct tcf_common *p;
 
-	read_lock_bh(hinfo->lock);
+	read_lock_bh(&hinfo->lock);
 	for (p = hinfo->htab[tcf_hash(index, hinfo->hmask)]; p;
 	     p = p->tcfc_next) {
 		if (p->tcfc_index == index)
 			break;
 	}
-	read_unlock_bh(hinfo->lock);
+	read_unlock_bh(&hinfo->lock);
 
 	return p;
 }
@@ -257,10 +257,10 @@ void tcf_hash_insert(struct tcf_common *p, struct tcf_hashinfo *hinfo)
 {
 	unsigned int h = tcf_hash(p->tcfc_index, hinfo->hmask);
 
-	write_lock_bh(hinfo->lock);
+	write_lock_bh(&hinfo->lock);
 	p->tcfc_next = hinfo->htab[h];
 	hinfo->htab[h] = p;
-	write_unlock_bh(hinfo->lock);
+	write_unlock_bh(&hinfo->lock);
 }
 EXPORT_SYMBOL(tcf_hash_insert);
 
diff --git a/net/sched/act_csum.c b/net/sched/act_csum.c
index 5c5edf5..5d350c5 100644
--- a/net/sched/act_csum.c
+++ b/net/sched/act_csum.c
@@ -37,15 +37,8 @@
 #include <net/tc_act/tc_csum.h>
 
 #define CSUM_TAB_MASK 15
-static struct tcf_common *tcf_csum_ht[CSUM_TAB_MASK + 1];
 static u32 csum_idx_gen;
-static DEFINE_RWLOCK(csum_lock);
-
-static struct tcf_hashinfo csum_hash_info = {
-	.htab	= tcf_csum_ht,
-	.hmask	= CSUM_TAB_MASK,
-	.lock	= &csum_lock,
-};
+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), },
@@ -593,6 +586,10 @@ MODULE_LICENSE("GPL");
 
 static int __init csum_init_module(void)
 {
+	int err = tcf_hashinfo_init(&csum_hash_info, CSUM_TAB_MASK+1);
+	if (err)
+		return err;
+
 	return tcf_register_action(&act_csum_ops);
 }
 
diff --git a/net/sched/act_gact.c b/net/sched/act_gact.c
index 5645a4d..1e6e0e7 100644
--- a/net/sched/act_gact.c
+++ b/net/sched/act_gact.c
@@ -24,15 +24,8 @@
 #include <net/tc_act/tc_gact.h>
 
 #define GACT_TAB_MASK	15
-static struct tcf_common *tcf_gact_ht[GACT_TAB_MASK + 1];
 static u32 gact_idx_gen;
-static DEFINE_RWLOCK(gact_lock);
-
-static struct tcf_hashinfo gact_hash_info = {
-	.htab	=	tcf_gact_ht,
-	.hmask	=	GACT_TAB_MASK,
-	.lock	=	&gact_lock,
-};
+static struct tcf_hashinfo gact_hash_info;
 
 #ifdef CONFIG_GACT_PROB
 static int gact_net_rand(struct tcf_gact *gact)
@@ -215,6 +208,9 @@ MODULE_LICENSE("GPL");
 
 static int __init gact_init_module(void)
 {
+	int err = tcf_hashinfo_init(&gact_hash_info, GACT_TAB_MASK+1);
+	if (err)
+		return err;
 #ifdef CONFIG_GACT_PROB
 	pr_info("GACT probability on\n");
 #else
@@ -226,6 +222,7 @@ static int __init gact_init_module(void)
 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 882a897..8344380 100644
--- a/net/sched/act_ipt.c
+++ b/net/sched/act_ipt.c
@@ -29,15 +29,8 @@
 
 
 #define IPT_TAB_MASK     15
-static struct tcf_common *tcf_ipt_ht[IPT_TAB_MASK + 1];
 static u32 ipt_idx_gen;
-static DEFINE_RWLOCK(ipt_lock);
-
-static struct tcf_hashinfo ipt_hash_info = {
-	.htab	=	tcf_ipt_ht,
-	.hmask	=	IPT_TAB_MASK,
-	.lock	=	&ipt_lock,
-};
+static struct tcf_hashinfo ipt_hash_info;
 
 static int ipt_init_target(struct xt_entry_target *t, char *table, unsigned int hook)
 {
@@ -320,7 +313,11 @@ MODULE_ALIAS("act_xt");
 
 static int __init ipt_init_module(void)
 {
-	int ret1, ret2;
+	int ret1, ret2, err;
+	err = tcf_hashinfo_init(&ipt_hash_info, IPT_TAB_MASK+1);
+	if (err)
+		return err;
+
 	ret1 = tcf_register_action(&act_xt_ops);
 	if (ret1 < 0)
 		printk("Failed to load xt action\n");
@@ -328,9 +325,10 @@ static int __init ipt_init_module(void)
 	if (ret2 < 0)
 		printk("Failed to load ipt action\n");
 
-	if (ret1 < 0 && ret2 < 0)
+	if (ret1 < 0 && ret2 < 0) {
+		tcf_hashinfo_destroy(&ipt_hash_info);
 		return ret1;
-	else
+	} else
 		return 0;
 }
 
@@ -338,6 +336,7 @@ 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 c26bfe5..d92cbab 100644
--- a/net/sched/act_mirred.c
+++ b/net/sched/act_mirred.c
@@ -30,16 +30,9 @@
 #include <linux/if_arp.h>
 
 #define MIRRED_TAB_MASK     7
-static struct tcf_common *tcf_mirred_ht[MIRRED_TAB_MASK + 1];
 static u32 mirred_idx_gen;
-static DEFINE_RWLOCK(mirred_lock);
 static LIST_HEAD(mirred_list);
-
-static struct tcf_hashinfo mirred_hash_info = {
-	.htab	=	tcf_mirred_ht,
-	.hmask	=	MIRRED_TAB_MASK,
-	.lock	=	&mirred_lock,
-};
+static struct tcf_hashinfo mirred_hash_info;
 
 static int tcf_mirred_release(struct tcf_mirred *m, int bind)
 {
@@ -260,7 +253,6 @@ static struct notifier_block mirred_device_notifier = {
 	.notifier_call = mirred_device_event,
 };
 
-
 static struct tc_action_ops act_mirred_ops = {
 	.kind		=	"mirred",
 	.hinfo		=	&mirred_hash_info,
@@ -283,6 +275,11 @@ static int __init mirred_init_module(void)
 	if (err)
 		return err;
 
+	err = tcf_hashinfo_init(&mirred_hash_info, MIRRED_TAB_MASK+1);
+	if (err) {
+		unregister_netdevice_notifier(&mirred_device_notifier);
+		return err;
+	}
 	pr_info("Mirror/redirect action on\n");
 	return tcf_register_action(&act_mirred_ops);
 }
@@ -290,6 +287,7 @@ static int __init mirred_init_module(void)
 static void __exit mirred_cleanup_module(void)
 {
 	unregister_netdevice_notifier(&mirred_device_notifier);
+	tcf_hashinfo_destroy(&mirred_hash_info);
 	tcf_unregister_action(&act_mirred_ops);
 }
 
diff --git a/net/sched/act_nat.c b/net/sched/act_nat.c
index 6a15ace..409fe71 100644
--- a/net/sched/act_nat.c
+++ b/net/sched/act_nat.c
@@ -30,15 +30,9 @@
 
 
 #define NAT_TAB_MASK	15
-static struct tcf_common *tcf_nat_ht[NAT_TAB_MASK + 1];
 static u32 nat_idx_gen;
-static DEFINE_RWLOCK(nat_lock);
 
-static struct tcf_hashinfo nat_hash_info = {
-	.htab	=	tcf_nat_ht,
-	.hmask	=	NAT_TAB_MASK,
-	.lock	=	&nat_lock,
-};
+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) },
@@ -316,12 +310,16 @@ MODULE_LICENSE("GPL");
 
 static int __init nat_init_module(void)
 {
+	int err = tcf_hashinfo_init(&nat_hash_info, NAT_TAB_MASK+1);
+	if (err)
+		return err;
 	return tcf_register_action(&act_nat_ops);
 }
 
 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 03b6767..aa5347c 100644
--- a/net/sched/act_pedit.c
+++ b/net/sched/act_pedit.c
@@ -24,15 +24,9 @@
 #include <net/tc_act/tc_pedit.h>
 
 #define PEDIT_TAB_MASK	15
-static struct tcf_common *tcf_pedit_ht[PEDIT_TAB_MASK + 1];
 static u32 pedit_idx_gen;
-static DEFINE_RWLOCK(pedit_lock);
 
-static struct tcf_hashinfo pedit_hash_info = {
-	.htab	=	tcf_pedit_ht,
-	.hmask	=	PEDIT_TAB_MASK,
-	.lock	=	&pedit_lock,
-};
+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) },
@@ -252,11 +246,15 @@ MODULE_LICENSE("GPL");
 
 static int __init pedit_init_module(void)
 {
+	int err = tcf_hashinfo_init(&pedit_hash_info, PEDIT_TAB_MASK+1);
+	if (err)
+		return err;
 	return tcf_register_action(&act_pedit_ops);
 }
 
 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 16a62c3..f201576 100644
--- a/net/sched/act_police.c
+++ b/net/sched/act_police.c
@@ -41,15 +41,8 @@ struct tcf_police {
 	container_of(pc, struct tcf_police, common)
 
 #define POL_TAB_MASK     15
-static struct tcf_common *tcf_police_ht[POL_TAB_MASK + 1];
 static u32 police_idx_gen;
-static DEFINE_RWLOCK(police_lock);
-
-static struct tcf_hashinfo police_hash_info = {
-	.htab	=	tcf_police_ht,
-	.hmask	=	POL_TAB_MASK,
-	.lock	=	&police_lock,
-};
+static struct tcf_hashinfo police_hash_info;
 
 /* old policer structure from before tc actions */
 struct tc_police_compat {
@@ -71,12 +64,12 @@ static int tcf_act_police_walker(struct sk_buff *skb, struct netlink_callback *c
 	int err = 0, index = -1, i = 0, s_i = 0, n_i = 0;
 	struct nlattr *nest;
 
-	read_lock_bh(&police_lock);
+	read_lock_bh(&police_hash_info.lock);
 
 	s_i = cb->args[0];
 
 	for (i = 0; i < (POL_TAB_MASK + 1); i++) {
-		p = tcf_police_ht[tcf_hash(i, POL_TAB_MASK)];
+		p = police_hash_info.htab[tcf_hash(i, POL_TAB_MASK)];
 
 		for (; p; p = p->tcfc_next) {
 			index++;
@@ -101,7 +94,7 @@ static int tcf_act_police_walker(struct sk_buff *skb, struct netlink_callback *c
 		}
 	}
 done:
-	read_unlock_bh(&police_lock);
+	read_unlock_bh(&police_hash_info.lock);
 	if (n_i)
 		cb->args[0] += n_i;
 	return n_i;
@@ -116,11 +109,11 @@ static void tcf_police_destroy(struct tcf_police *p)
 	unsigned int h = tcf_hash(p->tcf_index, POL_TAB_MASK);
 	struct tcf_common **p1p;
 
-	for (p1p = &tcf_police_ht[h]; *p1p; p1p = &(*p1p)->tcfc_next) {
+	for (p1p = &police_hash_info.htab[h]; *p1p; p1p = &(*p1p)->tcfc_next) {
 		if (*p1p == &p->common) {
-			write_lock_bh(&police_lock);
+			write_lock_bh(&police_hash_info.lock);
 			*p1p = p->tcf_next;
-			write_unlock_bh(&police_lock);
+			write_unlock_bh(&police_hash_info.lock);
 			gen_kill_estimator(&p->tcf_bstats,
 					   &p->tcf_rate_est);
 			/*
@@ -266,10 +259,10 @@ override:
 	police->tcf_index = parm->index ? parm->index :
 		tcf_hash_new_index(&police_idx_gen, &police_hash_info);
 	h = tcf_hash(police->tcf_index, POL_TAB_MASK);
-	write_lock_bh(&police_lock);
-	police->tcf_next = tcf_police_ht[h];
-	tcf_police_ht[h] = &police->common;
-	write_unlock_bh(&police_lock);
+	write_lock_bh(&police_hash_info.lock);
+	police->tcf_next = police_hash_info.htab[h];
+	police_hash_info.htab[h] = &police->common;
+	write_unlock_bh(&police_hash_info.lock);
 
 	a->priv = police;
 	return ret;
@@ -414,12 +407,19 @@ static struct tc_action_ops act_police_ops = {
 static int __init
 police_init_module(void)
 {
-	return tcf_register_action(&act_police_ops);
+	int err = tcf_hashinfo_init(&police_hash_info, POL_TAB_MASK+1);
+	if (err)
+		return err;
+	err = tcf_register_action(&act_police_ops);
+	if (err)
+		tcf_hashinfo_destroy(&police_hash_info);
+	return err;
 }
 
 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 31157d3..2d7a0eb 100644
--- a/net/sched/act_simple.c
+++ b/net/sched/act_simple.c
@@ -25,15 +25,8 @@
 #include <net/tc_act/tc_defact.h>
 
 #define SIMP_TAB_MASK     7
-static struct tcf_common *tcf_simp_ht[SIMP_TAB_MASK + 1];
 static u32 simp_idx_gen;
-static DEFINE_RWLOCK(simp_lock);
-
-static struct tcf_hashinfo simp_hash_info = {
-	.htab	=	tcf_simp_ht,
-	.hmask	=	SIMP_TAB_MASK,
-	.lock	=	&simp_lock,
-};
+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,
@@ -209,14 +202,23 @@ MODULE_LICENSE("GPL");
 
 static int __init simp_init_module(void)
 {
-	int ret = tcf_register_action(&act_simp_ops);
+	int err, ret;
+	err = tcf_hashinfo_init(&simp_hash_info, SIMP_TAB_MASK+1);
+	if (err)
+		return err;
+
+	ret = tcf_register_action(&act_simp_ops);
 	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 cf20add..90ed04a 100644
--- a/net/sched/act_skbedit.c
+++ b/net/sched/act_skbedit.c
@@ -28,15 +28,8 @@
 #include <net/tc_act/tc_skbedit.h>
 
 #define SKBEDIT_TAB_MASK     15
-static struct tcf_common *tcf_skbedit_ht[SKBEDIT_TAB_MASK + 1];
 static u32 skbedit_idx_gen;
-static DEFINE_RWLOCK(skbedit_lock);
-
-static struct tcf_hashinfo skbedit_hash_info = {
-	.htab	=	tcf_skbedit_ht,
-	.hmask	=	SKBEDIT_TAB_MASK,
-	.lock	=	&skbedit_lock,
-};
+static struct tcf_hashinfo skbedit_hash_info;
 
 static int tcf_skbedit(struct sk_buff *skb, const struct tc_action *a,
 		       struct tcf_result *res)
@@ -210,11 +203,15 @@ MODULE_LICENSE("GPL");
 
 static int __init skbedit_init_module(void)
 {
+	int err = tcf_hashinfo_init(&skbedit_hash_info, SKBEDIT_TAB_MASK+1);
+	if (err)
+		return err;
 	return tcf_register_action(&act_skbedit_ops);
 }
 
 static void __exit skbedit_cleanup_module(void)
 {
+	tcf_hashinfo_destroy(&skbedit_hash_info);
 	tcf_unregister_action(&act_skbedit_ops);
 }
 
-- 
1.8.1.4

Re: [PATCH net-next v4 5/8] net_sched: init struct tcf_hashinfo at register time

[Jamal Hadi Salim]

On 12/15/13 23:15, Cong Wang wrote:
> It looks weird to store the lock out of the struct but
> still points to a static variable. Just move them into the struct.
>
> 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 - thanks.


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

cheers,
jamal

[PATCH net-next v4 6/8] net_sched: convert tcf_hashinfo to hlist and use spinlock

[Cong Wang]

So that we don't need to play with singly linked list,
and since the code is not on hot path, we can use spinlock
instead of rwlock.

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 +++++++-----
 net/sched/act_api.c    | 69 ++++++++++++++++++++++----------------------------
 net/sched/act_police.c | 45 +++++++++++++-------------------
 3 files changed, 58 insertions(+), 72 deletions(-)

diff --git a/include/net/act_api.h b/include/net/act_api.h
index 2b5ec5a..22418d1 100644
--- a/include/net/act_api.h
+++ b/include/net/act_api.h
@@ -9,7 +9,7 @@
 #include <net/pkt_sched.h>
 
 struct tcf_common {
-	struct tcf_common		*tcfc_next;
+	struct hlist_node		tcfc_head;
 	u32				tcfc_index;
 	int				tcfc_refcnt;
 	int				tcfc_bindcnt;
@@ -22,7 +22,7 @@ struct tcf_common {
 	spinlock_t			tcfc_lock;
 	struct rcu_head			tcfc_rcu;
 };
-#define tcf_next	common.tcfc_next
+#define tcf_head	common.tcfc_head
 #define tcf_index	common.tcfc_index
 #define tcf_refcnt	common.tcfc_refcnt
 #define tcf_bindcnt	common.tcfc_bindcnt
@@ -36,9 +36,9 @@ struct tcf_common {
 #define tcf_rcu		common.tcfc_rcu
 
 struct tcf_hashinfo {
-	struct tcf_common	**htab;
+	struct hlist_head	*htab;
 	unsigned int		hmask;
-	rwlock_t		lock;
+	spinlock_t		lock;
 };
 
 static inline unsigned int tcf_hash(u32 index, unsigned int hmask)
@@ -48,12 +48,16 @@ static inline unsigned int tcf_hash(u32 index, unsigned int hmask)
 
 static inline int tcf_hashinfo_init(struct tcf_hashinfo *hf, unsigned int mask)
 {
-	rwlock_init(&hf->lock);
+	int i;
+
+	spin_lock_init(&hf->lock);
 	hf->hmask = mask;
-	hf->htab = kzalloc((mask + 1) * sizeof(struct tcf_common *),
+	hf->htab = kzalloc((mask + 1) * sizeof(struct hlist_head),
 			   GFP_KERNEL);
 	if (!hf->htab)
 		return -ENOMEM;
+	for (i = 0; i < mask + 1; i++)
+		INIT_HLIST_HEAD(&hf->htab[i]);
 	return 0;
 }
 
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 125673d..dc457c9 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -29,25 +29,16 @@
 
 void tcf_hash_destroy(struct tcf_common *p, struct tcf_hashinfo *hinfo)
 {
-	unsigned int h = tcf_hash(p->tcfc_index, hinfo->hmask);
-	struct tcf_common **p1p;
-
-	for (p1p = &hinfo->htab[h]; *p1p; p1p = &(*p1p)->tcfc_next) {
-		if (*p1p == p) {
-			write_lock_bh(&hinfo->lock);
-			*p1p = p->tcfc_next;
-			write_unlock_bh(&hinfo->lock);
-			gen_kill_estimator(&p->tcfc_bstats,
-					   &p->tcfc_rate_est);
-			/*
-			 * gen_estimator est_timer() might access p->tcfc_lock
-			 * or bstats, wait a RCU grace period before freeing p
-			 */
-			kfree_rcu(p, tcfc_rcu);
-			return;
-		}
-	}
-	WARN_ON(1);
+	spin_lock_bh(&hinfo->lock);
+	hlist_del(&p->tcfc_head);
+	spin_unlock_bh(&hinfo->lock);
+	gen_kill_estimator(&p->tcfc_bstats,
+			   &p->tcfc_rate_est);
+	/*
+	 * gen_estimator est_timer() might access p->tcfc_lock
+	 * or bstats, wait a RCU grace period before freeing p
+	 */
+	kfree_rcu(p, tcfc_rcu);
 }
 EXPORT_SYMBOL(tcf_hash_destroy);
 
@@ -73,18 +64,19 @@ 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 hlist_head *head;
 	struct tcf_common *p;
 	int err = 0, index = -1, i = 0, s_i = 0, n_i = 0;
 	struct nlattr *nest;
 
-	read_lock_bh(&hinfo->lock);
+	spin_lock_bh(&hinfo->lock);
 
 	s_i = cb->args[0];
 
 	for (i = 0; i < (hinfo->hmask + 1); i++) {
-		p = hinfo->htab[tcf_hash(i, hinfo->hmask)];
+		head = &hinfo->htab[tcf_hash(i, hinfo->hmask)];
 
-		for (; p; p = p->tcfc_next) {
+		hlist_for_each_entry_rcu(p, head, tcfc_head) {
 			index++;
 			if (index < s_i)
 				continue;
@@ -107,7 +99,7 @@ static int tcf_dump_walker(struct sk_buff *skb, struct netlink_callback *cb,
 		}
 	}
 done:
-	read_unlock_bh(&hinfo->lock);
+	spin_unlock_bh(&hinfo->lock);
 	if (n_i)
 		cb->args[0] += n_i;
 	return n_i;
@@ -120,7 +112,9 @@ nla_put_failure:
 static int tcf_del_walker(struct sk_buff *skb, struct tc_action *a,
 			  struct tcf_hashinfo *hinfo)
 {
-	struct tcf_common *p, *s_p;
+	struct hlist_head *head;
+	struct hlist_node *n;
+	struct tcf_common *p;
 	struct nlattr *nest;
 	int i = 0, n_i = 0;
 
@@ -130,14 +124,11 @@ static int tcf_del_walker(struct sk_buff *skb, struct tc_action *a,
 	if (nla_put_string(skb, TCA_KIND, a->ops->kind))
 		goto nla_put_failure;
 	for (i = 0; i < (hinfo->hmask + 1); i++) {
-		p = hinfo->htab[tcf_hash(i, hinfo->hmask)];
-
-		while (p != NULL) {
-			s_p = p->tcfc_next;
+		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))
 				module_put(a->ops->owner);
 			n_i++;
-			p = s_p;
 		}
 	}
 	if (nla_put_u32(skb, TCA_FCNT, n_i))
@@ -168,15 +159,15 @@ EXPORT_SYMBOL(tcf_generic_walker);
 
 struct tcf_common *tcf_hash_lookup(u32 index, struct tcf_hashinfo *hinfo)
 {
-	struct tcf_common *p;
+	struct tcf_common *p = NULL;
+	struct hlist_head *head;
 
-	read_lock_bh(&hinfo->lock);
-	for (p = hinfo->htab[tcf_hash(index, hinfo->hmask)]; p;
-	     p = p->tcfc_next) {
+	spin_lock_bh(&hinfo->lock);
+	head = &hinfo->htab[tcf_hash(index, hinfo->hmask)];
+	hlist_for_each_entry_rcu(p, head, tcfc_head)
 		if (p->tcfc_index == index)
 			break;
-	}
-	read_unlock_bh(&hinfo->lock);
+	spin_unlock_bh(&hinfo->lock);
 
 	return p;
 }
@@ -236,6 +227,7 @@ struct tcf_common *tcf_hash_create(u32 index, struct nlattr *est,
 		p->tcfc_bindcnt = 1;
 
 	spin_lock_init(&p->tcfc_lock);
+	INIT_HLIST_NODE(&p->tcfc_head);
 	p->tcfc_index = index ? index : tcf_hash_new_index(idx_gen, hinfo);
 	p->tcfc_tm.install = jiffies;
 	p->tcfc_tm.lastuse = jiffies;
@@ -257,10 +249,9 @@ void tcf_hash_insert(struct tcf_common *p, struct tcf_hashinfo *hinfo)
 {
 	unsigned int h = tcf_hash(p->tcfc_index, hinfo->hmask);
 
-	write_lock_bh(&hinfo->lock);
-	p->tcfc_next = hinfo->htab[h];
-	hinfo->htab[h] = p;
-	write_unlock_bh(&hinfo->lock);
+	spin_lock_bh(&hinfo->lock);
+	hlist_add_head(&p->tcfc_head, &hinfo->htab[h]);
+	spin_unlock_bh(&hinfo->lock);
 }
 EXPORT_SYMBOL(tcf_hash_insert);
 
diff --git a/net/sched/act_police.c b/net/sched/act_police.c
index f201576..0cc305e 100644
--- a/net/sched/act_police.c
+++ b/net/sched/act_police.c
@@ -60,18 +60,19 @@ 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 hlist_head *head;
 	struct tcf_common *p;
 	int err = 0, index = -1, i = 0, s_i = 0, n_i = 0;
 	struct nlattr *nest;
 
-	read_lock_bh(&police_hash_info.lock);
+	spin_lock_bh(&police_hash_info.lock);
 
 	s_i = cb->args[0];
 
 	for (i = 0; i < (POL_TAB_MASK + 1); i++) {
-		p = police_hash_info.htab[tcf_hash(i, POL_TAB_MASK)];
+		head = &police_hash_info.htab[tcf_hash(i, POL_TAB_MASK)];
 
-		for (; p; p = p->tcfc_next) {
+		hlist_for_each_entry_rcu(p, head, tcfc_head) {
 			index++;
 			if (index < s_i)
 				continue;
@@ -94,7 +95,7 @@ static int tcf_act_police_walker(struct sk_buff *skb, struct netlink_callback *c
 		}
 	}
 done:
-	read_unlock_bh(&police_hash_info.lock);
+	spin_unlock_bh(&police_hash_info.lock);
 	if (n_i)
 		cb->args[0] += n_i;
 	return n_i;
@@ -106,25 +107,16 @@ nla_put_failure:
 
 static void tcf_police_destroy(struct tcf_police *p)
 {
-	unsigned int h = tcf_hash(p->tcf_index, POL_TAB_MASK);
-	struct tcf_common **p1p;
-
-	for (p1p = &police_hash_info.htab[h]; *p1p; p1p = &(*p1p)->tcfc_next) {
-		if (*p1p == &p->common) {
-			write_lock_bh(&police_hash_info.lock);
-			*p1p = p->tcf_next;
-			write_unlock_bh(&police_hash_info.lock);
-			gen_kill_estimator(&p->tcf_bstats,
-					   &p->tcf_rate_est);
-			/*
-			 * gen_estimator est_timer() might access p->tcf_lock
-			 * or bstats, wait a RCU grace period before freeing p
-			 */
-			kfree_rcu(p, tcf_rcu);
-			return;
-		}
-	}
-	WARN_ON(1);
+	spin_lock_bh(&police_hash_info.lock);
+	hlist_del(&p->tcf_head);
+	spin_unlock_bh(&police_hash_info.lock);
+	gen_kill_estimator(&p->tcf_bstats,
+			   &p->tcf_rate_est);
+	/*
+	 * gen_estimator est_timer() might access p->tcf_lock
+	 * or bstats, wait a RCU grace period before freeing p
+	 */
+	kfree_rcu(p, tcf_rcu);
 }
 
 static const struct nla_policy police_policy[TCA_POLICE_MAX + 1] = {
@@ -259,10 +251,9 @@ override:
 	police->tcf_index = parm->index ? parm->index :
 		tcf_hash_new_index(&police_idx_gen, &police_hash_info);
 	h = tcf_hash(police->tcf_index, POL_TAB_MASK);
-	write_lock_bh(&police_hash_info.lock);
-	police->tcf_next = police_hash_info.htab[h];
-	police_hash_info.htab[h] = &police->common;
-	write_unlock_bh(&police_hash_info.lock);
+	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);
 
 	a->priv = police;
 	return ret;
-- 
1.8.1.4

Re: [PATCH net-next v4 6/8] net_sched: convert tcf_hashinfo to hlist and use spinlock

[Jamal Hadi Salim]

On 12/15/13 23:15, Cong Wang wrote:
> So that we don't need to play with singly linked list,
> and since the code is not on hot path, we can use spinlock
> instead of rwlock.
>
> Cc: Jamal Hadi Salim <jhs@mojatatu.com>
> Cc: David S. Miller <davem@davemloft.net>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>

nice.

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

cheers,
jamal

[PATCH net-next v4 7/8] net_sched: convert tc_action_ops to use struct list_head

[Cong Wang]

We don't need to maintain our own singly linked list code.

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   | 20 +++++++++-----------
 2 files changed, 10 insertions(+), 12 deletions(-)

diff --git a/include/net/act_api.h b/include/net/act_api.h
index 22418d1..77d5d81 100644
--- a/include/net/act_api.h
+++ b/include/net/act_api.h
@@ -85,7 +85,7 @@ struct tc_action {
 
 #define TCA_CAP_NONE 0
 struct tc_action_ops {
-	struct tc_action_ops *next;
+	struct list_head head;
 	struct tcf_hashinfo *hinfo;
 	char    kind[IFNAMSIZ];
 	__u32   type; /* TBD to match kind */
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index dc457c9..8114fef 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -255,12 +255,12 @@ void tcf_hash_insert(struct tcf_common *p, struct tcf_hashinfo *hinfo)
 }
 EXPORT_SYMBOL(tcf_hash_insert);
 
-static struct tc_action_ops *act_base = NULL;
+static LIST_HEAD(act_base);
 static DEFINE_RWLOCK(act_mod_lock);
 
 int tcf_register_action(struct tc_action_ops *act)
 {
-	struct tc_action_ops *a, **ap;
+	struct tc_action_ops *a;
 
 	/* Must supply act, dump, cleanup and init */
 	if (!act->act || !act->dump || !act->cleanup || !act->init)
@@ -273,14 +273,13 @@ int tcf_register_action(struct tc_action_ops *act)
 		act->walk = tcf_generic_walker;
 
 	write_lock(&act_mod_lock);
-	for (ap = &act_base; (a = *ap) != NULL; ap = &a->next) {
+	list_for_each_entry(a, &act_base, head) {
 		if (act->type == a->type || (strcmp(act->kind, a->kind) == 0)) {
 			write_unlock(&act_mod_lock);
 			return -EEXIST;
 		}
 	}
-	act->next = NULL;
-	*ap = act;
+	list_add_tail(&act->head, &act_base);
 	write_unlock(&act_mod_lock);
 	return 0;
 }
@@ -288,16 +287,15 @@ EXPORT_SYMBOL(tcf_register_action);
 
 int tcf_unregister_action(struct tc_action_ops *act)
 {
-	struct tc_action_ops *a, **ap;
+	struct tc_action_ops *a;
 	int err = -ENOENT;
 
 	write_lock(&act_mod_lock);
-	for (ap = &act_base; (a = *ap) != NULL; ap = &a->next)
+	list_for_each_entry(a, &act_base, head)
 		if (a == act)
 			break;
 	if (a) {
-		*ap = a->next;
-		a->next = NULL;
+		list_del(&act->head);
 		err = 0;
 	}
 	write_unlock(&act_mod_lock);
@@ -312,7 +310,7 @@ static struct tc_action_ops *tc_lookup_action_n(char *kind)
 
 	if (kind) {
 		read_lock(&act_mod_lock);
-		for (a = act_base; a; a = a->next) {
+		list_for_each_entry(a, &act_base, head) {
 			if (strcmp(kind, a->kind) == 0) {
 				if (!try_module_get(a->owner)) {
 					read_unlock(&act_mod_lock);
@@ -333,7 +331,7 @@ static struct tc_action_ops *tc_lookup_action(struct nlattr *kind)
 
 	if (kind) {
 		read_lock(&act_mod_lock);
-		for (a = act_base; a; a = a->next) {
+		list_for_each_entry(a, &act_base, head) {
 			if (nla_strcmp(kind, a->kind) == 0) {
 				if (!try_module_get(a->owner)) {
 					read_unlock(&act_mod_lock);
-- 
1.8.1.4

Re: [PATCH net-next v4 7/8] net_sched: convert tc_action_ops to use struct list_head

[Jamal Hadi Salim]

On 12/15/13 23:15, Cong Wang wrote:
> We don't need to maintain our own singly linked list code.
>
> Cc: Jamal Hadi Salim <jhs@mojatatu.com>
> Cc: David S. Miller <davem@davemloft.net>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
> ---

nice!

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

cheers,
jamal

[PATCH net-next v4 8/8] net_sched: convert tcf_proto_ops to use struct list_head

[Cong Wang]

We don't need to maintain our own singly linked list code.

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/sch_generic.h |  2 +-
 net/sched/cls_api.c       | 18 ++++++++----------
 2 files changed, 9 insertions(+), 11 deletions(-)

diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index d0a6321..013d96d 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -185,7 +185,7 @@ struct tcf_result {
 };
 
 struct tcf_proto_ops {
-	struct tcf_proto_ops	*next;
+	struct list_head	head;
 	char			kind[IFNAMSIZ];
 
 	int			(*classify)(struct sk_buff *,
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 028c980..6b085cf 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -31,8 +31,7 @@
 #include <net/pkt_cls.h>
 
 /* The list of all installed classifier types */
-
-static struct tcf_proto_ops *tcf_proto_base __read_mostly;
+static LIST_HEAD(tcf_proto_base);
 
 /* Protects list of registered TC modules. It is pure SMP lock. */
 static DEFINE_RWLOCK(cls_mod_lock);
@@ -45,7 +44,7 @@ static const struct tcf_proto_ops *tcf_proto_lookup_ops(struct nlattr *kind)
 
 	if (kind) {
 		read_lock(&cls_mod_lock);
-		for (t = tcf_proto_base; t; t = t->next) {
+		list_for_each_entry(t, &tcf_proto_base, head) {
 			if (nla_strcmp(kind, t->kind) == 0) {
 				if (!try_module_get(t->owner))
 					t = NULL;
@@ -61,16 +60,15 @@ static const struct tcf_proto_ops *tcf_proto_lookup_ops(struct nlattr *kind)
 
 int register_tcf_proto_ops(struct tcf_proto_ops *ops)
 {
-	struct tcf_proto_ops *t, **tp;
+	struct tcf_proto_ops *t;
 	int rc = -EEXIST;
 
 	write_lock(&cls_mod_lock);
-	for (tp = &tcf_proto_base; (t = *tp) != NULL; tp = &t->next)
+	list_for_each_entry(t, &tcf_proto_base, head)
 		if (!strcmp(ops->kind, t->kind))
 			goto out;
 
-	ops->next = NULL;
-	*tp = ops;
+	list_add_tail(&ops->head, &tcf_proto_base);
 	rc = 0;
 out:
 	write_unlock(&cls_mod_lock);
@@ -80,17 +78,17 @@ EXPORT_SYMBOL(register_tcf_proto_ops);
 
 int unregister_tcf_proto_ops(struct tcf_proto_ops *ops)
 {
-	struct tcf_proto_ops *t, **tp;
+	struct tcf_proto_ops *t;
 	int rc = -ENOENT;
 
 	write_lock(&cls_mod_lock);
-	for (tp = &tcf_proto_base; (t = *tp) != NULL; tp = &t->next)
+	list_for_each_entry(t, &tcf_proto_base, head)
 		if (t == ops)
 			break;
 
 	if (!t)
 		goto out;
-	*tp = t->next;
+	list_del(&t->head);
 	rc = 0;
 out:
 	write_unlock(&cls_mod_lock);
-- 
1.8.1.4

Re: [PATCH net-next v4 8/8] net_sched: convert tcf_proto_ops to use struct list_head

[Jamal Hadi Salim]

On 12/15/13 23:15, Cong Wang wrote:
> We don't need to maintain our own singly linked list code.
>
> 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/sch_generic.h |  2 +-
>   net/sched/cls_api.c       | 18 ++++++++----------
>   2 files changed, 9 insertions(+), 11 deletions(-)
>

And another nice improvement. Thanks Cong

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

cheers,
jamal

Re: [PATCH net-next v4 0/8] net_sched: some cleanup and improvments

[Jamal Hadi Salim]

On 12/15/13 23:15, Cong Wang wrote:
> Here are some cleanup and improvements for tc filters
> and tc actions.
>

Thanks.
I would like to run some tests I have and look closely -
please give me some time; unfortunately I dont right away (yesterday was
good for me); will do this week.


cheers,
jamal

Re: [PATCH net-next v4 0/8] net_sched: some cleanup and improvments

[David Miller]

From: Jamal Hadi Salim <jhs@mojatatu.com>
Date: Mon, 16 Dec 2013 07:45:24 -0500

> I would like to run some tests I have and look closely - please give
> me some time; unfortunately I dont right away (yesterday was good
> for me); will do this week.

Please do this at your earliest convenience so Cong's patches don't
just rot in patchwork.  When patches are held up like this it makes
extra work for me as I analyze my patchwork todo list each and every
day.

Thank you.

Re: [PATCH net-next v4 0/8] net_sched: some cleanup and improvments

[Jamal Hadi Salim]

On 12/17/13 16:30, David Miller wrote:

> Please do this at your earliest convenience so Cong's patches don't
> just rot in patchwork.  When patches are held up like this it makes
> extra work for me as I analyze my patchwork todo list each and every
> day.

Will do. I was hoping for the weekend but the update didnt show
up until yesterday.
In the minimal i will do the patch review today and if nothing stands
out you can take in the patches.
For large changes like these Dave - you gotta let me do my testing. I am
OCD. Maybe i will clean up the tests and make them available somewhere
so i dont slow people down.

cheers,
jamal

Re: [PATCH net-next v4 0/8] net_sched: some cleanup and improvments

[Jamal Hadi Salim]

On 12/15/13 23:15, Cong Wang wrote:
> Here are some cleanup and improvements for tc filters
> and tc actions.
>

Hi Cong,
I think these patches are a long overdue useful improvement. Thanks for
the effort. I will comment on each individually.

cheers,
jamal

PS: checkpatch was whining to me - but thats probably because of
the way thunderbird saved things for me. Double check just in case.

Re: [PATCH net-next v4 0/8] net_sched: some cleanup and improvments

[David Miller]

From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Sun, 15 Dec 2013 20:15:03 -0800

> Here are some cleanup and improvements for tc filters
> and tc actions.

All except the mirred patch applied to net-next, thanks.

Re: [PATCH net-next v4 0/8] net_sched: some cleanup and improvments

[Cong Wang]

On Wed, Dec 18, 2013 at 10:51 AM, David Miller <davem@davemloft.net> wrote:
> From: Cong Wang <xiyou.wangcong@gmail.com>
> Date: Sun, 15 Dec 2013 20:15:03 -0800
>
>> Here are some cleanup and improvements for tc filters
>> and tc actions.
>
> All except the mirred patch applied to net-next, thanks.

Thanks, then I will rework on the mirred patch.