Skip to content

fix: report RAPL DRAM as RAM energy instead of adding it to CPU energy - #1344

Draft
davidberenstein1957 wants to merge 4 commits into
masterfrom
fix/rapl-include-dram
Draft

davidberenstein1957 wants to merge 4 commits into
masterfrom
fix/rapl-include-dram

Conversation

@davidberenstein1957

@davidberenstein1957 davidberenstein1957 commented Aug 12, 2026 •

Copy link
Copy Markdown
Collaborator

Draft: needs a run on a real Linux machine with readable RAPL (bare metal, ideally a two-socket Xeon) before merge. Everything below is tested against fake sysfs trees only.

Description

On Linux, rapl_include_dram=True read the DRAM RAPL domains but their energy never reached any total, so the flag did nothing. This PR reports that energy as RAM instead of adding it to CPU energy:

  • With rapl_include_dram=True and a DRAM domain present, ram_energy / ram_power come from the measured RAPL DRAM counter, and cpu_energy no longer includes DRAM. Memory is counted once.
  • DRAM zones are found both at the top level and as children of a package (intel-rapl:0/intel-rapl:0:N, the usual server layout), and deduplicated by zone id, so a two-socket machine reports both sockets.
  • The RAM estimate stays in place when there is no DRAM domain, in tracking_mode="process" (DRAM is machine-wide), and when psys is used (psys usually includes DRAM already).
  • Until the DRAM counter is seen moving, the estimate is used; a counter that never moves over 3 intervals is dropped with a warning. A negative delta (a counter wrap that cannot be corrected) falls back to the estimate for that interval.
  • With the flag off, nothing changes.

Related Issue

Fixes #1305. Addresses #1268 for Linux RAPL. Windows EMI still adds DRAM to CPU energy; that stays a follow-up on #1268.

Motivation and Context

The first version of this PR added DRAM to cpu_energy, which counted memory twice because ram_energy is still estimated separately. The maintainer decision was to route measured DRAM into the RAM component instead, as #1268 proposes.

How Has This Been Tested?

  • uv run task test-package: 672 passed.
  • New tests, each failing before the change: test_rapl_cpu_energy_excludes_dram (flag on and off), test_rapl_reads_nested_dram_zones_once_per_socket (nested layout plus flat symlinks, 1 and 2 sockets), test_rapl_psys_does_not_read_dram, test_ram_reports_measured_dram_energy, test_ram_falls_back_to_estimate_without_live_dram, test_ram_live_dram_counter_reports_a_zero_delta_as_measured, test_ram_dead_dram_counter_is_dropped_once, test_ram_negative_dram_delta_falls_back_to_estimate, test_ram_uses_rapl_dram_when_available (including process mode).

Still to check on hardware:

  • The nested DRAM layout and multi-socket dedup on a real Xeon.
  • Whether a machine can expose the same DRAM zone through both MSR (intel-rapl) and MMIO (intel-rapl-mmio); if so, it would be counted twice in RAM.

Screenshots (if appropriate):

N/A

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Note: a measurement change for users with rapl_include_dram=True on Linux: cpu_energy drops by the DRAM share and ram_energy becomes the measured value. DRAM log lines are now named "DRAM Energy Delta_N".

AI Usage Disclosure

  • 🟥 AI-vibecoded
  • 🟠 AI-generated
  • ⭐ AI-assisted
  • ♻️ No AI used

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the docs/how-to/contributing.md document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Commits

  • 9440639 fix: report RAPL DRAM as RAM energy instead of adding it to CPU energy
  • c2cc54b and earlier: the original DRAM naming fix and review follow-ups

@codecov

codecov Bot commented Aug 12, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.70%. Comparing base (e5e46ab) to head (c2cc54b).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1344   +/-   ##
=======================================
  Coverage   91.70%   91.70%           
=======================================
  Files          49       49           
  Lines        5157     5159    +2     
=======================================
+ Hits         4729     4731    +2     
  Misses        428      428           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@davidberenstein1957
davidberenstein1957 marked this pull request as ready for review August 12, 2026 17:37
@davidberenstein1957
davidberenstein1957 requested a review from a team as a code owner August 12, 2026 17:37
@davidberenstein1957
davidberenstein1957 force-pushed the fix/rapl-include-dram branch 2 times, most recently from f5a7183 to cb38949 Compare August 19, 2026 14:29
Aggregate the dram domain into the reported processor energy alongside package/psys, with tests covering aggregation and the non-power-domain fallback.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@benoit-cty

Copy link
Copy Markdown
Contributor

🤖 This review comment was written and posted by Claude Opus 5.5 (AI assistant), at the request of @benoit-cty. Findings were checked by reading the code and running tests locally (merged with current master where relevant), but please double-check before acting on them.

Verdict: 🔧 Request changes

The bug is real. DRAM domains keep the raw name dram, and _get_energy_from_cpus / _get_power_from_cpus only sum ^Processor Energy Delta_\d / ^Processor Power. So on master, rapl_include_dram=True does nothing. Renaming DRAM to Processor Energy Delta_N fixes that, but it collides with code that landed on master after this branch was created.

Must fix:

  1. It breaks master's mirror detection, and a master test fails once merged.
    • IntelRAPL.start() (codecarbon/core/cpu.py ~L890-895) leaves DRAM out of mirror detection with if "dram" not in rapl_file.name.lower(). After the rename, DRAM files are called Processor Energy Delta_N(kWh), so the guard never matches.
    • On the PR merged with master, tests/test_rapl_mmio_scanning.py::test_rapl_start_keeps_dram_when_it_matches_a_package_counter fails: DRAM gets flagged as a mirror of a package counter and dropped.
    • Fix: base the exclusion on the domain type, not the display name. For example, keep domain_name on RAPLFile or add an is_dram flag. Also update that test's assert "dram" in details.
  2. The fallback path ignores the flag.
    • In the fallback branch of _select_domains_to_use ("No package or psys domains found, using all available domains"), readable_domains includes top-level DRAM regardless of rapl_include_dram.
    • With the rename, DRAM would then be summed as CPU energy even with the flag off. That contradicts the description.
    • Please filter DRAM by the flag there too. Add a test next to test_rapl_non_power_domain_keeps_its_own_name, which only covers core.

Worth documenting:

  • With the flag on, memory power ends up in both cpu_energy (RAPL DRAM) and ram_energy (the RAM estimate). docs/how-to/configuration.md says this is by design, but it should go in the changelog.
  • DRAM exposed as a package subzone (intel-rapl:N:M) is still skipped, so the flag stays a no-op there. That's pre-existing; maybe open an issue.
  • No psys+DRAM double count: DRAM is only added in the package branch.

@davidberenstein1957

Copy link
Copy Markdown
Collaborator Author

Made the changes in c2cc54b: merged master, DRAM mirror exclusion now uses a RAPLFile.is_dram flag (master's DRAM test updated and passing), and the fallback path respects rapl_include_dram (new test). Added the CPU+RAM double-count and subzone notes to the description's changelog section. Didn't open the subzone issue; that's for a maintainer to decide.

@github-actions github-actions Bot added size/XL and removed size/M labels Sep 28, 2026
@davidberenstein1957
davidberenstein1957 marked this pull request as draft September 28, 2026 07:36
@davidberenstein1957 davidberenstein1957 changed the title fix: count DRAM in RAPL energy total fix: report RAPL DRAM as RAM energy instead of adding it to CPU energy Sep 28, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

rapl_include_dram=True reads DRAM domains but never adds them to the energy total

2 participants