From: Remi Gacogne Date: Wed, 16 Jun 2021 15:31:38 +0000 (+0200) Subject: rec: Do not require taking the lock to know the size of a cache X-Git-Tag: dnsdist-1.7.0-alpha1~62^2~3 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=7a83afe1a02e929ca39bbcedc37efb2b3e785e8a;p=thirdparty%2Fpdns.git rec: Do not require taking the lock to know the size of a cache --- diff --git a/pdns/cachecleaner.hh b/pdns/cachecleaner.hh index fecdbc1ab2..1841de9ba7 100644 --- a/pdns/cachecleaner.hh +++ b/pdns/cachecleaner.hh @@ -139,11 +139,12 @@ template uint64_t pruneMutexCollectionsVect } uint64_t maps_size = maps.size(); - if (maps_size == 0) - return 0; + if (maps_size == 0) { + return 0; + } - for (auto& lockGuardedMap : maps) { - auto mc = C::lock(lockGuardedMap); + for (auto& content : maps) { + auto mc = C::lock(content.d_content); mc->invalidate(); auto& sidx = boost::multi_index::get(mc->d_map); uint64_t erased = 0, lookedAt = 0; @@ -152,7 +153,7 @@ template uint64_t pruneMutexCollectionsVect container.preRemoval(*mc, *i); i = sidx.erase(i); erased++; - mc->d_entriesCount--; + --content.d_entriesCount; } else { ++i; } @@ -176,15 +177,15 @@ template uint64_t pruneMutexCollectionsVect while (true) { size_t pershard = toTrim / maps_size + 1; - for (auto& lockGuardedMap : maps) { - auto mc = C::lock(lockGuardedMap); + for (auto& content : maps) { + auto mc = C::lock(content.d_content); mc->invalidate(); auto& sidx = boost::multi_index::get(mc->d_map); size_t removed = 0; for (auto i = sidx.begin(); i != sidx.end() && removed < pershard; removed++) { container.preRemoval(*mc, *i); i = sidx.erase(i); - mc->d_entriesCount--; + --content.d_entriesCount; totErased++; toTrim--; if (toTrim == 0) { diff --git a/pdns/recursor_cache.cc b/pdns/recursor_cache.cc index 972b6010a6..2b526eefce 100644 --- a/pdns/recursor_cache.cc +++ b/pdns/recursor_cache.cc @@ -19,12 +19,11 @@ MemRecursorCache::MemRecursorCache(size_t mapsCount) : d_maps(mapsCount) { } -size_t MemRecursorCache::size() +size_t MemRecursorCache::size() const { size_t count = 0; - for (auto& lockGuardedMap : d_maps) { - auto map = lock(lockGuardedMap); - count += map->d_entriesCount; + for (const auto& map : d_maps) { + count += map.d_entriesCount; } return count; } @@ -32,10 +31,10 @@ size_t MemRecursorCache::size() pair MemRecursorCache::stats() { uint64_t c = 0, a = 0; - for (auto& lockGuardedMap : d_maps) { - auto map = lock(lockGuardedMap); - c += map->d_contended_count; - a += map->d_acquired_count; + for (auto& mc : d_maps) { + auto content = lock(mc.d_content); + c += content->d_contended_count; + a += content->d_acquired_count; } return pair(c, a); } @@ -44,9 +43,9 @@ size_t MemRecursorCache::ecsIndexSize() { // XXX! size_t count = 0; - for (auto& map : d_maps) { - auto m = lock(map); - count += m->d_ecsIndex.size(); + for (auto& mc : d_maps) { + auto content = lock(mc.d_content); + count += content->d_ecsIndex.size(); } return count; } @@ -55,8 +54,8 @@ size_t MemRecursorCache::ecsIndexSize() size_t MemRecursorCache::bytes() { size_t ret = 0; - for (auto& map : d_maps) { - auto m = lock(map); + for (auto& mc : d_maps) { + auto m = lock(mc.d_content); for (const auto& i : m->d_map) { ret += sizeof(struct CacheEntry); ret += i.d_qname.toString().length(); @@ -100,9 +99,9 @@ static void updateDNSSECValidationStateFromCache(boost::optional& state, } } -time_t MemRecursorCache::handleHit(MapCombo& map, MemRecursorCache::OrderedTagIterator_t& entry, const DNSName& qname, uint32_t& origTTL, vector* res, vector>* signatures, std::vector>* authorityRecs, bool* variable, boost::optional& state, bool* wasAuth, DNSName* fromAuthZone) +time_t MemRecursorCache::handleHit(MapCombo::LockedContent& content, MemRecursorCache::OrderedTagIterator_t& entry, const DNSName& qname, uint32_t& origTTL, vector* res, vector>* signatures, std::vector>* authorityRecs, bool* variable, boost::optional& state, bool* wasAuth, DNSName* fromAuthZone) { - // MUTEX SHOULD BE ACQUIRED + // MUTEX SHOULD BE ACQUIRED (as indicated by the reference to the content which is protected by a lock) time_t ttd = entry->d_ttd; origTTL = entry->d_orig_ttl; @@ -143,14 +142,14 @@ time_t MemRecursorCache::handleHit(MapCombo& map, MemRecursorCache::OrderedTagIt *fromAuthZone = entry->d_authZone; } - moveCacheItemToBack(map.d_map, entry); + moveCacheItemToBack(content.d_map, entry); return ttd; } -MemRecursorCache::cache_t::const_iterator MemRecursorCache::getEntryUsingECSIndex(MapCombo& map, time_t now, const DNSName &qname, const QType qtype, bool requireAuth, const ComboAddress& who) +MemRecursorCache::cache_t::const_iterator MemRecursorCache::getEntryUsingECSIndex(MapCombo::LockedContent& map, time_t now, const DNSName &qname, const QType qtype, bool requireAuth, const ComboAddress& who) { - // MUTEX SHOULD BE ACQUIRED + // MUTEX SHOULD BE ACQUIRED (as indicated by the reference to the content which is protected by a lock) auto ecsIndexKey = tie(qname, qtype); auto ecsIndex = map.d_ecsIndex.find(ecsIndexKey); if (ecsIndex != map.d_ecsIndex.end() && !ecsIndex->isEmpty()) { @@ -210,7 +209,7 @@ MemRecursorCache::cache_t::const_iterator MemRecursorCache::getEntryUsingECSInde return map.d_map.end(); } -MemRecursorCache::Entries MemRecursorCache::getEntries(MapCombo& map, const DNSName &qname, const QType qt, const OptTag& rtag ) +MemRecursorCache::Entries MemRecursorCache::getEntries(MapCombo::LockedContent& map, const DNSName &qname, const QType qt, const OptTag& rtag ) { // MUTEX SHOULD BE ACQUIRED if (!map.d_cachecachevalid || map.d_cachedqname != qname || map.d_cachedrtag != rtag) { @@ -273,8 +272,8 @@ time_t MemRecursorCache::get(time_t now, const DNSName &qname, const QType qt, b *wasAuth = true; } - auto& lockGuardedMap = getMap(qname); - auto map = lock(lockGuardedMap); + auto& mc = getMap(qname); + auto map = lock(mc.d_content); /* If we don't have any netmask-specific entries at all, let's just skip this to be able to use the nice d_cachecache hack. */ @@ -389,8 +388,8 @@ time_t MemRecursorCache::get(time_t now, const DNSName &qname, const QType qt, b void MemRecursorCache::replace(time_t now, const DNSName &qname, const QType qt, const vector& content, const vector>& signatures, const std::vector>& authorityRecs, bool auth, const DNSName& authZone, boost::optional ednsmask, const OptTag& routingTag, vState state, boost::optional from) { - auto& lockGuardedMap = getMap(qname); - auto map = lock(lockGuardedMap); + auto& mc = getMap(qname); + auto map = lock(mc.d_content); map->d_cachecachevalid = false; if (ednsmask) { @@ -404,7 +403,7 @@ void MemRecursorCache::replace(time_t now, const DNSName &qname, const QType qt, cache_t::iterator stored = map->d_map.find(key); if (stored == map->d_map.end()) { stored = map->d_map.insert(CacheEntry(key, auth)).first; - map->d_entriesCount++; + ++mc.d_entriesCount; isNew = true; } @@ -492,8 +491,8 @@ size_t MemRecursorCache::doWipeCache(const DNSName& name, bool sub, const QType size_t count = 0; if (!sub) { - auto& lockGuardedMap = getMap(name); - auto map = lock(lockGuardedMap); + auto& mc = getMap(name); + auto map = lock(mc.d_content); map->d_cachecachevalid = false; auto& idx = map->d_map.get(); auto range = idx.equal_range(name); @@ -502,7 +501,7 @@ size_t MemRecursorCache::doWipeCache(const DNSName& name, bool sub, const QType if (i->d_qtype == qtype || qtype == 0xffff) { i = idx.erase(i); count++; - map->d_entriesCount--; + --mc.d_entriesCount; } else { ++i; } @@ -520,8 +519,8 @@ size_t MemRecursorCache::doWipeCache(const DNSName& name, bool sub, const QType } } else { - for (auto& lockGuardedMap : d_maps) { - auto map = lock(lockGuardedMap); + for (auto& mc : d_maps) { + auto map = lock(mc.d_content); map->d_cachecachevalid = false; auto& idx = map->d_map.get(); for (auto i = idx.lower_bound(name); i != idx.end(); ) { @@ -530,7 +529,7 @@ size_t MemRecursorCache::doWipeCache(const DNSName& name, bool sub, const QType if (i->d_qtype == qtype || qtype == 0xffff) { count++; i = idx.erase(i); - map->d_entriesCount--; + --mc.d_entriesCount; } else { ++i; } @@ -553,8 +552,8 @@ size_t MemRecursorCache::doWipeCache(const DNSName& name, bool sub, const QType // Name should be doLimitTime or so bool MemRecursorCache::doAgeCache(time_t now, const DNSName& name, const QType qtype, uint32_t newTTL) { - auto& lockGuardedMap = getMap(name); - auto map = lock(lockGuardedMap); + auto& mc = getMap(name); + auto map = lock(mc.d_content); cache_t::iterator iter = map->d_map.find(tie(name, qtype)); if (iter == map->d_map.end()) { return false; @@ -589,8 +588,8 @@ bool MemRecursorCache::updateValidationStatus(time_t now, const DNSName &qname, throw std::runtime_error("Trying to update the DNSSEC validation status of several (via ADDR) records for " + qname.toLogString()); } - auto& lockGuardedMap = getMap(qname); - auto map = lock(lockGuardedMap); + auto& mc = getMap(qname); + auto map = lock(mc.d_content); bool updated = false; if (!map->d_ecsIndex.empty() && !routingTag) { @@ -642,8 +641,8 @@ uint64_t MemRecursorCache::doDump(int fd) fprintf(fp.get(), "; main record cache dump follows\n;\n"); uint64_t count = 0; - for (auto& lockGuardedMap : d_maps) { - auto map = lock(lockGuardedMap); + for (auto& mc : d_maps) { + auto map = lock(mc.d_content); const auto& sidx = map->d_map.get(); time_t now = time(nullptr); diff --git a/pdns/recursor_cache.hh b/pdns/recursor_cache.hh index dbe7435400..ebb2f70060 100644 --- a/pdns/recursor_cache.hh +++ b/pdns/recursor_cache.hh @@ -49,7 +49,7 @@ class MemRecursorCache : public boost::noncopyable // : public RecursorCache public: MemRecursorCache(size_t mapsCount = 1024); - size_t size(); + size_t size() const; size_t bytes(); pair stats(); size_t ecsIndexSize(); @@ -201,30 +201,35 @@ private: > ecsIndex_t; typedef std::pair Entries; - + struct MapCombo { MapCombo() {} MapCombo(const MapCombo &) = delete; MapCombo & operator=(const MapCombo &) = delete; - cache_t d_map; - ecsIndex_t d_ecsIndex; - DNSName d_cachedqname; - OptTag d_cachedrtag; - Entries d_cachecache; - bool d_cachecachevalid{false}; - std::atomic d_entriesCount{0}; - uint64_t d_contended_count{0}; - uint64_t d_acquired_count{0}; - - void invalidate() + struct LockedContent { - d_cachecachevalid = false; - } + cache_t d_map; + ecsIndex_t d_ecsIndex; + DNSName d_cachedqname; + OptTag d_cachedrtag; + Entries d_cachecache; + uint64_t d_contended_count{0}; + uint64_t d_acquired_count{0}; + bool d_cachecachevalid{false}; + + void invalidate() + { + d_cachecachevalid = false; + } + }; + + LockGuarded d_content; + std::atomic d_entriesCount{0}; }; - vector> d_maps; - LockGuarded& getMap(const DNSName &qname) + vector d_maps; + MapCombo& getMap(const DNSName &qname) { return d_maps.at(qname.hash() % d_maps.size()); } @@ -232,24 +237,24 @@ private: static time_t fakeTTD(OrderedTagIterator_t& entry, const DNSName& qname, QType qtype, time_t ret, time_t now, uint32_t origTTL, bool refresh); bool entryMatches(OrderedTagIterator_t& entry, QType qt, bool requireAuth, const ComboAddress& who); - Entries getEntries(MapCombo& map, const DNSName &qname, const QType qt, const OptTag& rtag); - cache_t::const_iterator getEntryUsingECSIndex(MapCombo& map, time_t now, const DNSName &qname, QType qtype, bool requireAuth, const ComboAddress& who); + Entries getEntries(MapCombo::LockedContent& content, const DNSName &qname, const QType qt, const OptTag& rtag); + cache_t::const_iterator getEntryUsingECSIndex(MapCombo::LockedContent& content, time_t now, const DNSName &qname, QType qtype, bool requireAuth, const ComboAddress& who); - time_t handleHit(MapCombo& map, OrderedTagIterator_t& entry, const DNSName& qname, uint32_t& origTTL, vector* res, vector>* signatures, std::vector>* authorityRecs, bool* variable, boost::optional& state, bool* wasAuth, DNSName* authZone); + time_t handleHit(MapCombo::LockedContent& content, OrderedTagIterator_t& entry, const DNSName& qname, uint32_t& origTTL, vector* res, vector>* signatures, std::vector>* authorityRecs, bool* variable, boost::optional& state, bool* wasAuth, DNSName* authZone); public: - static LockGuardedTryHolder lock(LockGuarded& map) + static LockGuardedTryHolder lock(LockGuarded& content) { - auto locked = map.try_lock(); + auto locked = content.try_lock(); if (!locked.owns_lock()) { locked.lock(); - locked->d_contended_count++; + ++locked->d_contended_count; } ++locked->d_acquired_count; return locked; } - void preRemoval(MapCombo& map, const CacheEntry& entry) + void preRemoval(MapCombo::LockedContent& map, const CacheEntry& entry) { if (entry.d_netmask.empty()) { return; diff --git a/pdns/recursordist/negcache.cc b/pdns/recursordist/negcache.cc index 717ce75b90..be51971196 100644 --- a/pdns/recursordist/negcache.cc +++ b/pdns/recursordist/negcache.cc @@ -31,12 +31,11 @@ NegCache::NegCache(size_t mapsCount) : { } -size_t NegCache::size() +size_t NegCache::size() const { size_t count = 0; - for (auto& lockGuardedMap : d_maps) { - auto map = lock(lockGuardedMap); - count += map->d_entriesCount; + for (const auto& map : d_maps) { + count += map.d_entriesCount; } return count; } @@ -60,19 +59,19 @@ bool NegCache::getRootNXTrust(const DNSName& qname, const struct timeval& now, N static const QType qtnull(0); DNSName lastLabel = qname.getLastLabel(); - auto& lockGuardedMap = getMap(lastLabel); - auto map = lock(lockGuardedMap); + auto& map = getMap(lastLabel); + auto content = lock(map.d_content); - negcache_t::const_iterator ni = map->d_map.find(tie(lastLabel, qtnull)); + negcache_t::const_iterator ni = content->d_map.find(tie(lastLabel, qtnull)); - while (ni != map->d_map.end() && ni->d_name == lastLabel && ni->d_auth.isRoot() && ni->d_qtype == qtnull) { + while (ni != content->d_map.end() && ni->d_name == lastLabel && ni->d_auth.isRoot() && ni->d_qtype == qtnull) { // We have something if (now.tv_sec < ni->d_ttd) { ne = *ni; - moveCacheItemToBack(map->d_map, ni); + moveCacheItemToBack(content->d_map, ni); return true; } - moveCacheItemToFront(map->d_map, ni); + moveCacheItemToFront(content->d_map, ni); ++ni; } return false; @@ -89,10 +88,10 @@ bool NegCache::getRootNXTrust(const DNSName& qname, const struct timeval& now, N */ bool NegCache::get(const DNSName& qname, const QType& qtype, const struct timeval& now, NegCacheEntry& ne, bool typeMustMatch) { - auto& lockGuardedMap = getMap(qname); - auto map = lock(lockGuardedMap); + auto& map = getMap(qname); + auto content = lock(map.d_content); - const auto& idx = map->d_map.get(); + const auto& idx = content->d_map.get(); auto range = idx.equal_range(qname); auto ni = range.first; @@ -100,16 +99,16 @@ bool NegCache::get(const DNSName& qname, const QType& qtype, const struct timeva // We have an entry if ((!typeMustMatch && ni->d_qtype.getCode() == 0) || ni->d_qtype == qtype) { // We match the QType or the whole name is denied - auto firstIndexIterator = map->d_map.project(ni); + auto firstIndexIterator = content->d_map.project(ni); if (now.tv_sec < ni->d_ttd) { // Not expired ne = *ni; - moveCacheItemToBack(map->d_map, firstIndexIterator); + moveCacheItemToBack(content->d_map, firstIndexIterator); return true; } // expired - moveCacheItemToFront(map->d_map, firstIndexIterator); + moveCacheItemToFront(content->d_map, firstIndexIterator); } ++ni; } @@ -123,11 +122,12 @@ bool NegCache::get(const DNSName& qname, const QType& qtype, const struct timeva */ void NegCache::add(const NegCacheEntry& ne) { - auto& lockGuardedMap = getMap(ne.d_name); - auto map = lock(lockGuardedMap); - bool inserted = lruReplacingInsert(map->d_map, ne); + bool inserted = false; + auto& map = getMap(ne.d_name); + auto content = lock(map.d_content); + inserted = lruReplacingInsert(content->d_map, ne); if (inserted) { - ++map->d_entriesCount; + ++map.d_entriesCount; } } @@ -140,8 +140,8 @@ void NegCache::add(const NegCacheEntry& ne) */ void NegCache::updateValidationStatus(const DNSName& qname, const QType& qtype, const vState newState, boost::optional capTTD) { - auto& lockGuardedMap = getMap(qname); - auto map = lock(lockGuardedMap); + auto& mc = getMap(qname); + auto map = lock(mc.d_content); auto range = map->d_map.equal_range(tie(qname, qtype)); if (range.first != range.second) { @@ -159,9 +159,9 @@ void NegCache::updateValidationStatus(const DNSName& qname, const QType& qtype, */ size_t NegCache::count(const DNSName& qname) { - auto& lockGuardedMap = getMap(qname); - auto map = lock(lockGuardedMap); - return map->d_map.count(tie(qname)); + auto& map = getMap(qname); + auto content = lock(map.d_content); + return content->d_map.count(tie(qname)); } /*! @@ -172,9 +172,9 @@ size_t NegCache::count(const DNSName& qname) */ size_t NegCache::count(const DNSName& qname, const QType qtype) { - auto& lockGuardedMap = getMap(qname); - auto map = lock(lockGuardedMap); - return map->d_map.count(tie(qname, qtype)); + auto& map = getMap(qname); + auto content = lock(map.d_content); + return content->d_map.count(tie(qname, qtype)); } /*! @@ -189,26 +189,26 @@ size_t NegCache::wipe(const DNSName& name, bool subtree) size_t ret = 0; if (subtree) { for (auto& map : d_maps) { - auto m = lock(map); + auto m = lock(map.d_content); for (auto i = m->d_map.lower_bound(tie(name)); i != m->d_map.end();) { if (!i->d_name.isPartOf(name)) break; i = m->d_map.erase(i); ret++; - --m->d_entriesCount; + --map.d_entriesCount; } } return ret; } - auto& lockGuardedMap = getMap(name); - auto map = lock(lockGuardedMap); - auto range = map->d_map.equal_range(tie(name)); + auto& map = getMap(name); + auto content = lock(map.d_content); + auto range = content->d_map.equal_range(tie(name)); auto i = range.first; while (i != range.second) { - i = map->d_map.erase(i); + i = content->d_map.erase(i); ret++; - --map->d_entriesCount; + --map.d_entriesCount; } return ret; } @@ -219,9 +219,9 @@ size_t NegCache::wipe(const DNSName& name, bool subtree) void NegCache::clear() { for (auto& map : d_maps) { - auto m = lock(map); + auto m = lock(map.d_content); m->d_map.clear(); - m->d_entriesCount = 0; + map.d_entriesCount = 0; } } @@ -245,8 +245,8 @@ size_t NegCache::dumpToFile(FILE* fp, const struct timeval& now) { size_t ret = 0; - for (auto& map : d_maps) { - auto m = lock(map); + for (auto& mc : d_maps) { + auto m = lock(mc.d_content); auto& sidx = m->d_map.get(); for (const NegCacheEntry& ne : sidx) { ret++; diff --git a/pdns/recursordist/negcache.hh b/pdns/recursordist/negcache.hh index 1a875e5960..5d2260762d 100644 --- a/pdns/recursordist/negcache.hh +++ b/pdns/recursordist/negcache.hh @@ -77,7 +77,7 @@ public: void clear(); size_t dumpToFile(FILE* fd, const struct timeval& now); size_t wipe(const DNSName& name, bool subtree = false); - size_t size(); + size_t size() const; private: struct CompositeKey @@ -106,37 +106,41 @@ private: MapCombo() {} MapCombo(const MapCombo&) = delete; MapCombo& operator=(const MapCombo&) = delete; - negcache_t d_map; + struct LockedContent + { + negcache_t d_map; + uint64_t d_contended_count{0}; + uint64_t d_acquired_count{0}; + void invalidate() {} + }; + LockGuarded d_content; std::atomic d_entriesCount{0}; - uint64_t d_contended_count{0}; - uint64_t d_acquired_count{0}; - void invalidate() {} }; - vector> d_maps; + vector d_maps; - LockGuarded& getMap(const DNSName& qname) + MapCombo& getMap(const DNSName& qname) { return d_maps.at(qname.hash() % d_maps.size()); } - const LockGuarded& getMap(const DNSName& qname) const + const MapCombo& getMap(const DNSName& qname) const { return d_maps.at(qname.hash() % d_maps.size()); } public: - static LockGuardedTryHolder lock(LockGuarded& map) + static LockGuardedTryHolder lock(LockGuarded& content) { - auto locked = map.try_lock(); + auto locked = content.try_lock(); if (!locked.owns_lock()) { locked.lock(); - locked->d_contended_count++; + ++locked->d_contended_count; } ++locked->d_acquired_count; return locked; } - void preRemoval(MapCombo& map, const NegCacheEntry& entry) + void preRemoval(MapCombo::LockedContent& map, const NegCacheEntry& entry) { } };