* [Patch net-next] net_sched: remove the leftover cleanup_a()
@ 2016-08-02 17:30 Cong Wang
2016-08-02 17:30 ` [Patch net-next] net_sched: remove an unnecessary list_del() Cong Wang
2016-08-03 11:36 ` [Patch net-next] net_sched: remove the leftover cleanup_a() Jamal Hadi Salim
0 siblings, 2 replies; 7+ messages in thread
From: Cong Wang @ 2016-08-02 17:30 UTC (permalink / raw)
To: netdev; +Cc: Cong Wang, Jamal Hadi Salim
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [Patch net-next] net_sched: remove an unnecessary list_del()
2016-08-02 17:30 [Patch net-next] net_sched: remove the leftover cleanup_a() Cong Wang
@ 2016-08-02 17:30 ` Cong Wang
2016-08-08 17:01 ` Cong Wang
2016-08-03 11:36 ` [Patch net-next] net_sched: remove the leftover cleanup_a() Jamal Hadi Salim
1 sibling, 1 reply; 7+ messages in thread
From: Cong Wang @ 2016-08-02 17:30 UTC (permalink / raw)
To: netdev; +Cc: Cong Wang, Jamal Hadi Salim
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [Patch net-next] net_sched: remove the leftover cleanup_a()
2016-08-02 17:30 [Patch net-next] net_sched: remove the leftover cleanup_a() Cong Wang
2016-08-02 17:30 ` [Patch net-next] net_sched: remove an unnecessary list_del() Cong Wang
@ 2016-08-03 11:36 ` Jamal Hadi Salim
2016-08-03 15:20 ` Cong Wang
1 sibling, 1 reply; 7+ messages in thread
From: Jamal Hadi Salim @ 2016-08-03 11:36 UTC (permalink / raw)
To: Cong Wang, netdev
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Patch net-next] net_sched: remove the leftover cleanup_a()
2016-08-03 11:36 ` [Patch net-next] net_sched: remove the leftover cleanup_a() Jamal Hadi Salim
@ 2016-08-03 15:20 ` Cong Wang
2016-08-04 12:10 ` Jamal Hadi Salim
0 siblings, 1 reply; 7+ messages in thread
From: Cong Wang @ 2016-08-03 15:20 UTC (permalink / raw)
To: Jamal Hadi Salim; +Cc: Linux Kernel Network Developers
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!
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Patch net-next] net_sched: remove the leftover cleanup_a()
2016-08-03 15:20 ` Cong Wang
@ 2016-08-04 12:10 ` Jamal Hadi Salim
2016-08-04 15:29 ` Cong Wang
0 siblings, 1 reply; 7+ messages in thread
From: Jamal Hadi Salim @ 2016-08-04 12:10 UTC (permalink / raw)
To: Cong Wang; +Cc: Linux Kernel Network Developers
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Patch net-next] net_sched: remove the leftover cleanup_a()
2016-08-04 12:10 ` Jamal Hadi Salim
@ 2016-08-04 15:29 ` Cong Wang
0 siblings, 0 replies; 7+ messages in thread
From: Cong Wang @ 2016-08-04 15:29 UTC (permalink / raw)
To: Jamal Hadi Salim; +Cc: Linux Kernel Network Developers
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Patch net-next] net_sched: remove an unnecessary list_del()
2016-08-02 17:30 ` [Patch net-next] net_sched: remove an unnecessary list_del() Cong Wang
@ 2016-08-08 17:01 ` Cong Wang
0 siblings, 0 replies; 7+ messages in thread
From: Cong Wang @ 2016-08-08 17:01 UTC (permalink / raw)
To: Linux Kernel Network Developers; +Cc: Cong Wang, Jamal Hadi Salim
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2016-08-08 17:02 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-08-02 17:30 [Patch net-next] net_sched: remove the leftover cleanup_a() Cong Wang
2016-08-02 17:30 ` [Patch net-next] net_sched: remove an unnecessary list_del() Cong Wang
2016-08-08 17:01 ` Cong Wang
2016-08-03 11:36 ` [Patch net-next] net_sched: remove the leftover cleanup_a() Jamal Hadi Salim
2016-08-03 15:20 ` Cong Wang
2016-08-04 12:10 ` Jamal Hadi Salim
2016-08-04 15:29 ` Cong Wang
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.