linux-kernel - Re: [PATCH] cpumask: Switch from inline to __always

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240703195724.GA292031@thelio-3990X>
Date: Wed, 3 Jul 2024 12:57:24 -0700
From: Nathan Chancellor <nathan@...nel.org>
To: Yury Norov <yury.norov@...il.com>
Cc: Brian Norris <briannorris@...omium.org>,
	Rasmus Villemoes <linux@...musvillemoes.dk>,
	Nick Desaulniers <ndesaulniers@...gle.com>,
	Bill Wendling <morbo@...gle.com>, llvm@...ts.linux.dev,
	linux-kernel@...r.kernel.org, Justin Stitt <justinstitt@...gle.com>
Subject: Re: [PATCH] cpumask: Switch from inline to __always_inline

On Wed, Jul 03, 2024 at 12:06:36PM -0700, Yury Norov wrote:
> 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@...il.com>
> > > Signed-off-by: Brian Norris <briannorris@...omium.org>
> > > ---
> > > 
> > >  include/linux/cpumask.h | 214 +++++++++++++++++++++-------------------
> > >  1 file changed, 113 insertions(+), 101 deletions(-)
...
> 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?

There are a whole host of reasons why Clang's inliner might decide not
to inline a function. There is '-Rpass-missed=inliner', which should
enable optimization remarks for the inliner to maybe see why LLVM
thought it was not profitable to inline these functions, which could be
passed to Kbuild via KCFLAGS=-Rpass-missed=inliner or selectively for a
translation unit by adding 'CFLAGS_<file>.o := -Rpass-missed=inliner' to
whatever translation unit's Makefile that has a warning.

However, given that this appears to only show up when GCOV is enabled,
it is likely just the case that the additional instrumentation makes
certain functions appear unprofitable. Overriding the inliner with
__always_inline is not the end of the world, especially if it is
genuinely beneficial to always inline them. I can see how that would be
the case with these functions.

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

If this does not result in noticeable bloat, this may not be a bad
idea. I seem to recall this being an issue in the past for us but I
cannot seem to find the issue at this point. Commit 1dc01abad654
("cpumask: Always inline helpers which use bit manipulation functions")
comes to mind.

Cheers,
Nathan