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

Don't retransmit lost packets that are acked later #3957

Merged
merged 8 commits into from Sep 1, 2020
Merged

Conversation

janaiyengar
Copy link
Contributor

Closes #3956.

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 useful information and in the right place. Thanks.

I have a small suggestion, but I'm OK with the original phrasing.

Comment on lines 3807 to 3813
When a sender declares a packet as lost and either retransmits the contained
frames or marks them for retransmission, the sender SHOULD avoid subsequent
retransmission of this information if it later receives an acknowledgement for
that packet. Doing so requires senders to retain information about packets after
they are declared lost. A sender can discard this information after a period of
time elapses, such as three times the PTO (Section 6.2 of {{QUIC-RECOVERY}}), or
on other events, such as reaching a memory limit.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
When a sender declares a packet as lost and either retransmits the contained
frames or marks them for retransmission, the sender SHOULD avoid subsequent
retransmission of this information if it later receives an acknowledgement for
that packet. Doing so requires senders to retain information about packets after
they are declared lost. A sender can discard this information after a period of
time elapses, such as three times the PTO (Section 6.2 of {{QUIC-RECOVERY}}), or
on other events, such as reaching a memory limit.
Acknowledgments can be received after a sender declares a packet as lost. A
sender SHOULD avoid sending information again after receiving a late
acknowledgment. Doing so requires senders to retain information about packets
for some time after they are declared lost. A sender can discard this
information after a period of time elapses, such as three times the PTO (Section
6.2 of {{QUIC-RECOVERY}}), or on other events, such as reaching a memory limit.

Copy link
Member

@kazuho kazuho Jul 24, 2020

Choose a reason for hiding this comment

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

I think I like @martinthomson's idea of splitting the sentence into two. The only thing I might prefer is changing the last part of the second sentence "after receiving a late acknowledgement" to something like "after receiving such an acknowledgement" to make it clearer, but anyways it's good.

Copy link
Contributor

Choose a reason for hiding this comment

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

How about "..after receiving an acknowledgement for a previously lost packet." I think late acknowledgement is potentially confusing.

Copy link
Contributor

Choose a reason for hiding this comment

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

And how about starting with "A packet can be acknowledged after being declared lost." instead of "Acknowledgements can be received after a sender declares a packet as lost."

Copy link
Contributor

@ianswett ianswett left a comment

Choose a reason for hiding this comment

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

Thanks for adding this, some more comments.

retransmission of this information if it later receives an acknowledgement for
that packet. Doing so requires senders to retain information about packets after
they are declared lost. A sender can discard this information after a period of
time elapses, such as three times the PTO (Section 6.2 of {{QUIC-RECOVERY}}), or
Copy link
Contributor

Choose a reason for hiding this comment

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

3*PTO is really long in this case, and it can consume a fair bit of memory depending upon implementation details. I'd suggest an ~RTT, since by that point you're expecting the acknowledgement of the retransmitted data.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The PTO is the period of time that you expect the ack back by, not RTT.

It's true that 3 PTO is a conservative value, but, it's only on lost packets. When that's the uncommon case, it won't consume a lot of memory. When it's the common case, you're not sending a lot of packets anymore because the sender is in congestion, so it shouldn't also cause memory exhaustion.

Either way, this is a suggestion, not a recommendation.

Copy link
Member

Choose a reason for hiding this comment

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

Generally speaking, I tend to think that 3*PTO is fine, and that is because the size of a sentmap entry (i.e., the amount of memory required to remember what was sent in a packet) is only a tiny fraction of the MTU.

If your send buffer is capable of retaining X packets amount of data, then it does not matter if you use one tiny fraction, two tiny fractions, or three, for sentmap.

All that said, I agree that it might sound a bit frightening, it does not need to be as long as 3PTO, and therefore my view is that this is a trade-off issue between simplicity (i.e. reusing 3PTO timer) vs. less frightening.

Comment on lines 3807 to 3813
When a sender declares a packet as lost and either retransmits the contained
frames or marks them for retransmission, the sender SHOULD avoid subsequent
retransmission of this information if it later receives an acknowledgement for
that packet. Doing so requires senders to retain information about packets after
they are declared lost. A sender can discard this information after a period of
time elapses, such as three times the PTO (Section 6.2 of {{QUIC-RECOVERY}}), or
on other events, such as reaching a memory limit.
Copy link
Contributor

Choose a reason for hiding this comment

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

How about "..after receiving an acknowledgement for a previously lost packet." I think late acknowledgement is potentially confusing.

Comment on lines 3807 to 3813
When a sender declares a packet as lost and either retransmits the contained
frames or marks them for retransmission, the sender SHOULD avoid subsequent
retransmission of this information if it later receives an acknowledgement for
that packet. Doing so requires senders to retain information about packets after
they are declared lost. A sender can discard this information after a period of
time elapses, such as three times the PTO (Section 6.2 of {{QUIC-RECOVERY}}), or
on other events, such as reaching a memory limit.
Copy link
Contributor

