[libc][__support] move CndVar to __support #89329
Merged
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
|
I still need to clean up the cmake dependencies properly, so marking this as a draft until then. |
|
@llvm/pr-subscribers-libc Author: Nick Desaulniers (nickdesaulniers) ChangesWe should be able to reuse this between the implementation of C11 cnd_t The current implementation is hyper linux specific, making use of Futex. That Fixes: #88580 Full diff: https://github.com/llvm/llvm-project/pull/89329.diff 11 Files Affected:
diff --git a/libc/src/__support/threads/CMakeLists.txt b/libc/src/__support/threads/CMakeLists.txt
index 731adf6f9c8e4e..5cff5029202846 100644
--- a/libc/src/__support/threads/CMakeLists.txt
+++ b/libc/src/__support/threads/CMakeLists.txt
@@ -69,3 +69,12 @@ if(TARGET libc.src.__support.threads.${LIBC_TARGET_OS}.callonce)
.${LIBC_TARGET_OS}.callonce
)
endif()
+
+if(TARGET libc.src.__support.threads.${LIBC_TARGET_OS}.CndVar)
+ add_object_library(
+ CndVar
+ ALIAS
+ DEPENDS
+ .${LIBC_TARGET_OS}.CndVar
+ )
+endif()
diff --git a/libc/src/__support/threads/CndVar.h b/libc/src/__support/threads/CndVar.h
new file mode 100644
index 00000000000000..ac602b650f165c
--- /dev/null
+++ b/libc/src/__support/threads/CndVar.h
@@ -0,0 +1,52 @@
+//===-- A platform independent abstraction layer for cond vars --*- 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___SUPPORT_SRC_THREADS_LINUX_CNDVAR_H
+#define LLVM_LIBC___SUPPORT_SRC_THREADS_LINUX_CNDVAR_H
+
+#include "src/__support/CPP/atomic.h"
+#include "src/__support/threads/mutex.h"
+
+#include <stdint.h> // uint32_t
+
+namespace LIBC_NAMESPACE {
+
+struct CndVar {
+ enum CndWaiterStatus : uint32_t {
+ WS_Waiting = 0xE,
+ WS_Signalled = 0x5,
+ };
+
+ struct CndWaiter {
+ cpp::Atomic<uint32_t> futex_word = WS_Waiting;
+ CndWaiter *next = nullptr;
+ };
+
+ CndWaiter *waitq_front;
+ CndWaiter *waitq_back;
+ Mutex qmtx;
+
+ static int init(CndVar *cv) {
+ cv->waitq_front = cv->waitq_back = nullptr;
+ auto err = Mutex::init(&cv->qmtx, false, false, false);
+ return err == MutexError::NONE ? 0 : -1;
+ }
+
+ static void destroy(CndVar *cv) {
+ cv->waitq_front = cv->waitq_back = nullptr;
+ }
+
+ // Returns 0 on success, -1 on error.
+ int wait(Mutex *m);
+ int notify_one();
+ int broadcast();
+};
+
+} // namespace LIBC_NAMESPACE
+
+#endif // LLVM_LIBC_SRC___SUPPORT_THREADS_LINUX_CNDVAR_H
diff --git a/libc/src/__support/threads/linux/CMakeLists.txt b/libc/src/__support/threads/linux/CMakeLists.txt
index 87a7a66ac6ea57..4abedc9c757422 100644
--- a/libc/src/__support/threads/linux/CMakeLists.txt
+++ b/libc/src/__support/threads/linux/CMakeLists.txt
@@ -55,3 +55,11 @@ add_object_library(
libc.src.__support.CPP.limits
libc.src.__support.OSUtil.osutil
)
+
+add_object_library(
+ CndVar
+ SRCS
+ CndVar.cpp
+ HDRS
+ ../CndVar.h
+)
diff --git a/libc/src/__support/threads/linux/CndVar.cpp b/libc/src/__support/threads/linux/CndVar.cpp
new file mode 100644
index 00000000000000..9dd006ea15174b
--- /dev/null
+++ b/libc/src/__support/threads/linux/CndVar.cpp
@@ -0,0 +1,102 @@
+//===-- Utility condition variable class ------------------------*- 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
+//
+//===----------------------------------------------------------------------===//
+
+#include "src/__support/threads/CndVar.h"
+
+namespace LIBC_NAMESPACE {
+
+int CndVar::wait(Mutex *m) {
+ // The goal is to perform "unlock |m| and wait" in an
+ // atomic operation. However, it is not possible to do it
+ // in the true sense so we do it in spirit. Before unlocking
+ // |m|, a new waiter object is added to the waiter queue with
+ // the waiter queue locked. Iff a signalling thread signals
+ // the waiter before the waiter actually starts waiting, the
+ // wait operation will not begin at all and the waiter immediately
+ // returns.
+
+ CndWaiter waiter;
+ {
+ MutexLock ml(&qmtx);
+ CndWaiter *old_back = nullptr;
+ if (waitq_front == nullptr) {
+ waitq_front = waitq_back = &waiter;
+ } else {
+ old_back = waitq_back;
+ waitq_back->next = &waiter;
+ waitq_back = &waiter;
+ }
+
+ if (m->unlock() != MutexError::NONE) {
+ // If we do not remove the queued up waiter before returning,
+ // then another thread can potentially signal a non-existing
+ // waiter. Note also that we do this with |qmtx| locked. This
+ // ensures that another thread will not signal the withdrawing
+ // waiter.
+ waitq_back = old_back;
+ if (waitq_back == nullptr)
+ waitq_front = nullptr;
+ else
+ waitq_back->next = nullptr;
+
+ return -1;
+ }
+ }
+
+ LIBC_NAMESPACE::syscall_impl<long>(FUTEX_SYSCALL_ID, &waiter.futex_word.val,
+ FUTEX_WAIT, WS_Waiting, 0, 0, 0);
+
+ // At this point, if locking |m| fails, we can simply return as the
+ // queued up waiter would have been removed from the queue.
+ auto err = m->lock();
+ return err == MutexError::NONE ? 0 : -1;
+}
+
+int CndVar::notify_one() {
+ // We don't use an RAII locker in this method as we want to unlock
+ // |qmtx| and signal the waiter using a single FUTEX_WAKE_OP signal.
+ qmtx.lock();
+ if (waitq_front == nullptr) {
+ qmtx.unlock();
+ return 0;
+ }
+
+ CndWaiter *first = waitq_front;
+ waitq_front = waitq_front->next;
+ if (waitq_front == nullptr)
+ waitq_back = nullptr;
+
+ qmtx.futex_word = FutexWordType(Mutex::LockState::Free);
+
+ LIBC_NAMESPACE::syscall_impl<long>(
+ FUTEX_SYSCALL_ID, &qmtx.futex_word.val, FUTEX_WAKE_OP, 1, 1,
+ &first->futex_word.val,
+ FUTEX_OP(FUTEX_OP_SET, WS_Signalled, FUTEX_OP_CMP_EQ, WS_Waiting));
+ return 0;
+}
+
+int CndVar::broadcast() {
+ MutexLock ml(&qmtx);
+ uint32_t dummy_futex_word;
+ CndWaiter *waiter = waitq_front;
+ waitq_front = waitq_back = nullptr;
+ while (waiter != nullptr) {
+ // FUTEX_WAKE_OP is used instead of just FUTEX_WAKE as it allows us to
+ // atomically update the waiter status to WS_Signalled before waking
+ // up the waiter. A dummy location is used for the other futex of
+ // FUTEX_WAKE_OP.
+ LIBC_NAMESPACE::syscall_impl<long>(
+ FUTEX_SYSCALL_ID, &dummy_futex_word, FUTEX_WAKE_OP, 1, 1,
+ &waiter->futex_word.val,
+ FUTEX_OP(FUTEX_OP_SET, WS_Signalled, FUTEX_OP_CMP_EQ, WS_Waiting));
+ waiter = waiter->next;
+ }
+ return 0;
+}
+
+} // namespace LIBC_NAMESPACE
diff --git a/libc/src/threads/linux/CMakeLists.txt b/libc/src/threads/linux/CMakeLists.txt
index be5407031aaddb..2fa492edc79001 100644
--- a/libc/src/threads/linux/CMakeLists.txt
+++ b/libc/src/threads/linux/CMakeLists.txt
@@ -1,7 +1,6 @@
add_header_library(
threads_utils
HDRS
- CndVar.h
Futex.h
DEPENDS
libc.include.sys_syscall
@@ -21,6 +20,7 @@ add_entrypoint_object(
DEPENDS
.threads_utils
libc.include.threads
+ libc.src.__support.threads.CndVar
)
add_entrypoint_object(
@@ -32,6 +32,7 @@ add_entrypoint_object(
DEPENDS
.threads_utils
libc.include.threads
+ libc.src.__support.threads.CndVar
)
add_entrypoint_object(
@@ -44,6 +45,7 @@ add_entrypoint_object(
.threads_utils
libc.include.threads
libc.src.__support.threads.mutex
+ libc.src.__support.threads.CndVar
)
add_entrypoint_object(
@@ -55,6 +57,7 @@ add_entrypoint_object(
DEPENDS
.threads_utils
libc.include.threads
+ libc.src.__support.threads.CndVar
)
add_entrypoint_object(
@@ -66,4 +69,5 @@ add_entrypoint_object(
DEPENDS
.threads_utils
libc.include.threads
+ libc.src.__support.threads.CndVar
)
diff --git a/libc/src/threads/linux/CndVar.h b/libc/src/threads/linux/CndVar.h
deleted file mode 100644
index b4afdef9f9eba7..00000000000000
--- a/libc/src/threads/linux/CndVar.h
+++ /dev/null
@@ -1,146 +0,0 @@
-//===-- Utility condition variable class ------------------------*- 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_THREADS_LINUX_CNDVAR_H
-#define LLVM_LIBC_SRC_THREADS_LINUX_CNDVAR_H
-
-#include "src/__support/CPP/atomic.h"
-#include "src/__support/OSUtil/syscall.h" // For syscall functions.
-#include "src/__support/threads/linux/futex_word.h"
-#include "src/__support/threads/mutex.h"
-
-#include <linux/futex.h> // For futex operations.
-#include <stdint.h>
-#include <sys/syscall.h> // For syscall numbers.
-#include <threads.h> // For values like thrd_success etc.
-
-namespace LIBC_NAMESPACE {
-
-struct CndVar {
- enum CndWaiterStatus : uint32_t {
- WS_Waiting = 0xE,
- WS_Signalled = 0x5,
- };
-
- struct CndWaiter {
- cpp::Atomic<uint32_t> futex_word = WS_Waiting;
- CndWaiter *next = nullptr;
- };
-
- CndWaiter *waitq_front;
- CndWaiter *waitq_back;
- Mutex qmtx;
-
- static int init(CndVar *cv) {
- cv->waitq_front = cv->waitq_back = nullptr;
- auto err = Mutex::init(&cv->qmtx, false, false, false);
- return err == MutexError::NONE ? thrd_success : thrd_error;
- }
-
- static void destroy(CndVar *cv) {
- cv->waitq_front = cv->waitq_back = nullptr;
- }
-
- int wait(Mutex *m) {
- // The goal is to perform "unlock |m| and wait" in an
- // atomic operation. However, it is not possible to do it
- // in the true sense so we do it in spirit. Before unlocking
- // |m|, a new waiter object is added to the waiter queue with
- // the waiter queue locked. Iff a signalling thread signals
- // the waiter before the waiter actually starts waiting, the
- // wait operation will not begin at all and the waiter immediately
- // returns.
-
- CndWaiter waiter;
- {
- MutexLock ml(&qmtx);
- CndWaiter *old_back = nullptr;
- if (waitq_front == nullptr) {
- waitq_front = waitq_back = &waiter;
- } else {
- old_back = waitq_back;
- waitq_back->next = &waiter;
- waitq_back = &waiter;
- }
-
- if (m->unlock() != MutexError::NONE) {
- // If we do not remove the queued up waiter before returning,
- // then another thread can potentially signal a non-existing
- // waiter. Note also that we do this with |qmtx| locked. This
- // ensures that another thread will not signal the withdrawing
- // waiter.
- waitq_back = old_back;
- if (waitq_back == nullptr)
- waitq_front = nullptr;
- else
- waitq_back->next = nullptr;
-
- return thrd_error;
- }
- }
-
- LIBC_NAMESPACE::syscall_impl<long>(FUTEX_SYSCALL_ID, &waiter.futex_word.val,
- FUTEX_WAIT, WS_Waiting, 0, 0, 0);
-
- // At this point, if locking |m| fails, we can simply return as the
- // queued up waiter would have been removed from the queue.
- auto err = m->lock();
- return err == MutexError::NONE ? thrd_success : thrd_error;
- }
-
- int notify_one() {
- // We don't use an RAII locker in this method as we want to unlock
- // |qmtx| and signal the waiter using a single FUTEX_WAKE_OP signal.
- qmtx.lock();
- if (waitq_front == nullptr) {
- qmtx.unlock();
- return thrd_success;
- }
-
- CndWaiter *first = waitq_front;
- waitq_front = waitq_front->next;
- if (waitq_front == nullptr)
- waitq_back = nullptr;
-
- qmtx.futex_word = FutexWordType(Mutex::LockState::Free);
-
- LIBC_NAMESPACE::syscall_impl<long>(
- FUTEX_SYSCALL_ID, &qmtx.futex_word.val, FUTEX_WAKE_OP, 1, 1,
- &first->futex_word.val,
- FUTEX_OP(FUTEX_OP_SET, WS_Signalled, FUTEX_OP_CMP_EQ, WS_Waiting));
- return thrd_success;
- }
-
- int broadcast() {
- MutexLock ml(&qmtx);
- uint32_t dummy_futex_word;
- CndWaiter *waiter = waitq_front;
- waitq_front = waitq_back = nullptr;
- while (waiter != nullptr) {
- // FUTEX_WAKE_OP is used instead of just FUTEX_WAKE as it allows us to
- // atomically update the waiter status to WS_Signalled before waking
- // up the waiter. A dummy location is used for the other futex of
- // FUTEX_WAKE_OP.
- LIBC_NAMESPACE::syscall_impl<long>(
- FUTEX_SYSCALL_ID, &dummy_futex_word, FUTEX_WAKE_OP, 1, 1,
- &waiter->futex_word.val,
- FUTEX_OP(FUTEX_OP_SET, WS_Signalled, FUTEX_OP_CMP_EQ, WS_Waiting));
- waiter = waiter->next;
- }
- return thrd_success;
- }
-};
-
-static_assert(sizeof(CndVar) == sizeof(cnd_t),
- "Mismatch in the size of the "
- "internal representation of condition variable and the public "
- "cnd_t type.");
-
-} // namespace LIBC_NAMESPACE
-
-#endif // LLVM_LIBC_SRC_THREADS_LINUX_CNDVAR_H
diff --git a/libc/src/threads/linux/cnd_broadcast.cpp b/libc/src/threads/linux/cnd_broadcast.cpp
index 180ac6d68ee864..5a6d34c28d080d 100644
--- a/libc/src/threads/linux/cnd_broadcast.cpp
+++ b/libc/src/threads/linux/cnd_broadcast.cpp
@@ -6,16 +6,16 @@
//
//===----------------------------------------------------------------------===//
-#include "CndVar.h"
-#include "src/threads/cnd_broadcast.h"
#include "src/__support/common.h"
+#include "src/__support/threads/CndVar.h"
+#include "src/threads/cnd_broadcast.h"
namespace LIBC_NAMESPACE {
LLVM_LIBC_FUNCTION(int, cnd_broadcast, (cnd_t * cond)) {
CndVar *cndvar = reinterpret_cast<CndVar *>(cond);
- return cndvar->broadcast();
+ return cndvar->broadcast() ? thrd_error : thrd_success;
}
} // namespace LIBC_NAMESPACE
diff --git a/libc/src/threads/linux/cnd_destroy.cpp b/libc/src/threads/linux/cnd_destroy.cpp
index 08eb3a1057b112..e1b2a3616ed095 100644
--- a/libc/src/threads/linux/cnd_destroy.cpp
+++ b/libc/src/threads/linux/cnd_destroy.cpp
@@ -6,10 +6,9 @@
//
//===----------------------------------------------------------------------===//
-#include "CndVar.h"
-
-#include "src/threads/cnd_destroy.h"
#include "src/__support/common.h"
+#include "src/__support/threads/CndVar.h"
+#include "src/threads/cnd_destroy.h"
namespace LIBC_NAMESPACE {
diff --git a/libc/src/threads/linux/cnd_init.cpp b/libc/src/threads/linux/cnd_init.cpp
index 5e3f360b1d2b99..d1bfdac89e004e 100644
--- a/libc/src/threads/linux/cnd_init.cpp
+++ b/libc/src/threads/linux/cnd_init.cpp
@@ -6,16 +6,16 @@
//
//===----------------------------------------------------------------------===//
-#include "CndVar.h"
-#include "src/threads/cnd_init.h"
#include "src/__support/common.h"
+#include "src/__support/threads/CndVar.h"
+#include "src/threads/cnd_init.h"
namespace LIBC_NAMESPACE {
LLVM_LIBC_FUNCTION(int, cnd_init, (cnd_t * cond)) {
CndVar *cndvar = reinterpret_cast<CndVar *>(cond);
- return CndVar::init(cndvar);
+ return CndVar::init(cndvar) ? thrd_error : thrd_success;
}
} // namespace LIBC_NAMESPACE
diff --git a/libc/src/threads/linux/cnd_signal.cpp b/libc/src/threads/linux/cnd_signal.cpp
index dba01abdefbc94..1111c2ce214eb4 100644
--- a/libc/src/threads/linux/cnd_signal.cpp
+++ b/libc/src/threads/linux/cnd_signal.cpp
@@ -6,16 +6,15 @@
//
//===----------------------------------------------------------------------===//
-#include "CndVar.h"
-
-#include "src/threads/cnd_signal.h"
#include "src/__support/common.h"
+#include "src/__support/threads/CndVar.h"
+#include "src/threads/cnd_signal.h"
namespace LIBC_NAMESPACE {
LLVM_LIBC_FUNCTION(int, cnd_signal, (cnd_t * cond)) {
CndVar *cndvar = reinterpret_cast<CndVar *>(cond);
- return cndvar->notify_one();
+ return cndvar->notify_one() ? thrd_error : thrd_success;
}
} // namespace LIBC_NAMESPACE
diff --git a/libc/src/threads/linux/cnd_wait.cpp b/libc/src/threads/linux/cnd_wait.cpp
index db3d7f1436eb76..32c5bfc64e14d3 100644
--- a/libc/src/threads/linux/cnd_wait.cpp
+++ b/libc/src/threads/linux/cnd_wait.cpp
@@ -6,9 +6,8 @@
//
//===----------------------------------------------------------------------===//
-#include "CndVar.h"
-
#include "src/__support/common.h"
+#include "src/__support/threads/CndVar.h"
#include "src/__support/threads/mutex.h"
#include "src/threads/cnd_wait.h"
@@ -17,7 +16,7 @@ namespace LIBC_NAMESPACE {
LLVM_LIBC_FUNCTION(int, cnd_wait, (cnd_t * cond, mtx_t *mtx)) {
CndVar *cndvar = reinterpret_cast<CndVar *>(cond);
Mutex *mutex = reinterpret_cast<Mutex *>(mtx);
- return cndvar->wait(mutex);
+ return cndvar->wait(mutex) ? thrd_error : thrd_success;
}
} // namespace LIBC_NAMESPACE
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
| LLVM_LIBC_FUNCTION(int, cnd_broadcast, (cnd_t * cond)) { | ||
| CndVar *cndvar = reinterpret_cast<CndVar *>(cond); | ||
| return cndvar->broadcast(); | ||
| return cndvar->broadcast() ? thrd_error : thrd_success; |
There was a problem hiding this comment.
the way it's currently implemented, cndvar->broadcast() can only return 0. This extra condition is unnecessary.
There was a problem hiding this comment.
I guess CndVar::broadcast (and CndVar::notify_one) can't return non-success values. Might as well make them void functions.
There was a problem hiding this comment.
done in 99bc8560f60a633c6fb08e75954bb040e7c65df7 PTAL
|
@SchrodingerZhu does this PR have any bearing on https://discourse.llvm.org/t/walking-through-pthread-synchronization-utilities/78967? |
|
may conflict with the mutex rework but nothing hard to fix |
nickdesaulniers
commented
May 21, 2024
| } | ||
| } | ||
|
|
||
| LIBC_NAMESPACE::syscall_impl<long>(FUTEX_SYSCALL_ID, &waiter.futex_word.val, |
There was a problem hiding this comment.
#91163 updated these naked syscalls. I should clean that up after this PR lands.
There was a problem hiding this comment.
going to rebase this onto main so that I fix this up BEFORE I land this PR.
We should be able to reuse this between the implementation of C11 cnd_t condition variables and POSIX pthread_cond_t condition variables. The current implementation is hyper linux specific, making use of Futex. That obviously wont work outside of linux, so split the OS specific functions off into their own source outside of the header. Fixes: llvm#88580 Link: llvm#88583
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
We should be able to reuse this between the implementation of C11 cnd_t
condition variables and POSIX pthread_cond_t condition variables.
The current implementation is hyper linux specific, making use of Futex. That
obviously wont work outside of linux, so split the OS specific functions off
into their own source outside of the header.
Modifies the return values of the to-be-shared impl to return 0 on success and
-1 on error. This pattern was shamelessly stolen from Bionic's
__bionic_thrd_error.
Fixes: #88580
Link: #88583