From: Remi Gacogne Date: Fri, 23 Mar 2018 22:13:52 +0000 (+0100) Subject: dnsdist: Use a separate lock for accessing the pool's servers X-Git-Tag: dnsdist-1.3.0~29^2 X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=a1b1a29ddbdc8d104d02564d023ba53015fa7861;p=pdns dnsdist: Use a separate lock for accessing the pool's servers We used to hold the Lua lock while applying the load-balancing policy to select a backend, which is only needed by Lua policies, not core ones. We do need a lock to make sure that the vector of servers is not altered under our feet, but a per-pool read-write lock is enough and reduces contention a lot, especially when the maintenance thread is doing some heavy-lifting. --- diff --git a/pdns/dnsdist-carbon.cc b/pdns/dnsdist-carbon.cc index 6ef88767f..32cab5d9a 100644 --- a/pdns/dnsdist-carbon.cc +++ b/pdns/dnsdist-carbon.cc @@ -111,8 +111,8 @@ try } const string base = "dnsdist." + hostname + ".main.pools." + poolName + "."; const std::shared_ptr pool = entry.second; - str<servers.size() << " " << now << "\r\n"; - str<countServersUp() << " " << now << "\r\n"; + str<countServers(false) << " " << now << "\r\n"; + str<countServers(true) << " " << now << "\r\n"; if (pool->packetCache != nullptr) { const auto& cache = pool->packetCache; str<getMaxEntries() << " " << now << "\r\n"; diff --git a/pdns/dnsdist-lua-bindings.cc b/pdns/dnsdist-lua-bindings.cc index a317e79d1..24e31590a 100644 --- a/pdns/dnsdist-lua-bindings.cc +++ b/pdns/dnsdist-lua-bindings.cc @@ -45,15 +45,16 @@ void setupLuaBindings(bool client) }); /* ServerPolicy */ - g_lua.writeFunction("newServerPolicy", [](string name, policyfunc_t policy) { return ServerPolicy{name, policy};}); + g_lua.writeFunction("newServerPolicy", [](string name, policyfunc_t policy) { return ServerPolicy{name, policy, true};}); g_lua.registerMember("name", &ServerPolicy::name); g_lua.registerMember("policy", &ServerPolicy::policy); + g_lua.registerMember("isLua", &ServerPolicy::isLua); - g_lua.writeVariable("firstAvailable", ServerPolicy{"firstAvailable", firstAvailable}); - g_lua.writeVariable("roundrobin", ServerPolicy{"roundrobin", roundrobin}); - g_lua.writeVariable("wrandom", ServerPolicy{"wrandom", wrandom}); - g_lua.writeVariable("whashed", ServerPolicy{"whashed", whashed}); - g_lua.writeVariable("leastOutstanding", ServerPolicy{"leastOutstanding", leastOutstanding}); + g_lua.writeVariable("firstAvailable", ServerPolicy{"firstAvailable", firstAvailable, false}); + g_lua.writeVariable("roundrobin", ServerPolicy{"roundrobin", roundrobin, false}); + g_lua.writeVariable("wrandom", ServerPolicy{"wrandom", wrandom, false}); + g_lua.writeVariable("whashed", ServerPolicy{"whashed", whashed, false}); + g_lua.writeVariable("leastOutstanding", ServerPolicy{"leastOutstanding", leastOutstanding, false}); /* ServerPool */ g_lua.registerFunction::*)(std::shared_ptr)>("setCache", [](std::shared_ptr pool, std::shared_ptr cache) { diff --git a/pdns/dnsdist-lua.cc b/pdns/dnsdist-lua.cc index 0e573f16d..1701af44e 100644 --- a/pdns/dnsdist-lua.cc +++ b/pdns/dnsdist-lua.cc @@ -411,7 +411,7 @@ void setupLuaConfig(bool client) }); g_lua.writeFunction("setServerPolicyLua", [](string name, policyfunc_t policy) { setLuaSideEffect(); - g_policy.setState(ServerPolicy{name, policy}); + g_policy.setState(ServerPolicy{name, policy, true}); }); g_lua.writeFunction("showServerPolicy", []() { @@ -1069,7 +1069,7 @@ void setupLuaConfig(bool client) } string servers; - for (const auto& server: pool->servers) { + for (const auto& server: pool->getServers()) { if (!servers.empty()) { servers += ", "; } @@ -1353,7 +1353,7 @@ void setupLuaConfig(bool client) g_lua.writeFunction("setPoolServerPolicyLua", [](string name, policyfunc_t policy, string pool) { setLuaSideEffect(); auto localPools = g_pools.getCopy(); - setPoolPolicy(localPools, pool, std::make_shared(ServerPolicy{name, policy})); + setPoolPolicy(localPools, pool, std::make_shared(ServerPolicy{name, policy, true})); g_pools.setState(localPools); }); diff --git a/pdns/dnsdist-tcp.cc b/pdns/dnsdist-tcp.cc index b3b74864e..74613c32e 100644 --- a/pdns/dnsdist-tcp.cc +++ b/pdns/dnsdist-tcp.cc @@ -385,15 +385,19 @@ void* tcpClientThread(int pipefd) } std::shared_ptr serverPool = getPool(*holders.pools, poolname); - std::shared_ptr packetCache = nullptr; - auto policy = holders.policy->policy; + std::shared_ptr packetCache = serverPool->packetCache; + + auto policy = *(holders.policy); if (serverPool->policy != nullptr) { - policy = serverPool->policy->policy; + policy = *(serverPool->policy); } - { + auto servers = serverPool->getServers(); + if (policy.isLua) { std::lock_guard lock(g_luamutex); - ds = policy(serverPool->servers, &dq); - packetCache = serverPool->packetCache; + ds = policy.policy(servers, &dq); + } + else { + ds = policy.policy(servers, &dq); } if (dq.useECS && ds && ds->useECS) { diff --git a/pdns/dnsdist-web.cc b/pdns/dnsdist-web.cc index b2bca4335..8ebb4818e 100644 --- a/pdns/dnsdist-web.cc +++ b/pdns/dnsdist-web.cc @@ -448,7 +448,7 @@ static void connectionThread(int sock, ComboAddress remote, string password, str Json::object entry { { "id", num++ }, { "name", pool.first }, - { "serversCount", (int) pool.second->servers.size() }, + { "serversCount", (int) pool.second->countServers(false) }, { "cacheSize", (double) (cache ? cache->getMaxEntries() : 0) }, { "cacheEntries", (double) (cache ? cache->getEntriesCount() : 0) }, { "cacheHits", (double) (cache ? cache->getHits() : 0) }, diff --git a/pdns/dnsdist.cc b/pdns/dnsdist.cc index 958701c06..c928994a1 100644 --- a/pdns/dnsdist.cc +++ b/pdns/dnsdist.cc @@ -790,22 +790,12 @@ void setPoolPolicy(pools_t& pools, const string& poolName, std::shared_ptr server) { std::shared_ptr pool = createPoolIfNotExists(pools, poolName); - unsigned int count = (unsigned int) pool->servers.size(); if (!poolName.empty()) { vinfolog("Adding server to pool %s", poolName); } else { vinfolog("Adding server to default pool"); } - pool->servers.push_back(make_pair(++count, server)); - /* we need to reorder based on the server 'order' */ - std::stable_sort(pool->servers.begin(), pool->servers.end(), [](const std::pair >& a, const std::pair >& b) { - return a.second->order < b.second->order; - }); - /* and now we need to renumber for Lua (custom policies) */ - size_t idx = 1; - for (auto& serv : pool->servers) { - serv.first = idx++; - } + pool->addServer(server); } void removeServerFromPool(pools_t& pools, const string& poolName, std::shared_ptr server) @@ -819,23 +809,7 @@ void removeServerFromPool(pools_t& pools, const string& poolName, std::shared_pt vinfolog("Removing server from default pool"); } - size_t idx = 1; - bool found = false; - for (NumberedVector >::iterator it = pool->servers.begin(); it != pool->servers.end();) { - if (found) { - /* we need to renumber the servers placed - after the removed one, for Lua (custom policies) */ - it->first = idx++; - it++; - } - else if (it->second == server) { - it = pool->servers.erase(it); - found = true; - } else { - idx++; - it++; - } - } + pool->removeServer(server); } std::shared_ptr getPool(const pools_t& pools, const std::string& poolName) @@ -849,10 +823,10 @@ std::shared_ptr getPool(const pools_t& pools, const std::string& poo return it->second; } -const NumberedServerVector& getDownstreamCandidates(const pools_t& pools, const std::string& poolName) +NumberedServerVector getDownstreamCandidates(const pools_t& pools, const std::string& poolName) { std::shared_ptr pool = getPool(pools, poolName); - return pool->servers; + return pool->getServers(); } // goal in life - if you send us a reasonably normal packet, we'll get Z for you, otherwise 0 @@ -1358,15 +1332,18 @@ static void processUDPQuery(ClientState& cs, LocalHolders& holders, const struct DownstreamState* ss = nullptr; std::shared_ptr serverPool = getPool(*holders.pools, poolname); - std::shared_ptr packetCache = nullptr; - auto policy = holders.policy->policy; + std::shared_ptr packetCache = serverPool->packetCache; + auto policy = *(holders.policy); if (serverPool->policy != nullptr) { - policy = serverPool->policy->policy; + policy = *(serverPool->policy); } - { + auto servers = serverPool->getServers(); + if (policy.isLua) { std::lock_guard lock(g_luamutex); - ss = policy(serverPool->servers, &dq).get(); - packetCache = serverPool->packetCache; + ss = policy.policy(servers, &dq).get(); + } + else { + ss = policy.policy(servers, &dq).get(); } bool ednsAdded = false; @@ -1804,10 +1781,7 @@ void* maintThread() const auto localPools = g_pools.getCopy(); std::shared_ptr packetCache = nullptr; for (const auto& entry : localPools) { - { - std::lock_guard lock(g_luamutex); - packetCache = entry.second->packetCache; - } + packetCache = entry.second->packetCache; if (packetCache) { size_t upTo = (packetCache->getMaxEntries()* (100 - g_cacheCleaningPercentage)) / 100; packetCache->purgeExpired(upTo); @@ -2272,7 +2246,7 @@ try } } - ServerPolicy leastOutstandingPol{"leastOutstanding", leastOutstanding}; + ServerPolicy leastOutstandingPol{"leastOutstanding", leastOutstanding, false}; g_policy.setState(leastOutstandingPol); if(g_cmdLine.beClient || !g_cmdLine.command.empty()) { diff --git a/pdns/dnsdist.hh b/pdns/dnsdist.hh index 7eca6bd72..862efa60b 100644 --- a/pdns/dnsdist.hh +++ b/pdns/dnsdist.hh @@ -645,25 +645,84 @@ struct ServerPolicy { string name; policyfunc_t policy; + bool isLua; }; struct ServerPool { + ServerPool() + { + pthread_rwlock_init(&d_lock, nullptr); + } + const std::shared_ptr getCache() const { return packetCache; }; - NumberedVector> servers; std::shared_ptr packetCache{nullptr}; std::shared_ptr policy{nullptr}; - size_t countServersUp() const { - size_t upFound = 0; - for (const auto& server : servers) { - if (std::get<1>(server)->isUp() ) { - upFound++; + size_t countServers(bool upOnly) + { + size_t count = 0; + ReadLock rl(&d_lock); + for (const auto& server : d_servers) { + if (!upOnly || std::get<1>(server)->isUp() ) { + count++; }; }; - return upFound; - }; + return count; + } + + NumberedVector> getServers() + { + NumberedVector> result; + { + ReadLock rl(&d_lock); + result = d_servers; + } + return result; + } + + void addServer(shared_ptr& server) + { + WriteLock wl(&d_lock); + unsigned int count = (unsigned int) d_servers.size(); + d_servers.push_back(make_pair(++count, server)); + /* we need to reorder based on the server 'order' */ + std::stable_sort(d_servers.begin(), d_servers.end(), [](const std::pair >& a, const std::pair >& b) { + return a.second->order < b.second->order; + }); + /* and now we need to renumber for Lua (custom policies) */ + size_t idx = 1; + for (auto& serv : d_servers) { + serv.first = idx++; + } + } + + void removeServer(shared_ptr& server) + { + WriteLock wl(&d_lock); + size_t idx = 1; + bool found = false; + for (auto it = d_servers.begin(); it != d_servers.end();) { + if (found) { + /* we need to renumber the servers placed + after the removed one, for Lua (custom policies) */ + it->first = idx++; + it++; + } + else if (it->second == server) { + it = d_servers.erase(it); + found = true; + } else { + idx++; + it++; + } + } + } + +private: + NumberedVector> d_servers; + pthread_rwlock_t d_lock; }; using pools_t=map>; void setPoolPolicy(pools_t& pools, const string& poolName, std::shared_ptr policy); @@ -786,7 +845,7 @@ void controlThread(int fd, ComboAddress local); vector> setupLua(bool client, const std::string& config); std::shared_ptr getPool(const pools_t& pools, const std::string& poolName); std::shared_ptr createPoolIfNotExists(pools_t& pools, const string& poolName); -const NumberedServerVector& getDownstreamCandidates(const pools_t& pools, const std::string& poolName); +NumberedServerVector getDownstreamCandidates(const pools_t& pools, const std::string& poolName); std::shared_ptr firstAvailable(const NumberedServerVector& servers, const DNSQuestion* dq);