From bd46c2c0b620bb92739fdd59e83e6c8c87a48a18 Mon Sep 17 00:00:00 2001 From: Eduardo Almeida Date: Fri, 11 Feb 2022 14:34:58 +0000 Subject: [PATCH] tcp (fixes #531): TcpWestwood divide-by-zero and floating point issues - Change TcpWestwood::EstimateBW trace source from double to DataRate. - Prevent divide-by-zero from occurring. - Remove TcpWestwood from tcp-rto-test.cc, tcp-cong-avoid-test.cc, tcp-slow-start-test.cc. - Add warning that currently TcpWestwood does not have unit tests. --- src/internet/doc/tcp.rst | 10 ++++--- src/internet/model/tcp-westwood.cc | 35 ++++++++++++------------ src/internet/model/tcp-westwood.h | 14 ++++------ src/internet/test/tcp-cong-avoid-test.cc | 2 -- src/internet/test/tcp-rto-test.cc | 2 -- src/internet/test/tcp-slow-start-test.cc | 2 -- 6 files changed, 30 insertions(+), 35 deletions(-) diff --git a/src/internet/doc/tcp.rst b/src/internet/doc/tcp.rst index 04585ce9f..7efe917f7 100644 --- a/src/internet/doc/tcp.rst +++ b/src/internet/doc/tcp.rst @@ -547,13 +547,15 @@ More information at: http://dl.acm.org/citation.cfm?id=2756518 Westwood ^^^^^^^^ -Westwood and Westwood+ employ the AIAD (Additive Increase/Adaptive Decrease)· -congestion control paradigm. When a congestion episode happens,· +Westwood and Westwood+ employ the AIAD (Additive Increase/Adaptive Decrease) +congestion control paradigm. When a congestion episode happens, instead of halving the cwnd, these protocols try to estimate the network's -bandwidth and use the estimated value to adjust the cwnd.· -While Westwood performs the bandwidth sampling every ACK reception,· +bandwidth and use the estimated value to adjust the cwnd. +While Westwood performs the bandwidth sampling every ACK reception, Westwood+ samples the bandwidth every RTT. +WARNING: this TCP model lacks validation and regression tests; use with caution. + More information at: http://dl.acm.org/citation.cfm?id=381704 and http://dl.acm.org/citation.cfm?id=2512757 diff --git a/src/internet/model/tcp-westwood.cc b/src/internet/model/tcp-westwood.cc index d1808a0e4..18c2331a8 100644 --- a/src/internet/model/tcp-westwood.cc +++ b/src/internet/model/tcp-westwood.cc @@ -59,7 +59,7 @@ TcpWestwood::GetTypeId (void) MakeEnumChecker(TcpWestwood::WESTWOOD, "Westwood",TcpWestwood::WESTWOODPLUS, "WestwoodPlus")) .AddTraceSource("EstimatedBW", "The estimated bandwidth", MakeTraceSourceAccessor(&TcpWestwood::m_currentBW), - "ns3::TracedValueCallback::Double") + "ns3::TracedValueCallback::DataRate") ; return tid; } @@ -130,34 +130,34 @@ TcpWestwood::EstimateBW (const Time &rtt, Ptr tcb) NS_ASSERT (!rtt.IsZero ()); - m_currentBW = m_ackedSegments * tcb->m_segmentSize / rtt.GetSeconds (); - if (m_pType == TcpWestwood::WESTWOOD) { Time currentAck = Simulator::Now (); - m_currentBW = m_ackedSegments * tcb->m_segmentSize / (currentAck - m_lastAck).GetSeconds (); + + NS_ABORT_MSG_IF (currentAck == m_lastAck, + "This violates a model assumption and would lead to divide-by-zero; please report to ns-3 maintainers if this occurs."); + + m_currentBW = DataRate (m_ackedSegments * tcb->m_segmentSize * 8.0 / (currentAck - m_lastAck).GetSeconds ()); m_lastAck = currentAck; } else if (m_pType == TcpWestwood::WESTWOODPLUS) { - m_currentBW = m_ackedSegments * tcb->m_segmentSize / rtt.GetSeconds (); + m_currentBW = DataRate (m_ackedSegments * tcb->m_segmentSize * 8.0 / rtt.GetSeconds ()); m_IsCount = false; } m_ackedSegments = 0; + NS_LOG_LOGIC ("Estimated BW: " << m_currentBW); // Filter the BW sample - double alpha = 0.9; + constexpr double alpha = 0.9; - if (m_fType == TcpWestwood::NONE) + if (m_fType == TcpWestwood::TUSTIN) { - } - else if (m_fType == TcpWestwood::TUSTIN) - { - double sample_bwe = m_currentBW; - m_currentBW = (alpha * m_lastBW) + ((1 - alpha) * ((sample_bwe + m_lastSampleBW) / 2)); + DataRate sample_bwe = m_currentBW; + m_currentBW = (m_lastBW * alpha) + (((sample_bwe + m_lastSampleBW) * 0.5) * (1 - alpha)); m_lastSampleBW = sample_bwe; m_lastBW = m_currentBW; } @@ -169,12 +169,13 @@ uint32_t TcpWestwood::GetSsThresh (Ptr tcb, [[maybe_unused]] uint32_t bytesInFlight) { - NS_LOG_LOGIC ("CurrentBW: " << m_currentBW << " minRtt: " << - tcb->m_minRtt << " ssthresh: " << - m_currentBW * static_cast (tcb->m_minRtt.GetSeconds ())); + uint32_t ssThresh = static_cast ((m_currentBW * tcb->m_minRtt) / 8.0); - return std::max (2*tcb->m_segmentSize, - uint32_t (m_currentBW * static_cast (tcb->m_minRtt.GetSeconds ()))); + NS_LOG_LOGIC ("CurrentBW: " << m_currentBW << + " minRtt: " << tcb->m_minRtt << + " ssThresh: " << ssThresh); + + return std::max (2 * tcb->m_segmentSize, ssThresh); } Ptr diff --git a/src/internet/model/tcp-westwood.h b/src/internet/model/tcp-westwood.h index 39a5b9be6..17715a4f8 100644 --- a/src/internet/model/tcp-westwood.h +++ b/src/internet/model/tcp-westwood.h @@ -34,17 +34,14 @@ #define TCP_WESTWOOD_H #include "tcp-congestion-ops.h" +#include "ns3/data-rate.h" #include "ns3/tcp-recovery-ops.h" -#include "ns3/sequence-number.h" #include "ns3/traced-value.h" #include "ns3/event-id.h" namespace ns3 { -class Packet; -class TcpHeader; class Time; -class EventId; /** * \ingroup congestionOps @@ -63,6 +60,8 @@ class EventId; * the number of acknowledged segments on the receipt of an ACK. * The EstimateBW estimates the bandwidth based on the value returned by CountAck * and the sampling interval (last ACK inter-arrival time for Westwood and last RTT for Westwood+). + * + * WARNING: this TCP model lacks validation and regression tests; use with caution. */ class TcpWestwood : public TcpNewReno { @@ -124,9 +123,9 @@ private: void EstimateBW (const Time& rtt, Ptr tcb); protected: - TracedValue m_currentBW; //!< Current value of the estimated BW - double m_lastSampleBW; //!< Last bandwidth sample - double m_lastBW; //!< Last bandwidth sample after being filtered + TracedValue m_currentBW; //!< Current value of the estimated BW + DataRate m_lastSampleBW; //!< Last bandwidth sample + DataRate m_lastBW; //!< Last bandwidth sample after being filtered enum ProtocolType m_pType; //!< 0 for Westwood, 1 for Westwood+ enum FilterType m_fType; //!< 0 for none, 1 for Tustin @@ -134,7 +133,6 @@ protected: bool m_IsCount; //!< Start keeping track of m_ackedSegments for Westwood+ if TRUE EventId m_bwEstimateEvent; //!< The BW estimation event for Westwood+ Time m_lastAck; //!< The last ACK time - }; } // namespace ns3 diff --git a/src/internet/test/tcp-cong-avoid-test.cc b/src/internet/test/tcp-cong-avoid-test.cc index 0f85edc3d..72185a6f3 100644 --- a/src/internet/test/tcp-cong-avoid-test.cc +++ b/src/internet/test/tcp-cong-avoid-test.cc @@ -18,7 +18,6 @@ */ #include "ns3/log.h" #include "ns3/simple-channel.h" -#include "ns3/tcp-westwood.h" #include "ns3/config.h" #include "ns3/test.h" #include "tcp-general-test.h" @@ -194,7 +193,6 @@ public: TcpRenoCongAvoidTestSuite () : TestSuite ("tcp-cong-avoid-test", UNIT) { std::list types; - types.insert (types.begin (), TcpWestwood::GetTypeId ()); types.insert (types.begin (), TcpNewReno::GetTypeId ()); for (std::list::iterator it = types.begin (); it != types.end (); ++it) diff --git a/src/internet/test/tcp-rto-test.cc b/src/internet/test/tcp-rto-test.cc index 3b4a21087..00cc1d217 100644 --- a/src/internet/test/tcp-rto-test.cc +++ b/src/internet/test/tcp-rto-test.cc @@ -19,7 +19,6 @@ #include "ns3/node.h" #include "ns3/log.h" -#include "ns3/tcp-westwood.h" #include "ns3/simple-channel.h" #include "ns3/rtt-estimator.h" #include "tcp-general-test.h" @@ -517,7 +516,6 @@ public: { std::list types; types.insert (types.begin (), TcpNewReno::GetTypeId ()); - types.insert (types.begin (), TcpWestwood::GetTypeId ()); for (std::list::iterator it = types.begin (); it != types.end (); ++it) { diff --git a/src/internet/test/tcp-slow-start-test.cc b/src/internet/test/tcp-slow-start-test.cc index 235e61b6d..c63670b20 100644 --- a/src/internet/test/tcp-slow-start-test.cc +++ b/src/internet/test/tcp-slow-start-test.cc @@ -21,7 +21,6 @@ #include "ns3/simple-channel.h" #include "ns3/node.h" #include "ns3/config.h" -#include "ns3/tcp-westwood.h" #include "ns3/tcp-header.h" #include "tcp-general-test.h" @@ -275,7 +274,6 @@ public: // This test have less packets to transmit than SsTh std::list types; types.insert (types.begin (), TcpNewReno::GetTypeId ()); - types.insert (types.begin (), TcpWestwood::GetTypeId ()); for (std::list::iterator it = types.begin (); it != types.end (); ++it) {