Re: [PATCH] cpumask: Switch from inline to __always_inline

[Yury Norov <yury.norov@gmail.com>]

On Tue, Jun 25, 2024 at 11:27:59AM -0700, Brian Norris wrote:
> Hi Yuri, Rasmus,
> 
> On Tue, May 14, 2024 at 01:49:01PM -0700, Brian Norris wrote:
> > On recent (v6.6+) builds with Clang (based on Clang 18.0.0) and certain
> > configurations [0], I'm finding that (lack of) inlining decisions may
> > lead to section mismatch warnings like the following:
> > 
> >   WARNING: modpost: vmlinux.o: section mismatch in reference:
> >   cpumask_andnot (section: .text) ->
> >   cpuhp_bringup_cpus_parallel.tmp_mask (section: .init.data) ERROR:
> >   modpost: Section mismatches detected.
> > 
> > or more confusingly:
> > 
> >   WARNING: modpost: vmlinux: section mismatch in reference:
> >   cpumask_andnot+0x5f (section: .text) -> efi_systab_phys (section:
> >   .init.data)
> > 
> > The first warning makes a little sense, because
> > cpuhp_bringup_cpus_parallel() (an __init function) calls
> > cpumask_andnot() on tmp_mask (an __initdata symbol). If the compiler
> > doesn't inline cpumask_andnot(), this may appear like a mismatch.
> > 
> > The second warning makes less sense, but might be because efi_systab_phys
> > and cpuhp_bringup_cpus_parallel.tmp_mask are laid out near each other,
> > and the latter isn't a proper C symbol definition.
> > 
> > In any case, it seems a reasonable solution to suggest more strongly to
> > the compiler that these cpumask macros *must* be inlined, as 'inline' is
> > just a recommendation.
> > 
> > This change (plus more) has been previously proposed for other reasons
> > -- that some of the bitmask 'const' machinery doesn't work without
> > inlining -- in the past as:
> > 
> >   Subject: [PATCH 1/3] bitmap: switch from inline to __always_inline
> >   https://lore.kernel.org/all/20221027043810.350460-2-yury.norov@gmail.com/
> > 
> > It seems like a good idea to at least make all cpumask functions use
> > __always_inline; several already do.
> > 
> > According to bloat-o-meter, my ~29MB vmlinux increases by a total of 61
> > bytes (0.00%) with this change.
> > 
> > [0] CONFIG_HOTPLUG_PARALLEL=y ('select'ed for x86 as of [1]) and
> >     CONFIG_GCOV_PROFILE_ALL.
> > 
> > [1] commit 0c7ffa32dbd6 ("x86/smpboot/64: Implement
> >     arch_cpuhp_init_parallel_bringup() and enable it")
> > 
> > Cc: Yury Norov <yury.norov@gmail.com>
> > Signed-off-by: Brian Norris <briannorris@chromium.org>
> > ---
> > 
> >  include/linux/cpumask.h | 214 +++++++++++++++++++++-------------------
> >  1 file changed, 113 insertions(+), 101 deletions(-)
> 
> Any thoughts here? scripts/get_maintainer.pl suggests you are
> maintainer/reviewer here.

Hi Brian,

I never received the original email, only this reply, and can't recover
any context.

cpumask_andnot() is a pure wrapper around bitmap_andnot(), and it's
really surprising that clang decided to make it an outline function.
Maybe the bitmap_andnot() is one that outlined? Did you apply only
this patch, or my patch for bitmaps too to fix the warning?

Clang people are already in CC. Guys, can you please comment if making
cpumask API __always_inline is OK for you? Why Clang decides to make a
pure wrapper outlined?

I feel that if we decide making cpumask an __always_inline is the
right way, we also should make underlying bitmap API __always_inline
just as well. Otherwise, there will be a chance of having outlined
bitmap helpers, which may confuse clang again.

Thanks,
Yury