From: Otto Moerbeek Date: Mon, 31 Jan 2022 10:40:16 +0000 (+0100) Subject: Move fail_t maps to global shared instead of thread local X-Git-Tag: auth-4.7.0-alpha1~23^2~4 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=7f176bc6590af0754b7618798c7390996c3745f0;p=thirdparty%2Fpdns.git Move fail_t maps to global shared instead of thread local These maps are only used or modified when we're going out on the net, so the performance impact of the locking should be relatively low, while other threads could benefit greatly from information learned by other threads. Also, pruning of these data structure is cheap, so holding the lock while pruning should be a short period of time. --- diff --git a/pdns/rec_channel_rec.cc b/pdns/rec_channel_rec.cc index 0cac23188a..f7c3f4dfbd 100644 --- a/pdns/rec_channel_rec.cc +++ b/pdns/rec_channel_rec.cc @@ -386,7 +386,7 @@ static uint64_t* pleaseDumpNonResolvingNS(int fd) } // Generic dump to file command -static RecursorControlChannel::Answer doDumpToFile(int s, uint64_t* (*function)(int s), const string& name) +static RecursorControlChannel::Answer doDumpToFile(int s, uint64_t* (*function)(int s), const string& name, bool threads = true) { auto fdw = getfd(s); @@ -396,8 +396,14 @@ static RecursorControlChannel::Answer doDumpToFile(int s, uint64_t* (*function)( uint64_t total = 0; try { - int fd = fdw; - total = broadcastAccFunction([function, fd] { return function(fd); }); + if (threads) { + int fd = fdw; + total = broadcastAccFunction([function, fd] { return function(fd); }); + } else { + auto ret = function(fdw); + total = *ret; + delete ret; + } } catch (std::exception& e) { return {1, name + ": error dumping data: " + string(e.what()) + "\n"}; @@ -1987,7 +1993,7 @@ RecursorControlChannel::Answer RecursorControlParser::getAnswer(int s, const str return doDumpToFile(s, pleaseDumpNSSpeeds, cmd); } if (cmd == "dump-failedservers") { - return doDumpToFile(s, pleaseDumpFailedServers, cmd); + return doDumpToFile(s, pleaseDumpFailedServers, cmd, false); } if (cmd == "dump-rpz") { return doDumpRPZ(s, begin, end); @@ -1996,7 +2002,7 @@ RecursorControlChannel::Answer RecursorControlParser::getAnswer(int s, const str return doDumpToFile(s, pleaseDumpThrottleMap, cmd); } if (cmd == "dump-non-resolving") { - return doDumpToFile(s, pleaseDumpNonResolvingNS, cmd); + return doDumpToFile(s, pleaseDumpNonResolvingNS, cmd, false); } if (cmd == "wipe-cache" || cmd == "flushname") { return {0, doWipeCache(begin, end, 0xffff)}; diff --git a/pdns/syncres.cc b/pdns/syncres.cc index 4daa7440bd..c36f653463 100644 --- a/pdns/syncres.cc +++ b/pdns/syncres.cc @@ -48,6 +48,8 @@ EDNSSubnetOpts SyncRes::s_ecsScopeZero; string SyncRes::s_serverID; SyncRes::LogMode SyncRes::s_lm; const std::unordered_set SyncRes::s_redirectionQTypes = {QType::CNAME, QType::DNAME}; +LockGuarded> SyncRes::s_fails; +LockGuarded> SyncRes::s_nonresolving; unsigned int SyncRes::s_maxnegttl; unsigned int SyncRes::s_maxbogusttl; @@ -535,7 +537,7 @@ uint64_t SyncRes::doDumpFailedServers(int fd) fprintf(fp.get(), "; remote IP\tcount\ttimestamp\n"); uint64_t count=0; - for(const auto& i : t_sstorage.fails.getMap()) + for(const auto& i : s_fails.lock()->getMap()) { count++; char tmp[26]; @@ -561,7 +563,7 @@ uint64_t SyncRes::doDumpNonResolvingNS(int fd) fprintf(fp.get(), "; name\tcount\ttimestamp\n"); uint64_t count=0; - for(const auto& i : t_sstorage.nonresolving.getMap()) + for(const auto& i : s_nonresolving.lock()->getMap()) { count++; char tmp[26]; @@ -2357,7 +2359,7 @@ vector SyncRes::retrieveAddressesForNS(const std::string& prefix, size_t nonresolvingfails = 0; if (!tns->first.empty()) { if (s_nonresolvingnsmaxfails > 0) { - nonresolvingfails = t_sstorage.nonresolving.value(tns->first); + nonresolvingfails = s_nonresolving.lock()->value(tns->first); if (nonresolvingfails >= s_nonresolvingnsmaxfails) { LOG(prefix<first<< " in non-resolving map, skipping"< SyncRes::retrieveAddressesForNS(const std::string& prefix, if (s_nonresolvingnsmaxfails > 0 && d_outqueries > oldOutQueries) { auto dontThrottleNames = g_dontThrottleNames.getLocal(); if (!dontThrottleNames->check(tns->first)) { - t_sstorage.nonresolving.incr(tns->first, d_now); + s_nonresolving.lock()->incr(tns->first, d_now); } } throw ex; @@ -2383,12 +2385,12 @@ vector SyncRes::retrieveAddressesForNS(const std::string& prefix, if (result.empty()) { auto dontThrottleNames = g_dontThrottleNames.getLocal(); if (!dontThrottleNames->check(tns->first)) { - t_sstorage.nonresolving.incr(tns->first, d_now); + s_nonresolving.lock()->incr(tns->first, d_now); } } else if (nonresolvingfails > 0) { // Succeeding resolve, clear memory of recent failures - t_sstorage.nonresolving.clear(tns->first); + s_nonresolving.lock()->clear(tns->first); } } pierceDontQuery=false; @@ -4056,7 +4058,7 @@ bool SyncRes::doResolveAtThisIP(const std::string& prefix, const DNSName& qname, t_sstorage.nsSpeeds[nsName.empty()? DNSName(remoteIP.toStringWithPort()) : nsName].submit(remoteIP, 1000000, d_now); // 1 sec // code below makes sure we don't filter COM or the root - if (s_serverdownmaxfails > 0 && (auth != g_rootdnsname) && t_sstorage.fails.incr(remoteIP, d_now) >= s_serverdownmaxfails) { + if (s_serverdownmaxfails > 0 && (auth != g_rootdnsname) && s_fails.lock()->incr(remoteIP, d_now) >= s_serverdownmaxfails) { LOG(prefix< 0) { - t_sstorage.fails.clear(remoteIP); + s_fails.lock()->clear(remoteIP); } if (lwr.d_tcbit) { diff --git a/pdns/syncres.hh b/pdns/syncres.hh index 90b0259b43..7f081d904b 100644 --- a/pdns/syncres.hh +++ b/pdns/syncres.hh @@ -196,7 +196,7 @@ template class fails_t : public boost::noncopyable { public: - typedef unsigned long long counter_t; + typedef uint64_t counter_t; struct value_t { value_t(const T &a) : key(a) {} T key; @@ -210,7 +210,7 @@ public: ordered_non_unique, member> >> cont_t; - cont_t getMap() const { + const cont_t& getMap() const { return d_cont; } counter_t value(const T& t) const @@ -406,12 +406,13 @@ public: }; + static LockGuarded> s_fails; + static LockGuarded> s_nonresolving; + struct ThreadLocalStorage { nsspeeds_t nsSpeeds; throttle_t throttle; ednsstatus_t ednsstatus; - fails_t fails; - fails_t nonresolving; std::shared_ptr domainmap; }; @@ -542,31 +543,31 @@ public: } static uint64_t getFailedServersSize() { - return t_sstorage.fails.size(); + return s_fails.lock()->size(); } static uint64_t getNonResolvingNSSize() { - return t_sstorage.nonresolving.size(); + return s_nonresolving.lock()->size(); } static void clearFailedServers() { - t_sstorage.fails.clear(); + s_fails.lock()->clear(); } static void clearNonResolvingNS() { - t_sstorage.nonresolving.clear(); + s_nonresolving.lock()->clear(); } static void pruneFailedServers(time_t cutoff) { - t_sstorage.fails.prune(cutoff); + s_fails.lock()->prune(cutoff); } static unsigned long getServerFailsCount(const ComboAddress& server) { - return t_sstorage.fails.value(server); + return s_fails.lock()->value(server); } static void pruneNonResolving(time_t cutoff) { - t_sstorage.nonresolving.prune(cutoff); + s_nonresolving.lock()->prune(cutoff); } static void setDomainMap(std::shared_ptr newMap) {