Subject: [quicwg/base-drafts] Incorporate F-RTO (#409)

From: ianswett <[email-masked]>
Date: Sat, 18 Mar 2017 06:49:35 -0700

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

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

-- Commit Summary --

  * Incorporate F-RTO

-- File Changes --

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

-- Patch Links --

https://github.com/quicwg/base-drafts/pull/409.patch
https://github.com/quicwg/base-drafts/pull/409.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/409

---

From: ianswett <[email-masked]>
Date: Sat, 18 Mar 2017 07:00:19 -0700

@ianswett pushed 1 commit.

6aa7a27  Fix trailing whitespace

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/409/files/f14ba70658b33562c29771456d9e3edad9a85d66..6aa7a2764150aa4ca12c2ecebc9f5b7a8f9818a6

---

From: ianswett <[email-masked]>
Date: Sat, 18 Mar 2017 07:22:22 -0700

ac0e9f4  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/409/files/6aa7a2764150aa4ca12c2ecebc9f5b7a8f9818a6..ac0e9f4590f317604aa1049d1eefc8fbffdd6147

---

From: ianswett <[email-masked]>
Date: Sat, 18 Mar 2017 07:30:10 -0700

ac603a7  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/409/files/ac0e9f4590f317604aa1049d1eefc8fbffdd6147..ac603a7faff1c94e0fef120f1fad2b51d8a71e13

---

From: ianswett <[email-masked]>
Date: Sat, 18 Mar 2017 07:35:23 -0700

615daa2  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/409/files/ac603a7faff1c94e0fef120f1fad2b51d8a71e13..615daa2cb890f016d41c8713d46b115afce6f7c8

---

From: janaiyengar <[email-masked]>
Date: Tue, 21 Mar 2017 12:45:54 -0700

janaiyengar commented on this pull request.

Thanks for writing this up, Ian.  A few comments

> @@ -248,6 +248,10 @@ tlp_count:
 rto_count:
 : The number of times an rto has been sent without receiving an ack.
 
+largest_sent_before_rto:
+: The last packet number sent prior to the first transmission due to

nit: change phrase to "prior to the first retransmission timeout"

> @@ -372,6 +377,12 @@ Pseudocode for OnPacketAcked follows:
 
 ~~~
    OnPacketAcked(acked_packet_number):
+     // If a packet sent prior to RTO was acked, then the RTO
+     // was spurious.  Otherwise, inform congestion control.
+     // Similar to the goal of F-RTO {{?RFC5682}}

I would remove this last line from here and add it (now or later) to the text description.

>       if (end_of_recovery < largest_lost_packet.packet_number):
        end_of_recovery = largest_sent_packet
        congestion_window *= kLossReductionFactor
+       congestion_window = max(congestion_window, kMinimumWindow)

nit: combine this line with the previous one:
congestion_window = 
    max(congestion_window * kLossReductionFactor, kMinimumWindow)

 ~~~
    TimeToSend(packet_size):
      if (bytes_in_flight + packet_size > congestion_window)
        return infinite
+     pacing_coefficient = 1.25
+     if (congestion_window < ssthresh)
+       pacing_coefficient = 2

I don't think we need a pacing coeff of 2 here. I never understood why FQ did this, but I don't think we need to have this in the doc. In general I'm less certain that we want to describe the pacing impl as part of the congestion controller... I'm comfortable with definiing the TimeToSend interface without implementing it here. For now, I would at least remove the use of pacing_coeff here.

-- 
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/409#pullrequestreview-28215606

---

From: ianswett <[email-masked]>
Date: Tue, 21 Mar 2017 13:44:27 -0700

ianswett commented on this pull request.

I tried, but it ends up taking up 3 lines, so I just kept it 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/409#discussion_r107270104

---

From: ianswett <[email-masked]>
Date: Tue, 21 Mar 2017 13:45:41 -0700

1be14db  Jana's review comments

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/409/files/615daa2cb890f016d41c8713d46b115afce6f7c8..1be14db55745f2a9f01598e5621afa0b75a7e691

---

From: ianswett <[email-masked]>
Date: Tue, 21 Mar 2017 13:51:09 -0700

65c9987  Line length.

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/409/files/1be14db55745f2a9f01598e5621afa0b75a7e691..65c9987ee8662f0e80ec7d6140cf0a74e767e784

---

From: Mike Bishop <[email-masked]>
Date: Sun, 26 Mar 2017 08:00:27 -0700

@MikeBishop pushed 1 commit.

06bde72  Shorten RFC Editor's Note line

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/409/files/65c9987ee8662f0e80ec7d6140cf0a74e767e784..06bde72918e1bf86236f227774990cfd4b40659e

---

From: janaiyengar <[email-masked]>
Date: Sun, 26 Mar 2017 08:48:04 -0700

janaiyengar 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/409#pullrequestreview-29072751

---

From: janaiyengar <[email-masked]>
Date: Sun, 26 Mar 2017 08:48:12 -0700

Merged #409.

-- 
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/409#event-1015716183