From: Remi Gacogne <remi.gacogne@powerdns.com>
Date: Sat, 12 Jun 2021 08:58:38 +0000 (+0200)
Subject: auth: Convert the caches to LockGuarded
X-Git-Tag: dnsdist-1.7.0-alpha1~11^2
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=refs%2Fpull%2F10653%2Fhead;p=thirdparty%2Fpdns.git

auth: Convert the caches to LockGuarded
---

diff --git a/pdns/auth-packetcache.cc b/pdns/auth-packetcache.cc
index e31a0182ef..15f2bf358a 100644
--- a/pdns/auth-packetcache.cc
+++ b/pdns/auth-packetcache.cc
@@ -43,24 +43,10 @@ AuthPacketCache::AuthPacketCache(size_t mapsCount): d_maps(mapsCount), d_lastcle
   d_statnumentries=S.getPointer("packetcache-size");
 }
 
-AuthPacketCache::~AuthPacketCache()
-{
-  try {
-    vector<WriteLock> locks;
-    for(auto& mc : d_maps) {
-      locks.push_back(WriteLock(mc.d_mut));
-    }
-    locks.clear();
-  }
-  catch(...) {
-  }
-}
-
 void AuthPacketCache::MapCombo::reserve(size_t numberOfEntries)
 {
 #if BOOST_VERSION >= 105600
-  WriteLock wl(&d_mut);
-  d_map.get<HashTag>().reserve(numberOfEntries);
+  d_map.write_lock()->get<HashTag>().reserve(numberOfEntries);
 #endif /* BOOST_VERSION >= 105600 */
 }
 
@@ -80,13 +66,13 @@ bool AuthPacketCache::get(DNSPacket& p, DNSPacket& cached)
   time_t now = time(nullptr);
   auto& mc = getMap(p.qdomain);
   {
-    TryReadLock rl(&mc.d_mut);
-    if(!rl.gotIt()) {
+    auto map = mc.d_map.try_read_lock();
+    if (!map.owns_lock()) {
       S.inc("deferred-packetcache-lookup");
       return false;
     }
 
-    haveSomething = getEntryLocked(mc.d_map, p.getString(), hash, p.qdomain, p.qtype.getCode(), p.d_tcp, now, value);
+    haveSomething = getEntryLocked(*map, p.getString(), hash, p.qdomain, p.qtype.getCode(), p.d_tcp, now, value);
   }
 
   if (!haveSomething) {
@@ -146,13 +132,13 @@ void AuthPacketCache::insert(DNSPacket& q, DNSPacket& r, unsigned int maxTTL)
   
   auto& mc = getMap(entry.qname);
   {
-    TryWriteLock l(&mc.d_mut);
-    if (!l.gotIt()) {
+    auto map = mc.d_map.try_write_lock();
+    if (!map.owns_lock()) {
       S.inc("deferred-packetcache-inserts");
       return;
     }
 
-    auto& idx = mc.d_map.get<HashTag>();
+    auto& idx = map->get<HashTag>();
     auto range = idx.equal_range(hash);
     auto iter = range.first;
 
@@ -161,7 +147,7 @@ void AuthPacketCache::insert(DNSPacket& q, DNSPacket& r, unsigned int maxTTL)
         continue;
       }
 
-      moveCacheItemToBack<SequencedTag>(mc.d_map, iter);
+      moveCacheItemToBack<SequencedTag>(*map, iter);
       iter->value = entry.value;
       iter->ttd = now + ourttl;
       iter->created = now;
@@ -169,11 +155,11 @@ void AuthPacketCache::insert(DNSPacket& q, DNSPacket& r, unsigned int maxTTL)
     }
 
     /* no existing entry found to refresh */
-    mc.d_map.insert(std::move(entry));
+    map->insert(std::move(entry));
 
     if (*d_statnumentries >= d_maxEntries) {
       /* remove the least recently inserted or replaced entry */
-      auto& sidx = mc.d_map.get<SequencedTag>();
+      auto& sidx = map->get<SequencedTag>();
       sidx.pop_front();
     }
     else {
@@ -182,7 +168,7 @@ void AuthPacketCache::insert(DNSPacket& q, DNSPacket& r, unsigned int maxTTL)
   }
 }
 
-bool AuthPacketCache::getEntryLocked(cmap_t& map, const std::string& query, uint32_t hash, const DNSName &qname, uint16_t qtype, bool tcp, time_t now, string& value)
+bool AuthPacketCache::getEntryLocked(const cmap_t& map, const std::string& query, uint32_t hash, const DNSName &qname, uint16_t qtype, bool tcp, time_t now, string& value)
 {
   auto& idx = map.get<HashTag>();
   auto range = idx.equal_range(hash);
diff --git a/pdns/auth-packetcache.hh b/pdns/auth-packetcache.hh
index bceab073de..618041cf17 100644
--- a/pdns/auth-packetcache.hh
+++ b/pdns/auth-packetcache.hh
@@ -48,7 +48,6 @@ class AuthPacketCache : public PacketCache
 {
 public:
   AuthPacketCache(size_t mapsCount=1024);
-  ~AuthPacketCache();
 
   void insert(DNSPacket& q, DNSPacket& r, uint32_t maxTTL);  //!< We copy the contents of *p into our cache. Do not needlessly call this to insert questions already in the cache as it wastes resources
 
@@ -116,8 +115,7 @@ private:
 
     void reserve(size_t numberOfEntries);
 
-    ReadWriteLock d_mut;
-    cmap_t d_map;
+    SharedLockGuarded<cmap_t> d_map;
   };
 
   vector<MapCombo> d_maps;
@@ -127,7 +125,7 @@ private:
   }
 
   static bool entryMatches(cmap_t::index<HashTag>::type::iterator& iter, const std::string& query, const DNSName& qname, uint16_t qtype, bool tcp);
-  bool getEntryLocked(cmap_t& map, const std::string& query, uint32_t hash, const DNSName &qname, uint16_t qtype, bool tcp, time_t now, string& entry);
+  bool getEntryLocked(const cmap_t& map, const std::string& query, uint32_t hash, const DNSName &qname, uint16_t qtype, bool tcp, time_t now, string& entry);
   void cleanupIfNeeded();
 
   AtomicCounter d_ops{0};
diff --git a/pdns/auth-querycache.cc b/pdns/auth-querycache.cc
index 7812a82e1f..6913296d84 100644
--- a/pdns/auth-querycache.cc
+++ b/pdns/auth-querycache.cc
@@ -44,24 +44,10 @@ AuthQueryCache::AuthQueryCache(size_t mapsCount): d_maps(mapsCount), d_lastclean
   d_statnumentries=S.getPointer("query-cache-size");
 }
 
