[PATCH net-next v2 0/4] net/sched: act_api: contiguous action arrays

[PATCH net-next v2 0/4] net/sched: act_api: contiguous action arrays

[Pedro Tammela]

When dealing with action arrays in act_api it's natural to ask if they
are always contiguous (no NULL pointers in between). Yes, they are in
all cases so far, so make use of the already present tcf_act_for_each_action
macro to explicitly document this assumption.

There was an instance where it was not, but it was refactorable (patch 2)
to make the array contiguous.

v1->v2:
- Respin
- Added Jamal's acked-by

Pedro Tammela (4):
  net/sched: act_api: use tcf_act_for_each_action
  net/sched: act_api: avoid non-contiguous action array
  net/sched: act_api: stop loop over ops array on NULL in
    tcf_action_init
  net/sched: act_api: use tcf_act_for_each_action in tcf_idr_insert_many

 net/sched/act_api.c | 57 ++++++++++++++++++++++++---------------------
 1 file changed, 30 insertions(+), 27 deletions(-)

-- 
2.40.1

[PATCH net-next v2 1/4] net/sched: act_api: use tcf_act_for_each_action

[Pedro Tammela]

Use the auxiliary macro tcf_act_for_each_action in all the
functions that expect a contiguous action array

Suggested-by: Marcelo Ricardo Leitner <mleitner@redhat.com>
Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>
---
 net/sched/act_api.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index c39252d61ebb..05aae374c159 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -1118,8 +1118,7 @@ int tcf_action_destroy(struct tc_action *actions[], int bind)
 	struct tc_action *a;
 	int ret = 0, i;
 
