issue: 5170344 Fix IB HCA core clock unit scaling in time converter - #640
issue: 5170344 Fix IB HCA core clock unit scaling in time converter#640pasis wants to merge 1 commit into
Conversation
time_converter_ib_ctx scaled the NIC core clock frequency with the wrong factor, corrupting hardware timestamp to system time conversion. The constructor receives ibv_device_attr_ex::hca_core_clock, which rdma-core reports in kHz. To store it as Hz it must be multiplied by 1000, but the code multiplied by USEC_PER_SEC (1,000,000), leaving the stored clock 1000x too large. The USEC_PER_SEC factor was only correct for the legacy experimental verbs field (ibv_exp_device_attr::hca_core_clock, expressed in MHz). The migration to rdma-core changed the unit to kHz without updating the multiplier. Effect: in calculate_delta() tv_sec = hw_time_diff / hca_core_clock comes out 1000x too small, so converted timestamps drift from the raw hardware time and the error grows linearly. TS_CONVERSION_MODE_RAW is affected for the lifetime of the converter; TS_CONVERSION_MODE_SYNC is affected until the periodic fix_hw_clock_deviation() recomputes the rate empirically. The kHz-to-Hz scale is applied through a dedicated HZ_PER_KHZ constant instead of the coincidentally-equal MSEC_PER_SEC, and the constructor parameter is renamed hca_core_clock_khz to document the input unit. Signed-off-by: Dmytro Podgornyi <dmytrop@nvidia.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThis PR fixes a 1000× scaling error in
Confidence Score: 5/5The change is a minimal, targeted fix to a single multiplication constant; all callers already pass the kHz value from ibv_device_attr_ex and are unmodified. The three-file change is self-contained: a constant rename in clock.h, a parameter rename in the header, and a multiplier correction in the constructor. The empirical clock-rate correction path in fix_hw_clock_deviation() derives hca_core_clock from measured tick/nanosecond ratios and is unaffected. The get_hca_core_clock() accessor has no callers outside the class. The fix straightforwardly corrects the documented 1000x scale error with no risk of introducing new issues. No files require special attention; the change is narrow and the callers in ib_ctx_handler.cpp are unmodified and correctly pass kHz values. Important Files Changed
Reviews (1): Last reviewed commit: "issue: 5170344 Fix IB HCA core clock uni..." | Re-trigger Greptile |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/core/dev/time_converter_ib_ctx.cpp (1)
40-40: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd regression coverage for the kHz-to-Hz contract.
Please add or verify a test covering a representative HCA clock value and
TS_CONVERSION_MODE_RAW, asserting thathca_core_clock_khz * 1000is stored and used as Hz. This would prevent a recurrence of the original 1000× scaling bug.🤖 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/core/dev/time_converter_ib_ctx.cpp` at line 40, Add regression coverage around the time-converter initialization that uses a representative HCA clock value with TS_CONVERSION_MODE_RAW, asserting hca_core_clock_khz is converted by multiplying by 1000 before being stored and used as Hz. Verify the test would fail if the kHz-to-Hz scaling regresses by 1000×.
🤖 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.
Nitpick comments:
In `@src/core/dev/time_converter_ib_ctx.cpp`:
- Line 40: Add regression coverage around the time-converter initialization that
uses a representative HCA clock value with TS_CONVERSION_MODE_RAW, asserting
hca_core_clock_khz is converted by multiplying by 1000 before being stored and
used as Hz. Verify the test would fail if the kHz-to-Hz scaling regresses by
1000×.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Enterprise
Run ID: 9809d450-415f-49eb-a24d-ee8904cda8ef
📒 Files selected for processing (3)
src/core/dev/time_converter_ib_ctx.cppsrc/core/dev/time_converter_ib_ctx.hsrc/utils/clock.h
1 participant
Description
time_converter_ib_ctx scaled the NIC core clock frequency with the wrong factor, corrupting hardware timestamp to system time conversion.
The constructor receives ibv_device_attr_ex::hca_core_clock, which rdma-core reports in kHz. To store it as Hz it must be multiplied by 1000, but the code multiplied by USEC_PER_SEC (1,000,000), leaving the stored clock 1000x too large.
The USEC_PER_SEC factor was only correct for the legacy experimental verbs field (ibv_exp_device_attr::hca_core_clock, expressed in MHz). The migration to rdma-core changed the unit to kHz without updating the multiplier.
Effect: in calculate_delta() tv_sec = hw_time_diff / hca_core_clock comes out 1000x too small, so converted timestamps drift from the raw hardware time and the error grows linearly. TS_CONVERSION_MODE_RAW is affected for the lifetime of the converter; TS_CONVERSION_MODE_SYNC is affected until the periodic fix_hw_clock_deviation() recomputes the rate empirically.
The kHz-to-Hz scale is applied through a dedicated HZ_PER_KHZ constant instead of the coincidentally-equal MSEC_PER_SEC, and the constructor parameter is renamed hca_core_clock_khz to document the input unit.
What
Fix time_converter_ib_ctx inaccuracy.
Why ?
Bugfix: fix time_converter_ib_ctx inaccuracy.
Change type
What kind of change does this PR introduce?
Check list
Summary by CodeRabbit