Skip to content

[UR][L0 v2] Port USM alloc to adapter v2 #18179

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.

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

Open
wants to merge 1 commit into
base: sycl
Choose a base branch
from

Conversation

staniewzki
Copy link
Contributor

@staniewzki staniewzki commented Apr 24, 2025

This PR ports USM alloc enqueue API introduced to L0 adapter in #17112 to L0 adapter v2.

@staniewzki staniewzki force-pushed the async-l0-adapter-v2 branch 2 times, most recently from 8fc9615 to 4ac080d Compare April 30, 2025 09:43
@staniewzki staniewzki changed the title [UR][L0 v2][draft] Port USM alloc to adapter v2 [UR][L0 v2] Port USM alloc to adapter v2 Apr 30, 2025
@staniewzki staniewzki marked this pull request as ready for review April 30, 2025 11:08
@staniewzki staniewzki requested a review from a team as a code owner April 30, 2025 11:08
@staniewzki staniewzki force-pushed the async-l0-adapter-v2 branch 2 times, most recently from a25c7e7 to 803bf54 Compare May 5, 2025 08:41
@igchor
Copy link
Member

igchor commented May 5, 2025

@@ -150,6 +150,8 @@ ur_result_t ur_queue_immediate_in_order_t::queueGetNativeHandle(
ur_result_t ur_queue_immediate_in_order_t::queueFinish() {
TRACK_SCOPE_LATENCY("ur_queue_immediate_in_order_t::queueFinish");

hContext->getAsyncPool()->cleanupPoolsForQueue(this);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm considering the following scenario: enqueueAlloc->enqueue(Kernel/USMFill)->enqueueFree->queueFinish.

In this case, enqueueFree immediately places the allocation into the freelist along with the associated free event handle. When enqueueAlloc retrieves the allocation from the freelist, it correctly appends a wait on the related free event, so that part works as expected.

But, during queueFinish, the freelist cleanup starts before any synchronization occurs, which may free allocations that are still in use. I think we haven’t seen test failures because the allocations are returned to the UMF pool, which doesn’t release the memory immediately.

Maybe we should move the cleanup after ZE2UR_CALL(zeCommandListHostSynchronize, (commandListLocked->getZeCommandList(), UINT64_MAX));.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch! :D We need to fix this in L0v1 as well (and cherry-pick this to xmain-rel).

@staniewzki staniewzki force-pushed the async-l0-adapter-v2 branch from ed39eb4 to bcde052 Compare May 6, 2025 17:27
@staniewzki staniewzki requested a review from a team as a code owner May 6, 2025 17:27
@staniewzki staniewzki requested a review from slawekptak May 6, 2025 17:27
@staniewzki staniewzki force-pushed the async-l0-adapter-v2 branch from bcde052 to 80ab937 Compare May 6, 2025 17:32
@staniewzki staniewzki force-pushed the async-l0-adapter-v2 branch from 80ab937 to a4e8e50 Compare May 7, 2025 08:13

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants