[Patch net-next] net_sched: remove the leftover cleanup_a()

[Patch net-next] net_sched: remove the leftover cleanup_a()

[Cong Wang]

After refactoring tc_action into tcf_common, we no
longer need to cleanup temporary "actions" in list,
they are permanently stored in the hashtable.

Reported-by: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 net/sched/act_api.c | 22 +++-------------------
 1 file changed, 3 insertions(+), 19 deletions(-)

diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index e4a5f26..cce6986 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -754,16 +754,6 @@ static struct tc_action *tcf_action_get_1(struct net *net, struct nlattr *nla,
 	return ERR_PTR(err);
 }
 
-static void cleanup_a(struct list_head *actions)
-{
-	struct tc_action *a, *tmp;
-
-	list_for_each_entry_safe(a, tmp, actions, list) {
-		list_del(&a->list);
-		kfree(a);
-	}
-}
-
 static int tca_action_flush(struct net *net, struct nlattr *nla,
 			    struct nlmsghdr *n, u32 portid)
 {
@@ -905,7 +895,7 @@ tca_action_gd(struct net *net, struct nlattr *nla, struct nlmsghdr *n,
 		return ret;
 	}
 err:
-	cleanup_a(&actions);
+	tcf_action_destroy(&actions, 0);
 	return ret;
 }
 
@@ -942,15 +932,9 @@ tcf_action_add(struct net *net, struct nlattr *nla, struct nlmsghdr *n,
 
 	ret = tcf_action_init(net, nla, NULL, NULL, ovr, 0, &actions);
 	if (ret)
-		goto done;
+		return ret;
 
-	/* dump then free all the actions after update; inserted policy
-	 * stays intact
-	 */
-	ret = tcf_add_notify(net, n, &actions, portid);
-	cleanup_a(&actions);
-done:
-	return ret;
+	return tcf_add_notify(net, n, &actions, portid);
 }
 
 static int tc_ctl_action(struct sk_buff *skb, struct nlmsghdr *n)
-- 
2.1.0

[Patch net-next] net_sched: remove an unnecessary list_del()

[Cong Wang]

This list_del() for tc action is not needed actually,
because we only use this list to chain bulk operations,
therefore should not be carried for latter operations.

Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 net/sched/act_api.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index cce6986..b4c7be3 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -64,7 +64,6 @@ int __tcf_hash_release(struct tc_action *p, bool bind, bool strict)
 		if (p->tcfa_bindcnt <= 0 && p->tcfa_refcnt <= 0) {
 			if (p->ops->cleanup)
 				p->ops->cleanup(p, bind);
-			list_del(&p->list);
 			tcf_hash_destroy(p->hinfo, p);
 			ret = ACT_P_DELETED;
 		}
-- 
2.1.0

Re: [Patch net-next] net_sched: remove an unnecessary list_del()

[Cong Wang]

On Tue, Aug 2, 2016 at 10:30 AM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> This list_del() for tc action is not needed actually,
> because we only use this list to chain bulk operations,
> therefore should not be carried for latter operations.
>
> Cc: Jamal Hadi Salim <jhs@mojatatu.com>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>

David, please drop this patch from your backlog, I will
resend it with Fixes tag.

Re: [Patch net-next] net_sched: remove the leftover cleanup_a()

[Jamal Hadi Salim]

Cong,

Improvement on the basic test as it no longer oopses, but:
here's a test that will oops:
-----------------------------
# add action that will be shared for multi-flows
sudo tc actions add action drop index 10
#add an action noone uses
sudo tc actions add action ok index 12
#
sudo tc actions ls action gact
# see both dumped... (good)
#Lets bind the shared action twice
#
sudo tc qdisc del dev lo parent ffff:
sudo tc qdisc add dev lo ingress
#
#use existing action index 10
sudo tc filter add dev lo parent ffff: protocol ip prio 8 \
u32 match ip dst 127.0.0.8/32 flowid 1:10 action gact index 10
#use existing action index 10
sudo tc filter add dev lo parent ffff: protocol ip prio 7 \
u32 match ip src 127.0.0.10/32 flowid 1:11 action gact index 10
# and now dump and see an oops
sudo tc filter ls dev lo parent ffff: protocol ip
-------------------

And btw, this should be submitted to -net not -net-next and should
specify something along the lines of "broken-by .."

cheers,
jamal

Re: [Patch net-next] net_sched: remove the leftover cleanup_a()

[Cong Wang]

On Wed, Aug 3, 2016 at 4:36 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> Cong,
>
> Improvement on the basic test as it no longer oopses, but:
> here's a test that will oops:

Make sure you test this together with the other patch I sent
(sorry I didn't number them), the other patch also fixed an oops.


>
> And btw, this should be submitted to -net not -net-next and should
> specify something along the lines of "broken-by .."


Oh, sure, my bad, I thought net-next is still not closed, it should
be already closed. Here it is:

Fixes: a85a970af265 ("net_sched: move tc_action into tcf_common")


Thanks!

Re: [Patch net-next] net_sched: remove the leftover cleanup_a()

[Jamal Hadi Salim]

On 16-08-03 11:20 AM, Cong Wang wrote:
> On Wed, Aug 3, 2016 at 4:36 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>> Cong,
>>
>> Improvement on the basic test as it no longer oopses, but:
>> here's a test that will oops:
>
> Make sure you test this together with the other patch I sent
> (sorry I didn't number them), the other patch also fixed an oops.
>

Ok, just included second patch - problem still there.
Try the testcase i outlined earlier.

cheers,
jamal

Re: [Patch net-next] net_sched: remove the leftover cleanup_a()

[Cong Wang]

On Thu, Aug 4, 2016 at 5:10 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> On 16-08-03 11:20 AM, Cong Wang wrote:
>>
>> On Wed, Aug 3, 2016 at 4:36 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>>>
>>> Cong,
>>>
>>> Improvement on the basic test as it no longer oopses, but:
>>> here's a test that will oops:
>>
>>
>> Make sure you test this together with the other patch I sent
>> (sorry I didn't number them), the other patch also fixed an oops.
>>
>
> Ok, just included second patch - problem still there.
> Try the testcase i outlined earlier.

I tried it yesterday, and have been working on a patch.
Basically, I need to replace the list with a flex_array
to hold pointers to these actions. I will send it out tomorrow.

Thanks.