edac: Add DIMM/rank layer support - #3760
Draft
at-blacknight wants to merge 1 commit into
Draft
Conversation
The edac collector only reads mc*/csrow*/chN. Controllers driven by sb_edac,
skx_edac or i10nm_edac register in the DIMM/MEM layer instead and expose no
csrow* directories at all, so on that hardware the collector reports only
controller totals plus csrow="unknown", with no per-slot attribution.
Read mc*/dimm* and mc*/rank* when a controller has no csrow* directories,
emitting node_edac_dimm_{correctable,uncorrectable}_errors_total labelled with
controller, dimm and dimm_label.
csrow* takes priority because amd64_edac_mod exposes rank* alongside csrow*,
repeating the same counts under EDAC-generated labels; reading both layers
would double-count. Only populated slots are registered, so the indices are
sparse and are globbed rather than counted.
dimm_label is emitted verbatim. Unlike the csrow layer's ch*_dimm_label, which
holds generated names, it is supplied by the platform and identifies a physical
slot, so it is kept byte-identical to what dmidecode and vendor tooling report.
Uncorrectable errors the controller cannot localise are counted in
ue_noinfo_count rather than against a DIMM, so dimm_ue_count is best effort and
the controller-level and *_noinfo series remain authoritative for totals.
Signed-off-by: Adam Butler <github@at-blacknight.com>
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.
Draft implementation of #3722, opened early for feedback on shape rather than as a finished proposal — there are open questions at the bottom I'd rather settle before polishing.
The gap
edacreads onlymc*/csrow*/chN. Controllers driven bysb_edac,skx_edacori10nm_edacregister in the DIMM/MEM layer and expose nocsrow*directories at all, so on that hardware the collector emits controller totals pluscsrow="unknown"and nothing else — the counts are right, the attribution is absent. #3720 added per-channel metrics but reads only the csrow layer, so it is empty there.Across a 55-host fleet of modern Intel hardware, 100% report
csrow="unknown"and nothing else — zero non-unknowncsrow values over a 24h window. On one host the DIMM layer identified a specific failing pair of slots carrying hundreds of millions of correctable errors that the controller-level counter could not attribute.What this does
Per controller: if
csrow*exists, nothing changes. Otherwise walk(dimm|rank)[0-9]*and emitController-level and
*_noinfoseries are untouched.csrow takes priority — thanks @arunsrini3082
The original issue assumed the two layouts were mutually exclusive. They are not: on
amd64_edac_moda controller exposescsrow*andrank*together, and therank*directories repeat the csrow counts under EDAC-generated labels. Reading both double-counts. So the rule is a priority order, not a branch —csrow*is authoritative and the DIMM layer is a fallback only when it is absent.Corroborated by two independent reporters on #3730: @saj on EPYC Rome /
amd64_edac, and @hoffie on HPE ProLiant DL38x Gen10/Gen11.dimm_labelis emitted verbatimDeliberately not passed through
edacDimmLabel(). Two reasons.CONTRIBUTING.md's Collector Implementation Guidelines say metrics "should not get transformed in a way that is hardware specific" and that a value should be exposed "as it is and not keep a mapping in code to make this human readable." The DIMM layer's
dimm_labelis a platform-supplied slot name, and stripping characters from it breaks equality with the kernel value and with whatdmidecodeprints for the same slot — the correlation you need to physically identify a DIMM for replacement.Second,
edacDimmLabel()'s transforms are specific to the csrow layer's dialect:The last two fire only on the synthetic form —
mc#0csrow#0channel#0becomesmc0_csrow0_channel0— which is precisely therank*dialect the priority rule skips. On a platform label likeCPU_SrcID#0_MC#1_Chan#0_DIMM#0neither matches (Chanis notchannel). Three distinct dialects showed up across the fleet, which one fixed transform can't normalize:sb_edacCPU_SrcID#0_Ha#0_Chan#N_DIMM#0skx_edac/i10nm_edacCPU_SrcID#0_MC#N_Chan#N_DIMM#0PROC 1 DIMM 8— spaces, no#at allWorth noting every
dimm_labelin the currente2e-output.txtreadsmc0_csrow0_channel0, so the synthetic dialect is the only one the transform has ever been exercised against.Uncorrectable errors are best effort
EDAC increments a per-DIMM counter only when it can localize the error to a slot; otherwise it goes to the controller's
ce_noinfo_count/ue_noinfo_count. Correctable errors localize reliably. Uncorrectable ones frequently do not —edac_mc.cnotes:So
dimm_ue_countoften reads 0 even when UEs occurred. It is still exposed, documented as best effort in the metric help, and the controller-level and*_noinfoseries remain authoritative for totals — which is why this change adds to them rather than replacing them.Fixtures and tests
Three new controllers in
sys.ttar:mc1— Intel DIMM-model, indicesdimm0/dimm1/dimm3. Only populated slots register on real hardware (observed0,1,3,4,6,7,9,10), so indices are sparse and must be globbed rather than counted.mc2— the same layer spelledrank*, with vendor BIOS labels containing spaces, andrank1missingdimm_ue_countto cover the partial case.mc3—csrow*andrank*coexisting, with therank*counters set to999so that reading both layers would be immediately visible.The
mc3fixture is hand-built from the directory listing @arunsrini3082 posted in #3722, not captured from AMD hardware — I don't have any. Corrections very welcome. The assertion it encodes is structural (rank*skipped whencsrow*is present) so it doesn't depend on label content, but I'd rather someone with an EPYC box confirmed the shape.This adds
collector/edac_linux_test.go, the collector's first unit tests. The e2e golden file already covers the metric output, so the test earns its place on what the golden file can't express:TestEdacCsrowTakesPriorityOverDimmLayernames the double-count failure and prints the offending value. Verified in both directions — removing the priority guard makes it fail with the999s.Cardinality
The DIMM layer adds one series pair per populated slot, replaces nothing, and only appears on hardware that reports no per-slot data at all today. Flagging it because @hoffie raised the opt-in question about the channel metrics on #3730 — happy to put this behind a flag if you'd prefer.
Open questions
node_edac_dimm_*here, parallel tonode_edac_csrow_*andnode_edac_channel_*. A common family across both layouts was discussed on edac: support the DIMM/rank sysfs layout (skx_edac / i10nm_edac / sb_edac) in addition to csrow*/chN #3722 and dropped, partly on timing and partly because a csrow/channel isn't a DIMM. Happy to be redirected.dimm_labelnormalization. I've argued for verbatim above, but this is really a ruling for you to make, and I'd like it documented either way so the next layout someone adds doesn't have to guess.Unrelated one-line fix in the same file, if it's useful to take first: #3759.