Skip to content

[SYCL][Bindless][UR][L0][E2E] Fix linear interop memory and L0 V1 adapter leaks. #18353

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

Merged
merged 16 commits into from
May 9, 2025
Merged

[SYCL][Bindless][UR][L0][E2E] Fix linear interop memory and L0 V1 adapter leaks. #18353

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
9ee064e
[SYCL][Bindless][UR][L0][E2E] Fix mapping linear interop memory. Fix …
przemektmalon May 2, 2025
c67d38b
Fix HIP UR adapter compilation
przemektmalon May 7, 2025
0b0e9ad
Add back REQUIRES vulkan directive
przemektmalon May 8, 2025
5151982
Rename external mem APIs from to . Clarify spec.
przemektmalon May 8, 2025
ff997d0
Format
przemektmalon May 8, 2025
f26ef83
Add linux symbols
przemektmalon May 8, 2025
cde614d
Add Windows symbols
przemektmalon May 8, 2025
eb8bb39
Make sycl::queue variants inline, removing SYCL_EXPORT
przemektmalon May 8, 2025
b4583ed
Use static_cast in UR
przemektmalon May 8, 2025
7cd0172
Replace static_cast with reinterpret_cast
przemektmalon May 8, 2025
366c783
Merge branch 'sycl' into przemek/vk-buffer-usm-interop
przemektmalon May 8, 2025
b9e9be7
Format ur_trcddi.cpp
przemektmalon May 8, 2025
532932c
Merge branch 'sycl' into przemek/vk-buffer-usm-interop
przemektmalon May 8, 2025
a85fdc2
Merge branch 'sycl' into przemek/vk-buffer-usm-interop
przemektmalon May 9, 2025
c5be140
Merge branch 'sycl' into przemek/vk-buffer-usm-interop
przemektmalon May 9, 2025
3486511
Merge branch 'sycl' into przemek/vk-buffer-usm-interop
przemektmalon May 9, 2025
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
21 changes: 15 additions & 6 deletions sycl/doc/extensions/experimental/sycl_ext_oneapi_bindless_images.asciidoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2166,14 +2166,26 @@ and `mipmap`. Attempting to import other image types will result in undefined
behaviour.

Once a user has finished operating on imported memory, they must ensure that
they destroy the imported memory handle through `release_external_memory`.
they free the mapped memory and release the external memory handle.

`release_external_memory` can only accept `external_mem` objects that were
created through `import_external_memory`.
Memory mapped using `map_external_image_memory` should be freed using
`free_image_mem`.

Memory mapped using `map_external_linear_memory` should be freed using
`free_mapped_linear_memory`.
Copy link
Contributor

Choose a reason for hiding this comment

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

It irks me that free_image_mem uses mem while all these other APIs use memory. I suppose it's a little late to address that though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it is unfortunate. alloc_image_mem is in the same boat. We should address this in the future, perhaps by deprecating and phasing out the _mem function naming first. Not something for this PR though.

Copy link
Contributor

Choose a reason for hiding this comment

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

If the intention is that the user is supposed to call release_external_memory after calling one of these, then I would prefer the following names instead: unmap_external_image_memory and unmap_external_linear_memory, where the former would be a new API. It is less overloading of the term "free" and makes a much stronger association with the map_* APIs they are associated with.

If we want to go that way, I think these deserve a separate section, instead of being bundled together with release_external_memory.

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've amended the naming to follow your suggestion. The unmap_ and release_ functions now also have their separate code sections.


Imported external memory handle should be released using
`release_external_memory`.
Copy link
Contributor

Choose a reason for hiding this comment

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

From these, it is unclear to me whether a user needs to call both free_mapped_linear_memory/free_image_mem and release_external_memory or just one. I.e., say I have:

external_mem EM = import_external_memory(...);
void *MEIM = map_external_image_memory(EM, ...);

...

free_image_mem(MEIM, ...);
release_external_memory(EM, ...);

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've clarified the wording in the spec. It should hopefully be clear now that the user should first unmap_ the mapped memory, and then call release_external_memory.

I've also added a paragraph stating that an external_mem handle can only be mapped to a single image_mem_handle or void * at any one time. While this use case should be possible, at least for mapping multiple linear memory regions, it is currently untested, so I am reluctant to state that it is possible (and I do not want to leave this underspecified in the spec). We will need to consider this use case in a future PR, ensure that all backends support what we need (or introduce device queries if necessary), and amend the spec with an accompanying test case.


```cpp
namespace sycl::ext::oneapi::experimental {

void free_mapped_linear_memory(void *mappedLinearRegion,
const sycl::device &syclDevice,
const sycl::context &syclContext);
void free_mapped_linear_memory(void *mappedLinearRegion,
const sycl::queue &syclQueue);

void release_external_memory(external_mem externalMem,
const sycl::device &syclDevice,
const sycl::context &syclContext);
Expand All @@ -2182,9 +2194,6 @@ void release_external_memory(external_mem externalMem,
}
```

Destroying or freeing any imported memory through `image_mem_free` or
`sycl::free` will result in undefined behavior.

=== Importing external semaphores [[importing_external_semaphores]]

In addition to proposing importation of external memory resources, we also
Expand Down
20 changes: 20 additions & 0 deletions sycl/include/sycl/ext/oneapi/bindless_images.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -285,6 +285,26 @@ __SYCL_EXPORT void release_external_memory(external_mem externalMem,
__SYCL_EXPORT void release_external_memory(external_mem externalMem,
const sycl::queue &syclQueue);

/**
* @brief Free mapped linear memory region
*
* @param mappedLinearRegion Pointer to the mapped memory region to free
* @param syclQueue The queue in which the external memory was created
*/
__SYCL_EXPORT void free_mapped_linear_memory(void *mappedLinearRegion,
const sycl::device &syclDevice,
const sycl::context &syclContext);

/**
* @brief Free mapped linear memory region
*
* @param mappedLinearRegion Pointer to the mapped memory region to free
* @param syclDevice The device in which the external memory was created
* @param syclContext The context in which the external memory was created
*/
__SYCL_EXPORT void free_mapped_linear_memory(void *mappedLinearRegion,
const sycl::queue &syclQueue);

/**
* @brief Create an image and return the device image handle
*
Expand Down
17 changes: 17 additions & 0 deletions sycl/source/detail/bindless_images.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -546,6 +546,23 @@ __SYCL_EXPORT void release_external_memory(external_mem extMem,
syclQueue.get_context());
}

__SYCL_EXPORT void free_mapped_linear_memory(void *mappedLinearRegion,
const sycl::device &syclDevice,
const sycl::context &syclContext) {
auto [urDevice, urCtx, Adapter] = get_ur_handles(syclDevice, syclContext);

Adapter->call<
sycl::errc::invalid,
sycl::detail::UrApiKind::urBindlessImagesFreeMappedLinearMemoryExp>(
urCtx, urDevice, mappedLinearRegion);
}

__SYCL_EXPORT void free_mapped_linear_memory(void *mappedLinearRegion,
const sycl::queue &syclQueue) {
free_mapped_linear_memory(mappedLinearRegion, syclQueue.get_device(),
syclQueue.get_context());
}

template <>
__SYCL_EXPORT external_semaphore import_external_semaphore(
external_semaphore_descriptor<resource_fd> externalSemaphoreDesc,
Expand Down
Loading
Loading