[PATCH] include/linux/module.h: mark init/cleanup_module aliases as __cold

[PATCH] include/linux/module.h: mark init/cleanup_module aliases as __cold

[Miguel Ojeda]

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).

Suggested-by: Martin Sebor <msebor@gcc.gnu.org>
Signed-off-by: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>
---
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(-)

diff --git a/include/linux/module.h b/include/linux/module.h
index 8fa38d3e7538..c4e805e87628 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -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
 
-- 
2.17.1

Re: [PATCH] include/linux/module.h: mark init/cleanup_module aliases as __cold

[Laura Abbott]

On 1/23/19 9:37 AM, Miguel Ojeda wrote:
> 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).
> 
> Suggested-by: Martin Sebor <msebor@gcc.gnu.org>
> Signed-off-by: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>
> ---
> 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(-)
> 
> diff --git a/include/linux/module.h b/include/linux/module.h
> index 8fa38d3e7538..c4e805e87628 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -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
>   
> 

Tested-by: Laura Abbott <labbott@redhat.com>

Re: [PATCH] include/linux/module.h: mark init/cleanup_module aliases as __cold

[Jessica Yu]

+++ Miguel Ojeda [23/01/19 18:37 +0100]:
>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).
>
>Suggested-by: Martin Sebor <msebor@gcc.gnu.org>
>Signed-off-by: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>
>---
>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?).

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

>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(-)
>
>diff --git a/include/linux/module.h b/include/linux/module.h
>index 8fa38d3e7538..c4e805e87628 100644
>--- a/include/linux/module.h
>+++ b/include/linux/module.h
>@@ -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
>
>-- 
>2.17.1
>

Re: [PATCH] include/linux/module.h: mark init/cleanup_module aliases as __cold

[Miguel Ojeda]

Hi Jessica,

On Thu, Jan 31, 2019 at 3:22 PM Jessica Yu <jeyu@kernel.org> wrote:
>
> Hi Miguel, sorry for the delay!

No worries! :)

> 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)?

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.

> 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?

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

Re: [PATCH] include/linux/module.h: mark init/cleanup_module aliases as __cold

[Jessica Yu]

+++ Miguel Ojeda [31/01/19 17:48 +0100]:
>Hi Jessica,
>
>On Thu, Jan 31, 2019 at 3:22 PM Jessica Yu <jeyu@kernel.org> wrote:
>>
>> Hi Miguel, sorry for the delay!
>
>No worries! :)
>
>> 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)?
>
>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.
>
>> 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?
>
>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).

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

Re: [PATCH] include/linux/module.h: mark init/cleanup_module aliases as __cold

[Miguel Ojeda]

On Mon, Feb 4, 2019 at 4:08 PM Jessica Yu <jeyu@kernel.org> wrote:
>
> 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.

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).

> 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

Re: [PATCH] include/linux/module.h: mark init/cleanup_module aliases as __cold

[Miguel Ojeda]

On Wed, Feb 6, 2019 at 5:31 PM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> On Mon, Feb 4, 2019 at 4:08 PM Jessica Yu <jeyu@kernel.org> wrote:
> >
> > 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

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