linux-kernel - Re: arch/x86/include/asm/cmpxchg_32.h:149:9: error: inline assembly requires more registers than available

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: <CAHk-=wiWEgSo2Tb_bih7mnS27zAPL+RGg_7yX4qK1f710-j-Ng@mail.gmail.com>
Date: Tue, 25 Jun 2024 11:31:05 -0700
From: Linus Torvalds <torvalds@...ux-foundation.org>
To: Uros Bizjak <ubizjak@...il.com>
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

[ 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?

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