Subject: [quicwg/base-drafts] Recovery: Initialization of min_rtt when used with Cubic (#4833)

From: Wesley Rosenblum <[email-masked]>
Date: Wed, 10 Mar 2021 11:05:30 -0800

In the Loss Recovery Pseudocode, `min_rtt` is initialized to 0: https://tools.ietf.org/html/draft-ietf-quic-recovery-34#appendix-A.4

This can result in dividing by zero if one implements a Cubic congestion controller ([RFC8312](https://tools.ietf.org/html/rfc8312)) that incorporates `min_rtt` (as the [Linux TCP Cubic implementation does](https://github.com/torvalds/linux/blob/05a59d79793d482f628a31753c671f2e92178a21/net/ipv4/tcp_cubic.c#L266)). Cubic specifies the [formula for `W_est`](https://tools.ietf.org/html/rfc8312#section-4.2), used in congestion avoidance, as:
```
 W_est(t) = W_max*beta_cubic + [3*(1-beta_cubic)/(1+beta_cubic)] * (t/RTT)
```

An endpoint can enter congestion avoidance without receiving any acks for any ack-eliciting packets (only non-ack eliciting packets). In such a scenario, `min_rtt` would still be 0, due to the requirement from [§5.1](https://tools.ietf.org/html/draft-ietf-quic-recovery-34#section-5.1):

```
An RTT sample MUST NOT be generated on receiving an ACK frame that does not newly acknowledge at least one ack-eliciting packet.
```
 Since `min_rtt` is 0 when `w_est` is calculated, the divide by 0 occurs. This could be addressed by initializing `min_rtt` to `kInitialRtt` instead of 0.

-- 
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/4833

---

From: Lucas Pardue <[email-masked]>
Date: Wed, 10 Mar 2021 11:18:21 -0800

FWIW this was changed from `infinite` back to 0 in https://github.com/quicwg/base-drafts/pull/2615

-- 
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/4833#issuecomment-795941691

---

From: ianswett <[email-masked]>
Date: Wed, 10 Mar 2021 12:33:59 -0800

I feel the existing pseudocode is ok, because min_rtt in the text and pseudocode is used for the purpose of rejecting potentially bad RTT samples, and is not a general purpose min_rtt.
https://tools.ietf.org/html/draft-ietf-quic-recovery-34#section-5.2

For example, BBR uses a windowed min_rtt that's fairly different.

-- 
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/4833#issuecomment-796056156

---

From: Wesley Rosenblum <[email-masked]>
Date: Wed, 10 Mar 2021 16:15:32 -0800

>This does bring up an edge case I hadn't thought much about, where no ack-eliciting packets are acknowledged, but a packet is declared lost. A compliant transport implementation shouldn't ACK only ACK-only packets, but a non-compliant one could.

The situation we saw was under severe packet loss, the peer sending probe packets containing a PING and ACK for non ack eliciting packets (ACKs and PADDING). The ACK-eliciting packets we send are getting dropped, so eventually we declare an ACK-eliciting packet lost and enter Recovery. When the next ACK comes in for a packet sent after we entered Recovery, we enter Congestion Avoidance and experience the issue with `min_rtt`. 

-- 
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/4833#issuecomment-796312101

---

From: ianswett <[email-masked]>
Date: Wed, 10 Mar 2021 16:44:34 -0800

I'm still a bit unclear.  Under what circumstances would a compliant peer send an ACK that only acknowledges non ack-eliciting 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/4833#issuecomment-796329395

---

From: Wesley Rosenblum <[email-masked]>
Date: Wed, 10 Mar 2021 16:52:20 -0800

After a probe timeout. Would that be non-compliant?

-- 
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/4833#issuecomment-796333802

---

From: ianswett <[email-masked]>
Date: Sun, 14 Mar 2021 10:01:43 -0700

In regards to the issue originally raised about min_rtt, I still feel the min_rtt is not general purpose, so the originally stated issue is not a problem in the context of QUIC recovery.  Other discussion follows.

First, you'd have to complete the handshake without an RTT sample, which seems unlikely, but possible, so let's assume that.

A PTO would send an ack-eliciting packet, and when the ACK for that packet came back, you'd have an RTT sample.  However, if the peer was sending ACK+PING for every other ACK+PADDING packet, then you're correct that the incoming ACK bundled with the PING would not create an RTT measurement, but would allow any previously sent ACK-eliciting packets to be declared as lost if the packet threshold was reached.

In regards to "peer sending probe packets containing a PING and ACK for non ack eliciting packets (ACKs and PADDING)", if you're saying that the peer is sending those in response to the ACK+PADDING packets, that sounds non-compliant to me.

In regards to whether it's ok to send ACK+PING in response to ACK+PADDING, I re-read "Sending ACK Frames"(https://tools.ietf.org/html/draft-ietf-quic-transport-34#section-13.2.1) and it says: "An endpoint MUST NOT send a non-ack-eliciting packet in response to a non-ack-eliciting packet, even if there are packet gaps that precede the received packet."  Are you sending PADDING+PING due to that prohibition?  I'm a bit concerned that could cause an infinite or near-infinite loop under the wrong circumstance, which was exactly the issue the paragraph highlights.

The last paragraph does specifically preclude sending ack-eliciting packets in response to every non-ack-eliciting packet: "A receiver MUST NOT send an ack-eliciting frame in all packets that would otherwise be non-ack-eliciting, to avoid an infinite feedback loop of acknowledgments."

It may make sense to move that final paragraph up, as it's a bit disconnected at the moment?

-- 
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/4833#issuecomment-798941529

---

From: Wesley Rosenblum <[email-masked]>
Date: Mon, 15 Mar 2021 11:49:30 -0700

> In regards to the issue originally raised about min_rtt, I still feel the min_rtt is not general purpose, so the originally stated issue is not a problem in the context of QUIC recovery. 

Agree that its not a problem within the context of QUIC recovery, I just wanted to call it out as an issue I experienced. If there wasn't much reason to initialize to 0 perhaps initializing to a value that is actually possible would make sense (and would prevent issues like the one I saw), but its not a big deal.

> "An endpoint MUST NOT send a non-ack-eliciting packet in response to a non-ack-eliciting packet, even if there are packet gaps that precede the received packet." Are you sending PADDING+PING due to that prohibition? 

Just to clarify, only the peer is sending PINGS in the scenario we saw (actually PADDING+PING+ACK)

> The last paragraph does specifically preclude sending ack-eliciting packets in response to every non-ack-eliciting packet: "A receiver MUST NOT send an ack-eliciting frame in all packets that would…

The peer is sending ack-eliciting packets (PADDING+PING+ACK) due to PTO timeouts, its not a case of upgrading non-ack-eliciting packets to ack-eliciting by throwing in a PING every time, more that the ACKs are getting included when the probes get sent. 

> It may make sense to move that final paragraph up, as it's a bit disconnected at the moment?

I did find this paragraph confusing until I read https://github.com/quicwg/base-drafts/issues/3853#issuecomment-656310128, I'm not sure moving it up would help without some additional rewording.

-- 
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/4833#issuecomment-799666431