Allow alternate restarts after idle #2187

janaiyengar · 2018-12-14T23:35:01Z

Closes #2138.

ianswett

One small suggestion, but LGTM

ianswett · 2018-12-14T23:47:20Z

draft-ietf-quic-recovery.md

-packets to no more than the initial congestion window and subsequent packets
-SHOULD be paced. The congestion window does not change while the connection
-is idle.
+If the sender uses pacing, the congestion window MUST not increase and does not


Possibly the "MUST not increase CWND" part should move a paragraph up, since it's really not pacing specific?

draft-ietf-quic-recovery.md

janaiyengar · 2018-12-15T01:01:50Z

I've restructured the section a bit more.

janaiyengar · 2018-12-16T19:08:19Z

@ianswett: Take a look again -- I've made some changes.

draft-ietf-quic-recovery.md

martinthomson · 2018-12-17T03:12:55Z

draft-ietf-quic-recovery.md

+
+  - If the sender does not use pacing, it SHOULD reset its congestion window to
+    the smaller of the current congestion window and the initial congestion
+    window, as recommended for TCP (see Section 4.1 of {{?RFC5681}}).


Maybe ignore my suggestions and use this instead:

When sending data after becoming idle, a sender MUST reset its congestion window to the initial congestion window (see blah), unless it paces the sending of packets. A sender MAY retain its congestion window if it paces the sending of any packets in excess of the initial congestion window.

I realize that this changes the emphasis slightly, but I think that it's clearer and more succinct.

I still think that this is an abrupt transition, but I guess the answer to any complaint there is simply "pacing".

Pacing is the answer.

There is a degenerate case where traffic arrives in blocks just under init cwnd at a high rate. The application can then burst at a high frequency.

I like this better. I'm using this.

mikkelfj · 2018-12-17T09:21:49Z

draft-ietf-quic-recovery.md

+
+  - If the sender uses pacing, it does not need to reduce its congestion
+    window. It SHOULD however pace the congestion window and MAY allow an
+    initial burst no larger than the initial congestion window.


I don't understand this. "MAY allow initial burst no larger", does this it MAY also allow larger?

How about "It MAY allow an initial burst as large as the initial congestion window and MUST pace out the rest of the congestion window."

ianswett

I'm a bit worried we're subtly missing an important case we want to catch.

ianswett · 2018-12-17T16:24:36Z

draft-ietf-quic-recovery.md

+
+A sender is considered idle if it has no bytes in flight and no pending
+ack-eliciting data to send.  A sender can become idle when it is application
+limited or when it encounters a retransmission timeout.


Probe timeout? Also, we want the case after a PTO to be considered idle, correct? But one may have data to send, so this text doesn't catch that.

I've replaced retransmission with probe.
Actually I think this entire second point is wrong now. A sender does not become idle on a PTO, it might become idle once loss is established, when an ACK is received.

Also, idle is when there's nothing in flight. I don't think we need the "pending data to send" bit.

ianswett · 2018-12-17T16:26:24Z

draft-ietf-quic-recovery.md

+
+  - If the sender uses pacing, it does not need to reduce its congestion
+    window. It SHOULD however pace the congestion window and MAY allow an
+    initial burst no larger than the initial congestion window.


How about "It MAY allow an initial burst as large as the initial congestion window and MUST pace out the rest of the congestion window."

ianswett · 2018-12-17T16:27:52Z

draft-ietf-quic-recovery.md

+
+  - If the sender does not use pacing, it SHOULD reset its congestion window to
+    the smaller of the current congestion window and the initial congestion
+    window, as recommended for TCP (see Section 4.1 of {{?RFC5681}}).


Pacing is the answer.

ianswett

I meant to un-approve this before.

Co-Authored-By: janaiyengar <jri.ietf@gmail.com>

janaiyengar · 2018-12-18T00:59:19Z

Ok, comments incorporated. I like @martinthomson's text, so I'm using that.

ianswett

This looks better, but I'm still a bit confused about the post-PTO case

ianswett · 2018-12-18T02:26:30Z

draft-ietf-quic-recovery.md

+
+## Sending data after an idle period
+
+A sender is considered idle if it has no bytes in flight.  A sender can become


Suggested change

A sender is considered idle if it has no bytes in flight. A sender can become

A sender is idle if it has no bytes in flight. A sender can become

Or

Suggested change

A sender is considered idle if it has no bytes in flight. A sender can become

A sender becomes idle if it has no bytes in flight. A sender can become

ianswett · 2018-12-18T02:27:25Z

draft-ietf-quic-recovery.md

+
+A sender is considered idle if it has no bytes in flight.  A sender can become
+idle when it is application limited and has no data to send, or when it marks
+packets as lost (and therefore not in flight).


Is this aiming at the PTO case?

The text that was there was probably leftover from a while ago... at any rate there is no PTO case. There is nothing marked as lost on a PTO, and bytes in flight does not change on a PTO (modulo the two packets that are sent in response). So a sender cannot become idle on a PTO.

ianswett · 2018-12-18T22:17:04Z

draft-ietf-quic-recovery.md

+## Sending data after an idle period
+
+A sender becomes idle if it has no bytes in flight.  A sender can become idle
+when it is application limited and has no data to send, or when it marks packets


Suggested change

when it is application limited and has no data to send, or when it marks packets

when it is application limited and has no data to send, or when it marks all packets

ianswett · 2018-12-18T22:17:24Z

draft-ietf-quic-recovery.md

+
+A sender becomes idle if it has no bytes in flight.  A sender can become idle
+when it is application limited and has no data to send, or when it marks packets
+as lost (and therefore not in flight).


Suggested change

as lost (and therefore not in flight).

as lost, and therefore not in flight.

Can this also happen when all packets are acknowledged? I'm not sure what the preferred behavior is in that case. But the current text now properly captures the "my last PTO was acked and everything else was declared lost."

Yes, that is a fine case ... this is what we've always had, the RTO text was incorrect. Basically the sender goes idle as soon as bytes in flight goes to 0. Maybe that's all we need to say. I've removed more text to make this clearer.

janaiyengar requested review from martinthomson and ianswett December 14, 2018 23:35

ianswett approved these changes Dec 14, 2018

View reviewed changes

ianswett reviewed Dec 15, 2018

View reviewed changes

draft-ietf-quic-recovery.md Show resolved Hide resolved

janaiyengar added 2 commits December 16, 2018 11:04

Allow alternate restarts after idle

6df0e3a

restructure

b866ed6

janaiyengar force-pushed the cwv branch from 847edda to b866ed6 Compare December 16, 2018 19:06

martinthomson approved these changes Dec 17, 2018

View reviewed changes

mikkelfj reviewed Dec 17, 2018

View reviewed changes

ianswett reviewed Dec 17, 2018

View reviewed changes

ianswett added editorial An issue that does not affect the design of the protocol; does not require consensus. -recovery labels Dec 17, 2018

ianswett requested changes Dec 17, 2018

View reviewed changes

martinthomson and others added 3 commits December 17, 2018 16:43

Update draft-ietf-quic-recovery.md

2133100

Co-Authored-By: janaiyengar <jri.ietf@gmail.com>

Update draft-ietf-quic-recovery.md

2bbc1ff

Co-Authored-By: janaiyengar <jri.ietf@gmail.com>

more changes

6fdb947

ianswett reviewed Dec 18, 2018

View reviewed changes

becomes

adfc4a0

ianswett reviewed Dec 18, 2018

View reviewed changes

simpler

1053a1f

ianswett approved these changes Dec 18, 2018

View reviewed changes

janaiyengar merged commit b68b46f into master Dec 18, 2018

martinthomson deleted the cwv branch December 19, 2018 04:13

marten-seemann mentioned this pull request Dec 19, 2018

fix MUST not in idle period #2210

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow alternate restarts after idle #2187

Allow alternate restarts after idle #2187

janaiyengar commented Dec 14, 2018

ianswett left a comment

ianswett Dec 14, 2018

janaiyengar commented Dec 15, 2018

janaiyengar commented Dec 16, 2018

martinthomson Dec 17, 2018

ianswett Dec 17, 2018

mikkelfj Dec 17, 2018

janaiyengar Dec 18, 2018

mikkelfj Dec 17, 2018

ianswett Dec 17, 2018

mikkelfj Dec 17, 2018

ianswett left a comment

ianswett Dec 17, 2018

janaiyengar Dec 18, 2018

janaiyengar Dec 18, 2018

ianswett Dec 17, 2018

ianswett Dec 17, 2018

ianswett left a comment

janaiyengar commented Dec 18, 2018

ianswett left a comment

ianswett Dec 18, 2018

martinthomson Dec 18, 2018

ianswett Dec 18, 2018

janaiyengar Dec 18, 2018

ianswett Dec 18, 2018

ianswett Dec 18, 2018

ianswett Dec 18, 2018

janaiyengar Dec 18, 2018


		## Sending data after an idle period

		A sender is considered idle if it has no bytes in flight. A sender can become

	A sender is considered idle if it has no bytes in flight. A sender can become
	A sender is idle if it has no bytes in flight. A sender can become

	A sender is considered idle if it has no bytes in flight. A sender can become
	A sender becomes idle if it has no bytes in flight. A sender can become

	when it is application limited and has no data to send, or when it marks packets
	when it is application limited and has no data to send, or when it marks all packets

	as lost (and therefore not in flight).
	as lost, and therefore not in flight.

Allow alternate restarts after idle #2187

Allow alternate restarts after idle #2187

Conversation

janaiyengar commented Dec 14, 2018

ianswett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janaiyengar commented Dec 15, 2018

janaiyengar commented Dec 16, 2018

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

ianswett 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

ianswett left a comment

Choose a reason for hiding this comment

janaiyengar commented Dec 18, 2018

ianswett 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment