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@xxxxxxxxxx> 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