[PATCH net-next v3 0/6] net_sched: some cleanup and improvments

[PATCH net-next v3 0/6] net_sched: some cleanup and improvments

[Cong Wang]

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

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 (6):
  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 rcu

 include/net/act_api.h          |  41 +++++++---
 include/net/pkt_cls.h          |  37 +++++----
 include/net/tc_act/tc_mirred.h |   4 +-
 net/sched/act_api.c            | 180 ++++++++++++++++++-----------------------
 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         |  65 +++++++--------
 net/sched/act_simple.c         |  20 ++---
 net/sched/act_skbedit.c        |  13 ++-
 net/sched/cls_api.c            |  80 +++++++++---------
 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 ++-
 23 files changed, 304 insertions(+), 360 deletions(-)

-- 
1.8.1.4

[PATCH net-next v3 1/6] 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 v3 1/6] net_sched: remove get_stats from tc_action_ops

[David Laight]

> From: Cong Wang
> 
> It is not used.
...
> --- 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);

Deleting a field out of the middle of a list of function pointers
seems dangerous.

	David

Re: [PATCH net-next v3 1/6] net_sched: remove get_stats from tc_action_ops

[Cong Wang]

On Fri, Dec 13, 2013 at 2:21 AM, David Laight <David.Laight@aculab.com> wrote:
>> From: Cong Wang
>>
>> It is not used.
> ...
>> --- 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);
>
> Deleting a field out of the middle of a list of function pointers
> seems dangerous.

Why? Keep in mind that we don't need to worry about any
out-of-tree modules. :)

Re: [PATCH net-next v3 1/6] net_sched: remove get_stats from tc_action_ops

[Jamal Hadi Salim]

On 12/13/13 13:33, Cong Wang wrote:

> Why? Keep in mind that we don't need to worry about any
> out-of-tree modules. :)

This is an API that is used to export additional stats that
are not generic.

cheers,
jamal

Re: [PATCH net-next v3 1/6] net_sched: remove get_stats from tc_action_ops

[Cong Wang]

On Fri, Dec 13, 2013 at 1:27 PM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> On 12/13/13 13:33, Cong Wang wrote:
>
>> Why? Keep in mind that we don't need to worry about any
>> out-of-tree modules. :)
>
>
> This is an API that is used to export additional stats that
> are not generic.
>

But no in-tree module implements it....

Re: [PATCH net-next v3 1/6] net_sched: remove get_stats from tc_action_ops

[Jamal Hadi Salim]

On 12/13/13 16:34, Cong Wang wrote:

> But no in-tree module implements it....

So? It is an API that is in there and expected by user space.

cheers,
jamal

Re: [PATCH net-next v3 1/6] net_sched: remove get_stats from tc_action_ops

[Cong Wang]

On Fri, Dec 13, 2013 at 1:40 PM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> On 12/13/13 16:34, Cong Wang wrote:
>
>> But no in-tree module implements it....
>
>
> So? It is an API that is in there and expected by user space.

I must miss something here...

Before this patch:

        if (a->ops != NULL && a->ops->get_stats != NULL)
                if (a->ops->get_stats(skb, a) < 0)
                        goto errout;

since ->get_stats is always NULL,  after we remove it, no behavior
is changed. So, how could this affect user-space?

Re: [PATCH net-next v3 1/6] net_sched: remove get_stats from tc_action_ops

[Jamal Hadi Salim]

On 12/13/13 16:50, Cong Wang wrote:

> I must miss something here...

You are.
This api is for use for additional stats other than the generic
stats. It will export such extra stats to user space. There is nothing
unusual here - if you look at qdiscs and classes you will see equivalent
interfaces.
It has been there for a decade. It has been used in the past and it
may still be in use by other modules in the wild (even if i or you
dont care about them). Please dont break things.

cheers,
jamal

Re: [PATCH net-next v3 1/6] net_sched: remove get_stats from tc_action_ops

[David Miller]

From: Jamal Hadi Salim <jhs@mojatatu.com>
Date: Fri, 13 Dec 2013 17:19:23 -0500

> On 12/13/13 16:50, Cong Wang wrote:
> 
>> I must miss something here...
> 
> You are.
> This api is for use for additional stats other than the generic
> stats. It will export such extra stats to user space. There is nothing
> unusual here - if you look at qdiscs and classes you will see
> equivalent
> interfaces.
> It has been there for a decade. It has been used in the past and it
> may still be in use by other modules in the wild (even if i or you
> dont care about them). Please dont break things.

