From: Marcin Siodelski Date: Mon, 2 May 2016 12:25:23 +0000 (+0200) Subject: [4493] Improve packet collecting mechanisms in perfdhcp. X-Git-Tag: trac4106_update_base~31^2~3 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=54b114ef031a40958cdd64e4d0c86e7ea8fa5089;p=thirdparty%2Fkea.git [4493] Improve packet collecting mechanisms in perfdhcp. When the perfdhcp performs unordered packet lookup within a bucket of sent packets, it will remove all timed out transactions. --- diff --git a/src/bin/perfdhcp/stats_mgr.h b/src/bin/perfdhcp/stats_mgr.h index 1d17c6cf23..ca2d62353b 100644 --- a/src/bin/perfdhcp/stats_mgr.h +++ b/src/bin/perfdhcp/stats_mgr.h @@ -446,12 +446,19 @@ public: PktListRemovalQueue to_remove; for (PktListTransidHashIterator it = p.first; it != p.second; ++it) { + // If transaction id is matching, we found the original + // packet sent to the server. Therefore, we reset the + // 'next sent' pointer to point to this location. We + // also indicate that the matching packet is found. + // Even though the packet has been found, we continue + // iterating over the bucket to remove all those packets + // that are timed out. if ((*it)->getTransid() == rcvd_packet->getTransid()) { packet_found = true; - next_sent_ = - sent_packets_.template project<0>(it); - break; + next_sent_ = sent_packets_.template project<0>(it); } + // Check if the packet should be removed due to timeout. + // This includes the packet matching the received one. ptime now = microsec_clock::universal_time(); ptime packet_time = (*it)->getTimestamp(); time_period packet_period(packet_time, now); @@ -468,20 +475,23 @@ public: } } - // Deal with the removal queue + // Deal with the removal queue. while (!to_remove.empty()) { PktListTransidHashIterator it = to_remove.front(); to_remove.pop(); - // The packet pointed to by 'it' is timed out so - // we have to remove it. - if (packet_found || !to_remove.empty()) { + // If timed out packet is not the one matching server response, + // we simply remove it and keep the pointer to the 'next sent' + // packet as it was. If the timed out packet appears to be the + // one that is matching the server response, we still want to + // remove it, but we need to update the 'next sent' pointer to + // point to a valid location. + if (sent_packets_.template project<0>(it) != next_sent_) { eraseSent(sent_packets_.template project<0>(it)); } else { - // Removal may invalidate the next_sent_ - // pointer if it points to the packet being - // removed. So, we set the next_sent_ to point - // to the next packet after removed one. next_sent_ = eraseSent(sent_packets_.template project<0>(it)); + // We removed the matching packet because of the timeout. It + // means that there is no match anymore. + packet_found = false; } ++collected_; } diff --git a/src/bin/perfdhcp/tests/stats_mgr_unittest.cc b/src/bin/perfdhcp/tests/stats_mgr_unittest.cc index 7df1f12457..d725543137 100644 --- a/src/bin/perfdhcp/tests/stats_mgr_unittest.cc +++ b/src/bin/perfdhcp/tests/stats_mgr_unittest.cc @@ -13,13 +13,13 @@ #include #include #include +#include "../stats_mgr.h" #include +#include #include -#include "../stats_mgr.h" - using namespace std; using namespace isc; using namespace isc::dhcp; @@ -32,6 +32,37 @@ typedef StatsMgr StatsMgr6; const uint32_t common_transid = 123; +/// @brief Number of packets to be used for testing packets collecting. +const size_t TEST_COLLECTED_PKT_NUM = 10; + +/// @brief DHCPv4 packet with modifiable internal values. +/// +/// Currently the only additional modifiable value is a packet +/// timestamp. +class Pkt4Modifiable : public Pkt4 { +public: + + /// @brief Constructor. + /// + /// @param msg_type DHCPv4 message type. + /// @param transid Transaction id. + Pkt4Modifiable(const uint8_t msg_type, const uint32_t transid) + : Pkt4(msg_type, transid) { + } + + /// @brief Modifies packet timestamp. + /// + /// @param delta Number of seconds to be added to the current + /// packet time. If this number is negative, the new timestamp + /// will point to earlier time than the original timestamp. + void modifyTimestamp(const long delta) { + timestamp_ += boost::posix_time::seconds(delta); + } +}; + +/// @brief Pointer to the Pkt4Modifiable. +typedef boost::shared_ptr Pkt4ModifiablePtr; + class StatsMgrTest : public ::testing::Test { public: StatsMgrTest() { @@ -44,15 +75,15 @@ public: /// \param msg_type DHCPv4 message type. /// \param transid transaction id for the packet. /// \return DHCPv4 packet. - Pkt4* createPacket4(const uint8_t msg_type, - const uint32_t transid) { - Pkt4* pkt = new Pkt4(msg_type, transid); + Pkt4Modifiable* createPacket4(const uint8_t msg_type, + const uint32_t transid) { + Pkt4Modifiable* pkt = new Pkt4Modifiable(msg_type, transid); // Packet timestamp is normally updated by interface // manager on packets reception or send. Unit tests // do not use interface manager so we need to do it // ourselves. pkt->updateTimestamp(); - return pkt; + return (pkt); } /// \brief Create DHCPv6 packet. @@ -151,6 +182,56 @@ public: ASSERT_FALSE(rcvd_time.is_not_a_date_time()); } + /// @brief This test verifies that timed out packets are collected. + /// + /// @param transid_index Index in the table of transaction ids which + /// points to the transaction id to be selected for the DHCPOFFER. + void testSendReceiveCollected(const size_t transid_index) { + boost::scoped_ptr stats_mgr(new StatsMgr4()); + // The second parameter indicates that transactions older than + // 2 seconds should be removed and respective packets collected. + stats_mgr->addExchangeStats(StatsMgr4::XCHG_DO, 2); + + // Transaction ids of packets to be sent. All transaction ids + // belong to the same bucket (match the transid & 1023 hashing + // function). + uint32_t transid[TEST_COLLECTED_PKT_NUM] = + { 1, 1025, 2049, 3073, 4097, 5121, 6145, 7169, 8193, 9217 }; + + // Simulate sending a number of packets. + for (unsigned int i = 0; i < TEST_COLLECTED_PKT_NUM; ++i) { + Pkt4ModifiablePtr sent_packet(createPacket4(DHCPDISCOVER, + transid[i])); + // For packets with even index of the transaction id we set + // the packet timestamp to 10s in the past. When DHCPOFFER + // is processed, the packets with timestamps older than + // 2s should be collected. + if (i % 2 == 0) { + sent_packet->modifyTimestamp(-10); + } + ASSERT_NO_THROW( + stats_mgr->passSentPacket(StatsMgr4::XCHG_DO, sent_packet) + ); + } + + // Create a server response for one of the packets sent. + Pkt4ModifiablePtr rcvd_packet(createPacket4(DHCPOFFER, + transid[transid_index])); + ASSERT_NO_THROW( + stats_mgr->passRcvdPacket(StatsMgr4::XCHG_DO, rcvd_packet); + ); + + // There is exactly one case (transaction id) for which perfdhcp + // will find a packet using ordered lookup. In this case, no + // packets will be collected. Otherwise, for any unordered lookup + // all packets from a bucket should be collected. + if (stats_mgr->getUnorderedLookups(StatsMgr4::XCHG_DO) > 0) { + // All packets in the bucket having even transaction id + // indexes should be removed. + EXPECT_EQ(TEST_COLLECTED_PKT_NUM / 2, + stats_mgr->getCollectedNum(StatsMgr4::XCHG_DO)); + } + } }; TEST_F(StatsMgrTest, Constructor) { @@ -339,6 +420,14 @@ TEST_F(StatsMgrTest, SendReceiveUnordered) { EXPECT_EQ(9, stats_mgr->getUnorderedLookups(StatsMgr4::XCHG_DO)); } +TEST_F(StatsMgrTest, SendReceiveCollectedHighTransid) { + // Check that the packet collection mechanism works fine + // for any packet returned by the server. + for (unsigned int i = 0; i < TEST_COLLECTED_PKT_NUM; ++i) { + testSendReceiveCollected(i); + } +} + TEST_F(StatsMgrTest, Orphans) { const int packets_num = 6; boost::scoped_ptr stats_mgr(new StatsMgr4());