Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Recovery: Initialization of min_rtt when used with Cubic #4833

Closed
WesleyRosenblum opened this issue Mar 10, 2021 · 7 comments · Fixed by #4859
Closed

Recovery: Initialization of min_rtt when used with Cubic #4833

WesleyRosenblum opened this issue Mar 10, 2021 · 7 comments · Fixed by #4859

Comments

@WesleyRosenblum
Copy link

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) that incorporates min_rtt (as the Linux TCP Cubic implementation does). Cubic specifies the formula for W_est, 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:

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.

@LPardue
Copy link
Member

LPardue commented Mar 10, 2021

FWIW this was changed from infinite back to 0 in #2615

@ianswett
Copy link
Contributor

ianswett commented Mar 10, 2021

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.

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.

@WesleyRosenblum
Copy link
Author

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.

@ianswett
Copy link
Contributor

I'm still a bit unclear. Under what circumstances would a compliant peer send an ACK that only acknowledges non ack-eliciting packets?

@WesleyRosenblum
Copy link
Author

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

@larseggert larseggert added this to Triage in Late Stage Processing via automation Mar 11, 2021
@ianswett
Copy link
Contributor

ianswett commented Mar 14, 2021

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 every ACK+PADDING packet, 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?

@WesleyRosenblum
Copy link
Author

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 otherwise be non-ack-eliciting, to avoid an infinite feedback loop of acknowledgments."

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 #3853 (comment), I'm not sure moving it up would help without some additional rewording.

ianswett added a commit that referenced this issue Mar 28, 2021
martinthomson added a commit that referenced this issue Mar 30, 2021
There is a little connective tissue added here to connect to the ACK
tracking thing.

Closes #4833.
Closes #4855.
Late Stage Processing automation moved this from Triage to Issue Handled Apr 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Late Stage Processing
  
Issue Handled
4 participants