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

Retransmitting an ACK frame warning #3049

Merged
merged 5 commits into from
Oct 24, 2019
Merged

Conversation

ianswett
Copy link
Contributor

It can result in an inflated RTT, as well as being a waste of bytes.

Fixes #2748

It can result in an inflated RTT, as well as being a waste of bytes.

Fixes #2748
@ianswett ianswett added editorial An issue that does not affect the design of the protocol; does not require consensus. -transport labels Sep 18, 2019
@@ -3183,7 +3183,8 @@ containing that information is acknowledged.

* The most recent set of acknowledgments are sent in ACK frames. An ACK frame
SHOULD contain all unacknowledged acknowledgments, as described in
{{sending-ack-frames}}.
{{sending-ack-frames}}. Retransmitting an ACK frame can result in an
Copy link
Contributor

Choose a reason for hiding this comment

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

This para is not great. That second sentence totally doesn't belong here. Suggested rewrite:

"ACK frames carry the most recent set of acknowledgements and the current Ack Delay estimate. Retransmitting an old ACK frame with stale Ack Delay can cause the peer to generate an inflated RTT sample."

Then in 13.2.1 (sending-ack-frames), add this as the first paragraph:
"ACK frames SHOULD acknowledge both ack-eliciting and non-ack-eliciting packets that have been received but not yet acknowledged."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is already text that says "When only non-ACK-eliciting packets need to be acknowledged, an endpoint MAY wait until an ACK-eliciting packet has been received to bundle an ACK frame with outgoing frames." I'm not sure that's sufficient, but your suggestion adds a SHOULD, which makes this change no longer editorial. The ACK generation text needs a full rewrite, and I'd rather tackle that suggestion as part of that PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I worked part of your first suggestion in.

The latter suggestion is now present(from a different PR).

Copy link
Member

@martinthomson martinthomson left a comment

Choose a reason for hiding this comment

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

This is my preferred option, but I might reword it.

* ACK frames are sent containing the most recent set of acknowledgements and
Ack Delay, as described in {{sending-ack-frames}}. Retransmitting an old ACK
frame with stale Ack Delay can cause the peer to generate an inflated RTT
sample.
Copy link
Member

Choose a reason for hiding this comment

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

Here's my alternative:

ACK frames always contain an acknowledgement for the highest packet number that has been processed and an Ack Delay that is updated to the time that the packet is sent. Note that delaying the transmission of packets containing ACK frames or sending copies of old ACK frames will cause problems for a peer, including inflated RTT estimates or unnecessary disabling of ECN.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I took part of this, but the first statement is incorrect: ACK frames do not have to contain the highest packet number that has been processed(there was an issue about this).

@janaiyengar janaiyengar merged commit 4f77dbe into master Oct 24, 2019
@janaiyengar janaiyengar deleted the ianswett-retransmitting-acks branch October 24, 2019 03:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-transport editorial An issue that does not affect the design of the protocol; does not require consensus.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Retransmitting ACKs may cause misleading RTT samples
3 participants