[scudo] Add ConditionVariable in SizeClassAllocator64 #69031
Merged
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
This may improve the waiting of `Region->MMLock` while trying to refill the freelist. Instead of always waiting on the completion of `populateFreeListAndPopBatch()` or `releaseToOSMaybe()`, `pushBlocks()` also refills the freelist. This increases the chance of earlier return from `popBatches()`. The support of condition variable hasn't been done for all platforms. Therefore, add another `popBatchWithCV()` and it can be configured in the allocator configuration by setting `Primary::UseConditionVariable` and the desired `ConditionVariableT`. Reviewed By: cferris Differential Revision: https://reviews.llvm.org/D156146
llvmbot
added
compiler-rt
compiler-rt:scudo
|
This is copied from https://reviews.llvm.org/D156146 and rebased |
|
@llvm/pr-subscribers-compiler-rt-sanitizer Author: None (ChiaHungDuan) ChangesThis may improve the waiting of The support of condition variable hasn't been done for all platforms. Therefore, add another Reviewed By: cferris Differential Revision: https://reviews.llvm.org/D156146 Patch is 25.79 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/69031.diff 11 Files Affected:
diff --git a/compiler-rt/lib/scudo/standalone/CMakeLists.txt b/compiler-rt/lib/scudo/standalone/CMakeLists.txt
index ba699f6a67c670a..60092005cc33bbf 100644
--- a/compiler-rt/lib/scudo/standalone/CMakeLists.txt
+++ b/compiler-rt/lib/scudo/standalone/CMakeLists.txt
@@ -62,6 +62,9 @@ set(SCUDO_HEADERS
bytemap.h
checksum.h
chunk.h
+ condition_variable.h
+ condition_variable_base.h
+ condition_variable_linux.h
combined.h
common.h
flags_parser.h
@@ -104,6 +107,7 @@ set(SCUDO_HEADERS
set(SCUDO_SOURCES
checksum.cpp
common.cpp
+ condition_variable_linux.cpp
crc32_hw.cpp
flags_parser.cpp
flags.cpp
diff --git a/compiler-rt/lib/scudo/standalone/allocator_config.h b/compiler-rt/lib/scudo/standalone/allocator_config.h
index 44c1ac5f74a2f2f..3c6aa3acb0e45b4 100644
--- a/compiler-rt/lib/scudo/standalone/allocator_config.h
+++ b/compiler-rt/lib/scudo/standalone/allocator_config.h
@@ -11,6 +11,7 @@
#include "combined.h"
#include "common.h"
+#include "condition_variable.h"
#include "flags.h"
#include "primary32.h"
#include "primary64.h"
@@ -82,6 +83,14 @@ namespace scudo {
// // Defines the minimal & maximal release interval that can be set.
// static const s32 MinReleaseToOsIntervalMs = INT32_MIN;
// static const s32 MaxReleaseToOsIntervalMs = INT32_MAX;
+//
+// // Use condition variable to shorten the waiting time of refillment of
+// // freelist. Note that this depends on the implementation of condition
+// // variable on each platform and the performance may vary so that it
+// // doesn't guarantee a performance benefit.
+// // Note that both variables have to be defined to enable it.
+// static const bool UseConditionVariable = true;
+// using ConditionVariableT = ConditionVariableLinux;
// };
// // Defines the type of Primary allocator to use.
// template <typename Config> using PrimaryT = SizeClassAllocator64<Config>;
diff --git a/compiler-rt/lib/scudo/standalone/condition_variable.h b/compiler-rt/lib/scudo/standalone/condition_variable.h
new file mode 100644
index 000000000000000..549f6e9f787bad9
--- /dev/null
+++ b/compiler-rt/lib/scudo/standalone/condition_variable.h
@@ -0,0 +1,60 @@
+//===-- condition_variable.h ------------------------------------*- 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 SCUDO_CONDITION_VARIABLE_H_
+#define SCUDO_CONDITION_VARIABLE_H_
+
+#include "condition_variable_base.h"
+
+#include "common.h"
+#include "platform.h"
+
+#include "condition_variable_linux.h"
+
+namespace scudo {
+
+// A default implementation of default condition variable. It doesn't do a real
+// `wait`, instead it spins a short amount of time only.
+class ConditionVariableDummy
+ : public ConditionVariableBase<ConditionVariableDummy> {
+public:
+ void notifyAllImpl(UNUSED HybridMutex &M) REQUIRES(M) {}
+
+ void waitImpl(UNUSED HybridMutex &M) REQUIRES(M) {
+ M.unlock();
+
+ constexpr u32 SpinTimes = 64;
+ volatile u32 V = 0;
+ for (u32 I = 0; I < SpinTimes; ++I) {
+ u32 Tmp = V + 1;
+ V = Tmp;
+ }
+
+ M.lock();
+ }
+};
+
+template <typename Config, typename = const bool>
+struct ConditionVariableState {
+ static constexpr bool enabled() { return false; }
+ // This is only used for compilation purpose so that we won't end up having
+ // many conditional compilations. If you want to use `ConditionVariableDummy`,
+ // define `ConditionVariableT` in your allocator configuration. See
+ // allocator_config.h for more details.
+ using ConditionVariableT = ConditionVariableDummy;
+};
+
+template <typename Config>
+struct ConditionVariableState<Config, decltype(Config::UseConditionVariable)> {
+ static constexpr bool enabled() { return true; }
+ using ConditionVariableT = typename Config::ConditionVariableT;
+};
+
+} // namespace scudo
+
+#endif // SCUDO_CONDITION_VARIABLE_H_
diff --git a/compiler-rt/lib/scudo/standalone/condition_variable_base.h b/compiler-rt/lib/scudo/standalone/condition_variable_base.h
new file mode 100644
index 000000000000000..e200e82d488f446
--- /dev/null
+++ b/compiler-rt/lib/scudo/standalone/condition_variable_base.h
@@ -0,0 +1,57 @@
+//===-- condition_variable_base.h ------------------------------------*- 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 SCUDO_CONDITION_VARIABLE_BASE_H_
+#define SCUDO_CONDITION_VARIABLE_BASE_H_
+
+#include "mutex.h"
+#include "thread_annotations.h"
+
+namespace scudo {
+
+template <typename Derived> class ConditionVariableBase {
+public:
+ constexpr ConditionVariableBase() = default;
+
+ void bindTestOnly(HybridMutex &Mutex) {
+#if SCUDO_DEBUG
+ boundMutex = &Mutex;
+#else
+ (void)Mutex;
+#endif
+ }
+
+ void notifyAll(HybridMutex &M) REQUIRES(M) {
+#if SCUDO_DEBUG
+ CHECK_EQ(&M, boundMutex);
+#endif
+ getDerived()->notifyAllImpl(M);
+ }
+
+ void wait(HybridMutex &M) REQUIRES(M) {
+#if SCUDO_DEBUG
+ CHECK_EQ(&M, boundMutex);
+#endif
+ getDerived()->waitImpl(M);
+ }
+
+protected:
+ Derived *getDerived() { return static_cast<Derived *>(this); }
+
+#if SCUDO_DEBUG
+ // Because thread-safety analysis doesn't support pointer aliasing, we are not
+ // able to mark the proper annotations without false positive. Instead, we
+ // pass the lock and do the same-lock check separately.
+ HybridMutex *boundMutex = nullptr;
+#endif
+};
+
+} // namespace scudo
+
+#endif // SCUDO_CONDITION_VARIABLE_BASE_H_
diff --git a/compiler-rt/lib/scudo/standalone/condition_variable_linux.cpp b/compiler-rt/lib/scudo/standalone/condition_variable_linux.cpp
new file mode 100644
index 000000000000000..5c14b8d562f0f3c
--- /dev/null
+++ b/compiler-rt/lib/scudo/standalone/condition_variable_linux.cpp
@@ -0,0 +1,52 @@
+//===-- condition_variable_linux.cpp ----------------------------*- 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 "platform.h"
+
+#if SCUDO_LINUX
+
+#include "condition_variable_linux.h"
+
+#include "atomic_helpers.h"
+
+#include <limits.h>
+#include <linux/futex.h>
+#include <sys/syscall.h>
+#include <unistd.h>
+
+namespace scudo {
+
+void ConditionVariableLinux::notifyAllImpl(UNUSED HybridMutex &M) {
+ const u32 V = atomic_load_relaxed(&Counter) + 1;
+ atomic_store_relaxed(&Counter, V);
+
+ // TODO(chiahungduan): Move the waiters from the futex waiting queue
+ // `Counter` to futex waiting queue `M` so that the awoken threads won't be
+ // blocked again due to locked `M` by current thread.
+ if (LastNotifyAll + 1 != V) {
+ syscall(SYS_futex, reinterpret_cast<uptr>(&Counter), FUTEX_WAKE_PRIVATE,
+ INT_MAX, nullptr, nullptr, 0);
+ }
+
+ LastNotifyAll = V;
+}
+
+void ConditionVariableLinux::waitImpl(HybridMutex &M) {
+ const u32 V = atomic_load_relaxed(&Counter) + 1;
+ atomic_store_relaxed(&Counter, V);
+
+ // TODO: Use ScopedUnlock when it's supported.
+ M.unlock();
+ syscall(SYS_futex, reinterpret_cast<uptr>(&Counter), FUTEX_WAIT_PRIVATE, V,
+ nullptr, nullptr, 0);
+ M.lock();
+}
+
+} // namespace scudo
+
+#endif // SCUDO_LINUX
diff --git a/compiler-rt/lib/scudo/standalone/condition_variable_linux.h b/compiler-rt/lib/scudo/standalone/condition_variable_linux.h
new file mode 100644
index 000000000000000..cd073287326d9be
--- /dev/null
+++ b/compiler-rt/lib/scudo/standalone/condition_variable_linux.h
@@ -0,0 +1,38 @@
+//===-- condition_variable_linux.h ------------------------------*- 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 SCUDO_CONDITION_VARIABLE_LINUX_H_
+#define SCUDO_CONDITION_VARIABLE_LINUX_H_
+
+#include "platform.h"
+
+#if SCUDO_LINUX
+
+#include "atomic_helpers.h"
+#include "condition_variable_base.h"
+#include "thread_annotations.h"
+
+namespace scudo {
+
+class ConditionVariableLinux
+ : public ConditionVariableBase<ConditionVariableLinux> {
+public:
+ void notifyAllImpl(HybridMutex &M) REQUIRES(M);
+
+ void waitImpl(HybridMutex &M) REQUIRES(M);
+
+private:
+ u32 LastNotifyAll = 0;
+ atomic_u32 Counter = {};
+};
+
+} // namespace scudo
+
+#endif // SCUDO_LINUX
+
+#endif // SCUDO_CONDITION_VARIABLE_LINUX_H_
diff --git a/compiler-rt/lib/scudo/standalone/primary64.h b/compiler-rt/lib/scudo/standalone/primary64.h
index 6e5ab7e3ba7965d..0f4ba88ee1f4b9d 100644
--- a/compiler-rt/lib/scudo/standalone/primary64.h
+++ b/compiler-rt/lib/scudo/standalone/primary64.h
@@ -22,6 +22,8 @@
#include "string_utils.h"
#include "thread_annotations.h"
+#include "condition_variable.h"
+
namespace scudo {
// SizeClassAllocator64 is an allocator tuned for 64-bit address space.
@@ -48,6 +50,8 @@ template <typename Config> class SizeClassAllocator64 {
public:
typedef typename Config::Primary::CompactPtrT CompactPtrT;
typedef typename Config::Primary::SizeClassMap SizeClassMap;
+ typedef typename ConditionVariableState<
+ typename Config::Primary>::ConditionVariableT ConditionVariableT;
static const uptr CompactPtrScale = Config::Primary::CompactPtrScale;
static const uptr RegionSizeLog = Config::Primary::RegionSizeLog;
static const uptr GroupSizeLog = Config::Primary::GroupSizeLog;
@@ -70,6 +74,10 @@ template <typename Config> class SizeClassAllocator64 {
static bool canAllocate(uptr Size) { return Size <= SizeClassMap::MaxSize; }
+ static bool conditionVariableEnabled() {
+ return ConditionVariableState<typename Config::Primary>::enabled();
+ }
+
void init(s32 ReleaseToOsInterval) NO_THREAD_SAFETY_ANALYSIS {
DCHECK(isAligned(reinterpret_cast<uptr>(this), alignof(ThisT)));
@@ -124,6 +132,7 @@ template <typename Config> class SizeClassAllocator64 {
for (uptr I = 0; I < NumClasses; I++) {
RegionInfo *Region = getRegionInfo(I);
+
// The actual start of a region is offset by a random number of pages
// when PrimaryEnableRandomOffset is set.
Region->RegionBeg = (PrimaryBase + (I << RegionSizeLog)) +
@@ -145,6 +154,11 @@ template <typename Config> class SizeClassAllocator64 {
}
shuffle(RegionInfoArray, NumClasses, &Seed);
+ // The binding should be done after region shuffling so that it won't bind
+ // the FLLock from the wrong region.
+ for (uptr I = 0; I < NumClasses; I++)
+ getRegionInfo(I)->FLLockCV.bindTestOnly(getRegionInfo(I)->FLLock);
+
setOption(Option::ReleaseInterval, static_cast<sptr>(ReleaseToOsInterval));
}
@@ -236,26 +250,26 @@ template <typename Config> class SizeClassAllocator64 {
bool ReportRegionExhausted = false;
TransferBatchT *B = nullptr;
- while (true) {
- // When two threads compete for `Region->MMLock`, we only want one of them
- // does the populateFreeListAndPopBatch(). To avoid both of them doing
- // that, always check the freelist before mapping new pages.
- //
- // TODO(chiahungduan): Use a condition variable so that we don't need to
- // hold `Region->MMLock` here.
- ScopedLock ML(Region->MMLock);
- {
- ScopedLock FL(Region->FLLock);
- B = popBatchImpl(C, ClassId, Region);
- if (LIKELY(B))
- return B;
- }
+ if (conditionVariableEnabled()) {
+ B = popBatchWithCV(C, ClassId, Region, ReportRegionExhausted);
+ } else {
+ while (true) {
+ // When two threads compete for `Region->MMLock`, we only want one of
+ // them to call populateFreeListAndPopBatch(). To avoid both of them
+ // doing that, always check the freelist before mapping new pages.
+ ScopedLock ML(Region->MMLock);
+ {
+ ScopedLock FL(Region->FLLock);
+ if ((B = popBatchImpl(C, ClassId, Region)))
+ break;
+ }
- const bool RegionIsExhausted = Region->Exhausted;
- if (!RegionIsExhausted)
- B = populateFreeListAndPopBatch(C, ClassId, Region);
- ReportRegionExhausted = !RegionIsExhausted && Region->Exhausted;
- break;
+ const bool RegionIsExhausted = Region->Exhausted;
+ if (!RegionIsExhausted)
+ B = populateFreeListAndPopBatch(C, ClassId, Region);
+ ReportRegionExhausted = !RegionIsExhausted && Region->Exhausted;
+ break;
+ }
}
if (UNLIKELY(ReportRegionExhausted)) {
@@ -280,6 +294,8 @@ template <typename Config> class SizeClassAllocator64 {
if (ClassId == SizeClassMap::BatchClassId) {
ScopedLock L(Region->FLLock);
pushBatchClassBlocks(Region, Array, Size);
+ if (conditionVariableEnabled())
+ Region->FLLockCV.notifyAll(Region->FLLock);
return;
}
@@ -306,6 +322,8 @@ template <typename Config> class SizeClassAllocator64 {
{
ScopedLock L(Region->FLLock);
pushBlocksImpl(C, ClassId, Region, Array, Size, SameGroup);
+ if (conditionVariableEnabled())
+ Region->FLLockCV.notifyAll(Region->FLLock);
}
}
@@ -538,6 +556,7 @@ template <typename Config> class SizeClassAllocator64 {
struct UnpaddedRegionInfo {
// Mutex for operations on freelist
HybridMutex FLLock;
+ ConditionVariableT FLLockCV GUARDED_BY(FLLock);
// Mutex for memmap operations
HybridMutex MMLock ACQUIRED_BEFORE(FLLock);
// `RegionBeg` is initialized before thread creation and won't be changed.
@@ -549,6 +568,7 @@ template <typename Config> class SizeClassAllocator64 {
uptr TryReleaseThreshold GUARDED_BY(MMLock) = 0;
ReleaseToOsInfo ReleaseInfo GUARDED_BY(MMLock) = {};
bool Exhausted GUARDED_BY(MMLock) = false;
+ bool isPopulatingFreeList GUARDED_BY(FLLock) = false;
};
struct RegionInfo : UnpaddedRegionInfo {
char Padding[SCUDO_CACHE_LINE_SIZE -
@@ -831,6 +851,76 @@ template <typename Config> class SizeClassAllocator64 {
InsertBlocks(Cur, Array + Size - Count, Count);
}
+ TransferBatchT *popBatchWithCV(CacheT *C, uptr ClassId, RegionInfo *Region,
+ bool &ReportRegionExhausted) {
+ TransferBatchT *B = nullptr;
+
+ while (true) {
+ // We only expect one thread doing the freelist refillment and other
+ // threads will be waiting for either the completion of the
+ // `populateFreeListAndPopBatch()` or `pushBlocks()` called by other
+ // threads.
+ bool PopulateFreeList = false;
+ {
+ ScopedLock FL(Region->FLLock);
+ if (!Region->isPopulatingFreeList) {
+ Region->isPopulatingFreeList = true;
+ PopulateFreeList = true;
+ }
+ }
+
+ if (PopulateFreeList) {
+ ScopedLock ML(Region->MMLock);
+
+ const bool RegionIsExhausted = Region->Exhausted;
+ if (!RegionIsExhausted)
+ B = populateFreeListAndPopBatch(C, ClassId, Region);
+ ReportRegionExhausted = !RegionIsExhausted && Region->Exhausted;
+
+ {
+ // Before reacquiring the `FLLock`, the freelist may be used up again
+ // and some threads are waiting for the freelist refillment by the
+ // current thread. It's important to set
+ // `Region->isPopulatingFreeList` to false so the threads about to
+ // sleep will notice the status change.
+ ScopedLock FL(Region->FLLock);
+ Region->isPopulatingFreeList = false;
+ Region->FLLockCV.notifyAll(Region->FLLock);
+ }
+
+ break;
+ }
+
+ // At here, there are two preconditions to be met before waiting,
+ // 1. The freelist is empty.
+ // 2. Region->isPopulatingFreeList == true, i.e, someone is still doing
+ // `populateFreeListAndPopBatch()`.
+ //
+ // Note that it has the chance that freelist is empty but
+ // Region->isPopulatingFreeList == false because all the new populated
+ // blocks were used up right after the refillment. Therefore, we have to
+ // check if someone is still populating the freelist.
+ ScopedLock FL(Region->FLLock);
+ if (LIKELY(B = popBatchImpl(C, ClassId, Region)))
+ break;
+
+ if (!Region->isPopulatingFreeList)
+ continue;
+
+ // Now the freelist is empty and someone's doing the refillment. We will
+ // wait until anyone refills the freelist or someone finishes doing
+ // `populateFreeListAndPopBatch()`. The refillment can be done by
+ // `populateFreeListAndPopBatch()`, `pushBlocks()`,
+ // `pushBatchClassBlocks()` and `mergeGroupsToReleaseBack()`.
+ Region->FLLockCV.wait(Region->FLLock);
+
+ if (LIKELY(B = popBatchImpl(C, ClassId, Region)))
+ break;
+ }
+
+ return B;
+ }
+
// Pop one TransferBatch from a BatchGroup. The BatchGroup with the smallest
// group id will be considered first.
//
@@ -1521,6 +1611,8 @@ template <typename Config> class SizeClassAllocator64 {
if (UNLIKELY(Idx + NeededSlots > MaxUnusedSize)) {
ScopedLock L(BatchClassRegion->FLLock);
pushBatchClassBlocks(BatchClassRegion, Blocks, Idx);
+ if (conditionVariableEnabled())
+ BatchClassRegion->FLLockCV.notifyAll(BatchClassRegion->FLLock);
Idx = 0;
}
Blocks[Idx++] =
@@ -1556,6 +1648,8 @@ template <typename Config> class SizeClassAllocator64 {
if (Idx != 0) {
ScopedLock L(BatchClassRegion->FLLock);
pushBatchClassBlocks(BatchClassRegion, Blocks, Idx);
+ if (conditionVariableEnabled())
+ BatchClassRegion->FLLockCV.notifyAll(BatchClassRegion->FLLock);
}
if (SCUDO_DEBUG) {
@@ -1565,6 +1659,9 @@ template <typename Config> class SizeClassAllocator64 {
CHECK_LT(Prev->CompactPtrGroupBase, Cur->CompactPtrGroupBase);
}
}
+
+ if (conditionVariableEnabled())
+ Region->FLLockCV.notifyAll(Region->FLLock);
}
// TODO: `PrimaryBase` can be obtained from ReservedMemory. This needs to be
diff --git a/compiler-rt/lib/scudo/standalone/tests/CMakeLists.txt b/compiler-rt/lib/scudo/standalone/tests/CMakeLists.txt
index a4a031d54d7c3ad..c6b6a1cb57ceea6 100644
--- a/compiler-rt/lib/scudo/standalone/tests/CMakeLists.txt
+++ b/compiler-rt/lib/scudo/standalone/tests/CMakeLists.txt
@@ -96,6 +96,7 @@ set(SCUDO_UNIT_TEST_SOURCES
chunk_test.cpp
combined_test.cpp
common_test.cpp
+ condition_variable_test.cpp
flags_test.cpp
list_test.cpp
map_test.cpp
diff --git a/compiler-rt/lib/scudo/standalone/tests/combined_test.cpp b/compiler-rt/lib/scudo/standalone/tests/combined_test.cpp
index 7958e9dabf60db2..3dbd93cacefd684 100644
--- a/compiler-rt/lib/scudo/standalone/tests/combined_test.cpp
+++ b/compiler-rt/lib/scudo/standalone/tests/combined_test.cpp
@@ -12,7 +12,9 @@
#include "allocator_config.h"
#include "chunk.h"
#include "combined.h"
+#include "condition_variable.h"
#include "mem_map.h"
+#include "size_class_map.h"
#include <algorithm>
#include <condition_variable>
@@ -164,13 +166,60 @@ template <class TypeParam> struct ScudoCombinedTest : public Test {
template <typename T> using ScudoCombinedDeathTest = ScudoCombinedTest<T>;
+namespace scudo {
+struct TestConditionVariableConfig {
+ static const bool MaySupportMemoryTagging = true;
+ template <class A>
+ using TSDRegistryT =
+ scudo::TSDRegistrySharedT<A, 8U, 4U>; // Shared, max 8 TSDs.
+
+ struct Primary {
+ using SizeClassMap = scudo::AndroidSizeClassMap;
+#if SCUDO_CAN_USE_PRIMARY64
+ static const scudo::uptr RegionSizeLog = 28U;
+ typedef scudo::u32 CompactPtrT;
+ static const scudo::uptr CompactPtrScale = SCUDO_MIN_ALIGNMENT_LOG;
+ static const scudo::uptr GroupSizeLog = 20U;
+ static const bool EnableRandomOffset = true;
+ static const scudo::uptr MapSizeI...
[truncated]
|
|
It has the last comment which is waiting for Hans input |
hboehm
reviewed
Oct 13, 2023
| namespace scudo { | ||
|
|
||
| void ConditionVariableLinux::notifyAllImpl(UNUSED HybridMutex &M) { | ||
| const u32 V = atomic_load_relaxed(&Counter) + 1; |
There was a problem hiding this comment.
I withdraw my earlier comment about repeated WAKE calls. I had misread the code. I don't see any correctness issues.
To me, the code would be a bit clearer if the above line were (modulo style rules):
const u32 PrevCounter = atomic_load_relaxed(&Counter);
without the +1, but all uses of V below adjusted to make the code correct again. The test would just be LastNotifyAll != PrevCounter.
For consistency, I would do the same in waitImpl.
There was a problem hiding this comment.
Nice suggestion! Thanks
|
Suggested by Hans, also updated the test to only call notifyAll() after the counter increment so that we will catch the case which loses wake up. |
cferris1000
reviewed
Oct 19, 2023
cferris1000
approved these changes
Oct 19, 2023
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 may improve the waiting of
Region->MMLockwhile trying to refill the freelist. Instead of always waiting on the completion ofpopulateFreeListAndPopBatch()orreleaseToOSMaybe(),pushBlocks()also refills the freelist. This increases the chance of earlier return frompopBatches().The support of condition variable hasn't been done for all platforms. Therefore, add another
popBatchWithCV()and it can be configured in the allocator configuration by settingPrimary::UseConditionVariableand the desiredConditionVariableT.Reviewed By: cferris
Differential Revision: https://reviews.llvm.org/D156146