Jamal, nobody implements this interface, therefore these extra
stats are _NEVER_ provided.

This is the end of the discussion, when there are in-tree users
of this method you can add it back.

Userland will have no idea whether it is there or not, because
there is no way for it to tell one way or another.

Re: [PATCH net-next v3 1/6] net_sched: remove get_stats from tc_action_ops

[Jamal Hadi Salim]

On 12/13/13 18:23, David Miller wrote:

> Jamal, nobody implements this interface, therefore these extra
> stats are _NEVER_ provided.
 >
> This is the end of the discussion, when there are in-tree users
> of this method you can add it back.

Dave, I think that would be a good arguement if the feature was being
provided in a new set of patches today.
I dont see this as any different than removing an something from a UAPI 
header nobody knows any user of.

cheers,
jamal

Re: [PATCH net-next v3 1/6] net_sched: remove get_stats from tc_action_ops

[David Miller]

From: Jamal Hadi Salim <jhs@mojatatu.com>
Date: Fri, 13 Dec 2013 21:59:01 -0500

> On 12/13/13 18:23, David Miller wrote:
> 
>> Jamal, nobody implements this interface, therefore these extra
>> stats are _NEVER_ provided.
>>
>> This is the end of the discussion, when there are in-tree users
>> of this method you can add it back.
> 
> Dave, I think that would be a good arguement if the feature was being
> provided in a new set of patches today.
> I dont see this as any different than removing an something from a
> UAPI header nobody knows any user of.

It is significantly different.

It is an internal kernel interface that was never used.

You cannot argue that, because it might have generated a netlink
attribute had it been implemented, that userland has a dependency upon
it.

You simply can't.

Re: [PATCH net-next v3 1/6] net_sched: remove get_stats from tc_action_ops

[Jamal Hadi Salim]

On 12/14/13 00:15, David Miller wrote:

> It is significantly different.
>
> It is an internal kernel interface that was never used.

It is not an internal API, Dave.
It an external kernel module API that has been visible for many years.
It provides "xstats" semantics - which is something an
action user has high potential use for.
It has been used in the past, just not by current in tree users.

> You cannot argue that, because it might have generated a netlink
> attribute had it been implemented, that userland has a dependency upon
> it.

The arguement is: it does expose some very strategically located
attributes. "xstats" sit between basic stats and other attributes
in the netlink layout (just as they do in qdisc and classes).
Yes, no in tree users exist but this feature has been there for
_a few years now_ and is not getting in the way of anything to
deserve chopping down.

Anyways, your call.

cheers,
jamal

Re: [PATCH net-next v3 1/6] net_sched: remove get_stats from tc_action_ops

[David Miller]

From: Jamal Hadi Salim <jhs@mojatatu.com>
Date: Sat, 14 Dec 2013 07:22:26 -0500

> On 12/14/13 00:15, David Miller wrote:
> 
>> It is significantly different.
>>
>> It is an internal kernel interface that was never used.
> 
> It is not an internal API, Dave.
> It an external kernel module API that has been visible for many years.

There are no stable kernel APIs, I'm sorry if this is news to
you.

Documentation/stable_api_nonsense.txt

Re: [PATCH net-next v3 1/6] net_sched: remove get_stats from tc_action_ops

[David Miller]

From: Jamal Hadi Salim <jhs@mojatatu.com>
Date: Fri, 13 Dec 2013 16:27:34 -0500

> On 12/13/13 13:33, Cong Wang wrote:
> 
>> Why? Keep in mind that we don't need to worry about any
>> out-of-tree modules. :)
> 
> This is an API that is used to export additional stats that
> are not generic.

Nobody implements it.

When someone does, add it back.

End of discussion.

Re: [PATCH net-next v3 1/6] net_sched: remove get_stats from tc_action_ops

[Jamal Hadi Salim]

So where is you 0/6? ;->
General: The spirit of the patches i find agreeable.
I am against patch 1. Dont just remove ABI/APIs that exist.
Some of these changes are substantial - did you do any testing?
If you havent I can help - but at minimal i will ask you do so.
I will also do more review then with whatever iteration you have around
sunday. I have other patches i meant to  submit - but i will do them on
top of yours.

cheers,
jamal

On 12/13/13 00:47, 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>
> ---
>   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 ||
>

Re: [PATCH net-next v3 1/6] net_sched: remove get_stats from tc_action_ops

[Cong Wang]

On Fri, Dec 13, 2013 at 4:05 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>
> So where is you 0/6? ;->

It is there, just without any CC. :-)

> General: The spirit of the patches i find agreeable.
> I am against patch 1. Dont just remove ABI/APIs that exist.

I don't think we care about kernel module ABI in upstream,
nor we need to care about any out-of-tree module.
Otherwise they are already broken by any change of sk_buff, right?

> Some of these changes are substantial - did you do any testing?
> If you havent I can help - but at minimal i will ask you do so.
> I will also do more review then with whatever iteration you have around
> sunday. I have other patches i meant to  submit - but i will do them on
> top of yours.
>

Yes, but I just have testcase for u32 filter and basic filter with
mirred action. I don't have time to write testcases for other things
yet.

Thanks!

Re: [PATCH net-next v3 1/6] net_sched: remove get_stats from tc_action_ops

[Jamal Hadi Salim]

On 12/13/13 13:38, Cong Wang wrote:

> Yes, but I just have testcase for u32 filter and basic filter with
> mirred action. I don't have time to write testcases for other things
> yet.

Good - at least you are testing. Please fix your patches based on
comments you received and i will do more testing on my side when
your patches are in good shape.

cheers,
jamal

[PATCH net-next v3 2/6] 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   |  15 +++++--
 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, 100 insertions(+), 99 deletions(-)

diff --git a/include/net/act_api.h b/include/net/act_api.h
index 04c6825..ebaf4b5 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 *head, int bind);
+int tcf_action_exec(struct sk_buff *skb, const struct list_head *head,
 		    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..a85264b 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 head;
 #endif
 };
 
@@ -85,12 +86,18 @@ static inline int
 tcf_exts_is_predicative(struct tcf_exts *exts)
 {
 #ifdef CONFIG_NET_CLS_ACT
-	return !!exts->action;
+	return list_empty(&exts->head);
 #else
 	return 0;
 #endif
 }
 
