From fd89e6d7081cfd756525843ecebc3ada48e64119 Mon Sep 17 00:00:00 2001 From: Sandro Wenzel Date: Sat, 25 Jul 2026 18:55:55 +0200 Subject: [PATCH] Fix CPU accounting for external generator subprocesses 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 --- Framework/Core/src/CommonServices.cxx | 17 ++++++++++++ Generators/include/Generators/Generator.h | 4 +++ .../include/Generators/GeneratorFileOrCmd.h | 16 +++++++++++- .../include/Generators/GeneratorHepMC.h | 3 +++ .../include/Generators/GeneratorService.h | 3 +++ .../include/Generators/GeneratorTParticle.h | 3 +++ Generators/src/GeneratorFileOrCmd.cxx | 21 ++++++++++++++- Generators/src/GeneratorHepMC.cxx | 26 ++++++++++++++----- Generators/src/GeneratorService.cxx | 16 ++++++++++++ Generators/src/GeneratorTParticle.cxx | 12 ++++++++- run/dpl_eventgen.cxx | 3 +++ 11 files changed, 115 insertions(+), 9 deletions(-) diff --git a/Framework/Core/src/CommonServices.cxx b/Framework/Core/src/CommonServices.cxx index 2cdd046dedc34..0bc701a01d8ea 100644 --- a/Framework/Core/src/CommonServices.cxx +++ b/Framework/Core/src/CommonServices.cxx @@ -54,12 +54,14 @@ #include "DecongestionService.h" #include "ArrowSupport.h" #include "DPLMonitoringBackend.h" +#include "ResourcesMonitoringHelper.h" #include "Headers/STFHeader.h" #include "Headers/DataHeader.h" #include #include #include +#include #include "Framework/Signpost.h" #include @@ -119,6 +121,15 @@ o2::framework::ServiceSpec CommonServices::monitoringSpec() .start = [](ServiceRegistryRef services, void* service) { auto* monitoring = (o2::monitoring::Monitoring*)service; + // Re-arm process monitoring: .stop takes the final measurement and stops + // the sampling thread, so without this a device would report nothing at + // all from its second run onwards. A no-op while already running. + auto interval = services.get().resourceMonitoringInterval; + if (ResourcesMonitoringHelper::isResourcesMonitoringEnabled(interval)) { + using o2::monitoring::PmMeasurement; + monitoring->enableProcessMonitoring(interval, {PmMeasurement::Cpu, PmMeasurement::Mem, PmMeasurement::Smaps}); + } + auto extRunNumber = services.get().device()->fConfig->GetProperty("runNumber", "unspecified"); if (extRunNumber == "unspecified") { return; @@ -127,6 +138,12 @@ o2::framework::ServiceSpec CommonServices::monitoringSpec() monitoring->setRunNumber(std::stoul(extRunNumber)); } catch (...) { } }, + // Final measurement here rather than in ~Monitoring() at .exit, which is + // not reliably reached before the process exits. Unlike postEOS this also + // covers devices that quit themselves via readyToQuit(). + .stop = [](ServiceRegistryRef, void* service) { + auto* monitoring = reinterpret_cast(service); + monitoring->finalizeProcessMonitoring(); }, .exit = [](ServiceRegistryRef registry, void* service) { auto* monitoring = reinterpret_cast(service); monitoring->flushBuffer(); diff --git a/Generators/include/Generators/Generator.h b/Generators/include/Generators/Generator.h index f413aeccfa3ab..7001a48410dd7 100644 --- a/Generators/include/Generators/Generator.h +++ b/Generators/include/Generators/Generator.h @@ -105,6 +105,10 @@ class Generator : public FairGenerator /** notification methods **/ virtual void notifyEmbedding(const o2::dataformats::MCEventHeader* eventHeader){}; + /** Release external resources (forked subprocesses, open files, ...) once + * the generator is no longer needed, without relying on destructor timing **/ + virtual void stop() {} + void setTriggerOkHook(std::function const& p, int eventCount)> f) { mTriggerOkHook = f; } void setTriggerFalseHook(std::function const& p, int eventCount)> f) { mTriggerFalseHook = f; } diff --git a/Generators/include/Generators/GeneratorFileOrCmd.h b/Generators/include/Generators/GeneratorFileOrCmd.h index 5a8f3411e883c..7d1c1e7a97e2a 100644 --- a/Generators/include/Generators/GeneratorFileOrCmd.h +++ b/Generators/include/Generators/GeneratorFileOrCmd.h @@ -143,9 +143,23 @@ struct GeneratorFileOrCmd { * Terminates the background command using PID of the child * process generated by fork. * + * @param graceMillis How long to wait for the command to exit on its + * own before killing its process group. A command started + * through an intermediate shell (a wrapper script, say) only + * contributes the CPU time of the actual generator to this + * process' RUSAGE_CHILDREN once that shell has reaped it, which + * it can no longer do once we have killed it. Zero preserves + * the previous kill-immediately behaviour. + * * @return true if the process was terminated successfully */ - virtual bool terminateCmd(); + virtual bool terminateCmd(unsigned int graceMillis = 0); + /** + * Time granted to the command to exit by itself when the generator is + * stopped normally. It has produced everything that was asked of it by + * then, so this only covers its teardown. + */ + static constexpr unsigned int sStopGraceMillis = 5000; /** * Create a temporary file (and close it immediately). On success, * the list of file names is cleared and the name of the temporary diff --git a/Generators/include/Generators/GeneratorHepMC.h b/Generators/include/Generators/GeneratorHepMC.h index 3c8172adb1009..176a4020e213f 100644 --- a/Generators/include/Generators/GeneratorHepMC.h +++ b/Generators/include/Generators/GeneratorHepMC.h @@ -86,6 +86,9 @@ class GeneratorHepMC : public Generator, public GeneratorFileOrCmd */ Bool_t importParticles() override; + /** Terminate the background command (if any), see Generator::stop(). */ + void stop() override; + /** setters **/ void setEventsToSkip(uint64_t val) { mEventsToSkip = val; }; void setVersion(const int& ver) { mVersion = ver; }; diff --git a/Generators/include/Generators/GeneratorService.h b/Generators/include/Generators/GeneratorService.h index 13ebe054f2940..18084ba6c34e7 100644 --- a/Generators/include/Generators/GeneratorService.h +++ b/Generators/include/Generators/GeneratorService.h @@ -70,6 +70,9 @@ class GeneratorService void generateEvent_MCTracks(o2::pmr::vector& tracks, o2::dataformats::MCEventHeader& header); void generateEvent_TParticles(std::vector& tparts, o2::dataformats::MCEventHeader& header); + /** Calls Generator::stop() on all registered generators **/ + void stopGenerators(); + private: PrimaryGenerator mPrimGen; o2::data::Stack mStack; diff --git a/Generators/include/Generators/GeneratorTParticle.h b/Generators/include/Generators/GeneratorTParticle.h index e4ddb5fa1f340..8cf34ccf552e3 100644 --- a/Generators/include/Generators/GeneratorTParticle.h +++ b/Generators/include/Generators/GeneratorTParticle.h @@ -73,6 +73,9 @@ class GeneratorTParticle : public Generator, public GeneratorFileOrCmd * program */ Bool_t Init() override; + /** Terminate the background command (if any), see Generator::stop(). */ + void stop() override; + /** * Configure the generator from parameters and the general * simulation configuration. This is implemented as a member diff --git a/Generators/src/GeneratorFileOrCmd.cxx b/Generators/src/GeneratorFileOrCmd.cxx index bc2083e025c14..3ee8eff6c8cd3 100644 --- a/Generators/src/GeneratorFileOrCmd.cxx +++ b/Generators/src/GeneratorFileOrCmd.cxx @@ -142,13 +142,32 @@ bool GeneratorFileOrCmd::executeCmdLine(const std::string& cmd) return true; } // ----------------------------------------------------------------- -bool GeneratorFileOrCmd::terminateCmd() +bool GeneratorFileOrCmd::terminateCmd(unsigned int graceMillis) { if (mCmdPid == -1) { LOG(info) << "No command is currently running"; return false; } + // Let the command finish by itself if it is about to: killing the process + // group first would deprive an intermediate shell of the chance to reap the + // actual generator, and with it this process of the generator's CPU time, + // which only reaches RUSAGE_CHILDREN through that reap. + constexpr unsigned int pollMillis = 10; + for (unsigned int waited = 0; waited < graceMillis; waited += pollMillis) { + int status; + pid_t reaped = waitpid(mCmdPid, &status, WNOHANG); + if (reaped == mCmdPid) { + LOG(info) << "Command with process ID " << mCmdPid << " exited by itself"; + mCmdPid = -1; + return true; + } + if (reaped == -1) { + break; // not our child (any more): let the kill path report it + } + std::this_thread::sleep_for(std::chrono::milliseconds(pollMillis)); + } + LOG(info) << "Terminating process ID group " << mCmdPid; if (kill(-mCmdPid, SIGKILL) == -1) { LOG(fatal) << "Failed to kill process: " << std::strerror(errno); diff --git a/Generators/src/GeneratorHepMC.cxx b/Generators/src/GeneratorHepMC.cxx index faacde7317664..d493366c46188 100644 --- a/Generators/src/GeneratorHepMC.cxx +++ b/Generators/src/GeneratorHepMC.cxx @@ -66,15 +66,29 @@ GeneratorHepMC::~GeneratorHepMC() if (mEvent) { delete mEvent; } - if (not mCmd.empty()) { - // Must be executed before removing the temporary file - // otherwise the current child process might still be writing on it - // causing unwanted stdout messages which could slow down the system - terminateCmd(); - } + stop(); removeTemp(); } +/*****************************************************************/ + +void GeneratorHepMC::stop() +{ + if (mCmd.empty()) { + return; + } + // Close our end of the pipe first: a generator still blocked writing to it + // then sees EPIPE and exits promptly, instead of sitting out the whole grace + // period and being killed - which would lose its CPU time (see terminateCmd). + if (mReader) { + mReader->close(); + } + // Must be executed before removing the temporary file + // otherwise the current child process might still be writing on it + // causing unwanted stdout messages which could slow down the system + terminateCmd(sStopGraceMillis); +} + /*****************************************************************/ void GeneratorHepMC::setup(const GeneratorFileOrCmdParam& param0, const GeneratorHepMCParam& param, diff --git a/Generators/src/GeneratorService.cxx b/Generators/src/GeneratorService.cxx index ae0de385a1b23..8873fc75a43d3 100644 --- a/Generators/src/GeneratorService.cxx +++ b/Generators/src/GeneratorService.cxx @@ -14,6 +14,8 @@ #include "SimConfig/SimConfig.h" #include "Generators/Generator.h" #include "DataFormatsCalibration/MeanVertexObject.h" +#include +#include using namespace o2::eventgen; @@ -90,3 +92,17 @@ void GeneratorService::generateEvent_TParticles(std::vector& tracks, tracks.clear(); tracks = mStack.getPrimaries(); } + +void GeneratorService::stopGenerators() +{ + auto* generators = mPrimGen.GetListOfGenerators(); + if (!generators) { + return; + } + TIter next(generators); + while (TObject* obj = next()) { + if (auto* gen = dynamic_cast(obj)) { + gen->stop(); + } + } +} diff --git a/Generators/src/GeneratorTParticle.cxx b/Generators/src/GeneratorTParticle.cxx index 06b4cbc147fca..47888df9cf108 100644 --- a/Generators/src/GeneratorTParticle.cxx +++ b/Generators/src/GeneratorTParticle.cxx @@ -43,10 +43,20 @@ GeneratorTParticle::~GeneratorTParticle() if (mCmd.empty()) { return; } - + // Must be executed before removing the temporary file, otherwise the child + // process might still be writing to it + stop(); removeTemp(); } /*****************************************************************/ +void GeneratorTParticle::stop() +{ + if (mCmd.empty()) { + return; + } + terminateCmd(sStopGraceMillis); +} +/*****************************************************************/ Bool_t GeneratorTParticle::Init() { mChain = new TChain(mTreeName.c_str()); diff --git a/run/dpl_eventgen.cxx b/run/dpl_eventgen.cxx index 3df16ee3e5ebb..296780305cbea 100644 --- a/run/dpl_eventgen.cxx +++ b/run/dpl_eventgen.cxx @@ -132,6 +132,9 @@ struct GeneratorTask { } } if (eventCounter >= nEvents || time_expired) { + // terminate and reap external generator subprocesses here: device + // teardown is not guaranteed to run the generators' destructors + genservice->stopGenerators(); pc.services().get().endOfStream(); pc.services().get().readyToQuit(QuitRequest::Me);