Subject: [quicwg/base-drafts] congestion window increase on every ACKed packet could result in bursty sends (#3094)

From: Vidhi Goel <[email-masked]>
Date: Tue, 15 Oct 2019 11:46:08 -0700

When an ACK arrives that acknowledges N packets (where N could be very large), QUIC will increase the congestion window by more than 1MSS per congestion window during Congestion Avoidance causing bursty send. This could also affect the slow start.

if (congestion_window < ssthresh):
       // Slow start.
       congestion_window += acked_packet.size
     else:
       // Congestion avoidance.
       congestion_window += kMaxDatagramSize * acked_packet.size / congestion_window

One could argue that QUIC ACKs every other packet, but there ACK packet could get lost or  ACK for ACK packet could get lost.

The draft is probably assuming packet pacing but not all implementations support that.

I discussed this with Jana and he thinks that we should add something in pseudo code or some specific text in the draft.

-- 
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/issues/3094

---

From: ianswett <[email-masked]>
Date: Tue, 15 Oct 2019 22:28:23 -0700

PR #2675 added more specific text on avoiding bursts into the network, FYI.

-- 
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/issues/3094#issuecomment-542522878

---

From: Vidhi Goel <[email-masked]>
Date: Wed, 16 Oct 2019 11:53:19 -0700

Please note that PR #2675 addresses the issue of app-limited or pacing-limited cases to not increase cwnd when it is under utilized.

My concern is cwnd being inflated due to an ACK frame containing ACKs for large number of 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/issues/3094#issuecomment-542842037

---

From: Jana Iyengar <[email-masked]>
Date: Wed, 16 Oct 2019 16:05:28 -0700

@ianswett : The problem here is that we don't talk about burstiness that can occur when pacing is not in place. PRR handles this, but we don't do PRR in the draft.

-- 
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/issues/3094#issuecomment-542926805

---

From: Christian Huitema <[email-masked]>
Date: Wed, 16 Oct 2019 16:10:12 -0700

No we need to spend lots of time on Reno as everybody appears to be doing Cubic or BBR?

-- 
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/issues/3094#issuecomment-542927908

---

From: Jana Iyengar <[email-masked]>
Date: Wed, 16 Oct 2019 16:11:56 -0700

Cubic has this problem. BBR cannot be assumed.

-- 
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/issues/3094#issuecomment-542928301

---

From: Christian Huitema <[email-masked]>
Date: Wed, 16 Oct 2019 16:12:26 -0700

Except Cubic opens the window based on time spent, not just ack 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/issues/3094#issuecomment-542928406

---

From: Christian Huitema <[email-masked]>
Date: Wed, 16 Oct 2019 16:22:10 -0700

I don't get the scenario. It seems that @goelvidhi is worried about receiving an ACK for a large number of packets. This is not a congestion window problem, because it happens even if the congestion windows size remains constant. The root cause is that the ACK of N packets now reduces the number of packet in flow by N. So this is strictly about pacing, not about congestion control.

-- 
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/issues/3094#issuecomment-542930806

---

From: Jana Iyengar <[email-masked]>
Date: Wed, 16 Oct 2019 16:22:18 -0700

@huitema: yes, and it can and still does burst because of stretch acks

-- 
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/issues/3094#issuecomment-542930830

---

From: Christian Huitema <[email-masked]>
Date: Wed, 16 Oct 2019 16:23:15 -0700

Not just stretch Acks -- deliberate Ack compression by the network happens too. Very few alternative to pacing.

-- 
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/issues/3094#issuecomment-542931093

---

From: Junho Choi <[email-masked]>
Date: Wed, 16 Oct 2019 16:31:07 -0700

https://tools.ietf.org/html/rfc3465 describes 
```This document specifies that TCP implementations MAY use L=2*SMSS
   bytes and MUST NOT use L > 2*SMSS bytes.  This choice balances
   between being conservative (L=1*SMSS bytes) and being potentially
   very aggressive.  In addition, L=2*SMSS bytes exactly balances the
   negative impact of the delayed ACK algorithm (as discussed in more
   detail in section 3.2).  Note that when L=2*SMSS bytes cwnd growth is
   roughly the same as the case when the standard algorithms are used in
   conjunction with a receiver that transmits an ACK for each incoming
   segment [All98] (assuming no or small amounts of ACK loss in both
   cases).```

-- 
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/issues/3094#issuecomment-542932928

---

From: Lars Eggert <[email-masked]>
Date: Wed, 16 Oct 2019 16:35:28 -0700

Discussed in Cupertino. @goelvidhi to prepare a PR that hopefully can incorporate some ideas from RFC3465 to add safety features for non-pacing stacks.

-- 
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/issues/3094#issuecomment-542933830

---

From: Christian Huitema <[email-masked]>
Date: Wed, 16 Oct 2019 16:37:15 -0700

But 3565 does not solve the burst problem. It limits the growth of the window, but does not do pacing of any kind.

-- 
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/issues/3094#issuecomment-542934208

---

From: Junho Choi <[email-masked]>
Date: Wed, 16 Oct 2019 16:40:16 -0700

I think we can change to something like this
> // Slow start.
>  congestion_window += min ( 2 * kMaxDatagramSize, 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/issues/3094#issuecomment-542934819

---

From: Vidhi Goel <[email-masked]>
Date: Wed, 16 Oct 2019 19:09:59 -0700

@junhochoi, I think we need to do 2MSS clamping per ACK received and not per packet acked.

I am thinking that we can remove OnPacketAckedCC and instead do OnAckReceivedCC. Pseudocode change something like this:
```
    OnAckReceived(ack, pn_space):

    Set total_bytes_acked to 0
    for acked_packet in newly_acked_packets:
        total_bytes_acked += acked_packet.size
        OnPacketAcked(acked_packet.packet_number, pn_space)

    OnAckReceivedCC(total_bytes_acked)

OnAckReceivedCC(total_bytes_acked):
    bytes_in_flight -= total_bytes_acked
    if (congestion_window < ssthresh):
         // Slow start.
        congestion_window += min(total_bytes_acked, 2* kMaxDatagramSize)
   else
       congestion_window += kMaxDatagramSize * total_bytes_acked
           / 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/issues/3094#issuecomment-542965003

---

From: Junho Choi <[email-masked]>
Date: Thu, 17 Oct 2019 09:47:52 -0700

I think we can keep `OnPacketAckedCC()` but just move cwnd update to `OnAckReceivedCC()` as you propose.

-- 
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/issues/3094#issuecomment-543263625

---

From: Jana Iyengar <[email-masked]>
Date: Thu, 17 Oct 2019 11:34:28 -0700

Yup, that's an important missing fix, though it needs to be applied only when not pacing. Additionally, #3106 helps with burstiness in general. @goelvidhi : I would make a `pacing` variable, and apply the min here when not pacing.

-- 
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/issues/3094#issuecomment-543305742

---

From: Christian Huitema <[email-masked]>
Date: Thu, 17 Oct 2019 14:01:11 -0700

The proposed change has the effect of reducing the effective Window if ACKs are compressed. Say you have a window of 12 packets, acks are delayed, and a single ACK acknowledges 12 packets. Now you have a window of 12 packets, your transmission rate is 6 times slower than before, and it will take you 10 RTT to recover. Is that really what you want?

-- 
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/issues/3094#issuecomment-543360065

---

From: Jana Iyengar <[email-masked]>
Date: Thu, 17 Oct 2019 14:33:03 -0700

@huitema: This is what ABC (RFC 3465) does for TCP. ACK compression has side-effects, and the way to deal with it is, as you say, to pace packets out. This reduction in performance is only when the endpoint does not pace, and it is the only way to limit bursts in that case.

-- 
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/issues/3094#issuecomment-543370889

---

From: Christian Huitema <[email-masked]>
Date: Thu, 17 Oct 2019 15:52:59 -0700

Also, ACK compression does not necessarily imply ACK pruning. I have traces in which 1 RTT worth of ACKs is compressed and delivered back to back. In that case, the proposed algorithm will do nothing: you will get 2 MSS added for each ACK, back to back within less than 1 ms.

-- 
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/issues/3094#issuecomment-543395415

---

From: Junho Choi <[email-masked]>
Date: Thu, 17 Oct 2019 16:21:02 -0700

FYI. FreeBSD implements rfc3465 (and enabled by default) https://github.com/freebsd/freebsd/blob/master/sys/netinet/cc/cc_newreno.c#L174
Linux removed tcp_abc implementation but saying it does in a different way: https://github.com/torvalds/linux/blob/master/net/ipv4/tcp_cong.c#L385
Considring FreeBSD doesn't have pacing by default, I think current proposed algorithm is not worse (or aggressive) than what tcp implementation does.

-- 
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/issues/3094#issuecomment-543404524