Subject: Re: [quicwg/base-drafts] Resetting min_rtt (#3975)

From: ianswett <[email-masked]>
Date: Sat, 01 Aug 2020 18:58:00 -0700

@ianswett approved this pull request.

-- 
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/3975#pullrequestreview-459614318

---

From: Kazuho Oku <[email-masked]>
Date: Sat, 01 Aug 2020 19:37:32 -0700

@kazuho approved this pull request.

I like the cautionary text. Just one suggestion but LGTM regardless. Thank you for working on this.

>  future RTT samples that are smaller than the new RTT to be included in
 smoothed_rtt.
 
+Endpoints SHOULD set the min_rtt to the newest RTT sample after persistent
+congestion is established. This is to allow a connection to reset its estimate
+of min_rtt and smoothed_rtt ({{smoothed-rtt}}) after a disruptive network event,
+and because it is possible that an increase in path delay resulted in persistent
+congestion being incorrectly declared.
+
+Endpoints MAY set the min_rtt to the newest RTT sample at other times in the

```suggestion
Endpoints MAY reestablish min_rtt at other times in the
```

I think it might be a good idea to imply that there could be other ways of refreshing min_rtt rather than just resetting it to the newest sample. To give an example, an endpoint can remember minimum of 100 most recently received packets, then replace `min_rtt` to that value.

-- 
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/3975#pullrequestreview-459615499

---

From: Jana Iyengar <[email-masked]>
Date: Sun, 02 Aug 2020 14:48:47 -0700

@janaiyengar pushed 1 commit.

3187cb3b5d05ee6d7c69b1b609412376439913f9  marten and kazuho comments

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3975/files/af88eeabd3f9e25c0eefebcb9beef7db3ccc8c45..3187cb3b5d05ee6d7c69b1b609412376439913f9

---

From: Martin Thomson <[email-masked]>
Date: Mon, 03 Aug 2020 17:56:52 -0700

@martinthomson commented on this pull request.

Seems fine.  I'm not 100% on the strength of the recommendation to reset after persistent congestion, but it's sensible advice.

 smoothed_rtt.
 
+Endpoints SHOULD set the min_rtt to the newest RTT sample after persistent
+congestion is established. This is to allow a connection to reset its estimate
+of min_rtt and smoothed_rtt ({{smoothed-rtt}}) after a disruptive network event,
+and because it is possible that an increase in path delay resulted in persistent
+congestion being incorrectly declared.
+
+Endpoints MAY re-establish the min_rtt at other times in the connection, such as
+when traffic volume is low and an acknowledgement is received with a low
+acknowledgement delay. Implementations should be careful not to refresh this

```suggestion
acknowledgement delay. Implementations SHOULD NOT refresh the min_rtt
```

 smoothed_rtt.
 
+Endpoints SHOULD set the min_rtt to the newest RTT sample after persistent
+congestion is established. This is to allow a connection to reset its estimate
+of min_rtt and smoothed_rtt ({{smoothed-rtt}}) after a disruptive network event,
+and because it is possible that an increase in path delay resulted in persistent
+congestion being incorrectly declared.
+
+Endpoints MAY re-establish the min_rtt at other times in the connection, such as
+when traffic volume is low and an acknowledgement is received with a low
+acknowledgement delay. Implementations should be careful not to refresh this
+value too frequently, since the actual minimum RTT of the path may not be

```suggestion
value too often, since the actual minimum RTT of the path is not
```

-- 
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/3975#pullrequestreview-460426778

---

From: Jana Iyengar <[email-masked]>
Date: Wed, 05 Aug 2020 22:32:43 -0700

bea06b7f7326d6d636ffc65580a6233d6f5cb9f1  Apply suggestions from code review

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3975/files/3187cb3b5d05ee6d7c69b1b609412376439913f9..bea06b7f7326d6d636ffc65580a6233d6f5cb9f1

---

From: ianswett <[email-masked]>
Date: Mon, 10 Aug 2020 17:11:27 -0700

-- 
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/3975#pullrequestreview-464666918

---

From: Christian Huitema <[email-masked]>
Date: Fri, 21 Aug 2020 08:46:49 -0700

@huitema commented on this pull request.

The resetting of min RTT is also specified with LEDBAT or BBR, which differ from each other and from this proposed spec. This is a complex issue, because resetting min RTT in LEDBAT "because of congestion" is a variation of the latecomer advantage. But as long as the text applies only to RENO as specified here, then I am fine with it.

-- 
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/3975#pullrequestreview-472600485