Re: [quicwg/base-drafts] Incorporate F-RTO (#409)

[janaiyengar <[e-mail]>]

janaiyengar commented on this pull request.

Thanks for writing this up, Ian.  A few comments

> @@ -248,6 +248,10 @@ tlp_count:
 rto_count:
 : The number of times an rto has been sent without receiving an ack.
 
+largest_sent_before_rto:
+: The last packet number sent prior to the first transmission due to

nit: change phrase to "prior to the first retransmission timeout"

> @@ -372,6 +377,12 @@ Pseudocode for OnPacketAcked follows:
 
 ~~~
    OnPacketAcked(acked_packet_number):
+     // If a packet sent prior to RTO was acked, then the RTO
+     // was spurious.  Otherwise, inform congestion control.
+     // Similar to the goal of F-RTO {{?RFC5682}}

I would remove this last line from here and add it (now or later) to the text description.

>       if (end_of_recovery < largest_lost_packet.packet_number):
        end_of_recovery = largest_sent_packet
        congestion_window *= kLossReductionFactor
+       congestion_window = max(congestion_window, kMinimumWindow)

nit: combine this line with the previous one:
congestion_window = 
    max(congestion_window * kLossReductionFactor, kMinimumWindow)

>  
 ~~~
    TimeToSend(packet_size):
      if (bytes_in_flight + packet_size > congestion_window)
        return infinite
+     pacing_coefficient = 1.25
+     if (congestion_window < ssthresh)
+       pacing_coefficient = 2

I don't think we need a pacing coeff of 2 here. I never understood why FQ did this, but I don't think we need to have this in the doc. In general I'm less certain that we want to describe the pacing impl as part of the congestion controller... I'm comfortable with definiing the TimeToSend interface without implementing it here. For now, I would at least remove the use of pacing_coeff here.

-- 
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/pull/409#pullrequestreview-28215606