From 2cd51fa7140e28838da519a4d1091bb5452bf3b8 Mon Sep 17 00:00:00 2001 From: Ichthyostega Date: Thu, 21 Dec 2023 20:24:51 +0100 Subject: [PATCH] Scheduler-test: fix out-of-bound access MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ...causing the system to freeze due to excess memory allocation. Fortunately it turned out this was not an error in the Scheduler core or memory manager, but rather a sloppiness in the test scaffolding. However, this incident highlights that the memory manager lacks some sanity checks to prevent outright nonsensical allocation requests. Moreover it became clear again that the allocation happens ''already before'' entering the Scheduler — and thus the existing sanity check comes too late. Now I've used the same reasoning also for additional checks in the allocator, limiting the Epoch increment to 3000 and the total memory allocation to 8GiB Talking of Gibitbytes... indeed we could use a shorthand notation for that purpose... --- src/lib/meta/util.hpp | 25 + src/vault/gear/block-flow.hpp | 23 + src/vault/mem/extent-family.hpp | 18 + tests/stage/test/mock-elm.hpp | 2 - tests/vault/gear/scheduler-stress-test.cpp | 7 +- tests/vault/gear/test-chain-load.hpp | 6 +- wiki/thinkPad.ichthyo.mm | 632 ++++++++++++++++++++- 7 files changed, 697 insertions(+), 16 deletions(-) diff --git a/src/lib/meta/util.hpp b/src/lib/meta/util.hpp index 9165a5d22..391f3304b 100644 --- a/src/lib/meta/util.hpp +++ b/src/lib/meta/util.hpp @@ -429,6 +429,7 @@ namespace util { std::string showDouble (double) noexcept; std::string showFloat (float) noexcept; + std::string showSize (size_t) noexcept; @@ -464,4 +465,28 @@ namespace util { }// namespace util + + + +/* === Literals for common size designations === */ + +inline uint +operator""_KiB (unsigned long long const siz) +{ + return uint(siz) * 1024u; +} + +inline uint +operator""_MiB (unsigned long long const siz) +{ + return uint(siz) * 1024u*1024u; +} + +inline unsigned long long +operator""_GiB (unsigned long long const siz) +{ + return siz * 1024uLL*1024uLL*1024uLL; +} + + #endif /*LIB_META_UTIL_H*/ diff --git a/src/vault/gear/block-flow.hpp b/src/vault/gear/block-flow.hpp index f57e79641..6ee76e53b 100644 --- a/src/vault/gear/block-flow.hpp +++ b/src/vault/gear/block-flow.hpp @@ -106,9 +106,21 @@ namespace gear { using lib::time::Duration; using lib::time::FrameRate; + namespace err = lumiera::error; + namespace blockFlow {///< Parametrisation of Scheduler memory management scheme + /** limit for maximum number of blocks allowed in Epoch expansion + * @note SchedulerCommutator::sanityCheck() defines a similar limit, + * but there the same reasoning is translated into a hard limit for + * deadlines to be < 20sec, while this limit here will only be triggered + * if the current block duration has been lowered to the OVERLOAD_LIMIT + * @see scheduler-commutator.hpp + */ + const size_t BLOCK_EXPAND_SAFETY_LIMIT = 3000; + + /** * Lightweight yet safe parametrisation of memory management. * Used as default setting and thus for most tests. @@ -484,6 +496,7 @@ namespace gear { EpochIter nextEpoch{alloc_.end()}; ENSURE (not nextEpoch); // not valid yet, but we will allocate starting there... auto requiredNew = distance / _raw(epochStep_); + ___sanityCheckAlloc(requiredNew); if (distance % _raw(epochStep_) > 0) ++requiredNew; // fractional: requested deadline lies within last epoch alloc_.openNew(requiredNew); // Note: nextEpoch now points to the first new Epoch @@ -643,6 +656,16 @@ namespace gear { } TimeVar pastDeadline_{Time::ANYTIME}; + void + ___sanityCheckAlloc (size_t newBlockCnt) + { + if (newBlockCnt > blockFlow::BLOCK_EXPAND_SAFETY_LIMIT) + throw err::Fatal{"Deadline expansion causes allocation of " + +util::showSize(newBlockCnt) +"blocks > " + +util::showSize(blockFlow::BLOCK_EXPAND_SAFETY_LIMIT) + , err::LUMIERA_ERROR_CAPACITY}; + } + /// „backdoor“ to watch internals from tests friend class FlowDiagnostic; diff --git a/src/vault/mem/extent-family.hpp b/src/vault/mem/extent-family.hpp index a445fd995..57e21e5a4 100644 --- a/src/vault/mem/extent-family.hpp +++ b/src/vault/mem/extent-family.hpp @@ -56,6 +56,10 @@ namespace vault{ namespace mem { + namespace { + const size_t ALLOC_SAFETY_LIMIT = 8_GiB; + } + using util::unConst; template @@ -198,6 +202,7 @@ namespace mem { size_t addSiz = cnt - freeSlotCnt() + EXCESS_ALLOC; // add a strike of new extents at the end + ___sanityCheckAllocSize (addSiz); extents_.resize (oldSiz + addSiz); if (isWrapped()) {// need the new elements in the middle, before the existing start_ @@ -317,6 +322,19 @@ namespace mem { return unConst(this)->extents_[idx].access(); } // deliberately const-ness does not cover payload + void + ___sanityCheckAllocSize (size_t addCnt) + { + size_t resultSiz = slotCnt()+addCnt; + size_t requiredSpace = resultSiz * sizeof(Extent); + using namespace lumiera::error; + if (requiredSpace > ALLOC_SAFETY_LIMIT) + throw Fatal{"Raw allocation exceeds safety limit: " + +util::showSize(requiredSpace) +" > " + +util::showSize(ALLOC_SAFETY_LIMIT) + , LUMIERA_ERROR_CAPACITY}; + } + /// „backdoor“ to watch internals from tests friend class ExtentDiagnostic; diff --git a/tests/stage/test/mock-elm.hpp b/tests/stage/test/mock-elm.hpp index 8784e650b..c39c7b71b 100644 --- a/tests/stage/test/mock-elm.hpp +++ b/tests/stage/test/mock-elm.hpp @@ -79,8 +79,6 @@ namespace stage { - namespace error = lumiera::error; - using error::LUMIERA_ERROR_ASSERTION; using lib::test::EventLog; using lib::test::EventMatch; diff --git a/tests/vault/gear/scheduler-stress-test.cpp b/tests/vault/gear/scheduler-stress-test.cpp index d8250ef8b..ef0dd6a50 100644 --- a/tests/vault/gear/scheduler-stress-test.cpp +++ b/tests/vault/gear/scheduler-stress-test.cpp @@ -84,9 +84,10 @@ namespace test { smokeTest() { MARK_TEST_FUN - TestChainLoad testLoad{64}; + TestChainLoad testLoad{128}; testLoad.configureShape_chain_loadBursts() - .buildToplolgy(); + .buildToplolgy() + .printTopologyDOT(); auto stats = testLoad.computeGraphStatistics(); cout << _Fmt{"Test-Load: Nodes: %d Levels: %d ∅Node/Level: %3.1f Forks: %d Joins: %d"} @@ -119,7 +120,7 @@ namespace test { double performanceTime = testLoad.setupSchedule(scheduler) .withLoadTimeBase(LOAD_BASE) - .withJobDeadline(30ms) + .withJobDeadline(50ms) .launch_and_wait(); cout << "runTime(Scheduler): "<2? nextChunkLevel-2 : 0; return calcStartTime(nextChunkLevel) - _uTicks(preRoll_); diff --git a/wiki/thinkPad.ichthyo.mm b/wiki/thinkPad.ichthyo.mm index 6c754155d..65dc55505 100644 --- a/wiki/thinkPad.ichthyo.mm +++ b/wiki/thinkPad.ichthyo.mm @@ -57587,6 +57587,102 @@ + + + + + + + + + + + + + + +

