From d70f891a828af394d3e2108b66fee08fcc0957da Mon Sep 17 00:00:00 2001 From: Alexander Shabalin Date: Mon, 31 Jul 2023 14:03:58 +0200 Subject: [PATCH] [K/N] Address AtomicStack tsan reports. --- .../src/custom_alloc/cpp/AtomicStack.hpp | 21 ++++--- .../src/custom_alloc/cpp/AtomicStackTest.cpp | 59 +++++++++++++++++++ .../src/custom_alloc/cpp/ExtraObjectPage.cpp | 21 +++---- .../src/custom_alloc/cpp/ExtraObjectPage.hpp | 6 +- .../src/custom_alloc/cpp/FixedBlockPage.hpp | 2 +- .../src/custom_alloc/cpp/NextFitPage.hpp | 2 +- .../src/custom_alloc/cpp/SingleObjectPage.hpp | 2 +- 7 files changed, 90 insertions(+), 23 deletions(-) create mode 100644 kotlin-native/runtime/src/custom_alloc/cpp/AtomicStackTest.cpp diff --git a/kotlin-native/runtime/src/custom_alloc/cpp/AtomicStack.hpp b/kotlin-native/runtime/src/custom_alloc/cpp/AtomicStack.hpp index ac107f85971..6523709e73e 100644 --- a/kotlin-native/runtime/src/custom_alloc/cpp/AtomicStack.hpp +++ b/kotlin-native/runtime/src/custom_alloc/cpp/AtomicStack.hpp @@ -35,14 +35,21 @@ public: // freeing pages during STW. T* Pop() noexcept { T* elm = stack_.load(std::memory_order_acquire); - while (elm && !stack_.compare_exchange_weak(elm, elm->next_, std::memory_order_acq_rel)) {} - return elm; + while (true) { + if (!elm) { + return nullptr; + } + auto* elmNext = elm->next_.load(std::memory_order_relaxed); + if (stack_.compare_exchange_weak(elm, elmNext, std::memory_order_acq_rel)) { + return elm; + } + } } void Push(T* elm) noexcept { T* head = nullptr; do { - elm->next_ = head; + elm->next_.store(head, std::memory_order_relaxed); } while (!stack_.compare_exchange_weak(head, elm, std::memory_order_acq_rel)); } @@ -58,14 +65,14 @@ public: } // `this` stack is not empty. Find the tail of `other`. If no deletions are performed, this is safe. T* otherTail = otherHead; - while (otherTail->next_) otherTail = otherTail->next_; + while (auto* next = otherTail->next_.load(std::memory_order_relaxed)) otherTail = next; // can't be because of the loop above - RuntimeAssert(otherTail->next_ == nullptr, "otherTail->next_ must be a tail"); + RuntimeAssert(otherTail->next_.load(std::memory_order_relaxed) == nullptr, "otherTail->next_ must be a tail"); // Now make `otherTail->next_` point to the current head of `this` and // simultaneously make `otherHead` the new current head. do { - otherTail->next_ = thisHead; - } while (!stack_.compare_exchange_weak(thisHead, otherHead, std::memory_order_acq_rel)); + otherTail->next_.store(thisHead, std::memory_order_relaxed); + } while (!stack_.compare_exchange_weak(thisHead, otherHead, std::memory_order_release, std::memory_order_relaxed)); } bool isEmpty() const noexcept { return stack_.load(std::memory_order_relaxed) == nullptr; } diff --git a/kotlin-native/runtime/src/custom_alloc/cpp/AtomicStackTest.cpp b/kotlin-native/runtime/src/custom_alloc/cpp/AtomicStackTest.cpp new file mode 100644 index 00000000000..5169c784b88 --- /dev/null +++ b/kotlin-native/runtime/src/custom_alloc/cpp/AtomicStackTest.cpp @@ -0,0 +1,59 @@ +/* + * Copyright 2010-2023 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license + * that can be found in the LICENSE file. + */ + +#include "AtomicStack.hpp" + +#include + +#include "TestSupport.hpp" +#include "gmock/gmock.h" +#include "gtest/gtest.h" + +#include "ScopedThread.hpp" + +using namespace kotlin; + +namespace { + +struct Element { + std::atomic next_ = nullptr; +}; + +} // namespace + +TEST(AtomicStackTest, StressPushPop) { + alloc::AtomicStack ready; + alloc::AtomicStack used; + std_support::vector elements(1000); + std_support::vector expected; + for (auto& element : elements) { + ready.Push(&element); + expected.push_back(&element); + } + + std::atomic canStart = false; + std_support::vector mutators; + for (int i = 0; i < kDefaultThreadCount; ++i) { + mutators.emplace_back([&]() NO_INLINE { + while (!canStart.load(std::memory_order_relaxed)) { + } + while (auto* element = ready.Pop()) { + used.Push(element); + } + }); + } + + canStart.store(true, std::memory_order_relaxed); + mutators.clear(); + + std_support::vector actual; + while (auto* element = ready.Pop()) { + actual.push_back(element); + } + while (auto* element = used.Pop()) { + actual.push_back(element); + } + EXPECT_THAT(actual, testing::UnorderedElementsAreArray(expected)); +} diff --git a/kotlin-native/runtime/src/custom_alloc/cpp/ExtraObjectPage.cpp b/kotlin-native/runtime/src/custom_alloc/cpp/ExtraObjectPage.cpp index e01ffa12df2..4f04ebad690 100644 --- a/kotlin-native/runtime/src/custom_alloc/cpp/ExtraObjectPage.cpp +++ b/kotlin-native/runtime/src/custom_alloc/cpp/ExtraObjectPage.cpp @@ -25,10 +25,10 @@ ExtraObjectPage* ExtraObjectPage::Create(uint32_t ignored) noexcept { ExtraObjectPage::ExtraObjectPage() noexcept { CustomAllocInfo("ExtraObjectPage(%p)::ExtraObjectPage()", this); - nextFree_ = cells_; + nextFree_.store(cells_, std::memory_order_relaxed); ExtraObjectCell* end = cells_ + EXTRA_OBJECT_COUNT; - for (ExtraObjectCell* cell = cells_; cell < end; cell = cell->next_) { - cell->next_ = cell + 1; + for (ExtraObjectCell* cell = cells_; cell < end; cell = cell->next_.load(std::memory_order_relaxed)) { + cell->next_.store(cell + 1, std::memory_order_relaxed); } } @@ -37,11 +37,12 @@ void ExtraObjectPage::Destroy() noexcept { } mm::ExtraObjectData* ExtraObjectPage::TryAllocate() noexcept { - if (nextFree_ >= cells_ + EXTRA_OBJECT_COUNT) { + auto* next = nextFree_.load(std::memory_order_relaxed); + if (next >= cells_ + EXTRA_OBJECT_COUNT) { return nullptr; } - ExtraObjectCell* freeBlock = nextFree_; - nextFree_ = freeBlock->next_; + ExtraObjectCell* freeBlock = next; + nextFree_.store(freeBlock->next_.load(std::memory_order_relaxed), std::memory_order_relaxed); CustomAllocDebug("ExtraObjectPage(%p)::TryAllocate() = %p", this, freeBlock->Data()); return freeBlock->Data(); } @@ -52,10 +53,10 @@ bool ExtraObjectPage::Sweep(GCSweepScope& sweepHandle, FinalizerQueue& finalizer // necessarily match an actual block starting point. ExtraObjectCell* end = cells_ + EXTRA_OBJECT_COUNT; bool alive = false; - ExtraObjectCell** nextFree = &nextFree_; + std::atomic* nextFree = &nextFree_; for (ExtraObjectCell* cell = cells_; cell < end; ++cell) { // If the current cell is free, move on. - if (cell == *nextFree) { + if (cell == nextFree->load(std::memory_order_relaxed)) { nextFree = &cell->next_; continue; } @@ -64,8 +65,8 @@ bool ExtraObjectPage::Sweep(GCSweepScope& sweepHandle, FinalizerQueue& finalizer alive = true; } else { // Free the current block and insert it into the free list. - cell->next_ = *nextFree; - *nextFree = cell; + cell->next_.store(nextFree->load(std::memory_order_relaxed), std::memory_order_relaxed); + nextFree->store(cell, std::memory_order_relaxed); nextFree = &cell->next_; } } diff --git a/kotlin-native/runtime/src/custom_alloc/cpp/ExtraObjectPage.hpp b/kotlin-native/runtime/src/custom_alloc/cpp/ExtraObjectPage.hpp index 8d5e5bb0983..1a2cde11b6c 100644 --- a/kotlin-native/runtime/src/custom_alloc/cpp/ExtraObjectPage.hpp +++ b/kotlin-native/runtime/src/custom_alloc/cpp/ExtraObjectPage.hpp @@ -21,7 +21,7 @@ struct ExtraObjectCell { // This is used to simultaneously build two lists: a free list and a finalizers queue. // A cell cannot exist in both of them, but can be in neither when it's alive. - ExtraObjectCell* next_; + std::atomic next_; struct alignas(mm::ExtraObjectData) { uint8_t data_[sizeof(mm::ExtraObjectData)]; }; @@ -54,8 +54,8 @@ private: ExtraObjectPage() noexcept; // Used for linking pages together in `pages` queue or in `unswept` queue. - ExtraObjectPage* next_; - ExtraObjectCell* nextFree_; + std::atomic next_; + std::atomic nextFree_; ExtraObjectCell cells_[]; }; diff --git a/kotlin-native/runtime/src/custom_alloc/cpp/FixedBlockPage.hpp b/kotlin-native/runtime/src/custom_alloc/cpp/FixedBlockPage.hpp index ca760bfa920..5557c2770af 100644 --- a/kotlin-native/runtime/src/custom_alloc/cpp/FixedBlockPage.hpp +++ b/kotlin-native/runtime/src/custom_alloc/cpp/FixedBlockPage.hpp @@ -53,7 +53,7 @@ private: friend class AtomicStack; // Used for linking pages together in `pages` queue or in `unswept` queue. - FixedBlockPage* next_; + std::atomic next_; FixedCellRange nextFree_; uint32_t blockSize_; uint32_t end_; diff --git a/kotlin-native/runtime/src/custom_alloc/cpp/NextFitPage.hpp b/kotlin-native/runtime/src/custom_alloc/cpp/NextFitPage.hpp index e8bd30fbd82..38c3837dda6 100644 --- a/kotlin-native/runtime/src/custom_alloc/cpp/NextFitPage.hpp +++ b/kotlin-native/runtime/src/custom_alloc/cpp/NextFitPage.hpp @@ -46,7 +46,7 @@ private: void UpdateCurBlock(uint32_t cellsNeeded) noexcept; friend class AtomicStack; - NextFitPage* next_; + std::atomic next_; Cell* curBlock_; Cell cells_[]; // cells_[0] is reserved for an empty block diff --git a/kotlin-native/runtime/src/custom_alloc/cpp/SingleObjectPage.hpp b/kotlin-native/runtime/src/custom_alloc/cpp/SingleObjectPage.hpp index 41dade484c8..5eed1f7fd8f 100644 --- a/kotlin-native/runtime/src/custom_alloc/cpp/SingleObjectPage.hpp +++ b/kotlin-native/runtime/src/custom_alloc/cpp/SingleObjectPage.hpp @@ -41,7 +41,7 @@ private: // Testing method std_support::vector GetAllocatedBlocks() noexcept; - SingleObjectPage* next_; + std::atomic next_; bool isAllocated_ = false; size_t size_; struct alignas(8) {