-
Notifications
You must be signed in to change notification settings - Fork 186
add the same range filter in tracer tool to spinlock tool #523
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
base: dev
Are you sure you want to change the base?
Conversation
William-An
commented
Jan 15, 2026
- The spinlock tool will also be able to specify the kernel range.
- We might want to create a util library
3d32723 to
e2306ca
Compare
|
rebased. Actually just realised after doing this we don't have to rebase on public PRs |
|
Any scripts require changes? |
|
Nope, by default this filter will profile all kernels. |
JRPan
left a comment
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.
Code Review
Overview
This PR adds kernel range filtering functionality to the spinlock tool, replicating similar filtering that already exists in the tracer tool. Users can now specify which kernels to trace via the DYNAMIC_KERNEL_RANGE environment variable, supporting single kernel IDs, ranges, open-ended ranges, and regex-based kernel name filtering.
Strengths
- Clean implementation that follows existing patterns in the codebase
- Comprehensive documentation in the env var help text
- Proper error handling for regex compilation errors
Issues & Suggestions
1. Code Duplication (High Priority)
The PR author notes this themselves: "We might want to create a util library"
The parse_kernel_ranges_from_env() and should_trace_kernel() functions appear to be duplicated from the tracer tool. This creates a maintenance burden.
Recommendation: Consider extracting these into a shared header/utility library in a follow-up PR to avoid divergence.
2. Inconsistent Logic in should_trace_kernel()
if (range.end == 0) {
if (kernel_id >= range.start) {The end == 0 check is used to indicate "open-ended" but also means "trace all" when start == 0 && end == 0. This dual meaning could be confusing.
Recommendation: Consider using a constant like UINT64_MAX for open-ended ranges instead of overloading 0.
3. Missing Early Exit Optimization
uint64_t g_max_kernel_id = 0;g_max_kernel_id is computed but never used. The tracer tool likely uses this to exit early once all specified kernels have been traced.
Recommendation: Either implement the early exit logic or remove the unused variable.
4. Double Negative Boolean Logic
bool enable_instrumentation = should_trace_kernel(kernel_id, mangled_func_name);
bool disable_print = !enable_instrumentation;Using disable_print as a double negative makes the code harder to read.
Recommendation: Use bool enable_print = enable_instrumentation; for clarity.
5. Uncaught Exception Risk
When std::stoull fails (invalid input), it throws an exception that isn't caught:
start = std::stoull(start_str); // Can throw!Recommendation: Wrap in try-catch or validate input.
Test Coverage
No tests appear to be included. Consider adding a brief testing section to the PR description showing manual validation with different DYNAMIC_KERNEL_RANGE values.
Summary
| Aspect | Assessment |
|---|---|
| Correctness | ✅ Logic appears correct |
| Code style | |
| Performance | ✅ Acceptable |
| Test coverage | |
| Security | ✅ No concerns |
Verdict: The PR is functionally sound and adds valuable filtering capability. The main concern is code duplication with the tracer tool - a shared utility library would be beneficial. The minor issues noted above are not blockers but would improve code quality.
|
up to you to fix it or not. I don't really mind |