+static inline void tcf_exts_init(struct tcf_exts *exts)
+{
+#ifdef CONFIG_NET_CLS_ACT
+	INIT_LIST_HEAD(&exts->head);
+#endif
+}
 /**
  * tcf_exts_is_available - check if at least one extension is present
  * @exts: tc filter extensions handle
@@ -120,8 +127,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->head))
+		return tcf_action_exec(skb, &exts->head, res);
 #endif
 	return 0;
 }
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 51e28f7..88b5087 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 *head,
 		    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, head, 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 *head, int bind)
 {
-	struct tc_action *a;
+	struct tc_action *a, *tmp;
 
-	for (a = act; a; a = act) {
+	list_for_each_entry_safe(a, tmp, head, 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 *head, 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, head, 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 *head)
 {
 	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, head);
 	}
-	return head;
+	return 0;
 
 err:
-	if (head != NULL)
-		tcf_action_destroy(head, bind);
-	return act;
+	tcf_action_destroy(head, 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 *head, 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, head, 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 *head, 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, head, 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 *head)
 {
-	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, head, 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(head);
 
 	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, &head);
 	}
 
 	if (event == RTM_GETACTION)
-		ret = act_get_notify(net, portid, n, head, event);
+		ret = act_get_notify(net, portid, n, &head, 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, &head, 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(&head, 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(&head);
 	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 *head,
 			  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, head, 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(head);
 	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, &head);
+	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, &head, portid, seq, RTM_NEWACTION, n->nlmsg_flags);
+	cleanup_a(&head);
 done:
 	return ret;
 }
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 8e118af..7e5f296 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->head, TCA_ACT_UNBIND);
+	INIT_LIST_HEAD(&exts->head);
 #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->head);
 		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->head);
 		} 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->head);
+			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->head)) {
+		LIST_HEAD(tmp);
 		tcf_tree_lock(tp);
-		act = dst->action;
-		dst->action = src->action;
+		list_splice_init(&dst->head, &tmp);
+		list_splice(&src->head, &dst->head);
 		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)->head, 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->head)) {
 		/*
 		 * 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->head, 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 v3 2/6] net_sched: act: use standard struct list_head

[Jamal Hadi Salim]

On 12/13/13 00:47, 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.
>

Looks reasonable - but can you find a more usable noun
than "head"?

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/act_api.h   |  12 +++---
>   include/net/pkt_cls.h   |  15 +++++--
>   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, 100 insertions(+), 99 deletions(-)
>
> diff --git a/include/net/act_api.h b/include/net/act_api.h
> index 04c6825..ebaf4b5 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 *head, int bind);
> +int tcf_action_exec(struct sk_buff *skb, const struct list_head *head,
>   		    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..a85264b 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 head;
>   #endif
>   };
>
> @@ -85,12 +86,18 @@ static inline int
>   tcf_exts_is_predicative(struct tcf_exts *exts)
>   {
>   #ifdef CONFIG_NET_CLS_ACT
> -	return !!exts->action;
> +	return list_empty(&exts->head);
>   #else
>   	return 0;
>   #endif
>   }
>
> +static inline void tcf_exts_init(struct tcf_exts *exts)
> +{
> +#ifdef CONFIG_NET_CLS_ACT
> +	INIT_LIST_HEAD(&exts->head);
> +#endif
> +}
>   /**
>    * tcf_exts_is_available - check if at least one extension is present
>    * @exts: tc filter extensions handle
> @@ -120,8 +127,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->head))
> +		return tcf_action_exec(skb, &exts->head, res);
>   #endif
>   	return 0;
>   }
> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
> index 51e28f7..88b5087 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 *head,
>   		    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, head, 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 *head, int bind)
>   {
> -	struct tc_action *a;
> +	struct tc_action *a, *tmp;
>
> -	for (a = act; a; a = act) {
> +	list_for_each_entry_safe(a, tmp, head, 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 *head, 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, head, 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 *head)
>   {
>   	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, head);
>   	}
> -	return head;
> +	return 0;
>
>   err:
> -	if (head != NULL)
> -		tcf_action_destroy(head, bind);
> -	return act;
> +	tcf_action_destroy(head, 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 *head, 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, head, 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 *head, 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, head, 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 *head)
>   {
> -	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, head, 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(head);
>
>   	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, &head);
>   	}
>
>   	if (event == RTM_GETACTION)
> -		ret = act_get_notify(net, portid, n, head, event);
> +		ret = act_get_notify(net, portid, n, &head, 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, &head, 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(&head, 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(&head);
>   	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 *head,
>   			  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, head, 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(head);
>   	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, &head);
> +	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, &head, portid, seq, RTM_NEWACTION, n->nlmsg_flags);
> +	cleanup_a(&head);
>   done:
>   	return ret;
>   }
> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
> index 8e118af..7e5f296 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->head, TCA_ACT_UNBIND);
> +	INIT_LIST_HEAD(&exts->head);
>   #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->head);
>   		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->head);
>   		} 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->head);
> +			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->head)) {
> +		LIST_HEAD(tmp);
>   		tcf_tree_lock(tp);
> -		act = dst->action;
> -		dst->action = src->action;
> +		list_splice_init(&dst->head, &tmp);
> +		list_splice(&src->head, &dst->head);
>   		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)->head, 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->head)) {
>   		/*
>   		 * 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->head, 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]) {
>

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

[Cong Wang]

On Fri, Dec 13, 2013 at 4:08 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> On 12/13/13 00:47, 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.
>>
>
> Looks reasonable - but can you find a more usable noun
> than "head"?
>

My English is not good, I can only find "head" or "list" here,
please suggest other better names?

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

[Jamal Hadi Salim]

On 12/13/13 13:48, Cong Wang wrote:

> My English is not good, I can only find "head" or "list" here,
> please suggest other better names?

One of the ones you replaced was simply "actions"; that would be
better.

cheers,
jamal

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

[Cong Wang]

On Fri, Dec 13, 2013 at 1:29 PM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> On 12/13/13 13:48, Cong Wang wrote:
>
>> My English is not good, I can only find "head" or "list" here,
>> please suggest other better names?
>
>
> One of the ones you replaced was simply "actions"; that would be
> better.

OK, I will change "head" to "actions".

[PATCH net-next v3 3/6] 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 v3 3/6] net_sched: mirred: remove action when the target device is gone

[Jamal Hadi Salim]

On 12/13/13 00:47, 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.
>
> 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;

What has the above cleverness got to do with anything you intended?

cheers,
jamal

RE: [PATCH net-next v3 3/6] net_sched: mirred: remove action when the target device is gone

[David Laight]

> From: Jamal Hadi Salim
> Sent: 13 December 2013 12:08
...
> > -	int			tcfm_ok_push;
> > +	unsigned int		tcfm_ok_push:1;
> > +	unsigned int		tcfm_bind:1;
> 
> What has the above cleverness got to do with anything you intended?

Not even 'cleverness'.
Using byte-sized fields is enough to stop the structure growing.
Bit fields are usually not a good idea at all.

	David

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

[Cong Wang]

On Fri, Dec 13, 2013 at 4:12 AM, David Laight <David.Laight@aculab.com> wrote:
>> From: Jamal Hadi Salim
>> Sent: 13 December 2013 12:08
> ...
>> > -   int                     tcfm_ok_push;
>> > +   unsigned int            tcfm_ok_push:1;
>> > +   unsigned int            tcfm_bind:1;
>>
>> What has the above cleverness got to do with anything you intended?
>
> Not even 'cleverness'.
> Using byte-sized fields is enough to stop the structure growing.
> Bit fields are usually not a good idea at all.
>

They are both boolean, I think it is a waste to use 'int' for both of them.
I am not a fan of bit fields though.

[PATCH net-next v3 4/6] 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   | 34 ++++++++++++++++------------------
 net/sched/cls_api.c     | 32 +++++++++++++-------------------
 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, 75 insertions(+), 120 deletions(-)

diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
index a85264b..367f9d3 100644
--- a/include/net/pkt_cls.h
+++ b/include/net/pkt_cls.h
@@ -65,16 +65,23 @@ struct tcf_exts {
 	__u32	type; /* for backward compat(TCA_OLD_COMPAT) */
 	struct list_head head;
 #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, int action, int police)
