Subject: [quicwg/base-drafts] No RTT samples, no persistent congestion (#3889)

From: ianswett <[email-masked]>
Date: Thu, 09 Jul 2020 16:09:58 -0700

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

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

-- Commit Summary --

  * No RTT samples, no persistent congestion

-- File Changes --

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

-- Patch Links --

https://github.com/quicwg/base-drafts/pull/3889.patch
https://github.com/quicwg/base-drafts/pull/3889.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/3889

---

From: Kazuho Oku <[email-masked]>
Date: Thu, 09 Jul 2020 16:43:50 -0700

@kazuho commented on this pull request.

I'm not sure if this is correct.

The problem being discussed in #3875 is that PTO could be significantly reduced when obtaining the first RTT sample, and that persistent congestion is declared because *that RTT sample* would be used.

I think we need to do something along the lies of forbidding declaration of persistent congestion using packets sent before RTT samples were obtained, or until the RTT estimation becomes stable.

-- 
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/3889#pullrequestreview-446037828

---

From: Martin Thomson <[email-masked]>
Date: Thu, 09 Jul 2020 18:03:18 -0700

So a slight rewording *might* help, but you would have to disqualify losses associated with packets sent prior to getting an RTT estimate, as noted.  But then RTT estimates are poor to start with (the first packet to establish a NAT binding can add noticeable delays, for instance).

-- 
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/3889#issuecomment-656424075

---

From: Marten Seemann <[email-masked]>
Date: Thu, 09 Jul 2020 18:17:28 -0700

Can we just forbid persistent congestion detection for the Initial and the Handshake PN space? That would be easier to reason about than the RTT estimate you had a while ago, when you sent the first packet.

-- 
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/3889#issuecomment-656427950

---

From: Kazuho Oku <[email-masked]>
Date: Thu, 09 Jul 2020 18:36:46 -0700

@marten-seemann I'm sot sure if that would be sufficient as a fix, because 1-RTT packets are also sent before an RTT sample is obtained (e.g., 0-RTT packets, 0.5-RTT data).

-- 
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/3889#issuecomment-656432665

---

From: Marten Seemann <[email-masked]>
Date: Thu, 09 Jul 2020 18:39:15 -0700

> @marten-seemann I'm sot sure if that would be sufficient as a fix, because packets belonging to ApplicationData PN space are also sent before an RTT sample is obtained (e.g., 0-RTT packets, 0.5-RTT data).

Right, this is what #3831 is about. Maybe we should say that only **packets sent after handshake completion** should be considered for persistent congestion detection?

-- 
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/3889#issuecomment-656433344

---

From: Kazuho Oku <[email-masked]>
Date: Thu, 09 Jul 2020 18:46:53 -0700

@marten-seemann I agree that using handshake completion (or confirmation) would be better than status quo, though it is still questionable if that's good enough.

IIUC, there are two issues:
* there is no continuity between initial RTT and real RTT
* the quality of RTT samples are bad during the early stages of a connection

Using handshake completion (or confirmation) as the signal concentrates on fixing the former, but does not address the second concern. If we are to make a design change, I think it would be beneficial to spend some time looking into addressing both of the issues.

-- 
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/3889#issuecomment-656435277

---

From: Jana Iyengar <[email-masked]>
Date: Sun, 12 Jul 2020 19:31:58 -0700

Yeah, this is tricky. Can we move the discussion to the issue please?

-- 
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/3889#issuecomment-657326246

---

From: ianswett <[email-masked]>
Date: Tue, 14 Jul 2020 12:43:01 -0700

@ianswett pushed 1 commit.

414498f28aff12124a94fd5ffc5c7c2ece8105bf  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/3889/files/2d92d2c893add972496120d364e881af55c70e11..414498f28aff12124a94fd5ffc5c7c2ece8105bf

---

From: Martin Thomson <[email-masked]>
Date: Wed, 15 Jul 2020 21:06:59 -0700

Based on the discussion we've had, don't we need a more comprehensive set of changes that deal with persistent congestion, sending probe packets, and calculating RTT?

-- 
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/3889#issuecomment-659145385

---

From: Kazuho Oku <[email-masked]>
Date: Wed, 15 Jul 2020 21:51:17 -0700

@martinthomson I think that depends on the following two aspects:

First, the accuracy we want to have in the definition of persistent congestion (see https://github.com/quicwg/base-drafts/issues/3875#issuecomment-658532149). If we think of persistent congestion as a ballpark figure that resembles a blackhole period of somewhere *around* 3 PTO, during which an endpoint would have sent *at least 2 or 3 packets*, then I think the change proposed in this PR is sufficient.

Second, if your concern is about endpoints not arming PTO for ApplicationData and that becoming an edge of persistent congestion, then I'd assume that the wort case that we might want to discuss is when all the following conditions are met:
* the TLS handshake transcript send by the server does not fit in 3 datagrams (therefore the server would have RTT estimate when sending the first 0.5 RTT data)
* client spends long time in verifying the certificate chain
* both the 1-RTT packets that carrried 0.5 RTT data and the first 1-RTT data (i.e. the one containing HANDSHAKE_DONE) gets lost

Under such scenario, persistent congestion would be declared. But then, the condition matches the definition of the previous paragraph. Therefore, I think we can call this a non-issue, if we are fine with what's written in the previous paragraph.

-- 
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/3889#issuecomment-659156793

---

From: ianswett <[email-masked]>
Date: Thu, 16 Jul 2020 07:03:08 -0700

I think we might need to do something to avoid or at least caution against terrible RTT samples, but that's being discussed in #3821

-- 
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/3889#issuecomment-659433292

---

From: Jana Iyengar <[email-masked]>
Date: Thu, 16 Jul 2020 18:53:07 -0700

This is separable from #3821, and I wouldn't mix the two. I agree with @kazuho that the design issue here is about whether 3 x PTO is about time, about packets, or both. That needs consensus. This PR is good if consensus is on the issue is that PTO is (mostly) about time. We should discuss that on #3875.

-- 
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/3889#issuecomment-659783382

---

From: Kazuho Oku <[email-masked]>
Date: Fri, 17 Jul 2020 17:47:32 -0700

@kazuho 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/3889#pullrequestreview-451015887

---

From: Jana Iyengar <[email-masked]>
Date: Fri, 17 Jul 2020 18:23:09 -0700

@janaiyengar 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/3889#pullrequestreview-451019581

---

From: Marten Seemann <[email-masked]>
Date: Fri, 17 Jul 2020 18:29:10 -0700

@marten-seemann commented on this pull request.

Do we need to add some pseudo code?

-- 
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/3889#pullrequestreview-451020105

---

From: Jana Iyengar <[email-masked]>
Date: Fri, 17 Jul 2020 18:46:24 -0700

@marten-seemann : That's probably a good idea.

-- 
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/3889#issuecomment-660404501

---

From: Martin Thomson <[email-masked]>
Date: Sun, 19 Jul 2020 16:49:54 -0700

@martinthomson commented on this pull request.

> +The persistent congestion period SHOULD NOT start until there is at
+least one RTT sample, both because the length of the period is unknown and
+the PTO may be excessively conservative.

Maybe a wordier version is clearer:

"Persistent congestion SHOULD NOT consider packets that were sent prior to obtaining an RTT sample.  Correctly detecting persistent congestion can depend on packets being sent on PTO expiration.  The initial RTT, which is used to set the PTO timer, might be too conservative.  If the first RTT sample results in a much smaller RTT, the resulting persistent congestion interval might contain too few packets for their loss to be indicative of congestion."

-- 
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/3889#pullrequestreview-451174759

---

From: ianswett <[email-masked]>
Date: Mon, 20 Jul 2020 08:11:55 -0700

@ianswett commented on this pull request.

I find the first sentence potentially confusing, but I can make this more verbose along the lines of the second two sentences.

-- 
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/3889#discussion_r457478097

---

From: ianswett <[email-masked]>
Date: Mon, 20 Jul 2020 08:25:03 -0700

5a790e1484d9eef476a63248bd8b2b5a3f91cdee  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/3889/files/414498f28aff12124a94fd5ffc5c7c2ece8105bf..5a790e1484d9eef476a63248bd8b2b5a3f91cdee

---

From: ianswett <[email-masked]>
Date: Mon, 20 Jul 2020 08:25:40 -0700

eabdc5c82c92ef0077d368a58c9f02e7f16b1394  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/3889/files/5a790e1484d9eef476a63248bd8b2b5a3f91cdee..eabdc5c82c92ef0077d368a58c9f02e7f16b1394

---

From: ianswett <[email-masked]>
Date: Mon, 20 Jul 2020 09:33:33 -0700

a5cfbdcf73f306a3cd2c3dc80159d7883d2121a8  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/3889/files/eabdc5c82c92ef0077d368a58c9f02e7f16b1394..a5cfbdcf73f306a3cd2c3dc80159d7883d2121a8

---

From: ianswett <[email-masked]>
Date: Mon, 20 Jul 2020 09:34:21 -0700

I added a few lines of pseudocode and a comment.  Hopefully that's helpful.

-- 
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/3889#issuecomment-661162164

---

From: Jana Iyengar <[email-masked]>
Date: Mon, 20 Jul 2020 20:02:44 -0700

@janaiyengar commented on this pull request.

> @@ -842,6 +842,12 @@ congestion, as TCP does with a Retransmission Timeout (RTO) {{?RFC5681}}.
 The RECOMMENDED value for kPersistentCongestionThreshold is 3, which is
 approximately equivalent to two TLPs before an RTO in TCP.
 
+The persistent congestion period SHOULD NOT start until there is at
+least one RTT sample.  Prior to an RTT sample, the duration cannot be
+correctly calculated.  Waiting for one RTT sample also avoids spuriously
+declaring persistent congestion when the initial RTT is larger than the
+actual RTT, leading to an insufficient number of PTOs.

This should either be explained, or it's fine to leave it out IMO.

```suggestion
actual RTT.
```

> +     if (has prior rtt sample):
+       return

```suggestion
     if (is first rtt sample):
       return false
```

> @@ -1210,6 +1216,7 @@ OnAckReceived(ack, pn_space):
   if (newly_acked_packets.largest().packet_number ==
           ack.largest_acked &&
       IncludesAckEliciting(newly_acked_packets)):
+    has_prior_rtt_sample

```suggestion
```

-- 
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/3889#pullrequestreview-452088507

---

From: Kazuho Oku <[email-masked]>
Date: Mon, 20 Jul 2020 20:10:57 -0700

> @@ -1581,6 +1588,10 @@ Invoked when DetectAndRemoveLostPackets deems packets lost.
 
 ~~~
    InPersistentCongestion(lost_packets):

IIUC, this function is expected to check for losses across all packet number spaces. Assuming that this understanding is correct, I think we should not pass `lost_packets` as an argument (to this function or to the `AreAllPacketsLost` that is being called from this function), because `lost_packets` is a per-PN value that is obtained by calling `DetectAndRemoveLostPackets(pn_space)`.

And I think that this mitigates the concern raised in #3831.

-- 
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/3889#pullrequestreview-452095286

---

From: ianswett <[email-masked]>
Date: Tue, 21 Jul 2020 14:23:34 -0700

89e99d1428500ac413cb5197afd67631290ed09b  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/3889/files/a5cfbdcf73f306a3cd2c3dc80159d7883d2121a8..89e99d1428500ac413cb5197afd67631290ed09b

---

From: ianswett <[email-masked]>
Date: Tue, 21 Jul 2020 14:23:50 -0700

4139dce450ec3c8f9e9bf658d6245e6a11426c51  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/3889/files/89e99d1428500ac413cb5197afd67631290ed09b..4139dce450ec3c8f9e9bf658d6245e6a11426c51

---

From: ianswett <[email-masked]>
Date: Tue, 21 Jul 2020 14:27:19 -0700

~~~

Agreed, it needs to be across PN spaces.  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/3889#discussion_r458398076

---

From: ianswett <[email-masked]>
Date: Tue, 21 Jul 2020 14:27:46 -0700

1acc648093ef6782cea2bf3942a66471d170b2cd  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/3889/files/4139dce450ec3c8f9e9bf658d6245e6a11426c51..1acc648093ef6782cea2bf3942a66471d170b2cd

---

From: ianswett <[email-masked]>
Date: Tue, 21 Jul 2020 14:30:42 -0700

3ddca85d4b885e053a7312f02af4dfb49bb9d3ea  Kazuho's suggestion

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3889/files/1acc648093ef6782cea2bf3942a66471d170b2cd..3ddca85d4b885e053a7312f02af4dfb49bb9d3ea

---

From: Jana Iyengar <[email-masked]>
Date: Tue, 21 Jul 2020 16:54:14 -0700

I think this is good.

-- 
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/3889#pullrequestreview-452905725

---

From: Kazuho Oku <[email-masked]>
Date: Tue, 21 Jul 2020 16:56:57 -0700

-- 
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/3889#pullrequestreview-452906647

---

From: Martin Thomson <[email-masked]>
Date: Tue, 21 Jul 2020 17:18:49 -0700

@martinthomson approved this pull request.

It took me quite a while to confirm that "is first rtt sample" is good.  It's slightly more expansive than what I had implemented (which records 

This has the effect of including packets sent prior to getting that sample.  But as this is only packets sent within a single RTT, it's guaranteed to be less than a PTO.

> @@ -1580,14 +1586,18 @@ Invoked when an ACK frame with an ECN section is received from the peer.
 Invoked when DetectAndRemoveLostPackets deems packets lost.
 
 ~~~
-   InPersistentCongestion(lost_packets):
+   InPersistentCongestion(largest_lost):
+     // Persistent congestion cannot be declared on the
+     // first RTT sample.
+     if (is first rtt sample):

```suggestion
     if (is first RTT sample):
```

-- 
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/3889#pullrequestreview-452913580

---

From: ianswett <[email-masked]>
Date: Tue, 21 Jul 2020 17:22:04 -0700

e4601fe7238b8cc101915888ad0585a673efef16  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/3889/files/3ddca85d4b885e053a7312f02af4dfb49bb9d3ea..e4601fe7238b8cc101915888ad0585a673efef16

---

From: Marten Seemann <[email-masked]>
Date: Tue, 21 Jul 2020 18:05:55 -0700

~~~

I disagree. While strictly speaking this change is not wrong, it makes things more complex than necessary: An ACK can only ever establish loss in its own packet number space, never across packet number spaces. So it's totally fine to pass `lost_packets` to `InPersistentCongestion`.

-- 
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/3889#discussion_r458474283

---

From: Marten Seemann <[email-masked]>
Date: Tue, 21 Jul 2020 19:08:46 -0700

~~~
-   InPersistentCongestion(lost_packets):
+   InPersistentCongestion(largest_lost):
+     // Persistent congestion cannot be declared on the
+     // first RTT sample.
+     if (is first RTT sample):
+       return false

I'm not sure if that's correct. What matters is that the packet you use for the start of the persistent congestion period was sent after you already had an RTT sample.

-- 
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/3889#pullrequestreview-452946337

---

From: Kazuho Oku <[email-masked]>
Date: Tue, 21 Jul 2020 19:42:04 -0700

~~~

My point is that passing `loss_packets` in the pseudo-code gives the readers false impression that persistent congestion is something to be observed per-PN-space.

An endpoint can pass arguments whatever necessary (or beneficial), but that does not mean that those arguments should appear in pseudo-code.

-- 
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/3889#discussion_r458500226

---

From: Marten Seemann <[email-masked]>
Date: Tue, 21 Jul 2020 20:04:14 -0700

~~~

The comment now says:
> Determine if all packets in the time period before the largest newly lost packet, including the edges and across all packet number spaces, are marked lost.

My point is that this is unnecessarily complicated, as only packets within a single packet number space, namely the packet number space the ACK was received in, can be newly lost.

-- 
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/3889#discussion_r458506049

---

From: Kazuho Oku <[email-masked]>
Date: Tue, 21 Jul 2020 20:46:32 -0700

~~~

I agree with @marten-seemann here.

Consider the case where an endpoint receives the first ACK 10 seconds after sending the first packet (with an RTT of 10ms), then receives the next ACK in 20ms (with RTT of 10ms).

When the second ACK is being processed, the execution would pass through this `if`, and invoke `AreAllPacketsLost`. Because all the packets that were sent during the first few seconds are not acked, persistent congestion would be declared.

I think we can simply remove these lines, and rely on the fact that the comment in the pseudo-code talking about "edges." We can clarify what "edge" means, if necessary.

-- 
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/3889#discussion_r458516578

---

From: Kazuho Oku <[email-masked]>
Date: Tue, 21 Jul 2020 20:49:29 -0700

~~~

@marten-seemann I think you are suggesting of writing down in the spec an algorithm that can be used to detect persistent congestion, rather than just having something like `AreAllPacketsLost`. I'm not generally opposed to the idea, though I think we need to first seek what kind of implementation that we want to write down (assuming that we'd be writing down one). I think that deserves a separate issue.

-- 
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/3889#discussion_r458517302

---

From: Marten Seemann <[email-masked]>
Date: Tue, 21 Jul 2020 20:52:35 -0700

~~~

> I think we can simply remove these lines, and rely on the fact that the comment in the pseudo-code talking about "edges." We can clarify what "edge" means, if necessary.

I'm not sure if that's sufficient. I think I'd prefer to make the pseudo-code more closely resemble what you'd have to implement. Specifically, to implement this, you'll need a new global variable `time_of_first_rtt_measurement`, or alternative `first_packet_sent_with_measured_rtt`, and then you'll have to implement a comparison with the time of the packets in `lost_packets`.

-- 
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/3889#discussion_r458518103

---

From: Marten Seemann <[email-masked]>
Date: Tue, 21 Jul 2020 20:55:58 -0700

~~~

That's a separate issue, see #3937.

Independent of that, I still think that 3ddca85d4b885e053a7312f02af4dfb49bb9d3ea doesn't clarifying things, and would prefer to revert it.

-- 
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/3889#discussion_r458519008

---

From: Kazuho Oku <[email-masked]>
Date: Tue, 21 Jul 2020 21:16:58 -0700

~~~

> Specifically, to implement this, you'll need a new global variable `time_of_first_rtt_measurement`, or alternative `first_packet_sent_with_measured_rtt`, and then you'll have to implement a comparison with the time of the packets in `lost_packets`.

Can you implement that way? Persistent congestion is declared across all packet number spaces. That means that you'd have to consult if there any acks were received in the loss period observed in `lost_packets`. But you cannot remember all the moments when acks were received, because the state would be unbounded.

-- 
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/3889#discussion_r458524100

---

From: Kazuho Oku <[email-masked]>
Date: Tue, 21 Jul 2020 21:18:09 -0700

~~~

I think your argument is based on the assumption that `lost_packets` is useful, which I disagree. Please see https://github.com/quicwg/base-drafts/pull/3889#discussion_r458524100.

-- 
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/3889#discussion_r458524373

---

From: Marten Seemann <[email-masked]>
Date: Tue, 21 Jul 2020 21:45:43 -0700

~~~

After talking with @kazuho OOB, I managed to work out what our disagreement is about. Consider the following example:

If there’s an ACK that declares loss in 1-RTT space for packets sent between T=0 and T=5 (which, taken for itself would establish persistent congestion), but there was a Handshake packet acknowledged (in a separate ACK frame) sent at T=3, @kazuho wouldn't declare persistent congestion. I very much agree with that logic, and note that this would constitute a fine solution for #3831.

Pseudo-code-wise, I think we'll have to get a bit fancy here, by introducing two new global variables:
* `time_of_first_rtt_measurement`, the timestamp we were first able to establish an RTT measurement
* `largest_acked_packet_send_time`, the timestamp when the largest acknowledged packet was sent, across all packet number spaces (note that this value is only updated **after** calling `InPersistentCongestion`).

When checking for persistent congestion in `InPersistentCongestion(lost_packets)`, you'd only consider packets sent after `max(time_of_first_rtt_measurement, largest_acked_packet_send_time)`. How exactly "considering" here looks like will depend on the outcome of #3937.

-- 
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/3889#discussion_r458531174

---

From: mjoras <[email-masked]>
Date: Tue, 21 Jul 2020 22:28:12 -0700

@mjoras commented on this pull request.

 ~~~

@kazuho can you explain your reasoning around "edges" being 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/3889#discussion_r458542680

---

From: mjoras <[email-masked]>
Date: Tue, 21 Jul 2020 22:30:43 -0700

FWIW this is what mvfst (and perhaps others?) has been doing since persistent congestion was implemented, since at the time it seemed silly to calculate the window using the initial RTT. However, at the time we still had the handshake timeout and so persistent congestion did not even apply to initial and handshake packets (see #2649). 

More recently our deployment has relied on PTOs for the handshake and thus this check, and there are no obvious problems with it on the Internet. I am in favor of this change even if it is perhaps not exhaustive in its coverage of unlikely pathological cases.

-- 
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/3889#issuecomment-662250038

---

From: Kazuho Oku <[email-masked]>
Date: Tue, 21 Jul 2020 22:54:56 -0700

~~~

@mjoras If we define "edge" as a lost packet next to a packet that has been acked, having both edges means that a series of packets being lost are surrounded by acks. As there would have been RTT samples obtained for each of the surrounding acks, there is no need for a separate condition that checks if an RTT sample has been previously obtained.

-- 
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/3889#discussion_r458551057

---

From: Kazuho Oku <[email-masked]>
Date: Tue, 21 Jul 2020 22:58:17 -0700

~~~

@marten-seemann It is impossible to rely on `largest_acked_packet_send_time`, because an ACK on another PN space might have arrived later than the persistent congestion period to be declared. See the example below.

![IMG_3331](https://user-images.githubusercontent.com/41567/88139839-81b4ef00-cc2b-11ea-8c19-90abd434ed91.jpeg)

As stated in https://github.com/quicwg/base-drafts/pull/3889#discussion_r458524100, I do not think it is possible can use `loss_packets`, unless you implement some lossy behavior to bound state.

-- 
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/3889#discussion_r458552186

---

From: Marten Seemann <[email-masked]>
Date: Tue, 21 Jul 2020 23:02:52 -0700

~~~

@kazuho That makes sense to me. The algorithm I envisioned doesn't work. Not sure how one would correctly implement a `InPersistentCongestion` that yields correct results across packet number spaces then...

-- 
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/3889#discussion_r458553685

---

From: mjoras <[email-masked]>
Date: Tue, 21 Jul 2020 23:17:39 -0700

~~~

That's very cute. I would be okay with that too if we cleaned up what "edge" means and also clarify the notion of which contiguous segment should be considered for the persistent period. 

-- 
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/3889#discussion_r458558814

---

From: Kazuho Oku <[email-masked]>
Date: Wed, 22 Jul 2020 17:45:15 -0700

~~~

@marten-seemann IIUC, one trivial way of implementing `InPersistentCongestion` is as follows:
* Retain the entries in sentmap (the list of packets being sent) for at least 3 PTO.
* When receiving an acknowledgement, mark a hole in the sentmap for the packet being acked.
* When a loss is detected in a packet number space, check if that loss spans across more than 3 PTO, by traversing the sentmap. Then, if did, check the sentmap of other packet number spaces (by traversing through those sentmaps) to see if any ACK-eliciting packet sent in that period has been acked.

This approach might sound too trivial, as it is O(N) where N is the number of packets being lost (or in case there are multiple PN spaces in action, the number of inflight packets in other PN spaces). But in practice, I think this approach is sufficient in terms of performance, because N would be like 3 for the current packet number space, and because we would not be sending that many packets on Initial and Handshake packet number spaces.

-- 
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/3889#discussion_r459160599

---

From: Marten Seemann <[email-masked]>
Date: Wed, 22 Jul 2020 21:04:43 -0700

~~~
    InPersistentCongestion(lost_packets):

> When receiving an acknowledgement, mark a hole in the sentmap for the packet being acked.

I don't think that's trivial. Creating a "hole" is extra state that has to be cleaned up at some point.

-- 
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/3889#discussion_r459203592

---

From: Kazuho Oku <[email-masked]>
Date: Wed, 22 Jul 2020 21:17:10 -0700

~~~

Moving the discussion to #3939, starting from https://github.com/quicwg/base-drafts/issues/3939#issuecomment-662808337. @marten-seemann I will respond there.

-- 
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/3889#discussion_r459206088

---

From: Kazuho Oku <[email-masked]>
Date: Thu, 23 Jul 2020 19:41:09 -0700

~~~

Discussed offline with @janaiyengar, it has turned out that the approach of tweaking the definion of edges does not work well, because the edges (from current time) can be any packet, while RTT can only be established with an ack-eliciting packet.

-- 
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/3889#discussion_r459826945

---

From: Jana Iyengar <[email-masked]>
Date: Thu, 23 Jul 2020 19:59:21 -0700

My reaction is the same as @mjoras. I had to think a few times over to make sure that this was adequate, and I believe it is. It covers an egregious case explicitly, which is important.

-- 
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/3889#issuecomment-663326035

---

From: Jana Iyengar <[email-masked]>
Date: Thu, 23 Jul 2020 19:59:34 -0700

-- 
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/3889#pullrequestreview-454611457

---

From: ianswett <[email-masked]>
Date: Wed, 29 Jul 2020 06:05:04 -0700

I'm merging this so Jana can pull any conflicts into #3961 

-- 
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/3889#issuecomment-665651191

---

From: ianswett <[email-masked]>
Date: Wed, 29 Jul 2020 13:05:09 +0000 (UTC)

Merged #3889 into master.

-- 
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/3889#event-3598946629