Subject: [quicwg/base-drafts] Replace end of recovery with recovery_start_time (#1930)

From: ianswett <[email-masked]>
Date: Mon, 29 Oct 2018 07:49:54 -0700

Fixes #1928
You can view, comment on, or merge this pull request online at:

  https://github.com/quicwg/base-drafts/pull/1930

-- Commit Summary --

  * Replace end of recovery with recovery_start_time

-- File Changes --

    M draft-ietf-quic-recovery.md (15)

-- Patch Links --

https://github.com/quicwg/base-drafts/pull/1930.patch
https://github.com/quicwg/base-drafts/pull/1930.diff

-- 
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/1930

---

From: ianswett <[email-masked]>
Date: Mon, 29 Oct 2018 07:52:54 -0700

@ianswett pushed 1 commit.

caad3e3  Update draft-ietf-quic-recovery.md

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/1930/files/a5b9bba9eb032601e99df3f9c837364fadbc0654..caad3e3123700a68b9c81e3ddd207fce6d4e0c56

---

From: ianswett <[email-masked]>
Date: Mon, 29 Oct 2018 14:53:50 +0000 (UTC)

51b24a3  Update draft-ietf-quic-recovery.md

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/1930/files/caad3e3123700a68b9c81e3ddd207fce6d4e0c56..51b24a3adbe7088e8637b2d30a31060b81dba5ac

---

From: ianswett <[email-masked]>
Date: Mon, 29 Oct 2018 07:58:51 -0700

61d27f4  Update draft-ietf-quic-recovery.md

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/1930/files/51b24a3adbe7088e8637b2d30a31060b81dba5ac..61d27f4a436abc3d720a3b33bac4aa4b84880da0

---

From: Masakazu Kitajo <[email-masked]>
Date: Mon, 29 Oct 2018 17:50:28 -0700

maskit approved this pull request.

Looks good to me.

-- 
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/1930#pullrequestreview-169576073

---

From: ianswett <[email-masked]>
Date: Mon, 29 Oct 2018 18:03:33 -0700

778d903  Update draft-ietf-quic-recovery.md

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/1930/files/61d27f4a436abc3d720a3b33bac4aa4b84880da0..778d90347aed1852daa0717f09cc78f0ed8be489

---

From: ianswett <[email-masked]>
Date: Tue, 30 Oct 2018 01:10:39 +0000 (UTC)

4dfbced  Update draft-ietf-quic-recovery.md

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/1930/files/778d90347aed1852daa0717f09cc78f0ed8be489..4dfbced983f2763691ec22fab8a9a57294bccb67

---

From: Masakazu Kitajo <[email-masked]>
Date: Mon, 29 Oct 2018 18:34:01 -0700

maskit commented on this pull request.

> @@ -1130,8 +1130,9 @@ Invoked when an ACK frame with an ECN section is received from the peer.
      if (ack.ce_counter > ecn_ce_counter):
        ecn_ce_counter = ack.ce_counter
        // Start a new congestion event if the last acknowledged
-       // packet is past the end of the previous recovery epoch.
-       CongestionEvent(ack.largest_acked_packet)
+       // packet was sent after the start of the previous
+       // recovery epoch.
+       CongestionEvent(sent_packets[ack.largest_acked].time)

I'm not sure if this is really an issue but `sent_packets` is a variable described in Loss Detection section. Other places except `OnRetransmissionTimeoutVerified ` don't use any of the variables described in Loss Detection section. This makes dependency for Loss Detection from Congestion Control.

If this function receive a value of `sent_packets[ack.largest_acked].time` as a parameter, we can remove the dependency.

-- 
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/1930#pullrequestreview-169582777

---

From: ianswett <[email-masked]>
Date: Mon, 29 Oct 2018 18:37:57 -0700

ianswett commented on this pull request.

I'm happy to have congestion control depend upon loss detection, but it likely would be cleaner to pass in the time directly.

-- 
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/1930#discussion_r229151810

---

From: ianswett <[email-masked]>
Date: Mon, 29 Oct 2018 18:39:23 -0700

Actually, we're using the ce counter as well.  I think I'll leave it as is unless you think there's a correctness issue?

-- 
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/1930#discussion_r229151979

---

From: Masakazu Kitajo <[email-masked]>
Date: Mon, 29 Oct 2018 19:00:55 -0700

> I'm happy to have congestion control depend upon loss detection, but it likely would be cleaner to pass in the time directly.

OK, I just thought other places might avoid it intentionally.

> Actually, we're using the ce counter as well.

If you mean `ack.ce_counter` that one is fine because it is in `ack`. I meant `ProcessECN(ack, largest_acked_sent_time)`

> I think I'll leave it as is unless you think there's a correctness issue?

I'd implement it differently but I'm ok with as-is.

-- 
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/1930#discussion_r229154830

---

From: ianswett <[email-masked]>
Date: Tue, 30 Oct 2018 07:07:35 -0700

Merged #1930 into master.

-- 
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/1930#event-1935155921