Re: [PATCH v2 1/2] padata: Mark padata_work_init() as __ref

[Nathan Chancellor <nathan@kernel.org>]

On Mon, Dec 12, 2022 at 10:07:24PM +0900, Masahiro Yamada wrote:
> On Thu, Dec 8, 2022 at 4:17 AM Nathan Chancellor <nathan@kernel.org> wrote:
> >
> > When building arm64 allmodconfig + ThinLTO with clang and a proposed
> > modpost update to account for -ffuncton-sections, the following warning
> > appears:
> >
> >   WARNING: modpost: vmlinux.o: section mismatch in reference: padata_work_init (section: .text.padata_work_init) -> padata_mt_helper (section: .init.text)
> >   WARNING: modpost: vmlinux.o: section mismatch in reference: padata_work_init (section: .text.padata_work_init) -> padata_mt_helper (section: .init.text)
> >
> > LLVM has optimized padata_work_init() to include the address of
> > padata_mt_helper() directly, which causes modpost to complain since
> > padata_work_init() is not __init, whereas padata_mt_helper() is. In
> > reality, padata_work_init() is only called with padata_mt_helper() as
> > the work_fn argument in code that is __init, so this warning will not
> > result in any problems. Silence it with __ref, which makes it clear to
> > modpost that padata_work_init() can only use padata_mt_helper() in
> > __init code.
> >
> > Suggested-by: Daniel Jordan <daniel.m.jordan@oracle.com>
> > Signed-off-by: Nathan Chancellor <nathan@kernel.org>
> > ---
> > Cc: Steffen Klassert <steffen.klassert@secunet.com>
> > Cc: Daniel Jordan <daniel.m.jordan@oracle.com>
> > Cc: linux-crypto@vger.kernel.org
> > ---
> >  kernel/padata.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/kernel/padata.c b/kernel/padata.c
> > index e5819bb8bd1d..4c3137fe8449 100644
> > --- a/kernel/padata.c
> > +++ b/kernel/padata.c
> > @@ -83,8 +83,8 @@ static struct padata_work *padata_work_alloc(void)
> >         return pw;
> >  }
> >
> > -static void padata_work_init(struct padata_work *pw, work_func_t work_fn,
> > -                            void *data, int flags)
> > +static __ref void padata_work_init(struct padata_work *pw, work_func_t work_fn,
> > +                                  void *data, int flags)
> >  {
> >         if (flags & PADATA_WORK_ONSTACK)
> >                 INIT_WORK_ONSTACK(&pw->pw_work, work_fn);
> >
> > base-commit: 76dcd734eca23168cb008912c0f69ff408905235
> > --
> > 2.38.1
> >
> 
> It took me a while to understand why LTO can embed
> padata_mt_helper's address into padata_work_init().

Sorry about that, I can try to expand on this in both the commit message
and in-code comment if I end up adding it.

> There are 3 call-sites to padata_work_init().
> 
> (1)  __init padata_work_alloc_mt()
>          -->  padata_work_init(..., padata_mt_helper, ...)
> 
> (2) padata_do_parallel()
>          -->  padata_work_init(..., padata_parallel_worker, ...)
> 
> (3) __init padata_do_multithreaded()
>         --> padata_work_init(..., padata_mt_helper, ...)
> 
> 
> The function call (2) is squashed away.
> 
> 
> With only (1) and (3) remaining, the 2nd parameter to
> padata_work_init() is always padata_mt_helper,
> therefore LLVM embeds padata_mt_hlper's address
> directly into padata_work_init().
> 
> I am not sure if the compiler should do this level of optimization
> because kernel/padata.c does not seem to be a special case.
> Perhaps, we might be hit with more cases that need __ref annotation,
> which is only required by LTO.

That's possible. I did only see this once instance in all my builds but
allmodconfig + ThinLTO might not be too interesting of a case,
since the sanitizers will be enabled, which makes optimization more
difficult. I could try to enable ThinLTO with some distribution
configurations to see if there are any more instances that crop up.

> One note is that, we could discard padata_work_init()
> because (1) and (3) are both annotated as __init.
> So, another way of fixing is
>    static __always_inline void padata_work_init(...)
> because the compiler would determine padata_work_init()
> would be small enough if the caller and callee belonged to
> the same section.
> 
> I do not have a strong opinion.
> Honestly, I do not know what the best approach would be to fix this.

Agreed to both points, it is really up to the padata maintainers.

> If we go with the __ref annotation, I can pick this, but
> at least can you add some comments?
> 
> 
> include/linux/init.h says:
> "optimally document why the __ref is needed and why it's OK"
> 
> 
> I think this is the case that needs some comments
> because LTO optimization looks too tricky to me.

Sure thing, I will send a v3 either Tuesday or Wednesday with an updated
commit message and code comment if we end up going this route.

Thank you for the review!

Cheers,
Nathan