Subject: comments on QUIC Recovery -24.

From: G Fairhurst <[email-masked]>
Date: Thu, 14 Nov 2019 19:18:49 +0800

QUIC Recovery

I have comments on QUIC Recovery -24. These sets of come join two parts, 
this is the first. I hope most of the comments below are easy to resolve:

Section 3, Para 1 last sentence. This sentence is true from the sender 
perspective, but the monotonic increase is not necessarily true at the 
receiver.

Section 3.1.2, Para 1
/is declared is acknowledged/
- maybe would be easier to read as /is declared as acknowledged/.

Section 3.1.2, Para 1
/QUIC will do it correctly once/
- There is an inference that TCP does not, and I would argue with that, 
because TCP measures the TCP loss recovery, that is often a larger 
interval than it could have been had TCP been designed differently, but 
this document is not a thesis on TCP. I would like to suggest / QUIC 
will appropriately reduce once/ as a more neutral text.

Section 3.1.6, Para 1
/allowing a peer to maintain a more accurate/ is I think an unnecessary 
judgment on TCP again, it would be fairer to say/allowing a peer to 
maintain an accurate/

The term /host delay/ appears several times, but there is no discussion 
of hosts, only endpoints. The term host delay seems odd, can we call it 
endpoint delay or perhaps even “ACK delay” which I think is closer to 
the field used for this later in the spec?

Section 4.1, final sentence
/Ensuring that RTT estimates retain sufficient history is an open 
research question/
- This is fine for an IRTF experiment, but really rests very 
uncomfortably in a standards track specification. I suggest this alone 
is not sufficient. An alternate could be to set some bound here that at 
least mitigates this - which could otherwise result in odd pathologies.

Somewhere there is this (it's in several places):
/smoothed RTT = latest RTT/
- This needs to cite an RFC.
- I'm not a huge fan of just assigning a RTT (without taking account of 
the variance) as a starting timeout. We've in the past been caught so 
many times with things being slightly or vastly different when the next 
flow starts and simply assigning the value seems like it could be the 
wrong thing to do.

Section 4 pseudo code uses both Ack_Delay and ack_delay, I don’t 
understand the difference.

Section 5
This uses /ack/ and /ACK/
- is there a difference?

Section 5.1.
/was sent long enough in the past/
- This isn’t clear, please improve the text here to explain what is 
intended.

/sent later was delivered, while …. /
- Text should be improved. I suggest /while/ is reworked to avoid the 
inference these occur in parallel.

Section 5.1.1
There is no minimum dupack threshold specified. Why not? I could be 
persuaded that the recommendation was for 3, and that more is possible, 
but when would less than 3 fit with current other recommendations in the 
RFC series ?
- also mentioned in B.1

Also 5.4 final sentence
- section 6 (last para -1) reads like this is intended as one “probe” 
packet, XXX what does RACK do in this cased with respect to CC?

Section 5.3, final sentence:
/may never be/
- seems like it would avoid all possible ambiguity in whether this is 
allowed by using /could/.

/Sending two packets on a PTO expiration increases resilience…/
- and arguably doubles the non-congestion-controlled load.

Section 6.1
/endpoints are permitted to experiment/
- This is much more than intent of RFC8311. The intent was that the IETF 
can publish experimental specs for other behaviours. That’s not the 
same, and I think the wording needs to be modified a little to reflect 
the actual consensus of the RFC.

Section 6.2
/anytime/any time/

Section 6.9
- the new text on pacing seems to be heading in the correct direction, 
but I don’t really understand how the congestion window will be managed 
if pacing is used and the connection becomes application-limited. The 
basic TCP spec would respond aggressively reducing the window; cwv would 
loosen this; what does QUIC say when pacing *is* used.

Section 7.3
/as having being marked/ should I think be /as having being CE-marked or 
lost/
- add loss.

section A.1 para 2.
- I suggest text is added that mentions the actual reordering could be 
larger and how QUIC handles this.

Section A4 - could the initialisation be clearer? - e.g. showing initial 
values for RTT etc.

---

From: Ian Swett <[email-masked]>
Date: Thu, 14 Nov 2019 10:30:07 -0500

Thanks for the review Gorry.  I'll try to fix the editorial issues in a PR
tonight and then file issues for the non-editorial comments or comments I'm
not sure how to resolve.

---

From: Ian Swett <[email-masked]>
Date: Sat, 16 Nov 2019 06:05:29 -0500

Thanks for the review, comments below and fixes in PR #3237
<https://github.com/quicwg/base-drafts/pull/3237>

> perspective, but the monotonic increase is not necessarily true at the

Fixed.

> These were 3.1.3, not 3.1.3 FYI.  This has already been improved to
"QUIC starts a loss epoch when a packet is lost and ends one when any packet
sent after the epoch starts is acknowledged." in the editor's copy.

> will appropriately reduce once/ as a more neutral text.
 Removed "correctly"

Section 3.1.6, Para 1
> judgment on TCP again, it would be fairer to say/allowing a peer to

