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

[Jessica Yu <jeyu@kernel.org>]

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