Review of draft-ietf-quic-recovery-08

[Praveen Balasubramanian <[e-mail]>]

Section 2.1.4.

"This mechanism also allows a receiver to measure and report the
   delay from when a packet was received by the OS kernel, which is
   useful in receivers which may incur delays such as context-switch
   latency before a userspace QUIC receiver processes a received packet.
"
Such context switch delays are possible in a kernel implementation as well based on whether execution is deferred to worker threads. Suggest that we remove the userspace word.


Section 3.1

"Ignoring ack delay for min RTT prevents

   intentional or unintentional underestimation of min RTT, which in

   turn prevents underestimating smoothed RTT."
Does this prevent converging to the actual minimum RTT if the receiver is always delaying ACKs. I can't tell for sure if this will hurt congestion control algorithms like BBR and LEDBAT.


Section 3.2.1
It seems in general that the text does not specify the actions to take as a result of loss detection whereas the pseudocode does (for example cwnd reductions). Should we add a section on  "Congestion Control" before the pseudo code?

Section 3.3.1

"PTO SHOULD be scheduled for max(1.5*SRTT+MaxAckDelay, 10ms)"

https://datatracker.ietf.org/doc/html/draft-ietf-tcpm-rack has an explanation for the 10ms value. Should it be a goal to explain the rationale for it here or do we point the reader to the RACK draft?

Section 3.3.2

"When this alarm fires, the sender sends two packets, to

   evoke acknowledgements from the receiver, and restarts the RTO alarm.

kMinimumWindow (default 2 * kDefaultMss):  Default minimum congestion

      window."


This implies that the slow start process post loss for a QUIC flow will be twice as aggressive as TCP sharing the same bottleneck link. Is it a goal that we want to fair share with TCP? If 2 is the right value of cwnd post timeout, should TCP implementations also switch to this? Seems like 2 versus 1 is worthy of an experiment if its not been done before. Is this better tracked as a github issue?


"QUIC's RTO algorithm differs from TCP in that the firing of an RTO
   alarm is not considered a strong enough signal of packet loss, so
   does not result in an immediate change to congestion window or
   recovery state"

I found this sentence a bit misleading and seems to imply that a genuine RTO does not alter congestion window state. The pseudocode does change the cwnd to kMinimumWindow post a confirmed RTO and I think it should.



Section 3 seems to be missing "Time Based Loss Detection" section but the pseudocode uses kUsingTimeLossDetection.



Section 3.4.7.3

"It
   is part of QUIC's time based loss detection"
ER is listed in Ack-based detection in section 3.2.2.



Section 4
"QUIC's congestion control is based on TCP NewReno[RFC6582] congestion
   control to determine the congestion window and pacing rate."
Looks like pacing is addressed in the editors copy already and this sentence has been modified to not reference pacing as part of NewReno.

Section 4.6 in Editor's copy
"It is RECOMMENDED that a sender pace sending of all data"
I think we will have to mark it a MUST for safety. Since QUIC increases the congestion window by the number of  acknowledged bytes when each ack is processed and there is no ABC limit, not having pacing will cause burst losses. OR maybe we should just call out the problems by not pacing. Thoughts?

Section 4.3
"from TCP's definition of recovery ending when the lost packet that
   started recovery is acknowledged"
I saw the discussion on this in the mailing list. I think we should call out that while QUIC can as a result drop the cwnd multiple times if there is sustained loss, this definition of recovery exit primarily serves to ensure the cwnd is dropped only once per RTT. Is there any other side effect I am missing?


Section 4.6
"In order to fairly compete with flows that are
   not pacing, it is recommended to not pace the first 10 sent packets
   when exiting quiescence."
This is now not in the Editor's copy so I assume this is not required. Is there a quiescence check in gQUIC? Does Linux TCP consistently pace throughout?


Section 4.7.1.
"kDefaultMss"
I think we should just have kMss and have that used everywhere and initialize kMss to max(kDefaultMss, "PMTU discovery MSS").