[libc] rework mutex #92168
Open
[libc] rework mutex #92168
+404
−118
Conversation
This file contains 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
|
@llvm/pr-subscribers-libc Author: Schrodinger ZHU Yifan (SchrodingerZhu) ChangesThis PR rework the
Patch is 24.87 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/92168.diff 19 Files Affected:
diff --git a/libc/config/config.json b/libc/config/config.json
index d6ef891b9f260..ee51ed03cdace 100644
--- a/libc/config/config.json
+++ b/libc/config/config.json
@@ -40,5 +40,15 @@
"value": true,
"doc": "Enable -fstack-protector-strong to defend against stack smashing attack."
}
+ },
+ "pthread": {
+ "LIBC_CONF_TIMEOUT_ENSURE_MONOTONICITY": {
+ "value": true,
+ "doc": "Automatically adjust timeout to CLOCK_MONOTONIC."
+ },
+ "LIBC_CONF_RAW_MUTEX_DEFAULT_SPIN_COUNT": {
+ "value": 100,
+ "doc": "Default number of spins before blocking if a mutex is contented."
+ }
}
}
diff --git a/libc/docs/configure.rst b/libc/docs/configure.rst
index 8f8c44caa1153..34ceadb071991 100644
--- a/libc/docs/configure.rst
+++ b/libc/docs/configure.rst
@@ -34,6 +34,9 @@ to learn about the defaults for your platform and target.
- ``LIBC_CONF_PRINTF_DISABLE_INDEX_MODE``: Disable index mode in the printf format string.
- ``LIBC_CONF_PRINTF_DISABLE_WRITE_INT``: Disable handling of %n in printf format string.
- ``LIBC_CONF_PRINTF_FLOAT_TO_STR_USE_MEGA_LONG_DOUBLE_TABLE``: Use large table for better printf long double performance.
+* **"pthread" options**
+ - ``LIBC_CONF_RAW_MUTEX_DEFAULT_SPIN_COUNT``: Default number of spins before blocking if a mutex is contented.
+ - ``LIBC_CONF_TIMEOUT_ENSURE_MONOTONICITY``: Automatically adjust timeout to CLOCK_MONOTONIC.
* **"string" options**
- ``LIBC_CONF_MEMSET_X86_USE_SOFTWARE_PREFETCHING``: Inserts prefetch for write instructions (PREFETCHW) for memset on x86 to recover performance when hardware prefetcher is disabled.
- ``LIBC_CONF_STRING_UNSAFE_WIDE_READ``: Read more than a byte at a time to perform byte-string operations like strlen.
diff --git a/libc/hdr/types/CMakeLists.txt b/libc/hdr/types/CMakeLists.txt
index 3a1bb2f3c340f..8e87642f66088 100644
--- a/libc/hdr/types/CMakeLists.txt
+++ b/libc/hdr/types/CMakeLists.txt
@@ -108,3 +108,12 @@ add_proxy_header_library(
libc.include.llvm-libc-types.struct_timeval
libc.include.sys_time
)
+
+add_proxy_header_library(
+ pid_t
+ HDRS
+ pid_t.h
+ FULL_BUILD_DEPENDS
+ libc.include.llvm-libc-types.pid_t
+ libc.include.sys_types
+)
diff --git a/libc/hdr/types/pid_t.h b/libc/hdr/types/pid_t.h
new file mode 100644
index 0000000000000..34306fc4118cf
--- /dev/null
+++ b/libc/hdr/types/pid_t.h
@@ -0,0 +1,22 @@
+//===-- Proxy for pid_t ---------------------------------------------------===//
+//
+// 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_HDR_TYPES_PID_T_H
+#define LLVM_LIBC_HDR_TYPES_PID_T_H
+
+#ifdef LIBC_FULL_BUILD
+
+#include "include/llvm-libc-types/pid_t.h"
+
+#else // Overlay mode
+
+#include <sys/types.h>
+
+#endif // LLVM_LIBC_FULL_BUILD
+
+#endif // LLVM_LIBC_HDR_TYPES_PID_T_H
diff --git a/libc/src/__support/File/dir.h b/libc/src/__support/File/dir.h
index a2f50f88e0e8b..ab445b6c0bec5 100644
--- a/libc/src/__support/File/dir.h
+++ b/libc/src/__support/File/dir.h
@@ -54,7 +54,7 @@ class Dir {
Dir(const Dir &) = delete;
explicit Dir(int fdesc)
- : fd(fdesc), readptr(0), fillsize(0), mutex(false, false, false) {}
+ : fd(fdesc), readptr(0), fillsize(0), mutex(false, false, false, false) {}
~Dir() = default;
Dir &operator=(const Dir &) = delete;
diff --git a/libc/src/__support/File/file.h b/libc/src/__support/File/file.h
index eafd3ab7d9107..6b41056cf40bd 100644
--- a/libc/src/__support/File/file.h
+++ b/libc/src/__support/File/file.h
@@ -154,7 +154,7 @@ class File {
uint8_t *buffer, size_t buffer_size, int buffer_mode,
bool owned, ModeFlags modeflags)
: platform_write(wf), platform_read(rf), platform_seek(sf),
- platform_close(cf), mutex(false, false, false), ungetc_buf(0),
+ platform_close(cf), mutex(false, false, false, false), ungetc_buf(0),
buf(buffer), bufsize(buffer_size), bufmode(buffer_mode), own_buf(owned),
mode(modeflags), pos(0), prev_op(FileOp::NONE), read_limit(0),
eof(false), err(false) {
diff --git a/libc/src/__support/threads/fork_callbacks.cpp b/libc/src/__support/threads/fork_callbacks.cpp
index 6efaf62f135ae..9ad3f38892f8f 100644
--- a/libc/src/__support/threads/fork_callbacks.cpp
+++ b/libc/src/__support/threads/fork_callbacks.cpp
@@ -33,7 +33,8 @@ class AtForkCallbackManager {
size_t next_index;
public:
- constexpr AtForkCallbackManager() : mtx(false, false, false), next_index(0) {}
+ constexpr AtForkCallbackManager()
+ : mtx(false, false, false, false), next_index(0) {}
bool register_triple(const ForkCallbackTriple &triple) {
cpp::lock_guard lock(mtx);
diff --git a/libc/src/__support/threads/gpu/mutex.h b/libc/src/__support/threads/gpu/mutex.h
index 71d0ef04cbfe5..9554083679335 100644
--- a/libc/src/__support/threads/gpu/mutex.h
+++ b/libc/src/__support/threads/gpu/mutex.h
@@ -19,7 +19,7 @@ namespace LIBC_NAMESPACE {
/// define the Mutex interface and require that only a single thread executes
/// code requiring a mutex lock.
struct Mutex {
- LIBC_INLINE constexpr Mutex(bool, bool, bool) {}
+ LIBC_INLINE constexpr Mutex(bool, bool, bool, bool) {}
LIBC_INLINE MutexError lock() { return MutexError::NONE; }
LIBC_INLINE MutexError unlock() { return MutexError::NONE; }
diff --git a/libc/src/__support/threads/linux/CMakeLists.txt b/libc/src/__support/threads/linux/CMakeLists.txt
index d3353f6b3ff8c..c207596e5072d 100644
--- a/libc/src/__support/threads/linux/CMakeLists.txt
+++ b/libc/src/__support/threads/linux/CMakeLists.txt
@@ -22,12 +22,35 @@ add_header_library(
libc.src.__support.time.linux.abs_timeout
)
+set(raw_mutex_additional_flags)
+if (LIBC_CONF_TIMEOUT_ENSURE_MONOTONICITY)
+ set(raw_mutex_additional_flags -DLIBC_COPT_TIMEOUT_ENSURE_MONOTONICITY)
+endif()
+
+add_header_library(
+ raw_mutex
+ HDRS
+ mutex.h
+ DEPENDS
+ .futex_utils
+ libc.src.__support.threads.sleep
+ libc.src.__support.time.linux.abs_timeout
+ libc.src.__support.time.linux.monotonicity
+ libc.src.__support.CPP.optional
+ libc.hdr.types.pid_t
+ COMPILE_OPTIONS
+ -DLIBC_COPT_RAW_MUTEX_DEFAULT_SPIN_COUNT=${LIBC_CONF_RAW_MUTEX_DEFAULT_SPIN_COUNT}
+ ${raw_mutex_additional_flags}
+
+)
+
add_header_library(
mutex
HDRS
mutex.h
DEPENDS
.futex_utils
+ .raw_mutex
libc.src.__support.threads.mutex_common
)
diff --git a/libc/src/__support/threads/linux/mutex.h b/libc/src/__support/threads/linux/mutex.h
index 6702de4651686..915919cba37e6 100644
--- a/libc/src/__support/threads/linux/mutex.h
+++ b/libc/src/__support/threads/linux/mutex.h
@@ -9,114 +9,81 @@
#ifndef LLVM_LIBC_SRC___SUPPORT_THREADS_LINUX_MUTEX_H
#define LLVM_LIBC_SRC___SUPPORT_THREADS_LINUX_MUTEX_H
+#include "hdr/types/pid_t.h"
+#include "src/__support/CPP/optional.h"
#include "src/__support/threads/linux/futex_utils.h"
+#include "src/__support/threads/linux/raw_mutex.h"
#include "src/__support/threads/mutex_common.h"
namespace LIBC_NAMESPACE {
-struct Mutex {
- unsigned char timed;
+
+// TODO: support shared/recursive/robust mutexes.
+class Mutex final : private internal::RawMutex {
+ // reserved timed, may be useful when combined with other flags.
+ [[maybe_unused]] unsigned char timed;
unsigned char recursive;
unsigned char robust;
+ unsigned char pshared;
- void *owner;
+ // TLS address may not work across forked processes. Use thead id instead.
+ pid_t owner;
unsigned long long lock_count;
- Futex futex_word;
-
enum class LockState : FutexWordType {
- Free,
- Locked,
- Waiting,
+ Free = UNLOCKED,
+ Locked = LOCKED,
+ Waiting = CONTENTED,
};
public:
- constexpr Mutex(bool istimed, bool isrecursive, bool isrobust)
- : timed(istimed), recursive(isrecursive), robust(isrobust),
- owner(nullptr), lock_count(0),
- futex_word(FutexWordType(LockState::Free)) {}
-
- static MutexError init(Mutex *mutex, bool istimed, bool isrecur,
- bool isrobust) {
- mutex->timed = istimed;
+ LIBC_INLINE constexpr Mutex(bool is_timed, bool is_recursive, bool is_robust,
+ bool is_pshared)
+ : internal::RawMutex(), timed(is_timed), recursive(is_recursive),
+ robust(is_robust), pshared(is_pshared), owner(0), lock_count(0) {}
+
+ LIBC_INLINE static MutexError init(Mutex *mutex, bool is_timed, bool isrecur,
+ bool isrobust, bool is_pshared) {
+ internal::RawMutex::init(mutex);
+ mutex->timed = is_timed;
mutex->recursive = isrecur;
mutex->robust = isrobust;
- mutex->owner = nullptr;
+ mutex->pshared = is_pshared;
+ mutex->owner = 0;
mutex->lock_count = 0;
- mutex->futex_word.set(FutexWordType(LockState::Free));
return MutexError::NONE;
}
- static MutexError destroy(Mutex *) { return MutexError::NONE; }
-
- MutexError reset();
-
- MutexError lock() {
- bool was_waiting = false;
- while (true) {
- FutexWordType mutex_status = FutexWordType(LockState::Free);
- FutexWordType locked_status = FutexWordType(LockState::Locked);
+ LIBC_INLINE static MutexError destroy(Mutex *) { return MutexError::NONE; }
- if (futex_word.compare_exchange_strong(
- mutex_status, FutexWordType(LockState::Locked))) {
- if (was_waiting)
- futex_word = FutexWordType(LockState::Waiting);
- return MutexError::NONE;
- }
-
- switch (LockState(mutex_status)) {
- case LockState::Waiting:
- // If other threads are waiting already, then join them. Note that the
- // futex syscall will block if the futex data is still
- // `LockState::Waiting` (the 4th argument to the syscall function
- // below.)
- futex_word.wait(FutexWordType(LockState::Waiting));
- was_waiting = true;
- // Once woken up/unblocked, try everything all over.
- continue;
- case LockState::Locked:
- // Mutex has been locked by another thread so set the status to
- // LockState::Waiting.
- if (futex_word.compare_exchange_strong(
- locked_status, FutexWordType(LockState::Waiting))) {
- // If we are able to set the futex data to `LockState::Waiting`, then
- // we will wait for the futex to be woken up. Note again that the
- // following syscall will block only if the futex data is still
- // `LockState::Waiting`.
- futex_word.wait(FutexWordType(LockState::Waiting));
- was_waiting = true;
- }
- continue;
- case LockState::Free:
- // If it was LockState::Free, we shouldn't be here at all.
- return MutexError::BAD_LOCK_STATE;
- }
- }
+ LIBC_INLINE MutexError lock() {
+ this->internal::RawMutex::lock(
+ /* timeout */ cpp::nullopt,
+ /* is_pshared */ this->pshared,
+ /* spin_count */ LIBC_COPT_RAW_MUTEX_DEFAULT_SPIN_COUNT);
+ // TODO: record owner and lock count.
+ return MutexError::NONE;
}
- MutexError unlock() {
- while (true) {
- FutexWordType mutex_status = FutexWordType(LockState::Waiting);
- if (futex_word.compare_exchange_strong(mutex_status,
- FutexWordType(LockState::Free))) {
- // If any thread is waiting to be woken up, then do it.
- futex_word.notify_one();
- return MutexError::NONE;
- }
+ LIBC_INLINE MutexError timed_lock(internal::AbsTimeout abs_time) {
+ if (this->internal::RawMutex::lock(
+ abs_time, /* is_shared */ this->pshared,
+ /* spin_count */ LIBC_COPT_RAW_MUTEX_DEFAULT_SPIN_COUNT))
+ return MutexError::NONE;
+ return MutexError::TIMEOUT;
+ }
- if (mutex_status == FutexWordType(LockState::Locked)) {
- // If nobody was waiting at this point, just free it.
- if (futex_word.compare_exchange_strong(mutex_status,
- FutexWordType(LockState::Free)))
- return MutexError::NONE;
- } else {
- // This can happen, for example if some thread tries to unlock an
- // already free mutex.
- return MutexError::UNLOCK_WITHOUT_LOCK;
- }
- }
+ LIBC_INLINE MutexError unlock() {
+ if (this->internal::RawMutex::unlock(/* is_shared */ this->pshared))
+ return MutexError::NONE;
+ return MutexError::UNLOCK_WITHOUT_LOCK;
}
- MutexError trylock();
+ LIBC_INLINE MutexError try_lock() {
+ if (this->internal::RawMutex::try_lock())
+ return MutexError::NONE;
+ return MutexError::BUSY;
+ }
+ friend struct CndVar;
};
} // namespace LIBC_NAMESPACE
diff --git a/libc/src/__support/threads/linux/raw_mutex.h b/libc/src/__support/threads/linux/raw_mutex.h
new file mode 100644
index 0000000000000..93da725d66d61
--- /dev/null
+++ b/libc/src/__support/threads/linux/raw_mutex.h
@@ -0,0 +1,116 @@
+//===--- Implementation of a Linux RawMutex 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___SUPPORT_THREADS_LINUX_RAW_MUTEX_H
+#define LLVM_LIBC_SRC___SUPPORT_THREADS_LINUX_RAW_MUTEX_H
+
+#include "futex_word.h"
+#include "src/__support/CPP/optional.h"
+#include "src/__support/common.h"
+#include "src/__support/macros/attributes.h"
+#include "src/__support/threads/linux/futex_utils.h"
+#include "src/__support/threads/sleep.h"
+#include "src/__support/time/linux/abs_timeout.h"
+#ifdef LIBC_COPT_TIMEOUT_ENSURE_MONOTONICITY
+#include "src/__support/time/linux/monotonicity.h"
+#endif
+
+#ifndef LIBC_COPT_RAW_MUTEX_DEFAULT_SPIN_COUNT
+#define LIBC_COPT_RAW_MUTEX_DEFAULT_SPIN_COUNT 100
+#endif
+
+namespace LIBC_NAMESPACE {
+namespace internal {
+// Lock is a simple timable lock for internal usage.
+// This is separated from Mutex because this one does not need to consider
+// rubustness and reentrancy. Also, this one has spin optimization for shorter
+// critical sections.
+class RawMutex {
+protected:
+ Futex futex;
+ LIBC_INLINE_VAR static constexpr FutexWordType UNLOCKED = 0b00;
+ LIBC_INLINE_VAR static constexpr FutexWordType LOCKED = 0b01;
+ LIBC_INLINE_VAR static constexpr FutexWordType CONTENTED = 0b10;
+
+ LIBC_INLINE FutexWordType spin(uint_fast32_t spin_count) {
+ FutexWordType result;
+ for (;;) {
+ result = futex.load(cpp::MemoryOrder::RELAXED);
+ // spin until one of the following conditions is met:
+ // - the mutex is unlocked
+ // - the mutex is contented
+ // - the spin count reaches 0
+ if (result != LOCKED || spin_count == 0)
+ return result;
+ // Pause the pipeline to avoid extraneous memory operations due to
+ // speculation.
+ sleep_briefly();
+ spin_count--;
+ };
+ }
+
+ // Return true if the lock is acquired. Return false if timeout happens before
+ // the lock is acquired.
+ [[gnu::cold]] LIBC_INLINE bool
+ lock_contented(cpp::optional<Futex::Timeout> timeout, bool is_pshared,
+ uint_fast32_t spin_count) {
+ FutexWordType state = spin(spin_count);
+ // Before go into contention state, try to grab the lock.
+ if (state == UNLOCKED &&
+ futex.compare_exchange_strong(state, LOCKED, cpp::MemoryOrder::ACQUIRE,
+ cpp::MemoryOrder::RELAXED))
+ return true;
+#ifdef LIBC_COPT_TIMEOUT_ENSURE_MONOTONICITY
+ if (timeout)
+ ensure_monotonicity(*timeout);
+#endif
+ for (;;) {
+ // Try to grab the lock if it is unlocked. Mark contented if it is locked.
+ if (state != CONTENTED &&
+ futex.exchange(CONTENTED, cpp::MemoryOrder::ACQUIRE) == UNLOCKED)
+ return true;
+ // Contention persists. Park the thread and wait for further notification.
+ if (-ETIMEDOUT == futex.wait(CONTENTED, timeout, is_pshared))
+ return false;
+ // Continue to spin after waking up.
+ state = spin(spin_count);
+ }
+ }
+
+ [[gnu::cold]] LIBC_INLINE void wake(bool is_pshared) {
+ futex.notify_one(is_pshared);
+ }
+
+public:
+ LIBC_INLINE static void init(RawMutex *mutex) { mutex->futex = UNLOCKED; }
+ LIBC_INLINE constexpr RawMutex() : futex(UNLOCKED) {}
+ LIBC_INLINE bool try_lock() {
+ FutexWordType expected = UNLOCKED;
+ // Use strong version since this is a one-time operation.
+ return futex.compare_exchange_strong(
+ expected, LOCKED, cpp::MemoryOrder::ACQUIRE, cpp::MemoryOrder::RELAXED);
+ }
+ LIBC_INLINE bool
+ lock(cpp::optional<Futex::Timeout> timeout = cpp::nullopt,
+ bool is_shared = false,
+ uint_fast32_t spin_count = LIBC_COPT_RAW_MUTEX_DEFAULT_SPIN_COUNT) {
+ // Timeout will not be checked if immediate lock is possible.
+ return try_lock() || lock_contented(timeout, is_shared, spin_count);
+ }
+ LIBC_INLINE bool unlock(bool is_pshared = false) {
+ FutexWordType prev = futex.exchange(UNLOCKED, cpp::MemoryOrder::RELEASE);
+ // if there is someone waiting, wake them up
+ if (prev == CONTENTED)
+ wake(is_pshared);
+ // Detect invalid unlock operation.
+ return prev == LOCKED;
+ }
+};
+} // namespace internal
+} // namespace LIBC_NAMESPACE
+
+#endif // LLVM_LIBC_SRC___SUPPORT_THREADS_LINUX_RAW_MUTEX_H
diff --git a/libc/src/__support/threads/thread.cpp b/libc/src/__support/threads/thread.cpp
index 7b02f8246e24b..2bea01bf332b3 100644
--- a/libc/src/__support/threads/thread.cpp
+++ b/libc/src/__support/threads/thread.cpp
@@ -54,7 +54,7 @@ class TSSKeyMgr {
cpp::array<TSSKeyUnit, TSS_KEY_COUNT> units;
public:
- constexpr TSSKeyMgr() : mtx(false, false, false) {}
+ constexpr TSSKeyMgr() : mtx(false, false, false, false) {}
cpp::optional<unsigned int> new_key(TSSDtor *dtor) {
cpp::lock_guard lock(mtx);
@@ -111,7 +111,7 @@ class ThreadAtExitCallbackMgr {
FixedVector<AtExitUnit, 1024> callback_list;
public:
- constexpr ThreadAtExitCallbackMgr() : mtx(false, false, false) {}
+ constexpr ThreadAtExitCallbackMgr() : mtx(false, false, false, false) {}
int add_callback(AtExitCallback *callback, void *obj) {
cpp::lock_guard lock(mtx);
diff --git a/libc/src/pthread/pthread_mutex_init.cpp b/libc/src/pthread/pthread_mutex_init.cpp
index f64d1fe68ad20..35615c449c044 100644
--- a/libc/src/pthread/pthread_mutex_init.cpp
+++ b/libc/src/pthread/pthread_mutex_init.cpp
@@ -26,9 +26,10 @@ LLVM_LIBC_FUNCTION(int, pthread_mutex_init,
const pthread_mutexattr_t *__restrict attr)) {
auto mutexattr = attr == nullptr ? DEFAULT_MUTEXATTR : *attr;
auto err =
- Mutex::init(reinterpret_cast<Mutex *>(m), false,
+ Mutex::init(reinterpret_cast<Mutex *>(m), /* timeout is supported */ true,
get_mutexattr_type(mutexattr) & PTHREAD_MUTEX_RECURSIVE,
- get_mutexattr_robust(mutexattr) & PTHREAD_MUTEX_ROBUST);
+ get_mutexattr_robust(mutexattr) & PTHREAD_MUTEX_ROBUST,
+ get_mutexattr_pshared(mutexattr) & PTHREAD_PROCESS_SHARED);
return err == MutexError::NONE ? 0 : EAGAIN;
}
diff --git a/libc/src/pthread/pthread_mutexattr.h b/libc/src/pthread/pthread_mutexattr.h
index 8b435854416a7..292ceebe049f9 100644
--- a/libc/src/pthread/pthread_mutexattr.h
+++ b/libc/src/pthread/pthread_mutexattr.h
@@ -43,6 +43,11 @@ LIBC_INLINE int get_mutexattr_robust(pthread_mutexattr_t attr) {
unsigned(PThreadMutexAttrPos::ROBUST_SHIFT);
}
+LIBC_INLINE int get_mutexattr_pshared(pthread_mutexattr_t attr) {
+ return (attr & unsigned(PThreadMutexAttrPos::PSHARED_MASK)) >>
+ unsigned(PThreadMutexAttrPos::PSHARED_SHIFT);
+}
+
} // namespace LIBC_NAMESPACE
#endif // LLVM_LIBC_SRC_PTHREAD_PTHREAD_MUTEXATTR_H
diff --git a/libc/src/stdlib/atexit.cpp b/libc/src/stdlib/atexit.cpp
index 4f0497444773d..fd24811000123 100644
--- a/libc/src/stdlib/atexit.cpp
+++ b/libc/src/stdlib/atexit.cpp
@@ -17,7 +17,7 @@ namespace LIBC_NAMESPACE {
namespace {
-Mutex handler_list_mtx(false, false, false);
+Mutex han...
[truncated]
|
QuarticCat
reviewed
May 14, 2024
|
I'm wondering if it's better to do CAS during spinning. |
Implementation of CAS typically generates a write event regardless of comparison. CAS loop may lead to significantly more cache controller events. |
|
Fair enough. |
michaelrj-google
requested changes
May 14, 2024
There was a problem hiding this comment.
I'm fine with the idea of having a raw mutex that other mutexes inherit from, but I'm not sure if the implementation of raw_mutex here is correct. Here's the posix standard on the behavior of the lock, trylock, and unlock functions: https://pubs.opengroup.org/onlinepubs/9699919799/functions/pthread_mutex_lock.html
My main concern is around spin. Generally it's better to let the kernel handle waiting (and possibly spinning) when possible, also 100 loops will take practically no time.
| return result; | ||
| // Pause the pipeline to avoid extraneous memory operations due to | ||
| // speculation. | ||
| sleep_briefly(); |
There was a problem hiding this comment.
this seems like it should use a similar mechanism to the previous lock function where it calls wait instead of spinning. Spinning tends to use more resources than just calling the wait function and letting the kernel handle the wakeup process..
There was a problem hiding this comment.
Modern mutex implementations often do a few spins before waiting. Because waiting is slow. This approach can greatly improve performance under light contention IIRC.
There was a problem hiding this comment.
that makes a lot of sense, do you have an example I could look at so I can understand this design better?
There was a problem hiding this comment.
I can only remember this blog post. But I do have seen this opinion in many places. Just think of many small critical regions contending for a single mutex. In this case, a few spins can avoid many syscalls.
There was a problem hiding this comment.
BTW, this PR looks pretty much similar to Rust's mutex implementation.
There was a problem hiding this comment.
Notice that the spin loop continues towards the limit if and only if the lock is held by someone and contention does not happen. In that case, the spinning thread knows that there is someone making progress in their critical section and the spinning one will likely obtain the lock once that critical section is over. Therefore, if the critical section is short, considerable syscalls can be avoided.
Notice that CndVar, RwLock exhibits waiting queues protected by mutexes. The critical section concerning the update of the queue is actually small. Hence, I think it may be a good choice to spin several rounds before going to sleep (futex_wait).
I currently choose 100 as the default value because it aligns with the choice of Rust's standard library.
There was a problem hiding this comment.
This does look very similar to the rust implementation, I have two comments about that: First, make sure that you're not just blindly copying what they do, it might not match the use case or semantics of libc. Second, make sure that you're not copying the code because that could have license implications. It should be fine as long as you're primarily writing the code yourself (but also I am not a lawyer).
As for leaving the spinning in, that's fine with me now that I understand the purpose of it.
There was a problem hiding this comment.
To my understanding, besides spin, Rust's mutex and rwlock implementation itself is largely similar to bionic's implementation (simplified because Rust does not support complicated pthread features). See
NormalMutexLock/NormalMutexTryLockin https://android.googlesource.com/platform/bionic/+/refs/heads/main/libc/bionic/pthread_mutex.cpp.- https://android.googlesource.com/platform/bionic/+/refs/heads/main/libc/bionic/pthread_rwlock.cpp (Rust's waiting queue is simplified because Rust has no timeout support).
The code here is not directly copied from Rust, whose RawMutex does not have pshared and timeout support (In fact, I was told that Rust's standard library is not designed to support forking the process without replacing the executable image).
However, I do think Rust has good clean and tidy codebase to study. Its platform abstraction layer can be very useful when learning about fundamental runtime operations.
There was a problem hiding this comment.
taking design inspiration from rust should be fine, and same for bionic. This code doesn't seem like it will be a problem but I wanted to make sure you were aware.
There was a problem hiding this comment.
heh, I was just poking through the book Rust Atomics and Lock: Low-Level Concurrency in Practice by Mara Bos for inspiration, coincidentally.
SchrodingerZhu
force-pushed
the
libc/mutex-rework-3
branch
2 times, most recently
from
2bb3e80
to
4e49145
Compare
Sure! |
SchrodingerZhu
force-pushed
the
libc/mutex-rework-3
branch
2 times, most recently
from
8ce4e6c
to
4ad3529
Compare
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
SchrodingerZhu
force-pushed
the
libc/mutex-rework-3
branch
3 times, most recently
from
172109a
to
a3abafd
Compare
| @@ -69,7 +71,7 @@ class Dir { | |||
| // cleaned up. | |||
| int close(); | |||
There was a problem hiding this comment.
all of the functions in this header should be marked LIBC_INLINE
There was a problem hiding this comment.
Just to confirm: including declarations with separate definitions in another TU?
michaelrj-google
approved these changes
May 28, 2024
SchrodingerZhu
force-pushed
the
libc/mutex-rework-3
branch
from
8148c1d
to
b9abf31
Compare
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.
This PR rework the
Muteximplementation.timedandpsharedsupport.