| Message ID | CAHA+R7McwZQ5PCWFvUh-wrktg3AP0tu7arkcp=+SFmiD5apRBQ@mail.gmail.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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++) {