Use PTO to declare in-flight packets lost #1965

ianswett · 2018-11-05T07:40:14Z

QUIC, unlike TCP, allows the connection to get into a situation where there could be packets in flight, and a connection could even be CWND limited, but there may not be any outstanding data to retransmit.

The suggested solution is to send a retransmittable frame.

QUIC, unlike TCP, allows the connection to get into a situation where there could be packets in flight, and a connection could even be CWND limited, but there may not be any outstanding data to retransmit. The suggested solution is to send a retransmittable frame.

martinduke

I agree with the concept, though I have some problems with the explanatory text.

One zoom-out question about TLP in QUIC: If there's a tail loss, you'll have to wait for an entire delayed-ack timeout to send, wait at worst for another DAT to get the ack, then a reordering timeout to declare the intervening packets lost. How does that compare to a plain old RTO?

martinduke · 2018-11-05T08:16:09Z

draft-ietf-quic-recovery.md

@@ -418,6 +418,12 @@ used to send a probe into the network prior to establishing any packet loss,
 prior unacknowledged packets SHOULD NOT be marked as lost when a TLP timer
 expires.

+If no new data or unacknowledged data is available to send, a retransmittable
+frame SHOULD be sent.  Sending a retransmittable frame ensures that any in
+flight packets are acknowledged or declared lost in a timely manner,


I don't think TLP accelerates acknowledgment of in-flight pkts unless you happen to hit what the maximum number of delayed-acked pkts are.

So I would delete "acknowledged or"

Well, if you TLP, then you haven't received an ACK that contained any information about the in flight packets, because otherwise you'd fast or early retransmit them. So you have some number of in flight packets, and you don't know what their status is. It's possible the ACK was just lost, etc. So in that sense, it accelerates receiving an acknowledgement for them if they were in fact not lost.

Ah yes, ack losses. You're right

martinduke · 2018-11-05T08:16:29Z

draft-ietf-quic-recovery.md

+If no new data or unacknowledged data is available to send, a retransmittable
+frame SHOULD be sent.  Sending a retransmittable frame ensures that any in
+flight packets are acknowledged or declared lost in a timely manner,
+potentially preventing a deadlock if all in flight packets contain no data


Delete everything after "deadlock."

Sorry, I update the text right as you were reviewing, PTAL.

New data also prevents a deadlock.

martinduke

D'oh! comments passed in flight.

martinduke · 2018-11-05T08:27:17Z

draft-ietf-quic-recovery.md

@@ -418,6 +418,12 @@ used to send a probe into the network prior to establishing any packet loss,
 prior unacknowledged packets SHOULD NOT be marked as lost when a TLP timer
 expires.

+If no new data or unacknowledged data is available to send, a retransmittable
+frame SHOULD be sent.  Sending a retransmittable frame ensures that any in
+flight packets are acknowledged or declared lost in a timely manner.


"acknowledged or" remains problematic.

Never mind, see above.

MikeBishop · 2018-11-05T09:30:18Z

draft-ietf-quic-recovery.md

+frame SHOULD be sent.  Sending a retransmittable frame ensures that any in
+flight packets are acknowledged or declared lost in a timely manner.
+Otherwise, a deadlock results if there is no available congestion window and
+all in flight packets contain no data that can be retransmitted.


Hyphenate "in-flight" when used before the noun this way.

ekr · 2018-11-05T11:33:51Z

I feel like maybe I don't understand this. It's titled "deadlock" but it looks to me like it's really just RTO. Am I misunderstanding?

ianswett · 2018-11-05T15:32:15Z

Good question, @ekr

It's definitely a detail point, but I think the above approach is likely the ideal fix. But here's what I expect to happen now.

If the implementation stores some state saying "the tlp alarm fired, but I couldn't send anything, and I'll remember that in case there is some data I can send in the near future." then when there's new data, everything should be fixed, though you'll also only be able to send a single packet. And if that packet or the corresponding ACK is lost, it'll be another TLP before you can send a packet.

Alternately, an implementation could fire the alarm, realize it can't send anything, then not re-arm the alarm(since if it was re-armed, it would just fire immediately and you'd spin).

The second approach is a deadlock. The first one is just going to be very slow recovery. Both seem somewhat suboptimal.

There's another solution to this, which is to declare all in-flight packets lost. But that goes against the approach we use throughout QUIC loss recovery where we don't declare packets lost until a larger packet is acknowledged.

OT: @martinduke pointed out that this should be fixed for RTO as well, since it's possible between TLP and RTO(or between two RTOs) in-flight data is cancelled, so now there's nothing to retransmit.

mikkelfj · 2018-11-05T17:02:25Z

draft-ietf-quic-recovery.md

@@ -142,7 +142,7 @@ of frames contained in a packet affect recovery and congestion control logic:

 * Packets that contain only ACK frames do not count toward congestion control
  limits and are not considered in-flight. Note that this means PADDING frames
-  cause packets to contribute toward bytes in flight without directly causing an
+  cause packets to contribute toward bytes in-flight without directly causing an
  acknowledgment to be sent.


I think it was agreed to spell this "bytes in flight" but I could be wrong.

If in-flight is used, I think other text needs to be updated also. The motivation for not using hyphen was the confusion with bytes_in_flight as a variable name.

"in-flight" when used as a compound modifier (in-flight packets, for example) should usually have a hyphen before the noun. Here, it's after the noun, so this wouldn't get one. See here, for example.

maolson-msft · 2018-11-05T20:42:04Z

Rather than adding a new paragraph to patch the hole, I'd update the description of what a "probe" packet is. Currently it's "a packet which is not subject to CC and which contains unsent data (if possible) or sent data (otherwise)."

I'd update this sentence and move it somewhere to apply to both TLP and RTO probes.

nibanks · 2018-11-05T20:51:30Z

@ianswett do you have an example of a case where you have packets in flight but no outstanding data to retransmit? I was trying to come up with one but couldn't. In the case of a stream getting cancelled, you'd still have the RST_STREAM frame that needs to be sent.

mikkelfj · 2018-11-05T20:55:15Z

@nibanks couldn't RST happen async separate from the pipeline of schedule transmissions. More generally, data might exist, but not (yet) in the low level tranmissions system that deals with retransmission.

nibanks · 2018-11-05T20:56:53Z

@mikkelfj that's really an implementation detail. For me, resetting a stream, queuing the RST_STREAM frame for delivery and removing the actual stream data for retransmission is an atomic operation.

mikkelfj · 2018-11-05T21:27:10Z

@nibanks I agree, but because it is an implementation detail, the protocol cannot make general assumptions on how implementations work. Forcing an implemtation to operate atomically is not ideal.

EDIT: can->cannot in the above

nibanks · 2018-11-05T21:32:23Z

@mikkelfj I totally agree. I was just trying to understand if there were any non-implementation specific scenarios where this could happen (in other words, does this apply to my implementation or can I ignore this bit of text).

ianswett · 2018-12-19T04:02:09Z

Given if there is something to be sent, I don't think this is a deadlock, I'm inclined to close this PR, both because it's old and I'm not convinced it's a practical issue.

martinduke · 2018-12-19T06:45:45Z

@nibanks The simplest way for this to happen is that the first TLP timeout resends some data, and then the original is acked. Thus we have an in-flight packet (the retransmission) but the actual application data is already gone from the send buffer.

The TLP timer will fire again, and there's nothing to send.

@ianswett There are still some problems with the new text. The main practical impact is that we will delay declaring the last packet lost, meaning we won't free up the flightsize or execute a congestion response as quickly. I'm having a little trouble reasoning about the full implications of this for new data.

ianswett · 2018-12-19T14:58:56Z

I updated this to clarify it's not really a deadlock, but suggest what should be done on PTO in this case: either send an ack-eliciting packet or declare the packets lost.

martinduke · 2018-12-19T16:16:39Z

For the record, I believe this was a potential deadlock:
Pkt 5 outstanding, flightsize = cwnd = 1 MSS
TLP timeout, resend in pkt 6 (flightsize = 2 MSS)
Pkt 5 acked, no data in send queue (flightsize = cwnd = 1 MSS)
TLP timeout, nothing to send, no new timers.
If new data arrives, there is no way to progress.

ianswett · 2018-12-19T16:25:37Z

Previously, Jana said you should store that the PTO had fired and there was nothing to send, so when data does arrive to send, you can immediately send it. But that wasn't explicitly documented anywhere to my knowledge.

janaiyengar

I think @martinduke is right that we should handle this particular case, and I'm fine with the resolution in this PR. A few comments (and the text is in teh wrong place)

janaiyengar · 2018-12-19T21:58:02Z

draft-ietf-quic-recovery.md

+
+A sender may not know that a packet being sent is a tail packet.  Consequently,
+a sender may have to arm or adjust the TLP timer on every sent retransmittable
+packet.


I suspect this para is leftover and should be removed.

draft-ietf-quic-recovery.md

janaiyengar · 2018-12-19T22:04:10Z

draft-ietf-quic-recovery.md

@@ -421,6 +421,15 @@ control and loss recovery state, including resetting any pending timers.  Either
 packet indicates that the Initial was received but not processed.  Neither
 packet can be treated as an acknowledgment for the Initial.

+If no new data or unacknowledged data is available to send, an ack-eliciting


This new text should be further below in the #pto section.

Co-Authored-By: ianswett <ianswett@users.noreply.github.com>

A proposal: I think I prefer to mark things as lost instead of suggesting that a sender SHOULD send something when there's really nothing to be sent. I was starting to rephrase your PR to say "SHOULD send ack-eliciting packet, but MAY mark as lost", and I needed to explain how a sender makes this choice. I realized that if it's been long enough (PTO is long enough) and there's nothing left to send, then maybe it is sensible to mark anything pending as lost. So, I changed my suggestion to that, which is this PR (against your PR).

janaiyengar · 2018-12-20T01:26:39Z

Hm. I started to think about how to rephrase a bit, which ended up leading to #2212. Take a look.

Proposed change to #1965

janaiyengar

@martinduke : See if this resolves your concerns.

ianswett · 2018-12-20T21:50:14Z

@martinduke Jana and I replaced my text with a version of his PR. We believe this solves the issue you point out as well.

martinduke · 2018-12-26T16:48:11Z

I like this language a lot. Ship it!

ianswett requested review from janaiyengar and martinduke November 5, 2018 07:40

ianswett added 2 commits November 5, 2018 03:00

Update draft-ietf-quic-recovery.md

38dbee0

Update draft-ietf-quic-recovery.md

5fc3b2e

martinduke approved these changes Nov 5, 2018

View reviewed changes

MikeBishop reviewed Nov 5, 2018

View reviewed changes

Mike in-flight

9905361

mikkelfj reviewed Nov 5, 2018

View reviewed changes

martinthomson added design An issue that affects the design of the protocol; resolution requires consensus. -recovery labels Nov 13, 2018

ianswett added 3 commits December 19, 2018 09:52

Merge branch 'master' into ianswett-tlp-deadlock

ae6aedc

Update draft-ietf-quic-recovery.md

fe11f58

Not a deadlock

699fb76

ianswett changed the title ~~Prevent an unlikely deadlock~~ Use PTO to declare in-flight packets lost Dec 19, 2018

janaiyengar reviewed Dec 19, 2018

View reviewed changes

janaiyengar and others added 4 commits December 19, 2018 19:06

Update draft-ietf-quic-recovery.md

b180724

Co-Authored-By: ianswett <ianswett@users.noreply.github.com>

Jana's suggestions

59377bf

Update draft-ietf-quic-recovery.md

6b71902

janaiyengar mentioned this pull request Dec 20, 2018

On PTO either abandon packets or send another one #2212

Merged

janaiyengar added 6 commits December 19, 2018 17:38

reflow

cf7894a

add MAY re-arm PTO

a7b0f36

more rephrasing

66c435b

more simple

302f3df

comma

8e1c599

Merge pull request #2212 from quicwg/janaiyengar-patch-2

f09333b

Proposed change to #1965

janaiyengar approved these changes Dec 20, 2018

View reviewed changes

ianswett merged commit acf67af into master Dec 27, 2018

martinthomson deleted the ianswett-tlp-deadlock branch January 22, 2019 07:59

Use PTO to declare in-flight packets lost #1965

Use PTO to declare in-flight packets lost #1965

Conversation

ianswett commented Nov 5, 2018

martinduke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinduke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekr commented Nov 5, 2018

ianswett commented Nov 5, 2018

Choose a reason for hiding this comment

mikkelfj Nov 5, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maolson-msft commented Nov 5, 2018

nibanks commented Nov 5, 2018 • edited

mikkelfj commented Nov 5, 2018

nibanks commented Nov 5, 2018

mikkelfj commented Nov 5, 2018 • edited

nibanks commented Nov 5, 2018

ianswett commented Dec 19, 2018

martinduke commented Dec 19, 2018

ianswett commented Dec 19, 2018

martinduke commented Dec 19, 2018

ianswett commented Dec 19, 2018

janaiyengar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janaiyengar commented Dec 20, 2018

janaiyengar left a comment

Choose a reason for hiding this comment

ianswett commented Dec 20, 2018

martinduke commented Dec 26, 2018

mikkelfj Nov 5, 2018 •

edited

nibanks commented Nov 5, 2018 •

edited

mikkelfj commented Nov 5, 2018 •

edited