+{
+#ifdef CONFIG_NET_CLS_ACT
+	exts->type = 0;
+	INIT_LIST_HEAD(&exts->head);
+#endif
+	exts->action = action;
+	exts->police = police;
+}
+
 /**
  * tcf_exts_is_predicative - check if a predicative extension is present
  * @exts: tc filter extensions handle
@@ -92,12 +99,6 @@ tcf_exts_is_predicative(struct tcf_exts *exts)
 #endif
 }
 
-static inline void tcf_exts_init(struct tcf_exts *exts)
-{
-#ifdef CONFIG_NET_CLS_ACT
-	INIT_LIST_HEAD(&exts->head);
-#endif
-}
 /**
  * tcf_exts_is_available - check if at least one extension is present
  * @exts: tc filter extensions handle
@@ -135,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 7e5f296..e673bb1 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -507,18 +507,14 @@ 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->head);
-		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 +522,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->head);
-		} 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->head);
 			if (err)
@@ -536,8 +532,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 +560,10 @@ EXPORT_SYMBOL(tcf_exts_change);
 #define tcf_exts_first_act(ext) \
 		list_first_entry(&(exts)->head, 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->head)) {
+	if (exts->action && !list_empty(&exts->head)) {
 		/*
 		 * again for backward compatible mode - we want
 		 * to work with both old and new modes of entering
@@ -576,15 +571,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->head, 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 +595,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

[PATCH net-next v3 5/6] 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 ebaf4b5..2678b67 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 88b5087..2641a8b 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

[PATCH net-next v3 6/6] net_sched: convert tcf_hashinfo to hlist and use rcu

[Cong Wang]

So that we don't need to play with singly linked list
and of course we can use RCU+spinlock instead of rwlock
now.

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    | 71 +++++++++++++++++++++++---------------------------
 net/sched/act_police.c | 47 ++++++++++++++-------------------
 3 files changed, 62 insertions(+), 72 deletions(-)

diff --git a/include/net/act_api.h b/include/net/act_api.h
index 2678b67..18af68e 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 2641a8b..974e8f7 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -29,25 +29,17 @@
 
 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_rcu(&p->tcfc_head);
+	spin_unlock_bh(&hinfo->lock);
+	synchronize_rcu();
+	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 +65,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);
+	rcu_read_lock_bh();
 
 	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 +100,7 @@ static int tcf_dump_walker(struct sk_buff *skb, struct netlink_callback *cb,
 		}
 	}
 done:
-	read_unlock_bh(&hinfo->lock);
+	rcu_read_unlock_bh();
 	if (n_i)
 		cb->args[0] += n_i;
 	return n_i;
@@ -120,7 +113,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 +125,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 +160,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) {
+	rcu_read_lock_bh();
+	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);
+	rcu_read_unlock_bh();
 
 	return p;
 }
@@ -236,6 +228,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 +250,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);
-	p->tcfc_next = hinfo->htab[h];
-	hinfo->htab[h] = p;
-	write_unlock_bh(&hinfo->lock);
+	spin_lock_bh(&hinfo->lock);
+	hlist_add_head_rcu(&p->tcfc_head, &hinfo->htab[h]);
+	spin_unlock_bh(&hinfo->lock);
+	synchronize_rcu();
 }
 EXPORT_SYMBOL(tcf_hash_insert);
 
diff --git a/net/sched/act_police.c b/net/sched/act_police.c
index f201576..26dffca 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);
+	rcu_read_lock_bh();
 
 	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);
+	rcu_read_unlock_bh();
 	if (n_i)
 		cb->args[0] += n_i;
 	return n_i;
@@ -106,25 +107,17 @@ 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_rcu(&p->tcf_head);
+	spin_unlock_bh(&police_hash_info.lock);
+	synchronize_rcu();
+	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 +252,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_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_rcu(&police->tcf_head, &police_hash_info.htab[h]);
+	spin_unlock_bh(&police_hash_info.lock);
+	synchronize_rcu();
 
 	a->priv = police;
 	return ret;
-- 
1.8.1.4

Re: [PATCH net-next v3 6/6] net_sched: convert tcf_hashinfo to hlist and use rcu

[Eric Dumazet]

On Thu, 2013-12-12 at 21:47 -0800, Cong Wang wrote:
> So that we don't need to play with singly linked list
> and of course we can use RCU+spinlock instead of rwlock
> now.

Just replace the rwlock by spinlock, no need for RCU ?

> 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    | 71 +++++++++++++++++++++++---------------------------
>  net/sched/act_police.c | 47 ++++++++++++++-------------------
>  3 files changed, 62 insertions(+), 72 deletions(-)
> 
> diff --git a/include/net/act_api.h b/include/net/act_api.h
> index 2678b67..18af68e 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 2641a8b..974e8f7 100644
> --- a/net/sched/act_api.c
> +++ b/net/sched/act_api.c
> @@ -29,25 +29,17 @@
>  
>  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_rcu(&p->tcfc_head);
> +	spin_unlock_bh(&hinfo->lock);
> +	synchronize_rcu();

Why is this needed ?

> +	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 +65,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);
> +	rcu_read_lock_bh();
>  
>  	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 +100,7 @@ static int tcf_dump_walker(struct sk_buff *skb, struct netlink_callback *cb,
>  		}
>  	}
>  done:
> -	read_unlock_bh(&hinfo->lock);
> +	rcu_read_unlock_bh();
>  	if (n_i)
>  		cb->args[0] += n_i;
>  	return n_i;
> @@ -120,7 +113,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 +125,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 +160,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) {
> +	rcu_read_lock_bh();
> +	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);
> +	rcu_read_unlock_bh();
>  
>  	return p;
>  }
> @@ -236,6 +228,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 +250,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);
> -	p->tcfc_next = hinfo->htab[h];
> -	hinfo->htab[h] = p;
> -	write_unlock_bh(&hinfo->lock);
> +	spin_lock_bh(&hinfo->lock);
> +	hlist_add_head_rcu(&p->tcfc_head, &hinfo->htab[h]);
> +	spin_unlock_bh(&hinfo->lock);
> +	synchronize_rcu();

Why is this needed ?

>  }
>  EXPORT_SYMBOL(tcf_hash_insert);
>  
> diff --git a/net/sched/act_police.c b/net/sched/act_police.c
> index f201576..26dffca 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);
> +	rcu_read_lock_bh();
>  
>  	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);
> +	rcu_read_unlock_bh();
>  	if (n_i)
>  		cb->args[0] += n_i;
>  	return n_i;
> @@ -106,25 +107,17 @@ 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_rcu(&p->tcf_head);
> +	spin_unlock_bh(&police_hash_info.lock);
> +	synchronize_rcu();

Why is this needed ?

> +	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 +252,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_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_rcu(&police->tcf_head, &police_hash_info.htab[h]);
> +	spin_unlock_bh(&police_hash_info.lock);
> +	synchronize_rcu();

Why is this needed ?

>  
>  	a->priv = police;
>  	return ret;


Really, before sending RCU conversions, you need to understand how it
works.

There are plenty documentations and samples, I do not need to even
explain more.

Anyway, do you _really_ need RCU in management path at all ?

RCU is generally harder to understand and maintain than plain
traditional locking, so its use should be limited to the places
it really is needed.

Re: [PATCH net-next v3 6/6] net_sched: convert tcf_hashinfo to hlist and use rcu

[Cong Wang]

On Thu, Dec 12, 2013 at 10:48 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Thu, 2013-12-12 at 21:47 -0800, Cong Wang wrote:
>> So that we don't need to play with singly linked list
>> and of course we can use RCU+spinlock instead of rwlock
>> now.
>
> Just replace the rwlock by spinlock, no need for RCU ?


Good point! It seems this hash list is not on hot path, I think
just spinlock should be enough.