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

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

[Nathan Chancellor]

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.

Link: https://github.com/llvm/llvm-project/commit/6b11573b8c5e3d36beee099dbe7347c2a007bf53 [1]
Signed-off-by: Nathan Chancellor <nathan@kernel.org>
---
 arch/x86/kernel/cpu/resctrl/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
index 1930fce9dfe9..b28646f1d9d6 100644
--- a/arch/x86/kernel/cpu/resctrl/core.c
+++ b/arch/x86/kernel/cpu/resctrl/core.c
@@ -199,7 +199,7 @@ static inline bool rdt_get_mb_table(struct rdt_resource *r)
 	return false;
 }
 
-static bool __get_mem_config_intel(struct rdt_resource *r)
+static bool __init __get_mem_config_intel(struct rdt_resource *r)
 {
 	struct rdt_hw_resource *hw_res = resctrl_to_arch_res(r);
 	union cpuid_0x10_3_eax eax;

---
base-commit: 7424fc6b86c8980a87169e005f5cd4438d18efe6
change-id: 20240822-x86-restctrl-get_mem_config_intel-init-3af02a5130ba

Best regards,
-- 
Nathan Chancellor <nathan@kernel.org>

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

[Reinette Chatre]

Hi Nathan,

Apologies for the delay.

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 and it is not clear to me why __get_mem_config_intel() would trigger
such warning, but not __rdt_get_mem_config_amd()?

> 
> Link: https://github.com/llvm/llvm-project/commit/6b11573b8c5e3d36beee099dbe7347c2a007bf53 [1]
> Signed-off-by: Nathan Chancellor <nathan@kernel.org>
> ---
>   arch/x86/kernel/cpu/resctrl/core.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> index 1930fce9dfe9..b28646f1d9d6 100644
> --- a/arch/x86/kernel/cpu/resctrl/core.c
> +++ b/arch/x86/kernel/cpu/resctrl/core.c
> @@ -199,7 +199,7 @@ static inline bool rdt_get_mb_table(struct rdt_resource *r)
>   	return false;
>   }
>   
> -static bool __get_mem_config_intel(struct rdt_resource *r)
> +static bool __init __get_mem_config_intel(struct rdt_resource *r)

Surely resctrl is not consistent in this regard but I understand from the coding style
doc that storage class should precede the return type, so perhaps:
	static __init bool __get_mem_config_intel(struct rdt_resource *r)

We may need to follow this recommended style for this to be included.

>   {
>   	struct rdt_hw_resource *hw_res = resctrl_to_arch_res(r);
>   	union cpuid_0x10_3_eax eax;
> 
> ---
> base-commit: 7424fc6b86c8980a87169e005f5cd4438d18efe6
> change-id: 20240822-x86-restctrl-get_mem_config_intel-init-3af02a5130ba
> 
> Best regards,

Thank you very much.

Reinette

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

[Nathan Chancellor]

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.

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

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.

Cheers,
Nathan

> > Link: https://github.com/llvm/llvm-project/commit/6b11573b8c5e3d36beee099dbe7347c2a007bf53 [1]
> > Signed-off-by: Nathan Chancellor <nathan@kernel.org>
> > ---
> >   arch/x86/kernel/cpu/resctrl/core.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> > index 1930fce9dfe9..b28646f1d9d6 100644
> > --- a/arch/x86/kernel/cpu/resctrl/core.c
> > +++ b/arch/x86/kernel/cpu/resctrl/core.c
> > @@ -199,7 +199,7 @@ static inline bool rdt_get_mb_table(struct rdt_resource *r)
> >   	return false;
> >   }
> > -static bool __get_mem_config_intel(struct rdt_resource *r)
> > +static bool __init __get_mem_config_intel(struct rdt_resource *r)
> 
> Surely resctrl is not consistent in this regard but I understand from the coding style
> doc that storage class should precede the return type, so perhaps:
> 	static __init bool __get_mem_config_intel(struct rdt_resource *r)
> 
> We may need to follow this recommended style for this to be included.
> 
> >   {
> >   	struct rdt_hw_resource *hw_res = resctrl_to_arch_res(r);
> >   	union cpuid_0x10_3_eax eax;
> > 
> > ---
> > base-commit: 7424fc6b86c8980a87169e005f5cd4438d18efe6
> > change-id: 20240822-x86-restctrl-get_mem_config_intel-init-3af02a5130ba
> > 
> > Best regards,
> 
> Thank you very much.
> 
> Reinette

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