Better Delayed ACK Support

src/core/ack_tracker.c

@@ -179,19 +179,21 @@ QuicAckTrackerAckPacket(
     //
     // There are several conditions where we decide to send an ACK immediately:
     //
-    //   1. We have received 'PacketTolerance' ACK eliciting packets.
+    //   1. The packet included an IMMEDIATE_ACK frame.
+    //   2. ACK delay is disabled (MaxAckDelayMs == 0).
+    //   3. We have received 'PacketTolerance' ACK eliciting packets.
     //   2. 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.
-    //   3. The delayed ACK timer fires after the configured time.
-    //   4. The packet included an IMMEDIATE_ACK frame.
+    //   4. The delayed ACK timer fires after the configured time.
     //
     // 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 (AckType == QUIC_ACK_TYPE_ACK_IMMEDIATE ||
+        Connection->Settings.MaxAckDelayMs == 0 ||
         (Tracker->AckElicitingPacketsToAcknowledge >= (uint16_t)Connection->PacketTolerance) ||
         (!Connection->State.IgnoreReordering &&
          (NewLargestPacketNumber &&
@@ -200,9 +202,7 @@ QuicAckTrackerAckPacket(
             &Tracker->PacketNumbersToAck,
           QuicRangeSize(&Tracker->PacketNumbersToAck) - 1)->Count == 1))) { // The gap is right before the last packet number.
         //
-        // Always send an ACK immediately if we have received enough ACK
-        // eliciting packets OR the latest one indicate a gap in the packet
-        // numbers, which likely means there was loss.
+        // Send the ACK immediately.
         //
         QuicSendSetSendFlag(&Connection->Send, QUIC_CONN_SEND_FLAG_ACK);
 

src/core/connection.c

@@ -2349,9 +2349,8 @@ QuicConnGenerateLocalTransportParameters(
         MaxUdpPayloadSizeFromMTU(
             CxPlatSocketGetLocalMtu(
                 Connection->Paths[0].Binding->Socket));
-    LocalTP->MaxAckDelay =
-        Connection->Settings.MaxAckDelayMs + MsQuicLib.TimerResolutionMs;
-    LocalTP->MinAckDelay = MS_TO_US(MsQuicLib.TimerResolutionMs);
+    LocalTP->MaxAckDelay = QuicConnGetAckDelay(Connection);
+    LocalTP->MinAckDelay = 0;
     LocalTP->ActiveConnectionIdLimit = QUIC_ACTIVE_CONNECTION_ID_LIMIT;
     LocalTP->Flags =
         QUIC_TP_FLAG_INITIAL_MAX_DATA |
@@ -5296,7 +5295,9 @@ QuicConnRecvFrames(
 
             Connection->NextRecvAckFreqSeqNum = Frame.SequenceNumber + 1;
             Connection->State.IgnoreReordering = Frame.IgnoreOrder;
-            if (Frame.UpdateMaxAckDelay < 1000) {
+            if (Frame.UpdateMaxAckDelay == 0) {
+                Connection->Settings.MaxAckDelayMs = 0;
+            } else if (Frame.UpdateMaxAckDelay < 1000) {
                 Connection->Settings.MaxAckDelayMs = 1;
             } else {
                 CXPLAT_DBG_ASSERT(US_TO_MS(Frame.UpdateMaxAckDelay) <= UINT32_MAX);
@@ -5318,7 +5319,7 @@ QuicConnRecvFrames(
         case QUIC_FRAME_IMMEDIATE_ACK: // Always accept the frame, because we always enable support.
             AckImmediately = TRUE;
             break;
-            
+
         case QUIC_FRAME_TIMESTAMP: { // Always accept the frame, because we always enable support.
             if (!Connection->State.TimestampRecvNegotiated) {
                 QuicTraceEvent(
@@ -5345,7 +5346,7 @@ QuicConnRecvFrames(
             Packet->SendTimestamp = Frame.Timestamp;
             break;
         }
-        
+
         default:
             //
             // No default case necessary, as we have already validated the frame

src/core/connection.h

@@ -1353,6 +1353,26 @@ QuicConnTimerExpired(
     _In_ uint64_t TimeNow
     );
 
+_IRQL_requires_max_(PASSIVE_LEVEL)
+inline
+uint64_t
+QuicConnGetAckDelay(
+    _In_ const QUIC_CONNECTION* Connection
+    )
+{
+    if (Connection->Settings.MaxAckDelayMs &&
+        (MsQuicLib.ExecutionConfig == NULL ||
+         Connection->Settings.MaxAckDelayMs > US_TO_MS(MsQuicLib.ExecutionConfig->PollingIdleTimeoutUs))) {
+        //
+        // If we are using delayed ACKs, and the ACK delay is greater than the
+        // polling timeout, then we need to account for delay resulting from
+        // from the timer resolution.
+        //
+        return (uint64_t)Connection->Settings.MaxAckDelayMs + (uint64_t)MsQuicLib.TimerResolutionMs;
+    }
+    return (uint64_t)Connection->Settings.MaxAckDelayMs;
+}
+
 //
 // Called when the QUIC version is set.
 //

src/core/inline.c

@@ -779,6 +779,11 @@ QuicConnTimerSet(
     _In_ uint64_t DelayUs
     );
 
+uint64_t
+QuicConnGetAckDelay(
+    _In_ const QUIC_CONNECTION* Connection
+    );
+
 uint8_t
 QuicPacketTraceType(
     _In_ const QUIC_SENT_PACKET_METADATA* Metadata

src/core/send.c

@@ -870,10 +870,7 @@ QuicSendWriteFrames(
             QUIC_ACK_FREQUENCY_EX Frame;
             Frame.SequenceNumber = Connection->SendAckFreqSeqNum;
             Frame.PacketTolerance = Connection->PeerPacketTolerance;
-            Frame.UpdateMaxAckDelay =
-                MS_TO_US(
-                    (uint64_t)Connection->Settings.MaxAckDelayMs +
-                    (uint64_t)MsQuicLib.TimerResolutionMs);
+            Frame.UpdateMaxAckDelay = QuicConnGetAckDelay(Connection);
             Frame.IgnoreOrder = FALSE;
             Frame.IgnoreCE = FALSE;
 
@@ -1489,6 +1486,7 @@ QuicSendStartDelayedAckTimer(
 {
     QUIC_CONNECTION* Connection = QuicSendGetConnection(Send);
 
+    CXPLAT_DBG_ASSERT(Connection->Settings.MaxAckDelayMs != 0);
     if (!Send->DelayedAckTimerActive &&
         !(Send->SendFlags & QUIC_CONN_SEND_FLAG_ACK) &&
         !Connection->State.ClosedLocally &&