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 e0d4b92850ad66090b8a6fb939871b6240317792..57f18f5540e223e7a31a3c582b42f372f0aa2891 100644 (file)
--- 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 3396e0dd05378f294938b45f73c8d309c0fc0d5f..22c6cb8272feaf7bdb6ad07b179fadbdf6bb9b5c 100644 (file)
--- 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<packetCache_t::index<HashTag>::type::iterator, packetCache_t::index<HashTag>::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<HashTag>();
-  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<uint32_t>(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<HashTag>();
+  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<HashTag>();
+  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 a68dd2fe8cb4c536f899133456dd619c0ad7e4be..ca8c5d38fa2e2689d002923aed458234ef4067e0 100644 (file)
--- 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<packetCache_t::index<HashTag>::type::iterator, packetCache_t::index<HashTag>::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 bdd9addc410e8fcfc7089856c1d79396f2cd87d3..f4b492b66652531adc727b07d8abd0317f5ebbd4 100644 (file)
--- 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);