Skip to content

Commit 95aad18

Browse files
lokokungDawn LUCI CQ
authored andcommitted
[dawn][wire] Make the server object tracking thread-safe.
- Since server-side callbacks can now be spontaneous, i.e. may be called on different threads such as in Metal threads, we need to make sure that the object table on the server is protected. We use a Mutex instead of the MutexProtected helpers because it's easier to use given the requirements. (See comment in ServerBase.h). - Removes direct access to the KnownObjects vectors in ServerBase so that they can be controlled via helpers that acquire locks when necessary. Bug: 412761856 Change-Id: I9a2f36851ff9f0809312e69dd9853ff7d2a89a4c Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/261280 Reviewed-by: Corentin Wallez <[email protected]> Reviewed-by: Kai Ninomiya <[email protected]> Commit-Queue: Loko Kung <[email protected]>
1 parent cd8b15b commit 95aad18

File tree

10 files changed

+113
-54
lines changed

10 files changed

+113
-54
lines changed

generator/templates/dawn/wire/server/ServerBase.h

Lines changed: 80 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@
3030

3131
#include <tuple>
3232

33+
#include "dawn/common/Mutex.h"
3334
#include "dawn/dawn_proc_table.h"
3435
#include "dawn/wire/ChunkedCommandHandler.h"
3536
#include "dawn/wire/Wire.h"
@@ -45,17 +46,59 @@ namespace dawn::wire::server {
4546
ServerBase(const DawnProcTable& procs) : mProcs(procs) {}
4647
~ServerBase() override = default;
4748

49+
Mutex::AutoLock GetGuard() { return Mutex::AutoLock(&mMutex); }
50+
4851
protected:
4952
// Proc table may be used by children as well.
5053
DawnProcTable mProcs;
5154

55+
// Template functions that implement helpers on KnownObjects.
56+
template <typename T>
57+
WireResult Get(ObjectId id, Reserved<T>* result) {
58+
return std::get<KnownObjects<T>>(mKnown).Get(id, result);
59+
}
60+
template <typename T>
61+
WireResult Get(ObjectId id, Known<T>* result) {
62+
return std::get<KnownObjects<T>>(mKnown).Get(id, result);
63+
}
64+
template <typename T>
65+
WireResult FillReservation(ObjectId id, T handle, Known<T>* known = nullptr) {
66+
auto result = std::get<KnownObjects<T>>(mKnown).FillReservation(id, handle, known);
67+
if (result == WireResult::FatalError) {
68+
Release(handle);
69+
}
70+
return result;
71+
}
5272
template <typename T>
53-
const KnownObjects<T>& Objects() const {
54-
return std::get<KnownObjects<T>>(mKnown);
73+
WireResult Allocate(Reserved<T>* result,
74+
ObjectHandle handler,
75+
AllocationState state = AllocationState::Allocated) {
76+
// Allocations always take the lock because |vector::push_back| may be called which
77+
// can invalidate pointers.
78+
auto serverGuard = GetGuard();
79+
return std::get<KnownObjects<T>>(mKnown).Allocate(result, handler, state);
5580
}
5681
template <typename T>
57-
KnownObjects<T>& Objects() {
58-
return std::get<KnownObjects<T>>(mKnown);
82+
WireResult Free(ObjectId id, ObjectData<T>* data) {
83+
// Free always take the lock to ensure that any callback handlers (which always take
84+
// the lock), i.e. On*Callback functions, cannot race with object deletion.
85+
auto serverGuard = GetGuard();
86+
Reserved<T> obj;
87+
WIRE_TRY(Get(id, &obj));
88+
*data = std::move(*obj.data);
89+
std::get<KnownObjects<T>>(mKnown).Free(id);
90+
return WireResult::Success;
91+
}
92+
93+
// Special functions that are currently needed.
94+
bool IsDeviceKnown(WGPUDevice device) const {
95+
return std::get<KnownObjects<WGPUDevice>>(mKnown).IsKnown(device);
96+
}
97+
std::vector<WGPUDevice> GetAllDeviceHandles() const {
98+
return std::get<KnownObjects<WGPUDevice>>(mKnown).GetAllHandles();
99+
}
100+
std::vector<WGPUInstance> GetAllInstanceHandles() const {
101+
return std::get<KnownObjects<WGPUInstance>>(mKnown).GetAllHandles();
59102
}
60103

61104
template <typename T>
@@ -65,7 +108,8 @@ namespace dawn::wire::server {
65108
void DestroyAllObjects() {
66109
//* Release devices first to force completion of any async work.
67110
{
68-
std::vector<WGPUDevice> handles = Objects<WGPUDevice>().AcquireAllHandles();
111+
std::vector<WGPUDevice> handles =
112+
std::get<KnownObjects<WGPUDevice>>(mKnown).AcquireAllHandles();
69113
for (WGPUDevice handle : handles) {
70114
Release(handle);
71115
}
@@ -74,7 +118,8 @@ namespace dawn::wire::server {
74118
{% for type in by_category["object"] if type.name.get() != "device" %}
75119
{% set cType = as_cType(type.name) %}
76120
{
77-
std::vector<{{cType}}> handles = Objects<{{cType}}>().AcquireAllHandles();
121+
std::vector<{{cType}}> handles =
122+
std::get<KnownObjects<{{cType}}>>(mKnown).AcquireAllHandles();
78123
for ({{cType}} handle : handles) {
79124
Release(handle);
80125
}
@@ -87,7 +132,7 @@ namespace dawn::wire::server {
87132
{% for type in by_category["object"] %}
88133
{% set cType = as_cType(type.name) %}
89134
WireResult GetFromId(ObjectId id, {{cType}}* out) const final {
90-
return Objects<{{cType}}>().GetNativeHandle(id, out);
135+
return std::get<KnownObjects<{{cType}}>>(mKnown).GetNativeHandle(id, out);
91136
}
92137

93138
WireResult GetOptionalFromId(ObjectId id, {{cType}}* out) const final {
@@ -99,7 +144,34 @@ namespace dawn::wire::server {
99144
}
100145
{% endfor %}
101146

102-
//* The list of known IDs for each object type.
147+
// The list of known IDs for each object type.
148+
// We use an explicit Mutex to protect these lists instead of MutexProtected because:
149+
// 1) It allows us to return AutoLock objects to hold the lock across function scopes.
150+
// 2) It allows us to use |mKnown| when we know we can access it without the lock.
151+
// To clarify the threading model of the KnownObjects, there is always a "main" thread on
152+
// the server which is responsible for flushing commands from the client. This thread is
153+
// the only thread that ever calls |HandleCommandsImpl| which ultimately calls into the
154+
// command handlers in the generated ServerHandlers.cpp. All other threads that can
155+
// interact with |mKnown| are async or spontaneous callback threads which always hold the
156+
// lock for the full duration of their call, see |ForwardToServerHelper| where we force all
157+
// callbacks to take the lock. On the "main" thread, we only acquire the lock whenever we
158+
// |Allocate| or |Free|. We need to lock w.r.t other threads for |Allocate| because
159+
// internally, that can cause a |std::vector::push_back| call which may invalidate all
160+
// pointers. As a result, we cannot have another thread doing anything during that window.
161+
// Similarly, for |Free|, we may release the WGPU* backing handle which is also not allowed
162+
// to race. Reads on the "main" thread are not explicitly synchronized because callbacks
163+
// are not allowed to allocate or free objects which guarantees that the reads on the
164+
// "main" thread will be pointer stable. Furthermore, it is actually necessary that we
165+
// don't hold the lock for reads on the "main" thread because APIs like |Device::Destroy|
166+
// can cause us to wait on callbacks which as stated above, require the lock to complete.
167+
// So if we were to hold the lock on the "main" thread during the read and execution of
168+
// |Device::Destroy|, we could deadlock when we try waiting for callbacks which can't
169+
// acquire this lock.
170+
// TODO(https://crbug.com/412761856): Revisit this logic and consider using a rw-lock
171+
// in conjunction with taking additional refs to "special" objects/function pairs that can
172+
// cause callbacks to run when called. This would make this logic less specific to the way
173+
// that the Dawn tests and Chromium uses the wire.
174+
Mutex mMutex;
103175
std::tuple<
104176
{% for type in by_category["object"] %}
105177
KnownObjects<{{as_cType(type.name)}}>{{ ", " if not loop.last else "" }}

generator/templates/dawn/wire/server/ServerDoers.cpp

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -82,19 +82,19 @@ namespace dawn::wire::server {
8282
{% for type in by_category["object"] %}
8383
{% set cType = as_cType(type.name) %}
8484
case ObjectType::{{type.name.CamelCase()}}: {
85-
Reserved<{{cType}}> obj;
86-
WIRE_TRY(Objects<{{cType}}>().Get(objectId, &obj));
85+
ObjectData<{{cType}}> data;
86+
WIRE_TRY(Free<{{cType}}>(objectId, &data));
8787

88-
if (obj->state == AllocationState::Allocated) {
89-
DAWN_ASSERT(obj->handle != nullptr);
88+
//* Handle actually releasing the object after untracking it.
89+
if (data.state == AllocationState::Allocated) {
90+
DAWN_ASSERT(data.handle != nullptr);
9091
{% if type.name.get() == "device" %}
9192
//* Deregisters uncaptured error and device lost callbacks since
9293
//* they should not be forwarded if the device no longer exists on the wire.
93-
ClearDeviceCallbacks(obj->handle);
94+
ClearDeviceCallbacks(data.handle);
9495
{% endif %}
95-
Release(obj->handle);
96+
Release(data.handle);
9697
}
97-
Objects<{{cType}}>().Free(objectId);
9898
return WireResult::Success;
9999
}
100100
{% endfor %}

generator/templates/dawn/wire/server/ServerHandlers.cpp

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,7 @@ namespace dawn::wire::server {
5353
{% set cType = as_cType(member.handle_type.name) %}
5454
{% set name = as_varName(member.name) %}
5555
Reserved<{{cType}}> {{name}}Data;
56-
WIRE_TRY(Objects<{{cType}}>().Allocate(&{{name}}Data, cmd.{{name}}));
56+
WIRE_TRY(Allocate(&{{name}}Data, cmd.{{name}}));
5757
{{name}}Data->generation = cmd.{{name}}.generation;
5858
{%- endfor %}
5959

@@ -62,7 +62,7 @@ namespace dawn::wire::server {
6262
{% set cType = as_cType(member.id_type.name) %}
6363
{% set name = as_varName(member.name) %}
6464
Known<{{cType}}> {{name}}Handle;
65-
WIRE_TRY(Objects<{{cType}}>().Get(cmd.{{name}}, &{{name}}Handle));
65+
WIRE_TRY(Get(cmd.{{name}}, &{{name}}Handle));
6666
{% endfor %}
6767

6868
//* Do command
@@ -124,7 +124,7 @@ namespace dawn::wire::server {
124124
// After the server handles all the commands from the stream, we additionally run
125125
// ProcessEvents on all known Instances so that any work done on the server side can be
126126
// forwarded through to the client.
127-
for (auto instance : Objects<WGPUInstance>().GetAllHandles()) {
127+
for (auto instance : GetAllInstanceHandles()) {
128128
if (DoInstanceProcessEvents(instance) != WireResult::Success) {
129129
return nullptr;
130130
}

src/dawn/wire/server/Server.cpp

Lines changed: 9 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@ Server::Server(const DawnProcTable& procs,
6161
Server::~Server() {
6262
// Un-set the error and lost callbacks since we cannot forward them
6363
// after the server has been destroyed.
64-
for (WGPUDevice device : Objects<WGPUDevice>().GetAllHandles()) {
64+
for (WGPUDevice device : GetAllDeviceHandles()) {
6565
ClearDeviceCallbacks(device);
6666
}
6767
DestroyAllObjects();
@@ -72,13 +72,13 @@ WireResult Server::InjectBuffer(WGPUBuffer buffer,
7272
const Handle& deviceHandle) {
7373
DAWN_ASSERT(buffer != nullptr);
7474
Known<WGPUDevice> device;
75-
WIRE_TRY(Objects<WGPUDevice>().Get(deviceHandle.id, &device));
75+
WIRE_TRY(Get(deviceHandle.id, &device));
7676
if (device->generation != deviceHandle.generation) {
7777
return WireResult::FatalError;
7878
}
7979

8080
Reserved<WGPUBuffer> data;
81-
WIRE_TRY(Objects<WGPUBuffer>().Allocate(&data, handle));
81+
WIRE_TRY(Allocate(&data, handle));
8282

8383
data->handle = buffer;
8484
data->generation = handle.generation;
@@ -96,13 +96,13 @@ WireResult Server::InjectTexture(WGPUTexture texture,
9696
const Handle& deviceHandle) {
9797
DAWN_ASSERT(texture != nullptr);
9898
Known<WGPUDevice> device;
99-
WIRE_TRY(Objects<WGPUDevice>().Get(deviceHandle.id, &device));
99+
WIRE_TRY(Get(deviceHandle.id, &device));
100100
if (device->generation != deviceHandle.generation) {
101101
return WireResult::FatalError;
102102
}
103103

104104
Reserved<WGPUTexture> data;
105-
WIRE_TRY(Objects<WGPUTexture>().Allocate(&data, handle));
105+
WIRE_TRY(Allocate(&data, handle));
106106

107107
data->handle = texture;
108108
data->generation = handle.generation;
@@ -120,13 +120,13 @@ WireResult Server::InjectSurface(WGPUSurface surface,
120120
const Handle& instanceHandle) {
121121
DAWN_ASSERT(surface != nullptr);
122122
Known<WGPUInstance> instance;
123-
WIRE_TRY(Objects<WGPUInstance>().Get(instanceHandle.id, &instance));
123+
WIRE_TRY(Get(instanceHandle.id, &instance));
124124
if (instance->generation != instanceHandle.generation) {
125125
return WireResult::FatalError;
126126
}
127127

128128
Reserved<WGPUSurface> data;
129-
WIRE_TRY(Objects<WGPUSurface>().Allocate(&data, handle));
129+
WIRE_TRY(Allocate(&data, handle));
130130

131131
data->handle = surface;
132132
data->generation = handle.generation;
@@ -142,7 +142,7 @@ WireResult Server::InjectSurface(WGPUSurface surface,
142142
WireResult Server::InjectInstance(WGPUInstance instance, const Handle& handle) {
143143
DAWN_ASSERT(instance != nullptr);
144144
Reserved<WGPUInstance> data;
145-
WIRE_TRY(Objects<WGPUInstance>().Allocate(&data, handle));
145+
WIRE_TRY(Allocate(&data, handle));
146146

147147
data->handle = instance;
148148
data->generation = handle.generation;
@@ -157,17 +157,12 @@ WireResult Server::InjectInstance(WGPUInstance instance, const Handle& handle) {
157157

158158
WGPUDevice Server::GetDevice(uint32_t id, uint32_t generation) {
159159
Known<WGPUDevice> device;
160-
if (Objects<WGPUDevice>().Get(id, &device) != WireResult::Success ||
161-
device->generation != generation) {
160+
if (Get(id, &device) != WireResult::Success || device->generation != generation) {
162161
return nullptr;
163162
}
164163
return device->handle;
165164
}
166165

167-
bool Server::IsDeviceKnown(WGPUDevice device) const {
168-
return Objects<WGPUDevice>().IsKnown(device);
169-
}
170-
171166
void Server::Flush() {
172167
if (mUseSpontaneousCallbacks) {
173168
mSerializer->Flush();

src/dawn/wire/server/Server.h

Lines changed: 5 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,10 @@ struct ForwardToServerHelper<F, void (Server::*)(UserdataT*, Args...)> {
8888
return;
8989
}
9090
// Forward the arguments and the typed userdata to the Server:: member function.
91-
(server.get()->*F)(data.get(), std::forward<decltype(args)>(args)...);
91+
{
92+
auto serverGuard = server.get()->GetGuard();
93+
(server.get()->*F)(data.get(), std::forward<decltype(args)>(args)...);
94+
}
9295
server.get()->Flush();
9396
}
9497
};
@@ -180,7 +183,7 @@ class Server : public ServerBase {
180183
WireResult InjectInstance(WGPUInstance instance, const Handle& handle);
181184

182185
WGPUDevice GetDevice(uint32_t id, uint32_t generation);
183-
bool IsDeviceKnown(WGPUDevice device) const;
186+
using ServerBase::IsDeviceKnown;
184187

185188
// Flushes the command serialized from server->client if spontaneous callbacks are enabled.
186189
void Flush();
@@ -215,15 +218,6 @@ class Server : public ServerBase {
215218
mSerializer->SerializeCommand(cmd, std::forward<Extensions>(es)...);
216219
}
217220

218-
template <typename T>
219-
WireResult FillReservation(ObjectId id, T handle, Known<T>* known = nullptr) {
220-
auto result = Objects<T>().FillReservation(id, handle, known);
221-
if (result == WireResult::FatalError) {
222-
Release(handle);
223-
}
224-
return result;
225-
}
226-
227221
// Wrapper RAII helper for structs with FreeMember calls.
228222
template <typename Struct>
229223
class FreeMembers : public Struct {

src/dawn/wire/server/ServerAdapter.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,7 @@ WireResult Server::DoAdapterRequestDevice(Known<WGPUAdapter> adapter,
4343
WGPUFuture deviceLostFuture,
4444
const WGPUDeviceDescriptor* descriptor) {
4545
Reserved<WGPUDevice> device;
46-
WIRE_TRY(Objects<WGPUDevice>().Allocate(&device, deviceHandle, AllocationState::Reserved));
46+
WIRE_TRY(Allocate(&device, deviceHandle, AllocationState::Reserved));
4747

4848
auto userdata = MakeUserdata<RequestDeviceUserdata>();
4949
userdata->eventManager = eventManager;

src/dawn/wire/server/ServerBuffer.cpp

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,7 @@ WireResult Server::PreHandleDeviceCreateErrorBuffer(const DeviceCreateErrorBuffe
5454

5555
WireResult Server::PreHandleBufferUnmap(const BufferUnmapCmd& cmd) {
5656
Known<WGPUBuffer> buffer;
57-
WIRE_TRY(Objects<WGPUBuffer>().Get(cmd.selfId, &buffer));
57+
WIRE_TRY(Get(cmd.selfId, &buffer));
5858

5959
if (buffer->mappedAtCreation && !(buffer->usage & WGPUBufferUsage_MapWrite)) {
6060
// This indicates the writeHandle is for mappedAtCreation only. Destroy on unmap
@@ -71,7 +71,7 @@ WireResult Server::PreHandleBufferUnmap(const BufferUnmapCmd& cmd) {
7171
WireResult Server::PreHandleBufferDestroy(const BufferDestroyCmd& cmd) {
7272
// Destroying a buffer does an implicit unmapping.
7373
Known<WGPUBuffer> buffer;
74-
WIRE_TRY(Objects<WGPUBuffer>().Get(cmd.selfId, &buffer));
74+
WIRE_TRY(Get(cmd.selfId, &buffer));
7575

7676
// The buffer was destroyed. Clear the Read/WriteHandle.
7777
buffer->readHandle = nullptr;
@@ -135,7 +135,7 @@ WireResult Server::DoDeviceCreateBuffer(Known<WGPUDevice> device,
135135
const uint8_t* writeHandleCreateInfo) {
136136
// Create and register the buffer object.
137137
Reserved<WGPUBuffer> buffer;
138-
WIRE_TRY(Objects<WGPUBuffer>().Allocate(&buffer, bufferHandle));
138+
WIRE_TRY(Allocate(&buffer, bufferHandle));
139139
buffer->handle = mProcs.deviceCreateBuffer(device->handle, descriptor);
140140
buffer->usage = descriptor->usage;
141141
buffer->mappedAtCreation = (descriptor->mappedAtCreation != 0u);
@@ -250,7 +250,7 @@ void Server::OnBufferMapAsyncCallback(MapUserdata* data,
250250
WGPUStringView message) {
251251
// Skip sending the callback if the buffer has already been destroyed.
252252
Known<WGPUBuffer> buffer;
253-
if (Objects<WGPUBuffer>().Get(data->buffer.id, &buffer) != WireResult::Success ||
253+
if (Get(data->buffer.id, &buffer) != WireResult::Success ||
254254
buffer->generation != data->buffer.generation) {
255255
return;
256256
}

src/dawn/wire/server/ServerDevice.cpp

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -101,8 +101,7 @@ WireResult Server::DoDeviceCreateComputePipelineAsync(
101101
ObjectHandle pipelineObjectHandle,
102102
const WGPUComputePipelineDescriptor* descriptor) {
103103
Reserved<WGPUComputePipeline> pipeline;
104-
WIRE_TRY(Objects<WGPUComputePipeline>().Allocate(&pipeline, pipelineObjectHandle,
105-
AllocationState::Reserved));
104+
WIRE_TRY(Allocate(&pipeline, pipelineObjectHandle, AllocationState::Reserved));
106105

107106
auto userdata = MakeUserdata<CreatePipelineAsyncUserData>();
108107
userdata->device = device.AsHandle();
@@ -142,8 +141,7 @@ WireResult Server::DoDeviceCreateRenderPipelineAsync(
142141
ObjectHandle pipelineObjectHandle,
143142
const WGPURenderPipelineDescriptor* descriptor) {
144143
Reserved<WGPURenderPipeline> pipeline;
145-
WIRE_TRY(Objects<WGPURenderPipeline>().Allocate(&pipeline, pipelineObjectHandle,
146-
AllocationState::Reserved));
144+
WIRE_TRY(Allocate(&pipeline, pipelineObjectHandle, AllocationState::Reserved));
147145

148146
auto userdata = MakeUserdata<CreatePipelineAsyncUserData>();
149147
userdata->device = device.AsHandle();

src/dawn/wire/server/ServerInstance.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ WireResult Server::DoInstanceRequestAdapter(Known<WGPUInstance> instance,
4141
ObjectHandle adapterHandle,
4242
const WGPURequestAdapterOptions* options) {
4343
Reserved<WGPUAdapter> adapter;
44-
WIRE_TRY(Objects<WGPUAdapter>().Allocate(&adapter, adapterHandle, AllocationState::Reserved));
44+
WIRE_TRY(Allocate(&adapter, adapterHandle, AllocationState::Reserved));
4545

4646
auto userdata = MakeUserdata<RequestAdapterUserdata>();
4747
userdata->eventManager = eventManager;

src/dawn/wire/server/ServerSurface.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@ WireResult Server::DoSurfaceGetCurrentTexture(Known<WGPUSurface> surface,
3434
Known<WGPUDevice> configuredDevice,
3535
ObjectHandle textureHandle) {
3636
Reserved<WGPUTexture> texture;
37-
WIRE_TRY(Objects<WGPUTexture>().Allocate(&texture, textureHandle, AllocationState::Reserved));
37+
WIRE_TRY(Allocate(&texture, textureHandle, AllocationState::Reserved));
3838

3939
WGPUSurfaceTexture surfaceTexture;
4040
mProcs.surfaceGetCurrentTexture(surface->handle, &surfaceTexture);

0 commit comments

Comments
 (0)