I disagree.  This is a new mechanism(explicit ack delay in the ACK frame)
to improve the accuracy of RTT which TCP does not have.  If it's not more
accurate, QUIC is wasting bytes sending it.

> the field used for this later in the spec?

Good point.  It seems like ACK delay is a reasonable substitute, but you
can verify in my PR.

> least mitigates this - which could otherwise result in odd pathologies.

I'm not sure how to address this, but I also don't have a problem with the
existing text.  Do you have a specific suggestion?

> - This needs to cite an RFC.

RFC6298 is cited twice in section 4 "Estimating the Round-Trip Time",
particularly in regards to smoothed RTT.  Is there another RFC you have in
mind?

> flow starts and simply assigning the value seems like it could be the

The timeout is twice the initial RTT if there are no RTT samples or the
PTO(SRTT+4*RTTVAR) if there is an RTT sample.  Which of those cases
concerns you?
I believe Issue #2789 <https://github.com/quicwg/base-drafts/issues/2789>
is open for this?

> understand the difference.

I can't find a spot in the psuedocode where Ack_Delay is used, but I do see
the use of "Ack Delay" in some text and ack_delay in others.  Are you
suggesting changes there?

> This uses /ack/ and /ACK/

ACK frame is used in some cases and acknowledgement should be used in
others.  I tried to fix the issues I found in the PR.

> Section 5.1.2 is referenced in this section and explains this in detail.
Do you have a suggested reword for this sentence?

> inference these occur in parallel.

'while' changed to 'and'.

> but when would less than 3 fit with current other recommendations in the

Is there an RFC I can cite that specifies a minimum?  5681 specifies 3, but
I can't see any discussion of minimum.

> packet, XXX what does RACK do in this cased with respect to CC?

Maybe you can quote the sentence and clarify your concerns?  I'm not sure
of the question here.

> allowed by using /could/.

It's certainly allowed.  Are you suggesting 'could never be'?

> the actual consensus of the RFC.

Do you have suggested text?

> Good call.

> loosen this; what does QUIC say when pacing *is* used.

I believe this is the relevant text:
 A sender MAY implement alternative mechanisms to update its
 congestion window after periods of under-utilization, such as those
 proposed for TCP in [RFC7661].

ie: You can reduce it, but you don't have to.

> I believe "when the packet is acknowledged" takes care of this case, since
if a packet is acknowledged it is not lost.

> larger and how QUIC handles this.

Larger than what?  1RTT?

> Section A4 - could the initialisation be clearer? - e.g. showing initial

The initial RTT never changes during a connection, since it's only used
prior to measuring the RTT, so the pseudocode uses kIntialRtt and
smoothed_rtt is initialized to 0 to indicate its unknown.

---

From: G Fairhurst <[email-masked]>
Date: Sun, 17 Nov 2019 14:24:17 +0800

I now believe these will be fixed in the next revision.

Gorry

(P.S. The timer initialisation issue was tracked down to one word and 
other edit, rather than a real issue - both now resolved in new text 
from Jana.)

On 16/11/2019, 19:05, Ian Swett wrote:
> On Thu, Nov 14, 2019 at 6:19 AM G Fairhurst <[email-masked] 
> <mailto:[email-masked]>> wrote:
>
>     QUIC Recovery
>
> I disagree.  This is a new mechanism(explicit ack delay in the ACK 
> frame) to improve the accuracy of RTT which TCP does not have.  If 
> it's not more accurate, QUIC is wasting bytes sending it.
>
>
> Good point.  It seems like ACK delay is a reasonable substitute, but 
> you can verify in my PR.
>
>
> I'm not sure how to address this, but I also don't have a problem with 
> the existing text.  Do you have a specific suggestion?
>
>
> particularly in regards to smoothed RTT.  Is there another RFC you 
> have in mind?
>
> The timeout is twice the initial RTT if there are no RTT samples or 
> the PTO(SRTT+4*RTTVAR) if there is an RTT sample.  Which of those 
> cases concerns you?
> I believe Issue #2789 
> <https://github.com/quicwg/base-drafts/issues/2789> is open for this?
>
>
> I can't find a spot in the psuedocode where Ack_Delay is used, but I 
> do see the use of "Ack Delay" in some text and ack_delay in others.  
> Are you suggesting changes there?
>
>
>     Section 5
> Section 5.1.2 is referenced in this section and explains this in 
> detail.  Do you have a suggested reword for this sentence?
>
> Is there an RFC I can cite that specifies a minimum?  5681 specifies 
> 3, but I can't see any discussion of minimum.
>
>
>     Also 5.4 final sentence
> Maybe you can quote the sentence and clarify your concerns?  I'm not 
> sure of the question here.
>
>
> I believe "when the packet is acknowledged" takes care of this case, 
> since if a packet is acknowledged it is not lost.
>
>     section A.1 para 2.
> The initial RTT never changes during a connection, since it's only 
> used prior to measuring the RTT, so the pseudocode uses kIntialRtt and