Updates for ACK Frequency Draft 2 (microsoft#2347)

* Updates for ACK Frequency Draft 2 * Fix Reserved name
prchander · Feb 2, 2022 · 455d5a3 · 455d5a3
1 parent 61dee3f
commit 455d5a3
Show file tree

Hide file tree

Showing 12 changed files with 190 additions and 47 deletions.
diff --git a/src/core/ack_tracker.c b/src/core/ack_tracker.c
@@ -103,7 +103,7 @@ QuicAckTrackerAckPacket(
     _In_ uint64_t PacketNumber,
     _In_ uint64_t RecvTimeUs,
     _In_ CXPLAT_ECN_TYPE ECN,
-    _In_ BOOLEAN AckElicitingPayload
+    _In_ QUIC_ACK_TYPE AckType
     )
 {
     QUIC_CONNECTION* Connection = QuicAckTrackerGetPacketSpace(Tracker)->Connection;
@@ -166,7 +166,7 @@ QuicAckTrackerAckPacket(
 
     Tracker->AlreadyWrittenAckFrame = FALSE;
 
-    if (!AckElicitingPayload) {
+    if (AckType == QUIC_ACK_TYPE_NON_ACK_ELICITING) {
         goto Exit;
     }
 
@@ -185,12 +185,14 @@ QuicAckTrackerAckPacket(
     //      been loss and should indicate this info to the peer. This logic is
     //      disabled if 'IgnoreReordering' is TRUE.
     //   3. The delayed ACK timer fires after the configured time.
+    //   4. The packet included an IMMEDIATE_ACK frame.
     //
     // If we don't queue an immediate ACK and this is the first ACK eliciting
     // packet received, we make sure the ACK delay timer is started.
     //
 
-    if ((Tracker->AckElicitingPacketsToAcknowledge >= (uint16_t)Connection->PacketTolerance) ||
+    if (AckType == QUIC_ACK_TYPE_ACK_IMMEDIATE ||
+        (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.

diff --git a/src/core/ack_tracker.h b/src/core/ack_tracker.h
@@ -91,6 +91,12 @@ QuicAckTrackerAddPacketNumber(
     _In_ uint64_t PacketNumber
     );
 
+typedef enum QUIC_ACK_TYPE {
+    QUIC_ACK_TYPE_NON_ACK_ELICITING,
+    QUIC_ACK_TYPE_ACK_ELICITING,
+    QUIC_ACK_TYPE_ACK_IMMEDIATE,
+} QUIC_ACK_TYPE;
+
 //
 // Adds the packet number to the list of packets that should be acknowledged.
 //
@@ -101,7 +107,7 @@ QuicAckTrackerAckPacket(
     _In_ uint64_t PacketNumber,
     _In_ uint64_t RecvTimeUs,
     _In_ CXPLAT_ECN_TYPE ECN,
-    _In_ BOOLEAN AckElicitingPayload
+    _In_ QUIC_ACK_TYPE AckType
     );
 
 //

diff --git a/src/core/connection.c b/src/core/connection.c
@@ -4130,7 +4130,8 @@ QuicConnRecvFrames(
     _In_ CXPLAT_ECN_TYPE ECN
     )
 {
-    BOOLEAN AckPacketImmediately = FALSE; // Allows skipping delayed ACK timer.
+    BOOLEAN AckEliciting = FALSE;
+    BOOLEAN AckImmediately = FALSE;
     BOOLEAN UpdatedFlowControl = FALSE;
     QUIC_ENCRYPT_LEVEL EncryptLevel = QuicKeyTypeToEncryptLevel(Packet->KeyType);
     BOOLEAN Closed = Connection->State.ClosedLocally || Connection->State.ClosedRemotely;
@@ -4241,7 +4242,7 @@ QuicConnRecvFrames(
             // No other payload. Just need to acknowledge the packet this was
             // contained in.
             //
-            AckPacketImmediately = TRUE;
+            AckEliciting = TRUE;
             Packet->HasNonProbingFrame = TRUE;
             break;
         }
@@ -4296,7 +4297,7 @@ QuicConnRecvFrames(
                     Packet->KeyType,
                     &Frame);
             if (QUIC_SUCCEEDED(Status)) {
-                AckPacketImmediately = TRUE;
+                AckEliciting = TRUE;
             } else if (Status == QUIC_STATUS_OUT_OF_MEMORY) {
                 return FALSE;
             } else {
@@ -4347,7 +4348,7 @@ QuicConnRecvFrames(
             // TODO - Save the token for future use.
             //
 
-            AckPacketImmediately = TRUE;
+            AckEliciting = TRUE;
             Packet->HasNonProbingFrame = TRUE;
             break;
         }
@@ -4390,7 +4391,7 @@ QuicConnRecvFrames(
                 return FALSE;
             }
 
-            AckPacketImmediately = TRUE;
+            AckEliciting = TRUE;
 
             BOOLEAN PeerOriginatedStream =
                 QuicConnIsServer(Connection) ?
@@ -4514,7 +4515,7 @@ QuicConnRecvFrames(
                     &Connection->Send, REASON_CONNECTION_FLOW_CONTROL);
             }
 
-            AckPacketImmediately = TRUE;
+            AckEliciting = TRUE;
             Packet->HasNonProbingFrame = TRUE;
             break;
         }
@@ -4546,7 +4547,7 @@ QuicConnRecvFrames(
                 Frame.BidirectionalStreams,
                 Frame.MaximumStreams);
 
-            AckPacketImmediately = TRUE;
+            AckEliciting = TRUE;
             Packet->HasNonProbingFrame = TRUE;
             break;
         }
@@ -4577,7 +4578,7 @@ QuicConnRecvFrames(
                 Frame.DataLimit);
             QuicSendSetSendFlag(&Connection->Send, QUIC_CONN_SEND_FLAG_MAX_DATA);
 
-            AckPacketImmediately = TRUE;
+            AckEliciting = TRUE;
             Packet->HasNonProbingFrame = TRUE;
             break;
         }
@@ -4605,7 +4606,7 @@ QuicConnRecvFrames(
                 "Peer Streams[%hu] FC blocked (%llu)",
                 Frame.BidirectionalStreams,
                 Frame.StreamLimit);
-            AckPacketImmediately = TRUE;
+            AckEliciting = TRUE;
 
             QUIC_CONNECTION_EVENT Event;
             Event.Type = QUIC_CONNECTION_EVENT_PEER_NEEDS_STREAMS; // TODO - Uni/Bidi
@@ -4699,7 +4700,7 @@ QuicConnRecvFrames(
                 return FALSE;
             }
 
-            AckPacketImmediately = TRUE;
+            AckEliciting = TRUE;
             break;
         }
 
@@ -4751,7 +4752,7 @@ QuicConnRecvFrames(
                 }
             }
 
-            AckPacketImmediately = TRUE;
+            AckEliciting = TRUE;
             Packet->HasNonProbingFrame = TRUE;
             break;
         }
@@ -4776,7 +4777,7 @@ QuicConnRecvFrames(
             CxPlatCopyMemory(Path->Response, Frame.Data, sizeof(Frame.Data));
             QuicSendSetSendFlag(&Connection->Send, QUIC_CONN_SEND_FLAG_PATH_RESPONSE);
 
-            AckPacketImmediately = TRUE;
+            AckEliciting = TRUE;
             break;
         }
 
@@ -4807,7 +4808,7 @@ QuicConnRecvFrames(
                 }
             }
 
-            AckPacketImmediately = TRUE;
+            AckEliciting = TRUE;
             break;
         }
 
@@ -4835,7 +4836,7 @@ QuicConnRecvFrames(
                 Frame.ReasonPhrase,
                 (uint16_t)Frame.ReasonPhraseLength);
 
-            AckPacketImmediately = TRUE;
+            AckEliciting = TRUE;
             Packet->HasNonProbingFrame = TRUE;
 
             if (Connection->State.HandleClosed) {
@@ -4867,7 +4868,7 @@ QuicConnRecvFrames(
                 QuicCryptoHandshakeConfirmed(&Connection->Crypto);
             }
 
-            AckPacketImmediately = TRUE;
+            AckEliciting = TRUE;
             Packet->HasNonProbingFrame = TRUE;
             break;
         }
@@ -4898,7 +4899,7 @@ QuicConnRecvFrames(
                 QuicConnTransportError(Connection, QUIC_ERROR_FRAME_ENCODING_ERROR);
                 return FALSE;
             }
-            AckPacketImmediately = TRUE;
+            AckEliciting = TRUE;
             break;
         }
 
@@ -4924,7 +4925,7 @@ QuicConnRecvFrames(
                 return FALSE;
             }
 
-            AckPacketImmediately = TRUE;
+            AckEliciting = TRUE;
             if (Frame.SequenceNumber < Connection->NextRecvAckFreqSeqNum) {
                 //
                 // This sequence number (or a higher one) has already been
@@ -4954,6 +4955,10 @@ QuicConnRecvFrames(
             break;
         }
 
+        case QUIC_FRAME_IMMEDIATE_ACK: // Always accept the frame, because we always enable support.
+            AckImmediately = TRUE;
+            break;
+
         default:
             //
             // No default case necessary, as we have already validated the frame
@@ -4983,12 +4988,21 @@ QuicConnRecvFrames(
             Packet->NewLargestPacketNumber = TRUE;
         }
 
+        QUIC_ACK_TYPE AckType;
+        if (AckImmediately) {
+            AckType = QUIC_ACK_TYPE_ACK_IMMEDIATE;
+        } else if (AckEliciting) {
+            AckType = QUIC_ACK_TYPE_ACK_ELICITING;
+        } else {
+            AckType = QUIC_ACK_TYPE_NON_ACK_ELICITING;
+        }
+
         QuicAckTrackerAckPacket(
             &Connection->Packets[EncryptLevel]->AckTracker,
             Packet->PacketNumber,
             RecvTime,
             ECN,
-            AckPacketImmediately);
+            AckType);
     }
 
     Packet->CompletelyValid = TRUE;

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                  0xFF73DB        // Blob
-#define QUIC_TP_ID_MIN_ACK_DELAY                            0xFF02DE1AULL   // varint
+#define QUIC_TP_ID_MIN_ACK_DELAY                            0xFF03DE1AULL   // varint
 
 BOOLEAN
 QuicTpIdIsReserved(

diff --git a/src/core/frame.c b/src/core/frame.c
@@ -1184,6 +1184,19 @@ 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(
@@ -1198,20 +1211,25 @@ QuicAckFrequencyFrameEncode(
         QuicVarIntSize(Frame->SequenceNumber) +
         QuicVarIntSize(Frame->PacketTolerance) +
         QuicVarIntSize(Frame->UpdateMaxAckDelay) +
-        sizeof(uint8_t);    // IgnoreOrder
+        sizeof(QUIC_ACK_FREQUENCY_EXTRAS);
 
     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(Frame->IgnoreOrder, Buffer);
+    QuicUint8Encode(Extras.Value, Buffer);
     *Offset += RequiredLength;
 
     return TRUE;
@@ -1227,13 +1245,15 @@ 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, &Frame->IgnoreOrder) ||
-        Frame->IgnoreOrder > 1) { // IgnoreOrder should only be 0 or 1.
+        !QuicUint8tDecode(BufferLength, Buffer, Offset, &Extras.Value)) {
         return FALSE;
     }
+    Frame->IgnoreOrder = Extras.IgnoreOrder;
+    Frame->IgnoreCE = Extras.IgnoreCE;
     return TRUE;
 }
 
