From: Remi Gacogne <remi.gacogne@powerdns.com>
Date: Mon, 19 Mar 2018 14:46:22 +0000 (+0100)
Subject: dnsdist: Use mutexes instead of read-write locks for the rings
X-Git-Tag: dnsdist-1.3.0~1^2~11
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=632e5fd051840adc83e26bb2c8655afe1c18c487;p=thirdparty%2Fpdns.git

dnsdist: Use mutexes instead of read-write locks for the rings
---

diff --git a/pdns/dnsdist-lua-inspection.cc b/pdns/dnsdist-lua-inspection.cc
index 3cf82e460f..74aeff3218 100644
--- a/pdns/dnsdist-lua-inspection.cc
+++ b/pdns/dnsdist-lua-inspection.cc
@@ -32,9 +32,9 @@ static std::unordered_map<unsigned int, vector<boost::variant<string,double>>> g
   unsigned int total=0;
   {
     for (size_t idx = 0; idx < g_rings.getNumberOfShards(); idx++) {
-      ReadLock rl(&g_rings.d_shards[idx].respLock);
+      std::lock_guard<std::mutex> rl(g_rings.d_shards[idx]->respLock);
       if(!labels) {
-        for(const auto& a : g_rings.d_shards[idx].respRing) {
+        for(const auto& a : g_rings.d_shards[idx]->respRing) {
           if(!pred(a))
             continue;
           counts[a.name]++;
@@ -43,7 +43,7 @@ static std::unordered_map<unsigned int, vector<boost::variant<string,double>>> g
       }
       else {
         unsigned int lab = *labels;
-        for(auto a : g_rings.d_shards[idx].respRing) {
+        for(auto a : g_rings.d_shards[idx]->respRing) {
           if(!pred(a))
             continue;
 
@@ -106,9 +106,9 @@ static void statNodeRespRing(statvisitor_t visitor, unsigned int seconds)
 
   StatNode root;
   for (size_t idx = 0; idx < g_rings.getNumberOfShards(); idx++) {
-    ReadLock rl(&g_rings.d_shards[idx].respLock);
+    std::lock_guard<std::mutex> rl(g_rings.d_shards[idx]->respLock);
 
-    for(const auto& c : g_rings.d_shards[idx].respRing) {
+    for(const auto& c : g_rings.d_shards[idx]->respRing) {
       if (now < c.when)
         continue;
 
@@ -130,11 +130,11 @@ static vector<pair<unsigned int, std::unordered_map<string,string> > > getRespRi
   vector<pair<unsigned int, entry_t > > ret;
 
   for (size_t idx = 0; idx < g_rings.getNumberOfShards(); idx++) {
-    ReadLock rl(&g_rings.d_shards[idx].respLock);
+    std::lock_guard<std::mutex> rl(g_rings.d_shards[idx]->respLock);
 
     entry_t e;
     unsigned int count=1;
-    for(const auto& c : g_rings.d_shards[idx].respRing) {
+    for(const auto& c : g_rings.d_shards[idx]->respRing) {
       if(rcode && (rcode.get() != c.dh.rcode))
         continue;
       e["qname"]=c.name.toString();
@@ -157,15 +157,15 @@ static counts_t exceedRespGen(unsigned int rate, int seconds, std::function<void
 
   size_t total = 0;
   for (size_t idx = 0; idx < g_rings.getNumberOfShards(); idx++) {
-    ReadLock rl(&g_rings.d_shards[idx].respLock);
-    total += g_rings.d_shards[idx].respRing.size();
+    std::lock_guard<std::mutex> rl(g_rings.d_shards[idx]->respLock);
+    total += g_rings.d_shards[idx]->respRing.size();
   }
 
   counts.reserve(total);
 
   for (size_t idx = 0; idx < g_rings.getNumberOfShards(); idx++) {
-    ReadLock rl(&g_rings.d_shards[idx].respLock);
-    for(const auto& c : g_rings.d_shards[idx].respRing) {
+    std::lock_guard<std::mutex> rl(g_rings.d_shards[idx]->respLock);
+    for(const auto& c : g_rings.d_shards[idx]->respRing) {
 
       if(seconds && c.when < cutoff)
         continue;
@@ -192,15 +192,15 @@ static counts_t exceedQueryGen(unsigned int rate, int seconds, std::function<voi
 
   size_t total = 0;
   for (size_t idx = 0; idx < g_rings.getNumberOfShards(); idx++) {
-    ReadLock rl(&g_rings.d_shards[idx].queryLock);
-    total += g_rings.d_shards[idx].queryRing.size();
+    std::lock_guard<std::mutex> rl(g_rings.d_shards[idx]->queryLock);
+    total += g_rings.d_shards[idx]->queryRing.size();
   }
 
   counts.reserve(total);
 
   for (size_t idx = 0; idx < g_rings.getNumberOfShards(); idx++) {
-    ReadLock rl(&g_rings.d_shards[idx].queryLock);
-    for(const auto& c : g_rings.d_shards[idx].queryRing) {
+    std::lock_guard<std::mutex> rl(g_rings.d_shards[idx]->queryLock);
+    for(const auto& c : g_rings.d_shards[idx]->queryRing) {
       if(seconds && c.when < cutoff)
         continue;
       if(now < c.when)
@@ -242,8 +242,8 @@ void setupLuaInspection()
       unsigned int total=0;
       {
         for (size_t idx = 0; idx < g_rings.getNumberOfShards(); idx++) {
-          ReadLock rl(&g_rings.d_shards[idx].queryLock);
-          for(const auto& c : g_rings.d_shards[idx].queryRing) {
+          std::lock_guard<std::mutex> rl(g_rings.d_shards[idx]->queryLock);
+          for(const auto& c : g_rings.d_shards[idx]->queryRing) {
             counts[c.requestor]++;
             total++;
           }
@@ -275,8 +275,8 @@ void setupLuaInspection()
       unsigned int total=0;
       if(!labels) {
         for (size_t idx = 0; idx < g_rings.getNumberOfShards(); idx++) {
-          ReadLock rl(&g_rings.d_shards[idx].queryLock);
-          for(const auto& a : g_rings.d_shards[idx].queryRing) {
+          std::lock_guard<std::mutex> rl(g_rings.d_shards[idx]->queryLock);
+          for(const auto& a : g_rings.d_shards[idx]->queryRing) {
             counts[a.name]++;
             total++;
           }
@@ -285,8 +285,8 @@ void setupLuaInspection()
       else {
 	unsigned int lab = *labels;
         for (size_t idx = 0; idx < g_rings.getNumberOfShards(); idx++) {
-          ReadLock rl(&g_rings.d_shards[idx].queryLock);
-          for(auto a : g_rings.d_shards[idx].queryRing) {
+          std::lock_guard<std::mutex> rl(g_rings.d_shards[idx]->queryLock);
+          for(auto a : g_rings.d_shards[idx]->queryRing) {
             a.name.trimToLabels(lab);
             counts[a.name]++;
             total++;
@@ -326,8 +326,8 @@ void setupLuaInspection()
       rings.reserve(g_rings.getNumberOfShards());
       for (size_t idx = 0; idx < g_rings.getNumberOfShards(); idx++) {
         {
-          ReadLock rl(&g_rings.d_shards[idx].respLock);
-          rings[idx] = g_rings.d_shards[idx].respRing;
+          std::lock_guard<std::mutex> rl(g_rings.d_shards[idx]->respLock);
+          rings[idx] = g_rings.d_shards[idx]->respRing;
         }
         totalEntries += rings[idx].size();
       }
@@ -420,16 +420,16 @@ void setupLuaInspection()
       std::vector<Rings::Response> rr;
       for (size_t idx = 0; idx < g_rings.getNumberOfShards(); idx++) {
         {
-          ReadLock rl(&g_rings.d_shards[idx].queryLock);
-          qr.resize(qr.size() + g_rings.d_shards[idx].queryRing.size());
-          for (const auto& entry : g_rings.d_shards[idx].queryRing) {
+          std::lock_guard<std::mutex> rl(g_rings.d_shards[idx]->queryLock);
+          qr.resize(qr.size() + g_rings.d_shards[idx]->queryRing.size());
+          for (const auto& entry : g_rings.d_shards[idx]->queryRing) {
             qr.push_back(entry);
           }
         }
         {
-          ReadLock rl(&g_rings.d_shards[idx].respLock);
-          rr.resize(rr.size() + g_rings.d_shards[idx].respRing.size());
-          for (const auto& entry : g_rings.d_shards[idx].respRing) {
+          std::lock_guard<std::mutex> rl(g_rings.d_shards[idx]->respLock);
+          rr.resize(rr.size() + g_rings.d_shards[idx]->respRing.size());
+          for (const auto& entry : g_rings.d_shards[idx]->respRing) {
             rr.push_back(entry);
           }
         }
@@ -515,8 +515,8 @@ void setupLuaInspection()
       unsigned int size=0;
       {
         for (size_t idx = 0; idx < g_rings.getNumberOfShards(); idx++) {
-          ReadLock rl(&g_rings.d_shards[idx].respLock);
-          for(const auto& r : g_rings.d_shards[idx].respRing) {
+          std::lock_guard<std::mutex> rl(g_rings.d_shards[idx]->respLock);
+          for(const auto& r : g_rings.d_shards[idx]->respRing) {
             /* skip actively discovered timeouts */
             if (r.usec == std::numeric_limits<unsigned int>::max())
               continue;
diff --git a/pdns/dnsdist-rings.cc b/pdns/dnsdist-rings.cc
index 83c9ca5878..d3cc5f43f0 100644
--- a/pdns/dnsdist-rings.cc
+++ b/pdns/dnsdist-rings.cc
@@ -22,12 +22,15 @@
 #include "dnsdist.hh"
 #include "lock.hh"
 
+std::atomic<size_t> Rings::s_queryInserterId;
+std::atomic<size_t> Rings::s_responseInserterId;
+
 size_t Rings::numDistinctRequestors()
 {
   std::set<ComboAddress, ComboAddress::addressOnlyLessThan> s;
   for (size_t idx = 0; idx < getNumberOfShards(); idx++) {
-    ReadLock rl(&d_shards[idx].queryLock);
-    for(const auto& q : d_shards[idx].queryRing) {
+    std::lock_guard<std::mutex> rl(d_shards[idx]->queryLock);
+    for(const auto& q : d_shards[idx]->queryRing) {
       s.insert(q.requestor);
     }
   }
@@ -40,15 +43,15 @@ std::unordered_map<int, vector<boost::variant<string,double>>> Rings::getTopBand
   uint64_t total=0;
   for (size_t idx = 0; idx < getNumberOfShards(); idx++) {
     {
-      ReadLock rl(&d_shards[idx].queryLock);
-      for(const auto& q : d_shards[idx].queryRing) {
+      std::lock_guard<std::mutex> rl(d_shards[idx]->queryLock);
+      for(const auto& q : d_shards[idx]->queryRing) {
         counts[q.requestor]+=q.size;
         total+=q.size;
       }
     }
     {
-      ReadLock rl(&d_shards[idx].respLock);
-      for(const auto& r : d_shards[idx].respRing) {
+      std::lock_guard<std::mutex> rl(d_shards[idx]->respLock);
+      for(const auto& r : d_shards[idx]->respRing) {
         counts[r.requestor]+=r.size;
         total+=r.size;
       }
diff --git a/pdns/dnsdist.hh b/pdns/dnsdist.hh
index 270b18b861..55b9db48da 100644
--- a/pdns/dnsdist.hh
+++ b/pdns/dnsdist.hh
@@ -401,8 +401,8 @@ struct Rings {
   {
     boost::circular_buffer<Query> queryRing;
     boost::circular_buffer<Response> respRing;
-    pthread_rwlock_t queryLock;
-    pthread_rwlock_t respLock;
+    std::mutex queryLock;
+    std::mutex respLock;
   };
 
   Rings(size_t capacity=10000, size_t numberOfShards=1): d_numberOfShards(numberOfShards)
@@ -418,35 +418,32 @@ struct Rings {
     }
 
     d_shards.resize(numberOfShards);
-
-    /* set up the locks for the new shards */
-    for (size_t idx = d_numberOfShards; idx < numberOfShards; idx++) {
-      pthread_rwlock_init(&d_shards[idx].queryLock, 0);
-      pthread_rwlock_init(&d_shards[idx].respLock, 0);
-    }
-
     d_numberOfShards = numberOfShards;
 
     /* resize all the rings */
     for (size_t idx = 0; idx < numberOfShards; idx++) {
+      d_shards[idx] = std::unique_ptr<Shard>(new Shard());
       {
-        WriteLock wl(&d_shards[idx].queryLock);
-        d_shards[idx].queryRing.set_capacity(newCapacity / numberOfShards);
+        std::lock_guard<std::mutex> wl(d_shards[idx]->queryLock);
+        d_shards[idx]->queryRing.set_capacity(newCapacity / numberOfShards);
       }
       {
-        WriteLock wl(&d_shards[idx].respLock);
-        d_shards[idx].respRing.set_capacity(newCapacity / numberOfShards);
+        std::lock_guard<std::mutex> wl(d_shards[idx]->respLock);
+        d_shards[idx]->respRing.set_capacity(newCapacity / numberOfShards);
       }
     }
   }
-  size_t getQueryInserterId()
+
+  static size_t getQueryInserterId()
   {
     return s_queryInserterId++;
   }
-  size_t getResponseInserterId()
+
+  static size_t getResponseInserterId()
   {
     return s_responseInserterId++;
   }
+
   size_t getNumberOfShards() const
   {
     return d_numberOfShards;
@@ -455,18 +452,18 @@ struct Rings {
   void insertQuery(const struct timespec& when, const ComboAddress& requestor, const DNSName& name, uint16_t qtype, uint16_t size, const struct dnsheader& dh, size_t queryInserterId)
   {
     auto shardId = getShardId(queryInserterId);
-    WriteLock wl(&d_shards[shardId].queryLock);
-    d_shards[shardId].queryRing.push_back({when, requestor, name, size, qtype, dh});
+    std::lock_guard<std::mutex> wl(d_shards[shardId]->queryLock);
+    d_shards[shardId]->queryRing.push_back({when, requestor, name, size, qtype, dh});
   }
 
   void insertResponse(const struct timespec& when, const ComboAddress& requestor, const DNSName& name, uint16_t qtype, unsigned int usec, unsigned int size, const struct dnsheader& dh, const ComboAddress& backend, size_t responseInserterId)
   {
     auto shardId = getShardId(responseInserterId);
-    WriteLock wl(&d_shards[shardId].respLock);
-    d_shards[shardId].respRing.push_back({when, requestor, name, qtype, usec, size, dh, backend});
+    std::lock_guard<std::mutex> wl(d_shards[shardId]->respLock);
+    d_shards[shardId]->respRing.push_back({when, requestor, name, qtype, usec, size, dh, backend});
   }
 
-  std::vector<Shard> d_shards;
+  std::vector<std::unique_ptr<Shard> > d_shards;
 
 private:
   size_t getShardId(size_t id) const
@@ -474,8 +471,9 @@ private:
     return (id % d_numberOfShards);
   }
 
-  std::atomic<size_t> s_queryInserterId{0};
-  std::atomic<size_t> s_responseInserterId{0};
+  static std::atomic<size_t> s_queryInserterId;
+  static std::atomic<size_t> s_responseInserterId;
+
   size_t d_numberOfShards;
 };