Skip to content

Name KvikIO thread-pool workers for profiling - #1014

Open
vyasr wants to merge 3 commits into
rapidsai:mainfrom
vyasr:feat/name_threads
Open

Name KvikIO thread-pool workers for profiling#1014
vyasr wants to merge 3 commits into
rapidsai:mainfrom
vyasr:feat/name_threads

Conversation

@vyasr

@vyasr vyasr commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Give KvikIO's IO thread-pool workers a real OS thread name (kvikio-<idx> for the default pool, kvikio-dev-<idx> for per-block-device pools) via BS::thread_pool's init_task hook. This makes them recognizable in profilers like nsys, py-spy, and /proc/<pid>/task/<tid>/comm instead of showing up as the parent process name.

vyasr added 2 commits July 22, 2026 21:06
BS::thread_pool accepts an init_task that runs once on each worker
thread as it starts. Use it to set the OS thread name (comm on Linux)
via pthread_setname_np, so KvikIO's IO workers show up in nsys,
py-spy, top -H, and /proc/<pid>/task/<tid>/comm as 'kvikio-<idx>'
(global default pool) or 'kvikio-dev-<idx>' (per-block-device pools)
instead of the generic parent process name.

Add a make_thread_pool_init_task(prefix) helper in
threadpool_wrapper.hpp that captures a shared atomic counter, so
threads within a pool get sequential indices and different pools
sharing the same prefix do not collide.

POSIX only (pthread_setname_np); a no-op elsewhere. Names fit
Linux's 15-char comm limit: 'kvikio-<idx>' up to 4-digit idx = 11
chars, 'kvikio-dev-<idx>' up to 3-digit idx = 14 chars.
KvikIO only supports Linux, so the '#if defined(__linux__) ||
defined(__APPLE__)' guards and the macOS pthread_setname_np branch in
make_thread_pool_init_task added noise without ever being compiled.
Remove them and let <pthread.h> be included unconditionally, along
with the docstring line about non-POSIX platforms.
@vyasr
vyasr requested a review from a team as a code owner July 22, 2026 23:32
@vyasr vyasr self-assigned this Jul 23, 2026
@vyasr vyasr added improvement Improves an existing functionality non-breaking Introduces a non-breaking change labels Jul 23, 2026
@vyasr
vyasr force-pushed the feat/name_threads branch from 813187a to 3f5f084 Compare July 23, 2026 01:16
Comment thread cpp/src/file_handle.cpp
// First file on this block device: create a new dedicated thread pool
auto thread_pool = std::make_shared<ThreadPool>(defaults::num_threads());
auto thread_pool = std::make_shared<ThreadPool>(defaults::num_threads(),
make_thread_pool_init_task("kvikio-dev"));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could we call it "kvikio-block-device" / "kvikio-block-dev" or just "kvikio", to avoid confusion with "development"?

// Linux comm limit is 15 chars + NUL.
std::array<char, 16> name{};
std::snprintf(name.data(), name.size(), "%s-%u", prefix.c_str(), idx);
pthread_setname_np(pthread_self(), name.data());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

NVTX has a function dedicated to thread renaming: nvtxNameOsThread . But it only worked with global NVTX domain, not with kvikio's custom domain ( https://nvidia.slack.com/archives/C07722DM4MA/p1739200643850559). What is interesting is that its documentation has the following statement:

Tools expect thread ID to be a number that uniquely identifies the thread at the time of the call. Note that a thread's ID can be reused after it is destroyed. Tools may choose how to handle aliasing of thread IDs.
POSIX pthread_t type returned by pthread_self() may not comply with these expectations. Please use OS-specific thread ID instead of pthread_t.

I was not sure if the "use OS-specific thread ID instead of pthread_t" recommendation applied to nvtxNameOsThread specifically, or to the profiling tools in general (including nsys) that want to consume NVTX.

But I think whatever works in nsys-UI is the one we should pick, so the method in this PR should be good.

@kingcrimsontianyu kingcrimsontianyu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good! Just a minor suggestion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

improvement Improves an existing functionality non-breaking Introduces a non-breaking change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants