Re: [quicwg/base-drafts] Rewrite of RTT estimation section (#2592)

[ianswett <[e-mail]>]

ianswett commented on this pull request.

Thanks for the substantial rework, some comments.

> @@ -226,24 +226,25 @@ QUIC supports many ACK ranges, opposed to TCP's 3 SACK ranges.  In high loss
 environments, this speeds recovery, reduces spurious retransmits, and ensures
 forward progress without relying on timeouts.
 
-### Explicit Correction For Delayed ACKs
+### Explicit Correction For Delayed ACKs {#ack-delay}

Actually, ACKs are delayed by the receiver in this case and this whole section talks about the ACK frame, which makes sense to me.

>  
 
-# Generating Acknowledgements
-
-QUIC SHOULD delay sending acknowledgements in response to packets, but MUST NOT
-excessively delay acknowledgements of ack-eliciting packets. Specifically,
-implementations MUST attempt to enforce a maximum ack delay to avoid causing
-the peer spurious timeouts.  The maximum ack delay is communicated in the
-`max_ack_delay` transport parameter and the default value is 25ms.
+# Generating Acknowledgements {#generating-acks}

Sidenote: Are we ever going to move this to transport?

> -RTT when multiple such ACK frames are received within an RTT.  When multiple
-samples are generated within an RTT, the smoothed RTT and RTT variance could
-retain inadequate history, as suggested in {{?RFC6298}}. Changing these
-computations is currently an open research question.
-
-min_rtt is the minimum RTT measured over the connection, prior to adjusting by
-ack delay.  Ignoring ack delay for min RTT prevents intentional or unintentional
-underestimation of min RTT, which in turn prevents underestimating smoothed RTT.
-
-A sender calculates both smoothed RTT (SRTT) and RTT variance (RTTVAR) similar
-to those specified in {{?RFC6298}}.  Note that computing smoothed_rtt does not
-use ack_delay for the first RTT sample, because doing so would result in a
-smoothed_rtt that is smaller than the min_rtt.
-
-On every newly acknowledged ack-eliciting largest acked:
+## Measuring and Reporting Host Delay {#host-delay}

Would "receiver delay" be more accurate?  I know I've heard the term host delay before, but I thought it usually implied unintentional delays(ie: processing time, etc), not intentional delays like delayed ACKs?

> -retain inadequate history, as suggested in {{?RFC6298}}. Changing these
-computations is currently an open research question.
-
-min_rtt is the minimum RTT measured over the connection, prior to adjusting by
-ack delay.  Ignoring ack delay for min RTT prevents intentional or unintentional
-underestimation of min RTT, which in turn prevents underestimating smoothed RTT.
-
-A sender calculates both smoothed RTT (SRTT) and RTT variance (RTTVAR) similar
-to those specified in {{?RFC6298}}.  Note that computing smoothed_rtt does not
-use ack_delay for the first RTT sample, because doing so would result in a
-smoothed_rtt that is smaller than the min_rtt.
-
-On every newly acknowledged ack-eliciting largest acked:
+## Measuring and Reporting Host Delay {#host-delay}
+
+An endpoint measures the delay incurred between when a packet is received and

endpoint -> receiver?

> -ack delay.  Ignoring ack delay for min RTT prevents intentional or unintentional
-underestimation of min RTT, which in turn prevents underestimating smoothed RTT.
-
-A sender calculates both smoothed RTT (SRTT) and RTT variance (RTTVAR) similar
-to those specified in {{?RFC6298}}.  Note that computing smoothed_rtt does not
-use ack_delay for the first RTT sample, because doing so would result in a
-smoothed_rtt that is smaller than the min_rtt.
-
-On every newly acknowledged ack-eliciting largest acked:
+## Measuring and Reporting Host Delay {#host-delay}
+
+An endpoint measures the delay incurred between when a packet is received and
+when the corresponding acknowledgment is sent.  The endpoint encodes this host
+delay for the largest acknowledged packet in the Ack Delay field of an ACK frame
+(see Section 19.3 of {{QUIC-TRANSPORT}}).  This allows the receiver of the ACK
+to adjust for any host delays - importantly, for delayed acknowledgements - when

Can you avoid ' - ' with ", which is particularly important for delayed acknowledgements,"

> -A sender calculates both smoothed RTT (SRTT) and RTT variance (RTTVAR) similar
-to those specified in {{?RFC6298}}.  Note that computing smoothed_rtt does not
-use ack_delay for the first RTT sample, because doing so would result in a
-smoothed_rtt that is smaller than the min_rtt.
-
-On every newly acknowledged ack-eliciting largest acked:
+## Measuring and Reporting Host Delay {#host-delay}
+
+An endpoint measures the delay incurred between when a packet is received and
+when the corresponding acknowledgment is sent.  The endpoint encodes this host
+delay for the largest acknowledged packet in the Ack Delay field of an ACK frame
+(see Section 19.3 of {{QUIC-TRANSPORT}}).  This allows the receiver of the ACK
+to adjust for any host delays - importantly, for delayed acknowledgements - when
+estimating the path RTT.  In certain deployments, a packet might be held in the
+OS kernel or elsewhere on the host before being processed by the QUIC
+stack. Where possible, an endpoint SHOULD include these delays when populating

Based on my experience, I might be inclined towards a MAY.  Using receive timestamps creates as many(or more) complexities as it solves.

> -On every newly acknowledged ack-eliciting largest acked:
+## Measuring and Reporting Host Delay {#host-delay}
+
+An endpoint measures the delay incurred between when a packet is received and
+when the corresponding acknowledgment is sent.  The endpoint encodes this host
+delay for the largest acknowledged packet in the Ack Delay field of an ACK frame
+(see Section 19.3 of {{QUIC-TRANSPORT}}).  This allows the receiver of the ACK
+to adjust for any host delays - importantly, for delayed acknowledgements - when
+estimating the path RTT.  In certain deployments, a packet might be held in the
+OS kernel or elsewhere on the host before being processed by the QUIC
+stack. Where possible, an endpoint SHOULD include these delays when populating
+the Ack Delay field in an ACK frame.
+
+An endpoint MUST NOT excessively delay acknowledgements of ack-eliciting
+packets.  The maximum ack delay is communicated in the max_ack_delay transport
+parameter, see Section 18.1 of {{QUIC-TRANSPORT}}.  max_ack_delay implies an

```suggestion
parameter, see Section 18.1 of {{QUIC-TRANSPORT}}.  max_ack_delay specifies an
```

> +OS kernel or elsewhere on the host before being processed by the QUIC
+stack. Where possible, an endpoint SHOULD include these delays when populating
+the Ack Delay field in an ACK frame.
+
+An endpoint MUST NOT excessively delay acknowledgements of ack-eliciting
+packets.  The maximum ack delay is communicated in the max_ack_delay transport
+parameter, see Section 18.1 of {{QUIC-TRANSPORT}}.  max_ack_delay implies an
+explicit contract: an endpoint promises to never delay acknowledgments of an
+ack-eliciting packet by more than the indicated value. If it does, any excess
+accrues to the RTT estimate and could result in spurious retransmissions from
+the peer.
+
+
+# Estimating the Round-Trip Time {#compute-rtt}
+
+At a high level, an endpoint measures the time from when a packet was sent to

```suggestion
An endpoint measures the time from when a packet was sent to
```

>  ~~~
 latest_rtt = ack_time - send_time_of_largest_acked
 ~~~
 
-First RTT sample:
+An endpoint uses only locally observed times in generating RTT samples and does

I'm confused about where this is going, since we're going to adjust later on?

>  ~~~
 latest_rtt = ack_time - send_time_of_largest_acked
 ~~~
 
-First RTT sample:
+An endpoint uses only locally observed times in generating RTT samples and does
+not adjust for any host delays reported by the peer ({{host-delay}}).
+
+A peer reports host delays for only the largest acknowledged packet in an ACK
+frame, which is assumed by subsequent computations of smoothed_rtt and rttvar in

I'm unclear what the latter half of this sentence means?  Maybe remove it?

>  ~~~
 latest_rtt = ack_time - send_time_of_largest_acked
 ~~~
 
-First RTT sample:
+An endpoint uses only locally observed times in generating RTT samples and does
+not adjust for any host delays reported by the peer ({{host-delay}}).
+
+A peer reports host delays for only the largest acknowledged packet in an ACK
+frame, which is assumed by subsequent computations of smoothed_rtt and rttvar in
+adjusting for host delays.  As a result, an RTT sample is only generated using
+the largest acknowledged packet in the received ACK frame.
+
+To avoid generating multiple RTT samples using the same packet, an ACK frame
+SHOULD NOT be used to update RTT estimates if it does not newly acknowledge the

You say this two different ways, which are subtly different.  Above, you say the largest acked needs to be larger.  Here you say it needs to be newly acknowledged.  I think the latter is more correct, but we should be consistent.

>  ~~~
 latest_rtt = ack_time - send_time_of_largest_acked
 ~~~
 
-First RTT sample:
+An endpoint uses only locally observed times in generating RTT samples and does
+not adjust for any host delays reported by the peer ({{host-delay}}).
+
+A peer reports host delays for only the largest acknowledged packet in an ACK
+frame, which is assumed by subsequent computations of smoothed_rtt and rttvar in
+adjusting for host delays.  As a result, an RTT sample is only generated using
+the largest acknowledged packet in the received ACK frame.
+
+To avoid generating multiple RTT samples using the same packet, an ACK frame
+SHOULD NOT be used to update RTT estimates if it does not newly acknowledge the
+largest acknowledged packet.
+
+An RTT sample MUST NOT be generated on receiving an ACK frame that does not
+newly acknowledge at least one ack-eliciting packet.  A peer does not send an

peer -> receiver?

Also, this paragraph could be a bit clearer, since I think there's an important point here and it may not be clear to all readers.

>    // Based on {{?RFC6298}}.
   if (smoothed_rtt == 0):
-    smoothed_rtt = latest_rtt
+    smoothed_rtt = adjusted_rtt

I'd suggest moving the adjusted_rtt calculation into the else and using latest_rtt here?  If not, I'd ensure smoothed_rtt and rttvar are set as a function of the same field, not one based on adjusted_rtt and the other latest_rtt.

-- 
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/2592#pullrequestreview-223838768