Subject: [quicwg/base-drafts] Limit CWND increase in slow start (#3232)

From: Vidhi Goel <[email-masked]>
Date: Tue, 12 Nov 2019 13:56:42 -0800

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

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

-- Commit Summary --

  * limit cwnd in slow start pseudocode update
  * Jana&#39;s comments and Ian&#39;s text change

-- File Changes --

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

-- Patch Links --

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

---

From: ianswett <[email-masked]>
Date: Tue, 12 Nov 2019 14:04:51 -0800

ianswett commented on this pull request.

> @@ -1368,7 +1377,14 @@ acked_packet from sent_packets.
        return
      if (congestion_window < ssthresh):
        // Slow start.
-       congestion_window += acked_packet.size
+       if (pacing):
+         congestion_window += acked_packet.size
+       else:
+         // If pacing is not used, limit the increase to
+         // 2*max_datagram_size per ACK as described in {{?RFC3465}}
+         if (cwnd_increase < 2 * max_datagram_size):
+           cwnd_increase += bytes_acked

cwnd_increase doesn't appear to be used anywhere to change cwnd/etc.  Are more changes necessary?

-- 
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/3232#pullrequestreview-315876486

---

From: Vidhi Goel <[email-masked]>
Date: Tue, 12 Nov 2019 14:20:37 -0800

@goelvidhi pushed 1 commit.

f2fc15e0309f04e49363aae515b8bfcf25302fae  build issue

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

---

From: Vidhi Goel <[email-masked]>
Date: Tue, 12 Nov 2019 14:25:31 -0800

goelvidhi commented on this pull request.

        return

It is only used to limit the increase to 2*MSS in slow start

-- 
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/3232#discussion_r345478346

---

From: Junho Choi <[email-masked]>
Date: Tue, 12 Nov 2019 15:30:53 -0800

junhochoi commented on this pull request.

        return

`bytes_acked` doesn't appear anywhere. I think it should be `acked_packet.size`?

-- 
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/3232#discussion_r345501217

---

From: Junho Choi <[email-masked]>
Date: Tue, 12 Nov 2019 15:31:27 -0800

> @@ -1368,7 +1377,15 @@ acked_packet from sent_packets.
        return
      if (congestion_window < ssthresh):
        // Slow start.
-       congestion_window += acked_packet.size
+       if (pacing):
+         congestion_window += acked_packet.size
+       else:
+         // If pacing is not used, limit the increase to
+         // 2*max_datagram_size per ACK as described in
+         // {{?RFC3465}}
+         if (cwnd_increase < 2 * max_datagram_size):
+           cwnd_increase += bytes_acked
+           congestion_window += bytes_acked

Same issue as L1387 (`bytes_acked`)

-- 
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/3232#pullrequestreview-315915258

---

From: Junho Choi <[email-masked]>
Date: Tue, 12 Nov 2019 15:48:02 -0800

return

I think this may allow increasing cwnd close to (3 x max_datagram_size).

e.g. max_datagram_size = 1000
ack 1 = 999 bytes
ack 2 = 999 bytes
ack 3 = 999 bytes

If we have 3 acks each acks 999 bytes, this code result in 999x3 bytes cwnd increase. 

To calculate remaining bytes correctly, I would propose the following:

```
  OnAckReceivedCC():
     cwnd_incr_max = 2 * max_datagram_size
...
  OnPacketAckedCC(acked_packet):
...
         // If pacing is not used, limit the increase to
         // 2*max_datagram_size per ACK as described in
         // {{?RFC3465}}
         cwnd_incr = cwnd_incr_max - max(cwnd_incr_max - acked_packet.size, 0)
         congestion_window += cwnd_incr
         cwnd_incr_max -= cnwd_incr
```

-- 
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/3232#discussion_r345506253

---

From: ianswett <[email-masked]>
Date: Tue, 12 Nov 2019 16:26:27 -0800

Thanks for your PR, it's helpful to see this clarified.

I think it's worth a discussion of two issues, maybe in Singapore:
1) Do we need anything in addition to the existing IW burst limit?  A single ACK loss would not cause a burst to exceed the IW10 limit, and multiple consecutive ACK losses in slow start are VERY rare in my experience.
2) Do we need to describe the non-pacing scenarios in the pseudocode, given we recommend pacing?  Using CWND to manipulate sending behavior is an indirect method that feels like a holdover from implementation details of older TCP algorithms to me.

I'm no on both counts currently, but if the WG feels otherwise, I can accept that.

-- 
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/3232#issuecomment-553180443

---

From: Vidhi Goel <[email-masked]>
Date: Tue, 12 Nov 2019 22:26:41 -0800

return

oops, that's correct. Fixing 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/3232#discussion_r345587254

---

From: Vidhi Goel <[email-masked]>
Date: Tue, 12 Nov 2019 22:31:05 -0800

return

good point. How about the below:
```
 if (pacing):
         congestion_window += acked_packet.size
       else:
         // If pacing is not used, limit the increase to
         // 2*max_datagram_size per ACK as described in
         // {{?RFC3465}}
         if (cwnd_increase + acked_packet.size <=
             2 * max_datagram_size):
           cwnd_increase += acked_packet.size
           congestion_window += acked_packet.size
```

-- 
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/3232#discussion_r345588269

---

From: Vidhi Goel <[email-masked]>
Date: Tue, 12 Nov 2019 22:39:51 -0800

eaea37635a08bf69558a650388c622dc319bd4a0  junhochoi's comment about 3 *MSS

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

---

From: Vidhi Goel <[email-masked]>
Date: Wed, 13 Nov 2019 14:49:21 -0800

return

There is a problem with my suggested code that the cwnd could end up being less than 2*MSS.

This is my updated proposal:
```
if (pacing):
         congestion_window += acked_packet.size
       else:
         // If pacing is not used, limit the increase to
         // 2*max_datagram_size per ACK as described in
         // {{?RFC3465}}
         if (cwnd_increase  < 2 * max_datagram_size):
           cwnd_increase = min(cwnd_increase + acked_packet.size,
                                            2 * max_datagram_size)
           congestion_window = cwnd_increase
```

-- 
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/3232#discussion_r346044805

---

From: Junho Choi <[email-masked]>
Date: Wed, 13 Nov 2019 21:46:36 -0800

return

>            congestion_window = cwnd_increase

I think it will reset congestion_window to a small value (<=2 mss).

-- 
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/3232#discussion_r346135502

---

From: Vidhi Goel <[email-masked]>
Date: Thu, 14 Nov 2019 11:28:16 -0800

return

@junhochoi could you give an example where it would be less than 2MSS?

-- 
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/3232#discussion_r346503804

---

From: Junho Choi <[email-masked]>
Date: Thu, 14 Nov 2019 17:34:54 -0800

return

> congestion_window = cwnd_increase

If you run my example above (3 acks 999 bytes each), cwin will be 2 x mss at the end with algorithms above.

I think you mean `+=`, not `=`, but if so it makes another issue (cwin will increase much faster than expected)

-- 
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/3232#discussion_r346628814

---

From: Junho Choi <[email-masked]>
Date: Thu, 14 Nov 2019 17:47:40 -0800

return

Last line is problematic because it will reset cwnd to cwnd_increase which is capped to 2 x mss.
```congestion_window = cwnd_increase```

-- 
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/3232#discussion_r346631453

---

From: Vidhi Goel <[email-masked]>
Date: Sun, 17 Nov 2019 12:04:19 -0800

return

Isn't capping to 2*MSS the desired congestion window? `=` is my intended proposal to cap the congestion window to 2MSS.

-- 
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/3232#discussion_r347156144

---

From: Vidhi Goel <[email-masked]>
Date: Sun, 17 Nov 2019 12:14:11 -0800

> * Do we need anything in addition to the existing IW burst limit?  A single ACK loss would not cause a burst to exceed the IW10 limit, and multiple consecutive ACK losses in slow start are VERY rare in my experience.
> * Do we need to describe the non-pacing scenarios in the pseudocode, given we recommend pacing?  Using CWND to manipulate sending behavior is an

