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

fix: do not access UMD-managed SVM-allocation while in-use #141

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

Conversation

MaciejBielski
Copy link
Contributor

@MaciejBielski MaciejBielski commented Jan 22, 2025

It is illegal with UMD-managed migrations to access a SVM-allocation
from CPU side while it is still in-use on GPU side. In such scenario
a CPU-side access triggers data fetch from GPU-side and then any
subsequent GPU-side modification will not be observable on the CPU-side.
With UMD-managed migrations there is no way to tell the GPU that it
should fetch the allocation again from CPU-side before modifying it.

The removed EXPECT_NE statements are not mandatory for the goals of
the tests while their side-effects make things worse.

Signed-off-by: Maciej Bielski [email protected]

It is illegal with UMD-managed migrations to access a SVM-allocation
from CPU side while it is still in-use on GPU side. In such scenario
a CPU-side access triggers data fetch from GPU-side and then any
subsequent GPU-side modification will not be observable on the CPU-side.
With UMD-managed migrations there is no way to tell the GPU that it
should fetch the allocation again from CPU-side before modifying it.

The removed `EXPECT_NE` statements are not mandatory for the goals of
the tests while their side-effects make things worse.

Signed-off-by: Maciej Bielski <[email protected]>
@MaciejBielski MaciejBielski force-pushed the fix-illegal-CPU-access-to-UMD-managed-SVM-allocation branch from a78e94c to 209f08f Compare January 22, 2025 17:25
@MaciejBielski MaciejBielski marked this pull request as ready for review January 22, 2025 17:27
Copy link

@MichalMrozek MichalMrozek left a comment

Choose a reason for hiding this comment

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

fix doesn't look correct, this access is fine if device supports concurrent access for shared usm between host & device which we turn on when we enable kmd migration.

Test needs to check for those caps and do not emit those memcpy if cap is not present

@HoppeMateusz
Copy link
Contributor

the property is : ::ZE_MEMORY_CONCURRENT_ACCESS

zeDeviceGetMemoryAccessProperties(ze_device_handle_t hDevice, ze_device_memory_access_properties_t *pMemAccessProperties)

ze_memory_access_capabilities_t sharedSingleDeviceAllocCapabilities

if the test allocates shared USM.

i think SVM in commit message should be changed to USM.

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.

3 participants