-AuthQueryCache::~AuthQueryCache()
-{
-  try {
-    vector<WriteLock> locks;
-    for(auto& mc : d_maps) {
-      locks.push_back(WriteLock(mc.d_mut));
-    }
-    locks.clear();
-  }
-  catch(...) {
-  }
-}
-
 void AuthQueryCache::MapCombo::reserve(size_t numberOfEntries)
 {
 #if BOOST_VERSION >= 105600
-  WriteLock wl(&d_mut);
-  d_map.get<HashTag>().reserve(numberOfEntries);
+  d_map.write_lock()->get<HashTag>().reserve(numberOfEntries);
 #endif /* BOOST_VERSION >= 105600 */
 }
 
@@ -74,13 +60,13 @@ bool AuthQueryCache::getEntry(const DNSName &qname, const QType& qtype, vector<D
   uint16_t qt = qtype.getCode();
   auto& mc = getMap(qname);
   {
-    TryReadLock rl(&mc.d_mut);
-    if(!rl.gotIt()) {
+    auto map = mc.d_map.try_read_lock();
+    if (!map.owns_lock()) {
       S.inc("deferred-cache-lookup");
       return false;
     }
 
-    return getEntryLocked(mc.d_map, qname, qt, value, zoneID, now);
+    return getEntryLocked(*map, qname, qt, value, zoneID, now);
   }
 }
 
@@ -103,24 +89,24 @@ void AuthQueryCache::insert(const DNSName &qname, const QType& qtype, vector<DNS
   auto& mc = getMap(val.qname);
 
   {
-    TryWriteLock l(&mc.d_mut);
-    if(!l.gotIt()) {
+    auto map = mc.d_map.try_write_lock();
+    if (!map.owns_lock()) {
       S.inc("deferred-cache-inserts"); 
       return;
     }
 
     bool inserted;
     cmap_t::iterator place;
-    tie(place, inserted) = mc.d_map.insert(val);
+    tie(place, inserted) = map->insert(val);
 
     if (!inserted) {
-      mc.d_map.replace(place, std::move(val));
-      moveCacheItemToBack<SequencedTag>(mc.d_map, place);
+      map->replace(place, std::move(val));
+      moveCacheItemToBack<SequencedTag>(*map, place);
     }
     else {
       if (*d_statnumentries >= d_maxEntries) {
         /* remove the least recently inserted or replaced entry */
-        auto& sidx = mc.d_map.get<SequencedTag>();
+        auto& sidx = map->get<SequencedTag>();
         sidx.pop_front();
       }
       else {
@@ -130,7 +116,7 @@ void AuthQueryCache::insert(const DNSName &qname, const QType& qtype, vector<DNS
   }
 }
 
-bool AuthQueryCache::getEntryLocked(cmap_t& map, const DNSName &qname, uint16_t qtype, vector<DNSZoneRecord>& value, int zoneID, time_t now)
+bool AuthQueryCache::getEntryLocked(const cmap_t& map, const DNSName &qname, uint16_t qtype, vector<DNSZoneRecord>& value, int zoneID, time_t now)
 {
   auto& idx = boost::multi_index::get<HashTag>(map);
   auto iter = idx.find(tie(qname, qtype, zoneID));
@@ -155,9 +141,9 @@ map<char,uint64_t> AuthQueryCache::getCounts()
   uint64_t queryCacheEntries=0, negQueryCacheEntries=0;
 
   for(auto& mc : d_maps) {
-    ReadLock l(&mc.d_mut);
+    auto map = mc.d_map.read_lock();
     
-    for(const auto & iter : mc.d_map) {
+    for(const auto & iter : *map) {
       if(iter.drs.empty())
         negQueryCacheEntries++;
       else
diff --git a/pdns/auth-querycache.hh b/pdns/auth-querycache.hh
index 33bd8a7cae..b5ee72db12 100644
--- a/pdns/auth-querycache.hh
+++ b/pdns/auth-querycache.hh
@@ -37,7 +37,6 @@ class AuthQueryCache : public boost::noncopyable
 {
 public:
   AuthQueryCache(size_t mapsCount=1024);
-  ~AuthQueryCache();
 
   void insert(const DNSName &qname, const QType& qtype, vector<DNSZoneRecord>&& content, uint32_t ttl, int zoneID);
 
@@ -99,8 +98,7 @@ private:
 
     void reserve(size_t numberOfEntries);
 
-    ReadWriteLock d_mut;
-    cmap_t d_map;
+    SharedLockGuarded<cmap_t> d_map;
   };
 
   vector<MapCombo> d_maps;
@@ -109,7 +107,7 @@ private:
     return d_maps[qname.hash() % d_maps.size()];
   }
 
-  bool getEntryLocked(cmap_t& map, const DNSName &content, uint16_t qtype, vector<DNSZoneRecord>& entry, int zoneID, time_t now);
+  bool getEntryLocked(const cmap_t& map, const DNSName &content, uint16_t qtype, vector<DNSZoneRecord>& entry, int zoneID, time_t now);
   void cleanupIfNeeded();
 
   AtomicCounter d_ops{0};
diff --git a/pdns/auth-zonecache.cc b/pdns/auth-zonecache.cc
index bdac5ed81a..687879d0c7 100644
--- a/pdns/auth-zonecache.cc
+++ b/pdns/auth-zonecache.cc
@@ -42,27 +42,14 @@ AuthZoneCache::AuthZoneCache(size_t mapsCount) :
   d_statnumentries = S.getPointer("zone-cache-size");
 }
 
-AuthZoneCache::~AuthZoneCache()
-{
-  try {
-    vector<WriteLock> locks;
-    for (auto& mc : d_maps) {
-      locks.push_back(WriteLock(mc.d_mut));
-    }
-    locks.clear();
-  }
-  catch (...) {
-  }
-}
-
 bool AuthZoneCache::getEntry(const DNSName& zone, int& zoneId)
 {
   auto& mc = getMap(zone);
   bool found = false;
   {
-    ReadLock rl(mc.d_mut);
-    auto iter = mc.d_map.find(zone);
-    if (iter != mc.d_map.end()) {
+    auto map = mc.d_map.read_lock();
+    auto iter = map->find(zone);
+    if (iter != map->end()) {
       found = true;
       zoneId = iter->second.zoneId;
     }
@@ -93,7 +80,7 @@ void AuthZoneCache::replace(const vector<tuple<DNSName, int>>& zone_indices)
     return;
 
   size_t count = zone_indices.size();
-  vector<MapCombo> newMaps(d_maps.size());
+  vector<cmap_t> newMaps(d_maps.size());
 
   // build new maps
   for (const tuple<DNSName, int>& tup : zone_indices) {
@@ -101,36 +88,36 @@ void AuthZoneCache::replace(const vector<tuple<DNSName, int>>& zone_indices)
     CacheValue val;
     val.zoneId = tup.get<1>();
     auto& mc = newMaps[getMapIndex(zone)];
-    auto iter = mc.d_map.find(zone);
-    if (iter != mc.d_map.end()) {
+    auto iter = mc.find(zone);
+    if (iter != mc.end()) {
       iter->second = std::move(val);
     }
     else {
-      mc.d_map.emplace(zone, val);
+      mc.emplace(zone, val);
     }
   }
 
   {
-    WriteLock globalLock(d_mut);
-    if (d_replacePending) {
+    auto pending = d_pending.lock();
+    if (pending->d_replacePending) {
       // add/replace all zones created while data collection for replace() was already in progress.
-      for (const tuple<DNSName, int>& tup : d_pendingAdds) {
+      for (const tuple<DNSName, int>& tup : pending->d_pendingAdds) {
         const DNSName& zone = tup.get<0>();
         CacheValue val;
         val.zoneId = tup.get<1>();
         auto& mc = newMaps[getMapIndex(zone)];
-        mc.d_map[zone] = val;
+        mc[zone] = val;
       }
     }
 
     for (size_t mapIndex = 0; mapIndex < d_maps.size(); mapIndex++) {
       auto& mc = d_maps[mapIndex];
-      WriteLock mcLock(mc.d_mut);
-      mc.d_map = std::move(newMaps[mapIndex].d_map);
+      auto map = mc.d_map.write_lock();
+      *map = std::move(newMaps[mapIndex]);
     }
 
-    d_pendingAdds.clear();
-    d_replacePending = false;
+    pending->d_pendingAdds.clear();
+    pending->d_replacePending = false;
   }
 
   d_statnumentries->store(count);
@@ -142,9 +129,9 @@ void AuthZoneCache::add(const DNSName& zone, const int zoneId)
     return;
 
   {
-    WriteLock globalLock(d_mut);
-    if (d_replacePending) {
-      d_pendingAdds.push_back({zone, zoneId});
+    auto pending = d_pending.lock();
+    if (pending->d_replacePending) {
+      pending->d_pendingAdds.push_back({zone, zoneId});
     }
   }
 
@@ -154,13 +141,13 @@ void AuthZoneCache::add(const DNSName& zone, const int zoneId)
   int mapIndex = getMapIndex(zone);
   {
     auto& mc = d_maps[mapIndex];
-    WriteLock mcLock(mc.d_mut);
-    auto iter = mc.d_map.find(zone);
-    if (iter != mc.d_map.end()) {
+    auto map = mc.d_map.write_lock();
+    auto iter = map->find(zone);
+    if (iter != map->end()) {
       iter->second = std::move(val);
     }
     else {
-      mc.d_map.emplace(zone, val);
+      map->emplace(zone, val);
     }
   }
 }
@@ -171,8 +158,8 @@ void AuthZoneCache::setReplacePending()
     return;
 
   {
-    WriteLock globalLock(d_mut);
-    d_replacePending = true;
-    d_pendingAdds.clear();
+    auto pending = d_pending.lock();
+    pending->d_replacePending = true;
+    pending->d_pendingAdds.clear();
   }
 }
diff --git a/pdns/auth-zonecache.hh b/pdns/auth-zonecache.hh
index e5ace8b265..e3968e1f89 100644
--- a/pdns/auth-zonecache.hh
+++ b/pdns/auth-zonecache.hh
@@ -31,7 +31,6 @@ class AuthZoneCache : public boost::noncopyable
 {
 public:
   AuthZoneCache(size_t mapsCount = 1024);
-  ~AuthZoneCache();
 
   void replace(const vector<tuple<DNSName, int>>& zone);
   void add(const DNSName& zone, const int zoneId);
@@ -70,8 +69,7 @@ private:
     MapCombo(const MapCombo&) = delete;
     MapCombo& operator=(const MapCombo&) = delete;
 
-    ReadWriteLock d_mut;
-    cmap_t d_map;
+    SharedLockGuarded<cmap_t> d_map;
   };
 
   vector<MapCombo> d_maps;
@@ -90,9 +88,12 @@ private:
 
   time_t d_refreshinterval{0};
 
-  ReadWriteLock d_mut;
-  std::vector<tuple<DNSName, int>> d_pendingAdds;
-  bool d_replacePending{false};
+  struct PendingData
+  {
+    std::vector<tuple<DNSName, int>> d_pendingAdds;
+    bool d_replacePending{false};
+  };
+  LockGuarded<PendingData> d_pending;
 };
 
 extern AuthZoneCache g_zoneCache;
diff --git a/pdns/cachecleaner.hh b/pdns/cachecleaner.hh
index 55ddd48fc1..75340d388e 100644
--- a/pdns/cachecleaner.hh
+++ b/pdns/cachecleaner.hh
@@ -102,12 +102,12 @@ template <typename S, typename T> uint64_t pruneLockedCollectionsVector(std::vec
   time_t now = time(nullptr);
 
   for(auto& mc : maps) {
-    WriteLock wl(&mc.d_mut);
+    auto map = mc.d_map.write_lock();
 
-    uint64_t lookAt = (mc.d_map.size() + 9) / 10; // Look at 10% of this shard
+    uint64_t lookAt = (map->size() + 9) / 10; // Look at 10% of this shard
     uint64_t erased = 0;
 
-    auto& sidx = boost::multi_index::get<S>(mc.d_map);
+    auto& sidx = boost::multi_index::get<S>(*map);
     for(auto i = sidx.begin(); i != sidx.end() && lookAt > 0; lookAt--) {
       if(i->ttd < now) {
         i = sidx.erase(i);
@@ -203,9 +203,9 @@ template <typename T> uint64_t purgeLockedCollectionsVector(std::vector<T>& maps
   uint64_t delcount=0;
 
   for(auto& mc : maps) {
-    WriteLock wl(&mc.d_mut);
-    delcount += mc.d_map.size();
-    mc.d_map.clear();
+    auto map = mc.d_map.write_lock();
+    delcount += map->size();
+    map->clear();
   }
 
   return delcount;
@@ -218,8 +218,8 @@ template <typename N, typename T> uint64_t purgeLockedCollectionsVector(std::vec
   prefix.resize(prefix.size()-1);
   DNSName dprefix(prefix);
   for(auto& mc : maps) {
-    WriteLock wl(&mc.d_mut);
-    auto& idx = boost::multi_index::get<N>(mc.d_map);
+    auto map = mc.d_map.write_lock();
+    auto& idx = boost::multi_index::get<N>(*map);
     auto iter = idx.lower_bound(dprefix);
     auto start = iter;
 
@@ -238,8 +238,8 @@ template <typename N, typename T> uint64_t purgeLockedCollectionsVector(std::vec
 template <typename N, typename T> uint64_t purgeExactLockedCollection(T& mc, const DNSName& qname)
 {
   uint64_t delcount=0;
-  WriteLock wl(&mc.d_mut);
-  auto& idx = boost::multi_index::get<N>(mc.d_map);
+  auto map = mc.d_map.write_lock();
+  auto& idx = boost::multi_index::get<N>(*map);
   auto range = idx.equal_range(qname);
   if(range.first != range.second) {
     delcount += distance(range.first, range.second);
diff --git a/pdns/lock.hh b/pdns/lock.hh
index 1b60d46a5e..70fc9e764a 100644
--- a/pdns/lock.hh
+++ b/pdns/lock.hh
@@ -290,6 +290,11 @@ public:
     return LockGuardedHolder<T>(d_value, d_mutex);
   }
 
+  LockGuardedHolder<const T> read_only_lock() const
+  {
+    return LockGuardedHolder<const T>(d_value, d_mutex);
+  }
+
 private:
   std::mutex d_mutex;
   T d_value;
diff --git a/pdns/statbag.cc b/pdns/statbag.cc
index bbacc27b4d..4e9f70bd46 100644
--- a/pdns/statbag.cc
+++ b/pdns/statbag.cc
@@ -217,7 +217,7 @@ template<typename T, typename Comp>
 vector<pair<T, unsigned int> >StatRing<T,Comp>::get() const
 {
   map<T,unsigned int, Comp> res;
-  for(typename boost::circular_buffer<T>::const_iterator i=d_items.begin();i!=d_items.end();++i) {
+  for (typename boost::circular_buffer<T>::const_iterator i = d_items.begin(); i != d_items.end(); ++i) {
     res[*i]++;
   }
   
diff --git a/pdns/statbag.hh b/pdns/statbag.hh
index e0aa3c04e6..acdc383230 100644
--- a/pdns/statbag.hh
+++ b/pdns/statbag.hh
@@ -20,7 +20,6 @@
  * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
  */
 #pragma once
-#include <pthread.h>
 #include <map>
 #include <functional>
 #include <string>