Skip to content

Commit

Permalink
[Offload] Properly guard modifications to the RPC device array (#126790)
Browse files Browse the repository at this point in the history
Summary:
If the user deallocates an RPC device this can sometimes fail if the RPC
server is still running. This will happen if the modification happens
while the server is still checking it. This patch adds a mutex to guard
modifications to it.

(cherry picked from commit baf7a3c)
  • Loading branch information
jhuber6 authored and llvmbot committed Feb 11, 2025
1 parent 553185b commit c03f46f
Show file tree
Hide file tree
Showing 2 changed files with 13 additions and 4 deletions.
12 changes: 9 additions & 3 deletions offload/plugins-nextgen/common/include/RPC.h
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,9 @@ struct RPCServerTy {
/// Array of associated devices. These must be alive as long as the server is.
std::unique_ptr<plugin::GenericDeviceTy *[]> Devices;

/// Mutex that guards accesses to the buffers and device array.
std::mutex BufferMutex{};

/// A helper class for running the user thread that handles the RPC interface.
/// Because we only need to check the RPC server while any kernels are
/// working, we track submission / completion events to allow the thread to
Expand All @@ -90,6 +93,9 @@ struct RPCServerTy {
std::condition_variable CV;
std::mutex Mutex;

/// A reference to the main server's mutex.
std::mutex &BufferMutex;

/// A reference to all the RPC interfaces that the server is handling.
llvm::ArrayRef<void *> Buffers;

Expand All @@ -98,9 +104,9 @@ struct RPCServerTy {

/// Initialize the worker thread to run in the background.
ServerThread(void *Buffers[], plugin::GenericDeviceTy *Devices[],
size_t Length)
: Running(false), NumUsers(0), CV(), Mutex(), Buffers(Buffers, Length),
Devices(Devices, Length) {}
size_t Length, std::mutex &BufferMutex)
: Running(false), NumUsers(0), CV(), Mutex(), BufferMutex(BufferMutex),
Buffers(Buffers, Length), Devices(Devices, Length) {}

~ServerThread() { assert(!Running && "Thread not shut down explicitly\n"); }

Expand Down
5 changes: 4 additions & 1 deletion offload/plugins-nextgen/common/src/RPC.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,7 @@ void RPCServerTy::ServerThread::run() {
Lock.unlock();
while (NumUsers.load(std::memory_order_relaxed) > 0 &&
Running.load(std::memory_order_relaxed)) {
std::lock_guard<decltype(Mutex)> Lock(BufferMutex);
for (const auto &[Buffer, Device] : llvm::zip_equal(Buffers, Devices)) {
if (!Buffer || !Device)
continue;
Expand All @@ -149,7 +150,7 @@ RPCServerTy::RPCServerTy(plugin::GenericPluginTy &Plugin)
Devices(std::make_unique<plugin::GenericDeviceTy *[]>(
Plugin.getNumDevices())),
Thread(new ServerThread(Buffers.get(), Devices.get(),
Plugin.getNumDevices())) {}
Plugin.getNumDevices(), BufferMutex)) {}

llvm::Error RPCServerTy::startThread() {
Thread->startThread();
Expand Down Expand Up @@ -190,13 +191,15 @@ Error RPCServerTy::initDevice(plugin::GenericDeviceTy &Device,
if (auto Err = Device.dataSubmit(ClientGlobal.getPtr(), &client,
sizeof(rpc::Client), nullptr))
return Err;
std::lock_guard<decltype(BufferMutex)> Lock(BufferMutex);
Buffers[Device.getDeviceId()] = RPCBuffer;
Devices[Device.getDeviceId()] = &Device;

return Error::success();
}

Error RPCServerTy::deinitDevice(plugin::GenericDeviceTy &Device) {
std::lock_guard<decltype(BufferMutex)> Lock(BufferMutex);
Device.free(Buffers[Device.getDeviceId()], TARGET_ALLOC_HOST);
Buffers[Device.getDeviceId()] = nullptr;
Devices[Device.getDeviceId()] = nullptr;
Expand Down

0 comments on commit c03f46f

Please sign in to comment.