Re: [PATCH] x86/resctrl: Annotate __get_mem_config_intel() as __init

[Reinette Chatre]

Hi Nathan,

On 9/13/24 12:41 PM, Nathan Chancellor wrote:
> Hi Reinette,
>
> On Thu, Sep 12, 2024 at 03:33:09PM -0700, Reinette Chatre wrote:
> > Apologies for the delay.
>
> No worries, this is not super high priority (except when the section
> mismatch warning is elevated to an error but that does not happen in too
> many real world configurations).
>
> > On 8/22/24 5:12 PM, Nathan Chancellor wrote:
> > > After a recent LLVM change [1] that deduces __cold on functions that
> > > only call cold code (such as __init functions), there is a section
> > > mismatch warning from __get_mem_config_intel(), which got moved to
> > > .text.unlikely. as a result of that optimization:
> > >
> > >     WARNING: modpost: vmlinux: section mismatch in reference: __get_mem_config_intel+0x77 (section: .text.unlikely.) -> thread_throttle_mode_init (section: .init.text)
> > >
> > > Mark __get_mem_config_intel() as __init as well since it is only called
> > > from __init code, which clears up the warning.
> >
> > It looks to me as though __rdt_get_mem_config_amd() may need the same __init
> > treatment
>
> It certainly looks like __init would be appropriate for
> __rdt_get_mem_config_amd(), although there is no current risk of a
> modpost warning like __get_mem_config_intel() because it does not call
> any __init functions, which is really what triggered this warning.

Ah I see ... I missed the part played by thread_throttle_mode_init().

> > it is not clear to me why __get_mem_config_intel() would trigger
> > such warning, but not __rdt_get_mem_config_amd()?
>
> Based on my understanding of the LLVM change linked below my comment
> here, __get_mem_config_intel() gets implicitly marked as __cold because
> it unconditionally calls thread_throttle_mode_init(), which is __cold
> through __init. If __get_mem_config_intel() does not get inlined into
> its caller (which could happen if a compiler decides not to optimize
> __cold code), that call to thread_throttle_mode_init() will appear to
> come from the .text section, even though it will really be from
> .init.text because __get_mem_config_intel() is only called from __init
> functions.
>
> __rdt_get_mem_config_amd() does not call any cold functions so it avoids
> this problem altogether.

Thank you very much for the detailed explanation. Much appreciated.

> I can send a v2 with __init added to __rdt_get_mem_config_amd() if you
> want, along with the style update you mention below. Just let me know
> what you prefer based on my comments above.

Could you please add __init to __rdt_get_mem_config_amd() also? I do understand
that it does not produce a warning today but __rdt_get_mem_config_amd() too is
only called from __init code. To me this already indicates that __init is
appropriate and ensuring its storage class is accurate protects against triggering
this warning in the future.

Thank you very much.

Reinette