From 2658c514e3b94fe7996521f1d8a8ff25e10c5c9e Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Fri, 6 Dec 2024 17:44:27 -0500 Subject: [PATCH 1/9] Reduce scope of modified loop indexes; upgraded Disks methods This reduction is not just a cosmetic improvement. It increases the chance of discovering stale/mismatching printf() formats and similar problems when reviewing the diff of changes (because it places the modified line much closer to actual index use cases inside the loop), improving our overall confidence in these changes. Also upgraded Disks::store() and Disks::Dir() argument to size_t because these methods use SwapDirByIndex() that was upgraded in an earlier branch commit. I checked that the removed variants are no longer used before removing those variants (by making Squid with those methods explicitly deleted). --- src/DiskIO/DiskThreads/aiops.cc | 6 ++---- src/fs/ufs/UFSSwapDir.cc | 3 +-- src/store/Disks.cc | 27 +++++++++++---------------- src/store/Disks.h | 4 ++-- src/tests/testStoreController.cc | 2 +- src/tests/testStoreHashIndex.cc | 2 +- 6 files changed, 18 insertions(+), 26 deletions(-) diff --git a/src/DiskIO/DiskThreads/aiops.cc b/src/DiskIO/DiskThreads/aiops.cc index 40058f81236..8eab4af24a6 100644 --- a/src/DiskIO/DiskThreads/aiops.cc +++ b/src/DiskIO/DiskThreads/aiops.cc @@ -215,7 +215,6 @@ squidaio_xstrfree(char *str) void squidaio_init(void) { - size_t i; squidaio_thread_t *threadp; if (squidaio_initialised) @@ -294,7 +293,7 @@ squidaio_init(void) assert(NUMTHREADS != 0); - for (i = 0; i < NUMTHREADS; ++i) { + for (size_t i = 0; i < NUMTHREADS; ++i) { threadp = (squidaio_thread_t *)squidaio_thread_pool->alloc(); threadp->status = _THREAD_STARTING; threadp->current_req = nullptr; @@ -999,7 +998,6 @@ void squidaio_stats(StoreEntry * sentry) { squidaio_thread_t *threadp; - size_t i; if (!squidaio_initialised) return; @@ -1010,7 +1008,7 @@ squidaio_stats(StoreEntry * sentry) threadp = threads; - for (i = 0; i < NUMTHREADS; ++i) { + for (size_t i = 0; i < NUMTHREADS; ++i) { storeAppendPrintf(sentry, "%zu\t0x%lx\t%ld\n", i + 1, (unsigned long)threadp->thread, threadp->requests); threadp = threadp->next; } diff --git a/src/fs/ufs/UFSSwapDir.cc b/src/fs/ufs/UFSSwapDir.cc index d651c47d1e5..26e99cc11f9 100644 --- a/src/fs/ufs/UFSSwapDir.cc +++ b/src/fs/ufs/UFSSwapDir.cc @@ -1039,7 +1039,6 @@ int Fs::Ufs::UFSSwapDir::HandleCleanEvent() { static int swap_index = 0; - size_t i; int j = 0; size_t n = 0; @@ -1054,7 +1053,7 @@ Fs::Ufs::UFSSwapDir::HandleCleanEvent() */ UFSDirToGlobalDirMapping = (int *)xcalloc(NumberOfUFSDirs, sizeof(*UFSDirToGlobalDirMapping)); - for (i = 0, n = 0; i < Config.cacheSwap.n_configured; ++i) { + for (size_t i = 0; i < Config.cacheSwap.n_configured; ++i) { /* This is bogus, the controller should just clean each instance once */ sd = dynamic_cast (INDEXSD(i)); diff --git a/src/store/Disks.cc b/src/store/Disks.cc index 7097bddeeec..35d5cf8601d 100644 --- a/src/store/Disks.cc +++ b/src/store/Disks.cc @@ -118,11 +118,10 @@ storeDirSelectSwapDirLeastLoad(const StoreEntry * e) int least_load = INT_MAX; int load; SwapDir *selectedDir = nullptr; - size_t i; const int64_t objsize = objectSizeForDirSelection(*e); - for (i = 0; i < Config.cacheSwap.n_configured; ++i) { + for (size_t i = 0; i < Config.cacheSwap.n_configured; ++i) { auto &sd = SwapDirByIndex(i); sd.flags.selected = false; @@ -173,13 +172,13 @@ Store::Disks::Disks(): } SwapDir * -Store::Disks::store(int const x) const +Store::Disks::store(const size_t x) const { return &SwapDirByIndex(x); } SwapDir & -Store::Disks::Dir(const int i) +Store::Disks::Dir(const size_t i) { return SwapDirByIndex(i); } @@ -232,11 +231,11 @@ Store::Disks::create() StoreEntry * Store::Disks::get(const cache_key *key) { - if (const size_t cacheDirs = Config.cacheSwap.n_configured) { + if (const auto cacheDirs = Config.cacheSwap.n_configured) { // ask each cache_dir until the entry is found; use static starting // point to avoid asking the same subset of disks more often // TODO: coordinate with put() to be able to guess the right disk often - static int idx = 0; + static size_t idx = 0; for (size_t n = 0; n < cacheDirs; ++n) { idx = (idx + 1) % cacheDirs; auto &sd = Dir(idx); @@ -531,11 +530,9 @@ Store::Disks::getStats(StoreInfoStats &stats) const void Store::Disks::stat(StoreEntry & output) const { - size_t i; - /* Now go through each store, calling its stat routine */ - for (i = 0; i < Config.cacheSwap.n_configured; ++i) { + for (size_t i = 0; i < Config.cacheSwap.n_configured; ++i) { storeAppendPrintf(&output, "\n"); store(i)->stat(output); } @@ -563,10 +560,9 @@ Store::Disks::updateHeaders(StoreEntry *e) void Store::Disks::maintain() { - size_t i; /* walk each fs */ - for (i = 0; i < Config.cacheSwap.n_configured; ++i) { + for (size_t i = 0; i < Config.cacheSwap.n_configured; ++i) { /* XXX FixMe: This should be done "in parallel" on the different * cache_dirs, not one at a time. */ @@ -618,7 +614,7 @@ Store::Disks::anchorToCache(StoreEntry &entry) // ask each cache_dir until the entry is found; use static starting // point to avoid asking the same subset of disks more often // TODO: coordinate with put() to be able to guess the right disk often - static int idx = 0; + static size_t idx = 0; for (size_t n = 0; n < cacheDirs; ++n) { idx = (idx + 1) % cacheDirs; SwapDir &sd = Dir(idx); @@ -703,7 +699,6 @@ storeDirWriteCleanLogs(int reopen) struct timeval start; double dt; - size_t dirn; int notdone = 1; // Check for store_dirs_rebuilding because fatal() often calls us in early @@ -719,7 +714,7 @@ storeDirWriteCleanLogs(int reopen) getCurrentTime(); start = current_time; - for (dirn = 0; dirn < Config.cacheSwap.n_configured; ++dirn) { + for (size_t dirn = 0; dirn < Config.cacheSwap.n_configured; ++dirn) { auto &sd = SwapDirByIndex(dirn); if (sd.writeCleanStart() < 0) { @@ -736,7 +731,7 @@ storeDirWriteCleanLogs(int reopen) while (notdone) { notdone = 0; - for (dirn = 0; dirn < Config.cacheSwap.n_configured; ++dirn) { + for (size_t dirn = 0; dirn < Config.cacheSwap.n_configured; ++dirn) { auto &sd = SwapDirByIndex(dirn); if (!sd.cleanLog) @@ -763,7 +758,7 @@ storeDirWriteCleanLogs(int reopen) } /* Flush */ - for (dirn = 0; dirn < Config.cacheSwap.n_configured; ++dirn) + for (size_t dirn = 0; dirn < Config.cacheSwap.n_configured; ++dirn) SwapDirByIndex(dirn).writeCleanDone(); if (reopen) diff --git a/src/store/Disks.h b/src/store/Disks.h index 9e9849d675c..d6f2efdaeba 100644 --- a/src/store/Disks.h +++ b/src/store/Disks.h @@ -60,8 +60,8 @@ class Disks: public Controlled private: /* migration logic */ - SwapDir *store(int const x) const; - static SwapDir &Dir(int const idx); + SwapDir *store(size_t index) const; + static SwapDir &Dir(size_t index); int64_t largestMinimumObjectSize; ///< maximum of all Disk::minObjectSize()s int64_t largestMaximumObjectSize; ///< maximum of all Disk::maxObjectSize()s diff --git a/src/tests/testStoreController.cc b/src/tests/testStoreController.cc index 86500e154f2..1b403f40217 100644 --- a/src/tests/testStoreController.cc +++ b/src/tests/testStoreController.cc @@ -111,7 +111,7 @@ addedEntry(Store::Disk *aStore, e->swap_filen = 0; /* garh - lower level*/ e->swap_dirn = -1; - for (size_t i=0; i < Config.cacheSwap.n_configured; ++i) { + for (size_t i = 0; i < Config.cacheSwap.n_configured; ++i) { if (INDEXSD(i) == aStore) e->swap_dirn = i; } diff --git a/src/tests/testStoreHashIndex.cc b/src/tests/testStoreHashIndex.cc index edb8d469a58..f429759b2c1 100644 --- a/src/tests/testStoreHashIndex.cc +++ b/src/tests/testStoreHashIndex.cc @@ -89,7 +89,7 @@ addedEntry(Store::Disk *aStore, e->swap_filen = 0; /* garh - lower level*/ e->swap_dirn = -1; - for (size_t i=0; i < Config.cacheSwap.n_configured; ++i) { + for (size_t i = 0; i < Config.cacheSwap.n_configured; ++i) { if (INDEXSD(i) == aStore) e->swap_dirn = i; } From 06d9d9507c5771f7a0100b693246c78d86d19070 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Fri, 6 Dec 2024 20:57:58 -0500 Subject: [PATCH 2/9] fixup: upgrade dirn as well It is derived from upgraded firstCandidate. It is used as an argument for upgraded SwapDirByIndex(). --- src/store/Disks.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/store/Disks.cc b/src/store/Disks.cc index 35d5cf8601d..47ecbc2ef29 100644 --- a/src/store/Disks.cc +++ b/src/store/Disks.cc @@ -80,7 +80,7 @@ storeDirSelectSwapDirRoundRobin(const StoreEntry * e) firstCandidate = 0; for (size_t i = 0; i < Config.cacheSwap.n_configured; ++i) { - const int dirn = (firstCandidate + i) % Config.cacheSwap.n_configured; + const auto dirn = (firstCandidate + i) % Config.cacheSwap.n_configured; auto &dir = SwapDirByIndex(dirn); int load = 0; From d6ce1393ac26d36c49c113ca7ba65725a366722d Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Fri, 6 Dec 2024 21:04:51 -0500 Subject: [PATCH 3/9] Restored pconn.* changes They were necessary (LTO builds fails without them AFAICT), and they themselves were not wrong (just insufficient). --- src/pconn.cc | 6 +++--- src/pconn.h | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/pconn.cc b/src/pconn.cc index 45adde1a3d7..9750e0d0b81 100644 --- a/src/pconn.cc +++ b/src/pconn.cc @@ -89,9 +89,9 @@ IdleConnList::findIndexOf(const Comm::ConnectionPointer &conn) const * \retval false The index is not an in-use entry. */ bool -IdleConnList::removeAt(int index) +IdleConnList::removeAt(size_t index) { - if (index < 0 || index >= size_) + if (index >= size_) return false; // shuffle the remaining entries to fill the new gap. @@ -174,7 +174,7 @@ IdleConnList::push(const Comm::ConnectionPointer &conn) capacity_ <<= 1; const Comm::ConnectionPointer *oldList = theList_; theList_ = new Comm::ConnectionPointer[capacity_]; - for (int index = 0; index < size_; ++index) + for (size_t index = 0; index < size_; ++index) theList_[index] = oldList[index]; delete[] oldList; diff --git a/src/pconn.h b/src/pconn.h index cd206b12d48..b72139ce23a 100644 --- a/src/pconn.h +++ b/src/pconn.h @@ -66,7 +66,7 @@ class IdleConnList: public hash_link, private IndependentRunner void endingShutdown() override; private: bool isAvailable(int i) const; - bool removeAt(int index); + bool removeAt(size_t index); int findIndexOf(const Comm::ConnectionPointer &conn) const; void findAndClose(const Comm::ConnectionPointer &conn); static IOCB Read; @@ -81,9 +81,9 @@ class IdleConnList: public hash_link, private IndependentRunner Comm::ConnectionPointer *theList_; /// Number of entries theList can currently hold without re-allocating (capacity). - int capacity_; + size_t capacity_; ///< Number of in-use entries in theList - int size_; + size_t size_; /** The pool containing this sub-list. * The parent performs all stats accounting, and From e4573ac36173645c78587af27c79abf256fc6c12 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Fri, 6 Dec 2024 21:13:07 -0500 Subject: [PATCH 4/9] fixup: Avoid subtracting from zero IdleConnList::size_ --- src/pconn.cc | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/pconn.cc b/src/pconn.cc index 9750e0d0b81..83f145cf606 100644 --- a/src/pconn.cc +++ b/src/pconn.cc @@ -73,7 +73,8 @@ IdleConnList::~IdleConnList() int IdleConnList::findIndexOf(const Comm::ConnectionPointer &conn) const { - for (int index = size_ - 1; index >= 0; --index) { + for (auto right = size_; right > 0; --right) { + const auto index = right - 1; if (conn->fd == theList_[index]->fd) { debugs(48, 3, "found " << conn << " at index " << index); return index; @@ -93,6 +94,7 @@ IdleConnList::removeAt(size_t index) { if (index >= size_) return false; + assert(size_ > 0); // shuffle the remaining entries to fill the new gap. for (; index < size_ - 1; ++index) @@ -214,7 +216,8 @@ IdleConnList::isAvailable(int i) const Comm::ConnectionPointer IdleConnList::pop() { - for (int i=size_-1; i>=0; --i) { + for (auto right = size_; right > 0; --right) { + const auto i = right - 1; if (!isAvailable(i)) continue; @@ -252,7 +255,8 @@ IdleConnList::findUseable(const Comm::ConnectionPointer &aKey) const bool keyCheckAddr = !aKey->local.isAnyAddr(); const bool keyCheckPort = aKey->local.port() > 0; - for (int i=size_-1; i>=0; --i) { + for (auto right = size_; right > 0; --right) { + const auto i = right - 1; if (!isAvailable(i)) continue; From a3cb66830910b2d485885472ad0162fa4553ffea Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Fri, 6 Dec 2024 21:18:45 -0500 Subject: [PATCH 5/9] fixup: Do not cast size_t to size_t --- src/pconn.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/pconn.cc b/src/pconn.cc index 83f145cf606..fd89bb89ce1 100644 --- a/src/pconn.cc +++ b/src/pconn.cc @@ -119,7 +119,7 @@ IdleConnList::closeN(size_t n) if (n < 1) { debugs(48, 2, "Nothing to do."); return; - } else if (n >= (size_t)size_) { + } else if (n >= size_) { debugs(48, 2, "Closing all entries."); while (size_ > 0) { const Comm::ConnectionPointer conn = theList_[--size_]; @@ -143,11 +143,11 @@ IdleConnList::closeN(size_t n) parent_->noteConnectionRemoved(); } // shuffle the list N down. - for (index = 0; index < (size_t)size_ - n; ++index) { + for (index = 0; index < size_ - n; ++index) { theList_[index] = theList_[index + n]; } // ensure the last N entries are unset - while (index < ((size_t)size_)) { + while (index < size_) { theList_[index] = nullptr; ++index; } From bffc9feb35ba0833f2f098b44825d2a011b9b9d2 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Fri, 6 Dec 2024 21:41:39 -0500 Subject: [PATCH 6/9] fixup: const-correctness and out-of-scope TODO --- src/fs/ufs/UFSSwapDir.cc | 2 +- src/pconn.cc | 2 +- src/pconn.h | 2 ++ src/store_rebuild.cc | 2 +- 4 files changed, 5 insertions(+), 3 deletions(-) diff --git a/src/fs/ufs/UFSSwapDir.cc b/src/fs/ufs/UFSSwapDir.cc index 26e99cc11f9..2d4c7a50e29 100644 --- a/src/fs/ufs/UFSSwapDir.cc +++ b/src/fs/ufs/UFSSwapDir.cc @@ -1111,7 +1111,7 @@ Fs::Ufs::UFSSwapDir::IsUFSDir(SwapDir * sd) * if not UFSSwapDir return 0; */ bool -Fs::Ufs::UFSSwapDir::FilenoBelongsHere(int fn, size_t F0, int F1, int F2) +Fs::Ufs::UFSSwapDir::FilenoBelongsHere(const int fn, const size_t F0, const int F1, const int F2) { int D1, D2; int L1, L2; diff --git a/src/pconn.cc b/src/pconn.cc index fd89bb89ce1..db943ccec34 100644 --- a/src/pconn.cc +++ b/src/pconn.cc @@ -114,7 +114,7 @@ IdleConnList::removeAt(size_t index) // almost a duplicate of removeFD. But drops multiple entries. void -IdleConnList::closeN(size_t n) +IdleConnList::closeN(const size_t n) { if (n < 1) { debugs(48, 2, "Nothing to do."); diff --git a/src/pconn.h b/src/pconn.h index b72139ce23a..4cb9fb22b29 100644 --- a/src/pconn.h +++ b/src/pconn.h @@ -59,7 +59,9 @@ class IdleConnList: public hash_link, private IndependentRunner void clearHandlers(const Comm::ConnectionPointer &conn); + // TODO: Upgrade to return size_t int count() const { return size_; } + void closeN(size_t count); // IndependentRunner API diff --git a/src/store_rebuild.cc b/src/store_rebuild.cc index 2863afca798..f90c81f949f 100644 --- a/src/store_rebuild.cc +++ b/src/store_rebuild.cc @@ -211,7 +211,7 @@ storeRebuildStart(void) * progress. */ void -storeRebuildProgress(size_t sd_index, int total, int sofar) +storeRebuildProgress(size_t sd_index, const int total, const int sofar) { static time_t last_report = 0; // TODO: Switch to int64_t and fix handling of unknown totals. From 98784813efe04e136eb9345c1cf42faf3632fd61 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Fri, 6 Dec 2024 21:50:35 -0500 Subject: [PATCH 7/9] Do not upgrade storeRebuildProgress index parameter ... because we are not upgrading its source -- Store::Disk::index (yet). This change also fixes "make check". --- src/store_rebuild.cc | 13 +++++++++---- src/store_rebuild.h | 2 +- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/src/store_rebuild.cc b/src/store_rebuild.cc index f90c81f949f..981e86bc64c 100644 --- a/src/store_rebuild.cc +++ b/src/store_rebuild.cc @@ -211,13 +211,18 @@ storeRebuildStart(void) * progress. */ void -storeRebuildProgress(size_t sd_index, const int total, const int sofar) +storeRebuildProgress(const int sd_index_raw, const int total, const int sofar) { static time_t last_report = 0; // TODO: Switch to int64_t and fix handling of unknown totals. double n = 0.0; double d = 0.0; + if (sd_index_raw < 0) + return; + + // TODO: Upgrade Disk::index and sd_index_raw to size_t, removing this cast. + const auto sd_index = static_cast(sd_index_raw); if (sd_index >= Config.cacheSwap.n_configured) return; @@ -231,9 +236,9 @@ storeRebuildProgress(size_t sd_index, const int total, const int sofar) if (squid_curtime - last_report < 15) return; - for (sd_index = 0; sd_index < Config.cacheSwap.n_configured; ++sd_index) { - n += (double) RebuildProgress[sd_index].scanned; - d += (double) RebuildProgress[sd_index].total; + for (size_t diskIndex = 0; diskIndex < Config.cacheSwap.n_configured; ++diskIndex) { + n += (double) RebuildProgress[diskIndex].scanned; + d += (double) RebuildProgress[diskIndex].total; } debugs(20, Important(57), "Indexing cache entries: " << Progress(n, d)); diff --git a/src/store_rebuild.h b/src/store_rebuild.h index fba039f45a6..8d9e647ba86 100644 --- a/src/store_rebuild.h +++ b/src/store_rebuild.h @@ -65,7 +65,7 @@ operator <<(std::ostream &os, const Progress &p) void storeRebuildStart(void); void storeRebuildComplete(StoreRebuildData *); -void storeRebuildProgress(size_t sd_index, int total, int sofar); +void storeRebuildProgress(int sd_index, int total, int sofar); /// loads entry from disk; fills supplied memory buffer on success bool storeRebuildLoadEntry(int fd, int diskIndex, MemBuf &buf, StoreRebuildData &counts); From b21738ecd4b024cfedffb87f7144a768a510cc05 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Fri, 6 Dec 2024 21:58:36 -0500 Subject: [PATCH 8/9] Do not upgrade UFSSwapDir::FilenoBelongsHere F0 parameter ... because we are not upgrading its sources -- Store::Disk::index and UFSDirToGlobalDirMapping[i] (yet). --- src/fs/ufs/UFSSwapDir.cc | 2 +- src/fs/ufs/UFSSwapDir.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/fs/ufs/UFSSwapDir.cc b/src/fs/ufs/UFSSwapDir.cc index 2d4c7a50e29..a349eff5d9d 100644 --- a/src/fs/ufs/UFSSwapDir.cc +++ b/src/fs/ufs/UFSSwapDir.cc @@ -1111,7 +1111,7 @@ Fs::Ufs::UFSSwapDir::IsUFSDir(SwapDir * sd) * if not UFSSwapDir return 0; */ bool -Fs::Ufs::UFSSwapDir::FilenoBelongsHere(const int fn, const size_t F0, const int F1, const int F2) +Fs::Ufs::UFSSwapDir::FilenoBelongsHere(int fn, int F0, int F1, int F2) { int D1, D2; int L1, L2; diff --git a/src/fs/ufs/UFSSwapDir.h b/src/fs/ufs/UFSSwapDir.h index ddce5a760e6..3df03f47e77 100644 --- a/src/fs/ufs/UFSSwapDir.h +++ b/src/fs/ufs/UFSSwapDir.h @@ -38,7 +38,7 @@ class UFSSwapDir : public SwapDir * \param level1dir level-1 dir in the cachedir * \param level2dir level-2 dir */ - static bool FilenoBelongsHere(int fn, size_t cachedir, int level1dir, int level2dir); + static bool FilenoBelongsHere(int fn, int cachedir, int level1dir, int level2dir); UFSSwapDir(char const *aType, const char *aModuleType); ~UFSSwapDir() override; From 042cd22b6de2e14ba41af6610951c2fc0c178229 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Fri, 6 Dec 2024 22:32:24 -0500 Subject: [PATCH 9/9] fixup: Fix build broken by my previous branch commit --- src/fs/ufs/UFSSwapDir.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/fs/ufs/UFSSwapDir.cc b/src/fs/ufs/UFSSwapDir.cc index a349eff5d9d..f005d445e12 100644 --- a/src/fs/ufs/UFSSwapDir.cc +++ b/src/fs/ufs/UFSSwapDir.cc @@ -1116,7 +1116,8 @@ Fs::Ufs::UFSSwapDir::FilenoBelongsHere(int fn, int F0, int F1, int F2) int D1, D2; int L1, L2; int filn = fn; - assert(F0 < Config.cacheSwap.n_configured); + assert(F0 >= 0); + assert(static_cast(F0) < Config.cacheSwap.n_configured); assert (UFSSwapDir::IsUFSDir (dynamic_cast(INDEXSD(F0)))); UFSSwapDir *sd = dynamic_cast(INDEXSD(F0));