You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by "masaori335 (via GitHub)" <gi...@apache.org> on 2026/01/09 02:27:43 UTC
[PR] Avoid Stripe Mutex lock contention for RWW [trafficserver]
masaori335 opened a new pull request, #12794:
URL: https://github.com/apache/trafficserver/pull/12794
I found that if `OpenDir` has own reader-writer lock, it doesn't need `StripeSM mutex`. This means we can avoid the StripeSM mutex lock contention issue for reader-while-writer cases.
Benchmarking RWW is a bit tricky but one of benchmark says max rps is improved 9.9% in below conditions.
Conditions:
- 10 urls
- plaintext http
- response body size: 256 bytes
- origin returns `Cache-Control: public, max-age=0` ///< some requests goes RWW path
- 63 exec_thread
- 40 stripes (8 disks x 5 volumes)
Result:
- vanilla: 58,220.9 req/s
- patch: 63,999.7 req/s
part of #12788
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Avoid Stripe Mutex lock contention for RWW [trafficserver]
Posted by "masaori335 (via GitHub)" <gi...@apache.org>.
masaori335 commented on code in PR #12794:
URL: https://github.com/apache/trafficserver/pull/12794#discussion_r2726069506
##########
src/iocore/cache/Cache.cc:
##########
@@ -543,20 +565,24 @@ Cache::open_read(Continuation *cont, const CacheKey *key, CacheHTTPHdr *request,
OpenDirEntry *od = nullptr;
CacheVC *c = nullptr;
+ // Read-While-Writer
+ // This OpenDirEntry lookup doesn't need stripe mutex lock because OpenDir has own reader-writer lock
+ od = stripe->open_read(key);
Review Comment:
I checked this case. `CacheVC::openReadFromWriter` holds `stripe->mutex` and other operations also covered by `stripe->mutex`, so it's fine.
Also, what Copilot concerned, the dead lock of two mutex(s), the order is always 1). lock `stripe->mutex` then 2). lock `_shared_mutex`, so there're no ordering issue for now.
In the future, when we do refactoring with the `stripe->mutex`, we need to be careful, though.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Avoid Stripe Mutex lock contention for RWW [trafficserver]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #12794:
URL: https://github.com/apache/trafficserver/pull/12794#discussion_r2784666832
##########
src/tsutil/unit_tests/test_Bravo.cc:
##########
@@ -227,3 +227,543 @@ TEST_CASE("BRAVO - check with race", "[libts][BRAVO]")
CHECK(i == 2);
}
}
+
+TEST_CASE("Recursive BRAVO - exclusive lock", "[libts][BRAVO]")
+{
+ SECTION("single lock/unlock")
+ {
+ ts::bravo::recursive_shared_mutex mutex;
+ mutex.lock();
+ mutex.unlock();
+ }
+
+ SECTION("recursive lock/unlock")
+ {
+ ts::bravo::recursive_shared_mutex mutex;
+ mutex.lock();
+ mutex.lock();
+ mutex.lock();
+ mutex.unlock();
+ mutex.unlock();
+ mutex.unlock();
+ }
+
+ SECTION("try_lock by owner succeeds")
+ {
+ ts::bravo::recursive_shared_mutex mutex;
+ mutex.lock();
+ CHECK(mutex.try_lock() == true);
+ mutex.unlock();
+ mutex.unlock();
+ }
+
+ SECTION("try_lock by non-owner fails")
+ {
+ ts::bravo::recursive_shared_mutex mutex;
+ mutex.lock();
+
+ std::thread t{[&mutex]() { CHECK(mutex.try_lock() == false); }};
+ t.join();
+
+ mutex.unlock();
+ }
+
+ SECTION("recursive try_lock")
+ {
+ ts::bravo::recursive_shared_mutex mutex;
+ CHECK(mutex.try_lock() == true);
+ CHECK(mutex.try_lock() == true);
+ CHECK(mutex.try_lock() == true);
+ mutex.unlock();
+ mutex.unlock();
+ mutex.unlock();
+ }
+
+ SECTION("writer-writer blocking")
+ {
+ ts::bravo::recursive_shared_mutex mutex;
+ int i = 0;
+
+ std::thread t1{[&]() {
+ std::lock_guard<ts::bravo::recursive_shared_mutex> lock(mutex);
+ std::this_thread::sleep_for(100ms);
+ CHECK(++i == 1);
+ }};
+
+ std::thread t2{[&]() {
+ std::this_thread::sleep_for(50ms);
+ std::lock_guard<ts::bravo::recursive_shared_mutex> lock(mutex);
+ CHECK(++i == 2);
+ }};
+
+ t1.join();
+ t2.join();
+
+ CHECK(i == 2);
+ }
+}
+
+TEST_CASE("Recursive BRAVO - shared lock", "[libts][BRAVO]")
+{
+ SECTION("single shared lock/unlock")
+ {
+ ts::bravo::recursive_shared_mutex mutex;
+ ts::bravo::Token token{0};
+ mutex.lock_shared(token);
+ mutex.unlock_shared(token);
+ }
+
+ SECTION("recursive shared lock/unlock")
+ {
+ ts::bravo::recursive_shared_mutex mutex;
+ ts::bravo::Token token1{0};
+ ts::bravo::Token token2{0};
+ ts::bravo::Token token3{0};
+ mutex.lock_shared(token1);
+ mutex.lock_shared(token2);
+ mutex.lock_shared(token3);
+ // All tokens should be the same (cached)
+ CHECK(token1 == token2);
+ CHECK(token2 == token3);
+ mutex.unlock_shared(token3);
+ mutex.unlock_shared(token2);
+ mutex.unlock_shared(token1);
+ }
+
+ SECTION("try_lock_shared recursive")
+ {
+ ts::bravo::recursive_shared_mutex mutex;
+ ts::bravo::Token token1{0};
+ ts::bravo::Token token2{0};
+ CHECK(mutex.try_lock_shared(token1) == true);
+ CHECK(mutex.try_lock_shared(token2) == true);
+ CHECK(token1 == token2);
+ mutex.unlock_shared(token2);
+ mutex.unlock_shared(token1);
+ }
+
+ SECTION("multiple readers concurrent")
+ {
+ ts::bravo::recursive_shared_mutex mutex;
+ int i = 0;
+
+ std::thread t1{[&]() {
+ ts::bravo::Token token{0};
+ mutex.lock_shared(token);
+ CHECK(i == 0);
+ std::this_thread::sleep_for(50ms);
+ mutex.unlock_shared(token);
+ }};
+
+ std::thread t2{[&]() {
+ ts::bravo::Token token{0};
+ mutex.lock_shared(token);
+ CHECK(i == 0);
+ std::this_thread::sleep_for(50ms);
+ mutex.unlock_shared(token);
+ }};
+
+ t1.join();
+ t2.join();
+
+ CHECK(i == 0);
+ }
+
+ SECTION("shared blocks exclusive")
+ {
+ ts::bravo::recursive_shared_mutex mutex;
+ ts::bravo::Token token{0};
+ mutex.lock_shared(token);
+
+ std::thread t{[&mutex]() { CHECK(mutex.try_lock() == false); }};
+ t.join();
+
+ mutex.unlock_shared(token);
+ }
+
+ SECTION("exclusive blocks shared")
+ {
+ ts::bravo::recursive_shared_mutex mutex;
+ mutex.lock();
+
+ std::thread t{[&mutex]() {
+ ts::bravo::Token token{0};
+ CHECK(mutex.try_lock_shared(token) == false);
+ }};
+ t.join();
+
+ mutex.unlock();
+ }
+}
+
+TEST_CASE("Recursive BRAVO - mixed lock scenarios", "[libts][BRAVO]")
+{
+ SECTION("downgrade: exclusive owner can acquire shared lock")
+ {
+ ts::bravo::recursive_shared_mutex mutex;
+ mutex.lock();
+
+ // While holding exclusive lock, we can acquire shared lock
+ ts::bravo::Token token{0};
+ mutex.lock_shared(token);
+ CHECK(token == 0); // Special token for downgrade
+
+ mutex.unlock_shared(token);
+ mutex.unlock();
+ }
+
+ SECTION("downgrade: try_lock_shared succeeds for exclusive owner")
+ {
+ ts::bravo::recursive_shared_mutex mutex;
+ mutex.lock();
+
+ ts::bravo::Token token{0};
+ CHECK(mutex.try_lock_shared(token) == true);
+ CHECK(token == 0); // Special token for downgrade
+
+ mutex.unlock_shared(token);
+ mutex.unlock();
+ }
+
+ SECTION("upgrade prevention: try_lock fails when holding shared lock")
+ {
+ ts::bravo::recursive_shared_mutex mutex;
+ ts::bravo::Token token{0};
+ mutex.lock_shared(token);
+
+ // Cannot upgrade: try_lock should fail
+ CHECK(mutex.try_lock() == false);
+
+ mutex.unlock_shared(token);
+ }
+
+ SECTION("downgrade with multiple shared locks")
+ {
+ ts::bravo::recursive_shared_mutex mutex;
+ mutex.lock();
+
+ ts::bravo::Token token1{0};
+ ts::bravo::Token token2{0};
+ mutex.lock_shared(token1);
+ mutex.lock_shared(token2);
+
+ mutex.unlock_shared(token2);
+ mutex.unlock_shared(token1);
+ mutex.unlock();
+ }
+
+ SECTION("proper unlock ordering: shared then exclusive")
+ {
+ ts::bravo::recursive_shared_mutex mutex;
+ mutex.lock();
+
+ ts::bravo::Token token{0};
+ mutex.lock_shared(token);
+
+ // Unlock shared first, then exclusive
+ mutex.unlock_shared(token);
+ mutex.unlock();
+
+ // Mutex should be fully unlocked now
+ CHECK(mutex.try_lock() == true);
+ mutex.unlock();
+ }
+
+ SECTION("nested exclusive locks with shared in between")
+ {
+ ts::bravo::recursive_shared_mutex mutex;
+ mutex.lock();
+ mutex.lock(); // Recursive exclusive
+
+ ts::bravo::Token token{0};
+ mutex.lock_shared(token);
+
+ mutex.unlock_shared(token);
+ mutex.unlock(); // Second exclusive
+ mutex.unlock(); // First exclusive
+
+ // Mutex should be fully unlocked now
+ CHECK(mutex.try_lock() == true);
+ mutex.unlock();
+ }
+}
+
+TEST_CASE("Recursive BRAVO - BRAVO optimizations", "[libts][BRAVO]")
+{
+ SECTION("first shared lock gets token from underlying BRAVO mutex")
+ {
+ ts::bravo::recursive_shared_mutex mutex;
+ ts::bravo::Token token{0};
+ mutex.lock_shared(token);
+ // Token should be set by underlying BRAVO mutex (0 = slow path, >0 = fast path)
+ // We can't guarantee which path is taken, but the lock should succeed
+ mutex.unlock_shared(token);
+ }
+
+ SECTION("recursive shared locks reuse cached token")
+ {
+ ts::bravo::recursive_shared_mutex mutex;
+ ts::bravo::Token token1{0};
+ ts::bravo::Token token2{0};
+ ts::bravo::Token token3{0};
+
+ mutex.lock_shared(token1);
+ mutex.lock_shared(token2);
+ mutex.lock_shared(token3);
+
+ // All tokens should be identical (cached from first lock)
+ CHECK(token1 == token2);
+ CHECK(token2 == token3);
+
+ mutex.unlock_shared(token3);
+ mutex.unlock_shared(token2);
+ mutex.unlock_shared(token1);
+ }
+
+ SECTION("writer revocation then reader works")
+ {
+ ts::bravo::recursive_shared_mutex mutex;
+
+ // First, acquire and release a shared lock to enable read_bias
+ {
+ ts::bravo::Token token{0};
+ mutex.lock_shared(token);
+ mutex.unlock_shared(token);
+ }
+
+ // Writer acquires lock (triggers revocation)
+ mutex.lock();
+ mutex.unlock();
+
+ // Reader should still work after writer releases
+ {
+ ts::bravo::Token token{0};
+ mutex.lock_shared(token);
+ mutex.unlock_shared(token);
+ }
+ }
+
+ SECTION("multiple readers then writer then readers")
+ {
+ ts::bravo::recursive_shared_mutex mutex;
+ std::atomic<int> readers_done{0};
+
Review Comment:
This file now uses `std::atomic` (e.g., readers_done) but does not include <atomic>. Please add the missing header explicitly rather than relying on transitive includes.
##########
include/tsutil/Bravo.h:
##########
@@ -372,4 +372,195 @@ template <typename T = std::shared_mutex, size_t SLOT_SIZE = 256, int SLOWDOWN_G
using shared_mutex = shared_mutex_impl<>;
+/**
+ ts::bravo::recursive_shared_mutex_impl
+
+ A recursive version of shared_mutex_impl that allows the same thread
+ to acquire exclusive and shared locks multiple times.
+
+ Uses DenseThreadId for efficient per-thread state tracking without map overhead.
+ Optimized to minimize expensive std::this_thread::get_id() calls by using
+ DenseThreadId for ownership tracking.
+
+ Mixed lock semantics:
+ - Upgrade prevention: A thread holding a shared lock cannot acquire an exclusive lock
+ (would cause deadlock). try_lock() returns false, lock() asserts.
+ - Downgrade allowed: A thread holding an exclusive lock can acquire a shared lock.
+ */
+template <typename T = shared_mutex_impl<>, size_t SLOT_SIZE = 256> class recursive_shared_mutex_impl
+{
+ // Use a sentinel value for "no owner" - DenseThreadId values are 0 to SLOT_SIZE-1
+ static constexpr size_t NO_OWNER = SLOT_SIZE;
+
+public:
+ recursive_shared_mutex_impl() = default;
+ ~recursive_shared_mutex_impl() = default;
+
+ // No copying or moving
+ recursive_shared_mutex_impl(recursive_shared_mutex_impl const &) = delete;
+ recursive_shared_mutex_impl &operator=(recursive_shared_mutex_impl const &) = delete;
+ recursive_shared_mutex_impl(recursive_shared_mutex_impl &&) = delete;
+ recursive_shared_mutex_impl &operator=(recursive_shared_mutex_impl &&) = delete;
+
+ ////
+ // Exclusive locking (recursive)
+ //
+ void
+ lock()
+ {
+ size_t tid = DenseThreadId::self();
+ // Fast path: check if we already own the lock
+ if (_exclusive_owner.load(std::memory_order_relaxed) == tid) {
+ ++_exclusive_count;
+ return;
+ }
+ // Upgrade prevention: cannot acquire exclusive lock while holding shared lock
+ ThreadState &state = _thread_states[tid];
+ debug_assert(state.shared_count == 0);
+ _mutex.lock();
+ _exclusive_owner.store(tid, std::memory_order_relaxed);
+ _exclusive_count = 1;
+ }
+
+ bool
+ try_lock()
+ {
+ size_t tid = DenseThreadId::self();
+ // Fast path: check if we already own the lock
+ if (_exclusive_owner.load(std::memory_order_relaxed) == tid) {
+ ++_exclusive_count;
+ return true;
+ }
+ // Upgrade prevention: cannot acquire exclusive lock while holding shared lock
+ ThreadState &state = _thread_states[tid];
+ if (state.shared_count > 0) {
+ return false;
+ }
+ if (_mutex.try_lock()) {
+ _exclusive_owner.store(tid, std::memory_order_relaxed);
+ _exclusive_count = 1;
+ return true;
+ }
+ return false;
+ }
+
+ void
+ unlock()
+ {
+ if (--_exclusive_count == 0) {
+ _exclusive_owner.store(NO_OWNER, std::memory_order_relaxed);
+ _mutex.unlock();
+ }
+ }
+
+ ////
+ // Shared locking (recursive)
+ //
+ void
+ lock_shared(Token &token)
+ {
+ size_t tid = DenseThreadId::self();
+ ThreadState &state = _thread_states[tid];
+
+ // Fast path: already holding shared lock - just increment count (most common case)
+ size_t count = state.shared_count;
+ if (count > 0) {
+ state.shared_count = count + 1;
+ token = state.cached_token;
+ return;
+ }
+
+ // Check for downgrade: if we hold exclusive lock, allow shared lock without acquiring underlying
+ if (_exclusive_owner.load(std::memory_order_relaxed) == tid) {
+ state.shared_count = 1;
+ token = 0; // Special token indicating we're under exclusive lock
+ return;
+ }
+
+ // Slow path: acquire underlying lock
+ _mutex.lock_shared(state.cached_token);
+ state.shared_count = 1;
+ token = state.cached_token;
+ }
+
+ bool
+ try_lock_shared(Token &token)
+ {
+ size_t tid = DenseThreadId::self();
+ ThreadState &state = _thread_states[tid];
+
+ // Fast path: already holding shared lock - just increment count (most common case)
+ size_t count = state.shared_count;
+ if (count > 0) {
+ state.shared_count = count + 1;
+ token = state.cached_token;
+ return true;
+ }
+
+ // Check for downgrade: if we hold exclusive lock, allow shared lock without acquiring underlying
+ if (_exclusive_owner.load(std::memory_order_relaxed) == tid) {
+ state.shared_count = 1;
+ token = 0; // Special token indicating we're under exclusive lock
+ return true;
+ }
+
+ // Slow path: try to acquire underlying lock
+ if (_mutex.try_lock_shared(state.cached_token)) {
+ state.shared_count = 1;
+ token = state.cached_token;
+ return true;
+ }
+ return false;
+ }
+
+ void
+ unlock_shared(const Token /* token */)
+ {
+ size_t tid = DenseThreadId::self();
+ ThreadState &state = _thread_states[tid];
+ if (--state.shared_count == 0) {
+ // Only unlock underlying mutex if we're not holding exclusive lock
+ if (_exclusive_owner.load(std::memory_order_relaxed) != tid) {
+ _mutex.unlock_shared(state.cached_token);
+ }
+ state.cached_token = 0;
Review Comment:
recursive_shared_mutex_impl::unlock_shared ignores the `token` parameter and decides whether to unlock the underlying mutex based on `_exclusive_owner`. This makes the correctness of unlock_shared depend on exclusive lock state rather than on how the shared lock was acquired (e.g., the special `token=0` path). Consider using the token value (or an explicit per-thread flag) to distinguish the downgrade path reliably and add assertions for misuse.
```suggestion
unlock_shared(const Token token)
{
size_t tid = DenseThreadId::self();
ThreadState &state = _thread_states[tid];
TS_ASSERT(state.shared_count > 0);
if (--state.shared_count == 0) {
if (token == 0) {
// Downgrade path: shared lock acquired while holding exclusive lock;
// no underlying shared lock to release.
TS_ASSERT(_exclusive_owner.load(std::memory_order_relaxed) == tid);
} else {
// Regular shared lock: must release underlying shared lock.
TS_ASSERT(_exclusive_owner.load(std::memory_order_relaxed) != tid);
_mutex.unlock_shared(token);
state.cached_token = 0;
}
```
##########
src/iocore/cache/StripeSM.h:
##########
@@ -253,6 +267,24 @@ extern std::atomic<int> gnstripes;
extern ClassAllocator<OpenDirEntry, false> openDirEntryAllocator;
extern unsigned short *vol_hash_table;
+template <typename U, typename Func>
+U
+StripeSM::read_op(Func read_op) const
+{
+ ts::bravo::shared_lock lock(_shared_mutex);
+
+ return read_op();
+}
+
+template <typename U, typename Func>
+U
+StripeSM::write_op(Func write_op)
+{
+ std::lock_guard lock(_shared_mutex);
+
+ return write_op();
+}
Review Comment:
StripeSM::write_op (and similarly read_op) unconditionally does `return functor();`. This breaks when the helper is instantiated for `void` (e.g., CacheDir.cc calls `_stripe->write_op<void>(...)`) because `return expr;` is ill-formed in a void-returning function. Consider switching to `decltype(auto)` and handling `void` via `if constexpr`, or add dedicated `void` overloads.
##########
include/tsutil/Bravo.h:
##########
@@ -372,4 +372,195 @@ template <typename T = std::shared_mutex, size_t SLOT_SIZE = 256, int SLOWDOWN_G
using shared_mutex = shared_mutex_impl<>;
+/**
+ ts::bravo::recursive_shared_mutex_impl
+
+ A recursive version of shared_mutex_impl that allows the same thread
+ to acquire exclusive and shared locks multiple times.
+
+ Uses DenseThreadId for efficient per-thread state tracking without map overhead.
+ Optimized to minimize expensive std::this_thread::get_id() calls by using
+ DenseThreadId for ownership tracking.
+
+ Mixed lock semantics:
+ - Upgrade prevention: A thread holding a shared lock cannot acquire an exclusive lock
+ (would cause deadlock). try_lock() returns false, lock() asserts.
+ - Downgrade allowed: A thread holding an exclusive lock can acquire a shared lock.
+ */
+template <typename T = shared_mutex_impl<>, size_t SLOT_SIZE = 256> class recursive_shared_mutex_impl
+{
+ // Use a sentinel value for "no owner" - DenseThreadId values are 0 to SLOT_SIZE-1
+ static constexpr size_t NO_OWNER = SLOT_SIZE;
+
+public:
+ recursive_shared_mutex_impl() = default;
+ ~recursive_shared_mutex_impl() = default;
+
+ // No copying or moving
+ recursive_shared_mutex_impl(recursive_shared_mutex_impl const &) = delete;
+ recursive_shared_mutex_impl &operator=(recursive_shared_mutex_impl const &) = delete;
+ recursive_shared_mutex_impl(recursive_shared_mutex_impl &&) = delete;
+ recursive_shared_mutex_impl &operator=(recursive_shared_mutex_impl &&) = delete;
+
+ ////
+ // Exclusive locking (recursive)
+ //
+ void
+ lock()
+ {
+ size_t tid = DenseThreadId::self();
+ // Fast path: check if we already own the lock
+ if (_exclusive_owner.load(std::memory_order_relaxed) == tid) {
+ ++_exclusive_count;
+ return;
+ }
+ // Upgrade prevention: cannot acquire exclusive lock while holding shared lock
+ ThreadState &state = _thread_states[tid];
+ debug_assert(state.shared_count == 0);
+ _mutex.lock();
+ _exclusive_owner.store(tid, std::memory_order_relaxed);
+ _exclusive_count = 1;
+ }
+
+ bool
+ try_lock()
+ {
+ size_t tid = DenseThreadId::self();
+ // Fast path: check if we already own the lock
+ if (_exclusive_owner.load(std::memory_order_relaxed) == tid) {
+ ++_exclusive_count;
+ return true;
+ }
+ // Upgrade prevention: cannot acquire exclusive lock while holding shared lock
+ ThreadState &state = _thread_states[tid];
+ if (state.shared_count > 0) {
+ return false;
+ }
+ if (_mutex.try_lock()) {
+ _exclusive_owner.store(tid, std::memory_order_relaxed);
+ _exclusive_count = 1;
+ return true;
+ }
+ return false;
+ }
+
+ void
+ unlock()
+ {
+ if (--_exclusive_count == 0) {
+ _exclusive_owner.store(NO_OWNER, std::memory_order_relaxed);
+ _mutex.unlock();
+ }
+ }
Review Comment:
recursive_shared_mutex_impl's shared-under-exclusive path does not acquire an underlying shared lock (it just sets `token = 0`). If a caller unlocks the exclusive lock while `ThreadState::shared_count` is still > 0, the thread will no longer hold *any* lock, and a later unlock_shared may attempt to unlock an underlying shared lock that was never acquired. Add a guard (e.g., debug_assert shared_count==0 before releasing the underlying exclusive lock) or implement a true downgrade mechanism.
##########
src/tsutil/unit_tests/test_Bravo.cc:
##########
@@ -227,3 +227,543 @@ TEST_CASE("BRAVO - check with race", "[libts][BRAVO]")
CHECK(i == 2);
}
}
+
+TEST_CASE("Recursive BRAVO - exclusive lock", "[libts][BRAVO]")
+{
+ SECTION("single lock/unlock")
+ {
+ ts::bravo::recursive_shared_mutex mutex;
+ mutex.lock();
+ mutex.unlock();
+ }
+
+ SECTION("recursive lock/unlock")
+ {
+ ts::bravo::recursive_shared_mutex mutex;
+ mutex.lock();
+ mutex.lock();
+ mutex.lock();
+ mutex.unlock();
+ mutex.unlock();
+ mutex.unlock();
+ }
+
+ SECTION("try_lock by owner succeeds")
+ {
+ ts::bravo::recursive_shared_mutex mutex;
+ mutex.lock();
+ CHECK(mutex.try_lock() == true);
+ mutex.unlock();
+ mutex.unlock();
+ }
+
+ SECTION("try_lock by non-owner fails")
+ {
+ ts::bravo::recursive_shared_mutex mutex;
+ mutex.lock();
+
+ std::thread t{[&mutex]() { CHECK(mutex.try_lock() == false); }};
+ t.join();
+
+ mutex.unlock();
+ }
+
+ SECTION("recursive try_lock")
+ {
+ ts::bravo::recursive_shared_mutex mutex;
+ CHECK(mutex.try_lock() == true);
+ CHECK(mutex.try_lock() == true);
+ CHECK(mutex.try_lock() == true);
+ mutex.unlock();
+ mutex.unlock();
+ mutex.unlock();
+ }
+
+ SECTION("writer-writer blocking")
+ {
+ ts::bravo::recursive_shared_mutex mutex;
+ int i = 0;
+
+ std::thread t1{[&]() {
+ std::lock_guard<ts::bravo::recursive_shared_mutex> lock(mutex);
+ std::this_thread::sleep_for(100ms);
+ CHECK(++i == 1);
+ }};
+
+ std::thread t2{[&]() {
+ std::this_thread::sleep_for(50ms);
+ std::lock_guard<ts::bravo::recursive_shared_mutex> lock(mutex);
+ CHECK(++i == 2);
+ }};
+
+ t1.join();
+ t2.join();
+
+ CHECK(i == 2);
+ }
+}
+
+TEST_CASE("Recursive BRAVO - shared lock", "[libts][BRAVO]")
+{
+ SECTION("single shared lock/unlock")
+ {
+ ts::bravo::recursive_shared_mutex mutex;
+ ts::bravo::Token token{0};
+ mutex.lock_shared(token);
+ mutex.unlock_shared(token);
+ }
+
+ SECTION("recursive shared lock/unlock")
+ {
+ ts::bravo::recursive_shared_mutex mutex;
+ ts::bravo::Token token1{0};
+ ts::bravo::Token token2{0};
+ ts::bravo::Token token3{0};
+ mutex.lock_shared(token1);
+ mutex.lock_shared(token2);
+ mutex.lock_shared(token3);
+ // All tokens should be the same (cached)
+ CHECK(token1 == token2);
+ CHECK(token2 == token3);
+ mutex.unlock_shared(token3);
+ mutex.unlock_shared(token2);
+ mutex.unlock_shared(token1);
+ }
+
+ SECTION("try_lock_shared recursive")
+ {
+ ts::bravo::recursive_shared_mutex mutex;
+ ts::bravo::Token token1{0};
+ ts::bravo::Token token2{0};
+ CHECK(mutex.try_lock_shared(token1) == true);
+ CHECK(mutex.try_lock_shared(token2) == true);
+ CHECK(token1 == token2);
+ mutex.unlock_shared(token2);
+ mutex.unlock_shared(token1);
+ }
+
+ SECTION("multiple readers concurrent")
+ {
+ ts::bravo::recursive_shared_mutex mutex;
+ int i = 0;
+
+ std::thread t1{[&]() {
+ ts::bravo::Token token{0};
+ mutex.lock_shared(token);
+ CHECK(i == 0);
+ std::this_thread::sleep_for(50ms);
+ mutex.unlock_shared(token);
+ }};
+
+ std::thread t2{[&]() {
+ ts::bravo::Token token{0};
+ mutex.lock_shared(token);
+ CHECK(i == 0);
+ std::this_thread::sleep_for(50ms);
+ mutex.unlock_shared(token);
+ }};
+
+ t1.join();
+ t2.join();
+
+ CHECK(i == 0);
+ }
+
+ SECTION("shared blocks exclusive")
+ {
+ ts::bravo::recursive_shared_mutex mutex;
+ ts::bravo::Token token{0};
+ mutex.lock_shared(token);
+
+ std::thread t{[&mutex]() { CHECK(mutex.try_lock() == false); }};
+ t.join();
+
+ mutex.unlock_shared(token);
+ }
+
+ SECTION("exclusive blocks shared")
+ {
+ ts::bravo::recursive_shared_mutex mutex;
+ mutex.lock();
+
+ std::thread t{[&mutex]() {
+ ts::bravo::Token token{0};
+ CHECK(mutex.try_lock_shared(token) == false);
+ }};
+ t.join();
+
+ mutex.unlock();
+ }
+}
+
+TEST_CASE("Recursive BRAVO - mixed lock scenarios", "[libts][BRAVO]")
+{
+ SECTION("downgrade: exclusive owner can acquire shared lock")
+ {
+ ts::bravo::recursive_shared_mutex mutex;
+ mutex.lock();
+
+ // While holding exclusive lock, we can acquire shared lock
+ ts::bravo::Token token{0};
+ mutex.lock_shared(token);
+ CHECK(token == 0); // Special token for downgrade
+
+ mutex.unlock_shared(token);
+ mutex.unlock();
+ }
+
+ SECTION("downgrade: try_lock_shared succeeds for exclusive owner")
+ {
+ ts::bravo::recursive_shared_mutex mutex;
+ mutex.lock();
+
+ ts::bravo::Token token{0};
+ CHECK(mutex.try_lock_shared(token) == true);
+ CHECK(token == 0); // Special token for downgrade
+
+ mutex.unlock_shared(token);
+ mutex.unlock();
+ }
+
+ SECTION("upgrade prevention: try_lock fails when holding shared lock")
+ {
+ ts::bravo::recursive_shared_mutex mutex;
+ ts::bravo::Token token{0};
+ mutex.lock_shared(token);
+
+ // Cannot upgrade: try_lock should fail
+ CHECK(mutex.try_lock() == false);
+
+ mutex.unlock_shared(token);
+ }
+
+ SECTION("downgrade with multiple shared locks")
+ {
+ ts::bravo::recursive_shared_mutex mutex;
+ mutex.lock();
+
+ ts::bravo::Token token1{0};
+ ts::bravo::Token token2{0};
+ mutex.lock_shared(token1);
+ mutex.lock_shared(token2);
+
+ mutex.unlock_shared(token2);
+ mutex.unlock_shared(token1);
+ mutex.unlock();
+ }
+
+ SECTION("proper unlock ordering: shared then exclusive")
+ {
+ ts::bravo::recursive_shared_mutex mutex;
+ mutex.lock();
+
+ ts::bravo::Token token{0};
+ mutex.lock_shared(token);
+
+ // Unlock shared first, then exclusive
+ mutex.unlock_shared(token);
+ mutex.unlock();
+
+ // Mutex should be fully unlocked now
+ CHECK(mutex.try_lock() == true);
+ mutex.unlock();
+ }
+
+ SECTION("nested exclusive locks with shared in between")
+ {
+ ts::bravo::recursive_shared_mutex mutex;
+ mutex.lock();
+ mutex.lock(); // Recursive exclusive
+
+ ts::bravo::Token token{0};
+ mutex.lock_shared(token);
+
+ mutex.unlock_shared(token);
+ mutex.unlock(); // Second exclusive
+ mutex.unlock(); // First exclusive
+
+ // Mutex should be fully unlocked now
+ CHECK(mutex.try_lock() == true);
+ mutex.unlock();
+ }
+}
+
+TEST_CASE("Recursive BRAVO - BRAVO optimizations", "[libts][BRAVO]")
+{
+ SECTION("first shared lock gets token from underlying BRAVO mutex")
+ {
+ ts::bravo::recursive_shared_mutex mutex;
+ ts::bravo::Token token{0};
+ mutex.lock_shared(token);
+ // Token should be set by underlying BRAVO mutex (0 = slow path, >0 = fast path)
+ // We can't guarantee which path is taken, but the lock should succeed
+ mutex.unlock_shared(token);
+ }
+
+ SECTION("recursive shared locks reuse cached token")
+ {
+ ts::bravo::recursive_shared_mutex mutex;
+ ts::bravo::Token token1{0};
+ ts::bravo::Token token2{0};
+ ts::bravo::Token token3{0};
+
+ mutex.lock_shared(token1);
+ mutex.lock_shared(token2);
+ mutex.lock_shared(token3);
+
+ // All tokens should be identical (cached from first lock)
+ CHECK(token1 == token2);
+ CHECK(token2 == token3);
+
+ mutex.unlock_shared(token3);
+ mutex.unlock_shared(token2);
+ mutex.unlock_shared(token1);
+ }
+
+ SECTION("writer revocation then reader works")
+ {
+ ts::bravo::recursive_shared_mutex mutex;
+
+ // First, acquire and release a shared lock to enable read_bias
+ {
+ ts::bravo::Token token{0};
+ mutex.lock_shared(token);
+ mutex.unlock_shared(token);
+ }
+
+ // Writer acquires lock (triggers revocation)
+ mutex.lock();
+ mutex.unlock();
+
+ // Reader should still work after writer releases
+ {
+ ts::bravo::Token token{0};
+ mutex.lock_shared(token);
+ mutex.unlock_shared(token);
+ }
+ }
+
+ SECTION("multiple readers then writer then readers")
+ {
+ ts::bravo::recursive_shared_mutex mutex;
+ std::atomic<int> readers_done{0};
+
+ // Start multiple readers
+ std::thread t1{[&]() {
+ ts::bravo::Token token{0};
+ mutex.lock_shared(token);
+ std::this_thread::sleep_for(50ms);
+ mutex.unlock_shared(token);
+ ++readers_done;
+ }};
+
+ std::thread t2{[&]() {
+ ts::bravo::Token token{0};
+ mutex.lock_shared(token);
+ std::this_thread::sleep_for(50ms);
+ mutex.unlock_shared(token);
+ ++readers_done;
+ }};
+
+ // Wait for readers to finish
+ t1.join();
+ t2.join();
+ CHECK(readers_done == 2);
+
+ // Writer acquires lock
+ mutex.lock();
+ mutex.unlock();
+
+ // More readers after writer
+ std::thread t3{[&]() {
+ ts::bravo::Token token{0};
+ mutex.lock_shared(token);
+ mutex.unlock_shared(token);
+ ++readers_done;
+ }};
+
+ std::thread t4{[&]() {
+ ts::bravo::Token token{0};
+ mutex.lock_shared(token);
+ mutex.unlock_shared(token);
+ ++readers_done;
+ }};
+
+ t3.join();
+ t4.join();
+ CHECK(readers_done == 4);
+ }
+
+ SECTION("recursive shared lock with concurrent writer")
+ {
+ ts::bravo::recursive_shared_mutex mutex;
+ std::atomic<bool> writer_done{false};
+
+ // Reader thread with recursive locks
+ std::thread reader{[&]() {
+ ts::bravo::Token token1{0};
+ ts::bravo::Token token2{0};
+ mutex.lock_shared(token1);
+ mutex.lock_shared(token2); // Recursive
+ CHECK(token1 == token2); // Should be same cached token
+ std::this_thread::sleep_for(100ms);
+ mutex.unlock_shared(token2);
+ mutex.unlock_shared(token1);
+ }};
+
+ // Writer thread tries to acquire after reader starts
+ std::thread writer{[&]() {
+ std::this_thread::sleep_for(50ms);
+ mutex.lock();
+ writer_done = true;
+ mutex.unlock();
+ }};
+
+ reader.join();
+ writer.join();
+ CHECK(writer_done == true);
+ }
+}
+
+TEST_CASE("Recursive BRAVO - stress test", "[libts][BRAVO]")
+{
+ SECTION("concurrent readers with recursive locks")
+ {
+ ts::bravo::recursive_shared_mutex mutex;
+ std::atomic<int> counter{0};
+ constexpr int NUM_THREADS = 8;
+ constexpr int NUM_ITERATIONS = 1000;
+
+ std::vector<std::thread> threads;
+ for (int i = 0; i < NUM_THREADS; ++i) {
Review Comment:
This file now uses `std::vector` for thread storage but does not include <vector>. Please include <vector> explicitly to avoid build breaks from missing transitive includes.
##########
src/iocore/cache/CacheDir.cc:
##########
@@ -110,48 +112,63 @@ OpenDir::open_write(CacheVC *cont, int allow_if_writers, int max_writers)
dir_clear(&od->first_dir);
cont->od = od;
cont->write_vector = &od->vector;
- bucket[b].push(od);
+ _bucket[b].push(od);
return 1;
}
+/**
+ This event handler is called in two cases:
+
+ 1. Direct call from OpenDir::close_write - writer lock is already acquired
+ 2. Self retry through event system - need to acquire writer lock
+ */
int
-OpenDir::signal_readers(int /* event ATS_UNUSED */, Event * /* e ATS_UNUSED */)
+OpenDir::signal_readers(int event, Event * /* ATS UNUSED */)
{
- Queue<CacheVC, Link_CacheVC_opendir_link> newly_delayed_readers;
- EThread *t = mutex->thread_holding;
- CacheVC *c = nullptr;
- while ((c = delayed_readers.dequeue())) {
- CACHE_TRY_LOCK(lock, c->mutex, t);
- if (lock.is_locked()) {
- c->f.open_read_timeout = 0;
- c->handleEvent(EVENT_IMMEDIATE, nullptr);
- continue;
+ auto write_op = [&] {
+ Queue<CacheVC, Link_CacheVC_opendir_link> newly_delayed_readers;
+ EThread *t = mutex->thread_holding;
+ CacheVC *c = nullptr;
+ while ((c = _delayed_readers.dequeue())) {
+ CACHE_TRY_LOCK(lock, c->mutex, t);
+ if (lock.is_locked()) {
Review Comment:
`EThread *t = mutex->thread_holding;` can be null when signal_readers is invoked directly from close_write (OpenDir's ProxyMutex isn't held in that path). CACHE_TRY_LOCK will dereference `t` (and the lock-failure injection code uses `t->generator`), so this can crash. Use `this_ethread()` (or another guaranteed non-null thread) as a fallback.
##########
src/iocore/cache/CacheDir.cc:
##########
@@ -110,48 +112,63 @@ OpenDir::open_write(CacheVC *cont, int allow_if_writers, int max_writers)
dir_clear(&od->first_dir);
cont->od = od;
cont->write_vector = &od->vector;
- bucket[b].push(od);
+ _bucket[b].push(od);
return 1;
}
+/**
+ This event handler is called in two cases:
+
+ 1. Direct call from OpenDir::close_write - writer lock is already acquired
+ 2. Self retry through event system - need to acquire writer lock
+ */
int
-OpenDir::signal_readers(int /* event ATS_UNUSED */, Event * /* e ATS_UNUSED */)
+OpenDir::signal_readers(int event, Event * /* ATS UNUSED */)
{
- Queue<CacheVC, Link_CacheVC_opendir_link> newly_delayed_readers;
- EThread *t = mutex->thread_holding;
- CacheVC *c = nullptr;
- while ((c = delayed_readers.dequeue())) {
- CACHE_TRY_LOCK(lock, c->mutex, t);
- if (lock.is_locked()) {
- c->f.open_read_timeout = 0;
- c->handleEvent(EVENT_IMMEDIATE, nullptr);
- continue;
+ auto write_op = [&] {
+ Queue<CacheVC, Link_CacheVC_opendir_link> newly_delayed_readers;
+ EThread *t = mutex->thread_holding;
+ CacheVC *c = nullptr;
+ while ((c = _delayed_readers.dequeue())) {
+ CACHE_TRY_LOCK(lock, c->mutex, t);
+ if (lock.is_locked()) {
+ c->f.open_read_timeout = 0;
+ c->handleEvent(EVENT_IMMEDIATE, nullptr);
+ continue;
+ }
+ newly_delayed_readers.push(c);
}
- newly_delayed_readers.push(c);
- }
- if (newly_delayed_readers.head) {
- delayed_readers = newly_delayed_readers;
- EThread *t1 = newly_delayed_readers.head->mutex->thread_holding;
- if (!t1) {
- t1 = mutex->thread_holding;
+ if (newly_delayed_readers.head) {
+ _delayed_readers = newly_delayed_readers;
+ EThread *t1 = newly_delayed_readers.head->mutex->thread_holding;
+ if (!t1) {
+ t1 = mutex->thread_holding;
+ }
Review Comment:
When re-scheduling retries, `t1` can still be null (e.g., if the head VC's mutex isn't currently held and OpenDir's mutex isn't held in this call path). `t1->schedule_in(...)` will then dereference null. Ensure `t1` is always set to a valid thread (e.g., fall back to `this_ethread()`) before calling schedule_in.
```suggestion
}
if (!t1) {
t1 = this_ethread();
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Avoid Stripe Mutex lock contention for RWW [trafficserver]
Posted by "masaori335 (via GitHub)" <gi...@apache.org>.
masaori335 commented on code in PR #12794:
URL: https://github.com/apache/trafficserver/pull/12794#discussion_r3020431876
##########
src/iocore/cache/StripeSM.h:
##########
@@ -253,6 +267,24 @@ extern std::atomic<int> gnstripes;
extern ClassAllocator<OpenDirEntry, false> openDirEntryAllocator;
extern unsigned short *vol_hash_table;
+template <typename U, typename Func>
+U
+StripeSM::read_op(Func read_op) const
+{
+ ts::bravo::shared_lock lock(_shared_mutex);
+
+ return read_op();
+}
+
+template <typename U, typename Func>
+U
+StripeSM::write_op(Func write_op)
+{
+ std::lock_guard lock(_shared_mutex);
+
+ return write_op();
+}
Review Comment:
it's no problem. At least, compiler doesn't make it error.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Avoid Stripe Mutex lock contention for RWW [trafficserver]
Posted by "bryancall (via GitHub)" <gi...@apache.org>.
bryancall commented on PR #12794:
URL: https://github.com/apache/trafficserver/pull/12794#issuecomment-3873994385
@masaori335 Is there a guarantee that close_write() can't run between open_read() returning and the caller setting c->od? If the caller and writer are on different EThreads (which they can be for the same stripe), this seems like a valid race. Could open_read() return the entry while still holding the shared lock (e.g., via a lock guard the caller holds)?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Avoid Stripe Mutex lock contention for RWW [trafficserver]
Posted by "masaori335 (via GitHub)" <gi...@apache.org>.
masaori335 commented on code in PR #12794:
URL: https://github.com/apache/trafficserver/pull/12794#discussion_r2726173444
##########
src/iocore/cache/CacheDir.cc:
##########
Review Comment:
I found this call back to the CacheVC can be dead lock, when it tries to acquire lock recursively.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Avoid Stripe Mutex lock contention for RWW [trafficserver]
Posted by "masaori335 (via GitHub)" <gi...@apache.org>.
masaori335 commented on code in PR #12794:
URL: https://github.com/apache/trafficserver/pull/12794#discussion_r2730250206
##########
src/iocore/cache/CacheDir.cc:
##########
Review Comment:
I introduced `ts::bravo::recursive_shared_mutex` with new commit.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Avoid Stripe Mutex lock contention for RWW [trafficserver]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #12794:
URL: https://github.com/apache/trafficserver/pull/12794#issuecomment-5487559853
This pull request has been automatically marked as stale because it has not had recent activity. Marking it stale to flag it for further consideration by the community.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Avoid Stripe Mutex lock contention for RWW [trafficserver]
Posted by "masaori335 (via GitHub)" <gi...@apache.org>.
masaori335 commented on PR #12794:
URL: https://github.com/apache/trafficserver/pull/12794#issuecomment-4174986665
The race of `open_read()` and `close_write()` is not problem now, but it can be problem in the future change, so I made `OpenDirEntry` `RefCountObj`. Also, Copilot concerns are addressed.
I spawn another benchmark on our tool, I'll reply the result later.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Avoid Stripe Mutex lock contention for RWW [trafficserver]
Posted by "masaori335 (via GitHub)" <gi...@apache.org>.
masaori335 commented on PR #12794:
URL: https://github.com/apache/trafficserver/pull/12794#issuecomment-3803122981
Benchmark of the new commit says almost the same, 58,904 rps vs 65,339 rps.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Avoid Stripe Mutex lock contention for RWW [trafficserver]
Posted by "masaori335 (via GitHub)" <gi...@apache.org>.
masaori335 commented on code in PR #12794:
URL: https://github.com/apache/trafficserver/pull/12794#discussion_r2692404318
##########
src/iocore/cache/Cache.cc:
##########
@@ -543,20 +565,24 @@ Cache::open_read(Continuation *cont, const CacheKey *key, CacheHTTPHdr *request,
OpenDirEntry *od = nullptr;
CacheVC *c = nullptr;
+ // Read-While-Writer
+ // This OpenDirEntry lookup doesn't need stripe mutex lock because OpenDir has own reader-writer lock
+ od = stripe->open_read(key);
Review Comment:
https://github.com/apache/trafficserver/blob/a5363d2adcb76add48553127bd23f18e3d49286b/src/iocore/cache/CacheRead.cc#L212
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Avoid Stripe Mutex lock contention for RWW [trafficserver]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #12794:
URL: https://github.com/apache/trafficserver/pull/12794#discussion_r2684178684
##########
src/iocore/cache/CacheDir.cc:
##########
@@ -107,17 +112,27 @@ OpenDir::open_write(CacheVC *cont, int allow_if_writers, int max_writers)
dir_clear(&od->first_dir);
cont->od = od;
cont->write_vector = &od->vector;
- bucket[b].push(od);
+ _bucket[b].push(od);
return 1;
}
+/**
+ This event handler is called in two cases:
+
+ 1. Direct call from OpenDir::close_write - writer lock is already acquired
+ 2. Self retry through event system - need to acquire writer lock
+ */
int
-OpenDir::signal_readers(int /* event ATS_UNUSED */, Event * /* e ATS_UNUSED */)
+OpenDir::signal_readers(int event, Event * /* ATS UNUSED*/)
Review Comment:
The comment formatting uses "ATS UNUSED" with a space, but elsewhere in the codebase it's typically "ATS_UNUSED" with an underscore. Consider maintaining consistency with the existing pattern.
```suggestion
OpenDir::signal_readers(int event, Event * /* ATS_UNUSED */)
```
##########
src/iocore/cache/StripeSM.h:
##########
@@ -101,14 +101,15 @@ class StripeSM : public Continuation, public Stripe
int recover_data();
- int open_write(CacheVC *cont, int allow_if_writers, int max_writers);
- int open_write_lock(CacheVC *cont, int allow_if_writers, int max_writers);
- int close_write(CacheVC *cont);
- int begin_read(CacheVC *cont) const;
- // unused read-write interlock code
- // currently http handles a write-lock failure by retrying the read
+ // OpenDir API
+ int open_write(CacheVC *cont, int allow_if_writers, int max_writers);
+ int open_write_lock(CacheVC *cont, int allow_if_writers, int max_writers);
+ int close_write(CacheVC *cont);
OpenDirEntry *open_read(const CryptoHash *key) const;
- int close_read(CacheVC *cont) const;
+
+ // PreservationTable API
+ int begin_read(CacheVC *cont) const;
+ int close_read(CacheVC *cont) const;
Review Comment:
Inconsistent formatting: The comment "// OpenDir API" and "// PreservationTable API" use double slashes, but there are also methods without such grouping comments. Consider whether this level of API grouping is helpful, and if so, ensure it's applied consistently throughout the class. For example, other method groups could benefit from similar API comments.
##########
src/iocore/cache/Cache.cc:
##########
@@ -543,20 +565,24 @@ Cache::open_read(Continuation *cont, const CacheKey *key, CacheHTTPHdr *request,
OpenDirEntry *od = nullptr;
CacheVC *c = nullptr;
+ // Read-While-Writer
+ // This OpenDirEntry lookup doesn't need stripe mutex lock because OpenDir has own reader-writer lock
+ od = stripe->open_read(key);
Review Comment:
Potential lock ordering issue: This code calls stripe->open_read() (which now acquires _shared_mutex as a reader) while holding stripe->mutex. In other places like CacheRead.cc line 212, the same pattern exists. However, in Cache.cc line 570, open_read() is called without holding stripe->mutex. While this change is intentional to reduce contention, ensure that this mixed lock ordering (sometimes stripe->mutex then _shared_mutex, sometimes just _shared_mutex) doesn't introduce potential deadlock scenarios. The Bravo reader-writer lock should handle this safely since multiple readers can coexist, but this should be verified through testing under high concurrency.
##########
src/iocore/cache/P_CacheDir.h:
##########
@@ -225,16 +226,23 @@ struct OpenDirEntry {
}
};
-struct OpenDir : public Continuation {
- Queue<CacheVC, Link_CacheVC_opendir_link> delayed_readers;
- DLL<OpenDirEntry> bucket[OPEN_DIR_BUCKETS];
+class OpenDir : public Continuation
+{
+public:
+ OpenDir();
int open_write(CacheVC *c, int allow_if_writers, int max_writers);
int close_write(CacheVC *c);
OpenDirEntry *open_read(const CryptoHash *key) const;
- int signal_readers(int event, Event *e);
- OpenDir();
+ // event handler
+ int signal_readers(int event, Event *e);
+
+private:
+ mutable ts::bravo::shared_mutex _shared_mutex;
+
+ Queue<CacheVC, Link_CacheVC_opendir_link> _delayed_readers;
+ DLL<OpenDirEntry> _bucket[OPEN_DIR_BUCKETS];
};
Review Comment:
Missing documentation: The class documentation should be updated to explain the new locking mechanism. Specifically, document that OpenDir now uses its own reader-writer lock (_shared_mutex) instead of relying on the StripeSM mutex, which allows concurrent reads while writes are in progress. This is a significant architectural change that should be clearly documented for maintainability.
##########
src/iocore/cache/CacheDir.cc:
##########
@@ -46,6 +49,7 @@ DbgCtl dbg_ctl_dir_clean{"dir_clean"};
#ifdef DEBUG
DbgCtl dbg_ctl_cache_stats{"cache_stats"};
+DbgCtl dbg_ctl_cache_open_dir{"cache_open_dir"};
Review Comment:
Unused debug control: The debug control variable dbg_ctl_cache_open_dir is defined but never used in the code. If this was added for future debugging purposes, consider adding a comment explaining its intended use. Otherwise, it should be removed to avoid clutter.
```suggestion
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Avoid Stripe Mutex lock contention for RWW [trafficserver]
Posted by "masaori335 (via GitHub)" <gi...@apache.org>.
masaori335 commented on PR #12794:
URL: https://github.com/apache/trafficserver/pull/12794#issuecomment-4181069262
The benchmark result was almost the same as before 58,098.5 vs 65,882.5 req/s.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Avoid Stripe Mutex lock contention for RWW [trafficserver]
Posted by "masaori335 (via GitHub)" <gi...@apache.org>.
masaori335 commented on code in PR #12794:
URL: https://github.com/apache/trafficserver/pull/12794#discussion_r2692399883
##########
src/iocore/cache/Cache.cc:
##########
@@ -543,20 +565,24 @@ Cache::open_read(Continuation *cont, const CacheKey *key, CacheHTTPHdr *request,
OpenDirEntry *od = nullptr;
CacheVC *c = nullptr;
+ // Read-While-Writer
+ // This OpenDirEntry lookup doesn't need stripe mutex lock because OpenDir has own reader-writer lock
+ od = stripe->open_read(key);
Review Comment:
I guess this is fine, because prior to the change, when we jump to the `CacheVC::openReadFromWriter` from the `Lwriter` label, it's out of scope of StripeSM mutex lock. `CacheVC::openReadFromWriter` has another check of OpenDir.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Avoid Stripe Mutex lock contention for RWW [trafficserver]
Posted by "masaori335 (via GitHub)" <gi...@apache.org>.
masaori335 commented on code in PR #12794:
URL: https://github.com/apache/trafficserver/pull/12794#discussion_r2692409704
##########
src/iocore/cache/Cache.cc:
##########
@@ -543,20 +565,24 @@ Cache::open_read(Continuation *cont, const CacheKey *key, CacheHTTPHdr *request,
OpenDirEntry *od = nullptr;
CacheVC *c = nullptr;
+ // Read-While-Writer
+ // This OpenDirEntry lookup doesn't need stripe mutex lock because OpenDir has own reader-writer lock
+ od = stripe->open_read(key);
Review Comment:
Ah, the reader lock of OpenDir might be need to be hold while this OpenDirEntry is used in the `CacheVC::openReadFromWriter`. Let me consider in case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Avoid Stripe Mutex lock contention for RWW [trafficserver]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #12794: Avoid Stripe Mutex lock contention for RWW
URL: https://github.com/apache/trafficserver/pull/12794
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org