Re: [quicwg/base-drafts] Recovery uses both time and packet thresholds (#1974)

[janaiyengar <[e-mail]>]

janaiyengar commented on this pull request.



>  
 ### Fast Retransmit
 
 An unacknowledged packet is marked as lost when an acknowledgment is received
 for a packet that was sent a threshold number of packets (kReorderingThreshold)
-and/or a threshold amount of time after the unacknowledged packet. Receipt of
-the acknowledgement indicates that a later packet was received, while the
-reordering threshold provides some tolerance for reordering of packets in the
-network.
+or a threshold amount of time (kTimeReorderingFraction) after the

how 'bout just kPacketThreshold and kTimeThreshold? The context makes clear what it's used for and exactly what the units are.

> @@ -274,34 +274,14 @@ TCP-NCR {{?RFC4653}}, to improve QUIC's reordering resilience.
 Time threshold loss detection uses a time threshold to determine how much
 reordering to tolerate.  In this document, the threshold is expressed as a
 fraction of an RTT, but implemenantations MAY experiment with absolute
-thresholds.  This may be used either as a replacement for a packet reordering
-threshold or in addition to it.
-
-When a larger packet is acknowledged, if it was sent more than the threshold
-after any in flight packets, those packets are immediately declared lost.
-Otherwise, a timer is set for the the reordering threshold minus the time
-difference between the earliest in flight packet and the largest newly
-acknowledged packet.  Note that in some cases the timer could become longer when
-packets are acknowleged out of order. The RECOMMENDED time threshold, expressed
+thresholds.  It is recommended this is is used in conjunction with packet
+threshold fast retransmit. The RECOMMENDED time threshold, expressed

This sentence is mostly unclear ("in conjunction with packet threshold fast retransmit") and not really useful. I would drop it entirely.

> -Unacknowledged packets close to the tail may have fewer than
-kReorderingThreshold retransmittable packets sent after them.  Loss of such
-packets cannot be detected via Packet Threshold Fast Retransmit. To enable
-ack-based loss detection of such packets, receipt of an acknowledgment for
-the last outstanding retransmittable packet triggers the Early Retransmit
-process, as follows.
-
-If there are unacknowledged in-flight packets still pending, they should
-be marked as lost. To compensate for the reduced reordering resilience, the
-sender SHOULD set a timer for a small period of time. If the unacknowledged
-in-flight packets are not acknowledged during this time, then these
-packets MUST be marked as lost.
-
-An endpoint SHOULD set the timer such that a packet is marked as lost no earlier
-than 1.125 * max(SRTT, latest_RTT) since when it was sent.
+An endpoint SHOULD declare packets lost no earlier than

I don't think the early retransmit section should be removed. This is a separate mechanism, and it is the only thing that uses the time threshold. I think a different structure might be useful... I'd actually argue for removing the time threshold section and leaving ER as is, since the time threshold section doesn't add much.

> @@ -312,16 +292,10 @@ Using max(SRTT, latest_RTT) protects from the two following cases:
 * the latest RTT sample is higher than the SRTT, perhaps due to a sustained
   increase in the actual RTT, but the smoothed SRTT has not yet caught up.
 
-The 1.125 multiplier increases reordering resilience. Implementers MAY
-experiment with using other multipliers, bearing in mind that a lower multiplier
-reduces reordering resilience and increases spurious retransmissions, and a
-higher multiplier increases loss recovery delay.
-
-This mechanism is based on Early Retransmit for TCP {{?RFC5827}}. However,

This is still true. I would leave this in.

> @@ -841,7 +807,7 @@ Pseudocode for SetLossDetectionTimer follows:
         time_of_last_sent_crypto_packet + timeout)
       return
     if (loss_time != 0):
-      // Early retransmit timer or time loss detection.
+      // Time threshold loss detection.

```suggestion
      // Early retransmit timer.
```

> @@ -875,7 +841,7 @@ Pseudocode for OnLossDetectionTimeout follows:
        RetransmitUnackedCryptoData()
        crypto_count++
      else if (loss_time != 0):
-       // Early retransmit or Time Loss Detection
+       // Time threshold loss Detection

```suggestion
       // Early retransmit
```

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1974#pullrequestreview-173860472