[master] Merge branch 'trac4493'

src/bin/perfdhcp/stats_mgr.h

@@ -14,7 +14,7 @@
 #include <boost/noncopyable.hpp>
 #include <boost/shared_ptr.hpp>
 #include <boost/multi_index_container.hpp>
-#include <boost/multi_index/hashed_index.hpp>
+#include <boost/multi_index/ordered_index.hpp>
 #include <boost/multi_index/sequenced_index.hpp>
 #include <boost/multi_index/global_fun.hpp>
 #include <boost/multi_index/mem_fun.hpp>
@@ -202,7 +202,7 @@ public:
         ///      }
         /// \endcode
         ///
-        /// Example 3: Access elements through hashed index
+        /// Example 3: Access elements through ordered index by hash
         /// \code
         /// // Get the instance of the second search index.
         /// PktListTransidHashIndex& idx = sent_packets_.template get<1>();
@@ -226,7 +226,10 @@ public:
                 // in the same way as std::list.
                 boost::multi_index::sequenced<>,
                 // The other index keeps products of transaction id.
-                boost::multi_index::hashed_non_unique<
+                // Elements with the same hash value are grouped together
+                // into buckets and transactions are ordered from the
+                // oldest to latest within a bucket.
+                boost::multi_index::ordered_non_unique<
                     // Specify hash function to get the product of
                     // transaction id. This product is obtained by calling
                     // hashTransid() function.
@@ -442,46 +445,72 @@ public:
                 // bucket size the better. If bucket sizes appear to big we
                 // might want to increase number of buckets.
                 unordered_lookup_size_sum_ += std::distance(p.first, p.second);
