From: Otto Moerbeek <otto.moerbeek@open-xchange.com>
Date: Tue, 13 Apr 2021 14:35:45 +0000 (+0200)
Subject: Fix TSAN complaints: max stacksize and response stats size counters.
X-Git-Tag: dnsdist-1.6.0~3^2~4
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=097b1e68dfe149694026a60332fe49bf5a331ee6;p=thirdparty%2Fpdns.git

Fix TSAN complaints: max stacksize and response stats size counters.

Start using new generic histogram code and while there, fix a bug
concerning the rcode stats (which was not passed in by the caller).
---

diff --git a/.not-formatted b/.not-formatted
index 452a903305..9778db3d7c 100644
--- a/.not-formatted
+++ b/.not-formatted
@@ -275,8 +275,6 @@
 ./pdns/resolver.cc
 ./pdns/resolver.hh
 ./pdns/responsestats-auth.cc
-./pdns/responsestats.cc
-./pdns/responsestats.hh
 ./pdns/rfc2136handler.cc
 ./pdns/root-addresses.hh
 ./pdns/root-dnssec.hh
diff --git a/pdns/histogram.hh b/pdns/histogram.hh
index 5abb3c39d5..a315b8813f 100644
--- a/pdns/histogram.hh
+++ b/pdns/histogram.hh
@@ -23,6 +23,7 @@
 
 #include <algorithm>
 #include <atomic>
+#include <string>
 #include <vector>
 
 namespace pdns {
diff --git a/pdns/mtasker.cc b/pdns/mtasker.cc
index a2c3a2fba2..96bf9e0125 100644
--- a/pdns/mtasker.cc
+++ b/pdns/mtasker.cc
@@ -399,7 +399,7 @@ template<class Key, class Val>int MTasker<Key,Val>::getTid() const
 }
 
 //! Returns the maximum stack usage so far of this MThread
-template<class Key, class Val>unsigned int MTasker<Key,Val>::getMaxStackUsage()
+template<class Key, class Val>uint64_t MTasker<Key,Val>::getMaxStackUsage()
 {
   return d_threads[d_tid].startOfStack - d_threads[d_tid].highestStackSeen;
 }
diff --git a/pdns/mtasker.hh b/pdns/mtasker.hh
index 607ca57745..fa1ede4127 100644
--- a/pdns/mtasker.hh
+++ b/pdns/mtasker.hh
@@ -136,7 +136,7 @@ public:
   bool noProcesses() const;
   unsigned int numProcesses() const;
   int getTid() const;
-  unsigned int getMaxStackUsage();
+  uint64_t getMaxStackUsage();
   unsigned int getUsec();
 
 private:
diff --git a/pdns/pdns_recursor.cc b/pdns/pdns_recursor.cc
index 5ff39d9fc8..d33d3a6c25 100644
--- a/pdns/pdns_recursor.cc
+++ b/pdns/pdns_recursor.cc
@@ -2096,7 +2096,7 @@ static void startDoResolve(void *p)
       pw.commit();
     }
 
-    g_rs.submitResponse(dc->d_mdp.d_qtype, packet.size(), !dc->d_tcp);
+    g_rs.submitResponse(dc->d_mdp.d_qtype, packet.size(), pw.getHeader()->rcode, !dc->d_tcp);
     updateResponseStats(res, dc->d_source, packet.size(), &dc->d_mdp.d_qname, dc->d_mdp.d_qtype);
 #ifdef NOD_ENABLED
     bool nod = false;
@@ -2272,7 +2272,7 @@ static void startDoResolve(void *p)
 
   runTaskOnce(g_logCommonErrors);
 
-  g_stats.maxMThreadStackUsage = max(MT->getMaxStackUsage(), g_stats.maxMThreadStackUsage);
+  g_stats.maxMThreadStackUsage = max(MT->getMaxStackUsage(), g_stats.maxMThreadStackUsage.load());
 }
 
 static void makeControlChannelSocket(int processNum=-1)
diff --git a/pdns/rec_channel_rec.cc b/pdns/rec_channel_rec.cc
index 08ca90b8a9..dca663d995 100644
--- a/pdns/rec_channel_rec.cc
+++ b/pdns/rec_channel_rec.cc
@@ -1140,7 +1140,6 @@ static void registerAllStats1()
   addGetStat("over-capacity-drops", &g_stats.overCapacityDrops);
   addGetStat("policy-drops", &g_stats.policyDrops);
   addGetStat("no-packet-error", &g_stats.noPacketError);
