Subject: [quicwg/base-drafts] SHOULD NOT include platform delay in ack delay (#2786)

From: ianswett <[email-masked]>
Date: Wed, 12 Jun 2019 18:58:29 -0700

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

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

-- Commit Summary --

  * SHOULD NOT include platform delay in ack delay

-- File Changes --

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

-- Patch Links --

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

---

From: ianswett <[email-masked]>
Date: Wed, 12 Jun 2019 19:00:51 -0700

@ianswett pushed 1 commit.

de487d9bd0eac70150ba24f07e6741568cc2506e  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/2786/files/9084559c8960e5a90ccb61b7e71afd2d6da42497..de487d9bd0eac70150ba24f07e6741568cc2506e

---

From: ianswett <[email-masked]>
Date: Wed, 12 Jun 2019 19:01:32 -0700

87d572710ca0b0292fa8c12dfed5cd4e5135e19e  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/2786/files/de487d9bd0eac70150ba24f07e6741568cc2506e..87d572710ca0b0292fa8c12dfed5cd4e5135e19e

---

From: Nick Banks <[email-masked]>
Date: Wed, 12 Jun 2019 19:29:20 -0700

nibanks approved this pull request.

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/2786#pullrequestreview-249101345

---

From: Martin Thomson <[email-masked]>
Date: Thu, 13 Jun 2019 01:13:10 -0700

martinthomson approved this pull request.

WFM. I would have preferred to see this be explicit that the intent is to capture *intentional* delays added by the stack though.

-- 
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/2786#pullrequestreview-249189531

---

From: ianswett <[email-masked]>
Date: Thu, 13 Jun 2019 05:51:21 -0700

e6c683cf6182a9105df829597f780e80a1023d50  Intentional delay

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/2786/files/87d572710ca0b0292fa8c12dfed5cd4e5135e19e..e6c683cf6182a9105df829597f780e80a1023d50

---

From: ianswett <[email-masked]>
Date: Thu, 13 Jun 2019 05:51:49 -0700

Thanks @martinthomson I clarified that this is intentional delay.

-- 
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/2786#issuecomment-501689193

---

From: Praveen Balasubramanian <[email-masked]>
Date: Thu, 13 Jun 2019 17:24:05 -0700

pravb commented on this pull request.

I think we need more refinement. Added ACK-eliciting and remove the word host since its confusing: 

An endpoint measures the intentional delay between when a ACK-eliciting packet is received
when the corresponding acknowledgment is sent. The endpoint encodes this delay for the largest

-- 
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/2786#pullrequestreview-249664178

---

From: ianswett <[email-masked]>
Date: Thu, 13 Jun 2019 17:48:08 -0700

494ba8bbb076d860c498a3d1fb3a6da7d44e50b2  Praveen's comments

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/2786/files/e6c683cf6182a9105df829597f780e80a1023d50..494ba8bbb076d860c498a3d1fb3a6da7d44e50b2

---

From: ianswett <[email-masked]>
Date: Thu, 13 Jun 2019 17:50:10 -0700

Thanks Praveen, updated with your suggestion.  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/2786#issuecomment-501928504

---

From: Praveen Balasubramanian <[email-masked]>
Date: Thu, 13 Jun 2019 17:52:18 -0700

pravb approved this pull request.

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/2786#pullrequestreview-249669020

---

From: Kazuho Oku <[email-masked]>
Date: Thu, 13 Jun 2019 17:57:38 -0700

kazuho commented on this pull request.

> @@ -302,15 +302,15 @@ continue making forward progress.
 
 ## Measuring and Reporting Host Delay {#host-delay}
 
-An endpoint measures the delay incurred between when a packet is received and
-when the corresponding acknowledgment is sent.  The endpoint encodes this host
-delay for the largest acknowledged packet in the Ack Delay field of an ACK frame
-(see Section 19.3 of {{QUIC-TRANSPORT}}).  This allows the receiver of the ACK
-to adjust for any host delays, which is important for delayed acknowledgements,
-when estimating the path RTT.  In certain deployments, a packet might be held in
-the OS kernel or elsewhere on the host before being processed by the QUIC
-stack. Where possible, an endpoint MAY include these delays when populating the
-Ack Delay field in an ACK frame.
+An endpoint measures the intentional delay between when a ACK-eliciting packet

_an_ ACK-eliciting packet?

> -delay for the largest acknowledged packet in the Ack Delay field of an ACK frame
-(see Section 19.3 of {{QUIC-TRANSPORT}}).  This allows the receiver of the ACK
-to adjust for any host delays, which is important for delayed acknowledgements,
-when estimating the path RTT.  In certain deployments, a packet might be held in
-the OS kernel or elsewhere on the host before being processed by the QUIC
-stack. Where possible, an endpoint MAY include these delays when populating the
-Ack Delay field in an ACK frame.
+An endpoint measures the intentional delay between when a ACK-eliciting packet
+is received and the corresponding acknowledgment is sent. The endpoint encodes
+this delay for the largest acknowledged packet in the Ack Delay field of an ACK
+frame (see Section 19.3 of {{QUIC-TRANSPORT}}).  This allows the receiver of the
+ACK to adjust for any intentional delays, which is important for delayed
+acknowledgements, when estimating the path RTT.  A packet might be held in the
+OS kernel or elsewhere on the host before being processed.  An endpoint
+SHOULD NOT include these delays when populating the Ack Delay field in an ACK
+frame.

Do we need to state something like "unless the delay is controlled by the stack"?

I ask this, because I'd assume that the delay should be taken into account even if it is introduced by the kernel or elsewhere, if it is "intentional" by the stack.

-- 
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/2786#pullrequestreview-249669670

---

From: ianswett <[email-masked]>
Date: Thu, 13 Jun 2019 18:02:36 -0700

a73dc85e6866481abf4ad99e7501872f6a13a936  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/2786/files/494ba8bbb076d860c498a3d1fb3a6da7d44e50b2..a73dc85e6866481abf4ad99e7501872f6a13a936

---

From: ianswett <[email-masked]>
Date: Thu, 13 Jun 2019 18:04:37 -0700

221245719ddfbb7e762a8a955c151a4d98267f11  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/2786/files/a73dc85e6866481abf4ad99e7501872f6a13a936..221245719ddfbb7e762a8a955c151a4d98267f11

---

From: ianswett <[email-masked]>
Date: Thu, 13 Jun 2019 18:05:16 -0700

ianswett commented on this pull request.

I added the word "unintentional" to clarify your point.  Does that work?

-- 
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/2786#discussion_r293629352

---

From: Kazuho Oku <[email-masked]>
Date: Thu, 13 Jun 2019 18:06:50 -0700

Thank you! 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/2786#discussion_r293629569

---

From: Kazuho Oku <[email-masked]>
Date: Thu, 13 Jun 2019 18:06:58 -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/2786#pullrequestreview-249671565

---

From: Martin Thomson <[email-masked]>
Date: Thu, 13 Jun 2019 20:32:33 -0700

Even better now than before!

We should merge this before it starts getting worse.

 
+An endpoint measures the intentional delay between when an ACK-eliciting packet

```suggestion
An endpoint measures the delays intentionally added between when an ACK-eliciting packet
```

(This blows out the line length.)

-- 
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/2786#pullrequestreview-249695817

---

From: ianswett <[email-masked]>
Date: Fri, 14 Jun 2019 05:52:54 -0700

bcac3c84be99ae74600bec570eece332031614c1  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/2786/files/221245719ddfbb7e762a8a955c151a4d98267f11..bcac3c84be99ae74600bec570eece332031614c1

---

From: ianswett <[email-masked]>
Date: Fri, 14 Jun 2019 05:53:31 -0700

4c45a9de23c393977985f406464fc074135770be  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/2786/files/bcac3c84be99ae74600bec570eece332031614c1..4c45a9de23c393977985f406464fc074135770be

---

From: ianswett <[email-masked]>
Date: Fri, 14 Jun 2019 06:25:03 -0700

d27606ec2e13778e75d9656ac49100854a59d1ca  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/2786/files/4c45a9de23c393977985f406464fc074135770be..d27606ec2e13778e75d9656ac49100854a59d1ca

---

From: Nick Banks <[email-masked]>
Date: Fri, 14 Jun 2019 08:32:25 -0700

A related question: should the time TLS takes to process handshake packets be included in RTT or ACK delay?

-- 
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/2786#issuecomment-502155160

---

From: ianswett <[email-masked]>
Date: Fri, 14 Jun 2019 09:14:36 -0700

Our implementation includes TLS and decrypt delay in ack delay.  I think that's the right thing to do, but given it's a relatively fixed cost, I'm not sure it matters that much either way.  Queueing delays of any sort are much more concerning, since you want to include those in the path RTT.

If one was doing async handshakes, then if the delayed ack timer went off before the handshake completed, you should still send an ACK frame and not wait for the handshake to complete.

-- 
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/2786#issuecomment-502170265

---

From: Nick Banks <[email-masked]>
Date: Fri, 14 Jun 2019 09:19:00 -0700

@ianswett you think TLS and decryption delays are "intentional delays"? The general goal is to make sure queuing delays, which realistically reflect load on the system, are included in RTT measurement; not ACK delay. In Windows, TLS itself is a global process and has queues for the whole system. So the amount of time a single TLS process call depends on the current TLS load of the system. That all sounds like "unintentional delay" to me.

-- 
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/2786#issuecomment-502171586

---

From: Kazuho Oku <[email-masked]>
Date: Fri, 14 Jun 2019 09:50:34 -0700

_I_ think that theoretically, decryption delay is not intentional (i.e. something an endpoint has opted in to add) and that therefore it should not be part of ACK delay. OTOH, processing of TLS handshake is about processing the payload of a "stream." That's something happening above the packet-level processing and ACK generation layer, and therefore it should be considered as part of the ACK delay. Note also that you can offload the handshake processing to a different thread or start the processing after you send an ACK.

-- 
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/2786#issuecomment-502184521

---

From: ianswett <[email-masked]>
Date: Fri, 14 Jun 2019 13:09:26 -0700

Fair point in both cases.  Is there anything about the PR I should change, or should I merge it before it gets worse, as Martin said?

-- 
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/2786#issuecomment-502246783

---

From: Nick Banks <[email-masked]>
Date: Fri, 14 Jun 2019 13:10:01 -0700

I think you should merge 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/2786#issuecomment-502246942

---

From: ianswett <[email-masked]>
Date: Fri, 14 Jun 2019 13:34:47 -0700

Merged #2786 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/2786#event-2414775880