+ ..zum einen der Namespace: das führt dann zu using-Klauseln an vielen Stellen +

+

+ ...außerdem die Sichtbarkeit: solche Definitionen erzeugen einen Sog +

+ + + + + + + + + + +

+ ...und zwar, weil for all practical purposes entweder igendwo iteriert wird (iter-adapter sind includiert) oder eine Format-Operation stattfindet oder zumindest eines der elementaren Metaprogrammnig-Hilfsmittel indirekt zum Einsatz kommt. D.h. die Wahrscheinlichkeit, daß lib/meta/util.hpp »zufällig schon« includiert wird, ist hoch, und damit kann dieser Sündenfall unter dem Radar fliegen +

+ + + + + + + + + + +

+ solange es nur limitiert genutzt wird, ist mir das lieber, als einen zentralen Header zu schaffen, der dann überall includiert wird und Begehrlichkeiten wecken könnte +

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

+ lb(1GiB) ≡ 30 +

+

+ wrap-around droht +

+ + + + + + + + + + + + +
    +
  • + bei Zuweisung an einen signed-Type passiert automatische Konversion (potentiell gefährlich). +
    • +
  • + in einem Ausdruck setzt sich der Unsigned durch (es sei denn, das andere Argument hat einen größeren Wertebereich) +
    • +
  • + Vergleiche erzeugen signed-unsigned-Warnings — aus praktischen Gründen ist das für mich ausschlaggebend +
    • +
