Skip to content

[SYCL] piDeviceRelease resource leak fix #2668

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

[SYCL] piDeviceRelease resource leak fix #2668

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 15 additions & 7 deletions sycl/source/detail/device_impl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,9 @@ device_impl::device_impl(pi_native_handle InteropDeviceHandle,
Platform = platform_impl::getPlatformFromPiDevice(MDevice, Plugin);
}
MPlatform = Platform;

// set MPlugin
MPlugin = std::make_shared<plugin>(Plugin);
}

device_impl::~device_impl() {
Expand Down Expand Up @@ -101,7 +104,10 @@ cl_device_id device_impl::get() const {
}

platform device_impl::get_platform() const {
return createSyclObjFromImpl<platform>(MPlatform);
if (PlatformImplPtr Platform = MPlatform.lock())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's not very clear what should be done if a platform is released before a device bound to this platform.
Can we do vice versa - device has a shared_ptr to platform, platform has a weak_ptr to device?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can try it.
The platform has multiple owners "higher up", like the context and the GlobalHandler. So it isn't affected if the device has a weak_ptr to it. But I'm less certain for the devices. I think the platform might be the primary owner, so if it's a weak pointer, then the device might be released before the platform is done with it. Also, the code has a clear chain of ownership now: context->platform->device, it seems unwise to meddle with it.

I would prefer that instead of returning just platform() in the else case, that we throw an exception - because no one should be asking for the platform after it has been released. However, that would be an ABI change, and I wanted to avoid anything controversial.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed this PR to draft, and opened a new one here: #2671

return createSyclObjFromImpl<platform>(Platform);
else
return platform();
}

bool device_impl::has_extension(const string_class &ExtensionName) const {
Expand Down Expand Up @@ -139,12 +145,14 @@ device_impl::create_sub_devices(const cl_device_partition_property *Properties,
// times with the same arguments?
//
vector_class<device> res;
std::for_each(SubDevices.begin(), SubDevices.end(),
[&res, this](const RT::PiDevice &a_pi_device) {
device sycl_device = detail::createSyclObjFromImpl<device>(
MPlatform->getOrMakeDeviceImpl(a_pi_device, MPlatform));
res.push_back(sycl_device);
});
if (PlatformImplPtr Platform = MPlatform.lock()) {
std::for_each(SubDevices.begin(), SubDevices.end(),
[&res, this, &Platform](const RT::PiDevice &a_pi_device) {
device sycl_device = detail::createSyclObjFromImpl<device>(
Platform->getOrMakeDeviceImpl(a_pi_device, Platform));
res.push_back(sycl_device);
});
}
return res;
}

Expand Down
13 changes: 11 additions & 2 deletions sycl/source/detail/device_impl.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ namespace detail {
// Forward declaration
class platform_impl;
using PlatformImplPtr = std::shared_ptr<platform_impl>;
using PlatformImplWeakPtr = std::weak_ptr<platform_impl>;

// TODO: Make code thread-safe
class device_impl {
Expand Down Expand Up @@ -119,7 +120,14 @@ class device_impl {
platform get_platform() const;

/// \return the associated plugin with this device.
const plugin &getPlugin() const { return MPlatform->getPlugin(); }
const plugin &getPlugin() const {
if (PlatformImplPtr Platform = MPlatform.lock())
return Platform->getPlugin(); // return whatever plugin the platform has
// (might be changed by unit testing Mocks)
else
return *MPlugin; // return the plugin with which this device was
// initialized.
}

/// Check SYCL extension support by device
///
Expand Down Expand Up @@ -229,7 +237,8 @@ class device_impl {
RT::PiDeviceType MType;
bool MIsRootDevice = false;
bool MIsHostDevice;
PlatformImplPtr MPlatform;
PlatformImplWeakPtr MPlatform;
std::shared_ptr<plugin> MPlugin;
}; // class device_impl

} // namespace detail
Expand Down
1 change: 1 addition & 0 deletions sycl/test/basic_tests/queue/release.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,3 +19,4 @@ int main() {
//CHECK: ---> piContextRelease(
//CHECK: ---> piKernelRelease(
//CHECK: ---> piProgramRelease(
//CHECK: ---> piDeviceRelease(