Subject: [quicwg/base-drafts] Make in_flight the criterion for declaring loss (#2104)

From: martinduke <[email-masked]>
Date: Thu, 06 Dec 2018 12:03:42 -0800

If a packet contains just ACKs and PADDING, it is not retransmittable but must be declared lost to free up the congestion window space.
You can view, comment on, or merge this pull request online at:

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

-- Commit Summary --

  * Make in_flight the criterion for declaring loss

-- File Changes --

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

-- Patch Links --

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

---

From: ianswett <[email-masked]>
Date: Thu, 06 Dec 2018 14:46:07 -0800

ianswett commented on this pull request.

This pseudocode is also being updated in PR#2066, but you're correct that it should be in_flight, not retransmittable.

-* The packet is unacknowledged, retransmittable, and was sent prior to an
-  acknowledged packet.
+* The packet is in-flight, and was sent prior to an acknowledged packet.

Did you intend to drop "unacknowledged"?

-- 
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/2104#pullrequestreview-182484226

---

From: janaiyengar <[email-masked]>
Date: Thu, 06 Dec 2018 18:09:23 -0800

I don't think this is correct. We mark something as lost because we expected to hear an ACK back but didn't. That's true of ack-eliciting (née retransmittable) but not of things that are considered in flight.  PADDING is the reason -- it is considered in flight, but doesn't elicit an ACK, so a sender that pads ACK-only packets should add a PING frame in there to elicit an ack for this reason, so that the bytes in flight can be recovered when the packet is 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/2104#issuecomment-445098073

---

From: ianswett <[email-masked]>
Date: Thu, 06 Dec 2018 18:26:15 -0800

I think strictly speaking, neither option may be correct, unfortunately.

Packets that are in_flight count towards bytes in flight, so need to be removed from flight eventually.  And they may never get acknowledged.  The transport and recovery drafts are either a bit unclear or in conflict about whether one should ACK a PADDING only packet that arrives out of order.

The transport draft currently says "To avoid creating an indefinite feedback loop, an endpoint MUST NOT send an ACK frame in response to a packet containing only ACK or PADDING frames, even if there are packet gaps which precede the received packet."

In recovery it says "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."

I think this is an excellent reason to move the text about "Generating Acknowlegements" into transport, because having it in two places increases the chances of inconsistencies.

I tend to think that the best thing to do is take Martin's change, but also say that if an in-flight packet arrives out of order, an immediate acknowledgement should be sent, because it counts towards bytes in flight.

Again, having a packet that counts towards bytes in flight, but doesn't generate an ACK, generates a bunch of edge cases I dislike, but I think we're stuck with 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/2104#issuecomment-445100934

---

From: janaiyengar <[email-masked]>
Date: Thu, 06 Dec 2018 19:16:38 -0800

> I think this is an excellent reason to move the text about "Generating Acknowlegements" into transport, because having it in two places increases the chances of inconsistencies.

Agreed. It's on my todo for next week.

That's about what to do with acking PADDING packets. The question I'm raising is whether loss detection is tied to being in flight or to eliciting an ACK. I'm arguing that loss detection is premised on bytes counting towards in-flight, but loss detection logic triggered by receiving an ACK (which is why we use max_ack_delay in the computations).  The bytes being in flight are a necessary condition, not sufficient.

Loss detection marks a packet as lost when no ACK is received for it. This is predicated on an ACK being sent by the receiver, delayed or not, if the packet is received, which is only true for ack-eliciting packets.

PADDING of ACK-only frames is a weird special case that causes inflight to go up without being ack-eliciting. We had agreed in Montreal that if someone did this they will need to either add a PING frame in there or handle special conditions.

-- 
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/2104#issuecomment-445109347

---

From: MikkelFJ <[email-masked]>
Date: Fri, 07 Dec 2018 00:25:01 -0800

I don’t understand the idea of adding a PING to ACK/PAD only frames. If you do that you could get infinite feedback loops and you might as well just ACK all frame promptly.

-- 
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/2104#issuecomment-445158298

---

From: MikkelFJ <[email-masked]>
Date: Fri, 07 Dec 2018 02:30:04 -0800

How about a sender self-acknowledge packets that are not ack-eliciting, for example packets older than largest acked or older than triggering TLP? Once these packets are ack'ed or self-ack'ed, they are retired from the congestion window.

Or just keep the packets out of the congestion window if that is at all feasible. I'm not really keen on the above complexity if it can be avoided.

-- 
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/2104#issuecomment-445190305

---

From: ianswett <[email-masked]>
Date: Fri, 07 Dec 2018 06:26:01 -0800

You're right Jana, we've agreed that padding without a ping/etc is a pointy stick.  But I think the idea was that as long as you sent an ack-eliciting frame(ping or otherwise) at least once per RTT, you'd get an ACK, and then you could do loss detection on the in flight packets.

If we leave the text as is, if a padding only packet is never ACKed, there is no mechanism for removing it from in flight, because that occurs only in OnPacketAckedCC and OnPacketsLost.

If we don't make any changes, it's really unsafe to send padding-only packets, and we're forcing implementers to figure out something to do here.  I'd rather recommend something that does not cause a deadlock, even if it may declare a few padding only packets as lost in cases when they were actually reordered.

-- 
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/2104#issuecomment-445248125

---

From: Nick Banks <[email-masked]>
Date: Fri, 07 Dec 2018 15:10:16 +0000 (UTC)

I feel like this is somewhat related to my issue (#2015) related to calling `OnPacketsLost` for all lost packets, not just retransmittable/ack-eliciting ones. If you know the packet was lost, then process it as such. Now, what you do it response to it being lost is where you would then put the `in_flight` check for removing the `bytes_in`flight` and whatever other logic.

-- 
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/2104#issuecomment-445261559

---

From: ianswett <[email-masked]>
Date: Fri, 07 Dec 2018 07:55:14 -0800

@ianswett pushed 1 commit.

7ce9746886d1b3d5b638f8bc24dc8defc1891f3c  Merge branch 'master' into patch-31

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/2104/files/46486dc1191b0ca0f08203597dde6d419e466bf0..7ce9746886d1b3d5b638f8bc24dc8defc1891f3c

---

From: ianswett <[email-masked]>
Date: Fri, 07 Dec 2018 15:55:56 +0000 (UTC)

b290a95ceecc5df8ef291211502af941bbbf045b  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/2104/files/7ce9746886d1b3d5b638f8bc24dc8defc1891f3c..b290a95ceecc5df8ef291211502af941bbbf045b

---

From: ianswett <[email-masked]>
Date: Fri, 07 Dec 2018 15:57:11 +0000 (UTC)

Hi Martin, I resolved the conflicts with the recently merged Ack-eliciting change for you.  Feel free to make further modifications.

If we decide to change this, we should ensure the text is clear as well and offers an explanation for the outcome of this discussion.

-- 
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/2104#issuecomment-445276417

---

From: janaiyengar <[email-masked]>
Date: Tue, 18 Dec 2018 15:50:32 -0800

I wanted to ensure that we don't set the timer based on in-flight. This PR doesn't do that... and this PR is correct. I'll merge.

-- 
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/2104#issuecomment-448415603

---

From: janaiyengar <[email-masked]>
Date: Tue, 18 Dec 2018 15:50:42 -0800

Merged #2104 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/2104#event-2034405286