+                bool non_expired_found = false;
                 // Removal can be done only after the loop
                 PktListRemovalQueue to_remove;
                 for (PktListTransidHashIterator it = p.first; it != p.second;
                      ++it) {
-                    if ((*it)->getTransid() == rcvd_packet->getTransid()) {
+                    // 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 (!packet_found && ((*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);
                     }
-                    ptime now = microsec_clock::universal_time();
-                    ptime packet_time = (*it)->getTimestamp();
-                    time_period packet_period(packet_time, now);
-                    if (!packet_period.is_null()) {
-                        double period_fractional =
-                            packet_period.length().total_seconds() +
-                            (static_cast<double>(packet_period.length().fractional_seconds())
-                             / packet_period.length().ticks_per_second());
-                        if (drop_time_ > 0 && (period_fractional > drop_time_)) {
-                            // Push the iterator on the removal queue.
-                            to_remove.push(it);
 
+                    if (!non_expired_found) {
+                        // 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);
+                        if (!packet_period.is_null()) {
+                            double period_fractional =
+                                packet_period.length().total_seconds() +
+                                (static_cast<double>(packet_period.length().fractional_seconds())
+                                 / packet_period.length().ticks_per_second());
+                            if (drop_time_ > 0 && (period_fractional > drop_time_)) {
+                                // Push the iterator on the removal queue.
+                                to_remove.push(it);
+
+                            } else {
+                                // We found first non-expired transaction. All other
+                                // transactions within this bucket are considered
+                                // non-expired because packets are held in the
+                                // order of addition within the bucket.
+                                non_expired_found = true;
+                            }
                         }
                     }
+
+                    // If we found the packet and all expired transactions,
+                    // there is nothing more to do.
+                    if (non_expired_found && packet_found) {
+                        break;
+                    }
                 }
 
-                // 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_;
                 }

src/bin/perfdhcp/test_control.cc

@@ -27,6 +27,7 @@
 #include <stdint.h>
 #include <unistd.h>
 #include <signal.h>
+#include <sstream>
 #include <sys/wait.h>
 
 using namespace std;
@@ -970,22 +971,37 @@ void
 TestControl::printRate() const {
     double rate = 0;
     CommandOptions& options = CommandOptions::instance();
+    std::string exchange_name = "4-way exchanges";
     if (options.getIpVersion() == 4) {
+        StatsMgr4::ExchangeType xchg_type =
+            options.getExchangeMode() == CommandOptions::DO_SA ?
+            StatsMgr4::XCHG_DO : StatsMgr4::XCHG_RA;
+        if (xchg_type == StatsMgr4::XCHG_DO) {
+            exchange_name = "DISCOVER-OFFER";
+        }
         double duration =
             stats_mgr4_->getTestPeriod().length().total_nanoseconds() / 1e9;
-        rate = stats_mgr4_->getRcvdPacketsNum(StatsMgr4::XCHG_DO) / duration;
+        rate = stats_mgr4_->getRcvdPacketsNum(xchg_type) / duration;
     } else if (options.getIpVersion() == 6) {
+        StatsMgr6::ExchangeType xchg_type =
+            options.getExchangeMode() == CommandOptions::DO_SA ?
+            StatsMgr6::XCHG_SA : StatsMgr6::XCHG_RR;
+        if (xchg_type == StatsMgr6::XCHG_SA) {
+            exchange_name = options.isRapidCommit() ? "Solicit-Reply" :
+                "Solicit-Advertise";
+        }
         double duration =
             stats_mgr6_->getTestPeriod().length().total_nanoseconds() / 1e9;
-        rate = stats_mgr6_->getRcvdPacketsNum(StatsMgr6::XCHG_SA) / duration;
+        rate = stats_mgr6_->getRcvdPacketsNum(xchg_type) / duration;
     }
-    std::cout << "***Rate statistics***" << std::endl;
+    std::ostringstream s;
+    s << "***Rate statistics***" << std::endl;
+    s << "Rate: " << rate << " " << exchange_name << "/second";
     if (options.getRate() > 0) {
-        std::cout << "Rate: " << rate << " exchanges/second, expected rate: "
-                  << options.getRate() << " exchanges/second" <<  std::endl << std::endl;
-    } else {
-        std::cout << "Rate: " << rate << std::endl << std::endl;
+        s << ", expected rate: " << options.getRate() << std::endl;
     }
+
+    std::cout << s.str() << std::endl;
 }
 
 void

src/bin/perfdhcp/tests/stats_mgr_unittest.cc

@@ -13,13 +13,13 @@
 #include <dhcp/dhcp6.h>
 #include <dhcp/pkt4.h>
 #include <dhcp/pkt6.h>
+#include "../stats_mgr.h"
 
 #include <gtest/gtest.h>
 
+#include <boost/date_time/posix_time/posix_time.hpp>
 #include <boost/scoped_ptr.hpp>
 
-#include "../stats_mgr.h"
-
 using namespace std;
 using namespace isc;
 using namespace isc::dhcp;
@@ -32,6 +32,37 @@ typedef StatsMgr<dhcp::Pkt6> 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<Pkt4Modifiable> 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,83 @@ 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<StatsMgr4> 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 low indexes we set the timestamps to
+            // 10s in the past. When DHCPOFFER is processed, the
+            // packets with timestamps older than 2s should be collected.
+            if (i < TEST_COLLECTED_PKT_NUM / 2) {
+                sent_packet->modifyTimestamp(-10);
+            }
+            ASSERT_NO_THROW(
+                stats_mgr->passSentPacket(StatsMgr4::XCHG_DO, sent_packet)
+            )  << "failure for transaction id " << transid[i];
+        }
+
+        // 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 transaction id
+            // index below TEST_COLLECTED_PKT_NUM / 2 should be removed.
+            EXPECT_EQ(TEST_COLLECTED_PKT_NUM / 2,
+                      stats_mgr->getCollectedNum(StatsMgr4::XCHG_DO));
+        }
+
+        // Make sure that we can still use the StatsMgr. It is possible
+        // that the pointer to 'next sent' packet was invalidated
+        // during packet removal.
+        for (unsigned int i = 0; i < TEST_COLLECTED_PKT_NUM; ++i) {
+            // Increase transaction ids by 1 so as they don't duplicate
+            // with transaction ids of already sent packets.
+            Pkt4ModifiablePtr sent_packet(createPacket4(DHCPDISCOVER,
+                                                    transid[i] + 1));
+            Pkt4ModifiablePtr rcvd_packet(createPacket4(DHCPOFFER,
+                                                        transid[i] + 1));
+            ASSERT_NO_THROW(
+                stats_mgr->passSentPacket(StatsMgr4::XCHG_DO, sent_packet)
+            ) << "failure for transaction id " << transid[i];
+
+            ASSERT_NO_THROW(
+                stats_mgr->passRcvdPacket(StatsMgr4::XCHG_DO, rcvd_packet);
+            ) << "failure for transaction id " << transid[i];
+        }
+
+        // We should have processed TEST_COLLECTED_PKT_NUM but it is possible
+        // that one of them we couldn't match (orphan packet), because
+        // the matched packet had to be collected because of the transaction
+        // timeout. Therefore, we have to count both received packets and
+        // orphans.
+        EXPECT_EQ(TEST_COLLECTED_PKT_NUM + 1,
+                  stats_mgr->getRcvdPacketsNum(StatsMgr4::XCHG_DO) +
+                  stats_mgr->getOrphans(StatsMgr4::XCHG_DO));
+    }
 };
 
 TEST_F(StatsMgrTest, Constructor) {
@@ -339,6 +447,14 @@ TEST_F(StatsMgrTest, SendReceiveUnordered) {
     EXPECT_EQ(9, stats_mgr->getUnorderedLookups(StatsMgr4::XCHG_DO));
 }
 
+TEST_F(StatsMgrTest, SendReceiveCollected) {
+    // 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<StatsMgr4> stats_mgr(new StatsMgr4());