[PATCH 1/1] sched.h: always_inline alloc_tag_{save|restore} to fix modpost warnings

[PATCH 1/1] sched.h: always_inline alloc_tag_{save|restore} to fix modpost warnings

[Suren Baghdasaryan]

Mark alloc_tag_{save|restore} as always_inline to fix the following
modpost warnings:

WARNING: modpost: vmlinux: section mismatch in reference: alloc_tag_save+0x1c (section: .text.unlikely) -> initcall_level_names (section: .init.data)
WARNING: modpost: vmlinux: section mismatch in reference: alloc_tag_restore+0x3c (section: .text.unlikely) -> initcall_level_names (section: .init.data)

Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202407032306.gi9nZsBi-lkp@intel.com/
Signed-off-by: Suren Baghdasaryan <surenb@google.com>
Cc: Kent Overstreet <kent.overstreet@linux.dev>
---
 include/linux/sched.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index 61591ac6eab6..a5f4b48fca18 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -2192,13 +2192,13 @@ static inline int sched_core_idle_cpu(int cpu) { return idle_cpu(cpu); }
 extern void sched_set_stop_task(int cpu, struct task_struct *stop);
 
 #ifdef CONFIG_MEM_ALLOC_PROFILING
-static inline struct alloc_tag *alloc_tag_save(struct alloc_tag *tag)
+static __always_inline struct alloc_tag *alloc_tag_save(struct alloc_tag *tag)
 {
 	swap(current->alloc_tag, tag);
 	return tag;
 }
 
