From: Remi Gacogne <remi.gacogne@powerdns.com>
Date: Wed, 29 Jul 2020 14:23:26 +0000 (+0200)
Subject: dnsdist: Speed up the round robin policy
X-Git-Tag: rec-4.4.0-beta1~13^2~1
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=6de0f4590d3bb3551a0afb5739be69e827bb790d;p=thirdparty%2Fpdns.git

dnsdist: Speed up the round robin policy

Working with indices instead of copying shared pointers results in
a nice speed up, cutting the CPU time in half.
---

diff --git a/pdns/dnsdistdist/dnsdist-lbpolicies.cc b/pdns/dnsdistdist/dnsdist-lbpolicies.cc
index 0dfc7e0c56..62f963ddef 100644
--- a/pdns/dnsdistdist/dnsdist-lbpolicies.cc
+++ b/pdns/dnsdistdist/dnsdist-lbpolicies.cc
@@ -199,23 +199,26 @@ shared_ptr<DownstreamState> chashed(const ServerPolicy::NumberedServerVector& se
 
 shared_ptr<DownstreamState> roundrobin(const ServerPolicy::NumberedServerVector& servers, const DNSQuestion* dq)
 {
-  ServerPolicy::NumberedServerVector poss;
+  vector<size_t> candidates;
+  candidates.reserve(servers.size());
 
-  for(auto& d : servers) {
-    if(d.second->isUp()) {
-      poss.push_back(d);
+  for (auto& d : servers) {
+    if (d.second->isUp()) {
+      candidates.push_back(d.first);
     }
   }
 
-  const auto *res=&poss;
-  if(poss.empty() && !g_roundrobinFailOnNoServer)
-    res = &servers;
-
-  if(res->empty())
-    return shared_ptr<DownstreamState>();
+  if (candidates.empty()) {
+    if (g_roundrobinFailOnNoServer) {
+      return shared_ptr<DownstreamState>();
+    }
+    for (auto& d : servers) {
+      candidates.push_back(d.first);
+    }
+  }
 
   static unsigned int counter;
-  return (*res)[(counter++) % res->size()].second;
+  return servers.at(candidates.at((counter++) % candidates.size()) - 1).second;
 }
 
 const std::shared_ptr<ServerPolicy::NumberedServerVector> getDownstreamCandidates(const pools_t& pools, const std::string& poolName)
diff --git a/pdns/dnsdistdist/test-dnsdistlbpolicies_cc.cc b/pdns/dnsdistdist/test-dnsdistlbpolicies_cc.cc
index c115ba047e..8e37c3f7cc 100644
--- a/pdns/dnsdistdist/test-dnsdistlbpolicies_cc.cc
+++ b/pdns/dnsdistdist/test-dnsdistlbpolicies_cc.cc
@@ -170,17 +170,64 @@ BOOST_AUTO_TEST_CASE(test_firstAvailable) {
   BOOST_REQUIRE(server != nullptr);
   BOOST_CHECK(server == servers.at(1).second);
 
-  std::vector<DNSName> names;
-  names.reserve(1000);
+  benchPolicy(pol);
+}
+
+BOOST_AUTO_TEST_CASE(test_roundRobin) {
+  auto dq = getDQ();
+
+  ServerPolicy pol{"roundrobin", roundrobin, false};
+  ServerPolicy::NumberedServerVector servers;
+  servers.push_back({ 1, std::make_shared<DownstreamState>(ComboAddress("192.0.2.1:53")) });
+
+  /* servers start as 'down' but the RR policy returns a server unless g_roundrobinFailOnNoServer is set */
+  g_roundrobinFailOnNoServer = true;
+  auto server = getSelectedBackendFromPolicy(pol, servers, dq);
+  BOOST_CHECK(server == nullptr);
+  g_roundrobinFailOnNoServer = false;
+  server = getSelectedBackendFromPolicy(pol, servers, dq);
+  BOOST_CHECK(server != nullptr);
+
+  /* mark the server as 'up' */
+  servers.at(0).second->setUp();
+  server = getSelectedBackendFromPolicy(pol, servers, dq);
+  BOOST_CHECK(server != nullptr);
+
+  /* add a second server, we should get the first one then the second one */
+  servers.push_back({ 2, std::make_shared<DownstreamState>(ComboAddress("192.0.2.2:53")) });
+  servers.at(1).second->setUp();
+  server = getSelectedBackendFromPolicy(pol, servers, dq);
+  BOOST_REQUIRE(server != nullptr);
+  BOOST_CHECK(server == servers.at(0).second);
+  server = getSelectedBackendFromPolicy(pol, servers, dq);
+  BOOST_REQUIRE(server != nullptr);
+  BOOST_CHECK(server == servers.at(1).second);
+
+  /* mark the first server as 'down', second as 'up' */
+  servers.at(0).second->setDown();
+  servers.at(1).second->setUp();
+  server = getSelectedBackendFromPolicy(pol, servers, dq);
+  BOOST_REQUIRE(server != nullptr);
+  BOOST_CHECK(server == servers.at(1).second);
+
+  std::map<std::shared_ptr<DownstreamState>, uint64_t> serversMap;
+  /* mark all servers 'up' */
+  for (auto& s : servers) {
+    s.second->setUp();
+    serversMap[s.second] = 0;
+  }
+
   for (size_t idx = 0; idx < 1000; idx++) {
-    names.push_back(DNSName("powerdns-" + std::to_string(idx) + ".com."));
+    server = getSelectedBackendFromPolicy(pol, servers, dq);
+    BOOST_REQUIRE(serversMap.count(server) == 1);
+    ++serversMap[server];
   }
-  std::map<std::shared_ptr<DownstreamState>, uint64_t> serversMap;
-  for (size_t idx = 1; idx <= 10; idx++) {
-    servers.push_back({ idx, std::make_shared<DownstreamState>(ComboAddress("192.0.2." + std::to_string(idx) + ":53")) });
-    serversMap[servers.at(idx - 1).second] = 0;
-    servers.at(idx - 1).second->setUp();
+  uint64_t total = 0;
+  for (const auto& entry : serversMap) {
+    BOOST_CHECK_EQUAL(entry.second, 1000 / servers.size());
+    total += entry.second;
   }
+  BOOST_CHECK_EQUAL(total, 1000U);
 
   benchPolicy(pol);
 }