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

Make in_flight the criterion for declaring loss #2104

Merged
merged 3 commits into from Dec 18, 2018
Merged

Make in_flight the criterion for declaring loss #2104

merged 3 commits into from Dec 18, 2018

Conversation

martinduke
Copy link
Contributor

If a packet contains just ACKs and PADDING, it is not retransmittable but must be declared lost to free up the congestion window space.

If a packet contains just ACKs and PADDING, it is not retransmittable but must be declared lost to free up the congestion window space.
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.

This pseudocode is also being updated in PR#2066, but you're correct that it should be in_flight, not retransmittable.


* The packet is unacknowledged, retransmittable, and was sent prior to an
acknowledged packet.
* The packet is in-flight, and was sent prior to an acknowledged packet.
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you intend to drop "unacknowledged"?

@janaiyengar
Copy link
Contributor

janaiyengar commented Dec 7, 2018

I don't think this is correct. We mark something as lost because we expected to hear an ACK back but didn't. That's true of ack-eliciting (née retransmittable) but not of all things that are considered in flight. PADDING is the exception -- it is considered in flight, but doesn't elicit an ACK. A sender that pads ACK-only packets should add a PING frame in there to elicit an ack for this reason, so that the bytes in flight can be recovered when the packet is lost.

@ianswett
Copy link
Contributor

ianswett commented Dec 7, 2018

I think strictly speaking, neither option may be correct, unfortunately.

Packets that are in_flight count towards bytes in flight, so need to be removed from flight eventually. And they may never get acknowledged. The transport and recovery drafts are either a bit unclear or in conflict about whether one should ACK a PADDING only packet that arrives out of order.

The transport draft currently says "To avoid creating an indefinite feedback loop, an endpoint MUST NOT send an ACK frame in response to a packet containing only ACK or PADDING frames, even if there are packet gaps which precede the received packet."

In recovery it says "Out-of-order packets SHOULD be acknowledged more quickly, in order to accelerate loss recovery. The receiver SHOULD send an immediate ACK when it receives a new packet which is not one greater than the largest received packet number."

I think this is an excellent reason to move the text about "Generating Acknowlegements" into transport, because having it in two places increases the chances of inconsistencies.

I tend to think that the best thing to do is take Martin's change, but also say that if an in-flight packet arrives out of order, an immediate acknowledgement should be sent, because it counts towards bytes in flight.

Again, having a packet that counts towards bytes in flight, but doesn't generate an ACK, generates a bunch of edge cases I dislike, but I think we're stuck with it.

@janaiyengar
Copy link
Contributor

I think this is an excellent reason to move the text about "Generating Acknowlegements" into transport, because having it in two places increases the chances of inconsistencies.

Agreed. It's on my todo for next week.

That's about what to do with acking PADDING packets. The question I'm raising is whether loss detection is tied to being in flight or to eliciting an ACK. I'm arguing that loss detection is premised on bytes counting towards in-flight, but loss detection logic triggered by receiving an ACK (which is why we use max_ack_delay in the computations). The bytes being in flight are a necessary condition, not sufficient.

Loss detection marks a packet as lost when no ACK is received for it. This is predicated on an ACK being sent by the receiver, delayed or not, if the packet is received, which is only true for ack-eliciting packets.

PADDING of ACK-only frames is a weird special case that causes inflight to go up without being ack-eliciting. We had agreed in Montreal that if someone did this they will need to either add a PING frame in there or handle special conditions.

@mikkelfj
Copy link
Contributor

mikkelfj commented Dec 7, 2018

I don’t understand the idea of adding a PING to ACK/PAD only frames. If you do that you could get infinite feedback loops and you might as well just ACK all frame promptly.

@mikkelfj
Copy link
Contributor

mikkelfj commented Dec 7, 2018

How about a sender self-acknowledge packets that are not ack-eliciting, for example packets older than largest acked or older than triggering TLP? Once these packets are ack'ed or self-ack'ed, they are retired from the congestion window.

Or just keep the packets out of the congestion window if that is at all feasible. I'm not really keen on the above complexity if it can be avoided.

@ianswett
Copy link
Contributor

ianswett commented Dec 7, 2018

You're right Jana, we've agreed that padding without a ping/etc is a pointy stick. But I think the idea was that as long as you sent an ack-eliciting frame(ping or otherwise) at least once per RTT, you'd get an ACK, and then you could do loss detection on the in flight packets.

If we leave the text as is, if a padding only packet is never ACKed, there is no mechanism for removing it from in flight, because that occurs only in OnPacketAckedCC and OnPacketsLost.

If we don't make any changes, it's really unsafe to send padding-only packets, and we're forcing implementers to figure out something to do here. I'd rather recommend something that does not cause a deadlock, even if it may declare a few padding only packets as lost in cases when they were actually reordered.

@nibanks
Copy link
Member

nibanks commented Dec 7, 2018

I feel like this is somewhat related to my issue (#2015) related to calling OnPacketsLost for all lost packets, not just retransmittable/ack-eliciting ones. If you know the packet was lost, then process it as such. Now, what you do it response to it being lost is where you would then put the in_flight check for removing the bytes_inflight` and whatever other logic.

@ianswett
Copy link
Contributor

ianswett commented Dec 7, 2018

Hi Martin, I resolved the conflicts with the recently merged Ack-eliciting change for you. Feel free to make further modifications.

If we decide to change this, we should ensure the text is clear as well and offers an explanation for the outcome of this discussion.

@janaiyengar
Copy link
Contributor

I wanted to ensure that we don't set the timer based on in-flight. This PR doesn't do that... and this PR is correct. I'll merge.

@janaiyengar janaiyengar merged commit c91232a into quicwg:master Dec 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants