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

[Reinette Chatre <reinette.chatre@intel.com>]

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