From: Remi Gacogne Date: Wed, 22 Feb 2017 16:42:02 +0000 (+0100) Subject: rec: Don't parse the QName in the packet cache if we already have it X-Git-Tag: rec-4.1.0-alpha1~229^2 X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=c15ff3dff8172940b70609298b673c6add973e03;p=pdns rec: Don't parse the QName in the packet cache if we already have it When `gettag()` or protobuf are enabled, we have already parsed the qname, qtype and qclass so pass them to the Packet Cache instead of parsing them again. Don't parse them several times if we have more than one match from the cache either. --- diff --git a/pdns/pdns_recursor.cc b/pdns/pdns_recursor.cc index e0d4b9285..57f18f554 100644 --- a/pdns/pdns_recursor.cc +++ b/pdns/pdns_recursor.cc @@ -1563,6 +1563,7 @@ string* doProcessUDPQuestion(const std::string& question, const ComboAddress& fr uint16_t qtype=0; uint16_t qclass=0; uint32_t age; + bool qnameParsed=false; #ifdef MALLOC_TRACE /* static uint64_t last=0; @@ -1577,8 +1578,9 @@ string* doProcessUDPQuestion(const std::string& question, const ComboAddress& fr if(needECS || (t_pdl->get() && (*t_pdl)->d_gettag)) { try { - ecsParsed = true; ecsFound = getQNameAndSubnet(question, &qname, &qtype, &qclass, &ednssubnet); + qnameParsed = true; + ecsParsed = true; if(t_pdl->get() && (*t_pdl)->d_gettag) { try { @@ -1607,7 +1609,13 @@ string* doProcessUDPQuestion(const std::string& question, const ComboAddress& fr } #endif /* HAVE_PROTOBUF */ - cacheHit = (!SyncRes::s_nopacketcache && t_packetCache->getResponsePacket(ctag, question, g_now.tv_sec, &response, &age, &qhash, &pbMessage)); + if (qnameParsed) { + cacheHit = (!SyncRes::s_nopacketcache && t_packetCache->getResponsePacket(ctag, question, qname, qtype, qclass, g_now.tv_sec, &response, &age, &qhash, &pbMessage)); + } + else { + cacheHit = (!SyncRes::s_nopacketcache && t_packetCache->getResponsePacket(ctag, question, g_now.tv_sec, &response, &age, &qhash, &pbMessage)); + } + if (cacheHit) { #ifdef HAVE_PROTOBUF if(luaconfsLocal->protobufServer && (!luaconfsLocal->protobufTaggedOnly || !pbMessage.getAppliedPolicy().empty() || !pbMessage.getPolicyTags().empty())) { diff --git a/pdns/recpacketcache.cc b/pdns/recpacketcache.cc index 3396e0dd0..22c6cb827 100644 --- a/pdns/recpacketcache.cc +++ b/pdns/recpacketcache.cc @@ -43,10 +43,8 @@ int RecursorPacketCache::doWipePacketCache(const DNSName& name, uint16_t qtype, return count; } -static bool qrMatch(const std::string& query, const DNSName& rname, uint16_t rtype, uint16_t rclass) +static bool qrMatch(const DNSName& qname, uint16_t qtype, uint16_t qclass, const DNSName& rname, uint16_t rtype, uint16_t rclass) { - uint16_t qtype, qclass; - DNSName qname(query.c_str(), query.length(), sizeof(dnsheader), false, &qtype, &qclass, 0); // this ignores checking on the EDNS subnet flags! return qname==rname && rtype == qtype && rclass == qclass; } @@ -94,27 +92,11 @@ uint32_t RecursorPacketCache::canHashPacket(const std::string& origPacket) return ret; } -bool RecursorPacketCache::getResponsePacket(unsigned int tag, const std::string& queryPacket, time_t now, - std::string* responsePacket, uint32_t* age, uint32_t* qhash) -{ - return getResponsePacket(tag, queryPacket, now, responsePacket, age, qhash, nullptr); -} - -bool RecursorPacketCache::getResponsePacket(unsigned int tag, const std::string& queryPacket, time_t now, - std::string* responsePacket, uint32_t* age, uint32_t* qhash, RecProtoBufMessage* protobufMessage) +bool RecursorPacketCache::checkResponseMatches(std::pair::type::iterator, packetCache_t::index::type::iterator> range, const std::string& queryPacket, const DNSName& qname, uint16_t qtype, uint16_t qclass, time_t now, std::string* responsePacket, uint32_t* age, RecProtoBufMessage* protobufMessage) { - *qhash = canHashPacket(queryPacket); - auto& idx = d_packetCache.get(); - auto range = idx.equal_range(tie(tag,*qhash)); - - if(range.first == range.second) { - d_misses++; - return false; - } - for(auto iter = range.first ; iter != range.second ; ++ iter) { // the possibility is VERY real that we get hits that are not right - birthday paradox - if(!qrMatch(queryPacket, iter->d_name, iter->d_type, iter->d_class)) + if(!qrMatch(qname, qtype, qclass, iter->d_name, iter->d_type, iter->d_class)) continue; if(now < iter->d_ttd) { // it is right, it is fresh! *age = static_cast(now - iter->d_creation); @@ -124,7 +106,7 @@ bool RecursorPacketCache::getResponsePacket(unsigned int tag, const std::string& string::size_type i=sizeof(dnsheader); for(;;) { - int labellen = (unsigned char)queryPacket[i]; + unsigned int labellen = (unsigned char)queryPacket[i]; if(!labellen || i + labellen > responsePacket->size()) break; i++; responsePacket->replace(i, labellen, queryPacket, i, labellen); @@ -151,6 +133,51 @@ bool RecursorPacketCache::getResponsePacket(unsigned int tag, const std::string& return false; } +bool RecursorPacketCache::getResponsePacket(unsigned int tag, const std::string& queryPacket, time_t now, + std::string* responsePacket, uint32_t* age, uint32_t* qhash) +{ + return getResponsePacket(tag, queryPacket, now, responsePacket, age, qhash, nullptr); +} + +bool RecursorPacketCache::getResponsePacket(unsigned int tag, const std::string& queryPacket, const DNSName& qname, uint16_t qtype, uint16_t qclass, time_t now, + std::string* responsePacket, uint32_t* age, uint32_t* qhash) +{ + return getResponsePacket(tag, queryPacket, qname, qtype, qclass, now, responsePacket, age, qhash, nullptr); +} + +bool RecursorPacketCache::getResponsePacket(unsigned int tag, const std::string& queryPacket, const DNSName& qname, uint16_t qtype, uint16_t qclass, time_t now, + std::string* responsePacket, uint32_t* age, uint32_t* qhash, RecProtoBufMessage* protobufMessage) +{ + *qhash = canHashPacket(queryPacket); + const auto& idx = d_packetCache.get(); + auto range = idx.equal_range(tie(tag,*qhash)); + + if(range.first == range.second) { + d_misses++; + return false; + } + + return checkResponseMatches(range, queryPacket, qname, qtype, qclass, now, responsePacket, age, protobufMessage); +} + +bool RecursorPacketCache::getResponsePacket(unsigned int tag, const std::string& queryPacket, time_t now, + std::string* responsePacket, uint32_t* age, uint32_t* qhash, RecProtoBufMessage* protobufMessage) +{ + *qhash = canHashPacket(queryPacket); + const auto& idx = d_packetCache.get(); + auto range = idx.equal_range(tie(tag,*qhash)); + + if(range.first == range.second) { + d_misses++; + return false; + } + + uint16_t qtype, qclass; + DNSName qname(queryPacket.c_str(), queryPacket.length(), sizeof(dnsheader), false, &qtype, &qclass, 0); + + return checkResponseMatches(range, queryPacket, qname, qtype, qclass, now, responsePacket, age, protobufMessage); +} + void RecursorPacketCache::insertResponsePacket(unsigned int tag, uint32_t qhash, const DNSName& qname, uint16_t qtype, uint16_t qclass, const std::string& responsePacket, time_t now, uint32_t ttl) { diff --git a/pdns/recpacketcache.hh b/pdns/recpacketcache.hh index a68dd2fe8..ca8c5d38f 100644 --- a/pdns/recpacketcache.hh +++ b/pdns/recpacketcache.hh @@ -52,6 +52,8 @@ public: RecursorPacketCache(); bool getResponsePacket(unsigned int tag, const std::string& queryPacket, time_t now, std::string* responsePacket, uint32_t* age, uint32_t* qhash); bool getResponsePacket(unsigned int tag, const std::string& queryPacket, time_t now, std::string* responsePacket, uint32_t* age, uint32_t* qhash, RecProtoBufMessage* protobufMessage); + bool getResponsePacket(unsigned int tag, const std::string& queryPacket, const DNSName& qname, uint16_t qtype, uint16_t qclass, time_t now, std::string* responsePacket, uint32_t* age, uint32_t* qhash); + bool getResponsePacket(unsigned int tag, const std::string& queryPacket, const DNSName& qname, uint16_t qtype, uint16_t qclass, time_t now, std::string* responsePacket, uint32_t* age, uint32_t* qhash, RecProtoBufMessage* protobufMessage); void insertResponsePacket(unsigned int tag, uint32_t qhash, const DNSName& qname, uint16_t qtype, uint16_t qclass, const std::string& responsePacket, time_t now, uint32_t ttl); void insertResponsePacket(unsigned int tag, uint32_t qhash, const DNSName& qname, uint16_t qtype, uint16_t qclass, const std::string& responsePacket, time_t now, uint32_t ttl, const RecProtoBufMessage* protobufMessage); void doPruneTo(unsigned int maxSize=250000); @@ -97,6 +99,8 @@ private: > packetCache_t; packetCache_t d_packetCache; + + bool checkResponseMatches(std::pair::type::iterator, packetCache_t::index::type::iterator> range, const std::string& queryPacket, const DNSName& qname, uint16_t qtype, uint16_t qclass, time_t now, std::string* responsePacket, uint32_t* age, RecProtoBufMessage* protobufMessage); }; #endif diff --git a/pdns/test-recpacketcache_cc.cc b/pdns/test-recpacketcache_cc.cc index bdd9addc4..f4b492b66 100644 --- a/pdns/test-recpacketcache_cc.cc +++ b/pdns/test-recpacketcache_cc.cc @@ -34,6 +34,7 @@ BOOST_AUTO_TEST_CASE(test_recPacketCacheSimple) { pw.startRecord(qname, QType::A, ttd); BOOST_CHECK_EQUAL(rpc.getResponsePacket(tag, qpacket, time(nullptr), &fpacket, &age, &qhash), false); + BOOST_CHECK_EQUAL(rpc.getResponsePacket(tag, qpacket, qname, QType::A, QClass::IN, time(nullptr), &fpacket, &age, &qhash), false); ARecordContent ar("127.0.0.1"); ar.toPacket(pw); @@ -55,6 +56,10 @@ BOOST_AUTO_TEST_CASE(test_recPacketCacheSimple) { BOOST_CHECK_EQUAL(found, true); BOOST_CHECK_EQUAL(qhash, qhash2); BOOST_CHECK_EQUAL(fpacket, rpacket); + found = rpc.getResponsePacket(tag, qpacket, qname, QType::A, QClass::IN, time(nullptr), &fpacket, &age, &qhash2); + BOOST_CHECK_EQUAL(found, true); + BOOST_CHECK_EQUAL(qhash, qhash2); + BOOST_CHECK_EQUAL(fpacket, rpacket); packet.clear(); qname+=DNSName("co.uk"); @@ -67,6 +72,8 @@ BOOST_AUTO_TEST_CASE(test_recPacketCacheSimple) { found = rpc.getResponsePacket(tag, qpacket, time(nullptr), &fpacket, &age, &qhash); BOOST_CHECK_EQUAL(found, false); + found = rpc.getResponsePacket(tag, qpacket, qname, QType::A, QClass::IN, time(nullptr), &fpacket, &age, &qhash); + BOOST_CHECK_EQUAL(found, false); rpc.doWipePacketCache(DNSName("com"), 0xffff, true); BOOST_CHECK_EQUAL(rpc.size(), 0);