Page MenuHomeFreeBSD

libcxx: Implement atomic::wait/wake using _umtx_op(2) ) for 64bit arches
ClosedPublic

Authored by kib on Jan 20 2023, 3:00 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, May 5, 10:50 AM
Unknown Object (File)
Thu, May 2, 4:09 PM
Unknown Object (File)
Apr 2 2024, 3:58 PM
Unknown Object (File)
Mar 30 2024, 4:38 AM
Unknown Object (File)
Jan 30 2024, 9:25 PM
Unknown Object (File)
Jan 30 2024, 9:25 PM
Unknown Object (File)
Jan 30 2024, 9:25 PM
Unknown Object (File)
Jan 30 2024, 9:25 PM
Subscribers

Details

Summary
Only 64bit architectures can be supported this way, because libcxx
defines __cxx_contention_t to be int64_t for FreeBSD, and 32bit arches
do not have a kind of UMTX_OP_WAIT_INT64_PRIVATE operation.

LLVM review: https://reviews.llvm.org/D142134

Tested by: emaste

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

kib requested review of this revision.Jan 20 2023, 3:00 AM
markj added inline comments.
contrib/llvm-project/libcxx/src/atomic.cpp
86

It looks like the __cxx_atomic_contention_t value is a int32_t on mips, and an int64_t otherwise. Is mips the reason for using the UINT variant?

This revision is now accepted and ready to land.Jan 20 2023, 2:08 PM

I'm fine with this, but we should upstream it. I'll have a look if upstream already has a regression test for the atomic stuff.

contrib/llvm-project/libcxx/src/atomic.cpp
86

Note that was committed from D26774 in rSd061adc48d54ebb91b1709b16c59e8ceca895be2, as a workaround for some older gcc. Maybe it could be dropped at some point, as this change is also not upstream.

Upstream review is https://reviews.llvm.org/D142134

There are upstream regression tests, see my comments in that review and in D128084. Some of the atomic tests were inconsistent (timing out, etc.) before in my CI work, but they've passed in all of my attempts with this change included.

kib marked 2 inline comments as done.Jan 20 2023, 9:36 PM
kib added inline comments.
contrib/llvm-project/libcxx/src/atomic.cpp
86

No, I simply missed the fact that contention object is always 64bit. In other words, this thing cannot work on 32bit arches, without either ABI change for libcxx, or adding a new umtx op. The later seems to be hard because some 32bit arches (ppc) do not have neccessary atomics, not to hope for their copyin version.

It seems that the mips change was not up-streamed.

kib marked an inline comment as done.
kib retitled this revision from libcxx: Implement atomic::wait/wake using _umtx_op(2) to libcxx: Implement atomic::wait/wake using _umtx_op(2) ) for 64bit arches.
kib edited the summary of this revision. (Show Details)
This revision now requires review to proceed.Jan 20 2023, 9:40 PM
This revision is now accepted and ready to land.Jan 21 2023, 2:31 AM

Could 32-bit arches just cast to an int32 pointer and ignore the remaining bits of the contention object?

Could 32-bit arches just cast to an int32 pointer and ignore the remaining bits of the contention object?

Never mind it looks like that would require changes to more functions.

Could 32-bit arches just cast to an int32 pointer and ignore the remaining bits of the contention object?

Never mind it looks like that would require changes to more functions

It is not a problem to cast to int32. Issue is that UMTX_OP_WAIT (or any of its variants) only compares 32bit of the destination with the 32bits of the passed value, causing unexpected wait if other 32bit of the value do not match. This would result in the hang of the waiting thread.