changed the reordering logic

microsoft · Dec 11, 2024 · 1d1e130 · 1d1e130
1 parent 6191f1f
commit 1d1e130
Show file tree

Hide file tree

Showing 3 changed files with 52 additions and 43 deletions.
diff --git a/src/core/ack_tracker.c b/src/core/ack_tracker.c
@@ -99,21 +99,38 @@ QuicAckTrackerAddPacketNumber(
 BOOLEAN
 QuicAckTrackerDidHitReorderingThreshold(
     _In_ QUIC_ACK_TRACKER* Tracker,
-    _In_ uint8_t ReorderingThreshold,
-    _In_ uint64_t PacketNumber
+    _In_ uint8_t ReorderingThreshold
     )
 {
-    if (ReorderingThreshold == 0 || 
-        QuicRangeSize(&Tracker->PacketNumbersToAck) <= 1 ||
-        PacketNumber != QuicRangeGetMax(&Tracker->PacketNumbersToAck)) { // There are more than two ranges, i.e. a gap somewhere.
+    if (ReorderingThreshold == 0) {
         return FALSE;
     }
 
     uint64_t LargestUnackedPacketNumber = QuicRangeGetMax(&Tracker->PacketNumbersToAck);
-    uint64_t SmallestUnreportedMissingPacketNumber = 
-        QuicRangeGetHigh(
-            QuicRangeGet(&Tracker->PacketNumbersToAck, 0)) + 1;
-
+    uint64_t LargestReported = 0; // The largest packet number that could be declared lost 
+    uint64_t SmallestUnreportedMissingPacketNumber; 
+
+    if (Tracker->LargestPacketNumberAcknowledged >= ReorderingThreshold) {
+        LargestReported = Tracker->LargestPacketNumberAcknowledged - ReorderingThreshold + 1;
+    }
+
+    if (LargestReported > LargestUnackedPacketNumber) {
+        return FALSE;
+    }
+
+    QUIC_RANGE_SEARCH_KEY Key = { LargestReported, LargestReported };
+    int result = QuicRangeSearch(&Tracker->PacketNumbersToAck, &Key);
+    if (result >= 0 && result < (int)QuicRangeSize(&Tracker->PacketNumbersToAck) - 1) {
+        SmallestUnreportedMissingPacketNumber = 
+            QuicRangeGetHigh(QuicRangeGet(&Tracker->PacketNumbersToAck, result)) + 1;
+    }
+    else if (result < 0) {
+        SmallestUnreportedMissingPacketNumber = LargestReported;
+    }
+    else {
+        return FALSE;
+    }
+
     return LargestUnackedPacketNumber - SmallestUnreportedMissingPacketNumber >= ReorderingThreshold;
 }
 
@@ -193,7 +210,7 @@ QuicAckTrackerAckPacket(
 
     Tracker->AckElicitingPacketsToAcknowledge++;
 
-    if (Connection->Send.SendFlags & QUIC_CONN_SEND_FLAG_ACK) {
+    if ((Connection->Send.SendFlags & QUIC_CONN_SEND_FLAG_ACK) || !NewLargestPacketNumber) {
         goto Exit; // Already queued to send an ACK, no more work to do.
     }
 
@@ -216,7 +233,7 @@ QuicAckTrackerAckPacket(
     if (AckType == QUIC_ACK_TYPE_ACK_IMMEDIATE ||
         Connection->Settings.MaxAckDelayMs == 0 ||
         (Tracker->AckElicitingPacketsToAcknowledge >= (uint16_t)Connection->PacketTolerance) ||
-        QuicAckTrackerDidHitReorderingThreshold(Tracker, Connection->ReorderingThreshold, PacketNumber)) {
+        QuicAckTrackerDidHitReorderingThreshold(Tracker, Connection->ReorderingThreshold)) {
         //
         // Send the ACK immediately.
         //

diff --git a/src/core/ack_tracker.h b/src/core/ack_tracker.h
@@ -96,8 +96,7 @@ QuicAckTrackerAddPacketNumber(
 BOOLEAN
 QuicAckTrackerDidHitReorderingThreshold(
     _In_ QUIC_ACK_TRACKER* Tracker,
-    _In_ uint8_t ReorderingThreshold,
-    _In_ uint64_t PacketNumber
+    _In_ uint8_t ReorderingThreshold
     );
 
 typedef enum QUIC_ACK_TYPE {

diff --git a/src/core/unittest/FrameTest.cpp b/src/core/unittest/FrameTest.cpp
@@ -255,73 +255,66 @@ TEST(FrameTest, TestQuicAckTrackerDidHitReorderingThreshold)
     // ReorderingThreshold is 0
     ReorderingThreshold = 0;
     QuicRangeAddValue(&Tracker.PacketNumbersToAck, 100);
-    ASSERT_FALSE(QuicAckTrackerDidHitReorderingThreshold(&Tracker, ReorderingThreshold, 100));
-
-    // The number of ranges is less than 2.
-    ReorderingThreshold = 3;
-    QuicRangeAddValue(&Tracker.PacketNumbersToAck, 101);
-    ASSERT_FALSE(QuicAckTrackerDidHitReorderingThreshold(&Tracker, ReorderingThreshold, 101));
-
-    // PacketNumber is not the largest unacked packet number
-    QuicRangeAddValue(&Tracker.PacketNumbersToAck, 104);
-    QuicRangeAddValue(&Tracker.PacketNumbersToAck, 105);
-    ASSERT_FALSE(QuicAckTrackerDidHitReorderingThreshold(&Tracker, ReorderingThreshold, 103));
+    ASSERT_FALSE(QuicAckTrackerDidHitReorderingThreshold(&Tracker, ReorderingThreshold));
 
     // Case 1 
+    ReorderingThreshold = 3;
     QuicAckTrackerReset(&Tracker); 
     QuicRangeAddValue(&Tracker.PacketNumbersToAck, 0);
-    ASSERT_FALSE(QuicAckTrackerDidHitReorderingThreshold(&Tracker, ReorderingThreshold, 0));
+    ASSERT_FALSE(QuicAckTrackerDidHitReorderingThreshold(&Tracker, ReorderingThreshold));
 
     QuicRangeAddValue(&Tracker.PacketNumbersToAck, 1);
-    ASSERT_FALSE(QuicAckTrackerDidHitReorderingThreshold(&Tracker, ReorderingThreshold, 1));
+    ASSERT_FALSE(QuicAckTrackerDidHitReorderingThreshold(&Tracker, ReorderingThreshold));
 
     QuicRangeAddValue(&Tracker.PacketNumbersToAck, 3);
-    ASSERT_FALSE(QuicAckTrackerDidHitReorderingThreshold(&Tracker, ReorderingThreshold, 3));
+    ASSERT_FALSE(QuicAckTrackerDidHitReorderingThreshold(&Tracker, ReorderingThreshold));
 
     QuicRangeAddValue(&Tracker.PacketNumbersToAck, 4);
-    ASSERT_FALSE(QuicAckTrackerDidHitReorderingThreshold(&Tracker, ReorderingThreshold, 4));
+    ASSERT_FALSE(QuicAckTrackerDidHitReorderingThreshold(&Tracker, ReorderingThreshold));
 
     QuicRangeAddValue(&Tracker.PacketNumbersToAck, 5);
-    ASSERT_TRUE(QuicAckTrackerDidHitReorderingThreshold(&Tracker, ReorderingThreshold, 5));
-    QuicRangeAddValue(&Tracker.PacketNumbersToAck, 2); // 2 is reported missing
+    ASSERT_TRUE(QuicAckTrackerDidHitReorderingThreshold(&Tracker, ReorderingThreshold));
+    Tracker.LargestPacketNumberAcknowledged = 5;
 
     QuicRangeAddValue(&Tracker.PacketNumbersToAck, 8);
-    ASSERT_FALSE(QuicAckTrackerDidHitReorderingThreshold(&Tracker, ReorderingThreshold, 8));
+    ASSERT_FALSE(QuicAckTrackerDidHitReorderingThreshold(&Tracker, ReorderingThreshold));
 
     QuicRangeAddValue(&Tracker.PacketNumbersToAck, 9);
-    ASSERT_TRUE(QuicAckTrackerDidHitReorderingThreshold(&Tracker, ReorderingThreshold, 9));
-    QuicRangeAddValue(&Tracker.PacketNumbersToAck, 6); // 6 is reported missing
+    ASSERT_TRUE(QuicAckTrackerDidHitReorderingThreshold(&Tracker, ReorderingThreshold));
+    Tracker.LargestPacketNumberAcknowledged = 9;
 
     QuicRangeAddValue(&Tracker.PacketNumbersToAck, 10);
-    ASSERT_TRUE(QuicAckTrackerDidHitReorderingThreshold(&Tracker, ReorderingThreshold, 10));
+    ASSERT_TRUE(QuicAckTrackerDidHitReorderingThreshold(&Tracker, ReorderingThreshold));
+    Tracker.LargestPacketNumberAcknowledged = 10;
 
     // Case 2
     ReorderingThreshold = 5;
     QuicAckTrackerReset(&Tracker); 
     QuicRangeAddValue(&Tracker.PacketNumbersToAck, 0);
-    ASSERT_FALSE(QuicAckTrackerDidHitReorderingThreshold(&Tracker, ReorderingThreshold, 0));
+    ASSERT_FALSE(QuicAckTrackerDidHitReorderingThreshold(&Tracker, ReorderingThreshold));
 
     QuicRangeAddValue(&Tracker.PacketNumbersToAck, 1);
-    ASSERT_FALSE(QuicAckTrackerDidHitReorderingThreshold(&Tracker, ReorderingThreshold, 1));
+    ASSERT_FALSE(QuicAckTrackerDidHitReorderingThreshold(&Tracker, ReorderingThreshold));
 
     QuicRangeAddValue(&Tracker.PacketNumbersToAck, 3);
-    ASSERT_FALSE(QuicAckTrackerDidHitReorderingThreshold(&Tracker, ReorderingThreshold, 3));
+    ASSERT_FALSE(QuicAckTrackerDidHitReorderingThreshold(&Tracker, ReorderingThreshold));
 
     QuicRangeAddValue(&Tracker.PacketNumbersToAck, 5);
-    ASSERT_FALSE(QuicAckTrackerDidHitReorderingThreshold(&Tracker, ReorderingThreshold, 5));
+    ASSERT_FALSE(QuicAckTrackerDidHitReorderingThreshold(&Tracker, ReorderingThreshold));
 
     QuicRangeAddValue(&Tracker.PacketNumbersToAck, 6);
-    ASSERT_FALSE(QuicAckTrackerDidHitReorderingThreshold(&Tracker, ReorderingThreshold, 6));
+    ASSERT_FALSE(QuicAckTrackerDidHitReorderingThreshold(&Tracker, ReorderingThreshold));
 
     QuicRangeAddValue(&Tracker.PacketNumbersToAck, 7);
-    ASSERT_TRUE(QuicAckTrackerDidHitReorderingThreshold(&Tracker, ReorderingThreshold, 7));
-    QuicRangeAddValue(&Tracker.PacketNumbersToAck, 2); // 2 is reported missing
+    ASSERT_TRUE(QuicAckTrackerDidHitReorderingThreshold(&Tracker, ReorderingThreshold));
+    Tracker.LargestPacketNumberAcknowledged = 7;
 
     QuicRangeAddValue(&Tracker.PacketNumbersToAck, 8);
-    ASSERT_FALSE(QuicAckTrackerDidHitReorderingThreshold(&Tracker, ReorderingThreshold, 8));
+    ASSERT_FALSE(QuicAckTrackerDidHitReorderingThreshold(&Tracker, ReorderingThreshold));
 
     QuicRangeAddValue(&Tracker.PacketNumbersToAck, 9);
-    ASSERT_TRUE(QuicAckTrackerDidHitReorderingThreshold(&Tracker, ReorderingThreshold, 9));
+    ASSERT_TRUE(QuicAckTrackerDidHitReorderingThreshold(&Tracker, ReorderingThreshold));
+    Tracker.LargestPacketNumberAcknowledged = 9;
 
 
     // Clean up