From 0f1eeab48f65105aed3dd0d83f84a154cabf2583 Mon Sep 17 00:00:00 2001 From: bert hubert Date: Fri, 28 Nov 2014 16:14:18 +0100 Subject: [PATCH] Our "StatBag" statistics class was 1) 32 bit 2) heavily locked 3) still allowed for lock-free operations for high speed counters. This commit 1) makes the StatBag 64 bit on 64 bit systems, 2) removes all locks and 3) has gone AtomicCounter 'native' The upshot of this is that 64 bit users will suddenly get 64 bit counters. A second upshot is that multicore systems should now see consistent statistics again. One important thing that changed is that the StatBag class used to be completely thread safe, and now it no longer is. You can't declare new counters while the StatBag is in use. This should not be happening, but you never know. Finally, there is now a StatBag unit test. --- pdns/Makefile.am | 2 +- pdns/common_startup.cc | 8 ++--- pdns/dnsproxy.hh | 6 ++-- pdns/misc.hh | 6 ++++ pdns/nameserver.cc | 8 ++--- pdns/packetcache.cc | 10 +++--- pdns/packetcache.hh | 6 ++-- pdns/packethandler.cc | 2 +- pdns/statbag.cc | 68 ++++++++++++----------------------------- pdns/statbag.hh | 18 +++-------- pdns/test-statbag_cc.cc | 57 ++++++++++++++++++++++++++++++++++ pdns/ueberbackend.cc | 4 +-- 12 files changed, 110 insertions(+), 85 deletions(-) create mode 100644 pdns/test-statbag_cc.cc diff --git a/pdns/Makefile.am b/pdns/Makefile.am index 7371abb19..6f36e7aba 100644 --- a/pdns/Makefile.am +++ b/pdns/Makefile.am @@ -317,7 +317,7 @@ testrunner_SOURCES=testrunner.cc test-misc_hh.cc test-nameserver_cc.cc test-dnsr dnswriter.cc base64.cc base32.cc dnsrecords.cc dnslabeltext.cc dnsparser.cc \ rcpgenerator.cc ednssubnet.cc nsecrecords.cc sillyrecords.cc dnssecinfra.cc \ test-base64_cc.cc test-iputils_hh.cc test-dns_random_hh.cc dns_random.cc \ - test-rcpgenerator_cc.cc responsestats.cc test-bindparser_cc.cc \ + test-rcpgenerator_cc.cc responsestats.cc test-bindparser_cc.cc test-statbag_cc.cc \ bindparser.cc bindlexer.c test-zoneparser_tng_cc.cc zoneparser-tng.hh \ zoneparser-tng.cc dns.cc diff --git a/pdns/common_startup.cc b/pdns/common_startup.cc index 8605cdabb..70b55988f 100644 --- a/pdns/common_startup.cc +++ b/pdns/common_startup.cc @@ -243,12 +243,12 @@ void *qthread(void *number) DNSPacket question; DNSPacket cached; - unsigned int &numreceived=*S.getPointer("udp-queries"); - unsigned int &numreceiveddo=*S.getPointer("udp-do-queries"); + AtomicCounter &numreceived=*S.getPointer("udp-queries"); + AtomicCounter &numreceiveddo=*S.getPointer("udp-do-queries"); - unsigned int &numreceived4=*S.getPointer("udp4-queries"); + AtomicCounter &numreceived4=*S.getPointer("udp4-queries"); - unsigned int &numreceived6=*S.getPointer("udp6-queries"); + AtomicCounter &numreceived6=*S.getPointer("udp6-queries"); int diff; bool logDNSQueries = ::arg().mustDo("log-dns-queries"); diff --git a/pdns/dnsproxy.hh b/pdns/dnsproxy.hh index 95ba9ab01..4b96a5e80 100644 --- a/pdns/dnsproxy.hh +++ b/pdns/dnsproxy.hh @@ -66,9 +66,9 @@ public: private: NetmaskGroup d_ng; int d_sock; - unsigned int* d_resanswers; - unsigned int* d_udpanswers; - unsigned int* d_resquestions; + AtomicCounter* d_resanswers; + AtomicCounter* d_udpanswers; + AtomicCounter* d_resquestions; pthread_mutex_t d_lock; uint16_t d_xor; int getID_locked(); diff --git a/pdns/misc.hh b/pdns/misc.hh index 2740b40b7..e68acff9e 100644 --- a/pdns/misc.hh +++ b/pdns/misc.hh @@ -366,6 +366,12 @@ public: return atomic_exchange_and_add( &value_, +1 ); } + native_t operator+=(int val) + { + return atomic_exchange_and_add( &value_, val ); + } + + native_t operator--() { return atomic_exchange_and_add( &value_, -1 ) - 1; diff --git a/pdns/nameserver.cc b/pdns/nameserver.cc index 10331802d..9e570ea3a 100644 --- a/pdns/nameserver.cc +++ b/pdns/nameserver.cc @@ -284,10 +284,10 @@ ResponseStats g_rs; void UDPNameserver::send(DNSPacket *p) { const string& buffer=p->getString(); - static unsigned int &numanswered=*S.getPointer("udp-answers"); - static unsigned int &numanswered4=*S.getPointer("udp4-answers"); - static unsigned int &numanswered6=*S.getPointer("udp6-answers"); - static unsigned int &bytesanswered=*S.getPointer("udp-answers-bytes"); + static AtomicCounter &numanswered=*S.getPointer("udp-answers"); + static AtomicCounter &numanswered4=*S.getPointer("udp4-answers"); + static AtomicCounter &numanswered6=*S.getPointer("udp6-answers"); + static AtomicCounter &bytesanswered=*S.getPointer("udp-answers-bytes"); g_rs.submitResponse(p->qtype.getCode(), buffer.length(), true); diff --git a/pdns/packetcache.cc b/pdns/packetcache.cc index 764280877..179b7ce34 100644 --- a/pdns/packetcache.cc +++ b/pdns/packetcache.cc @@ -188,7 +188,7 @@ int PacketCache::purge() WriteLock l(&d_mut); int delcount=d_map.size(); d_map.clear(); - *d_statnumentries=0; + *d_statnumentries=AtomicCounter(0); return delcount; } @@ -222,7 +222,7 @@ int PacketCache::purge(const string &match) pair range = d_map.equal_range(tie(qname)); d_map.erase(range.first, range.second); } - *d_statnumentries=d_map.size(); + *d_statnumentries=AtomicCounter(d_map.size()); return delcount; } // called from ueberbackend @@ -310,12 +310,12 @@ void PacketCache::cleanup() { WriteLock l(&d_mut); - *d_statnumentries=d_map.size(); + *d_statnumentries=AtomicCounter(d_map.size()); unsigned int maxCached=::arg().asNum("max-cache-entries"); unsigned int toTrim=0; - unsigned int cacheSize=*d_statnumentries; + AtomicCounter::native_t cacheSize=*d_statnumentries; if(maxCached && cacheSize > maxCached) { toTrim = cacheSize - maxCached; @@ -354,6 +354,6 @@ void PacketCache::cleanup() break; } // cerr<<"erased: "<d.rd) { - static unsigned int &rdqueries=*S.getPointer("rd-queries"); + static AtomicCounter &rdqueries=*S.getPointer("rd-queries"); rdqueries++; } diff --git a/pdns/statbag.cc b/pdns/statbag.cc index 58eed06d2..5a103ac79 100644 --- a/pdns/statbag.cc +++ b/pdns/statbag.cc @@ -34,17 +34,12 @@ StatBag::StatBag() { d_doRings=false; - pthread_mutex_init(&d_lock,0); } - - -/** this NEEDS TO HAVE THE LOCK held already! */ void StatBag::exists(const string &key) { if(!d_stats.count(key)) { - unlock(); // it's the details that count throw PDNSException("Trying to deposit into unknown StatBag key '"+key+"'"); } } @@ -53,14 +48,14 @@ string StatBag::directory() { string dir; ostringstream o; - lock(); - for(map::const_iterator i=d_stats.begin(); + + for(map::const_iterator i=d_stats.begin(); i!=d_stats.end(); i++) { o<first<<"="<<*(i->second)<<","; } - unlock(); + dir=o.str(); return dir; } @@ -69,75 +64,50 @@ string StatBag::directory() vectorStatBag::getEntries() { vector ret; - lock(); - for(map::const_iterator i=d_stats.begin(); + + for(map::const_iterator i=d_stats.begin(); i!=d_stats.end(); i++) ret.push_back(i->first); - unlock(); + return ret; } string StatBag::getDescrip(const string &item) { - lock(); - string tmp=d_keyDescrips[item]; - unlock(); - return tmp; + exists(item); + return d_keyDescrips[item]; } void StatBag::declare(const string &key, const string &descrip) { - lock(); - unsigned int *i=new unsigned int(0); + AtomicCounter *i=new AtomicCounter(0); d_stats[key]=i; d_keyDescrips[key]=descrip; - unlock(); } -void StatBag::set(const string &key, unsigned int value) +void StatBag::set(const string &key, AtomicCounter::native_t value) { - lock(); exists(key); - *d_stats[key]=value; - - unlock(); + *d_stats[key]=AtomicCounter(value); } -unsigned int StatBag::read(const string &key) +AtomicCounter::native_t StatBag::read(const string &key) { - lock(); - - if(!d_stats.count(key)) - { - unlock(); - return 0; - } - - unsigned int tmp=*d_stats[key]; + exists(key); - unlock(); - return tmp; + return *d_stats[key]; } -unsigned int StatBag::readZero(const string &key) +AtomicCounter::native_t StatBag::readZero(const string &key) { - lock(); - - if(!d_stats.count(key)) - { - unlock(); - return 0; - } - - unsigned int tmp=*d_stats[key]; + exists(key); + AtomicCounter::native_t tmp=*d_stats[key]; d_stats[key]=0; - - unlock(); return tmp; } @@ -156,7 +126,7 @@ string StatBag::getValueStrZero(const string &key) return o.str(); } -unsigned int *StatBag::getPointer(const string &key) +AtomicCounter *StatBag::getPointer(const string &key) { exists(key); return d_stats[key]; @@ -164,7 +134,7 @@ unsigned int *StatBag::getPointer(const string &key) StatBag::~StatBag() { - for(map::const_iterator i=d_stats.begin(); + for(map::const_iterator i=d_stats.begin(); i!=d_stats.end(); i++) { diff --git a/pdns/statbag.hh b/pdns/statbag.hh index b45e7df1e..6f669a0bd 100644 --- a/pdns/statbag.hh +++ b/pdns/statbag.hh @@ -60,11 +60,10 @@ private: //! use this to gather and query statistics class StatBag { - map d_stats; + map d_stats; map d_keyDescrips; mapd_rings; bool d_doRings; - pthread_mutex_t d_lock; public: StatBag(); //!< Naked constructor. You need to declare keys before this class becomes useful @@ -96,26 +95,19 @@ public: void exists(const string &key); //!< call this function to throw an exception in case a key does not exist inline void deposit(const string &key, int value); //!< increment the statistics behind this key by value amount inline void inc(const string &key); //!< increase this key's value by one - void set(const string &key, unsigned int value); //!< set this key's value - unsigned int read(const string &key); //!< read the value behind this key - unsigned int readZero(const string &key); //!< read the value behind this key, and zero it afterwards - unsigned int *getPointer(const string &key); //!< get a direct pointer to the value behind a key. Use this for high performance increments + void set(const string &key, AtomicCounter::native_t value); //!< set this key's value + AtomicCounter::native_t read(const string &key); //!< read the value behind this key + AtomicCounter::native_t readZero(const string &key); //!< read the value behind this key, and zero it afterwards + AtomicCounter *getPointer(const string &key); //!< get a direct pointer to the value behind a key. Use this for high performance increments string getValueStr(const string &key); //!< read a value behind a key, and return it as a string string getValueStrZero(const string &key); //!< read a value behind a key, and return it as a string, and zero afterwards - -private: - void lock(){pthread_mutex_lock(&d_lock);} - void unlock(){pthread_mutex_unlock(&d_lock);} }; inline void StatBag::deposit(const string &key, int value) { - lock(); exists(key); *d_stats[key]+=value; - - unlock(); } inline void StatBag::inc(const string &key) diff --git a/pdns/test-statbag_cc.cc b/pdns/test-statbag_cc.cc new file mode 100644 index 000000000..fd4c35ca0 --- /dev/null +++ b/pdns/test-statbag_cc.cc @@ -0,0 +1,57 @@ +#define BOOST_TEST_DYN_LINK +#define BOOST_TEST_NO_MAIN + +#include +#include +#include +#include +#include "misc.hh" +#include "dns.hh" +#include "statbag.hh" + +using std::string; + +static void *threadMangler(void* a) +{ + AtomicCounter* ac = (AtomicCounter*)a; + for(unsigned int n=0; n < 10000000; ++n) + (*ac)++; + return 0; +} + +BOOST_AUTO_TEST_SUITE(misc_hh) + +BOOST_AUTO_TEST_CASE(test_StatBagBasic) { + StatBag s; + s.declare("a", "description"); + s.declare("b", "description"); + s.declare("c", "description"); + s.inc("a"); + BOOST_CHECK_EQUAL(s.read("a"), 1); + + int n; + for(n=0; n < 1000000; ++n) + s.inc("b"); + + BOOST_CHECK_EQUAL(s.read("b"), n); + + AtomicCounter* ac = s.getPointer("a"); + for(n=0; n < 1000000; ++n) + (*ac)++; + + BOOST_CHECK_EQUAL(s.read("a"), n+1); + + AtomicCounter* acc = s.getPointer("c"); + pthread_t tid[4]; + for(int i=0; i < 4; ++i) + pthread_create(&tid[i], 0, threadMangler, (void*)acc); + void* res; + for(int i=0; i < 4 ; ++i) + pthread_join(tid[i], &res); + + BOOST_CHECK_EQUAL(s.read("c"), 40000000U); +} + + +BOOST_AUTO_TEST_SUITE_END() + diff --git a/pdns/ueberbackend.cc b/pdns/ueberbackend.cc index 2576ab5d6..0c48f6916 100644 --- a/pdns/ueberbackend.cc +++ b/pdns/ueberbackend.cc @@ -449,8 +449,8 @@ void UeberBackend::cleanup() int UeberBackend::cacheHas(const Question &q, vector &rrs) { extern PacketCache PC; - static unsigned int *qcachehit=S.getPointer("query-cache-hit"); - static unsigned int *qcachemiss=S.getPointer("query-cache-miss"); + static AtomicCounter *qcachehit=S.getPointer("query-cache-hit"); + static AtomicCounter *qcachemiss=S.getPointer("query-cache-miss"); if(!d_cache_ttl && ! d_negcache_ttl) { (*qcachemiss)++; -- 2.40.0