* [net-next PATCH 1/2] fib_trie: Fix warning on fib4_rules_exit @ 2015-03-27 21:14 Alexander Duyck 2015-03-27 21:14 ` [net-next PATCH 2/2] fib_trie: Cleanup ip_fib_net_exit code path Alexander Duyck ` (2 more replies) 0 siblings, 3 replies; 10+ messages in thread From: Alexander Duyck @ 2015-03-27 21:14 UTC (permalink / raw) To: netdev; +Cc: Cong Wang, davem This fixes the following warning: BUG: sleeping function called from invalid context at mm/slub.c:1268 in_atomic(): 1, irqs_disabled(): 0, pid: 6, name: kworker/u8:0 INFO: lockdep is turned off. CPU: 3 PID: 6 Comm: kworker/u8:0 Tainted: G W 4.0.0-rc5+ #895 Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011 Workqueue: netns cleanup_net 0000000000000006 ffff88011953fa68 ffffffff81a203b6 000000002c3a2c39 ffff88011952a680 ffff88011953fa98 ffffffff8109daf0 ffff8801186c6aa8 ffffffff81fbc9e5 00000000000004f4 0000000000000000 ffff88011953fac8 Call Trace: [<ffffffff81a203b6>] dump_stack+0x4c/0x65 [<ffffffff8109daf0>] ___might_sleep+0x1c3/0x1cb [<ffffffff8109db70>] __might_sleep+0x78/0x80 [<ffffffff8117a60e>] slab_pre_alloc_hook+0x31/0x8f [<ffffffff8117d4f6>] __kmalloc+0x69/0x14e [<ffffffff818ed0e1>] ? kzalloc.constprop.20+0xe/0x10 [<ffffffff818ed0e1>] kzalloc.constprop.20+0xe/0x10 [<ffffffff818ef622>] fib_trie_table+0x27/0x8b [<ffffffff818ef6bd>] fib_trie_unmerge+0x37/0x2a6 [<ffffffff810b06e1>] ? arch_local_irq_save+0x9/0xc [<ffffffff818e9793>] fib_unmerge+0x2d/0xb3 [<ffffffff818f5f56>] fib4_rule_delete+0x1f/0x52 [<ffffffff817f1c3f>] ? fib_rules_unregister+0x30/0xb2 [<ffffffff817f1c8b>] fib_rules_unregister+0x7c/0xb2 [<ffffffff818f64a1>] fib4_rules_exit+0x15/0x18 [<ffffffff818e8c0a>] ip_fib_net_exit+0x23/0xf2 [<ffffffff818e91f8>] fib_net_exit+0x32/0x36 [<ffffffff817c8352>] ops_exit_list+0x45/0x57 [<ffffffff817c8d3d>] cleanup_net+0x13c/0x1cd [<ffffffff8108b05d>] process_one_work+0x255/0x4ad [<ffffffff8108af69>] ? process_one_work+0x161/0x4ad [<ffffffff8108b4b1>] worker_thread+0x1cd/0x2ab [<ffffffff8108b2e4>] ? process_scheduled_works+0x2f/0x2f [<ffffffff81090686>] kthread+0xd4/0xdc [<ffffffff8109ec8f>] ? local_clock+0x19/0x22 [<ffffffff810905b2>] ? __kthread_parkme+0x83/0x83 [<ffffffff81a2c0c8>] ret_from_fork+0x58/0x90 [<ffffffff810905b2>] ? __kthread_parkme+0x83/0x83 The issue was that as a part of exiting the default rules were being deleted which resulted in the local trie being unmerged. By moving the freeing of the FIB tables up we can avoid the unmerge since there is no local table left when we call the fib4_rules_exit function. Fixes: 0ddcf43d5d4a ("ipv4: FIB Local/MAIN table collapse") Reported-by: Cong Wang <xiyou.wangcong@gmail.com> Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com> --- net/ipv4/fib_frontend.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/net/ipv4/fib_frontend.c b/net/ipv4/fib_frontend.c index e5b6b0534c5f..767120111d90 100644 --- a/net/ipv4/fib_frontend.c +++ b/net/ipv4/fib_frontend.c @@ -1176,10 +1176,6 @@ static void ip_fib_net_exit(struct net *net) rtnl_lock(); -#ifdef CONFIG_IP_MULTIPLE_TABLES - fib4_rules_exit(net); -#endif - for (i = 0; i < FIB_TABLE_HASHSZ; i++) { struct hlist_head *head = &net->ipv4.fib_table_hash[i]; struct hlist_node *tmp; @@ -1212,6 +1208,10 @@ static void ip_fib_net_exit(struct net *net) fib_free_table(tb); } } + +#ifdef CONFIG_IP_MULTIPLE_TABLES + fib4_rules_exit(net); +#endif rtnl_unlock(); kfree(net->ipv4.fib_table_hash); } ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [net-next PATCH 2/2] fib_trie: Cleanup ip_fib_net_exit code path 2015-03-27 21:14 [net-next PATCH 1/2] fib_trie: Fix warning on fib4_rules_exit Alexander Duyck @ 2015-03-27 21:14 ` Alexander Duyck 2015-03-31 17:19 ` David Miller 2015-03-27 22:14 ` [net-next PATCH 1/2] fib_trie: Fix warning on fib4_rules_exit Cong Wang 2015-03-31 17:19 ` David Miller 2 siblings, 1 reply; 10+ messages in thread From: Alexander Duyck @ 2015-03-27 21:14 UTC (permalink / raw) To: netdev; +Cc: davem While fixing a recent issue I noticed that we are doing some unnecessary work inside the loop for ip_fib_net_exit. As such I am pulling out the initialization to NULL for the locally stored fib_local, fib_main, and fib_default. In addition I am restoring the original code for flushing the table as there is no need to split up the fib_table_flush and hlist_del work since the code for packing the tnodes with multiple key vectors was dropped. Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com> --- net/ipv4/fib_frontend.c | 29 +++++++---------------------- 1 file changed, 7 insertions(+), 22 deletions(-) diff --git a/net/ipv4/fib_frontend.c b/net/ipv4/fib_frontend.c index 767120111d90..718b0a16ea40 100644 --- a/net/ipv4/fib_frontend.c +++ b/net/ipv4/fib_frontend.c @@ -1176,35 +1176,20 @@ static void ip_fib_net_exit(struct net *net) rtnl_lock(); +#ifdef CONFIG_IP_MULTIPLE_TABLES + RCU_INIT_POINTER(net->ipv4.fib_local, NULL); + RCU_INIT_POINTER(net->ipv4.fib_main, NULL); + RCU_INIT_POINTER(net->ipv4.fib_default, NULL); +#endif + for (i = 0; i < FIB_TABLE_HASHSZ; i++) { struct hlist_head *head = &net->ipv4.fib_table_hash[i]; struct hlist_node *tmp; struct fib_table *tb; - /* this is done in two passes as flushing the table could - * cause it to be reallocated in order to accommodate new - * tnodes at the root as the table shrinks. - */ - hlist_for_each_entry_safe(tb, tmp, head, tb_hlist) - fib_table_flush(tb); - hlist_for_each_entry_safe(tb, tmp, head, tb_hlist) { -#ifdef CONFIG_IP_MULTIPLE_TABLES - switch (tb->tb_id) { - case RT_TABLE_LOCAL: - RCU_INIT_POINTER(net->ipv4.fib_local, NULL); - break; - case RT_TABLE_MAIN: - RCU_INIT_POINTER(net->ipv4.fib_main, NULL); - break; - case RT_TABLE_DEFAULT: - RCU_INIT_POINTER(net->ipv4.fib_default, NULL); - break; - default: - break; - } -#endif hlist_del(&tb->tb_hlist); + fib_table_flush(tb); fib_free_table(tb); } } ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [net-next PATCH 2/2] fib_trie: Cleanup ip_fib_net_exit code path 2015-03-27 21:14 ` [net-next PATCH 2/2] fib_trie: Cleanup ip_fib_net_exit code path Alexander Duyck @ 2015-03-31 17:19 ` David Miller 0 siblings, 0 replies; 10+ messages in thread From: David Miller @ 2015-03-31 17:19 UTC (permalink / raw) To: alexander.h.duyck; +Cc: netdev From: Alexander Duyck <alexander.h.duyck@redhat.com> Date: Fri, 27 Mar 2015 14:14:22 -0700 > While fixing a recent issue I noticed that we are doing some unnecessary > work inside the loop for ip_fib_net_exit. As such I am pulling out the > initialization to NULL for the locally stored fib_local, fib_main, and > fib_default. > > In addition I am restoring the original code for flushing the table as > there is no need to split up the fib_table_flush and hlist_del work since > the code for packing the tnodes with multiple key vectors was dropped. > > Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com> Applied. I also agree with you that we shouldn't try to get too fancy here as Cong is suggesting, lest ->delete() have other side effects in the future. Thanks. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [net-next PATCH 1/2] fib_trie: Fix warning on fib4_rules_exit 2015-03-27 21:14 [net-next PATCH 1/2] fib_trie: Fix warning on fib4_rules_exit Alexander Duyck 2015-03-27 21:14 ` [net-next PATCH 2/2] fib_trie: Cleanup ip_fib_net_exit code path Alexander Duyck @ 2015-03-27 22:14 ` Cong Wang 2015-03-27 23:29 ` Alexander Duyck 2015-03-31 17:19 ` David Miller 2 siblings, 1 reply; 10+ messages in thread From: Cong Wang @ 2015-03-27 22:14 UTC (permalink / raw) To: Alexander Duyck; +Cc: netdev, Cong Wang, David Miller On Fri, Mar 27, 2015 at 2:14 PM, Alexander Duyck <alexander.h.duyck@redhat.com> wrote: > > The issue was that as a part of exiting the default rules were being > deleted which resulted in the local trie being unmerged. By moving the > freeing of the FIB tables up we can avoid the unmerge since there is no > local table left when we call the fib4_rules_exit function. > This literally means we no longer need to call ops->delete() in netns unregister path. diff --git a/net/core/fib_rules.c b/net/core/fib_rules.c index 68ea695..27b6e04 100644 --- a/net/core/fib_rules.c +++ b/net/core/fib_rules.c @@ -153,8 +153,6 @@ static void fib_rules_cleanup_ops(struct fib_rules_ops *ops) list_for_each_entry_safe(rule, tmp, &ops->rules_list, list) { list_del_rcu(&rule->list); - if (ops->delete) - ops->delete(rule); fib_rule_put(rule); } } diff --git a/net/ipv4/fib_frontend.c b/net/ipv4/fib_frontend.c index e5b6b05..1481b23 100644 --- a/net/ipv4/fib_frontend.c +++ b/net/ipv4/fib_frontend.c @@ -1178,6 +1178,7 @@ static void ip_fib_net_exit(struct net *net) #ifdef CONFIG_IP_MULTIPLE_TABLES fib4_rules_exit(net); + fib_flush_external(net); // <-------- Maybe not needed either. #endif for (i = 0; i < FIB_TABLE_HASHSZ; i++) { ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [net-next PATCH 1/2] fib_trie: Fix warning on fib4_rules_exit 2015-03-27 22:14 ` [net-next PATCH 1/2] fib_trie: Fix warning on fib4_rules_exit Cong Wang @ 2015-03-27 23:29 ` Alexander Duyck 2015-03-30 18:54 ` Cong Wang 0 siblings, 1 reply; 10+ messages in thread From: Alexander Duyck @ 2015-03-27 23:29 UTC (permalink / raw) To: Cong Wang; +Cc: netdev, Cong Wang, David Miller On 03/27/2015 03:14 PM, Cong Wang wrote: > On Fri, Mar 27, 2015 at 2:14 PM, Alexander Duyck > <alexander.h.duyck@redhat.com> wrote: >> The issue was that as a part of exiting the default rules were being >> deleted which resulted in the local trie being unmerged. By moving the >> freeing of the FIB tables up we can avoid the unmerge since there is no >> local table left when we call the fib4_rules_exit function. >> > This literally means we no longer need to call ops->delete() > in netns unregister path. You are confusing table entries and rules. The tables are cleared, the rules still have to be deleted. This patch breaks the reference counting for fib_num_tclassid_users. > > diff --git a/net/core/fib_rules.c b/net/core/fib_rules.c > index 68ea695..27b6e04 100644 > --- a/net/core/fib_rules.c > +++ b/net/core/fib_rules.c > @@ -153,8 +153,6 @@ static void fib_rules_cleanup_ops(struct fib_rules_ops *ops) > > list_for_each_entry_safe(rule, tmp, &ops->rules_list, list) { > list_del_rcu(&rule->list); > - if (ops->delete) > - ops->delete(rule); > fib_rule_put(rule); > } > } > diff --git a/net/ipv4/fib_frontend.c b/net/ipv4/fib_frontend.c > index e5b6b05..1481b23 100644 > --- a/net/ipv4/fib_frontend.c > +++ b/net/ipv4/fib_frontend.c > @@ -1178,6 +1178,7 @@ static void ip_fib_net_exit(struct net *net) > > #ifdef CONFIG_IP_MULTIPLE_TABLES > fib4_rules_exit(net); > + fib_flush_external(net); // <-------- Maybe not needed either. > #endif > > for (i = 0; i < FIB_TABLE_HASHSZ; i++) { Take a look at fib4_rule_delete. There is more there than just unmerge and flush external. - Alex ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [net-next PATCH 1/2] fib_trie: Fix warning on fib4_rules_exit 2015-03-27 23:29 ` Alexander Duyck @ 2015-03-30 18:54 ` Cong Wang 2015-03-30 20:24 ` Alexander Duyck 0 siblings, 1 reply; 10+ messages in thread From: Cong Wang @ 2015-03-30 18:54 UTC (permalink / raw) To: Alexander Duyck; +Cc: netdev, Cong Wang, David Miller On Fri, Mar 27, 2015 at 4:29 PM, Alexander Duyck <alexander.h.duyck@redhat.com> wrote: > > On 03/27/2015 03:14 PM, Cong Wang wrote: >> >> On Fri, Mar 27, 2015 at 2:14 PM, Alexander Duyck >> <alexander.h.duyck@redhat.com> wrote: >>> >>> The issue was that as a part of exiting the default rules were being >>> deleted which resulted in the local trie being unmerged. By moving the >>> freeing of the FIB tables up we can avoid the unmerge since there is no >>> local table left when we call the fib4_rules_exit function. >>> >> This literally means we no longer need to call ops->delete() >> in netns unregister path. > > > You are confusing table entries and rules. The tables are cleared, the > rules still have to be deleted. This patch breaks the reference counting > for fib_num_tclassid_users. It doesn't matter much here, the whole net is being unregistered, we are holding rtnl lock and existing readers don't mind to read an incorrect fib_num_tclassid_users. > >> >> diff --git a/net/core/fib_rules.c b/net/core/fib_rules.c >> index 68ea695..27b6e04 100644 >> --- a/net/core/fib_rules.c >> +++ b/net/core/fib_rules.c >> @@ -153,8 +153,6 @@ static void fib_rules_cleanup_ops(struct fib_rules_ops >> *ops) >> >> list_for_each_entry_safe(rule, tmp, &ops->rules_list, list) { >> list_del_rcu(&rule->list); >> - if (ops->delete) >> - ops->delete(rule); >> fib_rule_put(rule); >> } >> } >> diff --git a/net/ipv4/fib_frontend.c b/net/ipv4/fib_frontend.c >> index e5b6b05..1481b23 100644 >> --- a/net/ipv4/fib_frontend.c >> +++ b/net/ipv4/fib_frontend.c >> @@ -1178,6 +1178,7 @@ static void ip_fib_net_exit(struct net *net) >> >> #ifdef CONFIG_IP_MULTIPLE_TABLES >> fib4_rules_exit(net); >> + fib_flush_external(net); // <-------- Maybe not needed either. >> #endif >> >> for (i = 0; i < FIB_TABLE_HASHSZ; i++) { > > > Take a look at fib4_rule_delete. There is more there than just unmerge and > flush external. I am not stupid, what otherwise do you think the above fib_flush_external() comes from? Read fib4_rule_delete(), everything it cleans up is per net, you can argue ipv4.fib_num_tclassid_users is a refcount for rules, but still the whole net is being unregistered. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [net-next PATCH 1/2] fib_trie: Fix warning on fib4_rules_exit 2015-03-30 18:54 ` Cong Wang @ 2015-03-30 20:24 ` Alexander Duyck 2015-03-30 20:50 ` Cong Wang 0 siblings, 1 reply; 10+ messages in thread From: Alexander Duyck @ 2015-03-30 20:24 UTC (permalink / raw) To: Cong Wang; +Cc: netdev, Cong Wang, David Miller On 03/30/2015 11:54 AM, Cong Wang wrote: > On Fri, Mar 27, 2015 at 4:29 PM, Alexander Duyck > <alexander.h.duyck@redhat.com> wrote: >> On 03/27/2015 03:14 PM, Cong Wang wrote: >>> On Fri, Mar 27, 2015 at 2:14 PM, Alexander Duyck >>> <alexander.h.duyck@redhat.com> wrote: >>>> The issue was that as a part of exiting the default rules were being >>>> deleted which resulted in the local trie being unmerged. By moving the >>>> freeing of the FIB tables up we can avoid the unmerge since there is no >>>> local table left when we call the fib4_rules_exit function. >>>> >>> This literally means we no longer need to call ops->delete() >>> in netns unregister path. >> >> You are confusing table entries and rules. The tables are cleared, the >> rules still have to be deleted. This patch breaks the reference counting >> for fib_num_tclassid_users. > It doesn't matter much here, the whole net is being unregistered, > we are holding rtnl lock and existing readers don't mind to read an incorrect > fib_num_tclassid_users. Still best not to mess with this. For the sake of completeness if we have delete implemented it should be called. >>> diff --git a/net/core/fib_rules.c b/net/core/fib_rules.c >>> index 68ea695..27b6e04 100644 >>> --- a/net/core/fib_rules.c >>> +++ b/net/core/fib_rules.c >>> @@ -153,8 +153,6 @@ static void fib_rules_cleanup_ops(struct fib_rules_ops >>> *ops) >>> >>> list_for_each_entry_safe(rule, tmp, &ops->rules_list, list) { >>> list_del_rcu(&rule->list); >>> - if (ops->delete) >>> - ops->delete(rule); >>> fib_rule_put(rule); >>> } >>> } >>> diff --git a/net/ipv4/fib_frontend.c b/net/ipv4/fib_frontend.c >>> index e5b6b05..1481b23 100644 >>> --- a/net/ipv4/fib_frontend.c >>> +++ b/net/ipv4/fib_frontend.c >>> @@ -1178,6 +1178,7 @@ static void ip_fib_net_exit(struct net *net) >>> >>> #ifdef CONFIG_IP_MULTIPLE_TABLES >>> fib4_rules_exit(net); >>> + fib_flush_external(net); // <-------- Maybe not needed either. >>> #endif >>> >>> for (i = 0; i < FIB_TABLE_HASHSZ; i++) { >> >> Take a look at fib4_rule_delete. There is more there than just unmerge and >> flush external. > I am not stupid, what otherwise do you think the above fib_flush_external() > comes from? The fact is you are choosing to overlook things that will lead to issues, if not now, then later, and as a result make the code more difficult to maintain. It isn't as if this is hot-path code so there isn't any need to optimize it by dropping these calls. > Read fib4_rule_delete(), everything it cleans up is per net, you can argue > ipv4.fib_num_tclassid_users is a refcount for rules, but still the whole net > is being unregistered. Yes, the delete call will likely not do much more than update ipv4.fib_num_tclassid_users, but having that value updated until we free the net structure is useful for things like debugging. If anything it would be useful to go through and audit the other users to make sure they are all following a similar pattern. From what I can tell ip6mr_rules_exit is already in the same layout, and the same goes for ipmr_rules_exit though each is dealing with the RTNL lock differently. Your efforts would be much better placed there than trying to alter code that really should be left as-is for completeness. - Alex ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [net-next PATCH 1/2] fib_trie: Fix warning on fib4_rules_exit 2015-03-30 20:24 ` Alexander Duyck @ 2015-03-30 20:50 ` Cong Wang 2015-03-30 21:58 ` Alexander Duyck 0 siblings, 1 reply; 10+ messages in thread From: Cong Wang @ 2015-03-30 20:50 UTC (permalink / raw) To: Alexander Duyck; +Cc: netdev, Cong Wang, David Miller On Mon, Mar 30, 2015 at 1:24 PM, Alexander Duyck <alexander.h.duyck@redhat.com> wrote: > > On 03/30/2015 11:54 AM, Cong Wang wrote: >> >> On Fri, Mar 27, 2015 at 4:29 PM, Alexander Duyck >> <alexander.h.duyck@redhat.com> wrote: >>> >>> On 03/27/2015 03:14 PM, Cong Wang wrote: >>>> >>>> On Fri, Mar 27, 2015 at 2:14 PM, Alexander Duyck >>>> <alexander.h.duyck@redhat.com> wrote: >>>>> >>>>> The issue was that as a part of exiting the default rules were being >>>>> deleted which resulted in the local trie being unmerged. By moving the >>>>> freeing of the FIB tables up we can avoid the unmerge since there is no >>>>> local table left when we call the fib4_rules_exit function. >>>>> >>>> This literally means we no longer need to call ops->delete() >>>> in netns unregister path. >>> >>> >>> You are confusing table entries and rules. The tables are cleared, the >>> rules still have to be deleted. This patch breaks the reference counting >>> for fib_num_tclassid_users. >> >> It doesn't matter much here, the whole net is being unregistered, >> we are holding rtnl lock and existing readers don't mind to read an >> incorrect >> fib_num_tclassid_users. > > > Still best not to mess with this. For the sake of completeness if we have > delete implemented it should be called. > If it is to delete per-rule stuffs, of course yes. But it is to delete per-net stuffs, and this is the whole point. > > >>>> diff --git a/net/core/fib_rules.c b/net/core/fib_rules.c >>>> index 68ea695..27b6e04 100644 >>>> --- a/net/core/fib_rules.c >>>> +++ b/net/core/fib_rules.c >>>> @@ -153,8 +153,6 @@ static void fib_rules_cleanup_ops(struct >>>> fib_rules_ops >>>> *ops) >>>> >>>> list_for_each_entry_safe(rule, tmp, &ops->rules_list, list) { >>>> list_del_rcu(&rule->list); >>>> - if (ops->delete) >>>> - ops->delete(rule); >>>> fib_rule_put(rule); >>>> } >>>> } >>>> diff --git a/net/ipv4/fib_frontend.c b/net/ipv4/fib_frontend.c >>>> index e5b6b05..1481b23 100644 >>>> --- a/net/ipv4/fib_frontend.c >>>> +++ b/net/ipv4/fib_frontend.c >>>> @@ -1178,6 +1178,7 @@ static void ip_fib_net_exit(struct net *net) >>>> >>>> #ifdef CONFIG_IP_MULTIPLE_TABLES >>>> fib4_rules_exit(net); >>>> + fib_flush_external(net); // <-------- Maybe not needed either. >>>> #endif >>>> >>>> for (i = 0; i < FIB_TABLE_HASHSZ; i++) { >>> >>> >>> Take a look at fib4_rule_delete. There is more there than just unmerge >>> and >>> flush external. >> >> I am not stupid, what otherwise do you think the above >> fib_flush_external() >> comes from? > > > The fact is you are choosing to overlook things that will lead to issues, if > not now, then later, and as a result make the code more difficult to > maintain. It isn't as if this is hot-path code so there isn't any need to > optimize it by dropping these calls. Who said it is for optimization? It is just _logically_ not needed, that is all, please don't interrupt me too far beyond the point. > >> Read fib4_rule_delete(), everything it cleans up is per net, you can argue >> ipv4.fib_num_tclassid_users is a refcount for rules, but still the whole >> net >> is being unregistered. > > > Yes, the delete call will likely not do much more than update > ipv4.fib_num_tclassid_users, but having that value updated until we free the > net structure is useful for things like debugging. > > If anything it would be useful to go through and audit the other users to > make sure they are all following a similar pattern. From what I can tell As you said, only ipv4 fib has ops->delete(), I don't understand why others should follow. > ip6mr_rules_exit is already in the same layout, and the same goes for > ipmr_rules_exit though each is dealing with the RTNL lock differently. Your > efforts would be much better placed there than trying to alter code that > really should be left as-is for completeness. > They should be same with regarding to RTNL, I already sent a patch: http://permalink.gmane.org/gmane.linux.network/356700 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [net-next PATCH 1/2] fib_trie: Fix warning on fib4_rules_exit 2015-03-30 20:50 ` Cong Wang @ 2015-03-30 21:58 ` Alexander Duyck 0 siblings, 0 replies; 10+ messages in thread From: Alexander Duyck @ 2015-03-30 21:58 UTC (permalink / raw) To: Cong Wang; +Cc: netdev, Cong Wang, David Miller On 03/30/2015 01:50 PM, Cong Wang wrote: > On Mon, Mar 30, 2015 at 1:24 PM, Alexander Duyck > <alexander.h.duyck@redhat.com> wrote: >> On 03/30/2015 11:54 AM, Cong Wang wrote: >>> On Fri, Mar 27, 2015 at 4:29 PM, Alexander Duyck >>> <alexander.h.duyck@redhat.com> wrote: >>>> On 03/27/2015 03:14 PM, Cong Wang wrote: >>>>> On Fri, Mar 27, 2015 at 2:14 PM, Alexander Duyck >>>>> <alexander.h.duyck@redhat.com> wrote: >>>>>> The issue was that as a part of exiting the default rules were being >>>>>> deleted which resulted in the local trie being unmerged. By moving the >>>>>> freeing of the FIB tables up we can avoid the unmerge since there is no >>>>>> local table left when we call the fib4_rules_exit function. >>>>>> >>>>> This literally means we no longer need to call ops->delete() >>>>> in netns unregister path. >>>> >>>> You are confusing table entries and rules. The tables are cleared, the >>>> rules still have to be deleted. This patch breaks the reference counting >>>> for fib_num_tclassid_users. >>> It doesn't matter much here, the whole net is being unregistered, >>> we are holding rtnl lock and existing readers don't mind to read an >>> incorrect >>> fib_num_tclassid_users. >> >> Still best not to mess with this. For the sake of completeness if we have >> delete implemented it should be called. >> > > If it is to delete per-rule stuffs, of course yes. But it is to delete per-net > stuffs, and this is the whole point. The interface is much cleaner without duplicating code. What you are proposing means we have to periodically check the delete function in case anything else gets added to it which is highly possible since switchdev and the fib_trie have been fairly active lately and there are likely to be more changes. Please, just leave the delete function being called in fib_rules_cleanup_ops as is. > >> >>>>> diff --git a/net/core/fib_rules.c b/net/core/fib_rules.c >>>>> index 68ea695..27b6e04 100644 >>>>> --- a/net/core/fib_rules.c >>>>> +++ b/net/core/fib_rules.c >>>>> @@ -153,8 +153,6 @@ static void fib_rules_cleanup_ops(struct >>>>> fib_rules_ops >>>>> *ops) >>>>> >>>>> list_for_each_entry_safe(rule, tmp, &ops->rules_list, list) { >>>>> list_del_rcu(&rule->list); >>>>> - if (ops->delete) >>>>> - ops->delete(rule); >>>>> fib_rule_put(rule); >>>>> } >>>>> } >>>>> diff --git a/net/ipv4/fib_frontend.c b/net/ipv4/fib_frontend.c >>>>> index e5b6b05..1481b23 100644 >>>>> --- a/net/ipv4/fib_frontend.c >>>>> +++ b/net/ipv4/fib_frontend.c >>>>> @@ -1178,6 +1178,7 @@ static void ip_fib_net_exit(struct net *net) >>>>> >>>>> #ifdef CONFIG_IP_MULTIPLE_TABLES >>>>> fib4_rules_exit(net); >>>>> + fib_flush_external(net); // <-------- Maybe not needed either. >>>>> #endif >>>>> >>>>> for (i = 0; i < FIB_TABLE_HASHSZ; i++) { >>>> >>>> Take a look at fib4_rule_delete. There is more there than just unmerge >>>> and >>>> flush external. >>> I am not stupid, what otherwise do you think the above >>> fib_flush_external() >>> comes from? >> >> The fact is you are choosing to overlook things that will lead to issues, if >> not now, then later, and as a result make the code more difficult to >> maintain. It isn't as if this is hot-path code so there isn't any need to >> optimize it by dropping these calls. > > Who said it is for optimization? > > It is just _logically_ not needed, that is all, please don't interrupt > me too far > beyond the point. The thing is it is doing something useful. You don't agree, but it needs to be there for completeness and to reduce unnecessary duplication of code. > >>> Read fib4_rule_delete(), everything it cleans up is per net, you can argue >>> ipv4.fib_num_tclassid_users is a refcount for rules, but still the whole >>> net >>> is being unregistered. >> >> Yes, the delete call will likely not do much more than update >> ipv4.fib_num_tclassid_users, but having that value updated until we free the >> net structure is useful for things like debugging. >> >> If anything it would be useful to go through and audit the other users to >> make sure they are all following a similar pattern. From what I can tell > > As you said, only ipv4 fib has ops->delete(), I don't understand why > others should follow. Because it is a necessary bit if there is any other objects that are added per rule. Currently there aren't but there could be so please just leave it as is. If nothing else at this point it is taking care of reference counting. >> ip6mr_rules_exit is already in the same layout, and the same goes for >> ipmr_rules_exit though each is dealing with the RTNL lock differently. Your >> efforts would be much better placed there than trying to alter code that >> really should be left as-is for completeness. >> > They should be same with regarding to RTNL, I already sent a patch: > > http://permalink.gmane.org/gmane.linux.network/356700 The patch set is listed as "Changes Requested" in Dave's patchwork queue. https://patchwork.ozlabs.org/patch/454744/ My advice for the patch would be to look at moving the rtnl_unlock down one line in ip6mr_rules_exit and your patch so that it includes the fib_rules_unregister call in the RTNL locked region. While you are at it you could probably also wrap any other callers to fib_rules_unregister where it is called as a part of the exit path such as fib6_rules_net_exit and dn_fib_rules_cleanup. - Alex ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [net-next PATCH 1/2] fib_trie: Fix warning on fib4_rules_exit 2015-03-27 21:14 [net-next PATCH 1/2] fib_trie: Fix warning on fib4_rules_exit Alexander Duyck 2015-03-27 21:14 ` [net-next PATCH 2/2] fib_trie: Cleanup ip_fib_net_exit code path Alexander Duyck 2015-03-27 22:14 ` [net-next PATCH 1/2] fib_trie: Fix warning on fib4_rules_exit Cong Wang @ 2015-03-31 17:19 ` David Miller 2 siblings, 0 replies; 10+ messages in thread From: David Miller @ 2015-03-31 17:19 UTC (permalink / raw) To: alexander.h.duyck; +Cc: netdev, xiyou.wangcong From: Alexander Duyck <alexander.h.duyck@redhat.com> Date: Fri, 27 Mar 2015 14:14:16 -0700 > This fixes the following warning: ... > The issue was that as a part of exiting the default rules were being > deleted which resulted in the local trie being unmerged. By moving the > freeing of the FIB tables up we can avoid the unmerge since there is no > local table left when we call the fib4_rules_exit function. > > Fixes: 0ddcf43d5d4a ("ipv4: FIB Local/MAIN table collapse") > Reported-by: Cong Wang <xiyou.wangcong@gmail.com> > Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com> Applied. ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2015-03-31 17:19 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-03-27 21:14 [net-next PATCH 1/2] fib_trie: Fix warning on fib4_rules_exit Alexander Duyck 2015-03-27 21:14 ` [net-next PATCH 2/2] fib_trie: Cleanup ip_fib_net_exit code path Alexander Duyck 2015-03-31 17:19 ` David Miller 2015-03-27 22:14 ` [net-next PATCH 1/2] fib_trie: Fix warning on fib4_rules_exit Cong Wang 2015-03-27 23:29 ` Alexander Duyck 2015-03-30 18:54 ` Cong Wang 2015-03-30 20:24 ` Alexander Duyck 2015-03-30 20:50 ` Cong Wang 2015-03-30 21:58 ` Alexander Duyck 2015-03-31 17:19 ` David Miller
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.