[net-next PATCH 1/2] fib_trie: Fix warning on fib4_rules_exit

[net-next PATCH 1/2] fib_trie: Fix warning on fib4_rules_exit

[Alexander Duyck]

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);
 }

[net-next PATCH 2/2] fib_trie: Cleanup ip_fib_net_exit code path

[Alexander Duyck]

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);
 		}
 	}

Re: [net-next PATCH 2/2] fib_trie: Cleanup ip_fib_net_exit code path

[David Miller]

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.

Re: [net-next PATCH 1/2] fib_trie: Fix warning on fib4_rules_exit

[Cong Wang]

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++) {

Re: [net-next PATCH 1/2] fib_trie: Fix warning on fib4_rules_exit

[Alexander Duyck]

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

Re: [net-next PATCH 1/2] fib_trie: Fix warning on fib4_rules_exit

[Cong Wang]

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.

Re: [net-next PATCH 1/2] fib_trie: Fix warning on fib4_rules_exit

[Alexander Duyck]

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

Re: [net-next PATCH 1/2] fib_trie: Fix warning on fib4_rules_exit

[Cong Wang]

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

Re: [net-next PATCH 1/2] fib_trie: Fix warning on fib4_rules_exit

[Alexander Duyck]

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

Re: [net-next PATCH 1/2] fib_trie: Fix warning on fib4_rules_exit

[David Miller]

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.