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

thrust/mr: fix the case of reuising a block for a smaller alloc. #1232

Merged
merged 2 commits into from
Jan 23, 2024

Conversation

griwes
Copy link
Collaborator

@griwes griwes commented Dec 19, 2023

Description

Previously, the pool happily returned a pointer to a larger oversized block than requested, without storing the information that the block is now smaller, which meant that on deallocation, it'd look for the descriptor of the block in the wrong place. This is now fixed by moving the descriptor to always be where deallocation can find it using the user-provided size, and by storing the original size to restore the descriptor to its rightful place when deallocating.

Also a drive-by fix for a bug where in certain cases the reallocated cached oversized block wasn't removed from the cached list. Whoops. Kinda surprised this hasn't exploded before.

Resolves #585

Checklist

  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

Previously, the pool happily returned a pointer to a larger oversized
block than requested, without storing the information that the block is
now smaller, which meant that on deallocation, it'd look for the
descriptor of the block in the wrong place. This is now fixed by moving
the descriptor to always be where deallocation can find it using the
user-provided size, and by storing the original size to restore the
descriptor to its rightful place when deallocating.

Also a drive-by fix for a bug where in certain cases the reallocated
cached oversized block wasn't removed from the cached list. Whoops.
Kinda surprised this hasn't exploded before.
@griwes griwes requested review from a team as code owners December 19, 2023 00:55
@griwes griwes requested review from miscco and wmaxey December 19, 2023 00:55
@jrhemstad jrhemstad added the backport branch/2.3.x For backporting to the 2.3.x release branch label Jan 10, 2024
Copy link
Collaborator

@miscco miscco left a comment

Choose a reason for hiding this comment

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

Looks good to me.

Some suggestions regarding improved readability

thrust/testing/mr_pool.cu Show resolved Hide resolved
thrust/thrust/mr/pool.h Outdated Show resolved Hide resolved
thrust/thrust/mr/pool.h Outdated Show resolved Hide resolved
thrust/thrust/mr/pool.h Outdated Show resolved Hide resolved
thrust/thrust/mr/pool.h Outdated Show resolved Hide resolved
thrust/thrust/mr/pool.h Outdated Show resolved Hide resolved
@griwes griwes merged commit f857034 into NVIDIA:main Jan 23, 2024
537 checks passed
Copy link
Contributor

GitHub Actions is not permitted to create or approve pull requests.

griwes added a commit that referenced this pull request Jan 24, 2024
* thrust/mr: fix the case of reuising a block for a smaller alloc.

Previously, the pool happily returned a pointer to a larger oversized
block than requested, without storing the information that the block is
now smaller, which meant that on deallocation, it'd look for the
descriptor of the block in the wrong place. This is now fixed by moving
the descriptor to always be where deallocation can find it using the
user-provided size, and by storing the original size to restore the
descriptor to its rightful place when deallocating.

Also a drive-by fix for a bug where in certain cases the reallocated
cached oversized block wasn't removed from the cached list. Whoops.
Kinda surprised this hasn't exploded before.

* thrust/mr: add aliases to reused pointer traits in pool.h
jrhemstad added a commit that referenced this pull request Feb 22, 2024
…) (#1317)

* thrust/mr: fix the case of reuising a block for a smaller alloc.

Previously, the pool happily returned a pointer to a larger oversized
block than requested, without storing the information that the block is
now smaller, which meant that on deallocation, it'd look for the
descriptor of the block in the wrong place. This is now fixed by moving
the descriptor to always be where deallocation can find it using the
user-provided size, and by storing the original size to restore the
descriptor to its rightful place when deallocating.

Also a drive-by fix for a bug where in certain cases the reallocated
cached oversized block wasn't removed from the cached list. Whoops.
Kinda surprised this hasn't exploded before.

* thrust/mr: add aliases to reused pointer traits in pool.h

Co-authored-by: Jake Hemstad <jhemstad@nvidia.com>
@miscco miscco added backport done This PR has been backported to the relevant branch thrust For all items related to Thrust. bug: functional and removed backport branch/2.3.x For backporting to the 2.3.x release branch labels Feb 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport done This PR has been backported to the relevant branch bug: functional thrust For all items related to Thrust.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Memory pool gets corrupted when oversized allocations are returned to pool out of order
3 participants