+ + + + + + + + @@ -57613,6 +57709,15 @@ + + + + + + + + + @@ -83060,17 +83165,37 @@ Date:   Thu Apr 20 18:53:17 2023 +0200

+ - + + + + +

+ Das könnte man machen, klingt sogar sinnvoll — ABER... +

+

+ Der BlockFlow müßte dann seine eigene Config dynamischer auswerten. Das mit der Config war ein schwieriges Thema, denn ich wollte sie statisch haben, und das hat sich dann auch als sehr performance-relevant bestätigt. Daher möchte ich das Thema mit einer dynamischen Auswertung vertagen, bis wir insgesamt ein klareres Bild zum Thema System-Konfiguration haben. +

+
+

+ Die Lösung müßte in etwa so aussehen: Der BlockFlow hat dann eine Informationsfuntion, die ebenfalls auf die statische Config zugreift, und eben jeden Überschlags-Berechnungen macht, die ich hier nun mit dem Taschenrechner gemacht habe. Das würde natürlich eine Requirement-Analyse vorraussetzen, d.h. welche Informationen werden benötigt? Im Moment ist der Scheduler der einzige »Kunde«. +

+
+

+ Hinzu kommt, daß dies hier ohnehin ein zusätzliches Sicherheitsnetz ist: um den BlockFlow auf die minimale Blockgröße zu drücken, müßte man den Scheduler massivst überladen; er würde dann vermutlich die Deadline vom nächsten »Tick« überfahren, und dann eine »Scheduler-emergency« auslösen (da der Tick  compulsory ist). Das würde nach ca. 0.5 Sekunden passieren; hier aber reden wir über 20 Sekunden mit dieser Dichte. Es geht also im Grunde nur darum, daß das System kurzzeitige Lastsöße überlebt, ohne aus dem Gleichgewicht zu kommen +

+ + - + @@ -105595,6 +105720,7 @@ Date:   Thu Apr 20 18:53:17 2023 +0200
 + @@ -106342,9 +106468,9 @@ Date:   Thu Apr 20 18:53:17 2023 +0200
 - - - + + + @@ -106401,8 +106527,7 @@ Date:   Thu Apr 20 18:53:17 2023 +0200
das ist aber keine dramatische Verschlechterung, vielmehr nur sichtbar, wenn man genau nachrechnet

- - + @@ -106541,8 +106666,7 @@ Date:   Thu Apr 20 18:53:17 2023 +0200
   ·‖ E0: @8096 HT:0  -> ∘

- - + @@ -106557,9 +106681,448 @@ Date:   Thu Apr 20 18:53:17 2023 +0200
 + + + + + + + + + + + +

+ extent-family.hpp line 105 +

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

+ extent-family.hpp l.201 +

+ + + + + + + + + + +

