Re: comments on QUIC Recovery -24.

[G Fairhurst <[e-mail]>]

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:
> Thanks for the review, comments below and fixes in PR #3237 
> <https://github.com/quicwg/base-drafts/pull/3237>
>
> On Thu, Nov 14, 2019 at 6:19 AM G Fairhurst <[e-mail] 
> <mailto:[e-mail]>> wrote:
>
>     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.
>
>
> Fixed.
>
>
>     Section 3.1.2, Para 1
>     /is declared is acknowledged/
>     - maybe would be easier to read as /is declared as acknowledged/.
>
> 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.
>
>     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.
>
>
>  Removed "correctly"
>
>     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/
>
>
> 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 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?
>
>
> Good point.  It seems like ACK delay is a reasonable substitute, but 
> you can verify in my PR.
>
>
>     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.
>
>
> 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?
>
>
>     Somewhere there is this (it's in several places):
>     /smoothed RTT = latest RTT/
>     - 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?
>
>     - 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.
>
>
> 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?
>
>
>     Section 4 pseudo code uses both Ack_Delay and ack_delay, I don’t
>     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?
>
>
>     Section 5
>     This uses /ack/ and /ACK/
>     - is there a difference?
>
>
> 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.
>     /was sent long enough in the past/
>     - This isn’t clear, please improve the text here to explain what is
>     intended.
>
> Section 5.1.2 is referenced in this section and explains this in 
> detail.  Do you have a suggested reword for this sentence?
>
>     /sent later was delivered, while …. /
>     - Text should be improved. I suggest /while/ is reworked to avoid the
>     inference these occur in parallel.
>
>
> 'while' changed to 'and'.
>
>
>     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
>
>
> 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
>     - 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?
>
>
> Maybe you can quote the sentence and clarify your concerns?  I'm not 
> sure of the question here.
>
>
>     Section 5.3, final sentence:
>     /may never be/
>     - seems like it would avoid all possible ambiguity in whether this is
>     allowed by using /could/.
>
>
> It's certainly allowed.  Are you suggesting 'could never be'?
>
>
>     /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.
>
>
> Do you have suggested text?
>
>
>     Section 6.2
>     /anytime/any time/
>
> Good call.
>
>     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.
>
>
> 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.
>
>
>     Section 7.3
>     /as having being marked/ should I think be /as having being
>     CE-marked or
>     lost/
>     - add loss.
>
> 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.
>     - I suggest text is added that mentions the actual reordering
>     could be
>     larger and how QUIC handles this.
>
>
> Larger than what?  1RTT?
>
>
>     Section A4 - could the initialisation be clearer? - e.g. showing
>     initial
>     values for RTT etc.
>
>
> 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.