From 1dc8384c419b59e8da9c6af88902031fca4afaa5 Mon Sep 17 00:00:00 2001 From: Josiah VanderZee Date: Sat, 19 Oct 2024 11:56:28 -0500 Subject: [PATCH 1/5] Add a constructor to `PreservationTable` --- src/iocore/cache/PreservationTable.cc | 13 +++++++++++++ src/iocore/cache/PreservationTable.h | 12 +++++++++++- src/iocore/cache/StripeSM.cc | 8 ++------ 3 files changed, 26 insertions(+), 7 deletions(-) diff --git a/src/iocore/cache/PreservationTable.cc b/src/iocore/cache/PreservationTable.cc index c45deb2d953..5b3237d767d 100644 --- a/src/iocore/cache/PreservationTable.cc +++ b/src/iocore/cache/PreservationTable.cc @@ -31,7 +31,12 @@ #include "tsutil/DbgCtl.h" +#include "tscore/ink_assert.h" +#include "tscore/ink_memory.h" +#include "tscore/List.h" + #include +#include namespace { @@ -40,6 +45,14 @@ DbgCtl dbg_ctl_cache_evac{"cache_evac"}; } // namespace +PreservationTable::PreservationTable(int size) : evacuate_size{(size / EVACUATION_BUCKET_SIZE) + 2} +{ + ink_assert(size > 0); + int evac_len = this->evacuate_size * sizeof(DLL); + this->evacuate = static_cast *>(ats_malloc(evac_len)); + memset(static_cast(this->evacuate), 0, evac_len); +} + EvacuationBlock * PreservationTable::find(Dir const &dir) const { diff --git a/src/iocore/cache/PreservationTable.h b/src/iocore/cache/PreservationTable.h index 7641456b457..24013fc6f87 100644 --- a/src/iocore/cache/PreservationTable.h +++ b/src/iocore/cache/PreservationTable.h @@ -36,8 +36,8 @@ #include "tscore/ink_platform.h" #include "tscore/List.h" -#define EVACUATION_BUCKET_SIZE (2 * EVACUATION_SIZE) // 16MB #define EVACUATION_SIZE (2 * AGG_SIZE) // 8MB +#define EVACUATION_BUCKET_SIZE (2 * EVACUATION_SIZE) // 16MB #define PIN_SCAN_EVERY 16 // scan every 1/16 of disk #define dir_offset_evac_bucket(_o) (_o / (EVACUATION_BUCKET_SIZE / CACHE_BLOCK_SIZE)) @@ -99,6 +99,16 @@ class PreservationTable */ DLL *evacuate{nullptr}; + /** + * PreservationTable constructor + * + * This will abort the program if it is unable to allocate memory. + * + * @param size The total number of directory entries the table must + * be able to store. The size must be a positive number. + */ + PreservationTable(int size); + /** * Check whether the hash table may be indexed with the given offset. * diff --git a/src/iocore/cache/StripeSM.cc b/src/iocore/cache/StripeSM.cc index d3c901ec59b..237dea354ce 100644 --- a/src/iocore/cache/StripeSM.cc +++ b/src/iocore/cache/StripeSM.cc @@ -109,13 +109,9 @@ struct StripeInitInfo { } }; -StripeSM::StripeSM(CacheDisk *disk, off_t blocks, off_t dir_skip) : Continuation(new_ProxyMutex()), Stripe{disk, blocks, dir_skip} +StripeSM::StripeSM(CacheDisk *disk, off_t blocks, off_t dir_skip) + : Continuation(new_ProxyMutex()), Stripe{disk, blocks, dir_skip}, _preserved_dirs{static_cast(len)} { - this->_preserved_dirs.evacuate_size = static_cast(len / EVACUATION_BUCKET_SIZE) + 2; - int evac_len = this->_preserved_dirs.evacuate_size * sizeof(DLL); - this->_preserved_dirs.evacuate = static_cast *>(ats_malloc(evac_len)); - memset(static_cast(this->_preserved_dirs.evacuate), 0, evac_len); - open_dir.mutex = this->mutex; SET_HANDLER(&StripeSM::aggWrite); } From c849fc8087a8270ec4eccee0c086df13a8b66ec2 Mon Sep 17 00:00:00 2001 From: Josiah VanderZee Date: Sat, 19 Oct 2024 12:02:19 -0500 Subject: [PATCH 2/5] Delete `PreservationTable` copy constructors --- src/iocore/cache/PreservationTable.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/iocore/cache/PreservationTable.h b/src/iocore/cache/PreservationTable.h index 24013fc6f87..701137120b5 100644 --- a/src/iocore/cache/PreservationTable.h +++ b/src/iocore/cache/PreservationTable.h @@ -109,6 +109,9 @@ class PreservationTable */ PreservationTable(int size); + PreservationTable(PreservationTable const &that) = delete; + PreservationTable &operator=(PreservationTable const &that) = delete; + /** * Check whether the hash table may be indexed with the given offset. * From 311a97f33b9710d2bbb3412abd8d9bfdb59569cf Mon Sep 17 00:00:00 2001 From: Josiah VanderZee Date: Sat, 19 Oct 2024 12:09:30 -0500 Subject: [PATCH 3/5] Implement moves for `PreservationTable` --- src/iocore/cache/PreservationTable.cc | 18 ++++++++++++++++++ src/iocore/cache/PreservationTable.h | 3 +++ 2 files changed, 21 insertions(+) diff --git a/src/iocore/cache/PreservationTable.cc b/src/iocore/cache/PreservationTable.cc index 5b3237d767d..7435bacf2a5 100644 --- a/src/iocore/cache/PreservationTable.cc +++ b/src/iocore/cache/PreservationTable.cc @@ -53,6 +53,24 @@ PreservationTable::PreservationTable(int size) : evacuate_size{(size / EVACUATIO memset(static_cast(this->evacuate), 0, evac_len); } +PreservationTable::PreservationTable(PreservationTable &&that) +{ + this->evacuate_size = that.evacuate_size; + this->evacuate = that.evacuate; + that.evacuate_size = 0; + that.evacuate = nullptr; +} + +PreservationTable & +PreservationTable::operator=(PreservationTable &&that) +{ + this->evacuate_size = that.evacuate_size; + this->evacuate = that.evacuate; + that.evacuate_size = 0; + that.evacuate = nullptr; + return *this; +} + EvacuationBlock * PreservationTable::find(Dir const &dir) const { diff --git a/src/iocore/cache/PreservationTable.h b/src/iocore/cache/PreservationTable.h index 701137120b5..6947752820d 100644 --- a/src/iocore/cache/PreservationTable.h +++ b/src/iocore/cache/PreservationTable.h @@ -112,6 +112,9 @@ class PreservationTable PreservationTable(PreservationTable const &that) = delete; PreservationTable &operator=(PreservationTable const &that) = delete; + PreservationTable(PreservationTable &&that); + PreservationTable &operator=(PreservationTable &&that); + /** * Check whether the hash table may be indexed with the given offset. * From 0801427bf9a9217ac96174d4a6265cacc3c7fcbc Mon Sep 17 00:00:00 2001 From: Josiah VanderZee Date: Mon, 21 Oct 2024 12:08:11 -0500 Subject: [PATCH 4/5] Add destructor to `PreservationTable` --- src/iocore/cache/PreservationTable.cc | 6 ++++++ src/iocore/cache/PreservationTable.h | 2 ++ src/iocore/cache/unit_tests/test_Stripe.cc | 3 --- 3 files changed, 8 insertions(+), 3 deletions(-) diff --git a/src/iocore/cache/PreservationTable.cc b/src/iocore/cache/PreservationTable.cc index 7435bacf2a5..572e9af74f9 100644 --- a/src/iocore/cache/PreservationTable.cc +++ b/src/iocore/cache/PreservationTable.cc @@ -53,6 +53,12 @@ PreservationTable::PreservationTable(int size) : evacuate_size{(size / EVACUATIO memset(static_cast(this->evacuate), 0, evac_len); } +PreservationTable::~PreservationTable() +{ + ats_free(this->evacuate); + this->evacuate = nullptr; +} + PreservationTable::PreservationTable(PreservationTable &&that) { this->evacuate_size = that.evacuate_size; diff --git a/src/iocore/cache/PreservationTable.h b/src/iocore/cache/PreservationTable.h index 6947752820d..36e5c42a1f1 100644 --- a/src/iocore/cache/PreservationTable.h +++ b/src/iocore/cache/PreservationTable.h @@ -109,6 +109,8 @@ class PreservationTable */ PreservationTable(int size); + ~PreservationTable(); + PreservationTable(PreservationTable const &that) = delete; PreservationTable &operator=(PreservationTable const &that) = delete; diff --git a/src/iocore/cache/unit_tests/test_Stripe.cc b/src/iocore/cache/unit_tests/test_Stripe.cc index 6c09b1af00c..dd81dcc8bbe 100644 --- a/src/iocore/cache/unit_tests/test_Stripe.cc +++ b/src/iocore/cache/unit_tests/test_Stripe.cc @@ -193,7 +193,6 @@ TEST_CASE("The behavior of StripeSM::add_writer.") } ats_free(stripe.raw_dir); - ats_free(stripe.get_preserved_dirs().evacuate); ats_free(stripe.path); } @@ -305,7 +304,6 @@ TEST_CASE("aggWrite behavior with f.evacuator unset") } ats_free(stripe.raw_dir); - ats_free(stripe.get_preserved_dirs().evacuate); ats_free(stripe.path); } @@ -405,6 +403,5 @@ TEST_CASE("aggWrite behavior with f.evacuator set") delete[] source; ats_free(stripe.raw_dir); - ats_free(stripe.get_preserved_dirs().evacuate); ats_free(stripe.path); } From e504e73e5dd8c1b600cdf802cdce542164c45afb Mon Sep 17 00:00:00 2001 From: Josiah VanderZee Date: Tue, 29 Oct 2024 12:08:48 -0500 Subject: [PATCH 5/5] Add comments explaining weird stuff --- src/iocore/cache/PreservationTable.h | 3 +++ src/iocore/cache/StripeSM.cc | 3 +++ 2 files changed, 6 insertions(+) diff --git a/src/iocore/cache/PreservationTable.h b/src/iocore/cache/PreservationTable.h index 36e5c42a1f1..acf762cce97 100644 --- a/src/iocore/cache/PreservationTable.h +++ b/src/iocore/cache/PreservationTable.h @@ -85,6 +85,9 @@ struct EvacuationBlock { * This class is not safe for concurrent access. It should be protected * by a lock. * + * Destructing this object without first releasing all blocks and calling + * periodic_scan may result in memory leaks. + * * @see Stripe */ class PreservationTable diff --git a/src/iocore/cache/StripeSM.cc b/src/iocore/cache/StripeSM.cc index 237dea354ce..d5899dbe293 100644 --- a/src/iocore/cache/StripeSM.cc +++ b/src/iocore/cache/StripeSM.cc @@ -109,6 +109,9 @@ struct StripeInitInfo { } }; +// This is weird: the len passed to the constructor for _preserved_dirs is +// initialized in the superclasse's constructor. This is safe because the +// superclass should always be initialized first. StripeSM::StripeSM(CacheDisk *disk, off_t blocks, off_t dir_skip) : Continuation(new_ProxyMutex()), Stripe{disk, blocks, dir_skip}, _preserved_dirs{static_cast(len)} {