Subject: [quicwg/base-drafts] Byte counting at Congestion avoidance (#3917)

From: Junho Choi <[email-masked]>
Date: Thu, 16 Jul 2020 10:11:49 -0700

There is an issue in current congestion avoidance pseudocode
when `max_datagram_size * acked_packet.sent_bytes &lt; congestion_window`
cwnd increment effectively becomes zero, especially because most
implementations use integer operation to calculate cwnd.

This happens when `acked_packet.sent_bytes` is a small value or
`congestion_window` is very big. It may lead to too conversavative
cwnd growth during congestion avoidance.

To fix the issue, I propose to use a TCP ABC style cwnd growth
during congestion avoidance: https://www.rfc-editor.org/rfc/rfc3465.html#section-2.1

Basically it stores accumulated acked bytes in a new
variable `bytes_acked` and increate cwnd by `max_datagram_size` when
`bytes_acked` reaches to cwnd.
You can view, comment on, or merge this pull request online at:

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

-- Commit Summary --

  * Byte counting at Congestion avoidance

-- File Changes --

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

-- Patch Links --

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

---

From: ianswett <[email-masked]>
Date: Thu, 16 Jul 2020 11:00:29 -0700

I'm not sure this is necessary, since we don't specify the variables as integers, but if we were going to make a change, I'd prefer the CWND increment in bytes rather than max_datagram_size, since it's a smoother 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/3917#issuecomment-659576223

---

From: Junho Choi <[email-masked]>
Date: Thu, 16 Jul 2020 15:22:14 -0700

> I'm not sure this is necessary, since we don't specify the variables as integers, but if we were going to make a change, I'd prefer the CWND increment in bytes rather than max_datagram_size, since…

If we increase CWND in bytes (let say 10 x 1/10 mss, rather than 1 full mss), is it resulting too many small sending packets?

-- 
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/3917#issuecomment-659706920

---

From: Jana Iyengar <[email-masked]>
Date: Thu, 16 Jul 2020 18:11:25 -0700

I agree with @ianswett : I don't think this is necessary... perhaps a comment might help clarify that implementers should be careful about integer division here?

FWIW, you'll also need to multiply bytes_acked in OnCongestionEvent() by 0.5, and reset it to 0 on a persistent congestion event.

-- 
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/3917#issuecomment-659768179

---

From: Kazu Yamamoto <[email-masked]>
Date: Thu, 16 Jul 2020 19:28:29 -0700

I would like to add comments in the following places:
- `/` in `OnPacketAcked`
- `congestion_window *= kLossReductionFactor` in `CongestionEvent`
- "`lost_packets` should be retransmitted" in `OnPacketsLost`
- "`max_ack_delay` is updated when TP exchanged, default to 25ms" in `Initialization`
- "A sender can send bytes of `congestion_window` - `bytes_in_flight`" in `Variables of interest`

They might be trivial for experts but it is difficult for beginners like me to understand the assumptions.

-- 
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/3917#issuecomment-659796276

---

From: Kazu Yamamoto <[email-masked]>
Date: Thu, 16 Jul 2020 19:30:07 -0700

@janaiyengar This is an off topic but there is no `OnCongestionEvent`. The current name is `CongestionEvent`. I would like to suggest to change it to `OnCongestionEvent` or `OnNewCongestionEvent`.

-- 
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/3917#issuecomment-659796927

---

From: ianswett <[email-masked]>
Date: Fri, 17 Jul 2020 08:18:18 -0700

@kazu-yamamoto renaming to OnCongestionEvent seems fine.  

If you want to make a PR, we can go through the comments you want to add, but "lost_packets should be retransmitted" is potentially misleading, since packets aren't retransmitted and their frames aren't always retransmitted.  Also, the max_ack_delay is 0, not 25ms, until the transport parameters are received.

-- 
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/3917#issuecomment-660164720

---

From: Junho Choi <[email-masked]>
Date: Fri, 17 Jul 2020 11:18:43 -0700

@junhochoi pushed 1 commit.

5b0463c4d92de05d33b2728c243e0877d0b980d5  Reduce bytes_acked on CongestionEvent() and persistent congestion event

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3917/files/e889b45dc3e9e05d724e711fcf788b8106e01d3f..5b0463c4d92de05d33b2728c243e0877d0b980d5

---

From: Junho Choi <[email-masked]>
Date: Fri, 17 Jul 2020 11:28:08 -0700

@janaiyengar commenting on integer division is also good I think, but this PR basically removes such concern by not using division any more.

I looked at other places using a division in the pseudo code but other than Pacing section it uses a constant, so improving here is not a bad thing I think.

Also I updated to reduce bytes_acked in CongestionEvent() and reset persistent congestion event.

-- 
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/3917#issuecomment-660271095

---

From: Kazu Yamamoto <[email-masked]>
Date: Fri, 17 Jul 2020 13:10:02 -0700

> If you want to make a PR, we can go through the comments you want to add, but "lost_packets should be retransmitted" is potentially misleading, since packets aren't retransmitted and their frames aren't always retransmitted.

Yes. What about this?  "The information in `lost_packets` should be retransmitted if necessary."

> Also, the max_ack_delay is 0, not 25ms, until the transport parameters are received.

I meant that it is initialized to 0 and updated when  the transport parameters are received. If the parameter is not specified, 25ms is set to this parameter.

-- 
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/3917#issuecomment-660315433

---

From: ianswett <[email-masked]>
Date: Fri, 17 Jul 2020 13:28:12 -0700

@kazu-yamamoto those changes sound good to me, but it'd be good to see them in context.  Want to send me a 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/3917#issuecomment-660322873

---

From: Kazu Yamamoto <[email-masked]>
Date: Fri, 17 Jul 2020 17:20:46 -0700

Yes. I will do it on next Monday.

-- 
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/3917#issuecomment-660390686

---

From: Jana Iyengar <[email-masked]>
Date: Fri, 17 Jul 2020 18:45:24 -0700

@janaiyengar commented on this pull request.

I don't think it's necessary, but I am also quite fine with this change. I believe a number of implementations do it like this anyway. A couple of comments.

> +: Store how many bytes acked so far. During congestion avoidance,
+  congestion window will be incremented by max_datagram_size
+  when this value is same or bigger than the congestion window.

```suggestion
: Records number of bytes acked and used during congestion
  avoidance, for incrementing the congestion window.
```

> @@ -1562,6 +1569,7 @@ window.
        congestion_window *= kLossReductionFactor
        congestion_window = max(congestion_window, kMinimumWindow)
        ssthresh = congestion_window
+       bytes_acked *= kLossReductionFactor

move this up to the line below where the congestion window is reduced

> +      if (bytes_acked >= congestion_window):
+        bytes_acked -= congestion_window
+        congestion_window += max_datagram_size

There's a minor behavior difference here from the old code, in that the cwnd here increases by at most 1. I think that's fine; I don't think it's possible to have an increase that is larger. The following code is the same as the old behavior, but I don't think it's necessary.

```suggestion
      while (bytes_acked >= congestion_window):
        bytes_acked -= congestion_window
        congestion_window += max_datagram_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/3917#pullrequestreview-451020019

---

From: Junho Choi <[email-masked]>
Date: Sat, 18 Jul 2020 23:58:03 -0700

79cde271710e2ee76f9cf88fa66b8b31b48a2ec8  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/3917/files/5b0463c4d92de05d33b2728c243e0877d0b980d5..79cde271710e2ee76f9cf88fa66b8b31b48a2ec8

---

From: Junho Choi <[email-masked]>
Date: Sun, 19 Jul 2020 00:00:34 -0700

c1c5675e55da1c8f55945aaaf0b605ea92e6538b  Byte counting at Congestion avoidance

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3917/files/79cde271710e2ee76f9cf88fa66b8b31b48a2ec8..c1c5675e55da1c8f55945aaaf0b605ea92e6538b

---

From: Junho Choi <[email-masked]>
Date: Sun, 19 Jul 2020 00:01:55 -0700

b307d85ba1400ace816706e3870705a7b086a8d7  Byte counting at Congestion avoidance

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

---

From: Junho Choi <[email-masked]>
Date: Sun, 19 Jul 2020 00:02:29 -0700

@junhochoi commented on this pull request.

LGTM. Updated.

-- 
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/3917#discussion_r456870374

---

From: Junho Choi <[email-masked]>
Date: Sun, 19 Jul 2020 00:02:41 -0700

Done. Thanks.

-- 
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/3917#discussion_r456870406

---

From: Junho Choi <[email-masked]>
Date: Sun, 19 Jul 2020 00:06:02 -0700

I don't think `while()` is necessary because `OnPacketAcked()` (recently changed) is called per acked packet, so `acked_packet.sent_bytes` is always smaller than `max_datagram_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/3917#discussion_r456870737

---

From: Kazu Yamamoto <[email-masked]>
Date: Mon, 20 Jul 2020 17:33:33 -0700

@kazu-yamamoto commented on this pull request.

I agree with @junhochoi.

-- 
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/3917#discussion_r457766847

---

From: Kazu Yamamoto <[email-masked]>
Date: Mon, 20 Jul 2020 17:34:32 -0700

I will implement this today for review. Please wait for a while.

-- 
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/3917#issuecomment-661491446

---

From: Jana Iyengar <[email-masked]>
Date: Mon, 20 Jul 2020 20:17:22 -0700

A minor editorial nit, but looks good.

> +: Records number of bytes acked and used during congestion
+  avoidance, for incrementing the congestion window.

```suggestion
: Records number of bytes acked, and used for incrementing
  the congestion window during congestion avoidance.
```

Agreed. I wanted to note the difference, but as I said, I don't think it's 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/3917#pullrequestreview-452097360

---

From: Junho Choi <[email-masked]>
Date: Tue, 21 Jul 2020 03:49:21 -0700

aa97d63f36f1642681ec46ab172413b32520c99a  Update bytes_acked definition

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

---

From: Jana Iyengar <[email-masked]>
Date: Tue, 21 Jul 2020 12:07:50 -0700

@janaiyengar approved this pull request.

Looks good to me -- thanks! I'll wait for @ianswett to take a look.

-- 
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/3917#pullrequestreview-452750425

---

From: Kazu Yamamoto <[email-masked]>
Date: Tue, 21 Jul 2020 19:04:22 -0700

@kazu-yamamoto approved this pull request.

I have implemented this and works well so far.

-- 
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/3917#pullrequestreview-452945058

---

From: Jana Iyengar <[email-masked]>
Date: Wed, 22 Jul 2020 18:40:24 -0700

FWIW, I should add that this is how quicly implements it. If I remember correctly, that it's also how Chrome implements 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/3917#issuecomment-662779945

---

From: ianswett <[email-masked]>
Date: Mon, 27 Jul 2020 05:53:13 -0700

@ianswett commented on this pull request.

One editorial suggestion and a question, but LG otherwise.

> @@ -1483,6 +1483,10 @@ ssthresh:
   the mode is slow start and the window grows by the number of bytes
   acknowledged.
 
+bytes_acked:
+: Records number of bytes acked, and used for incrementing

```suggestion
:  Number of bytes acknowledged which have not yet been used to increment
```

> @@ -1560,6 +1566,7 @@ window.
      if (!InCongestionRecovery(sent_time)):
        congestion_recovery_start_time = now()
        congestion_window *= kLossReductionFactor
+       bytes_acked *= kLossReductionFactor

I thought this would be set to 0 here.  Why reduce 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/3917#pullrequestreview-455747977

---

From: Jana Iyengar <[email-masked]>
Date: Mon, 27 Jul 2020 06:07:34 -0700

The current cwnd is (cwnd + bytes_acked/cwnd). Multiplying that by kLossReductionFactor means you multiply bytes_acked by kLossReductionFactor as well.

-- 
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/3917#discussion_r460875753

---

From: Junho Choi <[email-masked]>
Date: Mon, 27 Jul 2020 11:11:15 -0700

Just thinking again... this is when a recovery episode starts and we enter congestion avoidance by ssthresh=cwnd. And bytes_acked is only used in congestion avoidance and doing nothing during slow start.

I think we simply clear bytes_acked to 0 as @ianswett said because it's a new start of congestion avoidance, or no need to touch bytes_acked because even if there is a previous leftover bytes_acked, it will not impact much.

-- 
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/3917#discussion_r461076097

---

From: Junho Choi <[email-masked]>
Date: Mon, 27 Jul 2020 11:12:49 -0700

c0d087f10a62fb4113e87c60cb7cb4f12d15b672  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/3917/files/aa97d63f36f1642681ec46ab172413b32520c99a..c0d087f10a62fb4113e87c60cb7cb4f12d15b672

---

From: Junho Choi <[email-masked]>
Date: Mon, 27 Jul 2020 11:13:45 -0700

1a09b63a9d511aa16615d6f50de75655fa081749  nit: remove whitespace

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3917/files/c0d087f10a62fb4113e87c60cb7cb4f12d15b672..1a09b63a9d511aa16615d6f50de75655fa081749

---

From: ianswett <[email-masked]>
Date: Mon, 27 Jul 2020 11:30:03 -0700

This is a slightly larger change than I first realized because it moves CWND increments from bytes to packets.  I'm trying to decide how much that matters in practice.

Thanks for the explanation.  I think I'm fine either way, but questions like this are one of the reason I liked the old approach of assuming it's the implementation's responsibility to deal with rounding/etc.

Reading this again, I realize there's a somewhat substantive difference in behavior between the previous pseudocode and this PR, because the previous pseudocode would increment CWND in bytes, not only max_datagram_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/3917#pullrequestreview-456031588

---

From: Jana Iyengar <[email-masked]>
Date: Mon, 27 Jul 2020 16:31:43 -0700

This isn't a big difference, so I'm fine with any resolution here. @ianswett : I don't understand what you mean by the substantive difference. The previous code incremented by (bytes_acked/cwnd) fraction of max_datagram_size on each ack, this one accumulates that fraction until it is 1 and then adds max_datagram_size. It is functionally the same.

-- 
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/3917#discussion_r461228576

---

From: ianswett <[email-masked]>
Date: Mon, 27 Jul 2020 17:40:42 -0700

I agree that this is a small difference either way.

-- 
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/3917#discussion_r461250063

---

From: ianswett <[email-masked]>
Date: Mon, 27 Jul 2020 17:51:41 -0700

A suggestion in order to maintain byte quantization instead of packet quantization, which minimizes changes from the existing draft.

> +      bytes_acked += acked_packet.sent_bytes

This part contains the larger change.  Previously, congestion_window was adjusted in 1 byte increments.  Now it's adjusted in max_datagram_size increments.  In practice, it likely doesn't matter that much, but I am concerned about perpetuating packet-size quantization in congestion control, since my impression is that it's an artifact of the Linux kernel, not an intentional design choice.

I believe BSD-based OS's use bytes instead of packets, and bytes always seemed more correct to me.

However, this isn't that difficult to fix.  How about:
  cwnd_in_packets = congestion_window / max_datagram_size
  congestion_window += bytes_acked / cwnd_in_packets
  bytes_acked = bytes_acked % cwnd_in_packets

-- 
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/3917#pullrequestreview-456229765

---

From: Jana Iyengar <[email-masked]>
Date: Mon, 27 Jul 2020 18:54:59 -0700

I don't see how this is packet-based computation. Yes, the cwnd is incremented only when it can be incremented in max_datagram_size chunks, but at any given time, you know exactly what your fractional component is: (bytes_acked/congestion_window). At any given point in time, your total send allowance, in bytes, is (congestion_window + bytes_acked * max_datagram_size / congestion_window).

This fractional difference only matters if you are sending packets smaller than max_datagram_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/3917#discussion_r461270578

---

From: Junho Choi <[email-masked]>
Date: Mon, 27 Jul 2020 21:23:43 -0700

FYI, FreeBSD uses byte counting by default and use the same mechanism of this PR: https://github.com/freebsd/freebsd/blob/0b7e58eb670d67cdc0268be5d588839038aa5abf/sys/netinet/tcp_input.c#L355

And I think @ianswett 's code has a problem

> bytes_acked = bytes_acked % cwnd_in_packets

this will make bytes_acked too small (only between 0 - cwnd_in_packets - 1 range)

I'm happy with the current logic.

-- 
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/3917#discussion_r461309103

---

From: Martin Thomson <[email-masked]>
Date: Mon, 27 Jul 2020 22:58:47 -0700

@martinthomson commented on this pull request.

> This fractional difference only matters if you are sending packets smaller than max_datagram_size.

This isn't quite right.  Based on this, you need to receive a complete window before you increase the window size.  Previously, you could maybe increment by smaller amounts sooner.  The difference is between a continuous increase (or at least an increase with every acknowledgments) and a stepped increase.  Let's say max_datagram_size is 1000 and your window is currently 10,000, you will have to receive acknowledgements for 10 packets before you can go to 11,000.  If you increase progressively, then you might go to 10,100 then 10,200, and so forth sooner.

If the assertion is that you can't use that small extra amount, consider that the window could be 9,999 to begin with.  That is entirely likely as you rarely settle on a strict multiple of packet size.  The first arriving acknowledgment would allow another full-sized packet to be sent much sooner than otherwise.

So this is a functional change, even if it you might easily decide that it is acceptable.

-- 
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/3917#discussion_r461336603

---

From: Martin Thomson <[email-masked]>
Date: Mon, 27 Jul 2020 23:08:00 -0700

Oh, I should probably concede that more increases to the congestion window will result in the final congestion window being smaller, so you are trading a larger final increase (this PR) with earlier availability of bytes (status quo).

Say you get 10 acks for 1000 bytes, with a starting cwnd of 10,000 and max_datagram_sizeo of 1000.

With this code, you end up with no change until the last ack, where you go to 11,000.

With the previous code, you would go to: 10100, 10199, 10297, 10394, 10490, 10585, 10680, 10773, 10866, 10958.  (That's using double-precision logic, you lose two bytes if you round.)

That's less in the end, but the value of that to you depends on whether you can use any of those intermediate values.

-- 
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/3917#discussion_r461339606

---

From: Martin Thomson <[email-masked]>
Date: Mon, 27 Jul 2020 23:20:02 -0700

If you want to realize increments immediately, but not lose any credit, then I think that the algorithm is:

```
credit = saved_credit + (bytes_acked * max_datagram_size)
congestion_window += credit / congestion_window
saved_credit = credit % congestion_window
```

This uses integer math to produce a value that matches the double-precision logic from my previous comment.

-- 
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/3917#discussion_r461343837

---

From: ianswett <[email-masked]>
Date: Tue, 28 Jul 2020 05:51:58 -0700

Yesterday was a long day, so I suspect my pseudocode has an error, but as @mt points out this does change how the pseudocode works.

-- 
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/3917#discussion_r461554326

---

From: Junho Choi <[email-masked]>
Date: Tue, 28 Jul 2020 15:06:34 -0700

In the scenario @martinthomson mentioned, I think the problem with small increase is that it can result in a small sized packets.

e.g. bytes_in_flight = 10000, ssthresh = cwnd = 10000, mss = 1000
  -> received ack of 1000 bytes
  -> byts_in_flight = 9000, cwnd = 10100

Now sender can send 1100 bytes, can result in sending 2 packets, with 1000 bytes and 100 bytes each when there is a enough size of data is ready. The pseudocode in this PR doesn't have such 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/3917#discussion_r461916399

---

From: Junho Choi <[email-masked]>
Date: Tue, 28 Jul 2020 15:34:26 -0700

Briefly tested using quiche (https://github.com/cloudflare/quiche/pull/610) -- when downloading 10Mbytes file using 10Mbit/40ms link, total # of sender packets can be reduced by 14% (mostly small packets than MSS) with slightly better performance.

-- 
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/3917#discussion_r461931350

---

From: ianswett <[email-masked]>
Date: Thu, 30 Jul 2020 12:12:09 -0700

The small fragmented packet issue is an interesting one, is there an existing RFC which discusses this we could cite with advice on how to avoid overly small packets?

-- 
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/3917#discussion_r463213188

---

From: Kazu Yamamoto <[email-masked]>
Date: Thu, 30 Jul 2020 17:09:10 -0700

What about RFC 3465?

-- 
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/3917#discussion_r463337515

---

From: Junho Choi <[email-masked]>
Date: Thu, 30 Jul 2020 17:37:20 -0700

This mechanism described in RFC3465 as in the original PR description but the RFC itself has no pseudocode. I don't know other references discussing about those small fragments due to byte counting in congestion avoidance -- probably it's because most of TCP RFC use window-based cwnd, not byte based?

-- 
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/3917#discussion_r463344939

---

From: Junho Choi <[email-masked]>
Date: Fri, 31 Jul 2020 12:19:15 -0700

neqo is implmenting this logic too https://github.com/mozilla/neqo/pull/875

-- 
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/3917#issuecomment-667309221

---

From: Jana Iyengar <[email-masked]>
Date: Fri, 31 Jul 2020 12:33:26 -0700

Yes, this is a change in how the sender works, but it's not a significant difference. And it is in the pseudo code in the appendix, not in the text. 

Let's be clear that we are defining something that we are calling Reno for QUIC, knowing fully well that it's just one of many controllers that people will attempt. The current pseudo code is not a reference, it is one way of doing Reno. What this PR proposes is what a number of implementations already do.

I don't think there's much more to see here. I'm fine with not including the PR, or with including the PR. Most implementations are likely to implement it the way the PR does, but the current pseudo code is also good in that it is algorithmically clear.

-- 
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/3917#discussion_r463794335

---

From: ianswett <[email-masked]>
Date: Fri, 31 Jul 2020 13:58:19 -0700

I think this PR is fine, but I'm also not seeing a big reason for changing the current pseudocode.

It turns out Chrome doesn't do it this way anymore, but we used to.

-- 
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/3917#discussion_r463835696