-  addGetStat("dlg-only-drops", &SyncRes::s_nodelegated);
   addGetStat("ignored-packets", &g_stats.ignoredCount);
   addGetStat("empty-queries", &g_stats.emptyQueriesCount);
   addGetStat("max-mthread-stack", &g_stats.maxMThreadStackUsage);
diff --git a/pdns/responsestats.cc b/pdns/responsestats.cc
index fefa35c69e..bd5e34ca91 100644
--- a/pdns/responsestats.cc
+++ b/pdns/responsestats.cc
@@ -8,54 +8,56 @@
 
 #include "dnsparser.hh"
 
-ResponseStats::ResponseStats() :   d_qtypecounters(new std::atomic<unsigned long>[65536]), d_rcodecounters(new std::atomic<unsigned long>[256])
+static auto sizeBounds()
 {
-  d_sizecounters.push_back(make_pair(20,0));
-  d_sizecounters.push_back(make_pair(40,0));
-  d_sizecounters.push_back(make_pair(60,0));
-  d_sizecounters.push_back(make_pair(80,0));
-  d_sizecounters.push_back(make_pair(100,0));
-  d_sizecounters.push_back(make_pair(150,0));
-  for(int n=200; n < 65000 ; n+=200)
-    d_sizecounters.push_back(make_pair(n,0));
-  d_sizecounters.push_back(make_pair(std::numeric_limits<uint16_t>::max(),0));
-  for(unsigned int n =0 ; n < 65535; ++n)
+  std::vector<uint64_t> bounds;
+
+  bounds.push_back(20);
+  bounds.push_back(40);
+  bounds.push_back(60);
+  bounds.push_back(80);
+  bounds.push_back(100);
+  bounds.push_back(150);
+  for (uint64_t n = 200; n < 65000; n += 200) {
+    bounds.push_back(n);
+  }
+  return bounds;
+}
+
+ResponseStats::ResponseStats() :
+  d_sizecounters("SizeCounters", sizeBounds())
+{
+  for (unsigned int n = 0; n < 65535; ++n) {
     d_qtypecounters[n] = 0;
-  for(unsigned int n =0 ; n < 256; ++n)
+  }
+  for (unsigned int n = 0; n < 256; ++n) {
     d_rcodecounters[n] = 0;
+  }
 }
 
 ResponseStats g_rs;
 
-static bool pcomp(const pair<uint16_t, uint64_t>&a , const pair<uint16_t, uint64_t>&b)
-{
-  return a.first < b.first;
-}
-
-void ResponseStats::submitResponse(uint16_t qtype,uint16_t respsize, uint8_t rcode, bool udpOrTCP)
+void ResponseStats::submitResponse(uint16_t qtype, uint16_t respsize, uint8_t rcode, bool udpOrTCP)
 {
-    d_rcodecounters[rcode]++;
-    submitResponse(qtype, respsize, udpOrTCP);
+  d_rcodecounters[rcode]++;
+  submitResponse(qtype, respsize, udpOrTCP);
 }
 
