| Message ID | 4F588BEC020000780007710A@nat28.tlf.novell.com |
|---|---|
| State | Rejected, archived |
| Delegated to: | David Miller |
| Headers | show |
From: "Jan Beulich" <JBeulich@suse.com> Date: Thu, 08 Mar 2012 09:37:32 +0000 > __net_exit, judging by the majority of its uses, was intended to serve > as an abstraction to allow calling such annotated functions from both > __init and __exit functions. Using the (bogus and unused elsewhere) > __exit_refok to implement this is inefficient - any non-modular code > really can reside in __init (as non-modular __exit code is never used). > > Therefore, adjust __net_exit to resolve to nothing (i.e. normal .text) > in modules, and __init in the core kernel. > > A few other adjustments are necessary/possible with this done - those > were likely just oversights when added originally. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> [ I have been waiting for more than a week for a netns developer to review this patch, I guess I'm too optimistic these days. :-( ] The only reason you think __exit_refok is "bogus" is because it's semantics got changed by Sam Ravnborg in commit 312b1485fb509c9bc32eda28ad29537896658cb8 ("Introduce new section reference annotations tags: __ref, __refdata, __refconst") Beforehand the __exit_refok was a real .exit section, so it got completely discarded AT LINK TIME. Now it sits together with __init_refok which is an unremovable kernel image section, which neither gets removed at compile time nor boot time. So __exit_refok did exactly what you say it should do before Sam's change. It's just completely stupid to change the netns section defines, and instead we should revert __exit_refok to mean what it always meant previously. I'm not applying this patch. -- 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 16, 2012 at 10:18:13PM -0700, David Miller wrote: > From: "Jan Beulich" <JBeulich@suse.com> > Date: Thu, 08 Mar 2012 09:37:32 +0000 > > > __net_exit, judging by the majority of its uses, was intended to serve > > as an abstraction to allow calling such annotated functions from both > > __init and __exit functions. Using the (bogus and unused elsewhere) > > __exit_refok to implement this is inefficient - any non-modular code > > really can reside in __init (as non-modular __exit code is never used). > > > > Therefore, adjust __net_exit to resolve to nothing (i.e. normal .text) > > in modules, and __init in the core kernel. > > > > A few other adjustments are necessary/possible with this done - those > > were likely just oversights when added originally. > > > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > [ I have been waiting for more than a week for a netns developer > to review this patch, I guess I'm too optimistic these days. :-( ] > > The only reason you think __exit_refok is "bogus" is because it's > semantics got changed by Sam Ravnborg in commit > 312b1485fb509c9bc32eda28ad29537896658cb8 ("Introduce new section > reference annotations tags: __ref, __refdata, __refconst") > > Beforehand the __exit_refok was a real .exit section, so it got > completely discarded AT LINK TIME. Now it sits together with > __init_refok which is an unremovable kernel image section, which > neither gets removed at compile time nor boot time. Some misunderstanding is going on here. The *ref* annotation is used to teach modpost that this function (or data) may reference functions (or data) which is annotated __init*. And the *ref* annotation never caused the annotated code to be discarded. This was true before the above mentioned patch - and it is still true. Before the path ("Introduce new section reference ....") the __exit_refok annotation moved functions to the section named ".exit.text.refok" which was explicit part of .text (TEXT_TEXT in vmlinux). So __exit_refok does exactly what it is intended to do: It puts the function in a section so modpost does not warn about references to __init or __exit sections. As Jan points out there is only a single user left - so this would be a good time to kill it. It is even documented in init.h that this is a backward compatibility define. The intention with Jan's patch is to move functions annotated __net_exit to a discardable section in the core kernel. Then at least __exit should be used - there is no logic using __init for exit code. I suggest to: 1) fix the patch to use __exit 2) fix up the bogus commit message Then it should be OK - iff the assumption hold that the functions can be discarded in the core kernel. I have grepped a little and saw no uses where this did not hold true. So based on this I assume the assumption is OK. Sam -- 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
Sam Ravnborg <sam@ravnborg.org> writes: > On Fri, Mar 16, 2012 at 10:18:13PM -0700, David Miller wrote: >> From: "Jan Beulich" <JBeulich@suse.com> >> Date: Thu, 08 Mar 2012 09:37:32 +0000 >> >> > __net_exit, judging by the majority of its uses, was intended to serve >> > as an abstraction to allow calling such annotated functions from both >> > __init and __exit functions. Using the (bogus and unused elsewhere) >> > __exit_refok to implement this is inefficient - any non-modular code >> > really can reside in __init (as non-modular __exit code is never used). >> > >> > Therefore, adjust __net_exit to resolve to nothing (i.e. normal .text) >> > in modules, and __init in the core kernel. >> > >> > A few other adjustments are necessary/possible with this done - those >> > were likely just oversights when added originally. >> > >> > Signed-off-by: Jan Beulich <jbeulich@suse.com> >> >> [ I have been waiting for more than a week for a netns developer >> to review this patch, I guess I'm too optimistic these days. :-( ] >> >> The only reason you think __exit_refok is "bogus" is because it's >> semantics got changed by Sam Ravnborg in commit >> 312b1485fb509c9bc32eda28ad29537896658cb8 ("Introduce new section >> reference annotations tags: __ref, __refdata, __refconst") >> >> Beforehand the __exit_refok was a real .exit section, so it got >> completely discarded AT LINK TIME. Now it sits together with >> __init_refok which is an unremovable kernel image section, which >> neither gets removed at compile time nor boot time. > > Some misunderstanding is going on here. > > The *ref* annotation is used to teach modpost that this function (or data) > may reference functions (or data) which is annotated __init*. > And the *ref* annotation never caused the annotated code to be discarded. > > This was true before the above mentioned patch - and it is still true. > > Before the path ("Introduce new section reference ....") the __exit_refok > annotation moved functions to the section named ".exit.text.refok" > which was explicit part of .text (TEXT_TEXT in vmlinux). > > So __exit_refok does exactly what it is intended to do: > It puts the function in a section so modpost does not warn about > references to __init or __exit sections. > > As Jan points out there is only a single user left - so > this would be a good time to kill it. It is even documented > in init.h that this is a backward compatibility define. Strange. > > The intention with Jan's patch is to move functions annotated > __net_exit to a discardable section in the core kernel. > Then at least __exit should be used - there is no logic > using __init for exit code. > > I suggest to: > 1) fix the patch to use __exit > 2) fix up the bogus commit message Those two sound reasonable to me. The purpose of __net_exit is to mark code that can be __exit if we don't enable network namespace support. Looking at the original code it appears that __exit was wrong because we had __init sections referring to the __exit code and that was causing modpost to complain. Now that the definition of __exit_refok has changed half a dozen times since it was introduced I don't know what will happen. Since this really isn't about network namespaces but what to do when the network namespace code is disabled I'm going to bow out now. > Then it should be OK - iff the assumption hold that the functions > can be discarded in the core kernel. > I have grepped a little and saw no uses where this did not hold true. > So based on this I assume the assumption is OK. Eric -- 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 18.03.12 at 00:03, Sam Ravnborg <sam@ravnborg.org> wrote: > On Fri, Mar 16, 2012 at 10:18:13PM -0700, David Miller wrote: >> From: "Jan Beulich" <JBeulich@suse.com> >> Date: Thu, 08 Mar 2012 09:37:32 +0000 >> >> > __net_exit, judging by the majority of its uses, was intended to serve >> > as an abstraction to allow calling such annotated functions from both >> > __init and __exit functions. Using the (bogus and unused elsewhere) >> > __exit_refok to implement this is inefficient - any non-modular code >> > really can reside in __init (as non-modular __exit code is never used). >> > >> > Therefore, adjust __net_exit to resolve to nothing (i.e. normal .text) >> > in modules, and __init in the core kernel. >> > >> > A few other adjustments are necessary/possible with this done - those >> > were likely just oversights when added originally. >> > >> > Signed-off-by: Jan Beulich <jbeulich@suse.com> >> >> [ I have been waiting for more than a week for a netns developer >> to review this patch, I guess I'm too optimistic these days. :-( ] >> >> The only reason you think __exit_refok is "bogus" is because it's >> semantics got changed by Sam Ravnborg in commit >> 312b1485fb509c9bc32eda28ad29537896658cb8 ("Introduce new section >> reference annotations tags: __ref, __refdata, __refconst") >> >> Beforehand the __exit_refok was a real .exit section, so it got >> completely discarded AT LINK TIME. Now it sits together with >> __init_refok which is an unremovable kernel image section, which >> neither gets removed at compile time nor boot time. > > Some misunderstanding is going on here. > > The *ref* annotation is used to teach modpost that this function (or data) > may reference functions (or data) which is annotated __init*. > And the *ref* annotation never caused the annotated code to be discarded. > > This was true before the above mentioned patch - and it is still true. > > Before the path ("Introduce new section reference ....") the __exit_refok > annotation moved functions to the section named ".exit.text.refok" > which was explicit part of .text (TEXT_TEXT in vmlinux). > > So __exit_refok does exactly what it is intended to do: > It puts the function in a section so modpost does not warn about > references to __init or __exit sections. > > As Jan points out there is only a single user left - so > this would be a good time to kill it. It is even documented > in init.h that this is a backward compatibility define. > > The intention with Jan's patch is to move functions annotated > __net_exit to a discardable section in the core kernel. > Then at least __exit should be used - there is no logic > using __init for exit code. > > I suggest to: > 1) fix the patch to use __exit That won't work, as the goal is to have the exit functions call from __net_init iiuc. > 2) fix up the bogus commit message Bogus in what way? > Then it should be OK - iff the assumption hold that the functions > can be discarded in the core kernel. Since __exit functions won't ever be called when built in, they can as well be marked __init and thus become possible to call from __init code (again iiuc). Perhaps this should even become a concept outside of net/, as not marking exit code __exit just because is also gets called on error paths from __init is not uncommon. Maybe it should even be __exit itself to get changed that way (albeit I vaguely recall that differences between arches of when .exit.* sections get discarded might need further consideration), as long as no arch would get penalized in that it discards .exit.* but not .init.* post-boot. Jan > I have grepped a little and saw no uses where this did not hold true. > So based on this I assume the assumption is OK. > > Sam -- 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
> >> > Using the (bogus and unused elsewhere) > >> > __exit_refok to implement this is inefficient - any non-modular code > >> > really can reside in __init (as non-modular __exit code is never used). > > 2) fix up the bogus commit message > > Bogus in what way? I fail to see that __exit_refok is bogus. The name is not optimal - which is why is is deprecated. Sam -- 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 19.03.12 at 09:47, Sam Ravnborg <sam@ravnborg.org> wrote: >> >> > Using the (bogus and unused elsewhere) >> >> > __exit_refok to implement this is inefficient - any non-modular code >> >> > really can reside in __init (as non-modular __exit code is never used). > >> > 2) fix up the bogus commit message >> >> Bogus in what way? > > I fail to see that __exit_refok is bogus. Either an exit function references only permitted code (in which case it is legitimately using __exit), or it references non-__exit code, in which case it shouldn't have an override at all. Permitting an exit function to reference e.g. __init code is suspicious (at best) in all cases I can think of (in contrast to allowing a non-init function to reference __init code on a code path that can be proven to be taken only during system startup). Jan > The name is not optimal - which is why is is deprecated. > > Sam -- 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
"Jan Beulich" <JBeulich@suse.com> writes: >>>> On 19.03.12 at 09:47, Sam Ravnborg <sam@ravnborg.org> wrote: >>> >> > Using the (bogus and unused elsewhere) >>> >> > __exit_refok to implement this is inefficient - any non-modular code >>> >> > really can reside in __init (as non-modular __exit code is never used). >> >>> > 2) fix up the bogus commit message >>> >>> Bogus in what way? >> >> I fail to see that __exit_refok is bogus. > > Either an exit function references only permitted code (in which case > it is legitimately using __exit), or it references non-__exit code, in > which case it shouldn't have an override at all. Permitting an exit > function to reference e.g. __init code is suspicious (at best) in all > cases I can think of (in contrast to allowing a non-init function to > reference __init code on a code path that can be proven to be taken > only during system startup). My apologies I said that there was nothing network namespace specific left to talk about and I had goofed in my analysis. The weirdness with references comes from how struct pernet_operations is used. Structures of type struct pernet_operations hold holds pointers to .init functions marked as __net_init and .exit and .exit_batch marked as __net_exit. The assertion is that references from static instances of struct pernet_operations that live in __net_initdata should be ok. Structures of type pernet_operations are passed to register_pernet_subsys or register_pernet_device which ultimately call register_pernet_operations. Register pernet_operations when the network namespace is disabled simply calls the .init function when the network namespace code is compiled out. The .exit and .exit_batch functions are not referenced at all. When the network namespace code is compiled in __net_init __net_exit and __net_initdata all go away. So the exit function references only permitted code. But the exit function is being referenced by data in an init section which requires the override. How should we mark functions in __net_exit that can be thrown away when outside of a module and never even loaded when compiled into the kernel when the network namespace is disabled? The original definition of __exit_refok seemed to say this very clearly to me. This is an __exit section but references to it from __init data are ok. Eric -- 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
--- 3.3-rc6/include/net/net_namespace.h +++ 3.3-rc6-net-exit/include/net/net_namespace.h @@ -242,7 +242,11 @@ static inline struct net *read_pnet(stru #define __net_initdata #else #define __net_init __init -#define __net_exit __exit_refok +#ifdef MODULE +#define __net_exit +#else +#define __net_exit __init +#endif #define __net_initdata __initdata #endif --- 3.3-rc6/net/ipv4/fib_frontend.c +++ 3.3-rc6-net-exit/net/ipv4/fib_frontend.c @@ -1062,7 +1062,7 @@ fail: return err; } -static void ip_fib_net_exit(struct net *net) +static void __net_exit ip_fib_net_exit(struct net *net) { unsigned int i; --- 3.3-rc6/net/netfilter/ipvs/ip_vs_ctl.c +++ 3.3-rc6-net-exit/net/netfilter/ipvs/ip_vs_ctl.c @@ -3621,7 +3621,7 @@ static void ip_vs_genl_unregister(void) * per netns intit/exit func. */ #ifdef CONFIG_SYSCTL -int __net_init ip_vs_control_net_init_sysctl(struct net *net) +static int __net_init ip_vs_control_net_init_sysctl(struct net *net) { int idx; struct netns_ipvs *ipvs = net_ipvs(net); @@ -3680,7 +3680,7 @@ int __net_init ip_vs_control_net_init_sy return 0; } -void __net_init ip_vs_control_net_cleanup_sysctl(struct net *net) +static void __net_exit ip_vs_control_net_cleanup_sysctl(struct net *net) { struct netns_ipvs *ipvs = net_ipvs(net); @@ -3691,8 +3691,8 @@ void __net_init ip_vs_control_net_cleanu #else -int __net_init ip_vs_control_net_init_sysctl(struct net *net) { return 0; } -void __net_init ip_vs_control_net_cleanup_sysctl(struct net *net) { } +static int __net_init ip_vs_control_net_init_sysctl(struct net *net) { return 0; } +static void __net_exit ip_vs_control_net_cleanup_sysctl(struct net *net) { } #endif
__net_exit, judging by the majority of its uses, was intended to serve as an abstraction to allow calling such annotated functions from both __init and __exit functions. Using the (bogus and unused elsewhere) __exit_refok to implement this is inefficient - any non-modular code really can reside in __init (as non-modular __exit code is never used). Therefore, adjust __net_exit to resolve to nothing (i.e. normal .text) in modules, and __init in the core kernel. A few other adjustments are necessary/possible with this done - those were likely just oversights when added originally. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- include/net/net_namespace.h | 6 +++++- net/ipv4/fib_frontend.c | 2 +- net/netfilter/ipvs/ip_vs_ctl.c | 8 ++++---- 3 files changed, 10 insertions(+), 6 deletions(-) -- 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