From: Remi Gacogne <remi.gacogne@powerdns.com>
Date: Thu, 18 Jul 2024 07:56:30 +0000 (+0200)
Subject: dnsdist: Use atomic variables for the per-protocol latencies
X-Git-Tag: rec-5.2.0-alpha1~162^2~1
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=3fd6acb523077c87cb6946282f1a0d0b20e565cb;p=thirdparty%2Fpdns.git

dnsdist: Use atomic variables for the per-protocol latencies

While we do not care about a race in the sense that it's OK if we
miss/overwrite a value from time to time, we still need to prevent
an interleaved write from two threads which could technically result
in a garbage value. Using atomics also makes it easier for TSAN to
understand what's going on.
---

diff --git a/pdns/dnsdistdist/dnsdist-carbon.cc b/pdns/dnsdistdist/dnsdist-carbon.cc
index ee396d0452..e37bece4e4 100644
--- a/pdns/dnsdistdist/dnsdist-carbon.cc
+++ b/pdns/dnsdistdist/dnsdist-carbon.cc
@@ -64,9 +64,6 @@ static bool doOneCarbonExport(const Carbon::Endpoint& endpoint)
         else if (const auto& adval = std::get_if<pdns::stat_t_trait<double>*>(&entry.d_value)) {
           str << (*adval)->load();
         }
-        else if (const auto& dval = std::get_if<double*>(&entry.d_value)) {
-          str << **dval;
-        }
         else if (const auto& func = std::get_if<dnsdist::metrics::Stats::statfunction_t>(&entry.d_value)) {
           str << (*func)(entry.d_name);
         }
diff --git a/pdns/dnsdistdist/dnsdist-lua-inspection.cc b/pdns/dnsdistdist/dnsdist-lua-inspection.cc
index 43d7e9b082..015d26f3ab 100644
--- a/pdns/dnsdistdist/dnsdist-lua-inspection.cc
+++ b/pdns/dnsdistdist/dnsdist-lua-inspection.cc
@@ -817,9 +817,6 @@ void setupLuaInspection(LuaContext& luaCtx)
       else if (const auto& adval = std::get_if<pdns::stat_t_trait<double>*>(&entry.d_value)) {
         second = (flt % (*adval)->load()).str();
       }
-      else if (const auto& dval = std::get_if<double*>(&entry.d_value)) {
-        second = (flt % (**dval)).str();
-      }
       else if (const auto& func = std::get_if<dnsdist::metrics::Stats::statfunction_t>(&entry.d_value)) {
         second = std::to_string((*func)(entry.d_name));
       }
diff --git a/pdns/dnsdistdist/dnsdist-metrics.hh b/pdns/dnsdistdist/dnsdist-metrics.hh
index 8e899cedea..35cbf1a1eb 100644
--- a/pdns/dnsdistdist/dnsdist-metrics.hh
+++ b/pdns/dnsdistdist/dnsdist-metrics.hh
@@ -81,14 +81,14 @@ struct Stats
   stat_t tcpQueryPipeFull{0};
   stat_t tcpCrossProtocolQueryPipeFull{0};
   stat_t tcpCrossProtocolResponsePipeFull{0};
-  double latencyAvg100{0}, latencyAvg1000{0}, latencyAvg10000{0}, latencyAvg1000000{0};
-  double latencyTCPAvg100{0}, latencyTCPAvg1000{0}, latencyTCPAvg10000{0}, latencyTCPAvg1000000{0};
-  double latencyDoTAvg100{0}, latencyDoTAvg1000{0}, latencyDoTAvg10000{0}, latencyDoTAvg1000000{0};
-  double latencyDoHAvg100{0}, latencyDoHAvg1000{0}, latencyDoHAvg10000{0}, latencyDoHAvg1000000{0};
-  double latencyDoQAvg100{0}, latencyDoQAvg1000{0}, latencyDoQAvg10000{0}, latencyDoQAvg1000000{0};
-  double latencyDoH3Avg100{0}, latencyDoH3Avg1000{0}, latencyDoH3Avg10000{0}, latencyDoH3Avg1000000{0};
+  pdns::stat_t_trait<double> latencyAvg100{0}, latencyAvg1000{0}, latencyAvg10000{0}, latencyAvg1000000{0};
+  pdns::stat_t_trait<double> latencyTCPAvg100{0}, latencyTCPAvg1000{0}, latencyTCPAvg10000{0}, latencyTCPAvg1000000{0};
+  pdns::stat_t_trait<double> latencyDoTAvg100{0}, latencyDoTAvg1000{0}, latencyDoTAvg10000{0}, latencyDoTAvg1000000{0};
+  pdns::stat_t_trait<double> latencyDoHAvg100{0}, latencyDoHAvg1000{0}, latencyDoHAvg10000{0}, latencyDoHAvg1000000{0};
+  pdns::stat_t_trait<double> latencyDoQAvg100{0}, latencyDoQAvg1000{0}, latencyDoQAvg10000{0}, latencyDoQAvg1000000{0};
+  pdns::stat_t_trait<double> latencyDoH3Avg100{0}, latencyDoH3Avg1000{0}, latencyDoH3Avg10000{0}, latencyDoH3Avg1000000{0};
   using statfunction_t = std::function<uint64_t(const std::string&)>;
