Date: Wed, 26 Jun 2024 09:54:32 +0200
From: Uros Bizjak <ubizjak@...il.com>
To: Linus Torvalds <torvalds@...ux-foundation.org>
Cc: kernel test robot <lkp@...el.com>, oe-kbuild-all@...ts.linux.dev, 
	linux-kernel@...r.kernel.org, Ingo Molnar <mingo@...nel.org>, 
	Borislav Petkov <bp@...en8.de>, Peter Zijlstra <peterz@...radead.org>
Subject: Re: arch/x86/include/asm/cmpxchg_32.h:149:9: error: inline assembly
 requires more registers than available

On Tue, Jun 25, 2024 at 8:31 PM Linus Torvalds
<torvalds@...ux-foundation.org> wrote:
>
> [ Sorry for not being hugely responsive the last few days - I was on
> the road for a family get-together on the east coast, so I spent time
> on airplanes and hotels and I don't particularly enjoy working with a
> laptop ]
>
> On Mon, 24 Jun 2024 at 08:42, Uros Bizjak <ubizjak@...il.com> wrote:
> >
> > >
> > > I'm sending the patch out in the hope that another set of eyes will
> > > make it actually better.
> >
> > + _lock "cmpxchg8b 0(%[ptr])", X86_FEATURE_CX8) \
> >
> > This can be just:
> >
> > + _lock "cmpxchg8b %a[ptr]", X86_FEATURE_CX8) \
>
> Thanks, yup, will fix.
>
> > - if (unlikely(!ret)) \
> > - *(_oldp) = o.full; \
> > + *(_oldp) = o; \
> >
> > This one should really update only when cmpxchg fails.
>
> The thing is, when cmpxchg doesn't fail, then oldp should already be "old", no?

Also replying here with the reply to your proposed patch, to document
a subtle issue with try_cmpxchg:

You probably want to look at 44fe84459faf1 ("locking/atomic: Fix
atomic_try_cmpxchg() semantics") [1] and the long LKML discussion at
[2].

--quote--
This code is broken with the current implementation, the problem is
with unconditional update of *__po.

In case of success it writes the same value back into *__po, but in
case of cmpxchg success we might have lose ownership of some memory
locations and potentially over what __po has pointed to. The same
holds for the re-read of *__po.
--/quote--

[1] https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/commit/?id=44fe84459faf1a7781595b7c64cd36daf2f2827d
[2] https://lore.kernel.org/lkml/CACT4Y+bG+a0w6j6v1AmBE7fqqMSPyPEm4QimCzCouicmHT8FqA@mail.gmail.com/

Uros.

>
> I mean, by the very definition, atomic_try_cmpxchg() can *not* be
> successful if the new value didn't match the old one.
>
> I mean, just look at the very doc you point to - the "definition" is
>
>   bool atomic_try_cmpxchg(atomic_t *ptr, int *oldp, int new)
>   {
>     int ret, old = *oldp;
>     ret = atomic_cmpxchg(ptr, old, new);
>     if (ret != old)
>       *oldp = ret;
>     return ret == old;
>   }
>
> iow, it only returns success of "ret == old", and "old" by definition
> is "the contents of oldp".
>
> (Here "oldp" is a local variable, not something that can be changing).
>
> So I *think* the whole
>
>     if (ret != old)
>       *oldp = ret;
>
> is actually counter-productive, and could/should be just that simpler
> unconditional *oldp = ret, because you have two cases:
>
>  - ret == old: the assignment doesn't change anything and is a no-op
>
>  - ret !=- old: the assignment needs to be done
>
> but doing it *unconditionally* means that now as fat as the compiler
> is concerned, the original *oldp value is unconditionally dead, which
> sounds to me like it should be good for register allocation (the
> context here being that it _looks_ like a pointer access, but it's
> really meant to be a "in-out argument in a register").
>
> Now, in practice, I suspect that everybody checks the return value and
> "old" is never used afterwards in the success case, so in that sense
> this doesn't matter and it's all dead regardless.
>
> But it seems to be a complication in the docs and the implementation.
>
> Of course, I may be missing something completely obvious, and/or you
> have some subtle code generation reason why you prefer the conditional
> there. Feel free to explain,
>
>              Linus

Powered by blists - more mailing lists