Skip to content

Remove padding from device_memory_resource#1278

Merged
rapids-bot[bot] merged 2 commits intorapidsai:branch-23.08from
vyasr:feat/remove_align_up
Jun 2, 2023
Merged

Remove padding from device_memory_resource#1278
rapids-bot[bot] merged 2 commits intorapidsai:branch-23.08from
vyasr:feat/remove_align_up

Conversation

@vyasr
Copy link
Copy Markdown
Contributor

@vyasr vyasr commented May 30, 2023 •
edited
Loading

Description

This PR removes the align_up call that enforces 8B padding on all RMM device memory allocations. Padding, where needed, should be the responsibility of callers, possibly with the help of a padding adapter. It should not be enforced by rmm.

I looked at all other calls to align_up in the library but didn't find any others that needed removing. The other calls all seemed to be used to guarantee specific intentional alignment requirements of other memory resources or adapters. I would appreciate verification from reviewers, though.

I've labeled this PR as breaking because it could break consumers that were implicitly relying on the current padding behavior.

Resolves #865
Resolves #1156

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@vyasr vyasr added 3 - Ready for review Ready for review by team breaking Breaking change improvement Improvement / enhancement to an existing function cpp Pertains to C++ code labels May 30, 2023
@vyasr vyasr requested a review from a team as a code owner May 30, 2023 22:36
@vyasr vyasr self-assigned this May 30, 2023
@vyasr vyasr requested review from jrhemstad and rongou May 30, 2023 22:36
@vyasr
Copy link
Copy Markdown
Contributor Author

vyasr commented May 30, 2023

See #865 (comment) for more of a summary on the decision here.

harrism
harrism approved these changes May 31, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@harrism harrism left a comment

Choose a reason for hiding this comment

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

Do you have a plan for downstream testing before merging, @vyasr ?

@vyasr vyasr added 5 - Merge After Dependencies Depends on another PR: do not merge out of order DO NOT MERGE Hold off on merging; see PR for details and removed 3 - Ready for review Ready for review by team labels May 31, 2023
@vyasr
Copy link
Copy Markdown
Contributor Author

vyasr commented May 31, 2023 •
edited
Loading

Thanks for the reminder Mark. I've created test PRs:

@harrism harrism mentioned this pull request May 31, 2023
Closed
@harrism
Copy link
Copy Markdown
Member

harrism commented May 31, 2023 •
edited
Loading

I created an issue for cuSpatial as well. Assigning you @vyasr . rapidsai/cuspatial#1164

@vyasr
Copy link
Copy Markdown
Contributor Author

vyasr commented May 31, 2023

Updated with a cuspatial PR.

@vyasr vyasr removed the DO NOT MERGE Hold off on merging; see PR for details label Jun 2, 2023
@vyasr
Copy link
Copy Markdown
Contributor Author

vyasr commented Jun 2, 2023

OK, we now have tests passing on all the testing PRs (cugraph's has an unrelated error blocking wheel builds). Think this is good to go now.

@vyasr
Copy link
Copy Markdown
Contributor Author

vyasr commented Jun 2, 2023

/merge

@rapids-bot rapids-bot bot merged commit 64da5fe into rapidsai:branch-23.08 Jun 2, 2023
@vyasr vyasr deleted the feat/remove_align_up branch June 2, 2023 22:11
@ttnghia
Copy link
Copy Markdown

ttnghia commented Jun 15, 2023

Sorry for jumping in late. What about padding when using arena allocator? It seems that we need to modify this too:

rmm/include/rmm/mr/device/arena_memory_resource.hpp

Lines 137 to 144 in 0c08dd5

void* do_allocate(std::size_t bytes, cuda_stream_view stream) override
{
if (bytes <= 0) { return nullptr; }
#ifdef RMM_ARENA_USE_SIZE_CLASSES
bytes = rmm::mr::detail::arena::align_to_size_class(bytes);
#else
bytes = rmm::detail::align_up(bytes, rmm::detail::CUDA_ALLOCATION_ALIGNMENT);
#endif

@jakirkham
Copy link
Copy Markdown
Member

jakirkham commented Jun 15, 2023

Could you please raise an issue?

This was referenced Jun 16, 2023
Merged
Closed
@ttnghia
Copy link
Copy Markdown

ttnghia commented Jun 16, 2023 •
edited
Loading

Could you please raise an issue?

Done.

@ttnghia ttnghia mentioned this pull request Jun 22, 2023
Open
rapids-bot bot pushed a commit to rapidsai/cudf that referenced this pull request Jun 23, 2023
@ttnghia
Fix memory issues in cuIO due to removal of memory padding (#13586)
0b4e354 
After `rmm` removed memory padding (rapidsai/rmm#1278), some of cuIO code started to have out-of-bound access issues because many of its compute kernels shift the input pointers back and forth to satisfy some alignment.

This adds back padding to various memory buffers so the buffers now will have some extra space enough for such shifting.

With this fix, the reported issues (#13567,  #13571, #13570) no longer show up.

Closes:
 * #13567
 * #13571
 * #13570

Authors:
  - Nghia Truong (https://github.com/ttnghia)

Approvers:
  - David Wendt (https://github.com/davidwendt)
  - Bradley Dice (https://github.com/bdice)
  - Vyas Ramasubramani (https://github.com/vyasr)
  - Vukasin Milovanovic (https://github.com/vuule)

URL: #13586
@mhaseeb123 mhaseeb123 mentioned this pull request Jul 31, 2024
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rongou rongou rongou approved these changes

@harrism harrism harrism approved these changes

@jrhemstad jrhemstad Awaiting requested review from jrhemstad jrhemstad is a code owner automatically assigned from rapidsai/rmm-cpp-codeowners

Assignees

@vyasr vyasr

Labels

5 - Merge After Dependencies Depends on another PR: do not merge out of order breaking Breaking change cpp Pertains to C++ code improvement Improvement / enhancement to an existing function

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[BUG] There is no overflow checking on alignment [FEA] RMM should not pad allocations

5 participants

@vyasr @harrism @ttnghia @jakirkham @rongou