-static inline void alloc_tag_restore(struct alloc_tag *tag, struct alloc_tag *old)
+static __always_inline void alloc_tag_restore(struct alloc_tag *tag, struct alloc_tag *old)
 {
 #ifdef CONFIG_MEM_ALLOC_PROFILING_DEBUG
 	WARN(current->alloc_tag != tag, "current->alloc_tag was changed:\n");

base-commit: 795c58e4c7fc6163d8fb9f2baa86cfe898fa4b19
-- 
2.45.2.803.g4e1b14247a-goog

Re: [PATCH 1/1] sched.h: always_inline alloc_tag_{save|restore} to fix modpost warnings

[Andrew Morton]

On Wed,  3 Jul 2024 15:15:20 -0700 Suren Baghdasaryan <surenb@google.com> wrote:

> Mark alloc_tag_{save|restore} as always_inline to fix the following
> modpost warnings:
> 
> WARNING: modpost: vmlinux: section mismatch in reference: alloc_tag_save+0x1c (section: .text.unlikely) -> initcall_level_names (section: .init.data)
> WARNING: modpost: vmlinux: section mismatch in reference: alloc_tag_restore+0x3c (section: .text.unlikely) -> initcall_level_names (section: .init.data)

Well, is it only about fixing warnings?  If the warning is correct then
this might be fixing kernel crashes.

Do you know where these references are coming from?

I'm curious about the .text.unlikely.  Makes me wonder if we should
also have .init.unlikely for unlikely() calls which happen from __init
code.  Maybe we already handle that.

> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202407032306.gi9nZsBi-lkp@intel.com/
> Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> Cc: Kent Overstreet <kent.overstreet@linux.dev>

Fixes: 22d407b164ff ("lib: add allocation tagging support for memory allocation profiling")
Cc: stable

yes?

Re: [PATCH 1/1] sched.h: always_inline alloc_tag_{save|restore} to fix modpost warnings

[Suren Baghdasaryan]

On Wed, Jul 3, 2024 at 3:51 PM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Wed,  3 Jul 2024 15:15:20 -0700 Suren Baghdasaryan <surenb@google.com> wrote:
>
> > Mark alloc_tag_{save|restore} as always_inline to fix the following
> > modpost warnings:
> >
> > WARNING: modpost: vmlinux: section mismatch in reference: alloc_tag_save+0x1c (section: .text.unlikely) -> initcall_level_names (section: .init.data)
> > WARNING: modpost: vmlinux: section mismatch in reference: alloc_tag_restore+0x3c (section: .text.unlikely) -> initcall_level_names (section: .init.data)
>
> Well, is it only about fixing warnings?  If the warning is correct then
> this might be fixing kernel crashes.
>
> Do you know where these references are coming from?

I *think* this happens when alloc_tag_save()/alloc_tag_restore() are
not inlined and are called from an __init function.  They access the
`tag` parameter passed to them and since that tag is a static local
variable inside an __init function, I assume it gets allocated inside
__initdata. If so, an example of such case is cma_activate_area()
which is an __init function and allocates memory using
bitmap_zalloc():
https://elixir.bootlin.com/linux/v6.10-rc6/source/mm/cma.c#L97. There
are likely more cases like that.

>
> I'm curious about the .text.unlikely.  Makes me wonder if we should
> also have .init.unlikely for unlikely() calls which happen from __init
> code.  Maybe we already handle that.

I don't really know.

>
> > Reported-by: kernel test robot <lkp@intel.com>
> > Closes: https://lore.kernel.org/oe-kbuild-all/202407032306.gi9nZsBi-lkp@intel.com/
> > Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> > Cc: Kent Overstreet <kent.overstreet@linux.dev>
>
> Fixes: 22d407b164ff ("lib: add allocation tagging support for memory allocation profiling")

Yes. Do you want me to post a v2 or will handle that locally?

> Cc: stable

I don't think so. This feature was introduced in 6.10, so no backports
needed, right?

>
> yes?
>
>

Re: [PATCH 1/1] sched.h: always_inline alloc_tag_{save|restore} to fix modpost warnings

[Suren Baghdasaryan]

On Wed, Jul 3, 2024 at 4:23 PM Suren Baghdasaryan <surenb@google.com> wrote:
>
> On Wed, Jul 3, 2024 at 3:51 PM Andrew Morton <akpm@linux-foundation.org> wrote:
> >
> > On Wed,  3 Jul 2024 15:15:20 -0700 Suren Baghdasaryan <surenb@google.com> wrote:
> >
> > > Mark alloc_tag_{save|restore} as always_inline to fix the following
> > > modpost warnings:
> > >
> > > WARNING: modpost: vmlinux: section mismatch in reference: alloc_tag_save+0x1c (section: .text.unlikely) -> initcall_level_names (section: .init.data)
> > > WARNING: modpost: vmlinux: section mismatch in reference: alloc_tag_restore+0x3c (section: .text.unlikely) -> initcall_level_names (section: .init.data)
> >
> > Well, is it only about fixing warnings?  If the warning is correct then
> > this might be fixing kernel crashes.
> >
> > Do you know where these references are coming from?
>
> I *think* this happens when alloc_tag_save()/alloc_tag_restore() are
> not inlined and are called from an __init function.  They access the
> `tag` parameter passed to them and since that tag is a static local
> variable inside an __init function, I assume it gets allocated inside
> __initdata. If so, an example of such case is cma_activate_area()
> which is an __init function and allocates memory using
> bitmap_zalloc():
> https://elixir.bootlin.com/linux/v6.10-rc6/source/mm/cma.c#L97. There
> are likely more cases like that.

Actually, my theory is wrong. Allocation tags are always allocated
from the "alloc_tags" section. This makes me think that it's the
access to current->alloc_tag that causes these warnings. After my
change I actually see another warning like this:

WARNING: modpost: vmlinux: section mismatch in reference:
get_current+0xc (section: .text.unlikely) -> initcall_level_names
(section: .init.data)

get_current()/current_thread_info() for xtensa arch are inline functions:
https://elixir.bootlin.com/linux/v6.10-rc6/source/arch/xtensa/include/asm/current.h#L22
https://elixir.bootlin.com/linux/v6.10-rc6/source/arch/xtensa/include/asm/thread_info.h#L94

and they return a local variable `thread_info`.
Let me dig a bit more to understand what's really happening here.

>
> >
> > I'm curious about the .text.unlikely.  Makes me wonder if we should
> > also have .init.unlikely for unlikely() calls which happen from __init
> > code.  Maybe we already handle that.
>
> I don't really know.
>
> >
> > > Reported-by: kernel test robot <lkp@intel.com>
> > > Closes: https://lore.kernel.org/oe-kbuild-all/202407032306.gi9nZsBi-lkp@intel.com/
> > > Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> > > Cc: Kent Overstreet <kent.overstreet@linux.dev>
> >
> > Fixes: 22d407b164ff ("lib: add allocation tagging support for memory allocation profiling")
>
> Yes. Do you want me to post a v2 or will handle that locally?
>
> > Cc: stable
>
> I don't think so. This feature was introduced in 6.10, so no backports
> needed, right?
>
> >
> > yes?
> >
> >

Re: [PATCH 1/1] sched.h: always_inline alloc_tag_{save|restore} to fix modpost warnings

[Suren Baghdasaryan]

On Wed, Jul 3, 2024 at 5:10 PM Suren Baghdasaryan <surenb@google.com> wrote:
>
> On Wed, Jul 3, 2024 at 4:23 PM Suren Baghdasaryan <surenb@google.com> wrote:
> >
> > On Wed, Jul 3, 2024 at 3:51 PM Andrew Morton <akpm@linux-foundation.org> wrote:
> > >
> > > On Wed,  3 Jul 2024 15:15:20 -0700 Suren Baghdasaryan <surenb@google.com> wrote:
> > >
> > > > Mark alloc_tag_{save|restore} as always_inline to fix the following
> > > > modpost warnings:
> > > >
> > > > WARNING: modpost: vmlinux: section mismatch in reference: alloc_tag_save+0x1c (section: .text.unlikely) -> initcall_level_names (section: .init.data)
> > > > WARNING: modpost: vmlinux: section mismatch in reference: alloc_tag_restore+0x3c (section: .text.unlikely) -> initcall_level_names (section: .init.data)
> > >
> > > Well, is it only about fixing warnings?  If the warning is correct then
> > > this might be fixing kernel crashes.
> > >
> > > Do you know where these references are coming from?
> >
> > I *think* this happens when alloc_tag_save()/alloc_tag_restore() are
> > not inlined and are called from an __init function.  They access the
> > `tag` parameter passed to them and since that tag is a static local
> > variable inside an __init function, I assume it gets allocated inside
> > __initdata. If so, an example of such case is cma_activate_area()
> > which is an __init function and allocates memory using
> > bitmap_zalloc():
> > https://elixir.bootlin.com/linux/v6.10-rc6/source/mm/cma.c#L97. There
> > are likely more cases like that.
>
> Actually, my theory is wrong. Allocation tags are always allocated
> from the "alloc_tags" section. This makes me think that it's the
> access to current->alloc_tag that causes these warnings. After my
> change I actually see another warning like this:
>
> WARNING: modpost: vmlinux: section mismatch in reference:
> get_current+0xc (section: .text.unlikely) -> initcall_level_names
> (section: .init.data)
>
> get_current()/current_thread_info() for xtensa arch are inline functions:
> https://elixir.bootlin.com/linux/v6.10-rc6/source/arch/xtensa/include/asm/current.h#L22
> https://elixir.bootlin.com/linux/v6.10-rc6/source/arch/xtensa/include/asm/thread_info.h#L94
>
> and they return a local variable `thread_info`.
> Let me dig a bit more to understand what's really happening here.

Ok, I confirmed that the warning is happening due to the access to
"current" from alloc_tag_save()/alloc_tag_restore() functions. I guess
when these functions access "thread_info" variable:
https://elixir.bootlin.com/linux/v6.10-rc6/source/arch/xtensa/include/asm/thread_info.h#L96,
compiler flags that because the variable is on the stack of an __init
function while alloc_tag_save()/alloc_tag_restore() when not inlined
are from .text section.
To fix this warning completely I also need to change get_current() and
current_thread_info() for xtensa to be __always_inline. I confirmed
that after changing that the warnings caused by memory allocation
profiling are gone. Note that there are more similar warnings when
building this architecture but they happen even when
CONFIG_MEM_ALLOC_PROFILING=n. I tried fixing them all but one fix
leads to inner functions needing the same fix, so the patch grows
exponentially, so I left it for now.

If there are no objections, I'll post a v2 as two separate patches.
One changing the sched.h and the second one modifying xtensa-specific
get_current() and current_thread_info(). Will post tomorrow unless
someone replies with a better suggestion by then.
Thanks,
Suren.

>
> >
> > >
> > > I'm curious about the .text.unlikely.  Makes me wonder if we should
> > > also have .init.unlikely for unlikely() calls which happen from __init
> > > code.  Maybe we already handle that.
> >
> > I don't really know.
> >
> > >
> > > > Reported-by: kernel test robot <lkp@intel.com>
> > > > Closes: https://lore.kernel.org/oe-kbuild-all/202407032306.gi9nZsBi-lkp@intel.com/
> > > > Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> > > > Cc: Kent Overstreet <kent.overstreet@linux.dev>
> > >
> > > Fixes: 22d407b164ff ("lib: add allocation tagging support for memory allocation profiling")
> >
> > Yes. Do you want me to post a v2 or will handle that locally?
> >
> > > Cc: stable
> >
> > I don't think so. This feature was introduced in 6.10, so no backports
> > needed, right?
> >
> > >
> > > yes?
> > >
> > >

Re: [PATCH 1/1] sched.h: always_inline alloc_tag_{save|restore} to fix modpost warnings

[Andrew Morton]

On Wed, 3 Jul 2024 20:46:11 -0700 Suren Baghdasaryan <surenb@google.com> wrote:

> Ok, I confirmed that the warning is happening due to the access to
> "current" from alloc_tag_save()/alloc_tag_restore() functions. I guess
> when these functions access "thread_info" variable:
> https://elixir.bootlin.com/linux/v6.10-rc6/source/arch/xtensa/include/asm/thread_info.h#L96,
> compiler flags that because the variable is on the stack of an __init
> function while alloc_tag_save()/alloc_tag_restore() when not inlined
> are from .text section.

Well, is the warning legitimate?  I don't see why an automatic variable
of an __init function should be considered to be .init storage - we can
assume it won't become an invalid reference while the .init function is
executing?

Re: [PATCH 1/1] sched.h: always_inline alloc_tag_{save|restore} to fix modpost warnings

[Suren Baghdasaryan]

On Wed, Jul 3, 2024 at 8:54 PM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Wed, 3 Jul 2024 20:46:11 -0700 Suren Baghdasaryan <surenb@google.com> wrote:
>
> > Ok, I confirmed that the warning is happening due to the access to
> > "current" from alloc_tag_save()/alloc_tag_restore() functions. I guess
> > when these functions access "thread_info" variable:
> > https://elixir.bootlin.com/linux/v6.10-rc6/source/arch/xtensa/include/asm/thread_info.h#L96,
> > compiler flags that because the variable is on the stack of an __init
> > function while alloc_tag_save()/alloc_tag_restore() when not inlined
> > are from .text section.
>
> Well, is the warning legitimate?  I don't see why an automatic variable
> of an __init function should be considered to be .init storage - we can
> assume it won't become an invalid reference while the .init function is
> executing?

I don't think it's really a problem. __init function is executing, it
calls a function from .text (say alloc_tag_save() that was not
inlined) which in turn calls get_current(), which returns a pointer
somewhere inside __initdata. That should be fine since this can only
happen during init stage. If this call happens after init,
get_current() can't return a pointer from __initdata. If it does then
we have a much bigger problem.

>

Re: [PATCH 1/1] sched.h: always_inline alloc_tag_{save|restore} to fix modpost warnings

[Andrew Morton]

On Wed, 3 Jul 2024 21:07:56 -0700 Suren Baghdasaryan <surenb@google.com> wrote:

> On Wed, Jul 3, 2024 at 8:54 PM Andrew Morton <akpm@linux-foundation.org> wrote:
> >
> > On Wed, 3 Jul 2024 20:46:11 -0700 Suren Baghdasaryan <surenb@google.com> wrote:
> >
> > > Ok, I confirmed that the warning is happening due to the access to
> > > "current" from alloc_tag_save()/alloc_tag_restore() functions. I guess
> > > when these functions access "thread_info" variable:
> > > https://elixir.bootlin.com/linux/v6.10-rc6/source/arch/xtensa/include/asm/thread_info.h#L96,
> > > compiler flags that because the variable is on the stack of an __init
> > > function while alloc_tag_save()/alloc_tag_restore() when not inlined
> > > are from .text section.
> >
> > Well, is the warning legitimate?  I don't see why an automatic variable
> > of an __init function should be considered to be .init storage - we can
> > assume it won't become an invalid reference while the .init function is
> > executing?
> 
> I don't think it's really a problem. __init function is executing, it
> calls a function from .text (say alloc_tag_save() that was not
> inlined) which in turn calls get_current(), which returns a pointer
> somewhere inside __initdata. That should be fine since this can only
> happen during init stage. If this call happens after init,
> get_current() can't return a pointer from __initdata. If it does then
> we have a much bigger problem.

So I think you're saying "yes, the warning is legitimate and this might
be a problem, but it isn't in this case, so the checking code is OK so
let's just work around it"?

Re: [PATCH 1/1] sched.h: always_inline alloc_tag_{save|restore} to fix modpost warnings

[Suren Baghdasaryan]

On Wed, Jul 3, 2024 at 9:17 PM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Wed, 3 Jul 2024 21:07:56 -0700 Suren Baghdasaryan <surenb@google.com> wrote:
>
> > On Wed, Jul 3, 2024 at 8:54 PM Andrew Morton <akpm@linux-foundation.org> wrote:
> > >
> > > On Wed, 3 Jul 2024 20:46:11 -0700 Suren Baghdasaryan <surenb@google.com> wrote:
> > >
> > > > Ok, I confirmed that the warning is happening due to the access to
> > > > "current" from alloc_tag_save()/alloc_tag_restore() functions. I guess
> > > > when these functions access "thread_info" variable:
> > > > https://elixir.bootlin.com/linux/v6.10-rc6/source/arch/xtensa/include/asm/thread_info.h#L96,
> > > > compiler flags that because the variable is on the stack of an __init
> > > > function while alloc_tag_save()/alloc_tag_restore() when not inlined
> > > > are from .text section.
> > >
> > > Well, is the warning legitimate?  I don't see why an automatic variable
> > > of an __init function should be considered to be .init storage - we can
> > > assume it won't become an invalid reference while the .init function is
> > > executing?
> >
> > I don't think it's really a problem. __init function is executing, it
> > calls a function from .text (say alloc_tag_save() that was not
> > inlined) which in turn calls get_current(), which returns a pointer
> > somewhere inside __initdata. That should be fine since this can only
> > happen during init stage. If this call happens after init,
> > get_current() can't return a pointer from __initdata. If it does then
> > we have a much bigger problem.
>
> So I think you're saying "yes, the warning is legitimate and this might
> be a problem, but it isn't in this case, so the checking code is OK so
> let's just work around it"?

Yes, that's my understanding. If get_current() always returns a valid
pointer for the given execution stage (during init/after init) then it
should be safe to call it even from a function residing in the .text
section during init.

>

Re: [PATCH 1/1] sched.h: always_inline alloc_tag_{save|restore} to fix modpost warnings

[Andrew Morton]

On Wed, 3 Jul 2024 21:25:30 -0700 Suren Baghdasaryan <surenb@google.com> wrote:

> > > I don't think it's really a problem. __init function is executing, it
> > > calls a function from .text (say alloc_tag_save() that was not
> > > inlined) which in turn calls get_current(), which returns a pointer
> > > somewhere inside __initdata. That should be fine since this can only
> > > happen during init stage. If this call happens after init,
> > > get_current() can't return a pointer from __initdata. If it does then
> > > we have a much bigger problem.
> >
> > So I think you're saying "yes, the warning is legitimate and this might
> > be a problem, but it isn't in this case, so the checking code is OK so
> > let's just work around it"?
> 
> Yes, that's my understanding. If get_current() always returns a valid
> pointer for the given execution stage (during init/after init) then it
> should be safe to call it even from a function residing in the .text
> section during init.

Make sense, thanks for confirming.  I'll await the v2 series, if
you think it remains appropriate?