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

CUDA: Fix test_pinned on Jetson #7190

Merged
merged 1 commit into from Jul 28, 2021
Merged

CUDA: Fix test_pinned on Jetson #7190

merged 1 commit into from Jul 28, 2021

Conversation

gmarkall
Copy link
Member

@gmarkall gmarkall commented Jul 6, 2021

Pinning memory with cuMemHostRegister() now appears to be supported, but only for memory regions < 4MB. This commit fixes test_pinned by only attempting to pin a 2MB array on ARM.

Marking as draft for now as there is another test case in Issue #2849 that is failing that I need to check.

Pinning memory with `cuMemHostRegister()` now appears to be supported,
but only for memory regions < 4MB. This commit fixes test_pinned by only
attempting to pin a 2MB array on ARM.
@gmarkall gmarkall added 2 - In Progress CUDA CUDA related issue/PR labels Jul 6, 2021
@gmarkall gmarkall marked this pull request as ready for review July 9, 2021 09:14
@gmarkall
Copy link
Member Author

gmarkall commented Jul 9, 2021

Making this ready for review as it is a complete fix in itself, and I won't have time to look into whether I can reproduce the other (separate) issue.

@stuartarchibald stuartarchibald changed the title CUDA: Fix test_pinned on Jetson [WIP] CUDA: Fix test_pinned on Jetson Jul 9, 2021
@stuartarchibald stuartarchibald added this to the Numba 0.54 RC2 milestone Jul 9, 2021
Comment on lines +24 to +26
count = 262144 # 2MB
else:
count = 2097152 # 16MB
Copy link
Contributor

Choose a reason for hiding this comment

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

Assumes an 8 byte intp? Given a 4 byte intp would lead to a smaller allocation size, I think this is still fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed. All systems that support CUDA are 64-bit anyway.

Copy link
Contributor

@stuartarchibald stuartarchibald left a comment

Choose a reason for hiding this comment

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

Thanks for the patch, changes look on on inspection. Need to run this on hardware to test.

@stuartarchibald stuartarchibald added 4 - Waiting on CI Review etc done, waiting for CI to finish and removed 3 - Ready for Review labels Jul 9, 2021
@sklam
Copy link
Member

sklam commented Jul 27, 2021

BFID numba_smoketest_cuda_yaml_83

@sklam sklam added Pending BuildFarm For PRs that have been reviewed but pending a push through our buildfarm BuildFarm Passed For PRs that have been through the buildfarm and passed and removed Pending BuildFarm For PRs that have been reviewed but pending a push through our buildfarm labels Jul 27, 2021
@sklam
Copy link
Member

sklam commented Jul 27, 2021

BF passed, but i thk we still need a Jetson test(?)

@gmarkall
Copy link
Member Author

BF passed, but i thk we still need a Jetson test(?)

That would be ideal, but it's up to you :-)

@sklam
Copy link
Member

sklam commented Jul 28, 2021

I actually don't have a Xavier to make it work. Since I'm not seeing any new failures, i'm marking this as ready.

@sklam sklam added 5 - Ready to merge Review and testing done, is ready to merge and removed 4 - Waiting on CI Review etc done, waiting for CI to finish labels Jul 28, 2021
@sklam sklam merged commit e5e1031 into numba:master Jul 28, 2021
@gmarkall
Copy link
Member Author

Does this fail on a Jetson Nano? (I don't have one to test)

sklam added a commit to sklam/numba that referenced this pull request Aug 4, 2021
CUDA: Fix test_pinned on Jetson
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5 - Ready to merge Review and testing done, is ready to merge BuildFarm Passed For PRs that have been through the buildfarm and passed CUDA CUDA related issue/PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants