Enable caching allocator for pinned (page-locked) memory #275
Conversation
This file contains hidden or 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
apaszke
suggested changes
Dec 1, 2016
Contributor
apaszke
left a comment
apaszke
left a comment
There was a problem hiding this comment.
Shouldn't we set some kind of a hard limit on the cache size, so that we don't take up everything with page-locked memory? We could query the available memory size and set it depending on that as part of initiaization.
torch/cuda/__init__.py
Outdated
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
torch/cuda/streams.py
Outdated
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
colesbury
commented
Dec 1, 2016
Member
Author
There was a problem hiding this comment.
I'm worried that a limit on the cache size will introduce as many problems as it will solve:
- What seems like a reasonable limit might be too small for some use cases. For example, I have a large model which doesn't fit on the GPU. I store the weights in pinned memory and swap parts of it on and off the GPU. A large "base" allocation of pinned memory like this might push transient allocations over what seemed like a reasonable limit.
- The same fraction might be too large to help with other use cases. For example, programs on our cluster see the entire system memory but will be killed if they use more than their fraction.
- It introduces new failure modes, which makes debugging and analysis harder. Currently there's one failure mode: you use too much memory. With the limit you still have that failure mode, but now you also have a case where you might be constantly synchronizing due to
cudaFreeHostcalls - It makes allocating pinned memory potentially synchronize (because it might push you over the limit). Without the caching allocator, only free's synchronize. If we're not careful, this can potentially lead to deadlocks if a synchronization in the dataloader happens while NCCL kernels are launched.
If people run in to memory issues than we should address it, but I'd like to keep the implementation simple until we have evidence of real problems.
torch/cuda/streams.py
Outdated
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
colesbury
force-pushed
the
caching
branch
2 times, most recently
from
59b8307 to
f0f8df4
Compare
apaszke
approved these changes
Dec 1, 2016
Member
Author
|
I've submitted the PR to cutorch (torch/cutorch#618) |
colesbury
changed the title
Also add binding for CUDA "sleep" kernel
resistor
pushed a commit
to resistor/pytorch
that referenced
this pull request
Mar 11, 2020
KsenijaS
pushed a commit
to KsenijaS/pytorch
that referenced
this pull request
Dec 14, 2020
* file size updates for BERT
KyleCZH
pushed a commit
to KyleCZH/pytorch
that referenced
this pull request
Sep 20, 2021
Include build_hash in libtorch-binary
eellison
pushed a commit
to eellison/pytorch
that referenced
this pull request
Jun 29, 2022
hubertlu-tw
pushed a commit
to hubertlu-tw/pytorch
that referenced
this pull request
Nov 1, 2022
* Persistent group batchnorm added Added persistent grouped batch norm for performance run on strong scaling case: currently only supporting: 1. nhwc layout 2. fp16 3. synchronization only within a node! Environment variable is used to tune LAUNCH_MARGIN that limits the CTAs usage by the persistent kernel. Documentation and examples will follow. * updating type().scalarType() to scalar_type() * moving launch margin to be defined at layer creation, adding a knob cap max ctas per sm * fixing the cta computation * review comment: set device_id through cudaGetDevice() move cudaMemset to cudaMemsetAsync updated __threadfence() to __threadfence_system() inter device write
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Adds a caching allocator for CUDA pinned (page-locked) memory. This avoid synchronization due to cudaFreeHost or cudaHostUnregister calls.
To ensure read-after-write and write-after-read consistency, a CUDA event is recorded after every cudaMemcpyAsync between host and device involving pinned memory created by this allocator. Memory allocations are only re-used after they're freed and all associated CUDA events have completed.
Unlike the caching device allocator, allocations are never split. This means that requests for small allocations may be filled by much larger cached buffers. I think this should be OK in practice.
Also, CUDA events are processed in the order in which they're recorded, even though events may occur out-of-order between devices or streams. This does not affect correctness, but means that cached allocations may not be considered "ready" for re-use until a little later. In practice, I don't think this should matter.
I'll send a PR to cutorch soon, but I want to make sure the continuous builds pass.
I'm interested if @ngimel or @thatguymike have any comments.
See #265