events: add performance MSRs to boot event - #11412
Open
ripatel-fd wants to merge 1 commit into
Open
Conversation
Collect x86 model-specific registers on startup and report them back to telemetry. It turns out some hardware mitigations like BpSpecReduce get enabled even if mitigations are off and cause a big performance impact (e.g. through SEV-SNP).
┌─ ⚡ PERF · 018b4cb vs main@2fc40e5 ─────────────────────────────────
│ SUITE BASELINE NEW Δ
│ replay tps, mainnet 28,305 tps 28,232 tps · -0.26%
│ bench tps, localnet 128,647 tps 128,646 tps · 0.00%
│ snapshot load, testnet 15.66 s 15.77 s · +0.72%
│ mem total, mainnet 171.44 GiB 171.44 GiB · 0.00%
│ mem total, testnet 102.31 GiB 102.31 GiB · 0.00%
│ clean compile, firedancer 315.9 cpu·s 320.4 cpu·s · +1.44%
│ binary size, firedancer 85.94 MB 85.95 MB · +0.01%
├─────────────────────────────────────────────────────────────────────
@@ 0 REGRESSIONS · 0 WARNINGS · 0 IMPROVED · 7 NOISE @@
└───────────────────────────────────────────────────────────────────── |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Moderate findings remain unresolved around EINTR handling, optional-field test support, and nested nullable-type validation.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds CPU0 x86 MSR collection to boot telemetry for diagnosing hardware mitigation performance effects.
Changes:
- Adds seven optional MSR fields to event schemas.
- Collects and publishes available MSR values with presence tracking.
- Extends nullable integer schema generation.
File summaries
| File | Reviewed changes and final findings |
|---|---|
src/disco/events/schema/events.proto |
Adds optional MSR fields. Moderate (2 votes): update the test parser for optional and add presence/value assertions. |
src/disco/events/schema/boot.json |
Defines nullable MSR telemetry columns. |
src/disco/events/gen_events.py |
Adds nullable integer generation. Moderate (2 votes): reject or recursively validate nullable types nested in tuples or arrays. |
src/disco/events/fd_boot_report.h |
Stores MSR values and presence flags. |
src/disco/events/fd_boot_report.c |
Reads and publishes MSRs. Moderate (2 votes): retry pread on EINTR. Moderate (1 vote): update the boot-report decoder and test successful MSR publication. |
Review details
Suppressed comments (1)
src/disco/events/fd_boot_report.c:326
- The existing
test_boot_reportdecoder only stripsrepeated, so it does not recognize these newoptionaldeclarations. On a host where/dev/cpu/0/msris readable, at leastmsr_x86_spec_ctrlis emitted, the declaration is skipped byproto_msg_fields, andcheck_msgfails on the unknown field; update the test parser and cover the presence/value path.
r->msr_x86_spec_ctrl_present = !read_msr( 0U, 0x00000048UL, &r->msr_x86_spec_ctrl );
if( r->cpu_vendor==2 ) { /* AMD specific */
- Files reviewed: 5/5 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| @@ -275,6 +286,8 @@ def field_is_supported(f: Field) -> bool: | |||
| _NON_LEAF = (ClickHouseType.String, ClickHouseType.Bytes, | |||
| ClickHouseType.Flatten, ClickHouseType.Tuple, ClickHouseType.Array) | |||
| _SUB_UNSUPPORTED = _NON_LEAF + (ClickHouseType.LowCardinalityString,) | |||
| // A numeric identifier summarizing the exact set of protocol features this build understands, used to detect when nodes disagree on consensus rules. | ||
| uint32 feature_set_id = 75; | ||
| // Raw value of AMD MSR C001_0010 (AMD64_SYSCFG) read from CPU0, or NULL when unavailable. | ||
| optional uint64 msr_x86_amd_syscfg = 76; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Collect x86 model-specific registers on startup and report them
back to telemetry. It turns out some hardware mitigations like
BpSpecReduce get enabled even if mitigations are off and cause a
big performance impact (e.g. through SEV-SNP).