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 padding from device_memory_resource #1278

Merged
merged 2 commits into from
Jun 2, 2023

Conversation

vyasr
Copy link
Contributor

@vyasr vyasr commented May 30, 2023

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 rongou and jrhemstad May 30, 2023 22:36
@vyasr
Copy link
Contributor Author

vyasr commented May 30, 2023

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

Copy link
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 5 - 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
Contributor Author

vyasr commented May 31, 2023

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

@harrism
Copy link
Member

harrism commented May 31, 2023

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

@vyasr
Copy link
Contributor Author

vyasr commented May 31, 2023

Updated with a cuspatial PR.

@vyasr vyasr removed the 5 - DO NOT MERGE Hold off on merging; see PR for details label Jun 2, 2023
@vyasr
Copy link
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
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

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:

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
Member

Could you please raise an issue?

@ttnghia
Copy link

ttnghia commented Jun 16, 2023

Could you please raise an issue?

Done.

rapids-bot bot pushed a commit to rapidsai/cudf that referenced this pull request Jun 23, 2023
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
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