From: Andrew Murray <firstname.lastname@example.org>
To: Arnd Bergmann <email@example.com>
Cc: Catalin Marinas <firstname.lastname@example.org>,
Will Deacon <email@example.com>,
Mark Rutland <firstname.lastname@example.org>,
Linux ARM <email@example.com>,
Subject: Re: [PATCH] arm64: fix unreachable code issue with cmpxchg
Date: Tue, 10 Sep 2019 11:24:59 +0100 [thread overview]
Message-ID: <20190910102458.GJ9720@e119886-lin.cambridge.arm.com> (raw)
On Tue, Sep 10, 2019 at 11:38:37AM +0200, Arnd Bergmann wrote:
> On Tue, Sep 10, 2019 at 11:23 AM Andrew Murray <firstname.lastname@example.org> wrote:
> > > arch/arm64/include/asm/cmpxchg.h | 15 ++++++++-------
> > > 1 file changed, 8 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/arch/arm64/include/asm/cmpxchg.h b/arch/arm64/include/asm/cmpxchg.h
> > > index a1398f2f9994..fd64dc8a235f 100644
> > > --- a/arch/arm64/include/asm/cmpxchg.h
> > > +++ b/arch/arm64/include/asm/cmpxchg.h
> > > @@ -19,7 +19,7 @@
> > > * acquire+release for the latter.
> > > */
> > > #define __XCHG_CASE(w, sfx, name, sz, mb, nop_lse, acq, acq_lse, rel, cl) \
> > > -static inline u##sz __xchg_case_##name##sz(u##sz x, volatile void *ptr) \
> > > +static __always_inline u##sz __xchg_case_##name##sz(u##sz x, volatile void *ptr)\
> > This hunk isn't needed, there is no BUILD_BUG here.
> Right, I noticed this, but it seemed like a good idea regardless given the small
> size of the function compared with the overhead of a function call. We clearly
> want these to be inlined all the time.
> Same for the others.
I'm not so sure - isn't the point of something like OPTIMIZE_INLINING to give
more freedom to the tooling (and by virtue of the option - the user)?
Surely any decent optimising compiler will do the right thing by inlining small
trivial functions that are annotated with inline? And if not, the compiler
should be fixed not the kernel - unless of course it causes an issue - and then
we should fix those specific cases.
There must be dozens of trivial functions that are marked with __inline, I
don't think it would make sense to mark those as __always_inline. For example the
atomics in atomic_lse.h are trivial but only marked inline. We obviously want
them inline, though I don't think we should babysit the compiler to do the
(Also the commit message implies that all the hunks are required to fix this
particular issue which they are not).
> > Alternatively is it possible to replace the BUILD_BUG's with something else?
> > I think because we use BUILD_BUG at the end of a switch statement, we make
> > the assumption that size is known at compile time, for this reason we should
> > ensure the function containing the BUILD_BUG is __always_inline.
> > Looking across the kernel where BUILD_BUG is used as a default in a switch
> > statment ($ git grep -B 3 BUILD_BUG\( | grep default), most instances are
> > within macros, but many are found in an __always_inline function:
> > arch/x86/kvm/cpuid.h
> > mm/kasan/generic.c
> > Though some are not:
> > include/linux/signal.h
> > arch/arm64/include/asm/arm_dsu/pmu.h
> > I wonder if there may be a latent mole ready to whack with pmu.h?
> Right, it can't hurt to annotate those as well. I actually have another
> fixup for linux/signal.h that I would have to revisit at some point.
> See https://bugs.llvm.org/show_bug.cgi?id=38789, I think this is
> fixed with clang-9 now, but maybe not with clang-8.
prev parent reply other threads:[~2019-09-10 10:25 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-09-09 20:21 [PATCH] arm64: fix unreachable code issue with cmpxchg Arnd Bergmann
2019-09-09 21:06 ` Nick Desaulniers
2019-09-09 21:35 ` Nick Desaulniers
2019-09-10 3:42 ` Nathan Chancellor
2019-09-10 7:46 ` Will Deacon
2019-09-10 8:04 ` Arnd Bergmann
2019-09-10 13:24 ` Will Deacon
2019-09-10 13:43 ` Arnd Bergmann
2019-09-10 14:21 ` Andrew Murray
2019-09-10 9:23 ` Andrew Murray
2019-09-10 9:38 ` Arnd Bergmann
2019-09-10 10:17 ` Masahiro Yamada
2019-09-10 10:24 ` Andrew Murray [this message]
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).