From: Eduard Bagdasaryan Date: Sat, 23 Sep 2023 03:57:01 +0000 (+0000) Subject: Do not use raw pointers to index sourcehash CachePeers (#1474) X-Git-Tag: SQUID_7_0_1~349 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=2d5ac435c3bc120b6ddb3458d6360139b09eb891;p=thirdparty%2Fsquid.git Do not use raw pointers to index sourcehash CachePeers (#1474) Simplified and improved code safety by using CbcPointers for sourcehash cache_peers, as we have done for CARP peers in recent commit e7959b5. Also fixed mgr:sourcehash Cache Manager reports to detail relevant cache_peers instead of all cache_peers. This problem existed since inception (2008 commit f7e1d9c) as detailed in recent commit e7959b5. Also applied the new "no new globals" policy to CARP peering code, to keep improved CARP and sourcehash peering code in sync. --- diff --git a/src/carp.cc b/src/carp.cc index d8f1738945..7327f20dc4 100644 --- a/src/carp.cc +++ b/src/carp.cc @@ -24,7 +24,12 @@ #define ROTATE_LEFT(x, n) (((x) << (n)) | ((x) >> (32-(n)))) /// CARP cache_peers ordered by their CARP weight -static SelectedCachePeers TheCarpPeers; +static auto & +CarpPeers() +{ + static const auto carpPeers = new SelectedCachePeers(); + return *carpPeers; +} static OBJH carpCachemgr; @@ -50,7 +55,7 @@ carpInit(void) char *t; /* Clean up */ - TheCarpPeers.clear(); + CarpPeers().clear(); /* initialize cache manager before we have a chance to leave the execution path */ carpRegisterWithCacheManager(); @@ -126,7 +131,7 @@ carpInit(void) P_last = p->carp.load_factor; } - TheCarpPeers.assign(rawCarpPeers.begin(), rawCarpPeers.end()); + CarpPeers().assign(rawCarpPeers.begin(), rawCarpPeers.end()); } CachePeer * @@ -141,14 +146,14 @@ carpSelectParent(PeerSelector *ps) double score; double high_score = 0; - if (TheCarpPeers.empty()) + if (CarpPeers().empty()) return nullptr; /* calculate hash key */ debugs(39, 2, "carpSelectParent: Calculating hash for " << request->effectiveRequestUri()); /* select CachePeer */ - for (const auto &tp: TheCarpPeers) { + for (const auto &tp: CarpPeers()) { if (!tp) continue; // peer gone @@ -217,13 +222,13 @@ carpCachemgr(StoreEntry * sentry) "Factor", "Actual"); - for (const auto &p: TheCarpPeers) { + for (const auto &p: CarpPeers()) { if (!p) continue; sumfetches += p->stats.fetches; } - for (const auto &p: TheCarpPeers) { + for (const auto &p: CarpPeers()) { if (!p) continue; storeAppendPrintf(sentry, "%24s %10x %10f %10f %10f\n", diff --git a/src/peer_sourcehash.cc b/src/peer_sourcehash.cc index 2cd54c02f4..a47cf06caf 100644 --- a/src/peer_sourcehash.cc +++ b/src/peer_sourcehash.cc @@ -23,8 +23,14 @@ #define ROTATE_LEFT(x, n) (((x) << (n)) | ((x) >> (32-(n)))) -static int n_sourcehash_peers = 0; -static CachePeer **sourcehash_peers = nullptr; +/// sourcehash peers ordered by their sourcehash weight +static auto & +SourceHashPeers() +{ + static const auto hashPeers = new SelectedCachePeers(); + return *hashPeers; +} + static OBJH peerSourceHashCachemgr; static void peerSourceHashRegisterWithCacheManager(void); @@ -40,21 +46,17 @@ void peerSourceHashInit(void) { int W = 0; - int K; - int k; double P_last, X_last, Xn; char *t; /* Clean up */ - for (k = 0; k < n_sourcehash_peers; ++k) { - cbdataReferenceDone(sourcehash_peers[k]); - } - - safe_free(sourcehash_peers); - n_sourcehash_peers = 0; + SourceHashPeers().clear(); /* find out which peers we have */ + RawCachePeers rawSourceHashPeers; for (const auto &p: CurrentCachePeers()) { + const auto peer = p.get(); + if (!p->options.sourcehash) continue; @@ -63,29 +65,18 @@ peerSourceHashInit(void) if (p->weight == 0) continue; - ++n_sourcehash_peers; + rawSourceHashPeers.push_back(peer); W += p->weight; } peerSourceHashRegisterWithCacheManager(); - if (n_sourcehash_peers == 0) + if (rawSourceHashPeers.empty()) return; - sourcehash_peers = (CachePeer **)xcalloc(n_sourcehash_peers, sizeof(*sourcehash_peers)); - - auto P = sourcehash_peers; - /* Build a list of the found peers and calculate hashes and load factors */ - for (const auto &peer: CurrentCachePeers()) { - const auto p = peer.get(); - - if (!p->options.sourcehash) - continue; - - if (p->weight == 0) - continue; - + /* calculate hashes and load factors */ + for (const auto &p: rawSourceHashPeers) { /* calculate this peers hash */ p->sourcehash.hash = 0; @@ -101,13 +92,10 @@ peerSourceHashInit(void) if (floor(p->sourcehash.load_factor * 1000.0) == 0.0) p->sourcehash.load_factor = 0.0; - - /* add it to our list of peers */ - *P++ = cbdataReference(p); } /* Sort our list on weight */ - qsort(sourcehash_peers, n_sourcehash_peers, sizeof(*sourcehash_peers), peerSortWeight); + qsort(rawSourceHashPeers.data(), rawSourceHashPeers.size(), sizeof(decltype(rawSourceHashPeers)::value_type), peerSortWeight); /* Calculate the load factor multipliers X_k * @@ -117,7 +105,7 @@ peerSourceHashInit(void) * X_k = pow (X_k, {1/(K-k+1)}) * simplified to have X_1 part of the loop */ - K = n_sourcehash_peers; + const auto K = rawSourceHashPeers.size(); P_last = 0.0; /* Empty P_0 */ @@ -125,9 +113,9 @@ peerSourceHashInit(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); - const auto p = sourcehash_peers[k - 1]; + const auto p = rawSourceHashPeers[k - 1]; p->sourcehash.load_multiplier = (Kk1 * (p->sourcehash.load_factor - P_last)) / Xn; p->sourcehash.load_multiplier += pow(X_last, Kk1); p->sourcehash.load_multiplier = pow(p->sourcehash.load_multiplier, 1.0 / Kk1); @@ -135,6 +123,8 @@ peerSourceHashInit(void) X_last = p->sourcehash.load_multiplier; P_last = p->sourcehash.load_factor; } + + SourceHashPeers().assign(rawSourceHashPeers.begin(), rawSourceHashPeers.end()); } static void @@ -147,10 +137,8 @@ peerSourceHashRegisterWithCacheManager(void) CachePeer * peerSourceHashSelectParent(PeerSelector *ps) { - int k; const char *c; CachePeer *p = nullptr; - CachePeer *tp; unsigned int user_hash = 0; unsigned int combined_hash; double score; @@ -158,7 +146,7 @@ peerSourceHashSelectParent(PeerSelector *ps) const char *key = nullptr; char ntoabuf[MAX_IPSTRLEN]; - if (n_sourcehash_peers == 0) + if (SourceHashPeers().empty()) return nullptr; assert(ps); @@ -173,8 +161,10 @@ peerSourceHashSelectParent(PeerSelector *ps) user_hash += ROTATE_LEFT(user_hash, 19) + *c; /* select CachePeer */ - for (k = 0; k < n_sourcehash_peers; ++k) { - tp = sourcehash_peers[k]; + for (const auto &tp: SourceHashPeers()) { + if (!tp) + continue; // peer gone + combined_hash = (user_hash ^ tp->sourcehash.hash); combined_hash += combined_hash * 0x62531965; combined_hash = ROTATE_LEFT(combined_hash, 21); @@ -182,8 +172,8 @@ peerSourceHashSelectParent(PeerSelector *ps) debugs(39, 3, *tp << " 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; } } @@ -205,10 +195,15 @@ peerSourceHashCachemgr(StoreEntry * sentry) "Factor", "Actual"); - for (const auto &p: CurrentCachePeers()) + for (const auto &p: SourceHashPeers()) { + if (!p) + continue; sumfetches += p->stats.fetches; + } - for (const auto &p: CurrentCachePeers()) { + for (const auto &p: SourceHashPeers()) { + if (!p) + continue; storeAppendPrintf(sentry, "%24s %10x %10f %10f %10f\n", p->name, p->sourcehash.hash, p->sourcehash.load_multiplier,