Fix slow-clocksource check for HW monotonic clock and non-x86 advisories - #4272
Conversation
The --check-system warning always suggested switching to tsc, which is x86-specific and misleading on ARM64 where arch_sys_counter is often the only available clocksource. Suggest an actual alternative when one exists, and otherwise explain that switching is not possible. Fixes valkey-io#4269 Signed-off-by: quanyeyang <quanyemostima@gmail.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe slow-clocksource check now recognizes hardware monotonic clocks, reads current and available kernel clocksources, selects a suitable alternative when possible, and reports either a switch command or why switching cannot be recommended. ChangesClocksource check
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/syscheck.c (1)
64-108: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for clocksource selection and advisory branches.
Add
src/unit/GoogleTests fortscpreference, fallback selection, a sole available source, and unreadable sysfs values. This behavior change otherwise has no regression coverage. As per coding guidelines, “Code changes should include relevant tests when the repository has a matching test location,” and low-level logic tests belong insrc/unit/as C++ GoogleTest tests.Also applies to: 151-172
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/syscheck.c` around lines 64 - 108, Add C++ GoogleTest coverage under src/unit for clocksource selection, exercising tsc preference, fallback to the first alternative, a sole available source, and unreadable or missing sysfs values. Target the clocksourceHasToken and pickAlternativeClocksource behavior, including advisory branches, while preserving the existing selection semantics.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/syscheck.c`:
- Around line 149-171: Update the error-message branching around
pickAlternativeClocksource() to distinguish unreadable clocksource data from a
successfully read list with no alternative. Emit a “could not determine
clocksources” diagnostic when curr or avail was not read, retain the existing
no-alternative message only when both values are available, and preserve the
suggestion path when suggest is non-NULL.
---
Nitpick comments:
In `@src/syscheck.c`:
- Around line 64-108: Add C++ GoogleTest coverage under src/unit for clocksource
selection, exercising tsc preference, fallback to the first alternative, a sole
available source, and unreadable or missing sysfs values. Target the
clocksourceHasToken and pickAlternativeClocksource behavior, including advisory
branches, while preserving the existing selection semantics.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
![valkey-review-bot[bot]](https://avatars.githubusercontent.com/in/3633497?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed the advisory rework in src/syscheck.c. The direction is right (no hard-coded tsc), and the code compiles clean and the tokenizers terminate correctly (verified by running both helpers verbatim over the realistic sysfs inputs). Two issues with the picked suggestion and with the "no alternative" wording are below.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## unstable #4272 +/- ##
============================================
- Coverage 76.84% 76.82% -0.03%
============================================
Files 162 162
Lines 81479 81507 +28
============================================
+ Hits 62610 62615 +5
- Misses 18869 18892 +23
🚀 New features to boost your workflow:
|
Signed-off-by: quanyeyang <quanyemostima@gmail.com>
Co-authored-by: Shubham S Taple <subbutaple@gmail.com> Signed-off-by: Quanye Yang <quanyemostima@gmail.com>
Signed-off-by: quanyeyang <quanyemostima@gmail.com>
Signed-off-by: quanyeyang <quanyemostima@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/syscheck.c`:
- Line 34: Remove the monotonic.h include and the early return based on
monotonicGetType() in the syscheck flow, allowing the existing kernel
clocksource performance probe to run unconditionally. Retain sysfs clocksource
detection only for tailoring the advisory message.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
quanyeyang
changed the title
|
@quanyeyang Maybe you can check the failing CI (I don't think its failing because of your changes though) |
|
@ShubhamTaple Emmm,I think it's flaky. |
|
Thanks for the review, @dvkashapov. It looks like the PR is still blocked because it requires an approving review from someone with write access. Could you help request a review from an appropriate maintainer, or let me know who I should ask? Thanks! |
zuiderkwast
left a comment
zuiderkwast
left a comment
There was a problem hiding this comment.
LGMT, thanks!
Thanks also to @dal1wg and @dvkashapov for sorting this out in the discussion in the issue!
| /* Return 1 if `name` appears as a whitespace-separated token in `list`. */ | ||
| static int clocksourceHasToken(const char *list, const char *name) { |
There was a problem hiding this comment.
Minor: This is a generic string function. We could put it in util.c – or we can move it there later if we need it for anything else.
4 participants
Fixes #4269
Summary
valkey-server --check-systemused to probeclock_gettime(CLOCK_MONOTONIC)and, on failure, always suggested switching to
tsc. That is wrong on ARM64,where
arch_sys_counteris normal and often the only available clocksource.Two changes, as discussed in #4269:
Skip the probe when Valkey uses a hardware monotonic clock
(
monotonicGetType() == MONOTONIC_CLOCK_HW, e.g. aarch64CNTVCT/x86
TSC). In that case the server hot path does not depend on the Linuxclock_gettime/ clocksource path, so the warning is not actionable forValkey and can also false-positive due to short
getrusage()noise.The probe is kept for the POSIX fallback path.
Fix the advisory text when the probe still runs
(
tsc,arch_sys_counter,kvm-clock,hyperv_clocksource_tsc_page).tsc).This does not whitelist
arch_sys_counterby name.Test plan
CFLAGS=-DNO_PROCESSOR_CLOCK(POSIX fallback): probe still runs;warning does not hard-code
tsc--check-systemshows[slow-clocksource]...skippedfast alternative when available