Rewrite of RTT estimation section #2592

janaiyengar · 2019-04-08T05:29:23Z

I started with trying to introduce rationale for why only ack-eliciting packets are used for RTT computation, and ended up restructuring the section.

Closes #2567, #2568.

rewrite of RTT estimation section

draft-ietf-quic-recovery.md

martinthomson · 2019-04-08T05:53:40Z

draft-ietf-quic-recovery.md

+endpoint:
+
+- SHOULD use the lesser of the value reported in Ack Delay field of the ACK
+  frame and the peer's max_ack_delay transport parameter ({{ack-delay}}).


Why SHOULD? Why use 2119 language here at all?

When correcting an RTT sample, endpoints first limit the reported Ack Delay to the value of the max_ack_delay transport parameter. The RTT sample is then reduced by the resulting delay value. If correcting for host delays would result in a sample that is less than min_rtt, the unmodified sample is used. This guards against underestimation of RTT as a result of inflated reporting of host delays.

Fair point. Ultimately, this is bounded by another value that the peer advertises (max_ack_delay), so there's no additional safety issue here.

martinthomson · 2019-04-08T05:56:02Z

draft-ietf-quic-recovery.md

+On the first RTT sample in a connection, the smoothed_rtt is set to the
+latest_rtt.  The peer-reported host delay MUST NOT be used on the first sample,
+since correcting for it would result in a smoothed_rtt that is smaller than the
+min_rtt.


This assertion isn't necessary. Yes, the one sample you have for min_rtt is equal to latest_rtt, so of course any host delay will not be applied. So this just duplicates the earlier requirement. If anything, just note that there is no point in applying host delay corrections as they are never applied.

I think the way the text flows now makes this statement moot. I'm removing it.

draft-ietf-quic-recovery.md

Co-Authored-By: janaiyengar <jri.ietf@gmail.com>

janaiyengar

Thanks for your comments, @martinthomson

janaiyengar · 2019-04-09T00:43:54Z

draft-ietf-quic-recovery.md

+An endpoint uses only locally observed times in generating RTT samples and does
+not correct for any host delays reported by the peer ({{ack-delay}}).
+
+An RTT sample MUST NOT be generated for a packet that is not newly acknowledged.


Samples are generated using both the packet and the ack, so I have used "for packet" a couple of times. I like your phrasing better though, so using it.

janaiyengar · 2019-04-09T01:08:11Z

draft-ietf-quic-recovery.md

+endpoint:
+
+- SHOULD use the lesser of the value reported in Ack Delay field of the ACK
+  frame and the peer's max_ack_delay transport parameter ({{ack-delay}}).


Fair point. Ultimately, this is bounded by another value that the peer advertises (max_ack_delay), so there's no additional safety issue here.

janaiyengar · 2019-04-09T01:12:10Z

draft-ietf-quic-recovery.md

+On the first RTT sample in a connection, the smoothed_rtt is set to the
+latest_rtt.  The peer-reported host delay MUST NOT be used on the first sample,
+since correcting for it would result in a smoothed_rtt that is smaller than the
+min_rtt.


I think the way the text flows now makes this statement moot. I'm removing it.

draft-ietf-quic-recovery.md

martinthomson

This is a big improvement over what we had. Nice work.

draft-ietf-quic-recovery.md

martinthomson · 2019-04-09T01:37:43Z

draft-ietf-quic-recovery.md


 An endpoint MUST NOT excessively delay acknowledgements of ack-eliciting
-packets. Specifically, an endpoint is expected to enforce a maximum delay to
+packets.  Specifically, an endpoint is expected to enforce a maximum delay to
 avoid causing spurious timeouts at the peer.  The maximum ack delay is


The text doesn't say what delay is being "enforced". Maybe:

[...] an endpoint is expected to send acknowledgments within its advertised delay to avoid causing spurious timeouts at its peer.

But maybe you can remove this sentence now that the "contract" text is here.

martinthomson · 2019-04-09T01:39:05Z

draft-ietf-quic-recovery.md

-  latest_rtt = latest_rtt - ack_delay
-smoothed_rtt = 7/8 * smoothed_rtt + 1/8 * latest_rtt
-rttvar_sample = abs(smoothed_rtt - latest_rtt)
+  corrected_rtt = latest_rtt - ack_delay


Maybe adjusted_rtt instead. "Corrected" implies that there is some notion of "correctness" that applies.

ianswett

Thanks for the substantial rework, some comments.

draft-ietf-quic-recovery.md

ianswett · 2019-04-09T21:52:58Z

draft-ietf-quic-recovery.md

 ~~~
 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?

This subsection is talking about generating RTT samples, which do not adjust for host delays.

I would just remove this paragraph, because it's potentially confusing and I'm not sure what it's adding?

Fair, it's not necessary yet, so removed.

ianswett · 2019-04-09T21:53:32Z

draft-ietf-quic-recovery.md

+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?

Same as above -- this section on generating RTT samples doesn't use host delay, but later subsections (srtt, rttvar) do.

Bump, I'm still unclear on what the latter half of the sentence is trying to communicate.

I've rewritten the para, so hopefully it reads better.

draft-ietf-quic-recovery.md

janaiyengar

Thanks, Ian -- updated.

draft-ietf-quic-recovery.md

janaiyengar · 2019-04-09T22:49:37Z

draft-ietf-quic-recovery.md

+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