I agree that increasing congestion window and controlling sending rate are orthogonal in theory. But in practice, congestion window is being used in many implementations to indirectly control the sending rate. I think the addition of RFC3465 text and pseudocode is more for completeness than what should be ideal behavior.

-- 
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/3232#issuecomment-554781647

---

From: Junho Choi <[email-masked]>
Date: Sun, 17 Nov 2019 20:45:44 -0800

return

I believe the basic intention of RFC3465 (section 2.3) is limiting cwnd *increase* during slow start per ACK,  (e.g. one ACK acknowledges 4 MSS --> only increase cwnd by 2 MSS, not 4 MSS), not resetting cwnd. If cwnd is reset to 2xMSS, it is basically same to idle timeout scenario.

-- 
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/3232#discussion_r347199932

---

From: Vidhi Goel <[email-masked]>
Date: Mon, 18 Nov 2019 02:07:19 -0800

return

My bad. I would have to fix this. 

-- 
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/3232#discussion_r347291704

---

From: Vidhi Goel <[email-masked]>
Date: Mon, 18 Nov 2019 12:58:02 -0800

return

The below should work in all cases:
```
if (pacing):
         congestion_window += acked_packet.size
       else:
         // If pacing is not used, limit the increase to
         // 2*max_datagram_size per ACK as described in
         // {{?RFC3465}}
         if (cwnd_increase  < 2 * max_datagram_size):
           old_cwnd_increase = cwnd_increase
           cwnd_increase = min(cwnd_increase + acked_packet.size,
                                            2 * max_datagram_size)
           congestion_window += cwnd_increase - old_cwnd_increase
```

-- 
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/3232#discussion_r347605376

---

From: Junho Choi <[email-masked]>
Date: Mon, 18 Nov 2019 13:11:10 -0800

return

See mine at https://github.com/quicwg/base-drafts/pull/3232#discussion_r345506253
Basically same approach as yours. Note that I removed `if` and use `max()` instead of `min()`. I think either way works fine.

-- 
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/3232#discussion_r347611109

---

From: Vidhi Goel <[email-masked]>
Date: Mon, 18 Nov 2019 13:50:24 -0800

21953f431d2cc0df58c87e12e609df9ada781618  fix the congestion window growth

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3232/files/eaea37635a08bf69558a650388c622dc319bd4a0..21953f431d2cc0df58c87e12e609df9ada781618

---

From: Vidhi Goel <[email-masked]>
Date: Mon, 18 Nov 2019 13:55:14 -0800

return

I pushed my proposed changes before reading your comment. :-) I am fine with both approaches.
This issue will be discussed at IETF either today or tomorrow; we can decide it there.

-- 
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/3232#discussion_r347629917

---

From: Jana Iyengar <[email-masked]>
Date: Mon, 18 Nov 2019 17:51:36 -0800

janaiyengar commented on this pull request.

Now that it's all put together, I think Ian's is a fair question. I'd also like to take the working group's sense on whether we should have this change in the pseudo code. Having it in the text makes sense to me -- that's just parity with TCP for non-paced QUIC flows.

That said, the pseudo code is in the Appendix for a reason -- it's perfectly reasonable to add things there or leave things out as we see fit. The code is an example of how things might be done. 

> @@ -775,7 +775,10 @@ Implementations MUST either use pacing or limit such bursts to the initial
 congestion window, which is recommended to be the minimum of
 10 * max_datagram_size and max(2* max_datagram_size, 14720)), where
 max_datagram_size is the current maximum size of a datagram for the connection,
-not including UDP or IP overhead.
+not including UDP or IP overhead. Implementations not using a pacer SHOULD

```suggestion
not including UDP or IP overhead. An implementation that does not use pacing SHOULD
```

-- 
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/3232#pullrequestreview-318739432

---

From: Vidhi Goel <[email-masked]>
Date: Mon, 18 Nov 2019 18:39:10 -0800

50812828d969df1f5c5b239671640717a6b30463  Update the text to limit cwnd

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3232/files/21953f431d2cc0df58c87e12e609df9ada781618..50812828d969df1f5c5b239671640717a6b30463

