Skip to content

[SYCL][UR][L0] Fix urTearDown multiple call crash #10512

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
bratpiorka wants to merge 1 commit into from
Closed

[SYCL][UR][L0] Fix urTearDown multiple call crash #10512

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
14 changes: 10 additions & 4 deletions sycl/plugins/unified_runtime/ur/adapters/level_zero/platform.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,17 @@ UR_APIEXPORT ur_result_t UR_APICALL urTearDown(
) {
std::ignore = Params;
// reclaim pi_platform objects here since we don't have piPlatformRelease.
for (ur_platform_handle_t Platform : *PiPlatformsCache) {
delete Platform;

if (PiPlatformsCache) {
Copy link
Contributor

@jandres742 jandres742 Jul 21, 2023

Choose a reason for hiding this comment

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

thanks @bratpiorka

@callumfare : does this conflict with your PR in #10349 ? would it be safe to merge this and then to rebase yours on top of it?

Copy link
Contributor

@callumfare callumfare Jul 21, 2023

Choose a reason for hiding this comment

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

This will conflict but it should be a simple fix, I can rebase my PR when this is merged

Copy link
Contributor

@pbalcer pbalcer Jul 21, 2023

Choose a reason for hiding this comment

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

I don't think this patch will be necessary after your changes @callumfare since urTeardown will be an empty function and the actual teardown code (what this patch is changing) will be protected with a reference count.

Copy link
Contributor

@callumfare callumfare Jul 21, 2023

Choose a reason for hiding this comment

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

That's a good point, the case of multiple teardowns won't be a problem any more.

One related scenario I've just thought of is when the adapter is fully released and the reference count is 0, but is then reinitialized with a call to urAdapterGet again. The teardown should leave the adapter in a state where it's safe to call the setup code again. I'm not sure if this PR will ensure that but maybe that's something to consider in a future change.

Copy link
Contributor

@jandres742 jandres742 Jul 25, 2023

Choose a reason for hiding this comment

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

thanks @callumfare , @pbalcer . So, do we want to not merge this patch in favor of yours?

Copy link
Contributor

@pbalcer pbalcer Jul 25, 2023

Choose a reason for hiding this comment

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

Yes, that's my recommendation.

@callumfare We should definitely have wording to that effect in the docs for related functions, with a matching CTS scenario.

Copy link
Contributor Author

@bratpiorka bratpiorka Jul 25, 2023

Choose a reason for hiding this comment

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

Ok, so I'll close this PR, thanks for the clarification

for (ur_platform_handle_t Platform : *PiPlatformsCache) {
delete Platform;
}

delete PiPlatformsCache;
PiPlatformsCache = nullptr;
delete PiPlatformsCacheMutex;
PiPlatformsCacheMutex = nullptr;
}
delete PiPlatformsCache;
delete PiPlatformsCacheMutex;

bool LeakFound = false;

Expand Down