include/linux/module.h: mark init/cleanup_module aliases as __cold
PATCH 1/1 include/linux/module.h: mark init/cleanup_module aliases as __cold
The upcoming GCC 9 release adds the -Wmissing-attributes warnings (enabled by -Wall), which trigger for all the init/cleanup_module aliases in the kernel (defined by the module_init/exit macros), ending up being very noisy.
These aliases point to the __init/__exit functions of a module, which are defined as __cold (among other attributes). However, the aliases themselves do not have the __cold attribute.
Since the compiler behaves differently when compiling a __cold function as well as when compiling paths leading to calls to __cold functions, the warning is trying to point out the possibly-forgotten attribute in the alias.
In order to keep the warning enabled, we choose to silence the warning by marking the aliases as __cold. This is possible marking either the extern declaration, the definition, or both. In order to avoid changing the behavior of callers, we do it only in the definition of the aliases (since those are not seen by any other TU).
---
Note that an alternative is using the new copy attribute introduced by GCC 9 (Martin told me about it, as well as the new warning).
What I am concerned about using __copy is that I am not sure we should be copying all the attributes (even if some are blacklisted by the copy itself), since:
- We have unknown-to-GCC attributes (e.g. from plugins).
- We wouldn't enjoy the fix for older compilers (e.g. if the fix had an actual impact).
So here I took the conservative approach for the moment, and we can discuss/apply whether another solution is best.
Jessica: please review what I explain in the commit message. Do we actually want the __cold attribute in the declaration as well? If yes, AFAIK, GCC would assume paths that end up calling the __init/__exit functions are not meant to be taken (but when we are asked to load modules, that is the expected path, no?).
I will put this in the compiler-attributes tree and get some time in linux-next, unless you want to pick it up!
include/linux/module.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
include/linux/module.h +2 -2
@@ -129,13 +129,13 @@ extern void cleanup_module(void);#define module_init (initfn) \ static inline initcall_t __maybe_unused __inittest(void) \ { return initfn; } \ - int init_module(void) __attribute__((alias(#initfn))); + int init_module(void) __cold __attribute__((alias(#initfn)));/* This is only required if you want to be unloadable. */ #define module_exit (exitfn) \ static inline exitcall_t __maybe_unused __exittest(void) \ { return exitfn; } \ - void cleanup_module(void) __attribute__((alias(#exitfn))); + void cleanup_module(void) __cold __attribute__((alias(#exitfn)));#endif
Signature · 1 line
2.17.1
Quoted 62 lines · Miguel Ojeda
The upcoming GCC 9 release adds the -Wmissing-attributes warnings (enabled by -Wall), which trigger for all the
init/cleanup_modulealiases in the kernel (defined by the module_init/exit macros), ending up being very noisy.These aliases point to the __init/__exit functions of a module, which are defined as
__cold(among other attributes). However, the aliases themselves do not have the__coldattribute.Since the compiler behaves differently when compiling a
__coldfunction as well as when compiling paths leading to calls to__coldfunctions, the warning is trying to point out the possibly-forgotten attribute in the alias.In order to keep the warning enabled, we choose to silence the warning by marking the aliases as
__cold. This is possible marking either the extern declaration, the definition, or both. In order to avoid changing the behavior of callers, we do it only in the definition of the aliases (since those are not seen by any other TU).Suggested-by Martin Sebor <[email protected]>Signed-off-by Miguel Ojeda <[email protected]>---
Note that an alternative is using the new copy attribute introduced by GCC 9 (Martin told me about it, as well as the new warning).What I am concerned about using
__copyis that I am not sure we should be copying all the attributes (even if some are blacklisted by the copy itself), since:
- We have unknown-to-GCC attributes (e.g. from plugins).
- We wouldn't enjoy the fix for older compilers (e.g. if the fix had an actual impact).So here I took the conservative approach for the moment, and we can discuss/apply whether another solution is best.
Jessica: please review what I explain in the commit message. Do we actually want the
__coldattribute in the declaration as well? If yes, AFAIK, GCC would assume paths that end up calling the __init/__exit functions are not meant to be taken (but when we are asked to load modules, that is the expected path, no?).I will put this in the compiler-attributes tree and get some time in linux-next, unless you want to pick it up!
include/linux/module.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)include/linux/module.h +1
-1@@ -129,13 +129,13 @@ extern void cleanup_module(void);#define module_init (initfn) \ static inline initcall_t __maybe_unused __inittest(void) \ { return initfn; } \ - int init_module(void) __attribute__((alias(#initfn))); + int init_module(void) __cold __attribute__((alias(#initfn)));
include/linux/module.h extern void cleanup_module(void); · line 141
Quoted 10 lines
include/linux/module.h +1
-1/* This is only required if you want to be unloadable. */ #define module_exit (exitfn) \ static inline exitcall_t __maybe_unused __exittest(void) \ { return exitfn; } \ - void cleanup_module(void) __attribute__((alias(#exitfn))); + void cleanup_module(void) __cold __attribute__((alias(#exitfn)));#endif
+++ Miguel Ojeda [23/01/19 18:37 +0100]:
Quoted 38 lines
The upcoming GCC 9 release adds the -Wmissing-attributes warnings (enabled by -Wall), which trigger for all the
init/cleanup_modulealiases in the kernel (defined by the module_init/exit macros), ending up being very noisy.These aliases point to the __init/__exit functions of a module, which are defined as
__cold(among other attributes). However, the aliases themselves do not have the__coldattribute.Since the compiler behaves differently when compiling a
__coldfunction as well as when compiling paths leading to calls to__coldfunctions, the warning is trying to point out the possibly-forgotten attribute in the alias.In order to keep the warning enabled, we choose to silence the warning by marking the aliases as
__cold. This is possible marking either the extern declaration, the definition, or both. In order to avoid changing the behavior of callers, we do it only in the definition of the aliases (since those are not seen by any other TU).Suggested-by Martin Sebor <[email protected]>Signed-off-by Miguel Ojeda <[email protected]>---
Note that an alternative is using the new copy attribute introduced by GCC 9 (Martin told me about it, as well as the new warning).What I am concerned about using
__copyis that I am not sure we should be copying all the attributes (even if some are blacklisted by the copy itself), since:
- We have unknown-to-GCC attributes (e.g. from plugins).
- We wouldn't enjoy the fix for older compilers (e.g. if the fix had an actual impact).So here I took the conservative approach for the moment, and we can discuss/apply whether another solution is best.
Quoted 6 lines
Jessica: please review what I explain in the commit message. Do we actually want the
__coldattribute in the declaration as well? If yes, AFAIK, GCC would assume paths that end up calling the __init/__exit functions are not meant to be taken (but when we are asked to load modules, that is the expected path, no?).
Hi Miguel, sorry for the delay!
The module init functions are only called once from do_init_module(). Does the __cold attribute just assume it is unlikely to be executed, or just that it is infrequently called (which would be true for the module init functions since they're just called once)?
In any case, module init functions are normally annotated with __init, so they get the __cold attribute anyway. I'm wondering why not just annotate the alias with __init instead, instead of cherry picking attributes to silence the warnings? That way the alias and the actual module init function would always have the same declaration/attributes. Would this work to silence the warnings or am I missing something?
Thanks,
Jessica
Quoted 29 lines
I will put this in the compiler-attributes tree and get some time in linux-next, unless you want to pick it up!
include/linux/module.h| 4 ++--1 file changed, 2 insertions(+), 2 deletions(-)include/linux/module.h +2
-2@@ -129,13 +129,13 @@ extern void cleanup_module(void);#define module_init (initfn) \ static inline initcall_t __maybe_unused __inittest(void) \ { return initfn; } \ - int init_module(void) __attribute__((alias(#initfn))); + int init_module(void) __cold __attribute__((alias(#initfn)));/* This is only required if you want to be unloadable. */ #define module_exit (exitfn) \ static inline exitcall_t __maybe_unused __exittest(void) \ { return exitfn; } \ - void cleanup_module(void) __attribute__((alias(#exitfn))); + void cleanup_module(void) __cold __attribute__((alias(#exitfn)));#endif Signature · 1 line
2.17.1
Hi Jessica,
Quoted 2 lines · Jessica Yu
Hi Miguel, sorry for the delay!
No worries! :)
Quoted 4 lines
The module init functions are only called once from
do_init_module(). Does the__coldattribute just assume it is unlikely to be executed, or just that it is infrequently called (which would be true for the module init functions since they're just called once)?
That was exactly my concern :-) Martin can provide way better details than me, but as far as I understand it, it is the paths that end up calling __cold functions that are treated as unlikely to happen. For instance, if f() has a few branches and calls a cold g() in one of them, that branch is understood to be rarely executed and f() will be laid out assuming the other branches are more likely.
Then there is the other aspect of __cold, in the definition of the function. There, it affects how it is compiled and where it is placed, etc.
Therefore, I assume the current situation is the correct one: we want to callers to *not* see __cold, but we want the init function to be compiled as __cold.
Now, the alias is not seen by other TUs (i.e. they only see the extern declaration), so it does not matter whether the alias is cold or not (except for the warning), as far as I understand.
Quoted 6 lines
In any case, module init functions are normally annotated with
__init, so they get the__coldattribute anyway. I'm wondering why not just annotate the alias with__initinstead, instead of cherry picking attributes to silence the warnings? That way the alias and the actual module init function would always have the same declaration/attributes. Would this work to silence the warnings or am I missing something?
We could do indeed do that too (Martin actually proposed a solution with the new copy attribute, which would do something like that).
I chose to only add __cold to avoid any problems derived from the rest of the attributes, since I don't know how they behave or what are the implications (if any) of putting them into the alias (and not into the extern declaration).
Cheers,
Miguel
+++ Miguel Ojeda [31/01/19 17:48 +0100]:
Quoted 39 lines
Hi Jessica,
Quoted 2 lines · Jessica Yu
Hi Miguel, sorry for the delay!
No worries! :)
Quoted 4 lines
The module init functions are only called once from
do_init_module(). Does the__coldattribute just assume it is unlikely to be executed, or just that it is infrequently called (which would be true for the module init functions since they're just called once)?That was exactly my concern :-) Martin can provide way better details than me, but as far as I understand it, it is the paths that end up calling
__coldfunctions that are treated as unlikely to happen. For instance, iff()has a few branches and calls a coldg()in one of them, that branch is understood to be rarely executed andf()will be laid out assuming the other branches are more likely.Then there is the other aspect of
__cold, in the definition of the function. There, it affects how it is compiled and where it is placed, etc.Therefore, I assume the current situation is the correct one: we want to callers to *not* see
__cold, but we want the init function to be compiled as__cold.Now, the alias is not seen by other TUs (i.e. they only see the extern declaration), so it does not matter whether the alias is cold or not (except for the warning), as far as I understand.
Quoted 6 lines
In any case, module init functions are normally annotated with
__init, so they get the__coldattribute anyway. I'm wondering why not just annotate the alias with__initinstead, instead of cherry picking attributes to silence the warnings? That way the alias and the actual module init function would always have the same declaration/attributes. Would this work to silence the warnings or am I missing something?
Quoted 7 lines
We could do indeed do that too (Martin actually proposed a solution with the new copy attribute, which would do something like that).
I chose to only add
__coldto avoid any problems derived from the rest of the attributes, since I don't know how they behave or what are the implications (if any) of putting them into the alias (and not into the extern declaration).
IMHO I think annotating with __init is more straightforward, instead of cherry-picking attributes (we wouldn't know at first glance why the aliases are specifically annotated with __cold without looking at git history). Plus the actual module init function and alias declarations would be consistent. Just looking at the __init attributes:
#define __init __section(.init.text) __cold __latent_entropy __noinitretpoline
__section(.init.text) - alias already has same section ndx as the target symbol so this doesn't have any effect.
__latent_entropy - according to commit 0766f788eb7, if this attribute is used on a function then the plugin will utilize it for gathering entropy (apparently a local variable is created in every marked function, the value of which is modified randomly, and before function return it will write into the latent_entropy global variable). Module init functions are already annotated with this since they are annotated with __init, I don't think marking the alias would do any harm.
__noinitretpoline - compiled away if the function is in a module and not built-in. The alias is not utilized if the module is built-in. So this wouldn't apply to the alias.
Unfortunately I don't have gcc9 set up on my machine so I can't actually test if it gets rid of all the warnings, so testing this would be appreciated :)
Thanks,
Jessica
Quoted 21 lines · Jessica Yu
IMHO I think annotating with
__initis more straightforward, instead of cherry-picking attributes (we wouldn't know at first glance why the aliases are specifically annotated with__coldwithout looking at git history). Plus the actual module init function and alias declarations would be consistent. Just looking at the__initattributes:#define __init __section(.init.text) __cold __latent_entropy __noinitretpoline
__section(.init.text)- alias already has same section ndx as the target symbol so this doesn't have any effect.
__latent_entropy- according to commit0766f788eb7, if this attribute is used on a function then the plugin will utilize it for gathering entropy (apparently a local variable is created in every marked function, the value of which is modified randomly, and before function return it will write into thelatent_entropyglobal variable). Module init functions are already annotated with this since they are annotated with__init, I don't think marking the alias would do any harm.
Quoted 3 lines
__noinitretpoline- compiled away if the function is in a module and not built-in. The alias is not utilized if the module is built-in. So this wouldn't apply to the alias.
In that case, there is also the option suggested by Martin: using the new "copy" attribute which copies all attributes, except those blacklisted by GCC, at the moment:
alias, always_inline, gnu_inline, ifunc, noinline, visibility, weak, weakref
Since we have the __init macro, there is not much gain in this instance (but if you prefer the copy alternative, let me know).
Quoted 3 lines
Unfortunately I don't have gcc9 set up on my machine so I can't actually test if it gets rid of all the warnings, so testing this would be appreciated :)
The warning triggers currently for a subset of attributes only:
alloc_align, alloc_size, cold, const, hot, leaf, malloc, nonnull, noreturn, nothrow, pure, returns_nonnull, returns_twice
So the rest of the attributes do not make a difference w.r.t. the warnings.
I will change it to __init then and send the PR after a couple of days in -next :)
Cheers,
Miguel
Quoted 10 lines · Miguel Ojeda
Quoted 8 lines · Jessica Yu
IMHO I think annotating with
__initis more straightforward, instead of cherry-picking attributes (we wouldn't know at first glance why the aliases are specifically annotated with__coldwithout looking at git history). Plus the actual module init function and alias declarations would be consistent. Just looking at the__initattributes:#define __init __section(.init.text) __cold __latent_entropy __noinitretpoline
By the way, note that we also need to annotate the exit ones. To do something similar, for __exit we have:
#define __exit __section(.exit.text) __exitused __cold notrace
__exitused expands to nothing when MODULE is defined.
notrace is either hotpatch(0,0) or no_instrument_function; and they shouldn't matter in the alias since we are not generating code (and anyway they disable the extra code, instead of enabling).
So I will also use __exit there instead of only __cold too.
Cheers,
Miguel