-  using entry_t = std::variant<stat_t*, pdns::stat_t_trait<double>*, double*, statfunction_t>;
+  using entry_t = std::variant<stat_t*, pdns::stat_t_trait<double>*, statfunction_t>;
   struct EntryPair
   {
     std::string d_name;
diff --git a/pdns/dnsdistdist/dnsdist-snmp.cc b/pdns/dnsdistdist/dnsdist-snmp.cc
index be07bd9fe8..68359cd1b5 100644
--- a/pdns/dnsdistdist/dnsdist-snmp.cc
+++ b/pdns/dnsdistdist/dnsdist-snmp.cc
@@ -140,8 +140,8 @@ static int handleFloatStats(netsnmp_mib_handler* handler,
     return SNMP_ERR_GENERR;
   }
 
-  if (const auto& val = std::get_if<double*>(&stIt->second)) {
-    std::string str(std::to_string(**val));
+  if (const auto& val = std::get_if<pdns::stat_t_trait<double>*>(&stIt->second)) {
+    std::string str(std::to_string((*val)->load()));
     snmp_set_var_typed_value(requests->requestvb,
                              ASN_OCTET_STR,
                              str.c_str(),
@@ -152,7 +152,7 @@ static int handleFloatStats(netsnmp_mib_handler* handler,
   return SNMP_ERR_GENERR;
 }
 
-static void registerFloatStat(const char* name, const OIDStat& statOID, double* ptr)
+static void registerFloatStat(const char* name, const OIDStat& statOID, pdns::stat_t_trait<double>* ptr)
 {
   if (statOID.size() != OID_LENGTH(queriesOID)) {
     errlog("Invalid OID for SNMP Float statistic %s", name);
diff --git a/pdns/dnsdistdist/dnsdist-web.cc b/pdns/dnsdistdist/dnsdist-web.cc
index 7790e755f0..65e3ad9218 100644
--- a/pdns/dnsdistdist/dnsdist-web.cc
+++ b/pdns/dnsdistdist/dnsdist-web.cc
@@ -515,9 +515,6 @@ static void handlePrometheus(const YaHTTP::Request& req, YaHTTP::Response& resp)
       else if (const auto& adval = std::get_if<pdns::stat_t_trait<double>*>(&entry.d_value)) {
         output << (*adval)->load();
       }
-      else if (const auto& dval = std::get_if<double*>(&entry.d_value)) {
-        output << **dval;
-      }
       else if (const auto& func = std::get_if<dnsdist::metrics::Stats::statfunction_t>(&entry.d_value)) {
         output << (*func)(entry.d_name);
       }
@@ -935,9 +932,6 @@ static void addStatsToJSONObject(Json::object& obj)
     else if (const auto& adval = std::get_if<pdns::stat_t_trait<double>*>(&entry.d_value)) {
       obj.emplace(entry.d_name, (*adval)->load());
     }
-    else if (const auto& dval = std::get_if<double*>(&entry.d_value)) {
-      obj.emplace(entry.d_name, (**dval));
-    }
     else if (const auto& func = std::get_if<dnsdist::metrics::Stats::statfunction_t>(&entry.d_value)) {
       obj.emplace(entry.d_name, (double)(*func)(entry.d_name));
     }
@@ -1407,12 +1401,6 @@ static void handleStatsOnly(const YaHTTP::Request& req, YaHTTP::Response& resp)
           {"name", item.d_name},
           {"value", (*adval)->load()}});
       }
-      else if (const auto& dval = std::get_if<double*>(&item.d_value)) {
-        doc.emplace_back(Json::object{
-          {"type", "StatisticItem"},
-          {"name", item.d_name},
-          {"value", (**dval)}});
-      }
       else if (const auto& func = std::get_if<dnsdist::metrics::Stats::statfunction_t>(&item.d_value)) {
         doc.emplace_back(Json::object{
           {"type", "StatisticItem"},
diff --git a/pdns/dnsdistdist/dnsdist.cc b/pdns/dnsdistdist/dnsdist.cc
index 41e8c1ca6d..49b5de0df8 100644
--- a/pdns/dnsdistdist/dnsdist.cc
+++ b/pdns/dnsdistdist/dnsdist.cc
@@ -187,8 +187,8 @@ static std::unique_ptr<DelayPipe<DelayedPacket>> g_delay{nullptr};
 
 static void doLatencyStats(dnsdist::Protocol protocol, double udiff)
 {
-  constexpr auto doAvg = [](double& var, double n, double weight) {
-    var = (weight - 1) * var / weight + n / weight;
+  constexpr auto doAvg = [](pdns::stat_t_trait<double>& var, double n, double weight) {
+    var.store((weight - 1) * var.load() / weight + n / weight);
   };
 
   if (protocol == dnsdist::Protocol::DoUDP || protocol == dnsdist::Protocol::DNSCryptUDP) {