From: Otto <otto.moerbeek@open-xchange.com>
Date: Fri, 25 Jun 2021 09:16:04 +0000 (+0200)
Subject: Change the static lock() method of MemRecursorCache to be an instance
X-Git-Tag: dnsdist-1.7.0-alpha1~62^2~1
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=05fbdcd7e69b9d0acc586e51cbca6639ef3a7863;p=thirdparty%2Fpdns.git

Change the static lock() method of MemRecursorCache to be an instance
method of MapCombo. Likewise for negcache.
---

diff --git a/pdns/cachecleaner.hh b/pdns/cachecleaner.hh
index 1841de9ba7..55ddd48fc1 100644
--- a/pdns/cachecleaner.hh
+++ b/pdns/cachecleaner.hh
@@ -144,7 +144,7 @@ template <typename S, typename C, typename T> uint64_t pruneMutexCollectionsVect
   }
 
   for (auto& content : maps) {
-    auto mc = C::lock(content.d_content);
+    auto mc = content.d_content.lock();
     mc->invalidate();
     auto& sidx = boost::multi_index::get<S>(mc->d_map);
     uint64_t erased = 0, lookedAt = 0;
@@ -175,10 +175,10 @@ template <typename S, typename C, typename T> uint64_t pruneMutexCollectionsVect
 
   toTrim -= totErased;
 
-    while (true) {
+  while (true) {
     size_t pershard = toTrim / maps_size + 1;
     for (auto& content : maps) {
-      auto mc = C::lock(content.d_content);
+      auto mc = content.d_content.lock();
       mc->invalidate();
       auto& sidx = boost::multi_index::get<S>(mc->d_map);
       size_t removed = 0;
diff --git a/pdns/recursor_cache.cc b/pdns/recursor_cache.cc
index 2b526eefce..9e2aa85815 100644
--- a/pdns/recursor_cache.cc
+++ b/pdns/recursor_cache.cc
@@ -32,7 +32,7 @@ pair<uint64_t,uint64_t> MemRecursorCache::stats()
 {
   uint64_t c = 0, a = 0;
   for (auto& mc : d_maps) {
-    auto content = lock(mc.d_content);
+    auto content = mc.d_content.lock();
     c += content->d_contended_count;
     a += content->d_acquired_count;
   }
@@ -44,7 +44,7 @@ size_t MemRecursorCache::ecsIndexSize()
   // XXX!
   size_t count = 0;
   for (auto& mc : d_maps) {
-    auto content = lock(mc.d_content);
+    auto content = mc.d_content.lock();
     count += content->d_ecsIndex.size();
   }
   return count;
@@ -55,7 +55,7 @@ size_t MemRecursorCache::bytes()
 {
   size_t ret = 0;
   for (auto& mc : d_maps) {
-    auto m = lock(mc.d_content);
+    auto m = mc.d_content.lock();
     for (const auto& i : m->d_map) {
       ret += sizeof(struct CacheEntry);
       ret += i.d_qname.toString().length();
@@ -273,7 +273,7 @@ time_t MemRecursorCache::get(time_t now, const DNSName &qname, const QType qt, b
   }
 
   auto& mc = getMap(qname);
-  auto map = lock(mc.d_content);
+  auto map = mc.d_content.lock();
 
   /* 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,7 +389,7 @@ 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<DNSRecord>& content, const vector<shared_ptr<RRSIGRecordContent>>& signatures, const std::vector<std::shared_ptr<DNSRecord>>& authorityRecs, bool auth, const DNSName& authZone, boost::optional<Netmask> ednsmask, const OptTag& routingTag, vState state, boost::optional<ComboAddress> from)
 {
   auto& mc = getMap(qname);
-  auto map = lock(mc.d_content);
+  auto map = mc.d_content.lock();
 
   map->d_cachecachevalid = false;
   if (ednsmask) {
@@ -492,7 +492,7 @@ size_t MemRecursorCache::doWipeCache(const DNSName& name, bool sub, const QType
 
   if (!sub) {
     auto& mc = getMap(name);
-    auto map = lock(mc.d_content);
+    auto map = mc.d_content.lock();
     map->d_cachecachevalid = false;
     auto& idx = map->d_map.get<OrderedTag>();
     auto range = idx.equal_range(name);
@@ -520,7 +520,7 @@ size_t MemRecursorCache::doWipeCache(const DNSName& name, bool sub, const QType
   }
   else {
     for (auto& mc : d_maps) {
-      auto map = lock(mc.d_content);
+      auto map = mc.d_content.lock();
       map->d_cachecachevalid = false;
       auto& idx = map->d_map.get<OrderedTag>();
       for (auto i = idx.lower_bound(name); i != idx.end(); ) {
@@ -553,7 +553,7 @@ size_t MemRecursorCache::doWipeCache(const DNSName& name, bool sub, const QType
 bool MemRecursorCache::doAgeCache(time_t now, const DNSName& name, const QType qtype, uint32_t newTTL)
 {
   auto& mc = getMap(name);
-  auto map = lock(mc.d_content);
+  auto map = mc.d_content.lock();
   cache_t::iterator iter = map->d_map.find(tie(name, qtype));
   if (iter == map->d_map.end()) {
     return false;
@@ -589,7 +589,7 @@ bool MemRecursorCache::updateValidationStatus(time_t now, const DNSName &qname,
   }
 
   auto& mc = getMap(qname);
-  auto map = lock(mc.d_content);
+  auto map = mc.d_content.lock();
 
   bool updated = false;
   if (!map->d_ecsIndex.empty() && !routingTag) {
@@ -642,7 +642,7 @@ uint64_t MemRecursorCache::doDump(int fd)
   uint64_t count = 0;
 
   for (auto& mc : d_maps) {
-    auto map = lock(mc.d_content);
+    auto map = mc.d_content.lock();
     const auto& sidx = map->d_map.get<SequencedTag>();
 
     time_t now = time(nullptr);
diff --git a/pdns/recursor_cache.hh b/pdns/recursor_cache.hh
index ebb2f70060..9afebd22fa 100644
--- a/pdns/recursor_cache.hh
+++ b/pdns/recursor_cache.hh
@@ -226,6 +226,17 @@ private:
 
     LockGuarded<LockedContent> d_content;
     std::atomic<uint64_t> d_entriesCount{0};
+
+    LockGuardedTryHolder<LockedContent> lock()
+    {
+      auto locked = d_content.try_lock();
+      if (!locked.owns_lock()) {
+        locked.lock();
+        ++locked->d_contended_count;
+      }
+      ++locked->d_acquired_count;
+      return locked;
+    }
   };
 
   vector<MapCombo> d_maps;
@@ -243,17 +254,6 @@ private:
   time_t handleHit(MapCombo::LockedContent& content, OrderedTagIterator_t& entry, const DNSName& qname, uint32_t& origTTL, vector<DNSRecord>* res, vector<std::shared_ptr<RRSIGRecordContent>>* signatures, std::vector<std::shared_ptr<DNSRecord>>* authorityRecs, bool* variable, boost::optional<vState>& state, bool* wasAuth, DNSName* authZone);
 
 public:
-  static LockGuardedTryHolder<MapCombo::LockedContent> lock(LockGuarded<MapCombo::LockedContent>& content)
-  {
-    auto locked = content.try_lock();
-    if (!locked.owns_lock()) {
-      locked.lock();
-      ++locked->d_contended_count;
-    }
-    ++locked->d_acquired_count;
-    return locked;
-  }
-
   void preRemoval(MapCombo::LockedContent& map, const CacheEntry& entry)
   {
     if (entry.d_netmask.empty()) {
diff --git a/pdns/recursordist/negcache.cc b/pdns/recursordist/negcache.cc
index be51971196..1fe7c296e8 100644
--- a/pdns/recursordist/negcache.cc
+++ b/pdns/recursordist/negcache.cc
@@ -60,7 +60,7 @@ bool NegCache::getRootNXTrust(const DNSName& qname, const struct timeval& now, N
   DNSName lastLabel = qname.getLastLabel();
 
   auto& map = getMap(lastLabel);
-  auto content = lock(map.d_content);
+  auto content = map.d_content.lock();
 
   negcache_t::const_iterator ni = content->d_map.find(tie(lastLabel, qtnull));
 
@@ -89,7 +89,7 @@ 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& map = getMap(qname);
-  auto content = lock(map.d_content);
+  auto content = map.d_content.lock();
 
   const auto& idx = content->d_map.get<NegCacheEntry>();
   auto range = idx.equal_range(qname);
@@ -124,7 +124,7 @@ void NegCache::add(const NegCacheEntry& ne)
 {
   bool inserted = false;
   auto& map = getMap(ne.d_name);
-  auto content = lock(map.d_content);
+  auto content = map.d_content.lock();
   inserted = lruReplacingInsert<SequenceTag>(content->d_map, ne);
   if (inserted) {
     ++map.d_entriesCount;
@@ -141,7 +141,7 @@ void NegCache::add(const NegCacheEntry& ne)
 void NegCache::updateValidationStatus(const DNSName& qname, const QType& qtype, const vState newState, boost::optional<time_t> capTTD)
 {
   auto& mc = getMap(qname);
-  auto map = lock(mc.d_content);
+  auto map = mc.d_content.lock();
   auto range = map->d_map.equal_range(tie(qname, qtype));
 
   if (range.first != range.second) {
@@ -160,7 +160,7 @@ void NegCache::updateValidationStatus(const DNSName& qname, const QType& qtype,
 size_t NegCache::count(const DNSName& qname)
 {
   auto& map = getMap(qname);
-  auto content = lock(map.d_content);
+  auto content = map.d_content.lock();
   return content->d_map.count(tie(qname));
 }
 
@@ -173,7 +173,7 @@ size_t NegCache::count(const DNSName& qname)
 size_t NegCache::count(const DNSName& qname, const QType qtype)
 {
   auto& map = getMap(qname);
-  auto content = lock(map.d_content);
+  auto content = map.d_content.lock();
   return content->d_map.count(tie(qname, qtype));
 }
 
@@ -189,7 +189,7 @@ size_t NegCache::wipe(const DNSName& name, bool subtree)
   size_t ret = 0;
   if (subtree) {
     for (auto& map : d_maps) {
-      auto m = lock(map.d_content);
+      auto m = map.d_content.lock();
       for (auto i = m->d_map.lower_bound(tie(name)); i != m->d_map.end();) {
         if (!i->d_name.isPartOf(name))
           break;
@@ -202,7 +202,7 @@ size_t NegCache::wipe(const DNSName& name, bool subtree)
   }
 
   auto& map = getMap(name);
-  auto content = lock(map.d_content);
+  auto content = map.d_content.lock();
   auto range = content->d_map.equal_range(tie(name));
   auto i = range.first;
   while (i != range.second) {
@@ -219,7 +219,7 @@ size_t NegCache::wipe(const DNSName& name, bool subtree)
 void NegCache::clear()
 {
   for (auto& map : d_maps) {
-    auto m = lock(map.d_content);
+    auto m = map.d_content.lock();
     m->d_map.clear();
     map.d_entriesCount = 0;
   }
@@ -246,7 +246,7 @@ size_t NegCache::dumpToFile(FILE* fp, const struct timeval& now)
   size_t ret = 0;
 
   for (auto& mc : d_maps) {
-    auto m = lock(mc.d_content);
+    auto m = mc.d_content.lock();
     auto& sidx = m->d_map.get<SequenceTag>();
     for (const NegCacheEntry& ne : sidx) {
       ret++;
diff --git a/pdns/recursordist/negcache.hh b/pdns/recursordist/negcache.hh
index 5d2260762d..97dbe3e8c8 100644
--- a/pdns/recursordist/negcache.hh
+++ b/pdns/recursordist/negcache.hh
@@ -115,6 +115,17 @@ private:
     };
     LockGuarded<LockedContent> d_content;
     std::atomic<uint64_t> d_entriesCount{0};
+
+    LockGuardedTryHolder<MapCombo::LockedContent> lock()
+    {
+      auto locked = d_content.try_lock();
+      if (!locked.owns_lock()) {
+        locked.lock();
+        ++locked->d_contended_count;
+      }
+      ++locked->d_acquired_count;
+      return locked;
+    }
   };
 
   vector<MapCombo> d_maps;
@@ -129,16 +140,6 @@ private:
   }
 
 public:
-  static LockGuardedTryHolder<MapCombo::LockedContent> lock(LockGuarded<MapCombo::LockedContent>& content)
-  {
-    auto locked = content.try_lock();
-    if (!locked.owns_lock()) {
-      locked.lock();
-      ++locked->d_contended_count;
-    }
-    ++locked->d_acquired_count;
-    return locked;
-  }
 
   void preRemoval(MapCombo::LockedContent& map, const NegCacheEntry& entry)
   {