diff --git a/runtime/built_ins/built_ins.cpp b/runtime/built_ins/built_ins.cpp index 5438d2c112..ae3b4d56f7 100644 --- a/runtime/built_ins/built_ins.cpp +++ b/runtime/built_ins/built_ins.cpp @@ -817,7 +817,7 @@ void BuiltInOwnershipWrapper::takeOwnership(BuiltinDispatchInfoBuilder &inputBui UNRECOVERABLE_IF(builder); builder = &inputBuilder; for (auto &kernel : builder->peekUsedKernels()) { - kernel->takeOwnership(true); + kernel->takeOwnership(); kernel->setContext(context); } } diff --git a/runtime/helpers/base_object.h b/runtime/helpers/base_object.h index f49a6904ac..2b2ecc072f 100644 --- a/runtime/helpers/base_object.h +++ b/runtime/helpers/base_object.h @@ -101,27 +101,29 @@ class TakeOwnershipWrapper { public: TakeOwnershipWrapper(T &obj) : obj(obj) { - this->locked = obj.takeOwnership(true); + lock(); } TakeOwnershipWrapper(T &obj, bool lockImmediately) : obj(obj) { if (lockImmediately) { - this->locked = obj.takeOwnership(true); + lock(); } } ~TakeOwnershipWrapper() { - if (locked) { - obj.releaseOwnership(); - } + unlock(); } void unlock() { - obj.releaseOwnership(); - locked = false; + if (locked) { + obj.releaseOwnership(); + locked = false; + } } void lock() { - if (!locked) - this->locked = obj.takeOwnership(true); + if (!locked) { + obj.takeOwnership(); + locked = true; + } } private: @@ -191,7 +193,7 @@ class BaseObject : public B, public ReferenceTrackedObject> { return this->getRefApiCount(); } - MOCKABLE_VIRTUAL bool takeOwnership(bool waitUntilGet) const { + MOCKABLE_VIRTUAL void takeOwnership() const { DEBUG_BREAK_IF(!isValid()); std::unique_lock theLock(mtx); @@ -199,22 +201,17 @@ class BaseObject : public B, public ReferenceTrackedObject> { if (owner == invalidThreadID) { owner = self; - return true; + return; } if (owner == self) { ++recursiveOwnageCounter; - return true; - } - - if (waitUntilGet == false) { - return false; + return; } cond.wait(theLock, [&] { return owner == invalidThreadID; }); owner = self; recursiveOwnageCounter = 0; - return true; } MOCKABLE_VIRTUAL void releaseOwnership() const { diff --git a/unit_tests/event/user_events_tests_mt.cpp b/unit_tests/event/user_events_tests_mt.cpp index 207c2dd03c..4e8953ecb6 100644 --- a/unit_tests/event/user_events_tests_mt.cpp +++ b/unit_tests/event/user_events_tests_mt.cpp @@ -92,7 +92,7 @@ TEST_F(EventTests, givenUserEventBlockingEnqueueWithBlockingFlagWhenUserEventIsC std::thread t([&]() { while (true) { - pCmdQ->takeOwnership(true); + pCmdQ->takeOwnership(); if (pCmdQ->taskLevel == Event::eventNotReady) { pCmdQ->releaseOwnership(); diff --git a/unit_tests/helpers/base_object_tests.cpp b/unit_tests/helpers/base_object_tests.cpp index 37787fc051..ad443791a9 100644 --- a/unit_tests/helpers/base_object_tests.cpp +++ b/unit_tests/helpers/base_object_tests.cpp @@ -227,13 +227,13 @@ TYPED_TEST(BaseObjectTests, WhenAlreadyOwnedByThreadOnTakeOrReleaseOwnershipUses TypeParam obj; EXPECT_FALSE(obj.hasOwnership()); - obj.takeOwnership(false); + obj.takeOwnership(); EXPECT_TRUE(obj.hasOwnership()); - obj.takeOwnership(false); + obj.takeOwnership(); EXPECT_TRUE(obj.hasOwnership()); - obj.takeOwnership(false); + obj.takeOwnership(); EXPECT_TRUE(obj.hasOwnership()); obj.releaseOwnership(); @@ -280,29 +280,8 @@ class MockBuffer : public MockBufferStorage, public Buffer { void setArgStateful(void *memory, bool forceNonAuxMode, bool disableL3Cache) override { } - - void setFakeOwnership() { - this->owner = tempThreadID; - } }; -TEST(BaseObjectTest, takeOwnership) { - MockBuffer buffer; - EXPECT_FALSE(buffer.hasOwnership()); - buffer.takeOwnership(false); - EXPECT_TRUE(buffer.hasOwnership()); - buffer.takeOwnership(false); - EXPECT_TRUE(buffer.hasOwnership()); - buffer.releaseOwnership(); - EXPECT_TRUE(buffer.hasOwnership()); - buffer.releaseOwnership(); - EXPECT_FALSE(buffer.hasOwnership()); - buffer.setFakeOwnership(); - EXPECT_FALSE(buffer.hasOwnership()); - buffer.takeOwnership(false); - EXPECT_FALSE(buffer.hasOwnership()); -} - TEST(BaseObjectTest, takeOwnershipWrapper) { MockBuffer buffer; { diff --git a/unit_tests/helpers/base_object_tests_mt.cpp b/unit_tests/helpers/base_object_tests_mt.cpp index f00d80ff09..27baf6ae9b 100644 --- a/unit_tests/helpers/base_object_tests_mt.cpp +++ b/unit_tests/helpers/base_object_tests_mt.cpp @@ -13,54 +13,39 @@ namespace NEO { -template -struct BaseObjectTestsMt : public ::testing::Test { - static void takeOwnerFailThreadFunc(TypeParam *obj) { - auto ret = obj->takeOwnership(false); - EXPECT_EQ(false, ret); - } - static void takeOwnerWaitThreadFunc(TypeParam *obj) { - auto ret = obj->takeOwnership(true); - EXPECT_EQ(true, ret); - obj->releaseOwnership(); - } -}; +TEST(BaseObjectTestsMt, givenObjectOwnershipForEachThreadWhenIncrementingNonAtomicValueThenNoDataRacesAreExpected) { + CommandQueue *object = new CommandQueue; + object->takeOwnership(); + uint32_t counter = 0; + const uint32_t loopCount = 50; + const uint32_t numThreads = 3; -typedef ::testing::Types< - Platform, - IntelAccelerator, - CommandQueue> - BaseObjectTypes; - -TYPED_TEST_CASE(BaseObjectTestsMt, BaseObjectTypes); - -// "typedef" BaseObjectTestsMt template to use with different TypeParams for testing -template -using BaseObjectWithDefaultCtorTests = BaseObjectTestsMt; - -TYPED_TEST(BaseObjectTestsMt, takeOwner) { - TypeParam *object = new TypeParam; - bool ret = object->takeOwnership(false); - EXPECT_EQ(true, ret); - - std::thread t1(BaseObjectTestsMt::takeOwnerFailThreadFunc, object); - t1.join(); + auto incrementNonAtomicValue = [&](CommandQueue *obj) { + for (uint32_t i = 0; i < loopCount; i++) { + obj->takeOwnership(); + counter++; + obj->releaseOwnership(); + } + }; EXPECT_EQ(0U, object->getCond().peekNumWaiters()); - std::thread t2(BaseObjectTestsMt::takeOwnerWaitThreadFunc, object); - //wait on condition var counter, so current threads know t2 thread waits on this variable - while (object->getCond().peekNumWaiters() == 0U) { + std::thread t1(incrementNonAtomicValue, object); + std::thread t2(incrementNonAtomicValue, object); + std::thread t3(incrementNonAtomicValue, object); + + while (object->getCond().peekNumWaiters() != numThreads) { std::this_thread::yield(); } - //t2 thread waits on conditional varialbe within takeOwnership - EXPECT_EQ(1U, object->getCond().peekNumWaiters()); - std::this_thread::yield(); - - //current thread releases ownership, so t2 can take it + EXPECT_EQ(0u, counter); object->releaseOwnership(); + + t1.join(); t2.join(); + t3.join(); + + EXPECT_EQ(loopCount * numThreads, counter); object->release(); } diff --git a/unit_tests/mocks/mock_kernel.h b/unit_tests/mocks/mock_kernel.h index 7890115b9d..607b1bd229 100644 --- a/unit_tests/mocks/mock_kernel.h +++ b/unit_tests/mocks/mock_kernel.h @@ -227,10 +227,9 @@ class MockKernel : public Kernel { void makeResident(CommandStreamReceiver &commandStreamReceiver) override; void getResidency(std::vector &dst) override; - bool takeOwnership(bool lock) const override { - auto retVal = Kernel::takeOwnership(lock); + void takeOwnership() const override { + Kernel::takeOwnership(); takeOwnershipCalls++; - return retVal; } void releaseOwnership() const override {