Updated ACK_FREQUENCY according to draft-ietf-quic-ack-frequency-10 (#…

…4670) * added reordering threshold in ack frequency * modified log filesd * few refactoring * minor refactoring * fixed clog * fixed clog * removed IgnoreOrder * removed IgnoreCE * updated names * updated the values * added logic for reordering * removed ignorereordering parameter in connection * added assert * updated logic for the reordering threshold function * minor change * added unit test * added more unit test cases * changed the reordering logic * minor refactoring * minor refactoring * changed variable names * modified way of implementing to be more optimised * addressed comments * resolved comments * modified variable name * changed the alogrithm logic * resolved comments * addressed comments * minor change * minor change to fix the pipeline * minor change * New Largest Packet Number check shifted
microsoft · Jan 3, 2025 · 5eb2b6a · 5eb2b6a
1 parent 453fb17
commit 5eb2b6a
Show file tree

Hide file tree

Showing 13 changed files with 228 additions and 109 deletions.
diff --git a/src/core/ack_tracker.c b/src/core/ack_tracker.c
@@ -96,6 +96,73 @@ QuicAckTrackerAddPacketNumber(
         !RangeUpdated;
 }
 
+//
+// This function implements the logic defined in Section 6.2 [draft-ietf-quic-frequence-10]
+// to determine if the reordering threshold has been hit.
+//
+BOOLEAN
+QuicAckTrackerDidHitReorderingThreshold(
+    _In_ QUIC_ACK_TRACKER* Tracker,
+    _In_ uint8_t ReorderingThreshold
+    )
+{
+    if (ReorderingThreshold == 0 || QuicRangeSize(&Tracker->PacketNumbersToAck) < 2) {
+        return FALSE;
+    }
+
+    const uint64_t LargestUnacked = QuicRangeGetMax(&Tracker->PacketNumbersToAck);
+    const uint64_t SmallestTracked = QuicRangeGet(&Tracker->PacketNumbersToAck, 0)->Low;
+
+    //
+    // Largest Reported is equal to the largest packet number acknowledged minus the 
+    // Reordering Threshold. If the difference between the largest packet number
+    // acknowledged and the Reordering Threshold is smaller than the smallest packet 
+    // in the ack tracker, then the largest reported is the smallest packet in the ack
+    // tracker.
+    //
+
+    const uint64_t LargestReported =
+        (Tracker->LargestPacketNumberAcknowledged >= SmallestTracked + ReorderingThreshold) ?
+            Tracker->LargestPacketNumberAcknowledged - ReorderingThreshold + 1 :
+            SmallestTracked;
+
+    //
+    // Loop through all previous ACK ranges (before last) to find the smallest missing
+    // packet number that is after the largest reported packet number. If the difference
+    // between that missing number and the largest unack'ed number is more than the
+    // reordering threshold, then the condition has been met to send an immediate
+    // acknowledgement.
+    //
+
+    for (uint32_t Index = QuicRangeSize(&Tracker->PacketNumbersToAck) - 1; Index > 0; --Index) {
+        const uint64_t RangeStart = QuicRangeGet(&Tracker->PacketNumbersToAck, Index)->Low;
+
+        if (LargestReported >= RangeStart) {
+            //
+            // Since we are only looking for packets more than LargestReported, we return
+            // false here.
+            //
+            return FALSE;
+        }
+
+        // 
+        // Check if largest reported packet is missing. In that case, the smallest missing 
+        // packet becomes the largest reported packet.
+        //
+
+        uint64_t PreviousSmallestMissing = QuicRangeGetHigh(QuicRangeGet(&Tracker->PacketNumbersToAck, Index - 1)) + 1;
+        if (LargestReported > PreviousSmallestMissing) {
+            PreviousSmallestMissing = LargestReported;
+        }
+
+        if (LargestUnacked - PreviousSmallestMissing >= ReorderingThreshold) {
+            return TRUE;
+        }
+    }
+
+    return FALSE;
+}
+
 _IRQL_requires_max_(DISPATCH_LEVEL)
 void
 QuicAckTrackerAckPacket(
@@ -182,10 +249,10 @@ QuicAckTrackerAckPacket(
     //   1. The packet included an IMMEDIATE_ACK frame.
     //   2. ACK delay is disabled (MaxAckDelayMs == 0).
     //   3. We have received 'PacketTolerance' ACK eliciting packets.
-    //   4. We received an ACK eliciting packet that doesn't directly follow the
-    //      previously received packet number. So we assume there might have
-    //      been loss and should indicate this info to the peer. This logic is
-    //      disabled if 'IgnoreReordering' is TRUE.
+    //   4. We have received an ACK eliciting packet that is out of order and the
+    //      gap between the smallest Unreported Missing packet and the Largest
+    //      Unacked is greater than or equal to the Reordering Threshold value. This logic is
+    //      disabled if the Reordering Threshold is 0.
     //   5. The delayed ACK timer fires after the configured time.
     //
     // If we don't queue an immediate ACK and this is the first ACK eliciting
@@ -195,12 +262,8 @@ QuicAckTrackerAckPacket(
     if (AckType == QUIC_ACK_TYPE_ACK_IMMEDIATE ||
         Connection->Settings.MaxAckDelayMs == 0 ||
         (Tracker->AckElicitingPacketsToAcknowledge >= (uint16_t)Connection->PacketTolerance) ||
-        (!Connection->State.IgnoreReordering &&
-         (NewLargestPacketNumber &&
-          QuicRangeSize(&Tracker->PacketNumbersToAck) > 1 && // There are more than two ranges, i.e. a gap somewhere.
-            QuicRangeGet(
-            &Tracker->PacketNumbersToAck,
-          QuicRangeSize(&Tracker->PacketNumbersToAck) - 1)->Count == 1))) { // The gap is right before the last packet number.
+        (NewLargestPacketNumber && 
+        QuicAckTrackerDidHitReorderingThreshold(Tracker, Connection->ReorderingThreshold))) {
         //
         // Send the ACK immediately.
         //

diff --git a/src/core/ack_tracker.h b/src/core/ack_tracker.h
@@ -5,6 +5,10 @@
 
 --*/
 
+#if defined(__cplusplus)
+extern "C" {
+#endif
+
 typedef struct QUIC_ACK_TRACKER {
 
     //
@@ -91,6 +95,12 @@ QuicAckTrackerAddPacketNumber(
     _In_ uint64_t PacketNumber
     );
 
+BOOLEAN
+QuicAckTrackerDidHitReorderingThreshold(
+    _In_ QUIC_ACK_TRACKER* Tracker,
+    _In_ uint8_t ReorderingThreshold
+    );
+
 typedef enum QUIC_ACK_TYPE {
     QUIC_ACK_TYPE_NON_ACK_ELICITING,
     QUIC_ACK_TYPE_ACK_ELICITING,
@@ -146,3 +156,7 @@ QuicAckTrackerHasPacketsToAck(
         !Tracker->AlreadyWrittenAckFrame &&
         QuicRangeSize(&Tracker->PacketNumbersToAck) != 0;
 }
+
+#if defined(__cplusplus)
+}
+#endif
diff --git a/src/core/connection.c b/src/core/connection.c
@@ -118,6 +118,8 @@ QuicConnAlloc(
     Connection->AckDelayExponent = QUIC_ACK_DELAY_EXPONENT;
     Connection->PacketTolerance = QUIC_MIN_ACK_SEND_NUMBER;
     Connection->PeerPacketTolerance = QUIC_MIN_ACK_SEND_NUMBER;
+    Connection->ReorderingThreshold = QUIC_MIN_REORDERING_THRESHOLD;
+    Connection->PeerReorderingThreshold = QUIC_MIN_REORDERING_THRESHOLD;
     Connection->PeerTransportParams.AckDelayExponent = QUIC_TP_ACK_DELAY_EXPONENT_DEFAULT;
     Connection->ReceiveQueueTail = &Connection->ReceiveQueue;
     QuicSettingsCopy(&Connection->Settings, &MsQuicLib.Settings);
@@ -5211,12 +5213,12 @@ QuicConnRecvFrames(
                 return FALSE;
             }
 
-            if (Frame.UpdateMaxAckDelay < MS_TO_US(MsQuicLib.TimerResolutionMs)) {
+            if (Frame.RequestedMaxAckDelay < MS_TO_US(MsQuicLib.TimerResolutionMs)) {
                 QuicTraceEvent(
                     ConnError,
                     "[conn][%p] ERROR, %s.",
                     Connection,
-                    "UpdateMaxAckDelay is less than TimerResolution");
+                    "RequestedMaxAckDelay is less than TimerResolution");
                 QuicConnTransportError(Connection, QUIC_ERROR_PROTOCOL_VIOLATION);
                 return FALSE;
             }
@@ -5231,20 +5233,24 @@ QuicConnRecvFrames(
             }
 
             Connection->NextRecvAckFreqSeqNum = Frame.SequenceNumber + 1;
-            Connection->State.IgnoreReordering = Frame.IgnoreOrder;
-            if (Frame.UpdateMaxAckDelay == 0) {
+            if (Frame.RequestedMaxAckDelay == 0) {
                 Connection->Settings.MaxAckDelayMs = 0;
-            } else if (Frame.UpdateMaxAckDelay < 1000) {
+            } else if (Frame.RequestedMaxAckDelay < 1000) {
                 Connection->Settings.MaxAckDelayMs = 1;
             } else {
-                CXPLAT_DBG_ASSERT(US_TO_MS(Frame.UpdateMaxAckDelay) <= UINT32_MAX);
-                Connection->Settings.MaxAckDelayMs = (uint32_t)US_TO_MS(Frame.UpdateMaxAckDelay);
+                CXPLAT_DBG_ASSERT(US_TO_MS(Frame.RequestedMaxAckDelay) <= UINT32_MAX);
+                Connection->Settings.MaxAckDelayMs = (uint32_t)US_TO_MS(Frame.RequestedMaxAckDelay);
             }
-            if (Frame.PacketTolerance < UINT8_MAX) {
-                Connection->PacketTolerance = (uint8_t)Frame.PacketTolerance;
+            if (Frame.AckElicitingThreshold < UINT8_MAX) {
+                Connection->PacketTolerance = (uint8_t)Frame.AckElicitingThreshold;
             } else {
                 Connection->PacketTolerance = UINT8_MAX; // Cap to 0xFF for space savings.
             }
+            if (Frame.ReorderingThreshold < UINT8_MAX) {
+                Connection->ReorderingThreshold = (uint8_t)Frame.ReorderingThreshold;
+            } else {
+                Connection->ReorderingThreshold = UINT8_MAX; // Cap to 0xFF for space savings.
+            }
             QuicTraceLogConnInfo(
                 UpdatePacketTolerance,
                 Connection,

diff --git a/src/core/connection.h b/src/core/connection.h
@@ -146,12 +146,6 @@ typedef union QUIC_CONNECTION_STATE {
         //
         BOOLEAN ResumptionEnabled : 1;
 
-        //
-        // When true,acknowledgment that reordering shouldn't elict an
-        // immediate acknowledgement.
-        //
-        BOOLEAN IgnoreReordering : 1;
-
         //
         // When true, this indicates that the connection is currently executing
         // an API call inline (from a reentrant call on a callback).
@@ -453,6 +447,21 @@ typedef struct QUIC_CONNECTION {
     //
     uint8_t PeerPacketTolerance;
 
+    //
+    // The maximum number of packets that can be out of order before an immediate
+    // acknowledgment (ACK) is triggered. If no specific instructions (ACK_FREQUENCY
+    // frames) are received from the peer, the receiver will immediately acknowledge
+    // any out-of-order packets, which means the default value is 1. A value of 0
+    // means out-of-order packets do not trigger an immediate ACK.
+    //
+    uint8_t ReorderingThreshold;
+
+    //
+    // The maximum number of packets that the peer can be out of order before an immediate
+    // acknowledgment (ACK) is triggered.
+    //
+    uint8_t PeerReorderingThreshold;
+
     //
     // The ACK frequency sequence number we are currently using to send.
     //

diff --git a/src/core/crypto_tls.c b/src/core/crypto_tls.c
@@ -63,7 +63,7 @@ typedef enum eSniNameType {
 #define QUIC_TP_ID_MAX_DATAGRAM_FRAME_SIZE                  32              // varint
 #define QUIC_TP_ID_DISABLE_1RTT_ENCRYPTION                  0xBAAD          // N/A
 #define QUIC_TP_ID_VERSION_NEGOTIATION_EXT                  0x11            // Blob
-#define QUIC_TP_ID_MIN_ACK_DELAY                            0xFF03DE1AULL   // varint
+#define QUIC_TP_ID_MIN_ACK_DELAY                            0xFF04DE1BULL   // varint
 #define QUIC_TP_ID_CIBIR_ENCODING                           0x1000          // {varint, varint}
 #define QUIC_TP_ID_GREASE_QUIC_BIT                          0x2AB2          // N/A
 #define QUIC_TP_ID_RELIABLE_RESET_ENABLED                   0x17f7586d2cb570   // varint

diff --git a/src/core/frame.c b/src/core/frame.c
@@ -1234,19 +1234,6 @@ QuicDatagramFrameDecode(
     return TRUE;
 }
 
-typedef struct QUIC_ACK_FREQUENCY_EXTRAS {
-
-    union {
-        struct {
-            uint8_t IgnoreOrder : 1;
-            uint8_t IgnoreCE    : 1;
-            uint8_t Reserved    : 6;
-        };
-        uint8_t Value;
-    };
-
-} QUIC_ACK_FREQUENCY_EXTRAS;
-
 _Success_(return != FALSE)
 BOOLEAN
 QuicAckFrequencyFrameEncode(
@@ -1259,27 +1246,20 @@ QuicAckFrequencyFrameEncode(
     uint16_t RequiredLength =
         QuicVarIntSize(QUIC_FRAME_ACK_FREQUENCY) +
         QuicVarIntSize(Frame->SequenceNumber) +
-        QuicVarIntSize(Frame->PacketTolerance) +
-        QuicVarIntSize(Frame->UpdateMaxAckDelay) +
-        sizeof(QUIC_ACK_FREQUENCY_EXTRAS);
+        QuicVarIntSize(Frame->AckElicitingThreshold) +
+        QuicVarIntSize(Frame->RequestedMaxAckDelay) +
+        QuicVarIntSize(Frame->ReorderingThreshold);
 
     if (BufferLength < *Offset + RequiredLength) {
         return FALSE;
     }
 
-    CXPLAT_DBG_ASSERT(Frame->IgnoreOrder <= 1); // IgnoreOrder should only be 0 or 1.
-    CXPLAT_DBG_ASSERT(Frame->IgnoreCE <= 1);    // IgnoreCE should only be 0 or 1.
-
-    QUIC_ACK_FREQUENCY_EXTRAS Extras = { .Value = 0 };
-    Extras.IgnoreOrder = Frame->IgnoreOrder;
-    Extras.IgnoreCE = Frame->IgnoreCE;
-
     Buffer = Buffer + *Offset;
     Buffer = QuicVarIntEncode(QUIC_FRAME_ACK_FREQUENCY, Buffer);
     Buffer = QuicVarIntEncode(Frame->SequenceNumber, Buffer);
-    Buffer = QuicVarIntEncode(Frame->PacketTolerance, Buffer);
-    Buffer = QuicVarIntEncode(Frame->UpdateMaxAckDelay, Buffer);
-    QuicUint8Encode(Extras.Value, Buffer);
+    Buffer = QuicVarIntEncode(Frame->AckElicitingThreshold, Buffer);
+    Buffer = QuicVarIntEncode(Frame->RequestedMaxAckDelay, Buffer);
+    QuicVarIntEncode(Frame->ReorderingThreshold, Buffer);
     *Offset += RequiredLength;
 
     return TRUE;
@@ -1295,15 +1275,12 @@ QuicAckFrequencyFrameDecode(
     _Out_ QUIC_ACK_FREQUENCY_EX* Frame
     )
 {
-    QUIC_ACK_FREQUENCY_EXTRAS Extras;
     if (!QuicVarIntDecode(BufferLength, Buffer, Offset, &Frame->SequenceNumber) ||
-        !QuicVarIntDecode(BufferLength, Buffer, Offset, &Frame->PacketTolerance) ||
-        !QuicVarIntDecode(BufferLength, Buffer, Offset, &Frame->UpdateMaxAckDelay) ||
-        !QuicUint8tDecode(BufferLength, Buffer, Offset, &Extras.Value)) {
+        !QuicVarIntDecode(BufferLength, Buffer, Offset, &Frame->AckElicitingThreshold) ||
+        !QuicVarIntDecode(BufferLength, Buffer, Offset, &Frame->RequestedMaxAckDelay) ||
+        !QuicVarIntDecode(BufferLength, Buffer, Offset, &Frame->ReorderingThreshold)) {
         return FALSE;
     }
-    Frame->IgnoreOrder = Extras.IgnoreOrder;
-    Frame->IgnoreCE = Extras.IgnoreCE;
     return TRUE;
 }
 
@@ -1963,15 +1940,14 @@ QuicFrameLog(
 
         QuicTraceLogVerbose(
             FrameLogAckFrequency,
-            "[%c][%cX][%llu]   ACK_FREQUENCY SeqNum:%llu PktTolerance:%llu MaxAckDelay:%llu IgnoreOrder:%hhu IgnoreCE:%hhu",
+            "[%c][%cX][%llu]   ACK_FREQUENCY SeqNum:%llu AckElicitThreshold:%llu MaxAckDelay:%llu ReorderThreshold:%llu",
             PtkConnPre(Connection),
             PktRxPre(Rx),
             PacketNumber,
             Frame.SequenceNumber,
-            Frame.PacketTolerance,
-            Frame.UpdateMaxAckDelay,
-            Frame.IgnoreOrder,
-            Frame.IgnoreCE);
+            Frame.AckElicitingThreshold,
+            Frame.RequestedMaxAckDelay,
+            Frame.ReorderingThreshold);
         break;
     }
 
@@ -2006,7 +1982,7 @@ QuicFrameLog(
             Frame.Timestamp);
         break;
     }
-    
+
     case QUIC_FRAME_RELIABLE_RESET_STREAM: {
         QUIC_RELIABLE_RESET_STREAM_EX Frame;
         if (!QuicReliableResetFrameDecode(PacketLength, Packet, Offset, &Frame)) {

diff --git a/src/core/frame.h b/src/core/frame.h
@@ -156,7 +156,7 @@ typedef enum QUIC_FRAME_TYPE {
     QUIC_FRAME_DATAGRAM_1           = 0x31ULL,
     /* 0x32 to 0xad are unused currently */
     QUIC_FRAME_ACK_FREQUENCY        = 0xafULL,
-    QUIC_FRAME_IMMEDIATE_ACK        = 0xacULL,
+    QUIC_FRAME_IMMEDIATE_ACK        = 0x1fULL,
     /* 0xaf to 0x2f4 are unused currently */
     QUIC_FRAME_TIMESTAMP            = 0x2f5ULL,
 
@@ -844,11 +844,9 @@ QuicDatagramFrameDecode(
 typedef struct QUIC_ACK_FREQUENCY_EX {
 
     QUIC_VAR_INT SequenceNumber;
-    QUIC_VAR_INT PacketTolerance;
-    QUIC_VAR_INT UpdateMaxAckDelay; // In microseconds (us)
-    BOOLEAN IgnoreOrder;
-    BOOLEAN IgnoreCE;
-
+    QUIC_VAR_INT AckElicitingThreshold;
+    QUIC_VAR_INT RequestedMaxAckDelay; // In microseconds (us)
+    QUIC_VAR_INT ReorderingThreshold;
 } QUIC_ACK_FREQUENCY_EX;
 
 _Success_(return != FALSE)

diff --git a/src/core/quicdef.h b/src/core/quicdef.h
@@ -103,6 +103,12 @@ typedef struct QUIC_RX_PACKET QUIC_RX_PACKET;
 //
 #define QUIC_MIN_ACK_SEND_NUMBER                2
 
+//
+// The value for Reordering threshold when no ACK_FREQUENCY frame is received.
+// This means that the receiver will immediately acknowledge any out-of-order packets.
+//
+#define QUIC_MIN_REORDERING_THRESHOLD           1
+
 //
 // The size of the stateless reset token.
 //

diff --git a/src/core/send.c b/src/core/send.c
@@ -894,10 +894,9 @@ QuicSendWriteFrames(
 
             QUIC_ACK_FREQUENCY_EX Frame;
             Frame.SequenceNumber = Connection->SendAckFreqSeqNum;
-            Frame.PacketTolerance = Connection->PeerPacketTolerance;
-            Frame.UpdateMaxAckDelay = MS_TO_US(QuicConnGetAckDelay(Connection));
-            Frame.IgnoreOrder = FALSE;
-            Frame.IgnoreCE = FALSE;
+            Frame.AckElicitingThreshold = Connection->PeerPacketTolerance;
+            Frame.RequestedMaxAckDelay = MS_TO_US(QuicConnGetAckDelay(Connection));
+            Frame.ReorderingThreshold = Connection->PeerReorderingThreshold;
 
             if (QuicAckFrequencyFrameEncode(
                     &Frame,