core/rmutex: use atomic utils

[maribu]

Contribution description

Replace use of C11 atomics with atomic utils. This fixes

error: address argument to atomic operation must be a pointer to a
       trivially-copyable type ('_Atomic(int) *' invalid)

error when compiling on AVR with LLVM.

Testing procedure

tests/rmutex should still pass

Issues/PRs references

None

[maribu]

On my Nucleo-F767ZI:

$ make BOARD=nucleo-f767zi -C tests/rmutex flash test

[...]

Help: Press s to start test, r to print it is ready
READY
s
START
main(): This is RIOT! (Version: 2021.10-devel-790-g7313a-core/rmutex)
Recursive Mutex test
Please refer to the README.md for more information

T2 (prio 6, depth 0): trying to lock rmutex now
T3 (prio 4, depth 0): trying to lock rmutex now
T4 (prio 5, depth 0): trying to lock rmutex now
T5 (prio 2, depth 0): trying to lock rmutex now
T6 (prio 4, depth 0): trying to lock rmutex now
main: unlocking recursive mutex
T5 (prio 2, depth 0): locked rmutex now
T5 (prio 2, depth 1): trying to lock rmutex now
T5 (prio 2, depth 1): locked rmutex now
T5 (prio 2, depth 2): trying to lock rmutex now
T5 (prio 2, depth 2): locked rmutex now
T5 (prio 2, depth 3): trying to lock rmutex now
T5 (prio 2, depth 3): locked rmutex now
T5 (prio 2, depth 3): unlocked rmutex
T5 (prio 2, depth 2): unlocked rmutex
T5 (prio 2, depth 1): unlocked rmutex
T5 (prio 2, depth 0): unlocked rmutex
T3 (prio 4, depth 0): locked rmutex now
T3 (prio 4, depth 1): trying to lock rmutex now
T3 (prio 4, depth 1): locked rmutex now
T3 (prio 4, depth 2): trying to lock rmutex now
T3 (prio 4, depth 2): locked rmutex now
T3 (prio 4, depth 2): unlocked rmutex
T3 (prio 4, depth 1): unlocked rmutex
T3 (prio 4, depth 0): unlocked rmutex
T6 (prio 4, depth 0): locked rmutex now
T6 (prio 4, depth 1): trying to lock rmutex now
T6 (prio 4, depth 1): locked rmutex now
T6 (prio 4, depth 2): trying to lock rmutex now
T6 (prio 4, depth 2): locked rmutex now
T6 (prio 4, depth 3): trying to lock rmutex now
T6 (prio 4, depth 3): locked rmutex now
T6 (prio 4, depth 4): trying to lock rmutex now
T6 (prio 4, depth 4): locked rmutex now
T6 (prio 4, depth 4): unlocked rmutex
T6 (prio 4, depth 3): unlocked rmutex
T6 (prio 4, depth 2): unlocked rmutex
T6 (prio 4, depth 1): unlocked rmutex
T6 (prio 4, depth 0): unlocked rmutex
T4 (prio 5, depth 0): locked rmutex now
T4 (prio 5, depth 1): trying to lock rmutex now
T4 (prio 5, depth 1): locked rmutex now
T4 (prio 5, depth 2): trying to lock rmutex now
T4 (prio 5, depth 2): locked rmutex now
T4 (prio 5, depth 2): unlocked rmutex
T4 (prio 5, depth 1): unlocked rmutex
T4 (prio 5, depth 0): unlocked rmutex
T2 (prio 6, depth 0): locked rmutex now
T2 (prio 6, depth 1): trying to lock rmutex now
T2 (prio 6, depth 1): locked rmutex now
T2 (prio 6, depth 2): trying to lock rmutex now
T2 (prio 6, depth 2): locked rmutex now
T2 (prio 6, depth 3): trying to lock rmutex now
T2 (prio 6, depth 3): locked rmutex now
T2 (prio 6, depth 4): trying to lock rmutex now
T2 (prio 6, depth 4): locked rmutex now

$ echo $?
0

[maribu]

And also:

$ make BOARD=samr21-xpro BUILD_IN_DOCKER=1 -C tests/rmutex

