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

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

[kernel test robot]

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
head:   5f583a3162ffd9f7999af76b8ab634ce2dac9f90
commit: 95ece48165c136b96fae0f6144f55cbf8b24aeb9 locking/atomic/x86: Rewrite x86_32 arch_atomic64_{,fetch}_{and,or,xor}() functions
date:   2 months ago
config: i386-randconfig-061-20240623 (https://download.01.org/0day-ci/archive/20240623/202406230912.F6XFIyA6-lkp@intel.com/config)
compiler: clang version 18.1.5 (https://github.com/llvm/llvm-project 617a15a9eac96088ae5e9134248d8236e34b91b1)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240623/202406230912.F6XFIyA6-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202406230912.F6XFIyA6-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from kernel/bpf/core.c:21:
   In file included from include/linux/filter.h:8:
   In file included from include/linux/atomic.h:7:
   In file included from arch/x86/include/asm/atomic.h:8:
   In file included from arch/x86/include/asm/cmpxchg.h:143:
>> arch/x86/include/asm/cmpxchg_32.h:149:9: error: inline assembly requires more registers than available
     149 |         return __arch_try_cmpxchg64_emu(ptr, oldp, new);
         |                ^
   arch/x86/include/asm/cmpxchg_32.h:131:15: note: expanded from macro '__arch_try_cmpxchg64_emu'
     131 |         asm volatile(ALTERNATIVE(LOCK_PREFIX_HERE                       \
         |                      ^
   arch/x86/include/asm/alternative.h:218:2: note: expanded from macro 'ALTERNATIVE'
     218 |         OLDINSTR(oldinstr, 1)                                           \
         |         ^
   arch/x86/include/asm/alternative.h:168:2: note: expanded from macro 'OLDINSTR'
     168 |         "# ALT: oldnstr\n"                                              \
         |         ^
   In file included from kernel/bpf/core.c:21:
   In file included from include/linux/filter.h:8:
   In file included from include/linux/atomic.h:7:
   In file included from arch/x86/include/asm/atomic.h:8:
   In file included from arch/x86/include/asm/cmpxchg.h:143:
>> arch/x86/include/asm/cmpxchg_32.h:149:9: error: inline assembly requires more registers than available
   arch/x86/include/asm/cmpxchg_32.h:131:15: note: expanded from macro '__arch_try_cmpxchg64_emu'
     131 |         asm volatile(ALTERNATIVE(LOCK_PREFIX_HERE                       \
         |                      ^
   arch/x86/include/asm/alternative.h:218:2: note: expanded from macro 'ALTERNATIVE'
     218 |         OLDINSTR(oldinstr, 1)                                           \
         |         ^
   arch/x86/include/asm/alternative.h:168:2: note: expanded from macro 'OLDINSTR'
     168 |         "# ALT: oldnstr\n"                                              \
         |         ^
   In file included from kernel/bpf/core.c:21:
   In file included from include/linux/filter.h:8:
   In file included from include/linux/atomic.h:7:
   In file included from arch/x86/include/asm/atomic.h:8:
   In file included from arch/x86/include/asm/cmpxchg.h:143:
>> arch/x86/include/asm/cmpxchg_32.h:149:9: error: inline assembly requires more registers than available
   arch/x86/include/asm/cmpxchg_32.h:131:15: note: expanded from macro '__arch_try_cmpxchg64_emu'
     131 |         asm volatile(ALTERNATIVE(LOCK_PREFIX_HERE                       \
         |                      ^
   arch/x86/include/asm/alternative.h:218:2: note: expanded from macro 'ALTERNATIVE'
     218 |         OLDINSTR(oldinstr, 1)                                           \
         |         ^
   arch/x86/include/asm/alternative.h:168:2: note: expanded from macro 'OLDINSTR'
     168 |         "# ALT: oldnstr\n"                                              \
         |         ^
   In file included from kernel/bpf/core.c:21:
   In file included from include/linux/filter.h:8:
   In file included from include/linux/atomic.h:7:
   In file included from arch/x86/include/asm/atomic.h:8:
   In file included from arch/x86/include/asm/cmpxchg.h:143:
>> arch/x86/include/asm/cmpxchg_32.h:149:9: error: inline assembly requires more registers than available
   arch/x86/include/asm/cmpxchg_32.h:131:15: note: expanded from macro '__arch_try_cmpxchg64_emu'
     131 |         asm volatile(ALTERNATIVE(LOCK_PREFIX_HERE                       \
         |                      ^
   arch/x86/include/asm/alternative.h:218:2: note: expanded from macro 'ALTERNATIVE'
     218 |         OLDINSTR(oldinstr, 1)                                           \
         |         ^
   arch/x86/include/asm/alternative.h:168:2: note: expanded from macro 'OLDINSTR'
     168 |         "# ALT: oldnstr\n"                                              \
         |         ^
   In file included from kernel/bpf/core.c:21:
   In file included from include/linux/filter.h:8:
   In file included from include/linux/atomic.h:7:
   In file included from arch/x86/include/asm/atomic.h:8:
   In file included from arch/x86/include/asm/cmpxchg.h:143:
>> arch/x86/include/asm/cmpxchg_32.h:149:9: error: inline assembly requires more registers than available
   arch/x86/include/asm/cmpxchg_32.h:131:15: note: expanded from macro '__arch_try_cmpxchg64_emu'
     131 |         asm volatile(ALTERNATIVE(LOCK_PREFIX_HERE                       \
         |                      ^
   arch/x86/include/asm/alternative.h:218:2: note: expanded from macro 'ALTERNATIVE'
     218 |         OLDINSTR(oldinstr, 1)                                           \
         |         ^
   arch/x86/include/asm/alternative.h:168:2: note: expanded from macro 'OLDINSTR'
     168 |         "# ALT: oldnstr\n"                                              \
         |         ^
   In file included from kernel/bpf/core.c:21:
   In file included from include/linux/filter.h:8:
   In file included from include/linux/atomic.h:7:
   In file included from arch/x86/include/asm/atomic.h:8:
   In file included from arch/x86/include/asm/cmpxchg.h:143:
>> arch/x86/include/asm/cmpxchg_32.h:149:9: error: inline assembly requires more registers than available
   arch/x86/include/asm/cmpxchg_32.h:131:15: note: expanded from macro '__arch_try_cmpxchg64_emu'
     131 |         asm volatile(ALTERNATIVE(LOCK_PREFIX_HERE                       \
         |                      ^
   arch/x86/include/asm/alternative.h:218:2: note: expanded from macro 'ALTERNATIVE'
     218 |         OLDINSTR(oldinstr, 1)                                           \
         |         ^
   arch/x86/include/asm/alternative.h:168:2: note: expanded from macro 'OLDINSTR'
     168 |         "# ALT: oldnstr\n"                                              \
         |         ^
   In file included from kernel/bpf/core.c:21:
   In file included from include/linux/filter.h:8:
   In file included from include/linux/atomic.h:7:
   In file included from arch/x86/include/asm/atomic.h:8:
   In file included from arch/x86/include/asm/cmpxchg.h:143:
>> arch/x86/include/asm/cmpxchg_32.h:149:9: error: inline assembly requires more registers than available
   arch/x86/include/asm/cmpxchg_32.h:131:15: note: expanded from macro '__arch_try_cmpxchg64_emu'
     131 |         asm volatile(ALTERNATIVE(LOCK_PREFIX_HERE                       \
         |                      ^
   arch/x86/include/asm/alternative.h:218:2: note: expanded from macro 'ALTERNATIVE'
     218 |         OLDINSTR(oldinstr, 1)                                           \
         |         ^
   arch/x86/include/asm/alternative.h:168:2: note: expanded from macro 'OLDINSTR'
     168 |         "# ALT: oldnstr\n"                                              \
         |         ^
   7 errors generated.


vim +149 arch/x86/include/asm/cmpxchg_32.h

aef95dac9ce4f2 Uros Bizjak 2024-04-08  146  
aef95dac9ce4f2 Uros Bizjak 2024-04-08  147  static __always_inline bool arch_try_cmpxchg64(volatile u64 *ptr, u64 *oldp, u64 new)
aef95dac9ce4f2 Uros Bizjak 2024-04-08  148  {
aef95dac9ce4f2 Uros Bizjak 2024-04-08 @149  	return __arch_try_cmpxchg64_emu(ptr, oldp, new);
aef95dac9ce4f2 Uros Bizjak 2024-04-08  150  }
aef95dac9ce4f2 Uros Bizjak 2024-04-08  151  #define arch_try_cmpxchg64 arch_try_cmpxchg64
aef95dac9ce4f2 Uros Bizjak 2024-04-08  152  

:::::: The code at line 149 was first introduced by commit
:::::: aef95dac9ce4f271cc43195ffc175114ed934cbe locking/atomic/x86: Introduce arch_try_cmpxchg64() for !CONFIG_X86_CMPXCHG64

:::::: TO: Uros Bizjak <ubizjak@gmail.com>
:::::: CC: Ingo Molnar <mingo@kernel.org>

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[Uros Bizjak]

On Sun, Jun 23, 2024 at 4:02 AM kernel test robot <lkp@intel.com> wrote:
>
> tree:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> head:   5f583a3162ffd9f7999af76b8ab634ce2dac9f90
> commit: 95ece48165c136b96fae0f6144f55cbf8b24aeb9 locking/atomic/x86: Rewrite x86_32 arch_atomic64_{,fetch}_{and,or,xor}() functions
> date:   2 months ago
> config: i386-randconfig-061-20240623 (https://download.01.org/0day-ci/archive/20240623/202406230912.F6XFIyA6-lkp@intel.com/config)
> compiler: clang version 18.1.5 (https://github.com/llvm/llvm-project 617a15a9eac96088ae5e9134248d8236e34b91b1)
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240623/202406230912.F6XFIyA6-lkp@intel.com/reproduce)
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202406230912.F6XFIyA6-lkp@intel.com/
>
> All errors (new ones prefixed by >>):
>
>    In file included from kernel/bpf/core.c:21:
>    In file included from include/linux/filter.h:8:
>    In file included from include/linux/atomic.h:7:
>    In file included from arch/x86/include/asm/atomic.h:8:
>    In file included from arch/x86/include/asm/cmpxchg.h:143:
> >> arch/x86/include/asm/cmpxchg_32.h:149:9: error: inline assembly requires more registers than available
>      149 |         return __arch_try_cmpxchg64_emu(ptr, oldp, new);

The referred patch actually mitigates the issue, as explained in
details here [1].

[1] https://lore.kernel.org/lkml/CAFULd4b8kRgy0p3pKwheLRHx7rX+4RuY-45_Zt-CCj5sCpGjCw@mail.gmail.com/

Based on the above explanation, the build will break even harder
without the patch, because *TWO* additional registers are needed to
carry the value to be compared over cmpxchg8b instruction. The trouble
that register allocator goes through can be seen at [2] and [3].

[2] https://lore.kernel.org/lkml/20240410062957.322614-1-ubizjak@gmail.com/
[3] https://lore.kernel.org/lkml/20240410062957.322614-3-ubizjak@gmail.com/

Up to now, the atomic64_{and,or,xor} functions (and their derivatives)
were/are seldom used in the kernel. Recently, their usage increased,
so the register pressure situation around cmpxchg8b instruction
worsened. For some reason, x86 implementations of these functions are
declared as inline functions, in contrast with much more used
atomic64_{inc,dec,add,sub}, which are declared as outline functions.
While using try_cmpxchg() instead of cmpxchg() helps, it can only go
so far.

The real and reliable fix is to re-declare x86
arch_atomic64_{and,or,xor} functions as outline functions in the same
way x86 arch_atomic64_{inc,dec,add,sub} are declared. The
infrastructure is already there, the assembly macros just need to be
generalized slightly to handle additional asm primitives.

I started this fix some time ago [4] by optimizing the infrastructure
first that could relax some register pressure, but there was no
reaction from x86 maintainers. I don't want to waste my time on
patches that get ignored, so I dropped the ball.

[4] https://lore.kernel.org/lkml/20240605181424.3228-1-ubizjak@gmail.com/

So, without some interest from x86 maintainers, the issue will remain
unfixed. If they want x86_32 arch to die then inline locking
primitives involving cmpxchg8b are one of the nails in its coffin.

Uros.

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

[Linus Torvalds]

On Sun, 23 Jun 2024 at 06:14, Uros Bizjak <ubizjak@gmail.com> wrote:
>
> The referred patch actually mitigates the issue, as explained in
> details here [1].

Clearly not.

It might help things in theory, but in practice it clearly just makes
things worse, and causes problems for clang.

And the thing about practice is that it always trumps theory.

> So, without some interest from x86 maintainers, the issue will remain
> unfixed. If they want x86_32 arch to die then inline locking
> primitives involving cmpxchg8b are one of the nails in its coffin.

What? No.

That commit will get reverted. It caused problems, and now you are
blaming others.

You can't just go "I caused issues, but the x86 maintainers aren't
interested in this area, so it's their fault".

And no, "improved code generation with one compiler version" does not
then mean "other compilers don't matter".

Now, I do suspect that the issue is some very random thing that could
be fixed by massaging the inline asm a bit.

For example, why does that 32-bit __arch_try_cmpxchg64() do this:

        if (unlikely(!ret))                                             \
                *(_oldp) = o.full;                                      \

when I think it would be simpler and more straightforward to just do
that *(_oldp) = o.full unconditionally? The "dereference" is purely
syntactic, the intent is that the compiler will just use a register
for this all (well, two registers on 32-bit), and making it
conditional makes things worse, because now that "oldp" register
cannot be the same as "o" itself.

I didn't look very closely into this, but maybe that would just fix
code generation. And maybe it wouldn't. If there's no other fix
somebody comes up with, we just have to revert.

                 Linus

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

[Linus Torvalds]

On Sun, 23 Jun 2024 at 11:42, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> For example, why does that 32-bit __arch_try_cmpxchg64() do this:
>
>         if (unlikely(!ret))                                             \
>                 *(_oldp) = o.full;                                      \
>
> when I think it would be simpler and more straightforward to just do
> that *(_oldp) = o.full unconditionally?

I can repro the clang issue, and no, removing the conditional doesn't fix it.

A plain revert of

   95ece48165c1 ("locking/atomic/x86: Rewrite x86_32
arch_atomic64_{,fetch}_{and,or,xor}() functions")

does fix it.

The problem does *NOT* happen with a defconfig, so it's clearly
triggered by some horror in that config:

    https://download.01.org/0day-ci/archive/20240623/202406230912.F6XFIyA6-lkp@intel.com/config

and a quick config bisection seems to imply that it might just be

 X86_MINIMUM_CPU_FAMILY 6 -> 4

from setting CONFIG_MGEODEGX1=y instead of M686.

Hmm. While I'm not willing to leave x86-32 behind, I personally have
thought for a couple of years that we should leave Pentium and earlier
behind. We should just require cmpxchg64b support.

It does look like the complication at least partly comes from the
emulation alternative, but I didn't go look any closer.

                Linus

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

[Uros Bizjak]

On Sun, Jun 23, 2024 at 5:42 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Sun, 23 Jun 2024 at 06:14, Uros Bizjak <ubizjak@gmail.com> wrote:
> >
> > The referred patch actually mitigates the issue, as explained in
> > details here [1].
>
> Clearly not.
>
> It might help things in theory, but in practice it clearly just makes
> things worse, and causes problems for clang.
>
> And the thing about practice is that it always trumps theory.
>
> > So, without some interest from x86 maintainers, the issue will remain
> > unfixed. If they want x86_32 arch to die then inline locking
> > primitives involving cmpxchg8b are one of the nails in its coffin.
>
> What? No.
>
> That commit will get reverted. It caused problems, and now you are
> blaming others.
>
> You can't just go "I caused issues, but the x86 maintainers aren't
> interested in this area, so it's their fault".

I disagree with the above. There is no one to blame, let alone blaming others.

The cmpxchg8b is a complex beast, It needs 4 input registers (a pair
of them inout) and optionally 2 address registers. When the frame
pointer is active, the instruction can consume all available registers
on x86_32, so the compiler has to work extremely hard to satisfy the
required insn constraints (and also pass live values over the
instruction). The two examples I quoted in my previous mail just
illustrate this.

> And no, "improved code generation with one compiler version" does not
> then mean "other compilers don't matter".

Using try_cmpxchg *improves* the situation, as there is a double-word
value that does not have to be live over the insn. So, leaving the
situation as it is will just trigger the issue later. We had exactly
these problems in GCC with __atomic builtins, and GCC learned how to
handle this extreme register pressure.

> Now, I do suspect that the issue is some very random thing that could
> be fixed by massaging the inline asm a bit.
>
> For example, why does that 32-bit __arch_try_cmpxchg64() do this:
>
>         if (unlikely(!ret))                                             \
>                 *(_oldp) = o.full;                                      \
>
> when I think it would be simpler and more straightforward to just do
> that *(_oldp) = o.full unconditionally? The "dereference" is purely
> syntactic, the intent is that the compiler will just use a register
> for this all (well, two registers on 32-bit), and making it
> conditional makes things worse, because now that "oldp" register
> cannot be the same as "o" itself.

The  primitive is written in this way because we would like to use it
in a cmpxchg loop, as well as as a condition in an if clause. Without
the above referred condition, the compiler will emit a dead store in
the later case. I have tested this macro extensively, and the above is
really the best we can do, it results in a tight cmpxchg loop and
without unneeded moves/stores in code without loops.

> I didn't look very closely into this, but maybe that would just fix
> code generation. And maybe it wouldn't. If there's no other fix
> somebody comes up with, we just have to revert.

As said in my previous message, if the compiler can't handle
__try_cmpxchg implementation, the most straightforward and reliable
solution is to implement atomic64_{and,or,xor} as outline functions,

Uros.

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

[Linus Torvalds]

On Sun, 23 Jun 2024 at 13:43, Uros Bizjak <ubizjak@gmail.com> wrote:
>
> I disagree with the above.

Your disagreement just doesn't matter.

We don't introduce regressions and then blame others.

There's a very clear rule in kernel development: things that break
other things ARE NOT FIXES.

EVER.

They get reverted, or the thing they broke gets fixed.

This is not debatable, or something you can "disagree"' with. This is
how we work, and if you disagree with that, you had better get out of
kernel development.

> As said in my previous message, if the compiler can't handle
> __try_cmpxchg implementation, the most straightforward and reliable
> solution is to implement atomic64_{and,or,xor} as outline functions,

You also seemed to say that nobody was doing it.

Which means "revert". Because that I can do trivially.

Now, from having looked a bit at this, I can point you to the
differences introduced by having to have the emulation fallback.

And I would personally be ok with leaving Pentium etc entirely behind,
and getting rid of said fallback, but that's an entirely separate
thing that will require much more discussion.

Some people still wanted to support old 486 clones not that long ago.

             Linus

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

[Linus Torvalds]

On Sun, 23 Jun 2024 at 13:48, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> Now, from having looked a bit at this, I can point you to the
> differences introduced by having to have the emulation fallback.

Ahh.

Itr does all the same things *and* it has

    "S" (_ptr)

as an added register pressure, because if we take the "call
cmpxchg8b_emu", we need the address in a fixed place.

I don't see any immediately obvious workaround.

              Linus

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

[Uros Bizjak]

On Sun, Jun 23, 2024 at 7:49 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Sun, 23 Jun 2024 at 13:43, Uros Bizjak <ubizjak@gmail.com> wrote:
> >
> > I disagree with the above.
>
> Your disagreement just doesn't matter.
>
> We don't introduce regressions and then blame others.
>
> There's a very clear rule in kernel development: things that break
> other things ARE NOT FIXES.
>
> EVER.
>
> They get reverted, or the thing they broke gets fixed.
>
> This is not debatable, or something you can "disagree"' with. This is
> how we work, and if you disagree with that, you had better get out of
> kernel development.

I disagree with *I tried to shift the blame to others*! I take full
responsibility for my patch, and I'm perfectly capable of fixing the
breakage with an alternative approach.

I'm OK with the revert, but it won't fix the underlying problem.
Please see the definition of __arch_cmpxchg64_emu - it forces the
address to %esi registers in the same way as __arch_try_cmpxchg64_emu.
Effectively, the compiler allocates 5 input registers just for the
instruction.

The alternative is to implement atomic64_{and,or,xor} as an outline
function, in the same way as e.g. arch_atomic64_add is implemented.
This will avoid the call to __arch_try_cmpxchg64_emu, and the whole
"instruction" will use just:

__alternative_atomic64(add, add_return,
      ASM_OUTPUT2("+A" (i), "+c" (v)),
      ASM_NO_INPUT_CLOBBER("memory"));

> Now, from having looked a bit at this, I can point you to the
> differences introduced by having to have the emulation fallback.

Yes, I know this - I also (runtime!) tested the emulation, but with GCC only.

> And I would personally be ok with leaving Pentium etc entirely behind,
> and getting rid of said fallback, but that's an entirely separate
> thing that will require much more discussion.
>
> Some people still wanted to support old 486 clones not that long ago.

This can be achieved by implementing atomic64_{and,or,xor} as an
outline function.

Uros.

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

[Linus Torvalds]

On Sun, 23 Jun 2024 at 14:14, Uros Bizjak <ubizjak@gmail.com> wrote:
>
> I disagree with *I tried to shift the blame to others*! I take full
> responsibility for my patch, and I'm perfectly capable of fixing the
> breakage with an alternative approach.

Ah, good. This is something I'm a bit sensitive about, just because
there's been so many arguments over it over the years, so now I go
into "preemptive nuclear mode" when the regression issue comes up.

Sorry.

> I'm OK with the revert, but it won't fix the underlying problem.
> Please see the definition of __arch_cmpxchg64_emu - it forces the
> address to %esi registers in the same way as __arch_try_cmpxchg64_emu.
> Effectively, the compiler allocates 5 input registers just for the
> instruction.

Oh, I entirely agree that this is a "random compiler implementation"
issue, and then the code around it makes all the difference.

> > Now, from having looked a bit at this, I can point you to the
> > differences introduced by having to have the emulation fallback.
>
> Yes, I know this - I also (runtime!) tested the emulation, but with GCC only.

Yeah, crossed emails, I started out just doing the "let's see what the
config difference is", and only after that realized that I had looked
at the wrong code for cmpxchg (ie I had looked at the simpler native
case).

> This can be achieved by implementing atomic64_{and,or,xor} as an
> outline function.

Yes, but then a lot of the whole point of commit 95ece48165c1 goes
away, doesn't it?

Or were you suggesting the out-of-line code only for the emulation
case? That would work.

             Linus

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

[Uros Bizjak]

On Sun, Jun 23, 2024 at 8:25 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:

> Ah, good. This is something I'm a bit sensitive about, just because
> there's been so many arguments over it over the years, so now I go
> into "preemptive nuclear mode" when the regression issue comes up.
>
> Sorry.

Also sorry from my side if my disagreement was understood as a
criticism of the kernel development process. I'm totally OK with it
(but ignored patches indeed cause a bit of frustration...).

> > I'm OK with the revert, but it won't fix the underlying problem.
> > Please see the definition of __arch_cmpxchg64_emu - it forces the
> > address to %esi registers in the same way as __arch_try_cmpxchg64_emu.
> > Effectively, the compiler allocates 5 input registers just for the
> > instruction.
>
> Oh, I entirely agree that this is a "random compiler implementation"
> issue, and then the code around it makes all the difference.
>
> > > Now, from having looked a bit at this, I can point you to the
> > > differences introduced by having to have the emulation fallback.
> >
> > Yes, I know this - I also (runtime!) tested the emulation, but with GCC only.
>
> Yeah, crossed emails, I started out just doing the "let's see what the
> config difference is", and only after that realized that I had looked
> at the wrong code for cmpxchg (ie I had looked at the simpler native
> case).
>
> > This can be achieved by implementing atomic64_{and,or,xor} as an
> > outline function.
>
> Yes, but then a lot of the whole point of commit 95ece48165c1 goes
> away, doesn't it?

True, because this commit pushed one of the compilers over the edge.

> Or were you suggesting the out-of-line code only for the emulation
> case? That would work.

I am suggesting simply following the approach of
arch_atomic64_{add,sub}{_return} in atomic64_32.h. These functions are
used extensively in the kernel, and if they didn't cause any problems,
then we can rightfully expect that the new ones also won't. This
approach will relax the register pressure, so we won't expect magic
from the compiler.

I can provide a patch series with the revert and a fix in a couple of
days (I'll be away from the keyboard for a short time).

Uros.

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

[Uros Bizjak]

On Sun, Jun 23, 2024 at 8:07 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Sun, 23 Jun 2024 at 13:48, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > Now, from having looked a bit at this, I can point you to the
> > differences introduced by having to have the emulation fallback.
>
> Ahh.
>
> Itr does all the same things *and* it has
>
>     "S" (_ptr)
>
> as an added register pressure, because if we take the "call
> cmpxchg8b_emu", we need the address in a fixed place.
>
> I don't see any immediately obvious workaround.

A real fix, not only a workaround, is to rewrite asm arguments to
something like (untested, but "should work"TM):

#define __arch_cmpxchg64_emu(_ptr, _old, _new, _lock_loc, _lock) \
({ \
union __u64_halves o = { .full = (_old), }, \
  n = { .full = (_new), }; \
\
asm volatile(ALTERNATIVE(_lock_loc \
"call cmpxchg8b_emu", \
_lock "cmpxchg8b %a[ptr]", X86_FEATURE_CX8) \
    : "+a" (o.low), "+d" (o.high) \
    : [ptr] "S" (_ptr), \
      "b" (n.low), "c" (n.high) \
    : "memory"); \
\
o.full; \
})

(and in a similar way for __arch_try_cmpxchg64_emu).

We already have a memory clobber in the asm template, so to solve the
register pressure issue, just reuse the pointer in %esi to also form
the memory address of cmpxchg8b. GCC is smart enough to do this by
itself, but the current asm template indeed doesn't force "other"
compilers to do so.

Uros.

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

[Linus Torvalds]

On Mon, 24 Jun 2024 at 03:36, Uros Bizjak <ubizjak@gmail.com> wrote:
>
> A real fix, not only a workaround, is to rewrite asm arguments to
> something like (untested, but "should work"TM):

Sadly, I already tried that, and it didn't help.

             Linus

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

[Linus Torvalds]

[-- Attachment #1: Type: text/plain, Size: 1015 bytes --]

On Mon, 24 Jun 2024 at 09:59, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Mon, 24 Jun 2024 at 03:36, Uros Bizjak <ubizjak@gmail.com> wrote:
> >
> > A real fix, not only a workaround, is to rewrite asm arguments to
> > something like (untested, but "should work"TM):
>
> Sadly, I already tried that, and it didn't help.

Bah. I _had_ tried that, but I had tried it on __arch_cmpxchg64_emu(),
not on the "try" version.

And no, it hadn't helped.

But doing it *right*, and also doing __arch_try_cmpxchg64_emu() *does*
actually help.

Well, at least the attached patch compiles with the bad config and clang.

I'm not certain it does the right thing, because I did more than just
the %esi games to try to simplify it.

It's *ENTIRELY* possible that I screwed up. Just in this thread, I
have looked at the wrong inline asm at least twice now, so I'm not
feeling super-competent right now.

I'm sending the patch out in the hope that another set of eyes will
make it actually better.

            Linus

[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch, Size: 2244 bytes --]

 arch/x86/include/asm/cmpxchg_32.h | 28 ++++++++++++----------------
 1 file changed, 12 insertions(+), 16 deletions(-)

diff --git a/arch/x86/include/asm/cmpxchg_32.h b/arch/x86/include/asm/cmpxchg_32.h
index ed2797f132ce..b19985520f3c 100644
--- a/arch/x86/include/asm/cmpxchg_32.h
+++ b/arch/x86/include/asm/cmpxchg_32.h
@@ -88,18 +88,17 @@ static __always_inline bool __try_cmpxchg64_local(volatile u64 *ptr, u64 *oldp,
 
 #define __arch_cmpxchg64_emu(_ptr, _old, _new, _lock_loc, _lock)	\
 ({									\
-	union __u64_halves o = { .full = (_old), },			\
-			   n = { .full = (_new), };			\
+	__u64 o = (_old);						\
+	union __u64_halves n = { .full = (_new), };			\
 									\
 	asm volatile(ALTERNATIVE(_lock_loc				\
 				 "call cmpxchg8b_emu",			\
-				 _lock "cmpxchg8b %[ptr]", X86_FEATURE_CX8) \
-		     : [ptr] "+m" (*(_ptr)),				\
-		       "+a" (o.low), "+d" (o.high)			\
-		     : "b" (n.low), "c" (n.high), "S" (_ptr)		\
+				 _lock "cmpxchg8b 0(%[ptr])", X86_FEATURE_CX8) \
+		     : "+A" (o)						\
+		     : "b" (n.low), "c" (n.high), [ptr] "S" (_ptr)	\
 		     : "memory");					\
 									\
-	o.full;								\
+	o;								\
 })
 
 static __always_inline u64 arch_cmpxchg64(volatile u64 *ptr, u64 old, u64 new)
@@ -116,22 +115,19 @@ static __always_inline u64 arch_cmpxchg64_local(volatile u64 *ptr, u64 old, u64
 
 #define __arch_try_cmpxchg64_emu(_ptr, _oldp, _new, _lock_loc, _lock)	\
 ({									\
-	union __u64_halves o = { .full = *(_oldp), },			\
-			   n = { .full = (_new), };			\
+	__u64 o = *(_oldp);						\
+	union __u64_halves n = { .full = (_new), };			\
 	bool ret;							\
 									\
 	asm volatile(ALTERNATIVE(_lock_loc				\
 				 "call cmpxchg8b_emu",			\
-				 _lock "cmpxchg8b %[ptr]", X86_FEATURE_CX8) \
+				 _lock "cmpxchg8b 0(%[ptr])", X86_FEATURE_CX8) \
 		     CC_SET(e)						\
-		     : CC_OUT(e) (ret),					\
-		       [ptr] "+m" (*(_ptr)),				\
-		       "+a" (o.low), "+d" (o.high)			\
-		     : "b" (n.low), "c" (n.high), "S" (_ptr)		\
+		     : CC_OUT(e) (ret), "+A" (o)			\
+		     : "b" (n.low), "c" (n.high), [ptr] "S" (_ptr)	\
 		     : "memory");					\
 									\
-	if (unlikely(!ret))						\
-		*(_oldp) = o.full;					\
+	*(_oldp) = o;							\
 									\
 	likely(ret);							\
 })

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

[Uros Bizjak]

On Mon, Jun 24, 2024 at 4:18 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Mon, 24 Jun 2024 at 09:59, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > On Mon, 24 Jun 2024 at 03:36, Uros Bizjak <ubizjak@gmail.com> wrote:
> > >
> > > A real fix, not only a workaround, is to rewrite asm arguments to
> > > something like (untested, but "should work"TM):
> >
> > Sadly, I already tried that, and it didn't help.
>
> Bah. I _had_ tried that, but I had tried it on __arch_cmpxchg64_emu(),
> not on the "try" version.
>
> And no, it hadn't helped.
>
> But doing it *right*, and also doing __arch_try_cmpxchg64_emu() *does*
> actually help.
>
> Well, at least the attached patch compiles with the bad config and clang.
>
> I'm not certain it does the right thing, because I did more than just
> the %esi games to try to simplify it.
>
> It's *ENTIRELY* possible that I screwed up. Just in this thread, I
> have looked at the wrong inline asm at least twice now, so I'm not
> feeling super-competent right now.
>
> 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) \

- if (unlikely(!ret)) \
- *(_oldp) = o.full; \
+ *(_oldp) = o; \

This one should really update only when cmpxchg fails. The relation
between cmpxchg and try_cmpxchg is documented in
Documentation/atomic_t.txt, section CMPXCHG vs TRY_CMPXCHG.

Does the compilation still crash with the condition?

Uros.

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

[Linus Torvalds]

[ 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@gmail.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

[PATCH] x86-32: fix cmpxchg8b_emu build error with clang

[Linus Torvalds]

The kernel test robot reported that clang no longer compiles the 32-bit
x86 kernel in some configurations due to commit 95ece48165c1
("locking/atomic/x86: Rewrite x86_32 arch_atomic64_{,fetch}_{and,or,xor}()
functions").

The build fails with

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

and the reason seems to be that not only does the cmpxchg8b instruction
need four fixed registers (EDX:EAX and ECX:EBX), with the emulation
fallback the inline asm also wants a fifth fixed register for the
address (it uses %esi for that, but that's just a software convention
with cmpxchg8b_emu).

Avoiding using another pointer input to the asm (and just forcing it to
use the "0(%esi)" addressing that we end up requiring for the sw
fallback) seems to fix the issue.

Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202406230912.F6XFIyA6-lkp@intel.com/
Fixes: 95ece48165c1 ("locking/atomic/x86: Rewrite x86_32 arch_atomic64_{,fetch}_{and,or,xor}() functions")
Link: https://lore.kernel.org/all/202406230912.F6XFIyA6-lkp@intel.com/
Suggested-by: Uros Bizjak <ubizjak@gmail.com>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---

Added commit message, and updated the asm to use '%a[ptr]' instead of
writing out the addressing by hand. 

Still doing the 'oldp' writeback unconmditionally.  The code generation
for the case I checked were the same for both clang and gcc, but until
Uros hits me with the big clue-hammer, I think it's the simpler code
that leaves room for potentially better optimizations too. 

This falls solidly in the "looks ok to me, but still untested" category
for me.  It fixes the clang build issue in my build testing, but I no
longer have a 32-bit test environment, so no actual runtime testing.

 arch/x86/include/asm/cmpxchg_32.h | 28 ++++++++++++----------------
 1 file changed, 12 insertions(+), 16 deletions(-)

diff --git a/arch/x86/include/asm/cmpxchg_32.h b/arch/x86/include/asm/cmpxchg_32.h
index ed2797f132ce..4444a8292c7a 100644
--- a/arch/x86/include/asm/cmpxchg_32.h
+++ b/arch/x86/include/asm/cmpxchg_32.h
@@ -88,18 +88,17 @@ static __always_inline bool __try_cmpxchg64_local(volatile u64 *ptr, u64 *oldp,
 
 #define __arch_cmpxchg64_emu(_ptr, _old, _new, _lock_loc, _lock)	\
 ({									\
-	union __u64_halves o = { .full = (_old), },			\
-			   n = { .full = (_new), };			\
+	__u64 o = (_old);						\
+	union __u64_halves n = { .full = (_new), };			\
 									\
 	asm volatile(ALTERNATIVE(_lock_loc				\
 				 "call cmpxchg8b_emu",			\
-				 _lock "cmpxchg8b %[ptr]", X86_FEATURE_CX8) \
-		     : [ptr] "+m" (*(_ptr)),				\
-		       "+a" (o.low), "+d" (o.high)			\
-		     : "b" (n.low), "c" (n.high), "S" (_ptr)		\
+				 _lock "cmpxchg8b %a[ptr]", X86_FEATURE_CX8) \
+		     : "+A" (o)						\
+		     : "b" (n.low), "c" (n.high), [ptr] "S" (_ptr)	\
 		     : "memory");					\
 									\
-	o.full;								\
+	o;								\
 })
 
 static __always_inline u64 arch_cmpxchg64(volatile u64 *ptr, u64 old, u64 new)
@@ -116,22 +115,19 @@ static __always_inline u64 arch_cmpxchg64_local(volatile u64 *ptr, u64 old, u64
 
 #define __arch_try_cmpxchg64_emu(_ptr, _oldp, _new, _lock_loc, _lock)	\
 ({									\
-	union __u64_halves o = { .full = *(_oldp), },			\
-			   n = { .full = (_new), };			\
+	__u64 o = *(_oldp);						\
+	union __u64_halves n = { .full = (_new), };			\
 	bool ret;							\
 									\
 	asm volatile(ALTERNATIVE(_lock_loc				\
 				 "call cmpxchg8b_emu",			\
-				 _lock "cmpxchg8b %[ptr]", X86_FEATURE_CX8) \
+				 _lock "cmpxchg8b %a[ptr]", X86_FEATURE_CX8) \
 		     CC_SET(e)						\
-		     : CC_OUT(e) (ret),					\
-		       [ptr] "+m" (*(_ptr)),				\
-		       "+a" (o.low), "+d" (o.high)			\
-		     : "b" (n.low), "c" (n.high), "S" (_ptr)		\
+		     : CC_OUT(e) (ret), "+A" (o)			\
+		     : "b" (n.low), "c" (n.high), [ptr] "S" (_ptr)	\
 		     : "memory");					\
 									\
-	if (unlikely(!ret))						\
-		*(_oldp) = o.full;					\
+	*(_oldp) = o;							\
 									\
 	likely(ret);							\
 })
-- 
2.45.1.209.gc6f12300df

Re: [PATCH] x86-32: fix cmpxchg8b_emu build error with clang

[Uros Bizjak]

On Wed, Jun 26, 2024 at 3:13 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> The kernel test robot reported that clang no longer compiles the 32-bit
> x86 kernel in some configurations due to commit 95ece48165c1
> ("locking/atomic/x86: Rewrite x86_32 arch_atomic64_{,fetch}_{and,or,xor}()
> functions").
>
> The build fails with
>
>   arch/x86/include/asm/cmpxchg_32.h:149:9: error: inline assembly requires more registers than available
>
> and the reason seems to be that not only does the cmpxchg8b instruction
> need four fixed registers (EDX:EAX and ECX:EBX), with the emulation
> fallback the inline asm also wants a fifth fixed register for the
> address (it uses %esi for that, but that's just a software convention
> with cmpxchg8b_emu).
>
> Avoiding using another pointer input to the asm (and just forcing it to
> use the "0(%esi)" addressing that we end up requiring for the sw
> fallback) seems to fix the issue.
>
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202406230912.F6XFIyA6-lkp@intel.com/
> Fixes: 95ece48165c1 ("locking/atomic/x86: Rewrite x86_32 arch_atomic64_{,fetch}_{and,or,xor}() functions")
> Link: https://lore.kernel.org/all/202406230912.F6XFIyA6-lkp@intel.com/
> Suggested-by: Uros Bizjak <ubizjak@gmail.com>
> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> ---
>
> Added commit message, and updated the asm to use '%a[ptr]' instead of
> writing out the addressing by hand.
>
> Still doing the 'oldp' writeback unconmditionally.  The code generation
> for the case I checked were the same for both clang and gcc, but until
> Uros hits me with the big clue-hammer, I think it's the simpler code
> that leaves room for potentially better optimizations too.

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.

>
> This falls solidly in the "looks ok to me, but still untested" category
> for me.  It fixes the clang build issue in my build testing, but I no
> longer have a 32-bit test environment, so no actual runtime testing.
>
>  arch/x86/include/asm/cmpxchg_32.h | 28 ++++++++++++----------------
>  1 file changed, 12 insertions(+), 16 deletions(-)
>
> diff --git a/arch/x86/include/asm/cmpxchg_32.h b/arch/x86/include/asm/cmpxchg_32.h
> index ed2797f132ce..4444a8292c7a 100644
> --- a/arch/x86/include/asm/cmpxchg_32.h
> +++ b/arch/x86/include/asm/cmpxchg_32.h
> @@ -88,18 +88,17 @@ static __always_inline bool __try_cmpxchg64_local(volatile u64 *ptr, u64 *oldp,
>
>  #define __arch_cmpxchg64_emu(_ptr, _old, _new, _lock_loc, _lock)       \
>  ({                                                                     \
> -       union __u64_halves o = { .full = (_old), },                     \
> -                          n = { .full = (_new), };                     \
> +       __u64 o = (_old);                                               \
> +       union __u64_halves n = { .full = (_new), };                     \
>                                                                         \
>         asm volatile(ALTERNATIVE(_lock_loc                              \
>                                  "call cmpxchg8b_emu",                  \
> -                                _lock "cmpxchg8b %[ptr]", X86_FEATURE_CX8) \
> -                    : [ptr] "+m" (*(_ptr)),                            \
> -                      "+a" (o.low), "+d" (o.high)                      \
> -                    : "b" (n.low), "c" (n.high), "S" (_ptr)            \
> +                                _lock "cmpxchg8b %a[ptr]", X86_FEATURE_CX8) \
> +                    : "+A" (o)                                         \
> +                    : "b" (n.low), "c" (n.high), [ptr] "S" (_ptr)      \
>                      : "memory");                                       \
>                                                                         \
> -       o.full;                                                         \
> +       o;                                                              \
>  })
>
>  static __always_inline u64 arch_cmpxchg64(volatile u64 *ptr, u64 old, u64 new)
> @@ -116,22 +115,19 @@ static __always_inline u64 arch_cmpxchg64_local(volatile u64 *ptr, u64 old, u64
>
>  #define __arch_try_cmpxchg64_emu(_ptr, _oldp, _new, _lock_loc, _lock)  \
>  ({                                                                     \
> -       union __u64_halves o = { .full = *(_oldp), },                   \
> -                          n = { .full = (_new), };                     \
> +       __u64 o = *(_oldp);                                             \
> +       union __u64_halves n = { .full = (_new), };                     \
>         bool ret;                                                       \
>                                                                         \
>         asm volatile(ALTERNATIVE(_lock_loc                              \
>                                  "call cmpxchg8b_emu",                  \
> -                                _lock "cmpxchg8b %[ptr]", X86_FEATURE_CX8) \
> +                                _lock "cmpxchg8b %a[ptr]", X86_FEATURE_CX8) \
>                      CC_SET(e)                                          \
> -                    : CC_OUT(e) (ret),                                 \
> -                      [ptr] "+m" (*(_ptr)),                            \
> -                      "+a" (o.low), "+d" (o.high)                      \
> -                    : "b" (n.low), "c" (n.high), "S" (_ptr)            \
> +                    : CC_OUT(e) (ret), "+A" (o)                        \
> +                    : "b" (n.low), "c" (n.high), [ptr] "S" (_ptr)      \
>                      : "memory");                                       \
>                                                                         \
> -       if (unlikely(!ret))                                             \
> -               *(_oldp) = o.full;                                      \
> +       *(_oldp) = o;                                                   \
>                                                                         \
>         likely(ret);                                                    \
>  })
> --
> 2.45.1.209.gc6f12300df
>

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

[Uros Bizjak]

On Tue, Jun 25, 2024 at 8:31 PM Linus Torvalds
<torvalds@linux-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@gmail.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

Re: [PATCH] x86-32: fix cmpxchg8b_emu build error with clang

[Uros Bizjak]

On Wed, Jun 26, 2024 at 9:39 AM Uros Bizjak <ubizjak@gmail.com> wrote:
>
> On Wed, Jun 26, 2024 at 3:13 AM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > The kernel test robot reported that clang no longer compiles the 32-bit
> > x86 kernel in some configurations due to commit 95ece48165c1
> > ("locking/atomic/x86: Rewrite x86_32 arch_atomic64_{,fetch}_{and,or,xor}()
> > functions").
> >
> > The build fails with
> >
> >   arch/x86/include/asm/cmpxchg_32.h:149:9: error: inline assembly requires more registers than available
> >
> > and the reason seems to be that not only does the cmpxchg8b instruction
> > need four fixed registers (EDX:EAX and ECX:EBX), with the emulation
> > fallback the inline asm also wants a fifth fixed register for the
> > address (it uses %esi for that, but that's just a software convention
> > with cmpxchg8b_emu).
> >
> > Avoiding using another pointer input to the asm (and just forcing it to
> > use the "0(%esi)" addressing that we end up requiring for the sw

A nit: offset 0 is required only for %ebp, so the above should read "(%esi)".

> > fallback) seems to fix the issue.
> >
> > Reported-by: kernel test robot <lkp@intel.com>
> > Closes: https://lore.kernel.org/oe-kbuild-all/202406230912.F6XFIyA6-lkp@intel.com/
> > Fixes: 95ece48165c1 ("locking/atomic/x86: Rewrite x86_32 arch_atomic64_{,fetch}_{and,or,xor}() functions")
> > Link: https://lore.kernel.org/all/202406230912.F6XFIyA6-lkp@intel.com/
> > Suggested-by: Uros Bizjak <ubizjak@gmail.com>
> > Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> > ---
> >
> > Added commit message, and updated the asm to use '%a[ptr]' instead of
> > writing out the addressing by hand.
> >
> > Still doing the 'oldp' writeback unconmditionally.  The code generation
> > for the case I checked were the same for both clang and gcc, but until
> > Uros hits me with the big clue-hammer, I think it's the simpler code
> > that leaves room for potentially better optimizations too.
>
> 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.
>
> >
> > This falls solidly in the "looks ok to me, but still untested" category
> > for me.  It fixes the clang build issue in my build testing, but I no
> > longer have a 32-bit test environment, so no actual runtime testing.                                     \

...

> >                                                                         \
> > -       if (unlikely(!ret))                                             \
> > -               *(_oldp) = o.full;                                      \
> > +       *(_oldp) = o;                                                   \

With the above part changed to:

       if (unlikely(!ret))                                             \
-               *(_oldp) = o.full;                                      \
+               *(_oldp) = o;                                           \

Reviewed-and-Tested-by: Uros Bizjak <ubizjak@gmail.com>

Runtime tested with .config provided by test robot on qemu-i386 with
both clang and GCC compiler:

LKP: ttyS0: 229: Kernel tests: Boot OK!

Thanks,
Uros.

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

[Peter Zijlstra]

On Tue, Jun 25, 2024 at 11:31:05AM -0700, Linus Torvalds wrote:

> The thing is, when cmpxchg doesn't fail, then oldp should already be "old", no?

Correct.

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

IIRC the reason I added that conditional is because at the time the GCC
compiler I tried it on generated slightly better code like this.

ISTR it emitting some superfluous assignments with the unconditional
store variant. Typically what seemed to happen is that since the
cmpxchg() user would have a loop termination on ret == old, it was able
to recognise it only needed that assignment in the failure case. Without
the condition on it would also do that assignment in the success case.

But yeah, otherwise it doesn't matter.

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

[Uros Bizjak]

On Wed, Jun 26, 2024 at 11:28 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Tue, Jun 25, 2024 at 11:31:05AM -0700, Linus Torvalds wrote:
>
> > The thing is, when cmpxchg doesn't fail, then oldp should already be "old", no?
>
> Correct.
>
> > 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,
>
> IIRC the reason I added that conditional is because at the time the GCC
> compiler I tried it on generated slightly better code like this.

Please see the thread at [1].

[1] https://lore.kernel.org/lkml/CACT4Y+bG+a0w6j6v1AmBE7fqqMSPyPEm4QimCzCouicmHT8FqA@mail.gmail.com/

Uros.

>
> ISTR it emitting some superfluous assignments with the unconditional
> store variant. Typically what seemed to happen is that since the
> cmpxchg() user would have a loop termination on ret == old, it was able
> to recognise it only needed that assignment in the failure case. Without
> the condition on it would also do that assignment in the success case.
>
> But yeah, otherwise it doesn't matter.

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

[Peter Zijlstra]

On Wed, Jun 26, 2024 at 11:33:09AM +0200, Uros Bizjak wrote:

> Please see the thread at [1].
> 
> [1] https://lore.kernel.org/lkml/CACT4Y+bG+a0w6j6v1AmBE7fqqMSPyPEm4QimCzCouicmHT8FqA@mail.gmail.com/

Ha, handy these archives, much better than my memory indeed.

Re: [PATCH] x86-32: fix cmpxchg8b_emu build error with clang

[Linus Torvalds]

On Wed, 26 Jun 2024 at 00:39, Uros Bizjak <ubizjak@gmail.com> wrote:
>
> >
> > Still doing the 'oldp' writeback unconmditionally.  The code generation
> > for the case I checked were the same for both clang and gcc, but until
> > Uros hits me with the big clue-hammer, I think it's the simpler code
> > that leaves room for potentially better optimizations too.
>
> You probably want to look at 44fe84459faf1 ("locking/atomic: Fix
> atomic_try_cmpxchg() semantics") [1] and the long LKML discussion at
> [2].

Christ. That use should be invalid.

The only _atomic_ pointer is "_ptr", not "old". Anybody who gives
something that can change during the operation in "old" is basically
already doing random things.

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

I think the only thing broken is that quote, and the crazy expectation
that "old" can change.

But obviously, I had completely forgotten that whole discussion from
seven years ago.

I don't actually find a single use of that invalid code sequence where
somebody would pass a non-private pointer as "oldp". So I really think
that part of the whole discussion was bogus to begin with, and
presumably from some other code base.

IOW, I think that example of a "classical lock-free stack push" is just broken.

That said, I can't find a case where it would matter for code
generation (every use will always do a conditional branch based on the
result, so the conditional assignment is practically speaking always
"static" anyway by the time you do branch following.

So I'll just send out a minimal patch with *only* the %esi changes.

           Linus

[PATCH v2] x86-32: fix cmpxchg8b_emu build error with clang

[Linus Torvalds]

The kernel test robot reported that clang no longer compiles the 32-bit
x86 kernel in some configurations due to commit 95ece48165c1
("locking/atomic/x86: Rewrite x86_32 arch_atomic64_{,fetch}_{and,or,xor}()
functions").

The build fails with

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

and the reason seems to be that not only does the cmpxchg8b instruction
need four fixed registers (EDX:EAX and ECX:EBX), with the emulation
fallback the inline asm also wants a fifth fixed register for the
address (it uses %esi for that, but that's just a software convention
with cmpxchg8b_emu).

Avoiding using another pointer input to the asm (and just forcing it to
use the "0(%esi)" addressing that we end up requiring for the sw
fallback) seems to fix the issue.

Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202406230912.F6XFIyA6-lkp@intel.com/
Fixes: 95ece48165c1 ("locking/atomic/x86: Rewrite x86_32 arch_atomic64_{,fetch}_{and,or,xor}() functions")
Link: https://lore.kernel.org/all/202406230912.F6XFIyA6-lkp@intel.com/
Suggested-by: Uros Bizjak <ubizjak@gmail.com>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---

This is the minimal patch, with the other simplifications removed.

I still think that the case where somebody passes in a "oldp" that can
change during the cmpxchg is entirely broken, and is not actually valid. 

If it was valid, we'd have to use READ_ONCE() and WRITE_ONCE() in the
"oldp" updates anyway just to make verification tools happy. We don't.

But hey, that simplification doesn't matter for fixing this build issue,
so let's keep the change minimal. 

 arch/x86/include/asm/cmpxchg_32.h | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/arch/x86/include/asm/cmpxchg_32.h b/arch/x86/include/asm/cmpxchg_32.h
index ed2797f132ce..62cef2113ca7 100644
--- a/arch/x86/include/asm/cmpxchg_32.h
+++ b/arch/x86/include/asm/cmpxchg_32.h
@@ -93,10 +93,9 @@ static __always_inline bool __try_cmpxchg64_local(volatile u64 *ptr, u64 *oldp,
 									\
 	asm volatile(ALTERNATIVE(_lock_loc				\
 				 "call cmpxchg8b_emu",			\
-				 _lock "cmpxchg8b %[ptr]", X86_FEATURE_CX8) \
-		     : [ptr] "+m" (*(_ptr)),				\
-		       "+a" (o.low), "+d" (o.high)			\
-		     : "b" (n.low), "c" (n.high), "S" (_ptr)		\
+				 _lock "cmpxchg8b %a[ptr]", X86_FEATURE_CX8) \
+		     : "+a" (o.low), "+d" (o.high)			\
+		     : "b" (n.low), "c" (n.high), [ptr] "S" (_ptr)	\
 		     : "memory");					\
 									\
 	o.full;								\
@@ -122,12 +121,11 @@ static __always_inline u64 arch_cmpxchg64_local(volatile u64 *ptr, u64 old, u64
 									\
 	asm volatile(ALTERNATIVE(_lock_loc				\
 				 "call cmpxchg8b_emu",			\
-				 _lock "cmpxchg8b %[ptr]", X86_FEATURE_CX8) \
+				 _lock "cmpxchg8b %a[ptr]", X86_FEATURE_CX8) \
 		     CC_SET(e)						\
 		     : CC_OUT(e) (ret),					\
-		       [ptr] "+m" (*(_ptr)),				\
 		       "+a" (o.low), "+d" (o.high)			\
-		     : "b" (n.low), "c" (n.high), "S" (_ptr)		\
+		     : "b" (n.low), "c" (n.high), [ptr] "S" (_ptr)	\
 		     : "memory");					\
 									\
 	if (unlikely(!ret))						\
-- 
2.45.1.209.gc6f12300df

Re: [PATCH v2] x86-32: fix cmpxchg8b_emu build error with clang

[Uros Bizjak]

On Wed, Jun 26, 2024 at 5:14 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> The kernel test robot reported that clang no longer compiles the 32-bit
> x86 kernel in some configurations due to commit 95ece48165c1
> ("locking/atomic/x86: Rewrite x86_32 arch_atomic64_{,fetch}_{and,or,xor}()
> functions").
>
> The build fails with
>
>   arch/x86/include/asm/cmpxchg_32.h:149:9: error: inline assembly requires more registers than available
>
> and the reason seems to be that not only does the cmpxchg8b instruction
> need four fixed registers (EDX:EAX and ECX:EBX), with the emulation
> fallback the inline asm also wants a fifth fixed register for the
> address (it uses %esi for that, but that's just a software convention
> with cmpxchg8b_emu).
>
> Avoiding using another pointer input to the asm (and just forcing it to
> use the "0(%esi)" addressing that we end up requiring for the sw
> fallback) seems to fix the issue.
>
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202406230912.F6XFIyA6-lkp@intel.com/
> Fixes: 95ece48165c1 ("locking/atomic/x86: Rewrite x86_32 arch_atomic64_{,fetch}_{and,or,xor}() functions")
> Link: https://lore.kernel.org/all/202406230912.F6XFIyA6-lkp@intel.com/
> Suggested-by: Uros Bizjak <ubizjak@gmail.com>
> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

Reviewed-and-Tested-by: Uros Bizjak <ubizjak@gmail.com>

> ---
>
> This is the minimal patch, with the other simplifications removed.
>
> I still think that the case where somebody passes in a "oldp" that can
> change during the cmpxchg is entirely broken, and is not actually valid.
>
> If it was valid, we'd have to use READ_ONCE() and WRITE_ONCE() in the
> "oldp" updates anyway just to make verification tools happy. We don't.
>
> But hey, that simplification doesn't matter for fixing this build issue,
> so let's keep the change minimal.
>
>  arch/x86/include/asm/cmpxchg_32.h | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/arch/x86/include/asm/cmpxchg_32.h b/arch/x86/include/asm/cmpxchg_32.h
> index ed2797f132ce..62cef2113ca7 100644
> --- a/arch/x86/include/asm/cmpxchg_32.h
> +++ b/arch/x86/include/asm/cmpxchg_32.h
> @@ -93,10 +93,9 @@ static __always_inline bool __try_cmpxchg64_local(volatile u64 *ptr, u64 *oldp,
>                                                                         \
>         asm volatile(ALTERNATIVE(_lock_loc                              \
>                                  "call cmpxchg8b_emu",                  \
> -                                _lock "cmpxchg8b %[ptr]", X86_FEATURE_CX8) \
> -                    : [ptr] "+m" (*(_ptr)),                            \
> -                      "+a" (o.low), "+d" (o.high)                      \
> -                    : "b" (n.low), "c" (n.high), "S" (_ptr)            \
> +                                _lock "cmpxchg8b %a[ptr]", X86_FEATURE_CX8) \
> +                    : "+a" (o.low), "+d" (o.high)                      \
> +                    : "b" (n.low), "c" (n.high), [ptr] "S" (_ptr)      \
>                      : "memory");                                       \
>                                                                         \
>         o.full;                                                         \
> @@ -122,12 +121,11 @@ static __always_inline u64 arch_cmpxchg64_local(volatile u64 *ptr, u64 old, u64
>                                                                         \
>         asm volatile(ALTERNATIVE(_lock_loc                              \
>                                  "call cmpxchg8b_emu",                  \
> -                                _lock "cmpxchg8b %[ptr]", X86_FEATURE_CX8) \
> +                                _lock "cmpxchg8b %a[ptr]", X86_FEATURE_CX8) \
>                      CC_SET(e)                                          \
>                      : CC_OUT(e) (ret),                                 \
> -                      [ptr] "+m" (*(_ptr)),                            \
>                        "+a" (o.low), "+d" (o.high)                      \
> -                    : "b" (n.low), "c" (n.high), "S" (_ptr)            \
> +                    : "b" (n.low), "c" (n.high), [ptr] "S" (_ptr)      \
>                      : "memory");                                       \
>                                                                         \
>         if (unlikely(!ret))                                             \
> --
> 2.45.1.209.gc6f12300df
>

Re: [PATCH v2] x86-32: fix cmpxchg8b_emu build error with clang

[Linus Torvalds]

On Wed, 26 Jun 2024 at 12:00, Uros Bizjak <ubizjak@gmail.com> wrote:
>
> Reviewed-and-Tested-by: Uros Bizjak <ubizjak@gmail.com>

I ended up committing this directly for rc6 as commit 769327258a14
("x86-32: fix cmpxchg8b_emu build error with clang") since nobody else
seemed to react much and it looked obvious enough.

              Linus