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

2018-05-30 Thread Jiri Pirko 
Sun, May 27, 2018 at 11:17:29PM CEST, vla...@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 

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

Acked-by: Jiri Pirko

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

2018-05-29 Thread Vlad Buslov 


On Mon 28 May 2018 at 21:31, Marcelo Ricardo Leitner 
 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(>list, );
>> -tcf_action_destroy(, 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!

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

2018-05-28 Thread Marcelo Ricardo Leitner 
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(>list, );
> - tcf_action_destroy(, bind);
> + tcf_action_destroy(actions, bind);
>   NL_SET_ERR_MSG(extack, "Failed to init TC action 
> chain");
>   return ERR_PTR(err);
>   }

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

2018-05-27 Thread Vlad Buslov 
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 
---
 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(-)

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);
+