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

MAY send 1 packet entering recovery #3443

Merged
merged 5 commits into from
Mar 5, 2020
Merged

Conversation

ianswett
Copy link
Contributor

@ianswett ianswett commented Feb 9, 2020

Similar to TCP as described in Section 5 of RFC 6675

Fixes #3335

Similar to TCP as described in Section 5 of RFC 6675

Fixes #3335
@ianswett ianswett added design An issue that affects the design of the protocol; resolution requires consensus. -recovery labels Feb 9, 2020
@martinduke
Copy link
Contributor

Should we restrict this to retransmittable frames that were lost in the packet? If it was a small packet, can I pack to the MTU? What about pure ACKs that I've declared lost?

@ianswett
Copy link
Contributor Author

Those are great questions. There are two goals in my mind. The first is to ensure loss recovery is not unnecessarily slower than TCP. The second is to ensure this behavior isn't significantly more aggressive than TCP. But within those bounds, I'm inclined to not be too restrictive, if that makes sense? Suggestions welcome if you think they'd be an improvement.

@martinduke
Copy link
Contributor

Well the pedantic answer is that TCP would only detect a loss if there was something in the bytestream. the QUIC equivalents would then be any packet that contained STREAM, CRYPTO, MAX_STREAM_DATA, etc, plus any packet that flipped the key update bit. But not ACK, MAX_DATA, and other things that are in the TCP header. That is obviously a dumb way to approach things.

I don't actually have a strong opinion on this one way of the other, but if I read the draft correctly, the loss of an ACK-only packet will affect cwnd in QUIC but not TCP (granted, a cwnd-constrained endpoint sending pure ACKs is a corner case). So I guess I'd err on the side of permitting the packet in response to loss of an ACK, as we're already quite a bit less aggressive than TCP in that case.

Anyway, do we want any normative language against, say, using this permission to send high-priority new data instead of the lost data?

@ianswett
Copy link
Contributor Author

Adding language against sending new data or data besides data that's been lost couples the "can I send" with "what should I send" questions more than I would like unless it's absolutely critical.

FYI, to be declared lost, it must be in-flight: https://github.com/quicwg/base-drafts/blob/master/draft-ietf-quic-recovery.md#acknowledgement-based-detection-ack-loss-detection
"A packet is declared lost if it meets all the following conditions:

  • The packet is unacknowledged, in-flight, and was sent prior to an acknowledged packet."

Copy link
Contributor

@janaiyengar janaiyengar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a nit, but LGTM.

draft-ietf-quic-recovery.md Outdated Show resolved Hide resolved
@martinduke
Copy link
Contributor

Ah, I think we're fine then.

@ianswett ianswett merged commit 8dfa613 into master Mar 5, 2020
@ianswett ianswett deleted the ianswett-send-one-packet branch March 5, 2020 00:44
@@ -706,6 +706,13 @@ The recovery period limits congestion window reduction to once per round trip.
During recovery, the congestion window remains unchanged irrespective of new
losses or increases in the ECN-CE counter.

When entering recovery, a single packet MAY be sent even if bytes in flight
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it is possible to enter recovery before amplification mitigation is no longer needed, sending a packet upon entering recovery might contradict amp. rules.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, but the amplification factor is a MUST and this is a MAY, so I think the MUST wins here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure. It depends on what you read first. It's like that NATO test with two pages of questions and instructions, the first stating to read everything first, the last stating the all other questions need not be answered. Why gives those entries priority over other entries?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-recovery design An issue that affects the design of the protocol; resolution requires consensus.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Immediate retransmission upon loss detection
4 participants