This repository has been archived by the owner on Nov 17, 2023. It is now read-only.
Do not touch GPU 0 during ReleaseAll #14550
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lebeg
reviewed
Mar 28, 2019
src/storage/pooled_storage_manager.h
Outdated
| @@ -54,10 +54,11 @@ class GPUPooledStorageManager final : public StorageManager { | |||
| /*! | |||
| * \brief Default constructor. | |||
| */ | |||
| GPUPooledStorageManager() { | |||
| GPUPooledStorageManager(int dev_id) { | |||
There was a problem hiding this comment.
Maybe pass the context instead? And call it creation_ or initial_context? And maybe add a short explanation to the parameters documentation?
|
@mxnet-label-bot add [Backend, pr-awaiting-review] |
marcoabreu
added
Backend
yuxihu
approved these changes
Mar 28, 2019
yuxihu
reviewed
Mar 28, 2019
src/storage/pooled_storage_manager.h
Outdated
| @@ -123,6 +126,8 @@ class GPUPooledStorageManager final : public StorageManager { | |||
| int reserve_; | |||
| // number of devices | |||
| const size_t NDEV = 32; | |||
| // context used by this Storage Manager | |||
| Context initial_context_; | |||
There was a problem hiding this comment.
One more comment: make it const?
src/storage/pooled_storage_manager.h
Outdated
| @@ -290,6 +299,8 @@ class GPUPooledRoundedStorageManager final : public StorageManager { | |||
| size_t cut_off_; | |||
| // percentage of reserved memory | |||
| int reserve_; | |||
| // context used by this Storage Manager | |||
| Context initial_context_; | |||
|
The CI failure seems completely unrelated. |
yuxihu
approved these changes
Mar 29, 2019
anirudh2290
approved these changes
Mar 29, 2019
|
@mxnet-label-bot update [Backend, pr-awaiting-merge] |
marcoabreu
added
pr-awaiting-merge
apeforest
approved these changes
Mar 30, 2019
|
The PR has been merged. Thank you for the fix! |
ZhennanQin
pushed a commit
to ZhennanQin/incubator-mxnet
that referenced
this pull request
Apr 3, 2019
* Do not touch GPU 0 during ReleaseAll * Fixing lint and fixes from review * Fix * Fixes from review
nswamy
pushed a commit
that referenced
this pull request
Apr 5, 2019
* Do not touch GPU 0 during ReleaseAll * Fixing lint and fixes from review * Fix * Fixes from review
haohuanw
pushed a commit
to haohuanw/incubator-mxnet
that referenced
this pull request
Jun 23, 2019
* Do not touch GPU 0 during ReleaseAll * Fixing lint and fixes from review * Fix * Fixes from review
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Currently, during call to ReleaseAll (either in pooled allocator destructor or when the memory is full and the allocator needs to release cached allocations) the memory handle is not fully populated (only ptr and size are populated, not context information), which makes the DirectFreeNoLock method to switch to GPU 0 (since 0 is the default constructed Context's dev_id). In the case of multi process training (e.g. with Horovod) this could produce Out of Memory errors due to context creation on GPU 0, or it could outright fail, crashing the application, if the exclusive mode is set on GPU 0.
This PR fixes that by adding
initial_context_parameter to pooled storage managers and populating the handle with it in ReleaseAll method.@apeforest @yuxihu FYI
Checklist
Essentials
Please feel free to remove inapplicable items for your PR.