+           if (not canAccomodate (cnt)) +

+

+             {//insufficient reserve => allocate  +

+

+               size_t oldSiz = slotCnt(); +

+

+               size_t addSiz = cnt - freeSlotCnt() +

+

+                               + EXCESS_ALLOC; +

+

+               // add a strike of new extents at the end +

+

+               extents_.resize (oldSiz + addSiz); +

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

+ block-flow.hpp, line 489 +

+ + + + + + + + + +

+               if (lastEpoch().deadline() < deadline) +

+

+                 {   // a deadline beyond the established Epochs... +

+

+                   //  create a grid of new epochs up to the requested point +

+

+                   TimeVar lastDeadline = lastEpoch().deadline(); +

+

+                   auto distance = _raw(deadline) - _raw(lastDeadline); +

+

+                   EpochIter nextEpoch{alloc_.end()}; +

+

+                   ENSURE (not nextEpoch);      // not valid yet, but we will allocate there... +

+

+                   auto requiredNew = distance / _raw(epochStep_); +

+

+                   if (distance % _raw(epochStep_) > 0) +

+

+                     ++requiredNew;  // fractional:  requested deadline lies within last epoch +

+

+                   alloc_.openNew(requiredNew); +

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

+ Fazit: es ist ein einziger Call — der verlangt Unmögiches vom Memory-Manager +

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

+ ...es geht hier um Performance-Messungen, die allerdings auch schon im Debug-Build aussagekräftig sein sollen (den Release-Build betrachte ich dann als zusätzlichen Bonus); deshalb habe ich auf viele Sicherheitsmaßnamen und Diagnose-Hilfsmittel verzichtet, die ich normalerweise in Tests einsetze +

+ + + + + + + + + + +

+ nun schon mehrfach aufgefallen: die Allokationen passieren schon vor der Verarbeitung +

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

+ ist gut, und setzt auch eine Begrenzung durch, die den Allokator vor Überforderung schützen soll, kommt aber leider zu spät für den Allokator, der ist dann schon tot +

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

+ ...hab ich in den letzten Tagen aufgeklärt und gefixt: das Grooming-Token wurde im Scheduler gedroppt, wird aber im Planungs-Job gebraucht, denn die Allokation erfolgt dort (also früher als gedacht) +

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

+ commit 892099412cdf171d9a7f960c4a5e2d2b063bd5fc +

+

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

+

+ Date:   Tue Nov 7 18:37:20 2023 +0100 +

+

+ +

+

+     Scheduler: integrate sanity check on timings +

+

+     +

+

+     ...especially to prevent a deadline way too far into the future, +

+

+     since this would provoke the BlockFlow (epoch based) memory manager +

+

+     to run out of space. +

+

+     +

+

+     Just based on gut feeling, I am now imposing a limit of 20seconds, +

+

+     which, given current parametrisation, with a minimum spacing of 6.6ms +

+

+     and 500 Activities per Block would at maximum require 360 MiB for +

+

+     the Activities, or 3000 Blocks. With *that much* blocks, the +

+

+     linear search would degrade horribly anyway... +

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

+ der sanityCheck prüft also vornehmlich daß es eine Deadline gibt +

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

+ ...dazu müßte nämlich der BlockFlow viel dynamischer seine Config auswerten — lästiges Thema — YAGNI +

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

+ wähle hier 8 GiB für die totale Allokation +

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

+ begrenze hier konsistent mit dem Scheduler auf +3000 neue Blöcke pro Schritt +

+ + + + + + + + + +

+ weil das Limit im BlockFlow-Allokator nur greift, wenn wir tatsächlich  auf Minimal-Blockggröße unten sind, während der Scheduler eine feste Obergrenze für die Deadlines erzwingt. +

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

+ und ist jetzt sogar schenller als der single-threaded Fall +

+ + + + + + + + + + + + + + + + + + + + + + + + + + + @@ -112950,6 +113513,57 @@ Date:   Thu Apr 20 18:53:17 2023 +0200
 + + + + + + + + + + + + + + + + + + + + + + + + + + +

+ the corresponding unsigned type has the same rank +

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