-
Notifications
You must be signed in to change notification settings - Fork 204
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 Arena MR to support simultaneous access by PTDS and other streams #1395
Changes from 10 commits
adaccaa
f9d56ff
1a8e271
c8c89e3
2e3a1b5
ff5e61c
89eb964
3a8e795
889271c
0140328
e72cfd5
244b95a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -533,6 +533,39 @@ TEST_F(ArenaTest, Defragment) // NOLINT | |||||||||||
}()); | ||||||||||||
} | ||||||||||||
|
||||||||||||
TEST_F(ArenaTest, PerThreadToStreamDealloc) // NOLINT | ||||||||||||
{ | ||||||||||||
// This is testing that deallocation of a ptr still works when | ||||||||||||
// it was originally allocated in a superblock that was in a thread | ||||||||||||
// arena that then moved to global arena during a defragmentation | ||||||||||||
// and then moved to a stream arena. | ||||||||||||
auto const arena_size = superblock::minimum_size * 2; | ||||||||||||
arena_mr mr(rmm::mr::get_current_device_resource(), arena_size); | ||||||||||||
auto per_thread_stream = rmm::cuda_stream_per_thread; | ||||||||||||
// Create an allocation from a per thread arena | ||||||||||||
void* thread_ptr = mr.allocate(256, per_thread_stream); | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit [non-blocking]: No need to store this constant in a variable.
Suggested change
|
||||||||||||
// Create an allocation in a stream arena to force global arena | ||||||||||||
// to be empty | ||||||||||||
cuda_stream stream{}; | ||||||||||||
void* ptr = mr.allocate(32_KiB, stream); | ||||||||||||
mr.deallocate(ptr, 32_KiB, stream); | ||||||||||||
// at this point the global arena doesn't have any superblocks so | ||||||||||||
// the next allocation causes defrag. Defrag causes all superblocks | ||||||||||||
// from the thread and stream arena allocated above to go back to | ||||||||||||
// global arena and it allocates one superblock to the stream arena. | ||||||||||||
auto* ptr1 = mr.allocate(superblock::minimum_size); | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Question: What stream will this use? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. e.g.
Suggested change
|
||||||||||||
// Allocate again to make sure all superblocks from | ||||||||||||
// global arena are owned by a stream arena instead of a thread arena | ||||||||||||
// or the global arena. | ||||||||||||
auto* ptr2 = mr.allocate(32_KiB); | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Question: What stream will this use? |
||||||||||||
// The original thread ptr is now owned by a stream arena so make | ||||||||||||
// sure deallocation works. | ||||||||||||
// NOLINTNEXTLINE(cppcoreguidelines-avoid-goto) | ||||||||||||
EXPECT_NO_THROW(mr.deallocate(thread_ptr, 256, per_thread_stream)); | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit [non-blocking]:
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit [non-blocking]: |
||||||||||||
mr.deallocate(ptr1, superblock::minimum_size); | ||||||||||||
mr.deallocate(ptr2, 32_KiB); | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Question: What stream will this use? |
||||||||||||
} | ||||||||||||
|
||||||||||||
TEST_F(ArenaTest, DumpLogOnFailure) // NOLINT | ||||||||||||
{ | ||||||||||||
arena_mr mr{rmm::mr::get_current_device_resource(), 1_MiB, true}; | ||||||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for an allocation to move ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated