Subject: [quicwg/base-drafts] First version of congestion control (#395)

From: ianswett <[email-masked]>
Date: Mon, 13 Mar 2017 14:02:57 -0700

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

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

-- Commit Summary --

  * First version of congestion control

-- File Changes --

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

-- Patch Links --

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

---

From: janaiyengar <[email-masked]>
Date: Mon, 13 Mar 2017 14:15:22 -0700

janaiyengar commented on this pull request.

> @@ -202,8 +202,8 @@ kDefaultInitialRtt (default 100ms):
 
 ## Variables of interest
 
-We first describe the variables required to implement the loss detection
-mechanisms described in this section.
+Variables required to implement the congestion control mechanisms
+described in this section.

"are described in this section"

>  ~~~
 
 # Congestion Control
 
-(describe NewReno-style congestion control {{?RFC6582}} for QUIC.)
-(describe appropriate byte counting.)
-(define recovery based on packet numbers.)
+QUIC's congestion control is based on TCP New Reno{{?RFC6582}}

NewReno -- 1 word

>  ~~~
 
 # Congestion Control
 
-(describe NewReno-style congestion control {{?RFC6582}} for QUIC.)
-(describe appropriate byte counting.)
-(define recovery based on packet numbers.)
+QUIC's congestion control is based on TCP New Reno{{?RFC6582}}
+congestion control to determine the congestion window and pacing rate.
+
+## Slow Start
+
+QUIC uses a slow start approach where the congestion window is increased
+by the same number of bytes as are acknowledged while in slow start.
+QUIC begins every connection in slow start and exits slow start upon
+loss or when the min_rtt increases by more than 1/8th for a full round.

remove the 1/8th RTT -- that's mixing hystart into this doc

> +QUIC's congestion control is based on TCP New Reno{{?RFC6582}}
+congestion control to determine the congestion window and pacing rate.
+
+## Slow Start
+
+QUIC uses a slow start approach where the congestion window is increased
+by the same number of bytes as are acknowledged while in slow start.
+QUIC begins every connection in slow start and exits slow start upon
+loss or when the min_rtt increases by more than 1/8th for a full round.
+
+## Recovery
+
+Recovery is a period of time beginning with detection of a lost packet.
+It ends when all packets outstanding at the time recovery began have been
+acknowledged or lost. During recovery, the congestion window is not
+increased and the pacing gain is reduced to 1.

I'd remove mention of "pacing gain"

> +     else:
+       congestion_window +=
+           1500 * acked_packets.bytes / congestion_window
+~~~
+
+## On Packets Lost
+
+Invoked by loss detection from DetectLostPackets when new packets
+are detected lost.
+
+~~~
+   OnPacketsLost(lost_packets):
+     largest_lost_packet = lost_packets.last()
+     if (end_of_recovery < largest_lost_packet.packet_number):
+       end_of_recovery = largest_sent_packet
+       congestion_window *= kLossReductionFactor

ssthresh = congestion_window

> +~~~
+
+## On Packets Lost
+
+Invoked by loss detection from DetectLostPackets when new packets
+are detected lost.
+
+~~~
+   OnPacketsLost(lost_packets):
+     largest_lost_packet = lost_packets.last()
+     if (end_of_recovery < largest_lost_packet.packet_number):
+       end_of_recovery = largest_sent_packet
+       congestion_window *= kLossReductionFactor
+~~~
+
+## Sending Packets

I would rename this section  "Pacing Packets" or something like it

> +
+QUIC sends a packet if there is available congestion window and
+sending the packet does not exceed the pacing rate.
+
+TimeToSend returns infinite if the congestion controller is congestion
+window limited, a time in the past if the packet can be sent
+immediately, and a time in the future if sending is pacing limited.
+
+~~~
+   TimeToSend(packet_size):
+     if (bytes_in_flight + packet_size > congestion_window)
+       return infinite
+     return time_of_last_sent_packet + 
+         packet_size * smoothed_rtt / congestion_window
+~~~
+
 (describe min_rtt based hystart.)

Remove this line

-- 
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/395#pullrequestreview-26667457

---

From: ianswett <[email-masked]>
Date: Thu, 16 Mar 2017 08:04:45 -0700

ianswett commented on this pull request.

 ## Variables of interest

Done

-- 
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/395#discussion_r106441852

---

From: ianswett <[email-masked]>
Date: Thu, 16 Mar 2017 08:05:00 -0700

# Congestion Control

Done

-- 
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/395#discussion_r106441934

---

From: ianswett <[email-masked]>
Date: Thu, 16 Mar 2017 08:05:47 -0700

# Congestion Control

Oh, NewReno doesn't have hystart?  How 80s.

-- 
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/395#discussion_r106442189

---

From: ianswett <[email-masked]>
Date: Thu, 16 Mar 2017 08:06:16 -0700

Done.

-- 
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/395#discussion_r106442336

---

From: ianswett <[email-masked]>
Date: Thu, 16 Mar 2017 08:07:11 -0700

+       congestion_window +=

Thanks, I forgot about ssthresh.

-- 
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/395#discussion_r106442650

---

From: ianswett <[email-masked]>
Date: Thu, 16 Mar 2017 08:21:04 -0700

@ianswett pushed 1 commit.

b35e297  Added ssthresh and small changes

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/395/files/88de9372a69205b5878ca52b7a0bb47f8756962d..b35e297ef3067bd418d2b45e3b4676bce65d55af

---

From: ianswett <[email-masked]>
Date: Thu, 16 Mar 2017 08:28:22 -0700

127a576  Trailing whitespace

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/395/files/b35e297ef3067bd418d2b45e3b4676bce65d55af..127a5768d0d7e8e8ecb5272aa8e5ec9d8aa4ca56

---

From: janaiyengar <[email-masked]>
Date: Thu, 16 Mar 2017 11:54:33 -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/395#pullrequestreview-27428840

---

From: ianswett <[email-masked]>
Date: Thu, 16 Mar 2017 12:04:27 -0700

Merged #395.

-- 
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/395#event-1003328303