-
Notifications
You must be signed in to change notification settings - Fork 916
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
Control pinned memory use with environment variables #17657
base: branch-25.02
Are you sure you want to change the base?
Conversation
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
/ok to test |
@@ -277,7 +279,7 @@ bool config_default_pinned_memory_resource(pinned_mr_options const& opts) | |||
CUDF_EXPORT auto& kernel_pinned_copy_threshold() | |||
{ | |||
// use cudaMemcpyAsync for all pinned copies | |||
static std::atomic<size_t> threshold = 0; | |||
static std::atomic<size_t> threshold = getenv_or("LIBCUDF_KERNEL_PINNED_COPY_THRESHOLD", 0); |
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.
One issue with static variable is that, they are initialized only once. The user will not be able to change them after they are set. Can we remove static?
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.
I'm torn on this one. You're right, but:
I really like reading the env var once so I can log it cleanly (already a part of getenv_or
).
Also, there are (C++) APIs to set these thresholds at runtime.
I'd keep this static until we have a use case that requires runtime changes.
Description
Adds
LIBCUDF_KERNEL_PINNED_COPY_THRESHOLD
andLIBCUDF_ALLOCATE_HOST_AS_PINNED_THRESHOLD
environment variables to set the pinned memory optimizations' thresholds.Checklist