[libc] refactor futex and remove dependency cycle #117923
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Member
|
@llvm/pr-subscribers-libc Author: Schrodinger ZHU Yifan (SchrodingerZhu) ChangesOn Linux, we have the following dependency chain: This was previously solved by separating out the implementation of We should address the problem in a saner approach. So, the real problem is that This PR separates the timeout support from the main futex implementation. Additionally, it lifts futex to the upper-level, making it a general platform abstraction rather a linux-specific abstraction. Windows support for futex will be added very soon. Full diff: https://github.com/llvm/llvm-project/pull/117923.diff 11 Files Affected:
diff --git a/libc/src/__support/threads/CMakeLists.txt b/libc/src/__support/threads/CMakeLists.txt
index bd49bbb5ad2fe7..5a39c9dff9216c 100644
--- a/libc/src/__support/threads/CMakeLists.txt
+++ b/libc/src/__support/threads/CMakeLists.txt
@@ -105,3 +105,20 @@ add_header_library(
libc.hdr.types.pid_t
${identifier_dependency_on_thread}
)
+
+# TODO: implement windows "futex" with WaitOnAddress
+add_header_library(
+ futex_utils
+ HDRS
+ futex_utils.h
+ DEPENDS
+ .${LIBC_TARGET_OS}.futex_utils
+)
+
+add_header_library(
+ futex_timeout
+ HDRS
+ futex_timeout.h
+ DEPENDS
+ .${LIBC_TARGET_OS}.futex_timeout
+)
diff --git a/libc/src/__support/threads/futex_timeout.h b/libc/src/__support/threads/futex_timeout.h
new file mode 100644
index 00000000000000..7adfbc594bfca6
--- /dev/null
+++ b/libc/src/__support/threads/futex_timeout.h
@@ -0,0 +1,18 @@
+//===--- Futex With Timeout Support -----------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+#ifndef LLVM_LIBC_SRC_SUPPORT_THREADS_FUTEX_TIMEOUT_H
+#define LLVM_LIBC_SRC_SUPPORT_THREADS_FUTEX_TIMEOUT_H
+
+// TODO: implement futex for other platforms.
+#ifdef __linux__
+#include "src/__support/threads/linux/futex_timeout.h"
+#else
+#error "Unsupported platform"
+#endif
+
+#endif // LLVM_LIBC_SRC_SUPPORT_THREADS_FUTEX_TIMEOUT_H
diff --git a/libc/src/__support/threads/futex_utils.h b/libc/src/__support/threads/futex_utils.h
new file mode 100644
index 00000000000000..61ccabeadc85b9
--- /dev/null
+++ b/libc/src/__support/threads/futex_utils.h
@@ -0,0 +1,28 @@
+//===--- Futex Utilities ----------------------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+//===--- Futex Wrapper ------------------------------------------*- C++ -*-===//
+// We take the name "futex" from Linux system. This library provides a general
+// wrapper for waiting and notifying on atomic words. Various platforms support
+// futex-like operations.
+// - Windows: WaitOnAddress and WakeByAddressSingle/WakeByAddressAll
+// (Windows futex cannot be used in inter-process synchronization)
+// - MacOS: os_sync_wait_on_address or __ulock_wait/__ulock_wake
+// - FreeBSD: _umtx_op
+
+#ifndef LLVM_LIBC_SRC_SUPPORT_THREADS_FUTEX_UTILS_H
+#define LLVM_LIBC_SRC_SUPPORT_THREADS_FUTEX_UTILS_H
+
+// TODO: implement futex for other platforms.
+#ifdef __linux__
+#include "src/__support/threads/linux/futex_utils.h"
+#else
+#error "Unsupported platform"
+#endif
+
+#endif // LLVM_LIBC_SRC_SUPPORT_THREADS_FUTEX_UTILS_H
diff --git a/libc/src/__support/threads/linux/CMakeLists.txt b/libc/src/__support/threads/linux/CMakeLists.txt
index 47598d98c98863..19ce75ce9cd6b7 100644
--- a/libc/src/__support/threads/linux/CMakeLists.txt
+++ b/libc/src/__support/threads/linux/CMakeLists.txt
@@ -19,6 +19,14 @@ add_header_library(
libc.src.__support.CPP.atomic
libc.src.__support.CPP.limits
libc.src.__support.CPP.optional
+)
+
+add_header_library(
+ futex_timeout
+ HDRS
+ futex_timeout.h
+ DEPENDS
+ .futex_utils
libc.src.__support.time.linux.abs_timeout
)
@@ -34,7 +42,7 @@ add_header_library(
HDRS
mutex.h
DEPENDS
- .futex_utils
+ .futex_timeout
libc.src.__support.threads.sleep
libc.src.__support.time.linux.abs_timeout
libc.src.__support.time.linux.monotonicity
@@ -50,7 +58,7 @@ add_header_library(
HDRS
rwlock.h
DEPENDS
- .futex_utils
+ .futex_timeout
.raw_mutex
libc.src.__support.common
libc.src.__support.OSUtil.osutil
diff --git a/libc/src/__support/threads/linux/CndVar.cpp b/libc/src/__support/threads/linux/CndVar.cpp
index be74c18dddf31a..2f7297b7433959 100644
--- a/libc/src/__support/threads/linux/CndVar.cpp
+++ b/libc/src/__support/threads/linux/CndVar.cpp
@@ -8,7 +8,7 @@
#include "src/__support/threads/CndVar.h"
#include "src/__support/CPP/mutex.h"
-#include "src/__support/OSUtil/syscall.h" // syscall_impl
+#include "src/__support/OSUtil/syscall.h" // syscall_impl
#include "src/__support/macros/config.h"
#include "src/__support/threads/linux/futex_word.h" // FutexWordType
#include "src/__support/threads/linux/raw_mutex.h" // RawMutex
@@ -56,7 +56,7 @@ int CndVar::wait(Mutex *m) {
}
}
- waiter.futex_word.wait(WS_Waiting, cpp::nullopt, true);
+ waiter.futex_word.wait(WS_Waiting, /*is_shared=*/true);
// At this point, if locking |m| fails, we can simply return as the
// queued up waiter would have been removed from the queue.
diff --git a/libc/src/__support/threads/linux/futex_timeout.h b/libc/src/__support/threads/linux/futex_timeout.h
new file mode 100644
index 00000000000000..ba12cab7d80660
--- /dev/null
+++ b/libc/src/__support/threads/linux/futex_timeout.h
@@ -0,0 +1,62 @@
+//===--- Futex Wrapper ------------------------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_LIBC_SRC___SUPPORT_THREADS_LINUX_FUTEX_TIMEOUT_H
+#define LLVM_LIBC_SRC___SUPPORT_THREADS_LINUX_FUTEX_TIMEOUT_H
+
+#include "src/__support/CPP/atomic.h"
+#include "src/__support/CPP/limits.h"
+#include "src/__support/CPP/optional.h"
+#include "src/__support/OSUtil/syscall.h"
+#include "src/__support/macros/attributes.h"
+#include "src/__support/macros/config.h"
+#include "src/__support/threads/linux/futex_utils.h"
+#include "src/__support/time/linux/abs_timeout.h"
+
+namespace LIBC_NAMESPACE_DECL {
+class TimedFutex : public Futex {
+public:
+ using Timeout = internal::AbsTimeout;
+ using Futex::Futex;
+ using Futex::operator=;
+ LIBC_INLINE long wait(FutexWordType expected,
+ cpp::optional<Timeout> timeout = cpp::nullopt,
+ bool is_shared = false) {
+ // use bitset variants to enforce abs_time
+ uint32_t op = is_shared ? FUTEX_WAIT_BITSET : FUTEX_WAIT_BITSET_PRIVATE;
+ if (timeout && timeout->is_realtime()) {
+ op |= FUTEX_CLOCK_REALTIME;
+ }
+ for (;;) {
+ if (this->load(cpp::MemoryOrder::RELAXED) != expected)
+ return 0;
+
+ long ret = syscall_impl<long>(
+ /* syscall number */ FUTEX_SYSCALL_ID,
+ /* futex address */ this,
+ /* futex operation */ op,
+ /* expected value */ expected,
+ /* timeout */ timeout ? &timeout->get_timespec() : nullptr,
+ /* ignored */ nullptr,
+ /* bitset */ FUTEX_BITSET_MATCH_ANY);
+
+ // continue waiting if interrupted; otherwise return the result
+ // which should normally be 0 or -ETIMEOUT
+ if (ret == -EINTR)
+ continue;
+
+ return ret;
+ }
+ }
+};
+
+static_assert(__is_standard_layout(TimedFutex),
+ "Futex must be a standard layout type.");
+} // namespace LIBC_NAMESPACE_DECL
+
+#endif // LLVM_LIBC_SRC___SUPPORT_THREADS_LINUX_FUTEX_TIMEOUT_H
diff --git a/libc/src/__support/threads/linux/futex_utils.h b/libc/src/__support/threads/linux/futex_utils.h
index 943a99ab38c8ca..0d36eb92232faa 100644
--- a/libc/src/__support/threads/linux/futex_utils.h
+++ b/libc/src/__support/threads/linux/futex_utils.h
@@ -16,28 +16,21 @@
#include "src/__support/macros/attributes.h"
#include "src/__support/macros/config.h"
#include "src/__support/threads/linux/futex_word.h"
-#include "src/__support/time/linux/abs_timeout.h"
#include <linux/errno.h>
#include <linux/futex.h>
namespace LIBC_NAMESPACE_DECL {
class Futex : public cpp::Atomic<FutexWordType> {
public:
- using Timeout = internal::AbsTimeout;
LIBC_INLINE constexpr Futex(FutexWordType value)
: cpp::Atomic<FutexWordType>(value) {}
LIBC_INLINE Futex &operator=(FutexWordType value) {
cpp::Atomic<FutexWordType>::store(value);
return *this;
}
- LIBC_INLINE long wait(FutexWordType expected,
- cpp::optional<Timeout> timeout = cpp::nullopt,
- bool is_shared = false) {
+ LIBC_INLINE long wait(FutexWordType expected, bool is_shared = false) {
// use bitset variants to enforce abs_time
uint32_t op = is_shared ? FUTEX_WAIT_BITSET : FUTEX_WAIT_BITSET_PRIVATE;
- if (timeout && timeout->is_realtime()) {
- op |= FUTEX_CLOCK_REALTIME;
- }
for (;;) {
if (this->load(cpp::MemoryOrder::RELAXED) != expected)
return 0;
@@ -47,7 +40,7 @@ class Futex : public cpp::Atomic<FutexWordType> {
/* futex address */ this,
/* futex operation */ op,
/* expected value */ expected,
- /* timeout */ timeout ? &timeout->get_timespec() : nullptr,
+ /* timeout */ nullptr,
/* ignored */ nullptr,
/* bitset */ FUTEX_BITSET_MATCH_ANY);
diff --git a/libc/src/__support/threads/linux/raw_mutex.h b/libc/src/__support/threads/linux/raw_mutex.h
index 47f0aa70f1c46f..717238b2c263ec 100644
--- a/libc/src/__support/threads/linux/raw_mutex.h
+++ b/libc/src/__support/threads/linux/raw_mutex.h
@@ -14,7 +14,7 @@
#include "src/__support/macros/attributes.h"
#include "src/__support/macros/config.h"
#include "src/__support/macros/optimization.h"
-#include "src/__support/threads/linux/futex_utils.h"
+#include "src/__support/threads/linux/futex_timeout.h"
#include "src/__support/threads/linux/futex_word.h"
#include "src/__support/threads/sleep.h"
#include "src/__support/time/linux/abs_timeout.h"
@@ -38,7 +38,7 @@ namespace LIBC_NAMESPACE_DECL {
// critical sections.
class RawMutex {
protected:
- Futex futex;
+ TimedFutex futex;
LIBC_INLINE_VAR static constexpr FutexWordType UNLOCKED = 0b00;
LIBC_INLINE_VAR static constexpr FutexWordType LOCKED = 0b01;
LIBC_INLINE_VAR static constexpr FutexWordType IN_CONTENTION = 0b10;
@@ -63,7 +63,7 @@ class RawMutex {
// Return true if the lock is acquired. Return false if timeout happens before
// the lock is acquired.
- LIBC_INLINE bool lock_slow(cpp::optional<Futex::Timeout> timeout,
+ LIBC_INLINE bool lock_slow(cpp::optional<TimedFutex::Timeout> timeout,
bool is_pshared, unsigned spin_count) {
FutexWordType state = spin(spin_count);
// Before go into contention state, try to grab the lock.
@@ -102,7 +102,7 @@ class RawMutex {
expected, LOCKED, cpp::MemoryOrder::ACQUIRE, cpp::MemoryOrder::RELAXED);
}
LIBC_INLINE bool
- lock(cpp::optional<Futex::Timeout> timeout = cpp::nullopt,
+ lock(cpp::optional<TimedFutex::Timeout> timeout = cpp::nullopt,
bool is_shared = false,
unsigned spin_count = LIBC_COPT_RAW_MUTEX_DEFAULT_SPIN_COUNT) {
// Timeout will not be checked if immediate lock is possible.
diff --git a/libc/src/__support/threads/linux/rwlock.h b/libc/src/__support/threads/linux/rwlock.h
index 57fcc7bb67a6a0..6d3e2f2ff85eaa 100644
--- a/libc/src/__support/threads/linux/rwlock.h
+++ b/libc/src/__support/threads/linux/rwlock.h
@@ -55,9 +55,9 @@ class WaitingQueue final : private RawMutex {
// Pending writer count (protected by the mutex)
FutexWordType pending_writers;
// Reader serialization (increases on each reader-waking operation)
- Futex reader_serialization;
+ TimedFutex reader_serialization;
// Writer serialization (increases on each writer-waking operation)
- Futex writer_serialization;
+ TimedFutex writer_serialization;
public:
// RAII guard to lock and unlock the waiting queue.
@@ -98,7 +98,7 @@ class WaitingQueue final : private RawMutex {
template <Role role>
LIBC_INLINE long wait(FutexWordType expected,
- cpp::optional<Futex::Timeout> timeout,
+ cpp::optional<TimedFutex::Timeout> timeout,
bool is_pshared) {
if constexpr (role == Role::Reader)
return reader_serialization.wait(expected, timeout, is_pshared);
@@ -389,7 +389,7 @@ class RwLock {
private:
template <Role role>
LIBC_INLINE LockResult
- lock_slow(cpp::optional<Futex::Timeout> timeout = cpp::nullopt,
+ lock_slow(cpp::optional<TimedFutex::Timeout> timeout = cpp::nullopt,
unsigned spin_count = LIBC_COPT_RWLOCK_DEFAULT_SPIN_COUNT) {
// Phase 1: deadlock detection.
// A deadlock happens if this is a RAW/WAW lock in the same thread.
@@ -468,7 +468,7 @@ class RwLock {
public:
[[nodiscard]]
LIBC_INLINE LockResult
- read_lock(cpp::optional<Futex::Timeout> timeout = cpp::nullopt,
+ read_lock(cpp::optional<TimedFutex::Timeout> timeout = cpp::nullopt,
unsigned spin_count = LIBC_COPT_RWLOCK_DEFAULT_SPIN_COUNT) {
LockResult result = try_read_lock();
if (LIBC_LIKELY(result != LockResult::Busy))
@@ -477,7 +477,7 @@ class RwLock {
}
[[nodiscard]]
LIBC_INLINE LockResult
- write_lock(cpp::optional<Futex::Timeout> timeout = cpp::nullopt,
+ write_lock(cpp::optional<TimedFutex::Timeout> timeout = cpp::nullopt,
unsigned spin_count = LIBC_COPT_RWLOCK_DEFAULT_SPIN_COUNT) {
LockResult result = try_write_lock();
if (LIBC_LIKELY(result != LockResult::Busy))
diff --git a/libc/src/__support/threads/linux/thread.cpp b/libc/src/__support/threads/linux/thread.cpp
index c531d74c533550..d55dc453f8279e 100644
--- a/libc/src/__support/threads/linux/thread.cpp
+++ b/libc/src/__support/threads/linux/thread.cpp
@@ -377,7 +377,7 @@ void Thread::wait() {
// We cannot do a FUTEX_WAIT_PRIVATE here as the kernel does a
// FUTEX_WAKE and not a FUTEX_WAKE_PRIVATE.
while (clear_tid->load() != 0)
- clear_tid->wait(CLEAR_TID_VALUE, cpp::nullopt, true);
+ clear_tid->wait(CLEAR_TID_VALUE, /*is_shared=*/true);
}
bool Thread::operator==(const Thread &thread) const {
diff --git a/libc/src/threads/linux/CMakeLists.txt b/libc/src/threads/linux/CMakeLists.txt
index 6c8e0845faf4c4..cf50e0ac560127 100644
--- a/libc/src/threads/linux/CMakeLists.txt
+++ b/libc/src/threads/linux/CMakeLists.txt
@@ -10,7 +10,7 @@ add_header_library(
libc.src.__support.OSUtil.osutil
libc.src.__support.threads.mutex
libc.src.__support.threads.linux.raw_mutex
- libc.src.__support.threads.linux.futex_utils
+ libc.src.__support.threads.futex_utils
)
add_entrypoint_object(
|
| using Futex::Futex; | ||
| using Futex::operator=; | ||
| LIBC_INLINE long wait(FutexWordType expected, | ||
| cpp::optional<Timeout> timeout = cpp::nullopt, |
Member
There was a problem hiding this comment.
So if we make the distinction between Futex now not having a timeout, and TimedFutex having a timeout, then I think the timeout parameter of the TimedFutex::wait method should not be optional.
Comment on lines
+32
to
+34
| if (timeout && timeout->is_realtime()) { | ||
| op |= FUTEX_CLOCK_REALTIME; | ||
| } |
Member
There was a problem hiding this comment.
Suggested change
| if (timeout && timeout->is_realtime()) { | |
| op |= FUTEX_CLOCK_REALTIME; | |
| } | |
| if (timeout && timeout->is_realtime()) | |
| op |= FUTEX_CLOCK_REALTIME; |
| public: | ||
| using Timeout = internal::AbsTimeout; | ||
| using Futex::Futex; | ||
| using Futex::operator=; |
Member
There was a problem hiding this comment.
Why do we need using statement for the assignment operator?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
On Linux, we have the following dependency chain:
This was previously solved by separating out the implementation of
abs_timeoutrelated implementation out of the its definition header, which is quite ugly. Recently, I started working on Windows implementations and such hidden cyclic dependency bring back some headache when I was refactoring the timeout to support both absolution timeout and relative timeout.We should address the problem in a saner approach.
So, the real problem is that
calloncedoes not need timeout at all and we should not pull in the timing APIs viacalloncewhich is used everywhere in low-level initialization operations. That is to make everything depending on timing APIs.This PR separates the timeout support from the main futex implementation. Additionally, it lifts futex to the upper-level, making it a general platform abstraction rather a linux-specific abstraction.
Windows support for futex will be added very soon.