-void ResponseStats::submitResponse(uint16_t qtype,uint16_t respsize, bool udpOrTCP)
+void ResponseStats::submitResponse(uint16_t qtype, uint16_t respsize, bool udpOrTCP)
 {
   d_qtypecounters[qtype]++;
-  pair<uint16_t, uint64_t> s(respsize, 0);
-  sizecounters_t::iterator iter = std::upper_bound(d_sizecounters.begin(), d_sizecounters.end(), s, pcomp);
-  if(iter!= d_sizecounters.begin())
-    --iter;
-  iter->second++;
+  d_sizecounters(respsize);
 }
 
 map<uint16_t, uint64_t> ResponseStats::getQTypeResponseCounts()
 {
   map<uint16_t, uint64_t> ret;
   uint64_t count;
-  for(unsigned int i = 0 ; i < 65535 ; ++i) {
-    count= d_qtypecounters[i];
-    if(count)
-      ret[i]=count;
+  for (unsigned int i = 0; i < 65535; ++i) {
+    count = d_qtypecounters[i];
+    if (count) {
+      ret[i] = count;
+    }
   }
   return ret;
 }
@@ -63,8 +65,10 @@ map<uint16_t, uint64_t> ResponseStats::getQTypeResponseCounts()
 map<uint16_t, uint64_t> ResponseStats::getSizeResponseCounts()
 {
   map<uint16_t, uint64_t> ret;
-  for(const auto & sizecounter : d_sizecounters) {
-    ret[sizecounter.first]=sizecounter.second;
+  for (const auto& sizecounter : d_sizecounters.getRawData()) {
+    if (sizecounter.d_count) {
+      ret[sizecounter.d_boundary] = sizecounter.d_count;
+    }
   }
   return ret;
 }
@@ -73,22 +77,22 @@ map<uint8_t, uint64_t> ResponseStats::getRCodeResponseCounts()
 {
   map<uint8_t, uint64_t> ret;
   uint64_t count;
-  for(unsigned int i = 0 ; i < 256 ; ++i) {
-    count= d_rcodecounters[i];
-    if(count)
-      ret[i]=count;
+  for (unsigned int i = 0; i < 256; ++i) {
+    count = d_rcodecounters[i];
+    if (count) {
+      ret[i] = count;
+    }
   }
   return ret;
 }
 
 string ResponseStats::getQTypeReport()
 {
-  typedef map<uint16_t, uint64_t> qtypenums_t;
-  qtypenums_t qtypenums = getQTypeResponseCounts();
+  auto qtypenums = getQTypeResponseCounts();
   ostringstream os;
   boost::format fmt("%s\t%d\n");
-  for(const qtypenums_t::value_type& val :  qtypenums) {
-    os << (fmt %DNSRecordContent::NumberToType( val.first) % val.second).str();
+  for (const auto& val : qtypenums) {
+    os << (fmt % DNSRecordContent::NumberToType(val.first) % val.second).str();
   }
   return os.str();
 }
diff --git a/pdns/responsestats.hh b/pdns/responsestats.hh
index d068a1c3d5..259a545c94 100644
--- a/pdns/responsestats.hh
+++ b/pdns/responsestats.hh
@@ -21,7 +21,8 @@
  */
 #pragma once
 
-#include <boost/scoped_array.hpp>
+#include <array>
+#include "histogram.hh"
 
 #include "dnspacket.hh"
 
@@ -30,7 +31,7 @@ class ResponseStats
 public:
   ResponseStats();
 
-  void submitResponse(DNSPacket &p, bool udpOrTCP, bool last=true);
+  void submitResponse(DNSPacket& p, bool udpOrTCP, bool last = true);
   void submitResponse(uint16_t qtype, uint16_t respsize, bool udpOrTCP);
   void submitResponse(uint16_t qtype, uint16_t respsize, uint8_t rcode, bool udpOrTCP);
   map<uint16_t, uint64_t> getQTypeResponseCounts();
@@ -39,10 +40,9 @@ public:
   string getQTypeReport();
 
 private:
-  boost::scoped_array<std::atomic<unsigned long>> d_qtypecounters;
-  boost::scoped_array<std::atomic<unsigned long>> d_rcodecounters;
-  typedef vector<pair<uint16_t, uint64_t> > sizecounters_t;
-  sizecounters_t d_sizecounters;
+  std::array<std::atomic<uint64_t>, 65535> d_qtypecounters;
+  std::array<std::atomic<uint64_t>, 256> d_rcodecounters;
+  pdns::AtomicHistogram<uint64_t> d_sizecounters;
 };
 
 extern ResponseStats g_rs;
diff --git a/pdns/syncres.hh b/pdns/syncres.hh
index 1a32e196f8..68aff8f259 100644
--- a/pdns/syncres.hh
+++ b/pdns/syncres.hh
@@ -1033,7 +1033,7 @@ struct RecursorStats
   std::atomic<uint64_t> dnssecAuthenticDataQueries;
   std::atomic<uint64_t> dnssecCheckDisabledQueries;
   std::atomic<uint64_t> variableResponses;
-  unsigned int maxMThreadStackUsage;
+  std::atomic<uint64_t> maxMThreadStackUsage;
   std::atomic<uint64_t> dnssecValidations; // should be the sum of all dnssecResult* stats
   std::map<vState, std::atomic<uint64_t> > dnssecResults;
   std::map<vState, std::atomic<uint64_t> > xdnssecResults;
diff --git a/pdns/tcpreceiver.cc b/pdns/tcpreceiver.cc
index 61dcc2fca4..1fbc4b2bb1 100644
--- a/pdns/tcpreceiver.cc
+++ b/pdns/tcpreceiver.cc
@@ -23,6 +23,7 @@
 #include "config.h"
 #endif
 #include <boost/algorithm/string.hpp>
+#include <boost/scoped_array.hpp>
 #include "auth-packetcache.hh"
 #include "utility.hh"
 #include "threadname.hh"