Skip to content
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

[ur] Introduce virtual memory interfaces #525

Merged
merged 1 commit into from
Jun 19, 2023

Conversation

kbenzie
Copy link
Contributor

@kbenzie kbenzie commented May 18, 2023

Fixes #357 by introducing a port of the PI changes from intel/llvm#8954
which add the ability to reserve virtual memory regions separately from
physical memory backing allocations.

Copy link

@jandres742 jandres742 left a comment

Choose a reason for hiding this comment

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

+1

@vinser52
Copy link
Contributor

vinser52 commented Jun 2, 2023

The question just raised in my mind:

For USM we are using Unified Memory Allocation Framework to organize pooling. In the future, we are considering this pooling framework as a common point for all allocations to improve interop with other runtimes. For example, MPI can benefit from it by using our pooling framework to get info about where memory (pointed by virtual address) resides.
The question is how this API correlated with the above-mentioned.

@kbenzie
Copy link
Contributor Author

kbenzie commented Jun 2, 2023

@vinser52 while porting the PI changes over to UR I was pondering how this might be more tightly integrated into the USM interfaces. I did wonder if it would be possible to use USM allocations as the backing for the virtual memory reservations but I think there are some complications with the need to track the adapter specific state lifetimes (e.g CUmemGenericAllocationHandle in CUDA) with ur_physical_mem_handle_t.

Would it make sense for ownership of adapter specific handles to be managed in a ur_usm_pool_handle_t?

Then maybe we could be to include a ur_usm_pool_handle_t as an optional parameter to urVirtualMemReserve.

I'm really not sure if this is feasible though, our hands are somewhat tied due to the CUDA interfaces.

I'd like to hear @steffenlarsen's thoughts on this.

@steffenlarsen
Copy link
Contributor

I share @kbenzie's concern regarding this idea, given the relation between this and the CUDA interfaces. I am not sure we can always make the association at reservation time either, as I don't believe there is any guarantee about USM-like behavior of the reserved pointer until it is mapped to the physical memory.

Copy link
Contributor

@steffenlarsen steffenlarsen left a comment

Choose a reason for hiding this comment

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

Interfaces look good!

include/ur.py Show resolved Hide resolved
@kbenzie kbenzie force-pushed the benie/virtual-memory branch 2 times, most recently from 59455ab to effd833 Compare June 9, 2023 16:11
@kbenzie kbenzie marked this pull request as ready for review June 9, 2023 16:13
@igchor
Copy link
Member

igchor commented Jun 15, 2023

LGTM, I don't have any good idea for integrating that with the USM interfaces for now. We'll need to think about how to best expose UMF memory providers from UR - then we could implement the memory reservation using UMF providers and make them discoverable

Fixes oneapi-src#357 by introducing a port of the PI changes from intel/llvm#8954
which add the ability to reserve virtual memory regions separately from
physical memory backing allocations.

* [x] Define interfaces in the spec & header
* [ ] Add conformance tests exercising interfaces - tracked in oneapi-src#525
@kbenzie
Copy link
Contributor Author

kbenzie commented Jun 19, 2023

@vinser52 @igchor I've opened #635 to track future work resulting from this PR.

@kbenzie kbenzie merged commit a24d904 into oneapi-src:main Jun 19, 2023
###############################################################################
## @brief Virtual memory access mode flags.
class ur_virtual_mem_access_flags_v(IntEnum):
READ_WRITE = UR_BIT(0) ## Virtual memory both read and write accessible
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for the late reaction to this, but I just realized that we are losing the ability to set the access mode to "none" here by defining READ_WRITE as 0. CUDA considers an access mode of 0 as revoking access. @kbenzie - Could we either have a NONE value here or offset the existing ones?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Missed that! Yeah, I'll add none.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#638 will fix this.

kbenzie added a commit to kbenzie/unified-runtime that referenced this pull request Jun 20, 2023
Address [feedback](oneapi-src#525 (comment))
and add `UR_VIRTUAL_MEM_ACCESS_FLAG_NONE` to allow specifying a 0 value
for the access flags.
kbenzie added a commit to kbenzie/unified-runtime that referenced this pull request Jun 20, 2023
Address [feedback](oneapi-src#525 (comment))
and add `UR_VIRTUAL_MEM_ACCESS_FLAG_NONE` to allow specifying a 0 value
for the access flags.
kbenzie added a commit to kbenzie/unified-runtime that referenced this pull request Jun 20, 2023
Address [feedback](oneapi-src#525 (comment))
and add `UR_VIRTUAL_MEM_ACCESS_FLAG_NONE` to allow specifying no access.
@steffenlarsen
Copy link
Contributor

steffenlarsen commented Jul 20, 2023

@kbenzie - Sorry for the (very) late realization, but I cannot find any analogue for PI_EXT_ONEAPI_DEVICE_INFO_SUPPORTS_VIRTUAL_MEM in these. Am I blind or was it overlooked?

@kbenzie
Copy link
Contributor Author

kbenzie commented Jul 20, 2023

@kbenzie - Sorry for the (very) late realization, but I cannot find any analogue for PI_EXT_ONEAPI_DEVICE_INFO_SUPPORTS_VIRTUAL_MEM in these. Am I blind or was it overlooked?

I do not believe you are blind, I think it was overlooked. I've opened #737 to add it.

@kbenzie kbenzie deleted the benie/virtual-memory branch January 29, 2024 13:48
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.

Expose Low-Level Memory Management API (with Virtual Memory reservation)
7 participants