Skip to content

Commit

Permalink
Revert "Release any queues left when atexit is called."
Browse files Browse the repository at this point in the history
This reverts commit 720cab7.
This commit tried to work around a dpc++ issue where temporary queues
were not deleted. However it was possible for queues to be released
from global destructors which could cause a segfault. Reverting the
workaround and pushing for dpc++ to fix the issue.
  • Loading branch information
coldav committed Oct 25, 2023
1 parent 06f4989 commit b58fe61
Show file tree
Hide file tree
Showing 4 changed files with 1 addition and 50 deletions.
23 changes: 0 additions & 23 deletions source/cl/include/cl/device.h
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,6 @@
#include <compiler/info.h>
#include <mux/mux.h>

#include <mutex>
#include <set>
#include <string>

/// @addtogroup cl
Expand All @@ -57,25 +55,6 @@ struct _cl_device_id final : public cl::base<_cl_device_id> {
/// @brief Destructor.
~_cl_device_id();

/// @brief Register a command queue with the device
/// This should usually be called on creation of the queue
void RegisterCommandQueue(cl_command_queue queue);

/// @brief Deregister a command queue with the device
/// This should usually be called on deletion of the queue
void DeregisterCommandQueue(cl_command_queue queue);

/// @brief Release any external queues that are still around.
/// This should only be called when we know that the application
/// is no longer in a position to do so e.g. at exit
/// @note This is to workaround an issue with dpc++ where temporary queues
/// can be left at exit if out of order queues are not supported.
/// This should be reviewed when https://github.com/intel/llvm/issues/11156 is
/// resolved.
void ReleaseAllExternalQueues();

std::set<cl_command_queue> registered_queues;

/// @brief Platform the device belongs to.
cl_platform_id platform;
/// @brief Mux allocator info.
Expand Down Expand Up @@ -373,8 +352,6 @@ struct _cl_device_id final : public cl::base<_cl_device_id> {
/// TODO: Should probably be a core property, see CA-2717.
size_t preferred_work_group_size_multiple;
#endif
// Used to keep the registering of queues thread safe
std::mutex device_lock;
};

/// @}
Expand Down
2 changes: 0 additions & 2 deletions source/cl/source/command_queue.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,6 @@ _cl_command_queue::_cl_command_queue(cl_context context, cl_device_id device,
in_flush(false) {
cl::retainInternal(context);
cl::retainInternal(device);
{ device->RegisterCommandQueue(this); }
}

_cl_command_queue::~_cl_command_queue() {
Expand Down Expand Up @@ -87,7 +86,6 @@ _cl_command_queue::~_cl_command_queue() {
muxDestroyQueryPool(mux_queue, counter_queries, device->mux_allocator);
}

device->DeregisterCommandQueue(this);
cl::releaseInternal(device);
cl::releaseInternal(context);
}
Expand Down
23 changes: 0 additions & 23 deletions source/cl/source/device.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
#include <cargo/string_algorithm.h>
#include <cargo/string_view.h>
#include <cl/binary/binary.h>
#include <cl/command_queue.h>
#include <cl/config.h>
#include <cl/device.h>
#include <cl/limits.h>
Expand Down Expand Up @@ -331,28 +330,6 @@ _cl_device_id::_cl_device_id(cl_platform_id platform,
#endif
}

void _cl_device_id::RegisterCommandQueue(cl_command_queue queue) {
std::lock_guard<std::mutex> lock(device_lock);
registered_queues.insert(queue);
}

void _cl_device_id::DeregisterCommandQueue(cl_command_queue queue) {
std::lock_guard<std::mutex> lock(device_lock);
registered_queues.erase(queue);
}

void _cl_device_id::ReleaseAllExternalQueues() {
// Need to copy as it will deregister as it goes along
auto queues = registered_queues;
for (auto q : queues) {
auto external_count = q->refCountExternal();
for (cl_uint i = 0; i < external_count; i++) {
cl::ReleaseCommandQueue(q);
}
}
registered_queues.clear();
}

_cl_device_id::~_cl_device_id() {
muxDestroyDevice(mux_device, mux_allocator);
cl::releaseInternal(platform);
Expand Down
3 changes: 1 addition & 2 deletions source/cl/source/platform.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -129,12 +129,11 @@ cargo::expected<cl_platform_id, cl_int> _cl_platform_id::getInstance() {

#if !defined(CA_PLATFORM_WINDOWS)
// Add an atexit handler to destroy the cl_platform_id. This is not done on
// Windows because DLL's which we rely on are not guaranteed to be loaded
// Windows because DLL's which we rely on are not guarenteed to be loaded
// when atexit handlers are invoked, the advice given by Microsoft is not
// to perform any tear down at all.
atexit([]() {
for (auto device : platform.value()->devices) {
device->ReleaseAllExternalQueues();
cl::releaseInternal(device);
}
cl::releaseInternal(platform.value());
Expand Down

0 comments on commit b58fe61

Please sign in to comment.