Subject: [QUIC] Loss recovery description

From: Martin Thomson <[email-masked]>
Date: Wed, 2 Nov 2016 15:21:20 +1100

On 2 November 2016 at 11:24, Jana Iyengar <[email-masked]> wrote:
> draft-iyengar-quic-loss-recovery-01:
> Based on list discussion, we've focussed on loss *detection* logic and
> steered clear of retransmission logic (though I realized that we still left
> a number of references to "QUIC loss recovery" in the text, which we'll
> change to "QUIC loss detection" in the next rev.)

I find the description quite baffling (though it's probably an
improvement over the previous version).  I'm guessing that this is
just because pieces are missing, but here's a taste:

loss_detection_alarm is apparently multi-modal, but the mode isn't
set, only the time is.  Is this just a single timer with varying
timeouts?

loss_detection_alarm has a bunch of functions (.reset(), .cancel(),
.set(), .get_delay()), of which I am left guessing as to their
intention (mainly how do .cancel() and .reset() differ).

What is delayed_ack_timer?  rttvar?

What does it mean to have retransmittable packets outstanding?  is
this unacknowledged, or in a buffer awaiting transmission?  It seems
like the handshake packets being outstanding is unacknowledged, but it
shortcuts processing and cancels the timer if retransmittable packets
are outstanding  (aren't handshake packets retransmittable?)

There are a whole bunch of constants, all of which are unexplained.
I'm looking for some "why" on every single constant.

What is smoothed_rtt set to initially?

Formatting-wise, you need to decide if there are semi-colons or not,
and whether you intend to treat variables as locals or globals.
Section 3.6 seems to treat lost_packets as a local, but neglects to
pass it anywhere.

Is the alarm that fires in Section 3.6 (OnAlarm(acked_packet)) the one
that was set in Section 3.3 (SetLossDetectionAlarm)?  Where does
acked_packet come from?

---

From: Jana Iyengar <[email-masked]>
Date: Wed, 2 Nov 2016 00:41:33 -0700

Hi Martin,

Thanks for your quick feedback. More inline:

> just because pieces are missing, but here's a taste:

Understanding the algorithm, as it's currently specified, assumes more
knowledge of TCP loss recovery mechanisms than it should. So, point taken
-- it's certainly not complete (pieces are missing indeed, as you note
below), but the progress was in adding algorithmic substance to the draft.
There's more work to be done in making it complete, so everything you point
out below is useful.

To be clear, we simplified and added the entire loss detection algorithm
(which was largely missing in -00), but admittedly there's scaffolding
missing. The draft needs significant work. FWIW, the algorithmic core is a
complete re-do of the TCP loss recovery zoo, and our goal was to get the
core novel mechanisms out before Seoul. I'll answer your more specific
questions below.

> set, only the time is.  Is this just a single timer with varying

Yes. This is worth clarifying though, so we'll do that in the next rev.

> intention (mainly how do .cancel() and .reset() differ).

Their intention is roughly what you'd guess, but yes, this was definitely
left out (to be fixed.) reset() and cancel() should be the same, we should
probably just use reset() to clear a pending alarm.

> What is delayed_ack_timer?  rttvar?

delayed_ack_timer is the standard delayed ack algorithm as specified in RFC
1122 and 5681. rttvar is as specified in RFC 6298. These should both be
documented separately for QUIC, and they probably belong in this document.

> are outstanding  (aren't handshake packets retransmittable?)

"outstanding" is used in TCP literature to mean unacknowledged data (see
RFC 5681.)  There's an unfortunate mistake in the first line of
SetLossDetectionAlarm -- the predicate should be "if (no retransmittable
packets are outstanding)", which might alleviate some confusion.

Retransmittable frames are defined above in Section 2: "Retransmittable
frames are frames requiring reliable delivery. The most common are STREAM
frames, which typically contain application data." Retransmittable packets
are packets containing one ore more retransmittable frames. This should be
clarified as well.

Handshake packets are retransmittable, since handshake data is sent in
STREAM frames. Again, should be clarified.

(That said, my script to convert a Google Doc to an XML file seems to have
failed badly. Comments in the pseudo code have gone missing. WillFix.)

> I'm looking for some "why" on every single constant.

Fair enough. For clarification the constants come from TCP loss recovery
mechanisms (RFCs 6298 for RTO constants, 5681 for reordering threshold,
draft-dukkipati-tcpm-tcp-loss-probe for TLP constants.)

> What is smoothed_rtt set to initially?

smoothed_rtt should be part of a description of the RTT computation
algorithm (basically RFC 6298), which is currently mentioned in Section 5.1
(as SRTT).

The globals are listed in Section 3.1, and the rest should be locals.

> Section 3.6 seems to treat lost_packets as a local, but neglects to

Yes, this was an oversight. OnAlarm() is an asynchronous call, and doesn't
return anything. This should
change MaybeRetransmitLostPackets(lost_packets)

(We should also change MaybeRetransmitLostPackets() to
NotifyConnectionOfLostPackets() or some such.)

> that was set in Section 3.3 (SetLossDetectionAlarm)?  Where does

Yes.

> acked_packet come from?

OnAlarm should not have an acked_packet as a parameter. We'll rewrite
OnAlarm() and DetectLostPackets() to be correct.

---

From: Martin Thomson <[email-masked]>
Date: Thu, 3 Nov 2016 02:10:24 +1100

Thanks Jana, I think that I got the gist, but found that I was
guessing too much.  I'm certainly short on my encyclopedic knowledge
of the TCP RFCs.

> The globals are listed in Section 3.1, and the rest should be locals.

The one that stands out is lost_packets.

Also time_loss isn't used [-Wunused]

---

From: Ian Swett <[email-masked]>
Date: Wed, 2 Nov 2016 12:41:50 -0400

On Wed, Nov 2, 2016 at 11:10 AM, Martin Thomson <[email-masked]>
wrote:

> There should be a "if (time_loss)" line directly after the foreach loop in
DetectLostPackets.  It was inadvertently deleted at some point, now fixed
in the master doc.

---

From: Jana Iyengar <[email-masked]>
Date: Wed, 2 Nov 2016 10:21:02 -0700

> guessing too much.  I'm certainly short on my encyclopedic knowledge

Yeah, that's a bug in the current draft, not a feature. Thank you again for
your read and your review -- I'd definitely appreciate another review after
we've had a chance to fill it out some more to make it more self-contained.