From: Eduard Bagdasaryan Date: Tue, 8 Aug 2023 01:59:50 +0000 (+0000) Subject: Do not use raw pointers to index CARP CachePeers (#1381) X-Git-Tag: SQUID_7_0_1~379 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=e7959b5;p=thirdparty%2Fsquid.git Do not use raw pointers to index CARP CachePeers (#1381) Simplified and improved code safety by using CbcPointers instead. Also fixed mgr:carp Cache Manager reports to detail relevant cache_peers instead of all cache_peers. When mgr:carp report was added in 2000 commit 8ee9b49, Squid did not index (or even distinguish!) CARP cache_peers, and the reporting loop naturally iterated through all cache_peers. 2002 commit b399543 added identification and indexing of CARP peers but forgot to adjust the reporting loop. Very similar changes will be applied to userhash and sourcehash cache_peers. 2008 commit 63104e2 simply copied problematic CARP code to add userhash and sourcehash cache_peer support. This change adds a few reusable types with those upcoming improvements in mind. --- diff --git a/src/CachePeers.h b/src/CachePeers.h new file mode 100644 index 0000000000..e15d6d0e52 --- /dev/null +++ b/src/CachePeers.h @@ -0,0 +1,26 @@ +/* + * Copyright (C) 1996-2023 The Squid Software Foundation and contributors + * + * Squid software is distributed under GPLv2+ license and includes + * contributions from numerous individuals and organizations. + * Please see the COPYING and CONTRIBUTORS files for details. + */ + +#ifndef SQUID_CACHEPEERS_H +#define SQUID_CACHEPEERS_H + +#include "base/forward.h" +#include "CachePeer.h" +#include "mem/PoolingAllocator.h" + +#include + +/// Weak pointers to zero or more Config.peers. +/// Users must specify the selection algorithm and the order of entries. +using SelectedCachePeers = std::vector< CbcPointer, PoolingAllocator< CbcPointer > >; + +/// Temporary, local storage of raw pointers to zero or more Config.peers. +using RawCachePeers = std::vector >; + +#endif /* SQUID_CACHEPEERS_H */ + diff --git a/src/Makefile.am b/src/Makefile.am index a29a69bf7b..f2eee16555 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -199,6 +199,7 @@ squid_SOURCES = \ CacheManager.h \ CachePeer.cc \ CachePeer.h \ + CachePeers.h \ ClientInfo.h \ ClientRequestContext.h \ CollapsedForwarding.cc \ diff --git a/src/carp.cc b/src/carp.cc index 126e443b09..3313a0b93d 100644 --- a/src/carp.cc +++ b/src/carp.cc @@ -10,6 +10,7 @@ #include "squid.h" #include "CachePeer.h" +#include "CachePeers.h" #include "carp.h" #include "HttpRequest.h" #include "mgr/Registration.h" @@ -22,8 +23,9 @@ #define ROTATE_LEFT(x, n) (((x) << (n)) | ((x) >> (32-(n)))) -static int n_carp_peers = 0; -static CachePeer **carp_peers = nullptr; +/// CARP cache_peers ordered by their CARP weight +static SelectedCachePeers TheCarpPeers; + static OBJH carpCachemgr; static int @@ -44,27 +46,22 @@ void carpInit(void) { int W = 0; - int K; - int k; double P_last, X_last, Xn; - CachePeer *p; - CachePeer **P; char *t; /* Clean up */ - for (k = 0; k < n_carp_peers; ++k) { - cbdataReferenceDone(carp_peers[k]); - } - - safe_free(carp_peers); - n_carp_peers = 0; + TheCarpPeers.clear(); /* initialize cache manager before we have a chance to leave the execution path */ carpRegisterWithCacheManager(); /* find out which peers we have */ - for (p = Config.peers; p; p = p->next) { + RawCachePeers rawCarpPeers; + for (auto p = Config.peers; p; p = p->next) { + if (!cbdataReferenceValid(p)) + continue; + if (!p->options.carp) continue; @@ -73,24 +70,16 @@ carpInit(void) if (p->weight == 0) continue; - ++n_carp_peers; + rawCarpPeers.push_back(p); W += p->weight; } - if (n_carp_peers == 0) + if (rawCarpPeers.empty()) return; - carp_peers = (CachePeer **)xcalloc(n_carp_peers, sizeof(*carp_peers)); - - /* Build a list of the found peers and calculate hashes and load factors */ - for (P = carp_peers, p = Config.peers; p; p = p->next) { - if (!p->options.carp) - continue; - - if (p->weight == 0) - continue; - + /* calculate hashes and load factors */ + for (const auto p: rawCarpPeers) { /* calculate this peers hash */ p->carp.hash = 0; @@ -106,14 +95,10 @@ carpInit(void) if (floor(p->carp.load_factor * 1000.0) == 0.0) p->carp.load_factor = 0.0; - - /* add it to our list of peers */ - *P = cbdataReference(p); - ++P; } /* Sort our list on weight */ - qsort(carp_peers, n_carp_peers, sizeof(*carp_peers), peerSortWeight); + qsort(rawCarpPeers.data(), rawCarpPeers.size(), sizeof(decltype(rawCarpPeers)::value_type), peerSortWeight); /* Calculate the load factor multipliers X_k * @@ -123,7 +108,7 @@ carpInit(void) * X_k = pow (X_k, {1/(K-k+1)}) * simplified to have X_1 part of the loop */ - K = n_carp_peers; + const auto K = rawCarpPeers.size(); P_last = 0.0; /* Empty P_0 */ @@ -131,9 +116,9 @@ carpInit(void) X_last = 0.0; /* Empty X_0, nullifies the first pow statement */ - for (k = 1; k <= K; ++k) { + for (size_t k = 1; k <= K; ++k) { double Kk1 = (double) (K - k + 1); - p = carp_peers[k - 1]; + const auto p = rawCarpPeers[k - 1]; p->carp.load_multiplier = (Kk1 * (p->carp.load_factor - P_last)) / Xn; p->carp.load_multiplier += pow(X_last, Kk1); p->carp.load_multiplier = pow(p->carp.load_multiplier, 1.0 / Kk1); @@ -141,6 +126,8 @@ carpInit(void) X_last = p->carp.load_multiplier; P_last = p->carp.load_factor; } + + TheCarpPeers.assign(rawCarpPeers.begin(), rawCarpPeers.end()); } CachePeer * @@ -149,24 +136,24 @@ carpSelectParent(PeerSelector *ps) assert(ps); HttpRequest *request = ps->request; - int k; CachePeer *p = nullptr; - CachePeer *tp; unsigned int user_hash = 0; unsigned int combined_hash; double score; double high_score = 0; - if (n_carp_peers == 0) + if (TheCarpPeers.empty()) return nullptr; /* calculate hash key */ debugs(39, 2, "carpSelectParent: Calculating hash for " << request->effectiveRequestUri()); /* select CachePeer */ - for (k = 0; k < n_carp_peers; ++k) { + for (const auto &tp: TheCarpPeers) { + if (!tp) + continue; // peer gone + SBuf key; - tp = carp_peers[k]; if (tp->options.carp_key.set) { // this code follows URI syntax pattern. // corner cases should use the full effective request URI @@ -208,8 +195,8 @@ carpSelectParent(PeerSelector *ps) debugs(39, 3, *tp << " key=" << key << " combined_hash=" << combined_hash << " score=" << std::setprecision(0) << score); - if ((score > high_score) && peerHTTPOkay(tp, ps)) { - p = tp; + if ((score > high_score) && peerHTTPOkay(tp.get(), ps)) { + p = tp.get(); high_score = score; } } @@ -223,7 +210,6 @@ carpSelectParent(PeerSelector *ps) static void carpCachemgr(StoreEntry * sentry) { - CachePeer *p; int sumfetches = 0; storeAppendPrintf(sentry, "%24s %10s %10s %10s %10s\n", "Hostname", @@ -232,10 +218,15 @@ carpCachemgr(StoreEntry * sentry) "Factor", "Actual"); - for (p = Config.peers; p; p = p->next) + for (const auto &p: TheCarpPeers) { + if (!p) + continue; sumfetches += p->stats.fetches; + } - for (p = Config.peers; p; p = p->next) { + for (const auto &p: TheCarpPeers) { + if (!p) + continue; storeAppendPrintf(sentry, "%24s %10x %10f %10f %10f\n", p->name, p->carp.hash, p->carp.load_multiplier,