Messages in this thread | | | Date | Mon, 4 Feb 2019 16:08:52 +0100 | From | Jessica Yu <> | Subject | Re: [PATCH] include/linux/module.h: mark init/cleanup_module aliases as __cold |
| |
+++ 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
| |