master:

   text	   data	    bss	    dec	    hex	filename
  10748	    132	  10056	  20936	   51c8	/data/riotbuild/riotbase/tests/rmutex/bin/samr21-xpro/tests_rmutex.elf

This PR:

   text	   data	    bss	    dec	    hex	filename
  10792	    132	  10056	  20980	   51f4	/data/riotbuild/riotbase/tests/rmutex/bin/samr21-xpro/tests_rmutex.elf

So this increases ROM consumption. The reason is that previously relaxed memory order was used, atomic utils always use sequentially consistent memory accesses. I think this is actually fixing a race condition in the code, as otherwise the compiler could load the owner of the rmutex before checking if it is already locked.

[chrysn]

While I'm waiting for the tools I'll need to run the test: Do you know if that test ever passed? Were mutexes ever usable on AVR?

On the more general topic: Are we moving off c11 atomics towards using the helper everywhere? (It'd make my life a lot easier, given that atomics are a pain to transpile right now, but that's off topic here).

How does seqcst vs. relaxed impact the code size by 40 bytes? (Played around with godbolt; "relaxed" is 0 and "seq_cst", is it just that in may code locations?)

[kaspar030]

On the more general topic: Are we moving off c11 atomics towards using the helper everywhere? (It'd make my life a lot easier, given that atomics are a pain to transpile right now, but that's off topic here).

Yeah. Also, is that a good idea, are C11 atomics that broken? Just to not open another NIH issue here. Are there pointers to reasoning?

[kaspar030]

Yeah. Also, is that a good idea, are C11 atomics that broken? Just to not open another NIH issue here. Are there pointers to reasoning?

Don't get me wrong, I'm for this. I think the reasons in #14331 are sufficient. We should write something somewhere stating that in-tree, atomic_utils should be preferred and why.

[maribu]

The cross language issues with C11 atomics are not unique to rust, btw. C++ also has its own atomics, which are inherently incompatible. The c11_atomics_compat.hpp could be dropped if we would move away from C11 atomics in-tree.

But I guess I should try to improve the ROM costs of the atomic utils first. There is one issue with GCC not optimizing unused variables across memory barriers - which LLVM just does if the variable could be (at least in theory) be allocated in registers, which LLVM reasons is not memory and thus not affected by memory barriers. I won't get past this issue, but GCC's optimizer is generally freaking out when it sees memory barriers or volatile and stops optimizing things that are not affected. (E.g. I often saw that pointer arithmetic done for every access, despite the memory the pointer points to was marked as volatile, not the pointer itself.) Making atomic utils consistently performing equally good or better than C11 atomics in every aspect will make them easy to sell.

[chrysn]

Are you sure there is much to optimize? I see the same 24 bytes difference, 8 in rmutex_unlock and 16 in _lock, that's not nothing but not the end of the world either, especially if it does indeed fix a race condition as you mention. I don't see the race condition clearly yet, but comparing the the old and the new disassembled code (where dmb ish instructions at 4 bytes each are inserted around the store of the owner), it looks like that's exactly the difference. (And it's probably two because the consensus around atomic utils was that seq-cst is what's best understood by everyone, so no halving this unless atomic-utils change.)

By the way, I still can't build the test (even with #16923 cherry-picked, BOARD=arduino-mega2560 make -C tests/rmutex all BUILD_IN_DOCKER=1 TOOLCHAIN=llvm CONTINUE_ON_EXPECTED_ERRORS=1 fails for me somewhere in the __asm__ code of cpu.h), is there a known good board this works on?

[maribu]

is there a known good board this works on?

No, I PR'ed everything from the effort in #16924 independently that was ready for review already. But you could revert this PR in #16924 and see that compilation starts to fail.

[benpicco]

Code looks good - anything that speaks against this?

[maribu]

A slight increase in code size, that some micro-optimizations in the atomic utils could improve again. But my work on that has stalled, sadly. Too much areas to work on, too little time ...

[benpicco]

Looks like there are some problems on ztimer64 too

[maribu]

Ah, yes. It has a separate implementation of ztimer{,64}_rmutex_unlock() that also needs to be adapted. I'll rebase and do so once the kids are in bed.

[maribu]

A single unrelated test failed. I'm restarting the CI without tests again.