Uh oh!
There was an error while loading. Please reload this page.
Account for CPU used by child processes - #378
Open
sawenzel wants to merge 1 commit into
Open
Conversation
sawenzelforce-pushed
the
fix/o2-7096-child-cpu-accounting
branch
from
July 25, 2026 17:09
4f8de26 to
72323b3Comparesawenzel
commented
Jul 25, 2026
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
sawenzelforce-pushed
the
fix/o2-7096-child-cpu-accounting
branch
from
July 25, 2026 20:48
72323b3 to
068142cComparektf
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.
ktf
commented
Aug 6, 2026
Member
Notice that RUSAGE_CHILDREN reports only children which have |
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 freeto 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