Subject: Re: [quicwg/base-drafts] Change PTO to be per packet number space (#3066)

From: martinduke <[email-masked]>
Date: Tue, 15 Oct 2019 00:12:07 -0700

martinduke commented on this pull request.

> @@ -496,9 +500,7 @@ be considered an RTT sample.
 
 Until the server has validated the client's address on the path, the amount of
 data it can send is limited, as specified in Section 8.1 of {{QUIC-TRANSPORT}}.
-Data at Initial encryption MUST be retransmitted before Handshake data and
-data at Handshake encryption MUST be retransmitted before any ApplicationData
-data.  If no data can be sent, then the PTO alarm MUST NOT be armed until
+If no data can be sent, then the PTO alarm MUST NOT be armed until
 data has been received from the client.

s/data/valid packets

"data", to me, implies payloads of CRYPTO or STREAM freams.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/3066#pullrequestreview-301692497

---

From: martinduke <[email-masked]>
Date: Tue, 15 Oct 2019 00:14:30 -0700

martinduke requested changes on this pull request.

As discussed this afternoon, as written this implies up to six packets sent per PTO interval. While I could live with that, I think we should limit it to "no more than one datagram per crypto level, but at least two datagrams" or something to that effect.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/3066#pullrequestreview-301693575

---

From: ianswett <[email-masked]>
Date: Wed, 23 Oct 2019 19:42:27 -0700

@ianswett pushed 1 commit.

33c994e1f8e9123c59f5265e507842ab076433b5  Merge branch 'master' into ianswett-pto-per-pn-space

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3066/files/3abb83565f1fc9772614f118c5dccc212a45137d..33c994e1f8e9123c59f5265e507842ab076433b5

---

From: ianswett <[email-masked]>
Date: Wed, 23 Oct 2019 19:43:20 -0700

6c3ae350dc546bf6756258f93a2d40a64cc2daa4  Update draft-ietf-quic-recovery.md

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3066/files/33c994e1f8e9123c59f5265e507842ab076433b5..6c3ae350dc546bf6756258f93a2d40a64cc2daa4

---

From: ianswett <[email-masked]>
Date: Wed, 23 Oct 2019 19:45:40 -0700

3f0da814e8a6ca8d0223457fbe5bb90157d6df15  Update draft-ietf-quic-recovery.md

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3066/files/6c3ae350dc546bf6756258f93a2d40a64cc2daa4..3f0da814e8a6ca8d0223457fbe5bb90157d6df15

---

From: ianswett <[email-masked]>
Date: Wed, 23 Oct 2019 19:56:47 -0700

62e394f7a538c0bf4ef44787510b0229eb118af2  Update draft-ietf-quic-recovery.md

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3066/files/3f0da814e8a6ca8d0223457fbe5bb90157d6df15..62e394f7a538c0bf4ef44787510b0229eb118af2

---

From: ianswett <[email-masked]>
Date: Thu, 24 Oct 2019 02:58:10 +0000 (UTC)

ianswett commented on this pull request.

Thanks for the feedback, PR updated.

> @@ -438,11 +438,13 @@ and larger thresholds increase loss detection delay.
 A Probe Timeout (PTO) triggers sending one or two probe datagrams when
 ack-eliciting packets are not acknowledged within the expected period of
 time or the handshake has not been completed.  A PTO enables a connection to
-recover from loss of tail packets or acknowledgements. The PTO algorithm used
-in QUIC implements the reliability functions of Tail Loss Probe
-{{?TLP=I-D.dukkipati-tcpm-tcp-loss-probe}} {{?RACK}}, RTO {{?RFC5681}} and
-F-RTO algorithms for TCP {{?RFC5682}}, and the timeout computation is based on
-TCP's retransmission timeout period {{?RFC6298}}.
+recover from loss of tail packets or acknowledgements.  As with loss
+detection, the probe timeout is per packet number space.
+
+The PTO algorithm used in QUIC implements the reliability functions of
+Tail Loss Probe {{?TLP=I-D.dukkipati-tcpm-tcp-loss-probe}} {{?RACK}},

Good point.  RACK includes TLP, so maybe we should remove the TLP reference?
https://tools.ietf.org/id/draft-ietf-tcpm-rack-03.html If so, I would prefer to do that in a separate PR #3137 

> @@ -522,9 +524,14 @@ removed from bytes in flight when the Initial and Handshake keys are discarded.
 ### Sending Probe Packets
 
 When a PTO timer expires, a sender MUST send at least one ack-eliciting packet
-as a probe, unless there is no data available to send.  An endpoint MAY send up
-to two full-sized datagrams containing ack-eliciting packets, to avoid an
-expensive consecutive PTO expiration due to a single lost datagram.
+in the packet number space as a probe, unless there is no data available to
+send.  An endpoint MAY send up to two full-sized datagrams containing
+ack-eliciting packets, to avoid an expensive consecutive PTO expiration due

It's mostly redundant, but if there are two packets, this makes it clear they have to be ack-eliciting.  This didn't change as part of this PR, FWIW.

> @@ -522,9 +524,14 @@ removed from bytes in flight when the Initial and Handshake keys are discarded.
 ### Sending Probe Packets
 
 When a PTO timer expires, a sender MUST send at least one ack-eliciting packet
-as a probe, unless there is no data available to send.  An endpoint MAY send up
-to two full-sized datagrams containing ack-eliciting packets, to avoid an
-expensive consecutive PTO expiration due to a single lost datagram.
+in the packet number space as a probe, unless there is no data available to
+send.  An endpoint MAY send up to two full-sized datagrams containing
+ack-eliciting packets, to avoid an expensive consecutive PTO expiration due
+to a single lost datagram.

Good point, I updated the PR to try to limit the number of packets sent when implementations did not coalesce.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/3066#pullrequestreview-306196401

---

From: ianswett <[email-masked]>
Date: Wed, 23 Oct 2019 20:07:27 -0700

8af3d28bd220814a89edce17726f563783deac0d  Update draft-ietf-quic-recovery.md

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3066/files/62e394f7a538c0bf4ef44787510b0229eb118af2..8af3d28bd220814a89edce17726f563783deac0d

---

From: ianswett <[email-masked]>
Date: Wed, 23 Oct 2019 20:08:11 -0700

> @@ -1096,12 +1103,20 @@ Pseudocode for SetLossDetectionTimer follows:
 // Returns the earliest loss_time and the packet number
 // space it's from.  Returns 0 if all times are 0.
 GetEarliestLossTime():
-  time = loss_time[Initial]
+  return GetEarliestTimeAndSpace(loss_time)
+
+// Returns the earliest time_of_last_sent_ack_eliciting_packet
+// and the packet number space it's from.
+GetEarliestAckElicitingTime():
+  return GetEarliestTimeAndSpace(loss_time)

I removed the functions, because they were overly verbose.  And as you said, I used the wrong argument in one of them.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/3066#discussion_r338363812

---

From: ianswett <[email-masked]>
Date: Wed, 23 Oct 2019 20:09:29 -0700

49a28334da5cbecebb108424ba5db0dca7840e7b  Update draft-ietf-quic-recovery.md

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3066/files/8af3d28bd220814a89edce17726f563783deac0d..49a28334da5cbecebb108424ba5db0dca7840e7b

---

From: ianswett <[email-masked]>
Date: Wed, 23 Oct 2019 20:10:14 -0700

Changed to datagram, PTAL

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/3066#discussion_r338364156

---

From: Jana Iyengar <[email-masked]>
Date: Wed, 23 Oct 2019 20:26:38 -0700

janaiyengar commented on this pull request.

> @@ -471,8 +474,10 @@ the PTOs might be caused by loss of packets or acknowledgements due to severe
 congestion.  The life of a connection that is experiencing consecutive PTOs is
 limited by the endpoint's idle timeout.
 
-A sender computes its PTO timer every time an ack-eliciting packet is sent. A
-sender might choose to optimize this by setting the timer fewer times if it
+A sender computes its PTO timer every time an ack-eliciting packet is sent.
+When ack-eliciting packets are in-flight in multiple packet number spaces,
+the timer MUST be set from the packet number space with the earliest timeout.

```suggestion
the timer MUST be set for the packet number space with the earliest timeout.
```

> @@ -471,8 +474,10 @@ the PTOs might be caused by loss of packets or acknowledgements due to severe
 congestion.  The life of a connection that is experiencing consecutive PTOs is
 limited by the endpoint's idle timeout.
 
-A sender computes its PTO timer every time an ack-eliciting packet is sent. A
-sender might choose to optimize this by setting the timer fewer times if it
+A sender computes its PTO timer every time an ack-eliciting packet is sent.
+When ack-eliciting packets are in-flight in multiple packet number spaces,
+the timer MUST be set from the packet number space with the earliest timeout.
+A sender might choose to optimize this by setting the timer fewer times if it
 knows that more ack-eliciting packets will be sent within a short period of
 time.

Remove this sentence, it's unnecessary.

> @@ -522,9 +525,17 @@ removed from bytes in flight when the Initial and Handshake keys are discarded.
 
+In addition to sending data in the packet number space for which the timer
+expired, the sender SHOULD coalesce ack-eliciting packets from all other packet

Is this necessarily "all" spaces?

> @@ -522,9 +525,17 @@ removed from bytes in flight when the Initial and Handshake keys are discarded.
 
 When a PTO timer expires, a sender MUST send at least one ack-eliciting packet
-as a probe, unless there is no data available to send.  An endpoint MAY send up
-to two full-sized datagrams containing ack-eliciting packets, to avoid an
-expensive consecutive PTO expiration due to a single lost datagram.
+in the packet number space as a probe, unless there is no data available to
+send.  An endpoint MAY send up to two full-sized datagrams containing
+ack-eliciting packets, to avoid an expensive consecutive PTO expiration due
+to a single lost datagram.
+
+In addition to sending data in the packet number space for which the timer
+expired, the sender SHOULD coalesce ack-eliciting packets from all other packet
+number spaces with in-flight data if sending coalesced packets is supported.
+If implementations do not send coalesced packets upon timeout when multiple
+packet number spaces have in-flight data, then they MUST only send a single

Suggestion: "If an endpoint has data in multiple packet number spaces in flight and is unable to send coalesced packet when a PTO expires, it MUST send only a single datagram on PTO expiration."

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/3066#pullrequestreview-306299934

---

From: Martin Thomson <[email-masked]>
Date: Wed, 23 Oct 2019 20:33:29 -0700

martinthomson approved this pull request.

@janaiyengar has it right, but repeats "PTO expiration", suggest:

> If an endpoint has data in multiple packet number spaces in flight and is unable to send coalesced packet when a PTO expires, it MUST send only a single datagram.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/3066#pullrequestreview-306303081

---

From: Jana Iyengar <[email-masked]>
Date: Wed, 23 Oct 2019 22:48:59 -0700

The description should be fixed to remove "Necessary for fixing #2863". That's not true.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/3066#issuecomment-545755713

---

From: ianswett <[email-masked]>
Date: Thu, 24 Oct 2019 10:16:59 -0700

054fae193d6d9ccffdf0b20b7011a24ed33eb3b1  Update draft-ietf-quic-recovery.md

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3066/files/49a28334da5cbecebb108424ba5db0dca7840e7b..054fae193d6d9ccffdf0b20b7011a24ed33eb3b1

---

From: ianswett <[email-masked]>
Date: Thu, 24 Oct 2019 10:20:47 -0700

'all' removed, since coalescing 2 is sufficient.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/3066#discussion_r338695555

---

From: ianswett <[email-masked]>
Date: Thu, 24 Oct 2019 10:23:05 -0700

2140c28bc8398957967bc017e78974668d95a86e  Martin's suggestion

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3066/files/054fae193d6d9ccffdf0b20b7011a24ed33eb3b1..2140c28bc8398957967bc017e78974668d95a86e

---

From: ianswett <[email-masked]>
Date: Thu, 24 Oct 2019 10:23:19 -0700

Done

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/3066#discussion_r338696763

---

From: ianswett <[email-masked]>
Date: Thu, 24 Oct 2019 10:26:23 -0700

I think I've addressed all the feedback, PTAL

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/3066#issuecomment-546020117

---

From: Marten Seemann <[email-masked]>
Date: Thu, 24 Oct 2019 18:00:30 -0700

marten-seemann requested changes on this pull request.

We also need to update the `OnLossDetectionTimeout` function. If I understand this PR correctly, the code should be:

```
OnLossDetectionTimeout():
  earliest_loss_time, pn_space = GetEarliestTimeAndSpace(loss_time)
  if (earliest_loss_time != 0):
    // Time threshold loss Detection
    DetectLostPackets(pn_space)
    SetLossDetectionTimer()
    return

  if (endpoint is client without 1-RTT keys):
    // Client sends an anti-deadlock packet: Initial is padded
    // to earn more anti-amplification credit,
    // a Handshake packet proves address ownership.
    if (has Handshake keys):
      SendOneAckElicitingHandshakePacket()
    else:
      SendOneAckElicitingPaddedInitialPacket()
  else:
    // PTO. Send new data if available, else retransmit old data.
    // If neither is available, send a single PING frame.
    pn_space, _ = GetEarliestTimeAndSpace(time_of_last_sent_ack_eliciting_packet)
    SendOneOrTwoAckElicitingPackets(pn_space)

  pto_count++
  SetLossDetectionTimer()
```

> @@ -1119,7 +1125,7 @@ PeerNotAwaitingAddressValidation():
          has received 1-RTT ACK
 
 SetLossDetectionTimer():
-  loss_time, _ = GetEarliestLossTime()
+  loss_time, _ = GetEarliestTimeAndSpace(loss_time)

Using `loss_time` here is confusing (and wouldn't compile in any language).

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/3066#pullrequestreview-306934189

---

From: ianswett <[email-masked]>
Date: Fri, 25 Oct 2019 10:58:39 -0700

69afe421890c5a94de801a9dc5c8876154b441e8  Marten's comment

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3066/files/2140c28bc8398957967bc017e78974668d95a86e..69afe421890c5a94de801a9dc5c8876154b441e8

---

From: ianswett <[email-masked]>
Date: Fri, 25 Oct 2019 10:59:28 -0700

has received 1-RTT ACK
 
 SetLossDetectionTimer():

Thanks, fixed

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/3066#discussion_r339172465

---

From: ianswett <[email-masked]>
Date: Fri, 25 Oct 2019 11:06:37 -0700

1c9886e38e9d316d18688b46ed75b9eda9a6bfc8  Marten's pseudocode

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3066/files/69afe421890c5a94de801a9dc5c8876154b441e8..1c9886e38e9d316d18688b46ed75b9eda9a6bfc8

---

From: ianswett <[email-masked]>
Date: Fri, 25 Oct 2019 11:09:13 -0700

a87a04c1a6607caded4daa780f5a8554a82885ab  line wrapping

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3066/files/1c9886e38e9d316d18688b46ed75b9eda9a6bfc8..a87a04c1a6607caded4daa780f5a8554a82885ab

---

From: martinduke <[email-masked]>
Date: Fri, 25 Oct 2019 15:50:24 -0700

martinduke approved this pull request.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/3066#pullrequestreview-307484169

---

From: Marten Seemann <[email-masked]>
Date: Fri, 25 Oct 2019 16:38:39 -0700

has received 1-RTT ACK
 
 SetLossDetectionTimer():
-  loss_time, _ = GetEarliestLossTime()
+  loss_time, _ = GetEarliestTimeAndSpace(loss_times)

`loss_time` is a global variable, `loss_times` is not defined.
This should be 
```
earliest_loss_time, _ = GetEarliestTimeAndSpace(loss_time)
```

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/3066#pullrequestreview-307493358

---

From: ianswett <[email-masked]>
Date: Mon, 28 Oct 2019 11:58:39 -0700

545e788428c30217c705ecb05f686dde00681a44  Define loss_times

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3066/files/a87a04c1a6607caded4daa780f5a8554a82885ab..545e788428c30217c705ecb05f686dde00681a44

---

From: ianswett <[email-masked]>
Date: Mon, 28 Oct 2019 11:59:00 -0700

has received 1-RTT ACK
 
 SetLossDetectionTimer():

Good catch, fixed.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/3066#discussion_r339738544

---

From: David Schinazi <[email-masked]>
Date: Mon, 28 Oct 2019 12:07:23 -0700

DavidSchinazi approved this pull request.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/3066#pullrequestreview-308082921

---

From: ianswett <[email-masked]>
Date: Mon, 28 Oct 2019 12:31:12 -0700

a00c4753d038f0861677ea547c4402746f219bae  Exponential backoff limits the packets sent

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3066/files/545e788428c30217c705ecb05f686dde00681a44..a00c4753d038f0861677ea547c4402746f219bae

---

From: martinduke <[email-masked]>
Date: Mon, 28 Oct 2019 13:29:31 -0700

> @@ -477,6 +477,9 @@ limited by the endpoint's idle timeout.
 A sender computes its PTO timer every time an ack-eliciting packet is sent.
 When ack-eliciting packets are in-flight in multiple packet number spaces,
 the timer MUST be set for the packet number space with the earliest timeout.
+Even when there are ack-eliciting packets in-flight in multiple packet number
+spaces, the exponential increase in probe timeout occurs across all spaces to

This interacts weirdly with the rest of this spec if I'm not coalescing, as I'm restricted to one packet per PTO. In the common case we are PTOing the server 1st flight, we get this behavior:
where P = srtt + 4 rttvar + max_ack_delay
 
time 0: send first flight
time P: send Initial
time 2P: send Handshake
time 4P: send 1RTT
time 9P: send Initial
time 18P: send Handshake

Maybe the lesson here is to coalesce, but we should understand what we're writing here.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/3066#pullrequestreview-308127871

---

From: ianswett <[email-masked]>
Date: Mon, 28 Oct 2019 14:55:01 -0700

5e81481786d1ba1c24c4d39c4712ac5003a5cbb4  Update draft-ietf-quic-recovery.md

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3066/files/a00c4753d038f0861677ea547c4402746f219bae..5e81481786d1ba1c24c4d39c4712ac5003a5cbb4

---

From: ianswett <[email-masked]>
Date: Mon, 28 Oct 2019 14:58:21 -0700

I removed the 1 packet limit when not coalescing when pointing out this existing exponential backoff behavior, so it should be:
 time 0: send first flight
 time P: send Initial and Handshake
 time 2P: Send 1RTT and Initial
 time 4P: send Handshake and 1RTT

Hopefully before 2P the server receives an ACK of something anyway, since that's seconds into the connection.

But we may want to tweak this as a result of the discard keys conclusion.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/3066#discussion_r339812035

---

From: martinduke <[email-masked]>
Date: Mon, 28 Oct 2019 15:13:33 -0700

LGTM, though I'll have to reread the whole loss recovery draft when everything lands.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/3066#pullrequestreview-308179909

---

From: ianswett <[email-masked]>
Date: Tue, 29 Oct 2019 03:42:14 -0700

ed8750ae9d8ae6279a44dde075638ca859b40658  Don't arm PTO for ApplicationData until complete

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3066/files/5e81481786d1ba1c24c4d39c4712ac5003a5cbb4..ed8750ae9d8ae6279a44dde075638ca859b40658

---

From: ianswett <[email-masked]>
Date: Tue, 29 Oct 2019 03:46:43 -0700

6322d369a5b25b731285b131e77a76b405a6ec16  Initial and Handshake have no max_ack_delay

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3066/files/ed8750ae9d8ae6279a44dde075638ca859b40658..6322d369a5b25b731285b131e77a76b405a6ec16

---

From: ianswett <[email-masked]>
Date: Tue, 29 Oct 2019 03:51:12 -0700

I've updated the PR to no longer arm PTO for ApplicationData until the handshake is complete.  On the server side, it's pretty obvious you shouldn't arm it, since the server doesn't have receive keys to process 1-RTT ACKs.  I think this achieves what you originally might have hoped and solves the silliest case(server sending a 1-RTT PTO) above.

PTAL.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/3066#discussion_r340005858

---

From: ianswett <[email-masked]>
Date: Tue, 29 Oct 2019 11:51:57 -0700

45bf06ce64636c9ded8fb178a2bc905be372dc39  Update draft-ietf-quic-recovery.md

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3066/files/6322d369a5b25b731285b131e77a76b405a6ec16..45bf06ce64636c9ded8fb178a2bc905be372dc39

---

From: martinduke <[email-masked]>
Date: Tue, 29 Oct 2019 13:09:13 -0700

>  When a PTO timer expires, the PTO period MUST be set to twice its current
 value. This exponential reduction in the sender's rate is important because
 the PTOs might be caused by loss of packets or acknowledgements due to severe
-congestion.  The life of a connection that is experiencing consecutive PTOs is
-limited by the endpoint's idle timeout.
-
-A sender computes its PTO timer every time an ack-eliciting packet is sent. A
-sender might choose to optimize this by setting the timer fewer times if it
-knows that more ack-eliciting packets will be sent within a short period of
-time.
+congestion.  Even when there are ack-eliciting packets in-flight in multiple
+packet number spaces, the exponential increase in probe timeout occurs across
+all spaces to prevent excess load on the network. The life of a connection that

"For example, a timeout in the Initial packet number space doubles the length of the timeout in the Handshake number space."

>  When a PTO timer expires, the PTO period MUST be set to twice its current
 value. This exponential reduction in the sender's rate is important because
 the PTOs might be caused by loss of packets or acknowledgements due to severe
-congestion.  The life of a connection that is experiencing consecutive PTOs is
-limited by the endpoint's idle timeout.
-
-A sender computes its PTO timer every time an ack-eliciting packet is sent. A
-sender might choose to optimize this by setting the timer fewer times if it
-knows that more ack-eliciting packets will be sent within a short period of
-time.
+congestion.  Even when there are ack-eliciting packets in-flight in multiple
+packet number spaces, the exponential increase in probe timeout occurs across
+all spaces to prevent excess load on the network. The life of a connection that

There is also some conceptual subtlety that I suspect people could mess up.

If I'm coalescing my probes, then there is *never* a timeout at the higher levels -- each time, the junior level times out and a new high-level packet is sent that resets that level.

Misinterpreting this paragraph might mean that coalescing three probes could result in three bitshifts of the PTO.

This doesn't have to be in this PR, but when I reread this draft when everything's landed, hopefully it's in here somewhere.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/3066#pullrequestreview-308797198

---

From: ianswett <[email-masked]>
Date: Tue, 29 Oct 2019 14:34:21 -0700

34e3d6173c558fb156166a88709f81fa0211742a  Martin Duke's suggestions

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3066/files/45bf06ce64636c9ded8fb178a2bc905be372dc39..34e3d6173c558fb156166a88709f81fa0211742a

---

From: ianswett <[email-masked]>
Date: Tue, 29 Oct 2019 14:37:14 -0700

Hopefully it's clearer now.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/3066#discussion_r340337991

---

From: ianswett <[email-masked]>
Date: Tue, 29 Oct 2019 14:38:10 -0700

54e26e7a39e80a265a642a512b1a8574ea2aed21  Update draft-ietf-quic-recovery.md

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3066/files/34e3d6173c558fb156166a88709f81fa0211742a..54e26e7a39e80a265a642a512b1a8574ea2aed21

---

From: martinduke <[email-masked]>
Date: Tue, 29 Oct 2019 15:06:07 -0700

LGTM

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/3066#pullrequestreview-308863891

---

From: Martin Thomson <[email-masked]>
Date: Tue, 29 Oct 2019 16:31:39 -0700

martinthomson commented on this pull request.

 The PTO value MUST be set to at least kGranularity, to avoid the timer expiring
 immediately.
 
+A sender computes its PTO timer every time an ack-eliciting packet is sent.
+When ack-eliciting packets are in-flight in multiple packet number spaces,
+the timer MUST be set for the packet number space with the earliest timeout,
+except for ApplicationData, which MUST be ignored until the handshake
+completes (See Section 4.1.1 of {{QUIC-TLS}}).  Not arming the PTO for

How does this work with 0-RTT?  That's Application Data, but won't you want to attempt to repair on PTO?

```suggestion
completes; see Section 4.1.1 of {{QUIC-TLS}}.  Not arming the PTO for
```

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/3066#pullrequestreview-308893659

---

From: Jana Iyengar <[email-masked]>
Date: Tue, 29 Oct 2019 19:02:26 -0700

has received 1-RTT ACK
 
 SetLossDetectionTimer():

Did you forget to push your fix?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/3066#discussion_r340402316

---

From: Jana Iyengar <[email-masked]>
Date: Tue, 29 Oct 2019 19:03:47 -0700

>  : The time the most recent ack-eliciting packet was sent.
 
 largest_acked_packet\[kPacketNumberSpace]:
 : The largest packet number acknowledged in the packet number space so far.
 
-loss_time\[kPacketNumberSpace]:
+loss_times\[kPacketNumberSpace]:

Revert this change. We use singular for other arrays, which reads better to me anyways.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/3066#pullrequestreview-308929681

---

From: Junho Choi <[email-masked]>
Date: Wed, 30 Oct 2019 15:01:11 -0700

junhochoi commented on this pull request.

 value. This exponential reduction in the sender's rate is important because
 the PTOs might be caused by loss of packets or acknowledgements due to severe
-congestion.  The life of a connection that is experiencing consecutive PTOs is
-limited by the endpoint's idle timeout.
+congestion.  Even when there are ack-eliciting packets in-flight in multiple
+packet number spaces, the exponential increase in probe timeout occurs across
+all spaces to prevent excess load on the network.  For example, a timeout in
+the Initial packet number space doubles the length of the timeout in the
+Handshake packet number space.

If I understand correctly,
When Initial PTO is 10ms and Handshake PTO is 15ms:
  when Initial PTO expires, Initial PTO becomes 10ms and Handshake PTO becomes 30ms

Does it means Handshake PTO need to be re-armed at the point of Initial PTO expiration?
It may mean that Handshake PTO never be fired if Initial PTO keeps firing. There is other
text L544 saying that Initial and Handshake ack-eliciting packet should be sent at
Initial PTO expires. Then probably Handshake PTO need to be rearmed because we sent
Handshake ack-eliciting packet.
So for me it looks like now Initial PTO dominates Handshake PTO until Initial phase is complete.
Is my understanding correct?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/3066#pullrequestreview-309548624

---

From: ianswett <[email-masked]>
Date: Thu, 31 Oct 2019 11:24:14 -0700

d0e699677b5760be95b93cdb9063747cd1e42f4b  Update draft-ietf-quic-recovery.md

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3066/files/54e26e7a39e80a265a642a512b1a8574ea2aed21..d0e699677b5760be95b93cdb9063747cd1e42f4b

---

From: ianswett <[email-masked]>
Date: Thu, 31 Oct 2019 11:28:33 -0700

has received 1-RTT ACK
 
 SetLossDetectionTimer():

I fixed it by defining loss_times, but I'll revert that if you don't like it.  I prefer plurals, but I'll keep them consistent and singular for now.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/3066#discussion_r341301588

---

From: ianswett <[email-masked]>
Date: Thu, 31 Oct 2019 11:30:59 -0700

50f670169e4cae5b13065c015674b597515cda2e  loss_times -> loss_time

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3066/files/d0e699677b5760be95b93cdb9063747cd1e42f4b..50f670169e4cae5b13065c015674b597515cda2e

---

From: ianswett <[email-masked]>
Date: Thu, 31 Oct 2019 12:06:16 -0700

The Initial and Handshake timeouts should always be the same length, because neither uses the peer's max_ack_delay.  But they could be started from different points in time.  

As long as you send both at once, then the Initial PTO dominates.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/3066#discussion_r341318303

---

From: Marten Seemann <[email-masked]>
Date: Thu, 31 Oct 2019 22:10:40 -0700

> @@ -1153,8 +1174,9 @@ Pseudocode for OnLossDetectionTimeout follows:
 
 ~~~
 OnLossDetectionTimeout():
-  loss_time, pn_space = GetEarliestLossTime()
-  if (loss_time != 0):
+  earliest_loss_time, pn_space =
+    GetEarliestTimeAndSpace(loss_times)

This should be `loss_time` instead of `loss_times`. `loss_time` is a global variable.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/3066#pullrequestreview-310293855