diff mbox series

[v3,11/11] net: sched: change action API to use array of pointers to actions

Message ID 1527455849-22327-12-git-send-email-vladbu@mellanox.com
State Changes Requested, archived
Delegated to: David Miller
Headers show
Series Modify action API for implementing lockless actions | expand

Commit Message

Vlad Buslov May 27, 2018, 9:17 p.m. UTC 
Act API used linked list to pass set of actions to functions. It is
intrusive data structure that stores list nodes inside action structure
itself, which means it is not safe to modify such list concurrently.
However, action API doesn't use any linked list specific operations on this
set of actions, so it can be safely refactored into plain pointer array.

Refactor action API to use array of pointers to tc_actions instead of
linked list. Change argument 'actions' type of exported action init,
destroy and dump functions.

Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
---
 include/net/act_api.h |  7 ++---
 net/sched/act_api.c   | 74 ++++++++++++++++++++++++++++-----------------------
 net/sched/cls_api.c   | 21 +++++----------
 3 files changed, 50 insertions(+), 52 deletions(-)

Comments

Marcelo Ricardo Leitner May 28, 2018, 9:31 p.m. UTC | #1 
On Mon, May 28, 2018 at 12:17:29AM +0300, Vlad Buslov wrote:
...
> -int tcf_action_destroy(struct list_head *actions, int bind)
> +int tcf_action_destroy(struct tc_action *actions[], int bind)
>  {
>  	const struct tc_action_ops *ops;
> -	struct tc_action *a, *tmp;
> -	int ret = 0;
> +	struct tc_action *a;
> +	int ret = 0, i;
>  
> -	list_for_each_entry_safe(a, tmp, actions, list) {
> +	for (i = 0; i < TCA_ACT_MAX_PRIO && actions[i]; i++) {
...
> @@ -878,10 +881,9 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
>  	if (TC_ACT_EXT_CMP(a->tcfa_action, TC_ACT_GOTO_CHAIN)) {
>  		err = tcf_action_goto_chain_init(a, tp);
>  		if (err) {
> -			LIST_HEAD(actions);
> +			struct tc_action *actions[TCA_ACT_MAX_PRIO] = { a };

Somewhat nit.. Considering tcf_action_destroy will stop at the first
NULL, you need only 2 slots here.

>  
> -			list_add_tail(&a->list, &actions);
> -			tcf_action_destroy(&actions, bind);
> +			tcf_action_destroy(actions, bind);
>  			NL_SET_ERR_MSG(extack, "Failed to init TC action chain");
>  			return ERR_PTR(err);
>  		}
Vlad Buslov May 29, 2018, 10:25 a.m. UTC | #2 
On Mon 28 May 2018 at 21:31, Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> wrote:
> On Mon, May 28, 2018 at 12:17:29AM +0300, Vlad Buslov wrote:
> ...
>> -int tcf_action_destroy(struct list_head *actions, int bind)
>> +int tcf_action_destroy(struct tc_action *actions[], int bind)
>>  {
>>  	const struct tc_action_ops *ops;
>> -	struct tc_action *a, *tmp;
>> -	int ret = 0;
>> +	struct tc_action *a;
>> +	int ret = 0, i;
>>  
>> -	list_for_each_entry_safe(a, tmp, actions, list) {
>> +	for (i = 0; i < TCA_ACT_MAX_PRIO && actions[i]; i++) {
> ...
>> @@ -878,10 +881,9 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
>>  	if (TC_ACT_EXT_CMP(a->tcfa_action, TC_ACT_GOTO_CHAIN)) {
>>  		err = tcf_action_goto_chain_init(a, tp);
>>  		if (err) {
>> -			LIST_HEAD(actions);
>> +			struct tc_action *actions[TCA_ACT_MAX_PRIO] = { a };
>
> Somewhat nit.. Considering tcf_action_destroy will stop at the first
> NULL, you need only 2 slots here.

Yes, I guess NULLing whole array when only first pointer is used, is
redundant. I didn't want to be too clever in this patch and made all
actions array of same size, but I don't see anything potentially
dangerous in reducing this one.

>
>>  
>> -			list_add_tail(&a->list, &actions);
>> -			tcf_action_destroy(&actions, bind);
>> +			tcf_action_destroy(actions, bind);
>>  			NL_SET_ERR_MSG(extack, "Failed to init TC action chain");
>>  			return ERR_PTR(err);
>>  		}

Thank you for reviewing my code!
Jiri Pirko May 30, 2018, 8:37 p.m. UTC | #3 
Sun, May 27, 2018 at 11:17:29PM CEST, vladbu@mellanox.com wrote:
>Act API used linked list to pass set of actions to functions. It is
>intrusive data structure that stores list nodes inside action structure
>itself, which means it is not safe to modify such list concurrently.
>However, action API doesn't use any linked list specific operations on this
>set of actions, so it can be safely refactored into plain pointer array.
>
>Refactor action API to use array of pointers to tc_actions instead of
>linked list. Change argument 'actions' type of exported action init,
>destroy and dump functions.
>
>Signed-off-by: Vlad Buslov <vladbu@mellanox.com>

Even with the nit Marcelo found, this looks fine to me.

Acked-by: Jiri Pirko <jiri@mellanox.com>
diff mbox series

Patch

diff --git a/include/net/act_api.h b/include/net/act_api.h
index cd4547476074..43dfa5e1b3b3 100644
--- a/include/net/act_api.h
+++ b/include/net/act_api.h
@@ -168,19 +168,20 @@  static inline int tcf_idr_release(struct tc_action *a, bool bind)
 int tcf_register_action(struct tc_action_ops *a, struct pernet_operations *ops);
 int tcf_unregister_action(struct tc_action_ops *a,
 			  struct pernet_operations *ops);
-int tcf_action_destroy(struct list_head *actions, int bind);
+int tcf_action_destroy(struct tc_action *actions[], int bind);
 int tcf_action_exec(struct sk_buff *skb, struct tc_action **actions,
 		    int nr_actions, struct tcf_result *res);
 int tcf_action_init(struct net *net, struct tcf_proto *tp, struct nlattr *nla,
 		    struct nlattr *est, char *name, int ovr, int bind,
-		    struct list_head *actions, size_t *attr_size,
+		    struct tc_action *actions[], size_t *attr_size,
 		    bool rtnl_held, struct netlink_ext_ack *extack);
 struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
 				    struct nlattr *nla, struct nlattr *est,
 				    char *name, int ovr, int bind,
 				    bool rtnl_held,
 				    struct netlink_ext_ack *extack);
-int tcf_action_dump(struct sk_buff *skb, struct list_head *, int, int);
+int tcf_action_dump(struct sk_buff *skb, struct tc_action *actions[], int bind,
+		    int ref);
 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/net/sched/act_api.c b/net/sched/act_api.c
index 9511502e1cbb..7f904bb84aab 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -657,13 +657,14 @@  int tcf_action_exec(struct sk_buff *skb, struct tc_action **actions,
 }
 EXPORT_SYMBOL(tcf_action_exec);
 
-int tcf_action_destroy(struct list_head *actions, int bind)
+int tcf_action_destroy(struct tc_action *actions[], int bind)
 {
 	const struct tc_action_ops *ops;
-	struct tc_action *a, *tmp;
-	int ret = 0;
+	struct tc_action *a;
+	int ret = 0, i;
 
-	list_for_each_entry_safe(a, tmp, actions, list) {
+	for (i = 0; i < TCA_ACT_MAX_PRIO && actions[i]; i++) {
+		a = actions[i];
 		ops = a->ops;
 		ret = __tcf_idr_release(a, bind, true);
 		if (ret == ACT_P_DELETED)
@@ -679,11 +680,12 @@  static int tcf_action_put(struct tc_action *p)
 	return __tcf_action_put(p, false);
 }
 
-static void tcf_action_put_lst(struct list_head *actions)
+static void tcf_action_put_many(struct tc_action *actions[])
 {
-	struct tc_action *a, *tmp;
+	int i;
 
-	list_for_each_entry_safe(a, tmp, actions, list) {
+	for (i = 0; i < TCA_ACT_MAX_PRIO && actions[i]; i++) {
+		struct tc_action *a = actions[i];
 		const struct tc_action_ops *ops = a->ops;
 
 		if (tcf_action_put(a))
@@ -735,14 +737,15 @@  tcf_action_dump_1(struct sk_buff *skb, struct tc_action *a, int bind, int ref)
 }
 EXPORT_SYMBOL(tcf_action_dump_1);
 
-int tcf_action_dump(struct sk_buff *skb, struct list_head *actions,
+int tcf_action_dump(struct sk_buff *skb, struct tc_action *actions[],
 		    int bind, int ref)
 {
 	struct tc_action *a;
-	int err = -EINVAL;
+	int err = -EINVAL, i;
 	struct nlattr *nest;
 
-	list_for_each_entry(a, actions, list) {
+	for (i = 0; i < TCA_ACT_MAX_PRIO && actions[i]; i++) {
+		a = actions[i];
 		nest = nla_nest_start(skb, a->order);
 		if (nest == NULL)
 			goto nla_put_failure;
@@ -878,10 +881,9 @@  struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
 	if (TC_ACT_EXT_CMP(a->tcfa_action, TC_ACT_GOTO_CHAIN)) {
 		err = tcf_action_goto_chain_init(a, tp);
 		if (err) {
-			LIST_HEAD(actions);
+			struct tc_action *actions[TCA_ACT_MAX_PRIO] = { a };
 
-			list_add_tail(&a->list, &actions);
-			tcf_action_destroy(&actions, bind);
+			tcf_action_destroy(actions, bind);
 			NL_SET_ERR_MSG(extack, "Failed to init TC action chain");
 			return ERR_PTR(err);
 		}
@@ -899,9 +901,11 @@  struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
 	return ERR_PTR(err);
 }
 
+/* Returns numbers of initialized actions or negative error. */
+
 int tcf_action_init(struct net *net, struct tcf_proto *tp, struct nlattr *nla,
 		    struct nlattr *est, char *name, int ovr, int bind,
-		    struct list_head *actions, size_t *attr_size,
+		    struct tc_action *actions[], size_t *attr_size,
 		    bool rtnl_held, struct netlink_ext_ack *extack)
 {
 	struct nlattr *tb[TCA_ACT_MAX_PRIO + 1];
@@ -923,11 +927,12 @@  int tcf_action_init(struct net *net, struct tcf_proto *tp, struct nlattr *nla,
 		}
 		act->order = i;
 		sz += tcf_action_fill_size(act);
-		list_add_tail(&act->list, actions);
+		/* Start from index 0 */
+		actions[i - 1] = act;
 	}
 
 	*attr_size = tcf_action_full_attrs_size(sz);
-	return 0;
+	return i - 1;
 
 err:
 	tcf_action_destroy(actions, bind);
@@ -978,7 +983,7 @@  int tcf_action_copy_stats(struct sk_buff *skb, struct tc_action *p,
 	return -1;
 }
 
-static int tca_get_fill(struct sk_buff *skb, struct list_head *actions,
+static int tca_get_fill(struct sk_buff *skb, struct tc_action *actions[],
 			u32 portid, u32 seq, u16 flags, int event, int bind,
 			int ref)
 {
@@ -1014,7 +1019,7 @@  static int tca_get_fill(struct sk_buff *skb, struct list_head *actions,
 
 static int
 tcf_get_notify(struct net *net, u32 portid, struct nlmsghdr *n,
-	       struct list_head *actions, int event,
+	       struct tc_action *actions[], int event,
 	       struct netlink_ext_ack *extack)
 {
 	struct sk_buff *skb;
@@ -1150,14 +1155,14 @@  static int tca_action_flush(struct net *net, struct nlattr *nla,
 	return err;
 }
 
-static int tcf_action_delete(struct net *net, struct list_head *actions,
+static int tcf_action_delete(struct net *net, struct tc_action *actions[],
 			     struct netlink_ext_ack *extack)
 {
-	struct tc_action *a, *tmp;
 	u32 act_index;
-	int ret;
+	int ret, i;
 
-	list_for_each_entry_safe(a, tmp, actions, list) {
+	for (i = 0; i < TCA_ACT_MAX_PRIO && actions[i]; i++) {
+		struct tc_action *a = actions[i];
 		const struct tc_action_ops *ops = a->ops;
 
 		/* Actions can be deleted concurrently so we must save their
@@ -1180,7 +1185,7 @@  static int tcf_action_delete(struct net *net, struct list_head *actions,
 }
 
 static int
-tcf_del_notify(struct net *net, struct nlmsghdr *n, struct list_head *actions,
+tcf_del_notify(struct net *net, struct nlmsghdr *n, struct tc_action *actions[],
 	       u32 portid, size_t attr_size, struct netlink_ext_ack *extack)
 {
 	int ret;
@@ -1221,7 +1226,7 @@  tca_action_gd(struct net *net, struct nlattr *nla, struct nlmsghdr *n,
 	struct nlattr *tb[TCA_ACT_MAX_PRIO + 1];
 	struct tc_action *act;
 	size_t attr_size = 0;
-	LIST_HEAD(actions);
+	struct tc_action *actions[TCA_ACT_MAX_PRIO] = {};
 
 	ret = nla_parse_nested(tb, TCA_ACT_MAX_PRIO, nla, NULL, extack);
 	if (ret < 0)
@@ -1243,26 +1248,27 @@  tca_action_gd(struct net *net, struct nlattr *nla, struct nlmsghdr *n,
 		}
 		act->order = i;
 		attr_size += tcf_action_fill_size(act);
-		list_add_tail(&act->list, &actions);
+		actions[i - 1] = act;
 	}
 
 	attr_size = tcf_action_full_attrs_size(attr_size);
 
 	if (event == RTM_GETACTION)
-		ret = tcf_get_notify(net, portid, n, &actions, event, extack);
+		ret = tcf_get_notify(net, portid, n, actions, event, extack);
 	else { /* delete */
-		ret = tcf_del_notify(net, n, &actions, portid, attr_size, extack);
+		ret = tcf_del_notify(net, n, actions, portid, attr_size,
+				     extack);
 		if (ret)
 			goto err;
 		return ret;
 	}
 err:
-	tcf_action_put_lst(&actions);
+	tcf_action_put_many(actions);
 	return ret;
 }
 
 static int
-tcf_add_notify(struct net *net, struct nlmsghdr *n, struct list_head *actions,
+tcf_add_notify(struct net *net, struct nlmsghdr *n, struct tc_action *actions[],
 	       u32 portid, size_t attr_size, struct netlink_ext_ack *extack)
 {
 	struct sk_buff *skb;
@@ -1293,15 +1299,15 @@  static int tcf_action_add(struct net *net, struct nlattr *nla,
 {
 	size_t attr_size = 0;
 	int ret = 0;
-	LIST_HEAD(actions);
+	struct tc_action *actions[TCA_ACT_MAX_PRIO] = {};
 
-	ret = tcf_action_init(net, NULL, nla, NULL, NULL, ovr, 0, &actions,
+	ret = tcf_action_init(net, NULL, nla, NULL, NULL, ovr, 0, actions,
 			      &attr_size, true, extack);
-	if (ret)
+	if (ret < 0)
 		return ret;
-	ret = tcf_add_notify(net, n, &actions, portid, attr_size, extack);
+	ret = tcf_add_notify(net, n, actions, portid, attr_size, extack);
 	if (ovr)
-		tcf_action_put_lst(&actions);
+		tcf_action_put_many(actions);
 
 	return ret;
 }
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index dd37d0ae3fce..b2bb6b7b038a 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -1415,10 +1415,7 @@  static int tc_dump_tfilter(struct sk_buff *skb, struct netlink_callback *cb)
 void tcf_exts_destroy(struct tcf_exts *exts)
 {
 #ifdef CONFIG_NET_CLS_ACT
-	LIST_HEAD(actions);
-
-	tcf_exts_to_list(exts, &actions);
-	tcf_action_destroy(&actions, TCA_ACT_UNBIND);
+	tcf_action_destroy(exts->actions, TCA_ACT_UNBIND);
 	kfree(exts->actions);
 	exts->nr_actions = 0;
 #endif
@@ -1445,18 +1442,15 @@  int tcf_exts_validate(struct net *net, struct tcf_proto *tp, struct nlattr **tb,
 			exts->actions[0] = act;
 			exts->nr_actions = 1;
 		} else if (exts->action && tb[exts->action]) {
-			LIST_HEAD(actions);
-			int err, i = 0;
+			int err;
 
 			err = tcf_action_init(net, tp, tb[exts->action],
 					      rate_tlv, NULL, ovr, TCA_ACT_BIND,
-					      &actions, &attr_size, true,
+					      exts->actions, &attr_size, true,
 					      extack);
-			if (err)
+			if (err < 0)
 				return err;
-			list_for_each_entry(act, &actions, list)
-				exts->actions[i++] = act;
-			exts->nr_actions = i;
+			exts->nr_actions = err;
 		}
 		exts->net = net;
 	}
@@ -1505,14 +1499,11 @@  int tcf_exts_dump(struct sk_buff *skb, struct tcf_exts *exts)
 		 * tc data even if iproute2  was newer - jhs
 		 */
 		if (exts->type != TCA_OLD_COMPAT) {
-			LIST_HEAD(actions);
-
 			nest = nla_nest_start(skb, exts->action);
 			if (nest == NULL)
 				goto nla_put_failure;
 
-			tcf_exts_to_list(exts, &actions);
-			if (tcf_action_dump(skb, &actions, 0, 0) < 0)
+			if (tcf_action_dump(skb, exts->actions, 0, 0) < 0)
 				goto nla_put_failure;
 			nla_nest_end(skb, nest);
 		} else if (exts->police) {