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

[PATCH net-next 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.

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 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>
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 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.

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 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

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 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

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 0/4] net/sched: act_api: contiguous action arrays

[Jakub Kicinski]

On Thu, 30 Nov 2023 12:20:37 -0300 Pedro Tammela 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.

Hi Pedro, this appears not to apply.
-- 
pw-bot: cr

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

[Pedro Tammela]

On 01/12/2023 14:33, Jakub Kicinski wrote:
> On Thu, 30 Nov 2023 12:20:37 -0300 Pedro Tammela 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.
> 
> Hi Pedro, this appears not to apply.

Oops, will respin, thanks!