Choose a reason for hiding this comment

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

And how about starting with "A packet can be acknowledged after being declared lost." instead of "Acknowledgements can be received after a sender declares a packet as lost."

Copy link
Contributor Author

@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.

I've rewritten the long sentence based on feedback

retransmission of this information if it later receives an acknowledgement for
that packet. Doing so requires senders to retain information about packets after
they are declared lost. A sender can discard this information after a period of
time elapses, such as three times the PTO (Section 6.2 of {{QUIC-RECOVERY}}), or
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The PTO is the period of time that you expect the ack back by, not RTT.

It's true that 3 PTO is a conservative value, but, it's only on lost packets. When that's the uncommon case, it won't consume a lot of memory. When it's the common case, you're not sending a lot of packets anymore because the sender is in congestion, so it shouldn't also cause memory exhaustion.

Either way, this is a suggestion, not a recommendation.

Copy link
Contributor

@ianswett ianswett left a comment

Choose a reason for hiding this comment

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

I still think 3*PTO is far too long. This time period exists to ensure there's sufficient tolerance of reordering, not to wait for multiple PTOs to fire.

Comment on lines 3810 to 3811
are declared lost. A sender can discard this information after a period of time
elapses, such as three times the PTO (Section 6.2 of {{QUIC-RECOVERY}}), or on
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
are declared lost. A sender can discard this information after a period of time
elapses, such as three times the PTO (Section 6.2 of {{QUIC-RECOVERY}}), or on
are declared lost. A sender can discard this information a period of time after the
packet is lost, such as after the Smoothed RTT (Section 5 of {{QUIC-RECOVERY}}), or on

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'm happy to change it to 1 x PTO. I'd like the ack delay and the rttdev to be included in this suggestion.

Copy link
Contributor

Choose a reason for hiding this comment

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

ack delay shouldn't need to be included, because the packet would be acknowledged immediately.

Including RTTVar implies that the network's reordering is sometimes as much 3*SRTT, but assuming the data was retransmitted, you'd expect the retransmission to be acknowledged in about 1 RTT, so suggesting you might need to wait longer seems odd to me.

@janaiyengar
Copy link
Contributor Author

Added changes to recovery as well.

Copy link
Contributor

@ianswett ianswett left a comment

Choose a reason for hiding this comment

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

Some suggestions, but I think we're close.

time comparable to the maximum expected packet reordering, such as 1 RTT.
This allows for detection of spurious retransmissions.
After a packet is declared lost, the endpoint can still maintain state for it
for an amount of time, such as a PTO, to allow for packet reordering. This
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
for an amount of time, such as a PTO, to allow for packet reordering. This
for an amount of time to allow for packet reordering. This

Comment on lines 3808 to 3812
acknowledged. This includes packets that are acknowledged after being declared
lost. Doing so requires senders to retain information about packets after they
are declared lost. A sender can discard this information after a period of time
elapses, such as a PTO (Section 6.2 of {{QUIC-RECOVERY}}), or on other events,
such as reaching a memory limit.
Copy link
Contributor

@ianswett ianswett Jul 28, 2020

Choose a reason for hiding this comment

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

Suggested change
acknowledged. This includes packets that are acknowledged after being declared
lost. Doing so requires senders to retain information about packets after they
are declared lost. A sender can discard this information after a period of time
elapses, such as a PTO (Section 6.2 of {{QUIC-RECOVERY}}), or on other events,
such as reaching a memory limit.
acknowledged. Doing this for packets acknowledged after they are declared
lost requires retaining state for lost packets. A sender can discard lost packet state
after the expected maximum packet reordering elapses, such as after PTO
(Section 6.2 of {{QUIC-RECOVERY}}), or on other events, such as reaching a packet limit.

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 will take the compression, but we don't need to say "expected maximum packet reordering", since that's not a well-defined term. I'll spell in out in other words.

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'm trying to wordsmith this compression, but it's a bit difficult. The text is a bit verbose, but I think it is clearer. Unless you think there's a substantive difference, I'd prefer the existing text for clarity.

Copy link
Member

Choose a reason for hiding this comment

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

The expanded form is fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

The intent was to both make it clearer and talk about the goal of waiting for a reordering period. I find the existing text a bit unclear, which is why I put the first two sentences together.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, added some rationale.

Copy link
Contributor

@ianswett ianswett left a comment

Choose a reason for hiding this comment

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

I think what you're saying is fine, but another attempt at an editorial suggstion.

draft-ietf-quic-transport.md Show resolved Hide resolved
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.

Let's not thrash too much on this one.

@ianswett ianswett added the design An issue that affects the design of the protocol; resolution requires consensus. label Aug 4, 2020
@janaiyengar janaiyengar merged commit c5923f5 into master Sep 1, 2020
@janaiyengar janaiyengar deleted the jri/late-ack branch September 1, 2020 19:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-transport 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.

Suggest endpoints may recognize acks after loss is declared
5 participants