[wire] Fix some other lock-inversions with client Buffers. - This fixes two more lock-inversion cases that weren't caught in: https://dawn-review.git.corp.google.com/c/dawn/+/321995. Specifically: 1) One lock inversion was a result of calling MapAsyncEvent's ReadyHook while also holding the EventManager's mTrackedEvents lock. The inversion happens when a user's MapAsync callback calls MapAsync inside itself which, while holding the Buffer lock, now tries to acquire mTrackedEvent's lock. This change ensures that ReadyHook is called while we are not holding the mTrackedEvent's lock so that those two locks are always acquired with the Buffer lock first. 2) Another lock inversion happens in our end2end tests because we synchronously mock the wire with the TerribleCommandBuffer in those tests. This case shouldn't cause an issue in production because it is a result of the TerribleCommandBuffer blocking synchronously and calling Flush as a part of GetCmdSpace. In Chromium, the Flush wouldn't be blocking since it would be an IPC back to the server, but in our tests, it happens while holding any locks on the client side, specifically, this inversion can happen when: Let M0 be the buffer->mState lock, M1 be the server-wide lock from server->GetGuard() M2 be a native lock, in this case the call_once in native::Buffer::MapAsyncBufferEvent::Complete. The inversion proposed by TSAN assumes the following happens: - Thread 1 calls something like Client::Buffer::Unmap which takes M0, then flushes to the server trying to take M1. - Thread 2 calls a server-side callback handler, i.e. OnBufferMapAsync which starts with M1 taken, then in the native code tries to takes M2. - Thread 3 calls native::MapAsyncEvent::Complete which takes M2, then triggers server->client response which calls the client::Buffer::MapAsyncEvent::ReadyHook which tries to take M0. With those three things happening, then we could have a deadlock. To address this, we made sure that the client->SerializeCommand calls are called without holding buffer->mState lock. This also required making the memoryHandle a shared_ptr for now so that we could properly serialize the memoryHandle data while being sure that the handle was still valid. Bug: 529413629 Change-Id: I561190c76bd26607193e9f847e5cb89744a0a3c4 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/323497 Reviewed-by: Kai Ninomiya <kainino@chromium.org> Commit-Queue: Loko Kung <lokokung@google.com>
diff --git a/src/dawn/wire/client/Buffer.cpp b/src/dawn/wire/client/Buffer.cpp index 22cbfc7..143c6ff 100644 --- a/src/dawn/wire/client/Buffer.cpp +++ b/src/dawn/wire/client/Buffer.cpp
@@ -213,7 +213,7 @@ } // Create the MemoryHandle for mappable buffers. - std::unique_ptr<MemoryTransferService::MemoryHandle> memoryHandle = nullptr; + std::shared_ptr<MemoryTransferService::MemoryHandle> memoryHandle = nullptr; size_t memoryHandleCreateInfoLength = 0; if (mappable) { memoryHandle = wireClient->GetMemoryTransferService()->CreateMemoryHandle(descriptor->size); @@ -234,8 +234,17 @@ Ref<Buffer> buffer = wireClient->Make<Buffer>(device->GetEventManagerHandle(), device, descriptor); + DeviceCreateBufferCmd cmd; + cmd.deviceId = device->GetWireHandle(wireClient).id; + cmd.descriptor = descriptor; + // Set the pointer lengths, but the pointed-to data itself won't be serialized as usual (due + // to skip_serialize). Instead, the custom CommandExtensions below fill that memory. + cmd.memoryHandleCreateInfoLength = memoryHandleCreateInfoLength; + cmd.memoryHandleCreateInfo = nullptr; // Skipped by skip_serialize. + cmd.result = buffer->GetWireHandle(wireClient); + buffer->mState.Use([&](auto state) { - state->memoryHandle = std::move(memoryHandle); + state->memoryHandle = memoryHandle; if (descriptor->mappedAtCreation) { // If the buffer is mapped at creation, a memory handle is created and will be @@ -248,28 +257,18 @@ DAWN_ASSERT(state->memoryHandle != nullptr); state->mappedData = state->memoryHandle->GetData(); } - - DeviceCreateBufferCmd cmd; - cmd.deviceId = device->GetWireHandle(wireClient).id; - cmd.descriptor = descriptor; - // Set the pointer lengths, but the pointed-to data itself won't be serialized as usual (due - // to skip_serialize). Instead, the custom CommandExtensions below fill that memory. - cmd.memoryHandleCreateInfoLength = memoryHandleCreateInfoLength; - cmd.memoryHandleCreateInfo = nullptr; // Skipped by skip_serialize. - cmd.result = buffer->GetWireHandle(wireClient); - - // Turning off clang format here because for some reason it does not format the - // CommandExtensions consistently, making it harder to read. - wireClient->SerializeCommand( - cmd, - // Extensions to replace fields skipped by skip_serialize. - CommandExtension{memoryHandleCreateInfoLength, [&](Span<std::byte> serializeBuffer) { - if (state->memoryHandle != nullptr) { - // Serialize the MemoryHandle into the space after the command. - state->memoryHandle->SerializeCreate(serializeBuffer); - } - }}); }); + + wireClient->SerializeCommand( + cmd, + // Extensions to replace fields skipped by skip_serialize. + CommandExtension{memoryHandleCreateInfoLength, [&](Span<std::byte> serializeBuffer) { + if (memoryHandle != nullptr) { + // Serialize the MemoryHandle into the space after the command. + memoryHandle->SerializeCreate(serializeBuffer); + } + }}); + return ReturnToAPI(std::move(buffer)); } @@ -468,33 +467,20 @@ // - Server -> Client: Result of MapRequest2 Client* client = GetClient(); + BufferUpdateMappedDataCmd cmd{}; + std::shared_ptr<MemoryTransferService::MemoryHandle> memoryHandle; + mState.Use([&](auto state) { if (state->IsMappedForWriting()) { // Writes need to be flushed before Unmap is sent. Unmap calls all associated // in-flight callbacks which may read the updated data. DAWN_ASSERT(state->memoryHandle != nullptr); - // Get the serialization size of data update writes. - size_t memoryDataUpdateInfoLength = state->memoryHandle->GetSerializeDataUpdateSize( - state->mappedOffset, state->mappedSize); - - BufferUpdateMappedDataCmd cmd{}; cmd.bufferId = GetWireHandle(client).id; - // Set the pointer length, but the pointed-to data itself won't be serialized as usual - // (due to skip_serialize). Instead, the custom CommandExtension below fills that - // memory. - cmd.dataUpdateInfoLength = memoryDataUpdateInfoLength; - cmd.dataUpdateInfo = nullptr; // Skipped by skip_serialize. cmd.offset = state->mappedOffset; cmd.size = state->mappedSize; - client->SerializeCommand( - cmd, - // Extensions to replace fields skipped by skip_serialize. - CommandExtension{memoryDataUpdateInfoLength, [&](Span<std::byte> serializeBuffer) { - state->memoryHandle->SerializeDataUpdate( - serializeBuffer, state->mappedOffset, state->mappedSize); - }}); + memoryHandle = state->memoryHandle; // If mDestructMemoryHandleOnUnmap is true, that means the memory handle is merely // for mappedAtCreation usage. It is destroyed on unmap after flush to server @@ -511,11 +497,30 @@ state->mappedSize = 0; }); + if (memoryHandle) { + size_t memoryDataUpdateInfoLength = + memoryHandle->GetSerializeDataUpdateSize(cmd.offset, cmd.size); + + // Set the pointer length, but the pointed-to data itself won't be serialized as usual + // (due to skip_serialize). Instead, the custom CommandExtension below fills that + // memory. + cmd.dataUpdateInfoLength = memoryDataUpdateInfoLength; + cmd.dataUpdateInfo = nullptr; // Skipped by skip_serialize. + + client->SerializeCommand( + cmd, + // Extensions to replace fields skipped by skip_serialize. + CommandExtension{memoryDataUpdateInfoLength, [&](Span<std::byte> serializeBuffer) { + memoryHandle->SerializeDataUpdate(serializeBuffer, cmd.offset, + cmd.size); + }}); + } + SetFutureStatus(WGPUMapAsyncStatus_Aborted, "Buffer was unmapped before mapping was resolved."); - BufferUnmapCmd cmd{}; - cmd.self = ToAPI(this); - client->SerializeCommand(cmd); + BufferUnmapCmd unmapCmd{}; + unmapCmd.self = ToAPI(this); + client->SerializeCommand(unmapCmd); } void Buffer::APIDestroy() {
diff --git a/src/dawn/wire/client/Buffer.h b/src/dawn/wire/client/Buffer.h index a2d5dbd..cf87904 100644 --- a/src/dawn/wire/client/Buffer.h +++ b/src/dawn/wire/client/Buffer.h
@@ -110,7 +110,7 @@ size_t mappedOffset = 0; size_t mappedSize = 0; - std::unique_ptr<MemoryTransferService::MemoryHandle> memoryHandle = nullptr; + std::shared_ptr<MemoryTransferService::MemoryHandle> memoryHandle = nullptr; }; using GuardedState = MutexRecursiveProtected<State>::Usage;
diff --git a/src/dawn/wire/client/Device.cpp b/src/dawn/wire/client/Device.cpp index 7c5f74c..f9b9f91 100644 --- a/src/dawn/wire/client/Device.cpp +++ b/src/dawn/wire/client/Device.cpp
@@ -34,6 +34,7 @@ #include "dawn/wire/client/ApiObjects_autogen.h" #include "partition_alloc/pointers/raw_ptr.h" +#include "src/dawn/common/MutexProtected.h" #include "src/dawn/common/StringViewUtils.h" #include "src/dawn/wire/client/Client.h" #include "src/dawn/wire/client/EventManager.h" @@ -188,19 +189,28 @@ EventType GetType() override { return kType; } WireResult ReadyHook(FutureID futureID, WGPUDeviceLostReason reason, WGPUStringView message) { - if (mMessage.empty()) { - mReason = reason; - mMessage = ToString(message); - } + mState.Use([&](auto state) { + if (state->message.empty()) { + state->reason = reason; + state->message = ToString(message); + } + }); return WireResult::Success; } private: void CompleteImpl(FutureID futureID, EventCompletionType completionType) override { - if (completionType == EventCompletionType::Shutdown) { - mReason = WGPUDeviceLostReason_CallbackCancelled; - mMessage = "A valid external Instance reference no longer exists."; - } + WGPUDeviceLostReason reason; + std::string message; + + mState.Use([&](auto state) { + if (completionType == EventCompletionType::Shutdown) { + state->reason = WGPUDeviceLostReason_CallbackCancelled; + state->message = "A valid external Instance reference no longer exists."; + } + reason = state->reason; + message = state->message; + }); // The uncaptured error and logging callbacks are spontaneous and must not be called // after we call the device lost's |mCallback| below, so we clear them and wait for them to @@ -213,8 +223,8 @@ if (mCallback != nullptr) { const auto device = - mReason != WGPUDeviceLostReason_FailedCreation ? ToAPI(mDevice.Get()) : nullptr; - mCallback(&device, mReason, ToOutputStringView(mMessage), userdata1, userdata2); + reason != WGPUDeviceLostReason_FailedCreation ? ToAPI(mDevice.Get()) : nullptr; + mCallback(&device, reason, ToOutputStringView(message), userdata1, userdata2); } } @@ -222,8 +232,11 @@ raw_ptr<void> mUserdata1 = nullptr; raw_ptr<void> mUserdata2 = nullptr; - WGPUDeviceLostReason mReason; - std::string mMessage; + struct State { + WGPUDeviceLostReason reason; + std::string message; + }; + MutexProtected<State> mState; // Strong reference to the device so that when we call the callback we can pass the device. Ref<Device> mDevice;
diff --git a/src/dawn/wire/client/EventManager.h b/src/dawn/wire/client/EventManager.h index 98bf46b..edba2ca 100644 --- a/src/dawn/wire/client/EventManager.h +++ b/src/dawn/wire/client/EventManager.h
@@ -100,8 +100,6 @@ // Subcomponent which tracks callback events for the Future-based callback // entrypoints. All events from this instance (regardless of whether from an adapter, device, queue, // etc.) are tracked here, and used by the instance-wide ProcessEvents and WaitAny entrypoints. -// -// TODO(crbug.com/dawn/2060): This should probably be merged together with RequestTracker. class EventManager final : NonMovable { public: using EventMap = std::map<FutureID, Ref<TrackedEvent>>; @@ -128,14 +126,14 @@ return WireResult::FatalError; } - Ref<TrackedEvent> spontaneousEvent; + Ref<TrackedEvent> trackedEvent; WireResult result = mTrackedEvents.Use([&](auto trackedEvents) { auto it = trackedEvents->find(futureID); if (it == trackedEvents->end()) { // If the future is not found, it must've already been completed. return WireResult::Success; } - auto& trackedEvent = it->second; + trackedEvent = it->second; if (trackedEvent->GetType() != Event::kType) { // Assert here for debugging, before returning a fatal error that is handled upwards @@ -143,22 +141,25 @@ DAWN_ASSERT(trackedEvent->GetType() == Event::kType); return WireResult::FatalError; } - - WireResult result = static_cast<Event*>(trackedEvent.Get()) - ->ReadyHook(futureID, std::forward<ReadyArgs>(readyArgs)...); - trackedEvent->SetReady(); - - // If the event can be spontaneously completed, prepare to do so now. - if (trackedEvent->GetCallbackMode() == WGPUCallbackMode_AllowSpontaneous) { - spontaneousEvent = trackedEvent; - } - - return result; + return WireResult::Success; }); + if (result != WireResult::Success || !trackedEvent) { + return result; + } + + // The ReadyHook function is assumed to be thread-safe or only triggered by a server + // response (which only happens in a single thread). The only events that can be triggered + // from the client directly as of writing are MapAsync and DeviceLost. + result = static_cast<Event*>(trackedEvent.Get()) + ->ReadyHook(futureID, std::forward<ReadyArgs>(readyArgs)...); + + // We need to set the event ready within the scope of the cond-var to signal it. + mTrackedEvents.Use([&](auto trackedEvents) { trackedEvent->SetReady(); }); + // Handle spontaneous completions. - if (spontaneousEvent) { - spontaneousEvent->Complete(futureID, EventCompletionType::Ready); + if (trackedEvent->GetCallbackMode() == WGPUCallbackMode_AllowSpontaneous) { + trackedEvent->Complete(futureID, EventCompletionType::Ready); mTrackedEvents.Use([&](auto trackedEvents) { trackedEvents->erase(futureID); }); } return result;