Subject: Re: [quicwg/base-drafts] Generating Acknowledgements (#1139)

From: Subodh Iyengar <[email-masked]>
Date: Fri, 02 Mar 2018 17:23:47 +0000 (UTC)

siyengar commented on this pull request.

> +non-ack frames.  Specifically, implementaions MUST attempt to
+enforce a maximum ack delay to avoid causing the peer spurious
+timeouts.  The default maximum ack delay in QUIC is 25ms.
+
+An acknowledgement MAY be sent for every second full-sized segment,
+as TCP does {{!RFC5681}}, or may be sent less frequently, as long as
+the delay does not exceed the maximum ack delay.  QUIC recovery algorithms
+do not assume the peer generates an acknowledgement immediately when
+receiving a second full-sized segment.
+
+Out-of-order packets SHOULD be acknowledged more quickly, in order
+to accelerate loss recovery.  Specifically, the receiver SHOULD send an
+immediate ACK when it receieves a packet with a packet number more than
+1 larger than the previous largest received packet number. The receiver
+SHOULD also send an immediate ACK when it receives a packet that fills
+in a gap in the packet numbers.  If no packet larger than the packet that

might i suggest a different wording.

The receiver SHOULD also send an immediate ACK when it receives a packet that is not the next expected packet number.

This is how we implement this.

-- 
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/1139#pullrequestreview-100849003

---

From: Subodh Iyengar <[email-masked]>
Date: Fri, 02 Mar 2018 18:21:10 +0000 (UTC)

i.e. i think this covers the case of a gap being filled.

-- 
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/1139#discussion_r171923302

---

From: ianswett <[email-masked]>
Date: Fri, 02 Mar 2018 12:04:26 -0800

@ianswett pushed 1 commit.

e5665c5  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/1139/files/204743e56628b745b840c0c59cf6bbb4f4fc81a7..e5665c5560e3066b0766b4c5ebd31ca00c4ce679

---

From: ianswett <[email-masked]>
Date: Fri, 02 Mar 2018 12:04:43 -0800

ianswett commented on this pull request.

Thanks, I tried to address all comments, PTAL.

Agreed that's a good algorithm.  Updated and I tried to reword the bulk receiving text to be clearer.  Suggestions welcome.

> @@ -437,33 +437,44 @@ do not assume the peer generates an acknowledgement immediately when
 receiving a second full-sized segment.
 
 Out-of-order packets SHOULD be acknowledged more quickly, in order
-to accelerate loss recovery.  The receiver SHOULD send an immediate
-ACK when it receives a packet that fills in a gap in the packet numbers
-if it has already acknowledged a larger packet number.  If it has not
-sent an acknowledgement for the larger packet and there are no other
-recent gaps, then it may revert to the standard delayed ack algorithm.
+to accelerate loss recovery.  Specifically, the receiver SHOULD send an
+immediate ACK when it receieves a packet with a packet number more than
+1 larger than the previous largest received packet number. The receiver
+SHOULD also send an immediate ACK when it receives a packet that fills
+in a gap in the packet numbers.  If no packet larger than the packet that
+fills a gap has been acknowledged, then the peer will not invoke fast
+recovery and it may use the standard delayed ack algorithm.  This
+circumstance may arise from delaying ack transmission when
+processing multiple packets.

I completely rewrote this because it was confusing.  Can you take a look at the new text?

 When a packet containing an ACK frame is sent, the largest acked in that
-frame is saved.  When a packet containing an ACK frame is acknowledged,
-the receiver stops acknowledging packets less than or equal to the largest
-acked in the sent ACK frame.  This approach is not sensitive to packet loss
-and allows for over 1 round trip of reordering.
+frame may be saved.  When a packet containing an ACK frame is acknowledged,
+the receiver can stop acknowledging packets less than or equal to the
+largest acked in the sent ACK frame.
+
+This approach does not guarantee every acknowledgement is seen by the
+sender before it is no longer included in the ack frame, because packets
+could be received out of order and all subsequent ACK frames containing them
+could be lost.  It does guarantee the sender is making forward progress and

I added this and then completely reworked the paragraph.  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/1139#pullrequestreview-100893809

---

From: MikkelFJ <[email-masked]>
Date: Fri, 02 Mar 2018 20:20:35 +0000 (UTC)

mikkelfj commented on this pull request.

> @@ -422,6 +422,58 @@ is available.
 (TODO: Work this section some more. Add text on client vs. server, and on
 stateless retry.)
 
+## Generating Acknowledgements
+
+QUIC SHOULD delay sending acknowledgements in response to packets,
+but MUST NOT excessively delay acknowledgements of packets containing
+non-ack frames.  Specifically, implementaions MUST attempt to
+enforce a maximum ack delay to avoid causing the peer spurious
+timeouts.  The default maximum ack delay in QUIC is 25ms.
+
+An acknowledgement MAY be sent for every second full-sized segment,
+as TCP does {{!RFC5681}}, or may be sent less frequently, as long as
+the delay does not exceed the maximum ack delay.  QUIC recovery algorithms

or it may ...
The segment terminology is a bit foreign if you are not up to speed on TCP.

-- 
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/1139#pullrequestreview-100901423

---

From: Mike Bishop <[email-masked]>
Date: Fri, 02 Mar 2018 20:58:57 +0000 (UTC)

MikeBishop commented on this pull request.

+non-ack frames.  Specifically, implementaions MUST attempt to
+enforce a maximum ack delay to avoid causing the peer spurious
+timeouts.  The default maximum ack delay in QUIC is 25ms.
+
+An acknowledgement MAY be sent for every second full-sized segment,
+as TCP does {{!RFC5681}}, or may be sent less frequently, as long as
+the delay does not exceed the maximum ack delay.  QUIC recovery algorithms
+do not assume the peer generates an acknowledgement immediately when
+receiving a second full-sized segment.
+
+Out-of-order packets SHOULD be acknowledged more quickly, in order
+to accelerate loss recovery.  The receiver SHOULD send an immediate ACK
+when it receives a new packet which is not one greater than the
+largest received packet number.  If a receiver processes multiple
+packets before sending any ACK frames in response, they MAY not 

I think trailing whitespace here is your build failure culprit.

-- 
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/1139#pullrequestreview-100911143

---

From: ianswett <[email-masked]>
Date: Fri, 02 Mar 2018 18:09:15 -0800

Good point, I based this text on a TCP RFC.

-- 
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/1139#discussion_r172002232

---

From: ianswett <[email-masked]>
Date: Sat, 03 Mar 2018 02:10:43 +0000 (UTC)

791aa42  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/1139/files/e5665c5560e3066b0766b4c5ebd31ca00c4ce679..791aa42a5296aee78b2b2410d67a235634dcc8a9

---

From: Martin Thomson <[email-masked]>
Date: Sun, 04 Mar 2018 23:24:14 +0000 (UTC)

martinthomson approved this pull request.

Take a look at #1153, but I will approve this anyway.

-- 
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/1139#pullrequestreview-101024591

---

From: ianswett <[email-masked]>
Date: Mon, 05 Mar 2018 14:17:37 +0000 (UTC)

20c2e47  Martin's editorial changes from #1153

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/1139/files/791aa42a5296aee78b2b2410d67a235634dcc8a9..20c2e478401aa87d090969bf853e5f8a6d2990a4

---

From: ianswett <[email-masked]>
Date: Mon, 05 Mar 2018 14:19:06 +0000 (UTC)

Thanks Martin, I'm going to land this with the editorial part of your PR and then we can discuss the proposed text change you made.

-- 
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/1139#issuecomment-370432914

---

From: ianswett <[email-masked]>
Date: Mon, 05 Mar 2018 14:19:19 +0000 (UTC)

Merged #1139.

-- 
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/1139#event-1504201330

---

From: Subodh Iyengar <[email-masked]>
Date: Mon, 05 Mar 2018 21:42:03 +0000 (UTC)

the changes look great

-- 
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/1139#discussion_r172338075