Account for CPU used by child processes - #378
Open
sawenzel wants to merge 2 commits into
Open
Conversation
sawenzel
force-pushed
the
fix/o2-7096-child-cpu-accounting
branch
from
July 25, 2026 17:09
4f8de26 to
72323b3
Compare
Author
|
This is part of a fix proposed by Claude Code Opus 5 to fix JIRA issue https://its.cern.ch/jira/browse/O2-7096; Needs to be merged and tagged before the related O2 PR AliceO2Group/AliceO2#15636; |
getCpuAndContexts() only sampled RUSAGE_SELF, so CPU burned by forked children was never reported. Sample RUSAGE_CHILDREN as well. The final measurement is reachable via finalizeProcessMonitoring() instead of only ~Monitoring(), and bypasses the 1s rate guard, which would otherwise discard a delta that no later call can pick up. Details: * RUSAGE_CHILDREN only becomes non-zero once a child has been reaped, so this is one half of the fix: the caller has to reap the child, and has to do so without killing an intermediate shell first. See the companion change in AliceO2Group/AliceO2#15636. * Forced (final) measurements are deliberately excluded from the percentage series. A reaped child's CPU becomes visible as one lump, and lump / (time since the last sample) is a meaningless rate - 14315% was observed before this exclusion. Only the absolute and accumulated fields carry meaning for such a sample, so consumers that care about external subprocesses (Hyperloop accounting) should read cpuTimeConsumedByProcess, not the percentage series. * Consequently a process that ends before the first periodic sample has no percentage at all - pushLoop() sleeps 100ms before sampling - and averaging over the empty series produced a NaN averageCpuUsedPercentage. Verified with a Monitoring instance destroyed after 10ms: 'nan' before, '0.14' on the unpatched library. Such a measurement now reports no average instead of a NaN one. * init() clears the aggregates, so monitoring that is stopped and started again reports the new period rather than blending it with the previous one. DPL devices go RUNNING -> READY -> RUNNING across runs and re-arm process monitoring on start. Related to https://its.cern.ch/jira/browse/O2-7096 Assisted by Claude Opus 5
sawenzel
added a commit
to sawenzel/AliceO2
that referenced
this pull request
Jul 25, 2026
GeneratorFileOrCmd forks an external generator but the child was only reaped from ~GeneratorHepMC(), which is not reached on the DPL device teardown path. Its CPU therefore never showed up in RUSAGE_CHILDREN. Generators: add Generator::stop(), called from GeneratorTask::run() once the stream ends, to reap the child deterministically. Framework: add a Monitoring .stop callback to take the final CPU measurement on the RUNNING -> READY transition. This also covers devices that end their own stream via readyToQuit(), for which postEOS is never invoked. Details: * Reaping alone is not enough. terminateCmd() SIGKILLs the process group, and a generator started through a wrapper script - epos.sh, and any command line that /bin/sh does not exec in place - is a grandchild whose CPU reaches us only through the intermediate shell's own reap. Killing that shell first loses the accounting completely, which made the fix a race that usually lost. terminateCmd() now waits a bounded time for the command to exit by itself before escalating to SIGKILL, and GeneratorHepMC::stop() closes the read end of the pipe first, so a generator still blocked writing to it sees EPIPE and exits promptly instead of sitting out the grace period. Measured with a Pythia8 -> HepMC3 external command (same sh -> binary -> fifo topology as EPOS4, 3.5s of child CPU, ground truth from the child's own getrusage): cpuTimeConsumedByProcess was correct in 3 of 10 runs before and in 10 of 10 after, with no change in wall time. Same result when the generator produces more events than the device consumes (0.18s reported before, 3.62s after). * GeneratorTParticle spawns a command the same way but never called terminateCmd() at all, leaking the process as well as its CPU. It now overrides stop() too. * The .stop callback stops the sampling thread, while process monitoring is armed only once, at device instantiation. Without a counterpart a device would report nothing at all from its second run onwards (reproduced: no periodic samples and no aggregates after the first stop), so .start re-arms it. That call is a no-op while monitoring is already running. * Requires AliceO2Group/Monitoring#378, which introduces finalizeProcessMonitoring() and the RUSAGE_CHILDREN sampling. That has to be merged, tagged and pinned in alidist before this one, or dev stops compiling. Fixes https://its.cern.ch/jira/browse/O2-7096 Assisted by Claude Opus 5
sawenzel
force-pushed
the
fix/o2-7096-child-cpu-accounting
branch
from
July 25, 2026 20:48
72323b3 to
068142c
Compare
ktf
reviewed
Aug 6, 2026
| } | ||
|
|
||
| Monitoring::~Monitoring() | ||
| void Monitoring::finalizeProcessMonitoring() |
Member
There was a problem hiding this comment.
Small remark. This is not thread safe. You probably want a:
bool expected = true;
if (!mMonitorRunning.compare_exchange_strong(expected, false)) {
return;
}
It should not matter for DPL (where the .stop will always be called on the main thread) however I do not know if there are other usages of this elsewhere. Given at some point it was advertised that Monitoring is thread safe, I do not know if that's an hard-requirement.
| mCpuPerctange.push_back(cpuUsedPerctange); | ||
| metrics.emplace_back(Metric{cpuUsedPerctange, metricsNames[CPU_USED_PERCENTAGE]}); | ||
| } | ||
| metrics.emplace_back(Metric{ |
Member
There was a problem hiding this comment.
Notice that the context switches will also be wrong.
Member
|
Notice that RUSAGE_CHILDREN reports only children which have |
The previous commit made the CPU numbers the sum over this process and its reaped children, but left the two context-switch counts on RUSAGE_SELF alone, so a single measurement mixed a process-tree quantity with a parent-only one. Both counts now sum the same pair of snapshots. Raised in review. The surrounding measurement code is tidied while we are here: the CPU delta is expressed in the same shape as the context-switch deltas, and the percentage is computed only on the path that actually reports one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Author
|
The PR has been updated: context switches now sum self and children, like the CPU numbers as pointed out in comments Then about the reap question: only waited-for children count, which is what we want here. A generator's CPU shows up only when it's reaped, so the O2 PR now reaps it when the stream ends, instead of from a destructor that runs after the last measurement. Concerning thread safety: I don't think the class is thread safe anyway (enableProcessMonitoring does the same thing, and the backends aren't guarded), so I would suggest to do a separate thread-safety PR as an independent step. I would suggest to squash all commits when merging. |
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
2 participants
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.
getCpuAndContexts() only sampled RUSAGE_SELF, so CPU burned by forked children was never reported. Sample RUSAGE_CHILDREN as well.
The final measurement is reachable via finalizeProcessMonitoring() instead of only ~Monitoring(), and bypasses the 1s rate guard, which would otherwise discard a delta that no later call can pick up.
Related to https://its.cern.ch/jira/browse/O2-7096
Assisted by Claude Opus 5