Yeah, this is going to need some more work anyway (#2596), but I've changed it to a MAY. What complexities does it create?

draft-ietf-quic-recovery.md

janaiyengar · 2019-04-09T23:39:24Z

draft-ietf-quic-recovery.md

 ~~~
 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


This subsection is talking about generating RTT samples, which do not adjust for host delays.

janaiyengar · 2019-04-09T23:40:29Z

draft-ietf-quic-recovery.md

+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


Same as above -- this section on generating RTT samples doesn't use host delay, but later subsections (srtt, rttvar) do.

draft-ietf-quic-recovery.md

janaiyengar

Thanks, Ian, responses below.

draft-ietf-quic-recovery.md

janaiyengar · 2019-04-10T20:16:04Z

draft-ietf-quic-recovery.md

 ~~~
 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


Fair, it's not necessary yet, so removed.

janaiyengar · 2019-04-10T20:21:11Z

draft-ietf-quic-recovery.md

+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've rewritten the para, so hopefully it reads better.

draft-ietf-quic-recovery.md

ianswett

Two small suggestions, but looks good otherwise.

Co-Authored-By: janaiyengar <jri.ietf@gmail.com>

janaiyengar · 2019-04-12T00:08:47Z

Thanks, @ianswett -- I've taken in your suggestions. Merging.

rewrite of RTT estimate sectionbn

524bc02

rewrite of RTT estimation section

janaiyengar requested review from martinthomson and ianswett April 8, 2019 05:29

fix lint

59751b7

martinthomson reviewed Apr 8, 2019

View reviewed changes

janaiyengar and others added 5 commits April 8, 2019 12:11

update pseudocode

bf9f483

Update draft-ietf-quic-recovery.md

f4dddc5

Co-Authored-By: janaiyengar <jri.ietf@gmail.com>

Update draft-ietf-quic-recovery.md

1ec3a95

Co-Authored-By: janaiyengar <jri.ietf@gmail.com>

Update draft-ietf-quic-recovery.md

6bfea40

Co-Authored-By: janaiyengar <jri.ietf@gmail.com>

martin's comments

21a0bda

janaiyengar commented Apr 9, 2019

View reviewed changes

martinthomson approved these changes Apr 9, 2019

View reviewed changes

janaiyengar mentioned this pull request Apr 9, 2019

Should platform delays be included in ack-delay? #2596

Closed

janaiyengar added 4 commits April 8, 2019 19:52

s/corrected/adjusted

c2b9da0

s/ack delay/host delay

f0c860d

moving host delay section to under Generating Acks

cb683af

nit

0b38d2c

ianswett reviewed Apr 9, 2019

View reviewed changes

ian's comments

9be9f1a

janaiyengar commented Apr 9, 2019

View reviewed changes

janaiyengar commented Apr 10, 2019

View reviewed changes

Ian's comments

39b6ef6

ianswett mentioned this pull request Apr 11, 2019

RTT Initial measurement #2607

Closed

ianswett reviewed Apr 11, 2019

View reviewed changes

draft-ietf-quic-recovery.md Outdated Show resolved Hide resolved

ianswett approved these changes Apr 11, 2019

View reviewed changes

Update draft-ietf-quic-recovery.md

9b9973c

Co-Authored-By: janaiyengar <jri.ietf@gmail.com>

martinthomson added -recovery editorial An issue that does not affect the design of the protocol; does not require consensus. labels Apr 12, 2019

Update draft-ietf-quic-recovery.md

9cf7d92

Co-Authored-By: janaiyengar <jri.ietf@gmail.com>

martinthomson removed the editorial An issue that does not affect the design of the protocol; does not require consensus. label Apr 12, 2019

martinthomson added the design An issue that affects the design of the protocol; resolution requires consensus. label Apr 12, 2019

reflow

77d6256

janaiyengar merged commit 5f8a1ce into master Apr 12, 2019

janaiyengar mentioned this pull request Apr 12, 2019

Acks for non-ack-eliciting packets do not generate RTT samples h2o/quicly#144

Merged

ianswett mentioned this pull request Apr 15, 2019

Reduce restrictions on valid RTT samples #2568

Closed

martinthomson deleted the jri/acking branch April 30, 2019 22:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite of RTT estimation section #2592

Rewrite of RTT estimation section #2592

janaiyengar commented Apr 8, 2019

martinthomson Apr 8, 2019

janaiyengar Apr 9, 2019

martinthomson Apr 8, 2019

janaiyengar Apr 9, 2019

janaiyengar left a comment

janaiyengar Apr 9, 2019

janaiyengar Apr 9, 2019

janaiyengar Apr 9, 2019

martinthomson left a comment

martinthomson Apr 9, 2019

martinthomson Apr 9, 2019

ianswett left a comment

ianswett Apr 9, 2019

janaiyengar Apr 9, 2019

ianswett Apr 10, 2019

janaiyengar Apr 10, 2019

ianswett Apr 9, 2019

janaiyengar Apr 9, 2019

ianswett Apr 10, 2019

janaiyengar Apr 10, 2019

janaiyengar left a comment

janaiyengar Apr 9, 2019

janaiyengar Apr 9, 2019

janaiyengar Apr 9, 2019

janaiyengar left a comment

janaiyengar Apr 10, 2019

janaiyengar Apr 10, 2019

ianswett left a comment

janaiyengar commented Apr 12, 2019

Rewrite of RTT estimation section #2592

Rewrite of RTT estimation section #2592

Conversation

janaiyengar commented Apr 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janaiyengar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinthomson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianswett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janaiyengar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janaiyengar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianswett left a comment

Choose a reason for hiding this comment

janaiyengar commented Apr 12, 2019