---

From: Vidhi Goel <[email-masked]>
Date: Mon, 18 Nov 2019 18:42:07 -0800

08fd5831859edb0814558d158ce58c8203af4d28  fix build issue

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3232/files/50812828d969df1f5c5b239671640717a6b30463..08fd5831859edb0814558d158ce58c8203af4d28

---

From: Junho Choi <[email-masked]>
Date: Mon, 18 Nov 2019 19:14:35 -0800

> @@ -1369,7 +1378,17 @@ acked_packet from sent_packets.
        return

```suggestion
         // 2 * max_datagram_size per ACK as described in
```

-- 
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/3232#pullrequestreview-318762485

---

From: Vidhi Goel <[email-masked]>
Date: Mon, 18 Nov 2019 19:18:53 -0800

e7d59817de98e39ad799be9f1428f82d3b1490fd  update comments

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

---

From: Jana Iyengar <[email-masked]>
Date: Wed, 20 Nov 2019 02:31:10 -0800

The sense in the room seems to be to document in the pseudo code the recommendations in the text.  Given that, I would suggest removing the pseudo code from this PR.

-- 
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/3232#issuecomment-555942383

---

From: Vidhi Goel <[email-masked]>
Date: Mon, 25 Nov 2019 18:27:09 -0800

@goelvidhi pushed 2 commits.

105de2940e571c32d1d9a5fc68fbbe17b2360714  remove pseudocode
78c27a726e47eefc2dd89a903448b93f1d28c241  Merge branch 'master' of github.com:quicwg/base-drafts into limit-cwnd

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3232/files/e7d59817de98e39ad799be9f1428f82d3b1490fd..78c27a726e47eefc2dd89a903448b93f1d28c241

---

From: ianswett <[email-masked]>
Date: Tue, 26 Nov 2019 22:05:45 -0800

> @@ -769,7 +769,10 @@ Implementations MUST either use pacing or limit such bursts to the initial
 congestion window, which is recommended to be the minimum of
 10 * max_datagram_size and max(2* max_datagram_size, 14720)), where
 max_datagram_size is the current maximum size of a datagram for the connection,
-not including UDP or IP overhead.
+not including UDP or IP overhead. An implementation that does not use pacing
+SHOULD limit the congestion window increase during slow start to

In order to meet the previous criteria, how about "An implementation that doest not use pacing MAY limit the congestion window increase during slow start to 2 * max_datagram_size_ per ACK frame received {{?RFC3465}} in order to help limit bursts to the  initial congestion window."

-- 
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/3232#pullrequestreview-323445010

---

From: Vidhi Goel <[email-masked]>
Date: Tue, 26 Nov 2019 22:56:57 -0800

I think we can say either "MAY" or "SHOULD" instead of MAY/SHOULD. I am ok with changing it to "MAY" as there is already a requirement for limiting it to 10 datagrams in the previous 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/3232#discussion_r351119764

---

From: Jana Iyengar <[email-masked]>
Date: Thu, 28 Nov 2019 03:52:40 -0800

We've already said that a sender MUST limit bursts to IW, so there's no reason to repeat that. I would argue this is a SHOULD-level requirement; MAY is strange, since that's really a permission and that's not what we're going after here. I'd suggest dropping the phrase "in order to limit bursts" --- "An implementation that does not use pacing SHOULD limit the congestion window increase during slow start to 2 * max_datagram_size per ACK frame received {{?RFC3465}}. "

-- 
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/3232#discussion_r351739490

---

From: Vidhi Goel <[email-masked]>
Date: Thu, 28 Nov 2019 23:54:49 -0800

c8a59b2fb1e716bb7dba6075567c538bfcce861a  remove the burst mention

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3232/files/78c27a726e47eefc2dd89a903448b93f1d28c241..c8a59b2fb1e716bb7dba6075567c538bfcce861a

---

From: Vidhi Goel <[email-masked]>
Date: Thu, 28 Nov 2019 23:55:01 -0800

sounds good to me. Updated 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/3232#discussion_r352013129