-	for (i = 0; i < TCA_ACT_MAX_PRIO && actions[i]; i++) {
-		a = actions[i];
+	tcf_act_for_each_action(i, a, actions) {
 		actions[i] = NULL;
 		ops = a->ops;
 		ret = __tcf_idr_release(a, bind, true);
@@ -1211,8 +1210,7 @@ int tcf_action_dump(struct sk_buff *skb, struct tc_action *actions[],
 	int err = -EINVAL, i;
 	struct nlattr *nest;
 
-	for (i = 0; i < TCA_ACT_MAX_PRIO && actions[i]; i++) {
-		a = actions[i];
+	tcf_act_for_each_action(i, a, actions) {
 		nest = nla_nest_start_noflag(skb, i + 1);
 		if (nest == NULL)
 			goto nla_put_failure;
@@ -1753,10 +1751,10 @@ static int tca_action_flush(struct net *net, struct nlattr *nla,
 
 static int tcf_action_delete(struct net *net, struct tc_action *actions[])
 {
+	struct tc_action *a;
 	int i;
 
-	for (i = 0; i < TCA_ACT_MAX_PRIO && actions[i]; i++) {
-		struct tc_action *a = actions[i];
+	tcf_act_for_each_action(i, a, actions) {
 		const struct tc_action_ops *ops = a->ops;
 		/* Actions can be deleted concurrently so we must save their
 		 * type and id to search again after reference is released.
@@ -1768,7 +1766,7 @@ static int tcf_action_delete(struct net *net, struct tc_action *actions[])
 		if (tcf_action_put(a)) {
 			/* last reference, action was deleted concurrently */
 			module_put(ops->owner);
-		} else  {
+		} else {
 			int ret;
 
 			/* now do the delete */
-- 
2.40.1

[PATCH net-next v2 2/4] net/sched: act_api: avoid non-contiguous action array

[Pedro Tammela]

In tcf_action_add, when putting the reference for the bound actions
it assigns NULLs to just created actions passing a non contiguous
array to tcf_action_put_many.
Refactor the code so the actions array is always contiguous.

Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>
---
 net/sched/act_api.c | 33 +++++++++++++++++++++------------
 1 file changed, 21 insertions(+), 12 deletions(-)

diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 05aae374c159..2e948e5992b6 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -1135,18 +1135,29 @@ static int tcf_action_put(struct tc_action *p)
 	return __tcf_action_put(p, false);
 }
 
-/* Put all actions in this array, skip those NULL's. */
 static void tcf_action_put_many(struct tc_action *actions[])
 {
+	struct tc_action *a;
 	int i;
 
-	for (i = 0; i < TCA_ACT_MAX_PRIO; i++) {
-		struct tc_action *a = actions[i];
-		const struct tc_action_ops *ops;
+	tcf_act_for_each_action(i, a, actions) {
+		const struct tc_action_ops *ops = a->ops;
+		if (tcf_action_put(a))
+			module_put(ops->owner);
+	}
+}
 
-		if (!a)
+static void tca_put_bound_many(struct tc_action *actions[], int init_res[])
+{
+	struct tc_action *a;
+	int i;
+
+	tcf_act_for_each_action(i, a, actions) {
+		const struct tc_action_ops *ops = a->ops;
+
+		if (init_res[i] == ACT_P_CREATED)
 			continue;
-		ops = a->ops;
+
 		if (tcf_action_put(a))
 			module_put(ops->owner);
 	}
@@ -1975,7 +1986,7 @@ static int tcf_action_add(struct net *net, struct nlattr *nla,
 			  struct netlink_ext_ack *extack)
 {
 	size_t attr_size = 0;
-	int loop, ret, i;
+	int loop, ret;
 	struct tc_action *actions[TCA_ACT_MAX_PRIO] = {};
 	int init_res[TCA_ACT_MAX_PRIO] = {};
 
@@ -1988,13 +1999,11 @@ static int tcf_action_add(struct net *net, struct nlattr *nla,
 
 	if (ret < 0)
 		return ret;
+
 	ret = tcf_add_notify(net, n, actions, portid, attr_size, extack);
 
-	/* only put existing actions */
-	for (i = 0; i < TCA_ACT_MAX_PRIO; i++)
-		if (init_res[i] == ACT_P_CREATED)
-			actions[i] = NULL;
-	tcf_action_put_many(actions);
+	/* only put bound actions */
+	tca_put_bound_many(actions, init_res);
 
 	return ret;
 }
-- 
2.40.1

[PATCH net-next v2 3/4] net/sched: act_api: stop loop over ops array on NULL in tcf_action_init

[Pedro Tammela]

The ops array is contiguous, so stop processing whenever a NULL is found

Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>
---
 net/sched/act_api.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 2e948e5992b6..d3cb9f5b25da 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -1506,10 +1506,8 @@ int tcf_action_init(struct net *net, struct tcf_proto *tp, struct nlattr *nla,
 err:
 	tcf_action_destroy(actions, flags & TCA_ACT_FLAGS_BIND);
 err_mod:
-	for (i = 0; i < TCA_ACT_MAX_PRIO; i++) {
-		if (ops[i])
-			module_put(ops[i]->owner);
-	}
+	for (i = 0; i < TCA_ACT_MAX_PRIO && ops[i]; i++)
+		module_put(ops[i]->owner);
 	return err;
 }
 
-- 
2.40.1

[PATCH net-next v2 4/4] net/sched: act_api: use tcf_act_for_each_action in tcf_idr_insert_many

[Pedro Tammela]

The actions array is contiguous, so stop processing whenever a NULL
is found. This is already the assumption for tcf_action_destroy[1],
which is called from tcf_actions_init.

[1] https://elixir.bootlin.com/linux/v6.7-rc3/source/net/sched/act_api.c#L1115

Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>
---
 net/sched/act_api.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index d3cb9f5b25da..abec5c45b5a4 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -1285,14 +1285,12 @@ static const struct nla_policy tcf_action_policy[TCA_ACT_MAX + 1] = {
 
 void tcf_idr_insert_many(struct tc_action *actions[])
 {
+	struct tc_action *a;
 	int i;
 
-	for (i = 0; i < TCA_ACT_MAX_PRIO; i++) {
-		struct tc_action *a = actions[i];
+	tcf_act_for_each_action(i, a, actions) {
 		struct tcf_idrinfo *idrinfo;
 
-		if (!a)
-			continue;
 		idrinfo = a->idrinfo;
 		mutex_lock(&idrinfo->lock);
 		/* Replace ERR_PTR(-EBUSY) allocated by tcf_idr_check_alloc if
-- 
2.40.1

Re: [PATCH net-next v2 3/4] net/sched: act_api: stop loop over ops array on NULL in tcf_action_init

[Marcelo Ricardo Leitner]

On Fri, Dec 01, 2023 at 02:50:14PM -0300, Pedro Tammela wrote:
> -	for (i = 0; i < TCA_ACT_MAX_PRIO; i++) {
> -		if (ops[i])
> -			module_put(ops[i]->owner);
> -	}
> +	for (i = 0; i < TCA_ACT_MAX_PRIO && ops[i]; i++)
> +		module_put(ops[i]->owner);
>  	return err;

Seems you thought it would have been an abuse to use
tcf_act_for_each_action() here as well, which I can understand.

Re: [PATCH net-next v2 1/4] net/sched: act_api: use tcf_act_for_each_action

[Marcelo Ricardo Leitner]

On Fri, Dec 01, 2023 at 02:50:12PM -0300, Pedro Tammela wrote:
> Use the auxiliary macro tcf_act_for_each_action in all the
> functions that expect a contiguous action array
>
> Suggested-by: Marcelo Ricardo Leitner <mleitner@redhat.com>

(I should update the email mapping..)

> Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
> Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>

Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>

Re: [PATCH net-next v2 2/4] net/sched: act_api: avoid non-contiguous action array

[Marcelo Ricardo Leitner]

On Fri, Dec 01, 2023 at 02:50:13PM -0300, Pedro Tammela wrote:
> In tcf_action_add, when putting the reference for the bound actions
> it assigns NULLs to just created actions passing a non contiguous
> array to tcf_action_put_many.
> Refactor the code so the actions array is always contiguous.
>
> Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
> Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>

Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>

Re: [PATCH net-next v2 3/4] net/sched: act_api: stop loop over ops array on NULL in tcf_action_init

[Marcelo Ricardo Leitner]

On Fri, Dec 01, 2023 at 10:05:12AM -0800, Marcelo Ricardo Leitner wrote:
> On Fri, Dec 01, 2023 at 02:50:14PM -0300, Pedro Tammela wrote:
> > -	for (i = 0; i < TCA_ACT_MAX_PRIO; i++) {
> > -		if (ops[i])
> > -			module_put(ops[i]->owner);
> > -	}
> > +	for (i = 0; i < TCA_ACT_MAX_PRIO && ops[i]; i++)
> > +		module_put(ops[i]->owner);
> >  	return err;
>
> Seems you thought it would have been an abuse to use
> tcf_act_for_each_action() here as well, which I can understand.

Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>

Re: [PATCH net-next v2 4/4] net/sched: act_api: use tcf_act_for_each_action in tcf_idr_insert_many

[Marcelo Ricardo Leitner]

On Fri, Dec 01, 2023 at 02:50:15PM -0300, Pedro Tammela wrote:
> The actions array is contiguous, so stop processing whenever a NULL
> is found. This is already the assumption for tcf_action_destroy[1],
> which is called from tcf_actions_init.
>
> [1] https://elixir.bootlin.com/linux/v6.7-rc3/source/net/sched/act_api.c#L1115
>
> Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
> Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>

Patch could have been squashed with patch 1, btw.

Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>

Re: [PATCH net-next v2 0/4] net/sched: act_api: contiguous action arrays

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by Paolo Abeni <pabeni@redhat.com>:

On Fri,  1 Dec 2023 14:50:11 -0300 you wrote:
> When dealing with action arrays in act_api it's natural to ask if they
> are always contiguous (no NULL pointers in between). Yes, they are in
> all cases so far, so make use of the already present tcf_act_for_each_action
> macro to explicitly document this assumption.
> 
> There was an instance where it was not, but it was refactorable (patch 2)
> to make the array contiguous.
> 
> [...]

Here is the summary with links:
  - [net-next,v2,1/4] net/sched: act_api: use tcf_act_for_each_action
    https://git.kernel.org/netdev/net-next/c/3872347e0a16
  - [net-next,v2,2/4] net/sched: act_api: avoid non-contiguous action array
    https://git.kernel.org/netdev/net-next/c/a0e947c9ccff
  - [net-next,v2,3/4] net/sched: act_api: stop loop over ops array on NULL in tcf_action_init
    https://git.kernel.org/netdev/net-next/c/e09ac779f736
  - [net-next,v2,4/4] net/sched: act_api: use tcf_act_for_each_action in tcf_idr_insert_many
    https://git.kernel.org/netdev/net-next/c/f9bfc8eb1342

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html