@@ -1851,14 +1871,25 @@ QuicFrameLog(
 
         QuicTraceLogVerbose(
             FrameLogAckFrequency,
-            "[%c][%cX][%llu]   ACK_FREQUENCY SeqNum:%llu PktTolerance:%llu MaxAckDelay:%llu IgnoreOrder:%hhu",
+            "[%c][%cX][%llu]   ACK_FREQUENCY SeqNum:%llu PktTolerance:%llu MaxAckDelay:%llu IgnoreOrder:%hhu IgnoreCE:%hhu",
             PtkConnPre(Connection),
             PktRxPre(Rx),
             PacketNumber,
             Frame.SequenceNumber,
             Frame.PacketTolerance,
             Frame.UpdateMaxAckDelay,
-            Frame.IgnoreOrder);
+            Frame.IgnoreOrder,
+            Frame.IgnoreCE);
+        break;
+    }
+
+    case QUIC_FRAME_IMMEDIATE_ACK: {
+        QuicTraceLogVerbose(
+            FrameLogImmediateAck,
+            "[%c][%cX][%llu]   IMMEDIATE_ACK",
+            PtkConnPre(Connection),
+            PktRxPre(Rx),
+            PacketNumber);
         break;
     }
 

diff --git a/src/core/frame.h b/src/core/frame.h
@@ -150,6 +150,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_MAX_SUPPORTED
 
@@ -162,7 +163,7 @@ CXPLAT_STATIC_ASSERT(
 #define QUIC_FRAME_IS_KNOWN(X) \
     (X <= QUIC_FRAME_HANDSHAKE_DONE || \
      (X >= QUIC_FRAME_DATAGRAM && X <= QUIC_FRAME_DATAGRAM_1) || \
-     X == QUIC_FRAME_ACK_FREQUENCY \
+     X == QUIC_FRAME_ACK_FREQUENCY || X == QUIC_FRAME_IMMEDIATE_ACK \
     )
 
 //
@@ -803,7 +804,8 @@ typedef struct QUIC_ACK_FREQUENCY_EX {
     QUIC_VAR_INT SequenceNumber;
     QUIC_VAR_INT PacketTolerance;
     QUIC_VAR_INT UpdateMaxAckDelay; // In microseconds (us)
-    uint8_t IgnoreOrder;
+    BOOLEAN IgnoreOrder;
+    BOOLEAN IgnoreCE;
 
 } QUIC_ACK_FREQUENCY_EX;
 

diff --git a/src/core/send.c b/src/core/send.c
@@ -825,6 +825,7 @@ QuicSendWriteFrames(
                     (uint64_t)Connection->Settings.MaxAckDelayMs +
                     (uint64_t)MsQuicLib.TimerResolutionMs);
             Frame.IgnoreOrder = FALSE;
+            Frame.IgnoreCE = FALSE;
 
             if (QuicAckFrequencyFrameEncode(
                     &Frame,

diff --git a/src/core/unittest/SpinFrame.cpp b/src/core/unittest/SpinFrame.cpp
@@ -79,6 +79,7 @@ TEST(SpinFrame, SpinFrame1000000)
         switch(FrameType) {
             case QUIC_FRAME_PADDING:
             case QUIC_FRAME_PING:
+            case QUIC_FRAME_IMMEDIATE_ACK:
                 break; // no-op
             case QUIC_FRAME_ACK:
             case QUIC_FRAME_ACK_1:

diff --git a/src/core/unittest/main.h b/src/core/unittest/main.h
@@ -85,6 +85,8 @@ std::ostream& operator << (std::ostream& o, const QUIC_FRAME_TYPE& type) {
             return o << "QUIC_FRAME_DATAGRAM_1";
         case QUIC_FRAME_ACK_FREQUENCY:
             return o << "QUIC_FRAME_ACK_FREQUENCY";
+        case QUIC_FRAME_IMMEDIATE_ACK:
+            return o << "QUIC_FRAME_IMMEDIATE_ACK";
         default:
             return o << "UNRECOGNIZED_FRAME_TYPE(" << (uint32_t) type << ")";
     }