From: Remi Gacogne <remi.gacogne@powerdns.com>
Date: Thu, 24 Jul 2025 14:02:39 +0000 (+0200)
Subject: dnsdist: Update a cache's atomic counter without holding the lock
X-Git-Tag: auth-5.1.0-alpha0~19^2
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=refs%2Fpull%2F15918%2Fhead;p=thirdparty%2Fpdns.git

dnsdist: Update a cache's atomic counter without holding the lock

Signed-off-by: Remi Gacogne <remi.gacogne@powerdns.com>
---

diff --git a/pdns/dnsdistdist/dnsdist-cache.cc b/pdns/dnsdistdist/dnsdist-cache.cc
index 0a861165c..afc1575ba 100644
--- a/pdns/dnsdistdist/dnsdist-cache.cc
+++ b/pdns/dnsdistdist/dnsdist-cache.cc
@@ -92,11 +92,11 @@ bool DNSDistPacketCache::cachedValueMatches(const CacheValue& cachedValue, uint1
   return true;
 }
 
-void DNSDistPacketCache::insertLocked(CacheShard& shard, std::unordered_map<uint32_t, CacheValue>& map, uint32_t key, CacheValue& newValue)
+bool DNSDistPacketCache::insertLocked(std::unordered_map<uint32_t, CacheValue>& map, uint32_t key, CacheValue& newValue)
 {
   /* check again now that we hold the lock to prevent a race */
   if (map.size() >= (d_settings.d_maxEntries / d_settings.d_shardCount)) {
-    return;
+    return false;
   }
 
   std::unordered_map<uint32_t, CacheValue>::iterator mapIt;
@@ -104,8 +104,7 @@ void DNSDistPacketCache::insertLocked(CacheShard& shard, std::unordered_map<uint
   std::tie(mapIt, result) = map.insert({key, newValue});
 
   if (result) {
-    ++shard.d_entriesCount;
-    return;
+    return true;
   }
 
   /* in case of collision, don't override the existing entry
@@ -115,15 +114,16 @@ void DNSDistPacketCache::insertLocked(CacheShard& shard, std::unordered_map<uint
 
   if (!wasExpired && !cachedValueMatches(value, newValue.queryFlags, newValue.qname, newValue.qtype, newValue.qclass, newValue.receivedOverUDP, newValue.dnssecOK, newValue.subnet)) {
     ++d_insertCollisions;
-    return;
+    return false;
   }
 
   /* if the existing entry had a longer TTD, keep it */
   if (newValue.validity <= value.validity) {
-    return;
+    return false;
   }
 
   value = newValue;
+  return false;
 }
 
 void DNSDistPacketCache::insert(uint32_t key, const boost::optional<Netmask>& subnet, uint16_t queryFlags, bool dnssecOK, const DNSName& qname, uint16_t qtype, uint16_t qclass, const PacketBuffer& response, bool receivedOverUDP, uint8_t rcode, boost::optional<uint32_t> tempFailureTTL)
@@ -199,6 +199,7 @@ void DNSDistPacketCache::insert(uint32_t key, const boost::optional<Netmask>& su
 
   auto& shard = d_shards.at(shardIndex);
 
+  bool inserted = false;
   if (d_settings.d_deferrableInsertLock) {
     auto lock = shard.d_map.try_write_lock();
 
@@ -206,12 +207,15 @@ void DNSDistPacketCache::insert(uint32_t key, const boost::optional<Netmask>& su
       ++d_deferredInserts;
       return;
     }
-    insertLocked(shard, *lock, key, newValue);
+    inserted = insertLocked(*lock, key, newValue);
   }
   else {
     auto lock = shard.d_map.write_lock();
 
-    insertLocked(shard, *lock, key, newValue);
+    inserted = insertLocked(*lock, key, newValue);
+  }
+  if (inserted) {
+    ++shard.d_entriesCount;
   }
 }
 
diff --git a/pdns/dnsdistdist/dnsdist-cache.hh b/pdns/dnsdistdist/dnsdist-cache.hh
index d48397e8f..64e19883c 100644
--- a/pdns/dnsdistdist/dnsdist-cache.hh
+++ b/pdns/dnsdistdist/dnsdist-cache.hh
@@ -133,7 +133,7 @@ private:
 
   bool cachedValueMatches(const CacheValue& cachedValue, uint16_t queryFlags, const DNSName& qname, uint16_t qtype, uint16_t qclass, bool receivedOverUDP, bool dnssecOK, const boost::optional<Netmask>& subnet) const;
   uint32_t getShardIndex(uint32_t key) const;
-  void insertLocked(CacheShard& shard, std::unordered_map<uint32_t, CacheValue>& map, uint32_t key, CacheValue& newValue);
+  bool insertLocked(std::unordered_map<uint32_t, CacheValue>& map, uint32_t key, CacheValue& newValue);
 
   std::vector<CacheShard> d_shards;