Subject: [quicwg/base-drafts] Pseudocode fix: Data should be sent on loss-threshold timer (#2604)

From: Jana Iyengar <[email-masked]>
Date: Tue, 09 Apr 2019 23:36:12 -0700

(@larseggert : Thanks for catching this)

Currently, in OnLossDetectionTimeout(), we detect losses but do not sent anything on time threshold loss detection.  This means that SetLossDetectionTimer() that is called later in the function does not set a timer because inflight can be 0 (if all inflight is marked lost).

This is an oversight, since clearly we send rtxs when we mark packets as lost, and in the case of threshold detection, we send as much as the cwnd allows.

My proposal is to add the last two lines below
if (loss_time != 0):
    // Time threshold loss Detection
    DetectLostPackets(pn_space)
    SendOnePacketWithRetransmittedData()
    SendDataAsCwndAllows()

-- 
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/issues/2604

---

From: ianswett <[email-masked]>
Date: Wed, 10 Apr 2019 07:02:55 -0700

I don't think you need to indicate what should be sent, so I'd recommend: "SendPacketsAsCongestionControllerAllows()" since it may be pacing or CWND limited.

-- 
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/issues/2604#issuecomment-481704208

---

From: Jana Iyengar <[email-masked]>
Date: Wed, 10 Apr 2019 20:40:29 +0000 (UTC)

Yeah I thought about this... your point is taken in the PTO case, but there is a difference between the PTO case and this case. What you suggest is exactly what quicly implements right now. Without PRR however, the sender waits for half an RTT until it is able to send out the retransmission, since it waits until inflight < cwnd before sending anything.

I'm suggesting (and this is what TCP does) that we allow _one_ packet of rtx to go through before applying the cwnd clamp. Maybe this pseduocode isn't the best, but it's what I could come up with to capture that intent.

-- 
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/issues/2604#issuecomment-481853355

---

From: ianswett <[email-masked]>
Date: Wed, 10 Apr 2019 19:28:09 -0700

Ah, that makes sense, thanks for clarifying.

My first suggestion is "SendOneOrMorePacketsAsCongestionControlAllows()" which is insanely verbose.  An alternative is to say:

// Time threshold loss Detection
if (DetectLostPackets(pn_space)):
  // PRR allows immediately sending a packet
  SendOnePacket()
SendDataAsCwndAllows()

We don't reference PRR anywhere I can find in the existing recovery text.  Should we?

-- 
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/issues/2604#issuecomment-481940778

---

From: Jana Iyengar <[email-masked]>
Date: Fri, 12 Apr 2019 16:48:43 -0700

Yeah, I realized that we don't really talk about PRR. If we want to bring the entire mechanism here, that might be a bit of work. How about we do as you propose above (without PRR in the comment), add some text about how a sender can implement PRR if they want a smoother way out of recovery with a reference to the PRR draft?

Separately, is it worth filing an issue to do PRR for QUIC? Since we don't assume pacing here, it might actually be useful. It will be some work to do it though. WDYT?

-- 
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/issues/2604#issuecomment-482754972

---

From: ianswett <[email-masked]>
Date: Fri, 12 Apr 2019 16:52:02 -0700

Given where congestion control and pacing are going, I'd be inclined to not mention PRR, but if we want to mention it as another informational reference, particularly when senders aren't pacing, that SGTM.

-- 
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/issues/2604#issuecomment-482755401