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);