Message-ID: <CAJuCfpEJkCN1XDDE+2B4ePoFKbvSCaUA8=RO1TTg23XV56mRZg@mail.gmail.com>
Date: Wed, 3 Jul 2024 20:46:11 -0700
From: Suren Baghdasaryan <surenb@...gle.com>
To: Andrew Morton <akpm@...ux-foundation.org>
Cc: mingo@...hat.com, peterz@...radead.org, juri.lelli@...hat.com, 
	vincent.guittot@...aro.org, kent.overstreet@...ux.dev, 
	linux-kernel@...r.kernel.org, kernel test robot <lkp@...el.com>
Subject: Re: [PATCH 1/1] sched.h: always_inline alloc_tag_{save|restore} to
 fix modpost warnings

On Wed, Jul 3, 2024 at 5:10 PM Suren Baghdasaryan <surenb@...gle.com> wrote:
>
> On Wed, Jul 3, 2024 at 4:23 PM Suren Baghdasaryan <surenb@...gle.com> wrote:
> >
> > On Wed, Jul 3, 2024 at 3:51 PM Andrew Morton <akpm@...ux-foundation.org> wrote:
> > >
> > > On Wed,  3 Jul 2024 15:15:20 -0700 Suren Baghdasaryan <surenb@...gle.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@...el.com>
> > > > Closes: https://lore.kernel.org/oe-kbuild-all/202407032306.gi9nZsBi-lkp@intel.com/
> > > > Signed-off-by: Suren Baghdasaryan <surenb@...gle.com>
> > > > Cc: Kent Overstreet <kent.overstreet@...ux.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?
> > >
> > >

Powered by blists - more mailing lists