From b497980522d0f3fbb17ff67b33604a464be5e5b3 Mon Sep 17 00:00:00 2001 From: Ichthyostega Date: Tue, 19 Dec 2023 23:38:57 +0100 Subject: [PATCH] Scheduler-test: guard memory allocations by grooming-token MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Turns out that we need to implemented fine grained and explicit handling logic to ensure that Activity planning only ever happens protected by the Grooming-Token. This is in accordance to the original design, which dictates that all management tasks must be done in »management mode«, which can only be entered by a single thread at a time. The underlying assumption is that the effort for management work is dwarfed in comparison to any media calculation work. However, in 5c6354882de1be ...I discovered an insidious border condition, an in an attempt to fix it, I broke that fundamental assumpton. The problem arises from the fact that we do want to expose a *public API* of the Scheduler. Even while this is only used to ''seed'' a calculation stream, because any further planning- and management work will be performed by the workers themselves (this is a design decision, we do not employ a "scheduler thread") Anyway, since the Scheduler API ''is'' public, ''someone from the outside'' could invoke those functions, and — unaware of any Scheduler internals — will automatically acquire the Grooming-Token, yet never release it, leading to deadlock. So we need a dedicated solution, which is hereby implemented as a scoped guard: in the standard case, the caller is a management-job and thus already holds the token (and nothing must be done). But in the rare case of an »outsider«, this guard now ''transparently'' acquires the token (possibly with a blocking wait) and ''drops it when leaving scope'' --- src/vault/gear/scheduler-commutator.hpp | 63 ++++ src/vault/gear/scheduler.hpp | 9 +- .../vault/gear/scheduler-commutator-test.cpp | 33 +++ wiki/thinkPad.ichthyo.mm | 270 +++++++++++++++++- 4 files changed, 367 insertions(+), 8 deletions(-) diff --git a/src/vault/gear/scheduler-commutator.hpp b/src/vault/gear/scheduler-commutator.hpp index 4d144944a..a882fc12d 100644 --- a/src/vault/gear/scheduler-commutator.hpp +++ b/src/vault/gear/scheduler-commutator.hpp @@ -93,10 +93,13 @@ namespace gear { using std::memory_order::memory_order_relaxed; using std::memory_order::memory_order_acquire; using std::memory_order::memory_order_release; + using std::chrono_literals::operator ""us; + using std::chrono::microseconds; namespace { // Configuration / Scheduling limit Offset FUTURE_PLANNING_LIMIT{FSecs{20}}; ///< limit timespan of deadline into the future (~360 MiB max) + microseconds GROOMING_WAIT_CYCLE{70us}; ///< wait-sleep in case a thread must forcibly acquire the Grooming-Token /** convenient short-notation, also used by SchedulerService */ auto inline thisThread() { return std::this_thread::get_id(); } @@ -165,6 +168,12 @@ namespace gear { } + class ScopedGroomingGuard; + /** a scope guard to force acquisition of the GroomingToken */ + ScopedGroomingGuard requireGroomingTokenHere(); + + + void sanityCheck (ActivationEvent const& event, Time now) { @@ -279,5 +288,59 @@ namespace gear { }; + + + class SchedulerCommutator::ScopedGroomingGuard + : util::MoveOnly + { + SchedulerCommutator& commutator_; + bool handledActively_; + + bool + ensureHoldsToken() + { + if (commutator_.holdsGroomingToken(thisThread())) + return false; + while (not commutator_.acquireGoomingToken()) + std::this_thread::sleep_for (GROOMING_WAIT_CYCLE); + return true; + } + + public: + /** @warning can block indefinitely if someone hogs the token */ + ScopedGroomingGuard (SchedulerCommutator& layer2) + : commutator_(layer2) + , handledActively_{ensureHoldsToken()} + { } + + ~ScopedGroomingGuard() + { + if (handledActively_) + commutator_.dropGroomingToken(); + } + }; + + + /** + * @warning this provides very specific functionality + * required by the »Scheduler Service« to handle both + * _external_ and _internal_ calls properly. + * - whenever a thread already holds the GroomingToken, + * no further action is performed (so the cost of this + * feature is one additional atomic read on the token) + * - however, a thread coming _from the outside_ and not + * belonging to the Scheduler ecosystem is typically not + * aware of the GroomingToken altogether. The token is + * acquired, possibly incurring a *blocking wait*, and + * it is dropped transparently when leaving the scope. + */ + inline SchedulerCommutator::ScopedGroomingGuard + SchedulerCommutator::requireGroomingTokenHere() + { + return ScopedGroomingGuard(*this); + } + + + }} // namespace vault::gear #endif /*SRC_VAULT_GEAR_SCHEDULER_COMMUTATOR_H_*/ diff --git a/src/vault/gear/scheduler.hpp b/src/vault/gear/scheduler.hpp index 16696567f..3231c65ca 100644 --- a/src/vault/gear/scheduler.hpp +++ b/src/vault/gear/scheduler.hpp @@ -336,6 +336,7 @@ namespace gear { ,ManifestationID manID = ManifestationID() ,FrameRate expectedAdditionalLoad = FrameRate(25)) { + auto guard = layer2_.requireGroomingTokenHere(); // allow mutation layer1_.activate (manID); activityLang_.announceLoad (expectedAdditionalLoad); continueMetaJob (RealClock::now(), planningJob, manID); @@ -352,6 +353,7 @@ namespace gear { { bool isCompulsory = true; Time deadline = nextStart + DUTY_CYCLE_TOLERANCE; + auto guard = layer2_.requireGroomingTokenHere(); // protect allocation // place the meta-Job into the timeline... postChain ({activityLang_.buildMetaJob(planningJob, nextStart, deadline) .post() @@ -359,7 +361,6 @@ namespace gear { , deadline , manID , isCompulsory}); - ensureDroppedGroomingToken(); } @@ -574,7 +575,8 @@ namespace gear { */ inline ScheduleSpec ScheduleSpec::post() - { + { // protect allocation + auto guard = theScheduler_->layer2_.requireGroomingTokenHere(); term_ = move( theScheduler_->activityLang_ .buildCalculationJob (job_, start_,death_)); @@ -583,7 +585,6 @@ namespace gear { , death_ , manID_ , isCompulsory_}); - theScheduler_->ensureDroppedGroomingToken(); return move(*this); } @@ -732,6 +733,8 @@ namespace gear { inline void Scheduler::handleDutyCycle (Time now, bool forceContinuation) { + auto guard = layer2_.requireGroomingTokenHere(); + // consolidate queue content layer1_.feedPrioritisation(); // clean-up of outdated tasks here diff --git a/tests/vault/gear/scheduler-commutator-test.cpp b/tests/vault/gear/scheduler-commutator-test.cpp index dfa15238a..0a00b13a2 100644 --- a/tests/vault/gear/scheduler-commutator-test.cpp +++ b/tests/vault/gear/scheduler-commutator-test.cpp @@ -95,6 +95,7 @@ namespace test { { demonstrateSimpleUsage(); verify_GroomingToken(); + verify_GroomingGuard(); torture_GroomingToken(); verify_DispatchDecision(); verify_findWork(); @@ -162,6 +163,38 @@ namespace test { + /** @test verify extended logic to protect a scope + * - if the thread already holds the grooming token, nothing happens + * - otherwise, it is acquired (blocking) and dropped on exit + */ + void + verify_GroomingGuard() + { + SchedulerCommutator sched; + + // Case-1: if a thread already holds the token.... + CHECK (sched.acquireGoomingToken()); + CHECK (sched.holdsGroomingToken (thisThread())); + { + auto guard = sched.requireGroomingTokenHere(); + CHECK (sched.holdsGroomingToken (thisThread())); + }// leave scope -> nothing happens in this case + CHECK (sched.holdsGroomingToken (thisThread())); + + // Case-2: when not holding the token... + sched.dropGroomingToken(); + { + // acquire automatically (this may block) + auto guard = sched.requireGroomingTokenHere(); + CHECK (sched.holdsGroomingToken (thisThread())); + }// leave scope -> dropped automatically + CHECK (not sched.holdsGroomingToken (thisThread())); + + ___ensureGroomingTokenReleased(sched); + } + + + /** @test ensure the GroomingToken mechanism indeed creates mutual * exclusion to protected against concurrent corruption. * @remark uses lib::test::threadBenchmark() to put the test-subject diff --git a/wiki/thinkPad.ichthyo.mm b/wiki/thinkPad.ichthyo.mm index b9d206f23..3d32455d6 100644 --- a/wiki/thinkPad.ichthyo.mm +++ b/wiki/thinkPad.ichthyo.mm @@ -104754,8 +104754,8 @@ Date:   Thu Apr 20 18:53:17 2023 +0200
- + @@ -105214,10 +105214,128 @@ Date:   Thu Apr 20 18:53:17 2023 +0200
- - - - + + + + + + + + +

+ dieses Prinzip könnte man aufweichen — ich halte das aber für eine schlechte Idee, denn der Scheduler ist auch so schon schwer genug zu verstehen, auch die Activity-Language beginnt schon zu »bröseln« ... gefährliche Tendenz. Meine Einschätzung ist, wir planen ehr so 10 Jobs pro Metajob-Lauf, d.h. das wären dann Größenordnung 500-1000µs Zeit, in der jedwede andere Management-Tätigkeit geblockt ist (renderjobs laufen trotzdem weiter). Sofern diese Annahme nicht zutrifft, möchte ich dafür konkrete Beweise sehen +

+ + + + + + + + + + + + + + + +

+ commit 5c6354882de1be63724221022b374bd559a499b0 +

+

+ Author: Ichthyostega <prg@ichthyostega.de> +

+

+ Date:   Wed Nov 8 20:58:32 2023 +0100 +

+

+ +

+

+     Scheduler: solve problem with transport from entrance-queue +

+

+     +

+

+     The test case "scheduleRenderJob()" -- while deliberately operated +

+

+     quite artificially with a disabled WorkForce (so the test can check +

+

+     the contents in the queue and then progress manually -- led to discovery +

+

+     of an open gap in the logic: in the (rare) case that a new task is +

+

+     added ''from the outside'' without acquiring the Grooming-Token, then +

+

+     the new task could sit in the entrace queue, in worst case for 50ms, +

+

+     until the next Scheduler-»Tick« routinely sweeps this queue. Under +

+

+     normal conditions however, each dispatch of another activity will +

+

+     also sweep the entrance queue, yet if there happens to be no other +

+

+     task right now, a new task could be stuck. +

+

+     +

+

+     Thinking through this problem also helped to amend some aspects +

+

+     of Grooming-Token handling and clarified the role of the API-functions. +

+ + + + + + + + + + + + +

+ aber sie dann auch zusätzlich in das post()  eingesetzt +

+ + + + + + + + + + + + + + +

+ ...dann erlangt er es, und behält es +

+ + + + + + + @@ -105238,6 +105356,148 @@ Date:   Thu Apr 20 18:53:17 2023 +0200
 + + + + + + + + + + + + + + + + + + + + + + + + + + +

+ und: die Allokation findet hier statt +

+ + + + + + + + +

+     term_ = move( +

+

+       theScheduler_->activityLang_ +

+

+           .buildCalculationJob (job_, start_,death_)); +

+ + + + + + + + + + + + + +

+ ...eben wenn jemand von außen kommt... das ist i.d.R. nur beim seedCalcStream() der Fall +

+ + + + + + + + + + + + + + + + + + + + + + +

+ zum einen ist das nicht genau das feste Raster, und außerdem braucht der OS-Scheduler sowiso immer etwas länger +

+ + + + + + + + + + + + + + + + +

+ sage bewußt »dokumentieren«, denn diese Logik hier ist derart einfach zu implementieren, daß eigentlich kein Test notwendig wäre +

+ + + + + + + + + + + +

+ das ist nämlich unverhältnismäßig schwer im Test zu verifizieren, und steht in keinem Verhältnis zur Aufgabe +

+ + + + + + + + + + + + + + + + + + + + + +