[SYCL][UR] Do not call `urDeviceRetain()` in `device_impl` constructor at all #20065

ldorau · 2025-09-12T10:38:24Z

urDeviceRetain(MDevice) should not be called in the device_impl constructor at all,
because RefCounter is initialized with 1, when the device is created.

It fixes URT-961.

ldorau · 2025-09-15T15:35:14Z

@pbalcer please review

cperkinsintel · 2025-09-15T16:35:37Z

@ldorau - please do not "force push". That messes up the continuity of the reviews on github. If you are having to "force push" it probably means you are doing something wrong on your end.

ldorau · 2025-09-15T16:47:44Z

@ldorau - please do not "force push". That messes up the continuity of the reviews on github. If you are having to "force push" it probably means you are doing something wrong on your end.

OK, so what should I do? Add more commits titled:

Fixes after review no. 1
Fixes after review no. 2
Fixes after review no. 3
...

?
Will they be squashed before merging?

pbalcer · 2025-09-16T07:52:09Z

sycl/source/detail/platform_impl.cpp

+}
+
+device_impl &
+platform_impl::getOrMakeSubDeviceImpl(ur_device_handle_t UrSubDevice) {


please implement a helper function that taken an IsSubDevice bool to eliminate duplication.

ldorau · 2025-09-16T09:14:44Z

@ldorau - please do not "force push". That messes up the continuity of the reviews on github. If you are having to "force push" it probably means you are doing something wrong on your end.

@cperkinsintel OK, a new commit added with fixes

github-actions · 2025-09-16T18:07:30Z

@intel/llvm-gatekeepers please consider merging

againull · 2025-09-16T21:12:51Z

sycl/source/detail/device_impl.cpp

+  if (!IsSubDevice) {
+    // Interoperability Constructor already calls DeviceRetain in
+    // urDeviceCreateWithNativeHandle.
+    getAdapter().call<UrApiKind::urDeviceRetain>(MDevice);
+  }


The destructor device_impl::~device_impl calls urDeviceRelease, which is meant to balance the urDeviceRetain in the constructor. However, after this change, urDeviceRetain is no longer called for subdevices in the constructor, while urDeviceRelease is still invoked for them in the destructor. This imbalance is somewhat concerning. It seems that the actual root cause of the memory leak may lie elsewhere. Could you please clarify what you believe the underlying issue is?

The reason is that when a device is created RefCount is equal 1 without any call to retain(), but it is destroyed when RefCount is equal 0, so there should be one more call to release() than a number of calls to retain() ... or the implementation of class RefCount is wrong ?

class RefCount { public: RefCount(uint32_t count = 1) : Count(count) {} ... uint32_t retain() { return ++Count; } bool release() { return --Count == 0; } void reset(uint32_t value = 1) { Count = value; } private: std::atomic_uint32_t Count; };

...

ur_result_t urDeviceRelease(ur_device_handle_t Device) { // Root devices are destroyed during the piTearDown process. if (Device->isSubDevice()) { if (Device->RefCount.release()) { delete Device; } } return UR_RESULT_SUCCESS; }

Also worth pointing out that only subdevices are really ref counted:

https://github.com/oneapi-src/unified-runtime/blob/cc3c318865438aceba8a967f238fa151994ad811/source/adapters/level_zero/device.cpp#L1737

@ldorau @pbalcer In such case, why we differentiate between root device and sub-device in this case? can we just remove retain from the constructor completely? (only sub-devices being ref counted seems like implementation detail of the adapter)

"(only sub-devices being ref counted seems like implementation detail of the adapter)"

That's the reason. I pointed it out because it's possible that the existing sequence of creates/retains/releases was always broken, and we never noticed because the leak happens in the rare case of subdevices. So in a way, I'm agreeing with your earlier comment.

I am testing not calling urDeviceRetain() in device_impl ctor at all (#20144) - one unittest (sycl/unittests/context_device/DeviceRefCounter.cpp) will have to be changed at least ...

@againull I have removed urDeviceRetain(MDevice) from the device_impl constructor completely.
Please re-review.

github-actions · 2025-09-19T19:04:07Z

@intel/llvm-gatekeepers please consider merging

github-actions · 2025-09-22T19:04:12Z

@intel/llvm-gatekeepers please consider merging

ldorau · 2025-09-22T20:22:20Z

@pbalcer @againull I have removed urDeviceRetain(MDevice) from the device_impl constructor completely.
Please re-review.

urDeviceRetain(MDevice) should not be called in device_impl ctor at all, because RefCounter is initialized with 1, when the device is created. It fixes URT-961. Signed-off-by: Lukasz Dorau <lukasz.dorau@intel.com>

pbalcer · 2025-09-23T08:23:04Z

@pbalcer @againull I have removed urDeviceRetain(MDevice) from the device_impl constructor completely. Please re-review.

I don't know enough about how the SYCL device object is managed to be able to say whether this is correct. But, generally, this makes sense to me - devices (and all other UR objects) start with refcount 1, so retaining them immediately after creation is unnecessary.

againull · 2025-09-23T23:21:05Z

sycl/unittests/context_device/DeviceRefCounter.cpp

-    // So for this test, we just do it.
-    sycl::detail::GlobalHandler::instance().getPlatformCache().clear();
+
+    EXPECT_EQ(DevRefCounter, 0);


It seems a bit off that ref count is zero here, because callback for urDeviceGet (redefinedDevicesGetAfter) increments the DevRefCounter. So, even if we don't call Retain anymore, ref count is still expected to be at least 1 because of urDevicesGet. It would be nice to either understand why it is normal to have zero ref count here or to update the test if necessary if there is something wrong.

@againull I have debugged this unittest. It calls 3 times platform_impl::getDevicesImplHelper():

llvm/sycl/source/detail/platform_impl.cpp

Line 497 in 6b29cf1

void platform_impl::getDevicesImplHelper(ur_device_type_t UrDeviceType,

which calls:

urDeviceGet(NumDevices == 0) what sets DevRefCounter == 0 (using redefinedDevicesGetAfter()):

llvm/sycl/source/detail/platform_impl.cpp

Lines 502 to 504 in 6b29cf1

MAdapter->call<UrApiKind::urDeviceGet>(MPlatform, UrDeviceType,

0u, // CP info::device_type::all

nullptr, &NumDevices);

urDeviceGet(NumDevices == 1) what sets DevRefCounter == 1 (using redefinedDevicesGetAfter()):

llvm/sycl/source/detail/platform_impl.cpp

Lines 527 to 530 in 6b29cf1

MAdapter->call<UrApiKind::urDeviceGet>(

MPlatform,

UrDeviceType, // CP info::device_type::all

NumDevices, UrDevices.data(), nullptr);

urDeviceRelease() on this device what decrements DevRefCounter to 0 (using redefinedDeviceReleaseAfter()):

llvm/sycl/source/detail/platform_impl.cpp

Lines 560 to 563 in 6b29cf1

// The reference counter for handles, that we used to create sycl objects, is

// incremented, so we need to call release here.

for (ur_device_handle_t &UrDev : UrDevicesToCleanUp)

MAdapter->call<UrApiKind::urDeviceRelease>(UrDev);

... what looks like:

- redefinedDevicesGetAfter(*params.pphDevices == 0) DevRefCounter == 0 from urDeviceGet(NumDevices == 0) - redefinedDevicesGetAfter(*params.pNumEntries == 1) DevRefCounter == 1 from urDeviceGet(NumDevices == 1) - redefinedDeviceReleaseAfter() DevRefCounter == 0 from urDeviceRelease() - redefinedDevicesGetAfter(*params.pphDevices == 0) DevRefCounter == 0 from urDeviceGet(NumDevices == 0) - redefinedDevicesGetAfter(*params.pNumEntries == 1) DevRefCounter == 1 from urDeviceGet(NumDevices == 1) - redefinedDeviceReleaseAfter() DevRefCounter == 0 from urDeviceRelease() - redefinedDevicesGetAfter(*params.pphDevices == 0) DevRefCounter == 0 from urDeviceGet(NumDevices == 0) - redefinedDevicesGetAfter(*params.pNumEntries == 1) DevRefCounter == 1 from urDeviceGet(NumDevices == 1) - redefinedDeviceReleaseAfter() DevRefCounter == 0 from urDeviceRelease()

@againull Does it make sense or is there anything wrong here?

ldorau had a problem deploying to WindowsCILock September 12, 2025 10:38 — with GitHub Actions Failure

ldorau temporarily deployed to WindowsCILock September 12, 2025 11:05 — with GitHub Actions Inactive

ldorau force-pushed the Do_not_call_DeviceRetain_in_constructor_of_device_impl branch from b07c1d4 to 51a4b37 Compare September 12, 2025 13:43

ldorau had a problem deploying to WindowsCILock September 12, 2025 13:43 — with GitHub Actions Error

ldorau force-pushed the Do_not_call_DeviceRetain_in_constructor_of_device_impl branch from 51a4b37 to ab4b9b9 Compare September 12, 2025 13:48

ldorau had a problem deploying to WindowsCILock September 12, 2025 13:49 — with GitHub Actions Failure

ldorau changed the title ~~[UR] Do not call urDeviceRetain() in constructor of device_impl~~ [UR] Do not call urDeviceRetain() in case of subdevices Sep 12, 2025

ldorau temporarily deployed to WindowsCILock September 12, 2025 14:15 — with GitHub Actions Inactive

ldorau force-pushed the Do_not_call_DeviceRetain_in_constructor_of_device_impl branch from ab4b9b9 to 7c033d0 Compare September 15, 2025 10:05

ldorau had a problem deploying to WindowsCILock September 15, 2025 10:06 — with GitHub Actions Failure

ldorau force-pushed the Do_not_call_DeviceRetain_in_constructor_of_device_impl branch from 7c033d0 to 054acf1 Compare September 15, 2025 10:15

ldorau temporarily deployed to WindowsCILock September 15, 2025 10:15 — with GitHub Actions Inactive

ldorau temporarily deployed to WindowsCILock September 15, 2025 10:45 — with GitHub Actions Inactive

ldorau force-pushed the Do_not_call_DeviceRetain_in_constructor_of_device_impl branch from 054acf1 to 7eb40c4 Compare September 15, 2025 15:27

ldorau had a problem deploying to WindowsCILock September 15, 2025 15:27 — with GitHub Actions Error

ldorau changed the title ~~[UR] Do not call urDeviceRetain() in case of subdevices~~ [SYCL] Do not call urDeviceRetain() in case of subdevices Sep 15, 2025

ldorau changed the title ~~[SYCL] Do not call urDeviceRetain() in case of subdevices~~ [SYCL] Do not call urDeviceRetain() in case of subdevices Sep 15, 2025

ldorau force-pushed the Do_not_call_DeviceRetain_in_constructor_of_device_impl branch from 7eb40c4 to 1e745dc Compare September 15, 2025 15:33

ldorau temporarily deployed to WindowsCILock September 15, 2025 15:34 — with GitHub Actions Inactive

ldorau marked this pull request as ready for review September 15, 2025 15:34

ldorau requested a review from a team as a code owner September 15, 2025 15:34

ldorau requested a review from againull September 15, 2025 15:34

ldorau temporarily deployed to WindowsCILock September 15, 2025 15:59 — with GitHub Actions Inactive

ldorau force-pushed the Do_not_call_DeviceRetain_in_constructor_of_device_impl branch from 1e745dc to 1ab563c Compare September 15, 2025 16:21

ldorau had a problem deploying to WindowsCILock September 15, 2025 16:21 — with GitHub Actions Error

ldorau force-pushed the Do_not_call_DeviceRetain_in_constructor_of_device_impl branch from 1ab563c to 1344429 Compare September 15, 2025 16:44

ldorau temporarily deployed to WindowsCILock September 15, 2025 16:45 — with GitHub Actions Inactive

ldorau temporarily deployed to WindowsCILock September 15, 2025 17:12 — with GitHub Actions Inactive

ldorau requested a review from cperkinsintel September 16, 2025 07:46

pbalcer reviewed Sep 16, 2025

View reviewed changes

ldorau had a problem deploying to WindowsCILock September 16, 2025 09:09 — with GitHub Actions Error

ldorau force-pushed the Do_not_call_DeviceRetain_in_constructor_of_device_impl branch from b6f2cce to 119f6a2 Compare September 16, 2025 09:12

ldorau temporarily deployed to WindowsCILock September 16, 2025 09:13 — with GitHub Actions Inactive

ldorau requested a review from pbalcer September 16, 2025 09:13

ldorau temporarily deployed to WindowsCILock September 16, 2025 09:43 — with GitHub Actions Inactive

cperkinsintel approved these changes Sep 16, 2025

View reviewed changes

againull reviewed Sep 16, 2025

View reviewed changes

ldorau requested a review from againull September 17, 2025 09:45

ldorau changed the title ~~[SYCL] Do not call urDeviceRetain() in case of subdevices~~ [SYCL][UR] Do not call urDeviceRetain() in device_impl constructor at all Sep 22, 2025

ldorau force-pushed the Do_not_call_DeviceRetain_in_constructor_of_device_impl branch from 119f6a2 to 6396c62 Compare September 22, 2025 20:12

ldorau temporarily deployed to WindowsCILock September 22, 2025 20:13 — with GitHub Actions Inactive

ldorau temporarily deployed to WindowsCILock September 22, 2025 20:40 — with GitHub Actions Inactive

[SYCL][UR] Do not call urDeviceRetain() in device_impl ctor at all

6396c62

urDeviceRetain(MDevice) should not be called in device_impl ctor at all, because RefCounter is initialized with 1, when the device is created. It fixes URT-961. Signed-off-by: Lukasz Dorau <lukasz.dorau@intel.com>

againull reviewed Sep 23, 2025

View reviewed changes

	MAdapter->call<UrApiKind::urDeviceGet>(MPlatform, UrDeviceType,
	0u, // CP info::device_type::all
	nullptr, &NumDevices);

	MAdapter->call<UrApiKind::urDeviceGet>(
	MPlatform,
	UrDeviceType, // CP info::device_type::all
	NumDevices, UrDevices.data(), nullptr);

	// The reference counter for handles, that we used to create sycl objects, is
	// incremented, so we need to call release here.
	for (ur_device_handle_t &UrDev : UrDevicesToCleanUp)
	MAdapter->call<UrApiKind::urDeviceRelease>(UrDev);

[SYCL][UR] Do not call urDeviceRetain() in device_impl constructor at all #20065

Are you sure you want to change the base?

[SYCL][UR] Do not call urDeviceRetain() in device_impl constructor at all #20065

Conversation

ldorau commented Sep 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ldorau commented Sep 15, 2025

Uh oh!

cperkinsintel commented Sep 15, 2025

Uh oh!

ldorau commented Sep 15, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ldorau commented Sep 16, 2025

Uh oh!

github-actions bot commented Sep 16, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

againull Sep 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ldorau Sep 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Sep 19, 2025

Uh oh!

github-actions bot commented Sep 22, 2025

Uh oh!

ldorau commented Sep 22, 2025

Uh oh!

pbalcer commented Sep 23, 2025

Uh oh!

againull Sep 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

[SYCL][UR] Do not call `urDeviceRetain()` in `device_impl` constructor at all #20065

[SYCL][UR] Do not call `urDeviceRetain()` in `device_impl` constructor at all #20065

ldorau commented Sep 12, 2025 •

edited

Loading

againull Sep 17, 2025 •

edited

Loading

ldorau Sep 22, 2025 •

edited

Loading

againull Sep 23, 2025 •

edited

Loading