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

Remove the call to set device flags. #1417

Merged
merged 1 commit into from
Sep 29, 2020

Conversation

WeiqunZhang
Copy link
Member

@WeiqunZhang WeiqunZhang commented Sep 27, 2020

Summary

This is no longer needed and may result in failure due to that the device
has already been set and initialized in another code.

Additional background

Exa-Wind has a code that uses both Kokkos and AMReX. It seems that
the call to cudaSetDeviceFlags fails because of that.

Checklist

The proposed changes:

  • fix a bug or incorrect behavior in AMReX
  • add new capabilities to AMReX
  • changes answers in the test suite to more than roundoff level
  • are likely to significantly affect the results of downstream AMReX users
  • are described in the proposed changes to the AMReX documentation, if appropriate

@kngott
Copy link
Contributor

kngott commented Sep 27, 2020

I feel like this should be done more robustly, especially since other libraries could do something similar but fail to include our required flags. This could make an error that we can't track down very easily.

We should check to see if this returns "cudaErrorSetOnActiveProcess", and if so call "cudaGetDeviceFlags" to make sure "cudaDeviceMapHost" is on. If not, throw an error stating that we require it or throw a warning and use "cudaDeviceReset" to do the setup ourselves with their flags plus ours.

@maxpkatz: That sound right? (Also, do we still need this flag / is this flag still doing the same thing since this setup was built?)

@maximumcats
Copy link
Member

I suggest we just remove the call to this API. On any platform that users realistically have access to today (>= CUDA 4, >= Fermi architecture), pinned memory is automatically part of a unified address space and automatically supported through the runtime API. I suspect this was added by someone reading the docs and thinking it would be needed just to be safe, but I don't think it's needed.

@WeiqunZhang
Copy link
Member Author

OK. That would be the easiest!

This is no longer needed and may result in failure due to that the device
has already been set and initialized in another code.
@WeiqunZhang WeiqunZhang changed the title Remove error check from call to set device flags. Remove the call to set device flags. Sep 27, 2020
@WeiqunZhang WeiqunZhang merged commit b7ef5a6 into AMReX-Codes:development Sep 29, 2020
@WeiqunZhang WeiqunZhang deleted the set_device_flags branch September 29, 2020 15:24
dwillcox pushed a commit to dwillcox/amrex that referenced this pull request Oct 3, 2020
## Summary
This is no longer needed and may result in failure due to that the device
has already been set and initialized in another code.

## Additional background
Exa-Wind has a code that uses both Kokkos and AMReX.  It seems that
the call to cudaSetDeviceFlags fails because of that.

## Checklist

The proposed changes:
- [ ] fix a bug or incorrect behavior in AMReX
- [ ] add new capabilities to AMReX
- [ ] changes answers in the test suite to more than roundoff level
- [ ] are likely to significantly affect the results of downstream AMReX users
- [ ] are described in the proposed changes to the AMReX documentation, if appropriate
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