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

From: ianswett <[email-masked]>
Date: Tue, 24 Sep 2019 14:49:20 -0700

Makes PTO consistent with loss detection, which is per packet number space.

Fixes #2863
You can view, comment on, or merge this pull request online at:

  https://github.com/quicwg/base-drafts/pull/3066

-- Commit Summary --

  * Change PTO to be per packet number space

-- File Changes --

    M draft-ietf-quic-recovery.md (60)

-- Patch Links --

https://github.com/quicwg/base-drafts/pull/3066.patch
https://github.com/quicwg/base-drafts/pull/3066.diff

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

---

From: David Schinazi <[email-masked]>
Date: Tue, 24 Sep 2019 15:16:24 -0700

DavidSchinazi commented on this pull request.

> @@ -1135,8 +1150,9 @@ SetLossDetectionTimer():
       max_ack_delay
   timeout = timeout * (2 ^ pto_count)
 
-  loss_detection_timer.update(
-    time_of_last_sent_ack_eliciting_packet + timeout)
+  // Set the 

missing end of this comment?

-- 
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-292738485

---

From: ianswett <[email-masked]>
Date: Tue, 24 Sep 2019 15:21:14 -0700

@ianswett pushed 1 commit.

c5510411b24a814f432e56a6d5d0d0434f1b7516  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/f7f22f901db17f280479153698a73a93f0fe9340..c5510411b24a814f432e56a6d5d0d0434f1b7516

---

From: ianswett <[email-masked]>
Date: Tue, 24 Sep 2019 15:24:16 -0700

3abb83565f1fc9772614f118c5dccc212a45137d  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/c5510411b24a814f432e56a6d5d0d0434f1b7516..3abb83565f1fc9772614f118c5dccc212a45137d

---

From: Martin Thomson <[email-masked]>
Date: Tue, 24 Sep 2019 16:58:47 -0700

martinthomson commented on this pull request.

I think that if we're going to use this in resolving the key discard thing, then we'll need stronger support for the advice about sending in multiple spaces.

> @@ -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}},

Is `{{?RACK}}` another citation for Tail Loss Probe?

> @@ -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}},
+RTO {{?RFC5681}} and F-RTO algorithms for TCP {{?RFC5682}}, and the timeout

The commas here are odd.  I think that you could start a new sentence here:

```suggestion
RTO {{?RFC5681}}, and F-RTO  {{?RFC5682}} algorithms for TCP.  The timeout
```

Yeah, this is old text, but it's really confusing still.

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

```suggestion
If no data can be sent, then the PTO alarm MUST NOT be armed for the corresponding packet number space until
```

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

I think that we can drop the "containing ack-eliciting packets" here, it's redundant with the first sentence.

> @@ -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.
+
+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 inflight data if sending coalesced packets is supported.

This is important, so I think that it needs more support.  Questions that arise are: What happens if I don't do this?  Under what conditions would it be fine not to send in both spaces?

> @@ -933,7 +940,7 @@ loss_detection_timer:
 pto_count:
 : The number of times a PTO has been sent without receiving an ack.
 
-time_of_last_sent_ack_eliciting_packet:
+time_of_last_sent_ack_eliciting_packet[kPacketNumberSpace]:

```suggestion
time_of_last_sent_ack_eliciting_packet\[kPacketNumberSpace]:
```

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

You have two identical functions here that don't seem to do anything other than indirect over the global.  I don't get this change.

-- 
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-292766008

---

From: Kazuho Oku <[email-masked]>
Date: Tue, 24 Sep 2019 19:29:58 -0700

kazuho commented on this pull request.

Confirmation question: does this mean that we'd be permitting more datagrams to be sent in effectively one PTO, when multiple packet number spaces are involved?

Consider the case where the PTO is armed for both the Handshake packet number space and the 1-RTT packet number space. Now that we have a PTO timer for each of the packet number space, we'd see both of them firing, and the text reads that an endpoint is allowed to send two datagrams for each.

-- 
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-292800593

---

From: MikkelFJ <[email-masked]>
Date: Tue, 24 Sep 2019 23:56:56 -0700

I'd like to see the different packet number spaces operate independently because the initial handshake can use a less optimised, more complicated code path. While separate PTOs works for that, the requirement about SHOULD coalesce across packet number spaces goes against it. It requires upstream coordination and one PTO timer firing might affect the timeout of another PTO timer because coalesced packets are being sent. Overall this sounds rather complicated.

-- 
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-534882298