Re: [tcpm] WGLC for draft-ietf-tcpm-rack-08

[Mirja Kuehlewind <[e-mail]>]

Hi all,

I reviewed this document and I think content-wise this is ready to move on (just some small questions below).

I would like to see another editorial pass on the document. There are more detail comments below but I think the main points are a) provide a more detailed algorithm overview section (either in section 3 or beginning of section 7 to make it easier to understand the steps in the subsections in 7 and b) have an own main section for TLP and introduce parameters used for TLP also in section 6 and mention TLP in the intro already. It's still obvious that TLP was squeezed in later on....

Further I think the security considerations could be more extensive: it could a) mention again some of the reordering consideration in section 4 (or maybe some of the text can be moved there entirely to not break the reading flow so much at the beginning of the doc) and b) say more about the impact of this algorithm on spurious retransmission (I guess it can both increase or decrease the number of spurious retransmissions in certain scenarios) and c) mention again the impact on congestion control as described in section 8.5 that a window reduction can happen both earlier or later in different scenarios.

Here are my technical questions/comments on section 7.2:

For this sentence:
"If no reordering has been observed, RACK uses
   RACK.reo_wnd of 0 during loss recovery, in order to retransmit
   quickly, or when the number of DUPACKs exceeds the classic DUPACK
   threshold."

And the respective pseudo code here:
       If RACK.reord is FALSE:
           If in loss recovery:  /* If in fast or timeout recovery */
               RACK.reo_wnd = 0
               Return
           Else if RACK.pkts_sacked >= RACK.dupthresh:
               RACK.reo_wnd = 0
               Return

My understanding here is that the RACK-based time threshold should not be used if in recovery or if no reordering is detected, right?

Why is that realized by setting the RACK.reo_wnd to zero rather than implementing this logic in RACK_detect_loss()? I think the difference would be that you don't reset RACK.reo_wnd but keep the previous state about adjustments. But wouldn't that be good/better?

My point it that the way it is described now confused me a bit and I'm looking for a way to better describe that part...

Also why is RACK.dupthresh a separate parameter in the RACK name space instead of using the "existing" one (as you do with Packet. xmit_ts)? Further this is actually not a parameter that is adjusted by the algorithm but a configured constant. It be helpful for the reader to indicate this in the naming, e.g. call it DUPTHRESH instead. In general I'm actually not sure that using the RACK name space in this document is really helpful.

And another unrelated question but on the same part of the algorithm: where does the number 16 come from for the number of recovery cycles to keep any adaptation?

Then my more detailed (small and larger) editorial comments:

1) In the intro you use  a couple of times "we" which is maybe less common in RFCs as an RFC is a product of a wg not only of the author group. Maybe you can rephrase this.

2) also in the intro:
"The goal of RACK is to solve all the problems..."
Maybe
"The goal of RACK is to address all the problems..."

3) Section 3 provides rather a high level overview. It would be nice to get more of an overview about the algorithm itself, e.g. explaining the different step described in section 7 on a high level. Or maybe you could add alternatively more text at the beginning of section 7 explaining how the different subsections "fit together". Actually doing both would also not hurt as some redundancy where you go step by step in more details is often really helpful for the reader.

4) Section 4 I first thought that some of this text could actually go into the security considerations, however, I guess there are also some requirement here, so not sure if that could be splitted up somehow. I think the problem I have here is that is breaks quite a bit the reading flow between the overview section and the actual algorithm part. 

5) I find the naming of both RACK.reo_wnd and RACK.reo_wnd_incr a bit confusing, as RACK.reo_wnd is a time  frame (and I would expect a packet window based on the naming, given we have the congestion window or the received window) and RACK.reo_wnd_incr is a multiplier (and I would expect some value that would be added when "incr" is used)

6) 7.2:
"   Sometimes the timestamps of RACK.Packet and Packet could carry the
   same transmit timestamps due to clock granularity or segmentation
   offloading (i.e. the two packets were handed to the NIC as a single
   unit).  In that case the sequence numbers of RACK.end_seq and
   Packet.end_seq are compared to break the tie."
The textual description is not fully clear to me (pseudo code is fine). I guess this text is in the context of update RACK.Packet itself, right? Maybe that the missing part that should be spelled out.

7) I would recommend to have section 7.3. to 7.6 in their own new main section as it is an supplemental but separate algorithm. Further I would also recommend to mention Tail Loss Probe also in the abstract and intro.

8) Section 7.5: At least SRTT was already used earlier on in the doc, so maybe just add all these definitions to section 6.1?

9) Section  8.1:
" The examples above show that RACK is particularly useful when the
   sender is limited by the application, which is common for
   interactive, request/response traffic."
I think that is actually an important point to make in the introduction of the document.

10) Also section 8.2:
"   RACK can easily and optionally support the conventional approach in
   [RFC6675][RFC5681] by resetting the reordering window to zero when
   the threshold is met.  Note that this approach differs slightly from
   [RFC6675] which considers a packet lost when at least DupThresh
   higher-sequence packets are SACKed. ..."
Also this information would be really helpful in the overview section already.

11) Also on both sections 8.1. and 8.2, I find the section titles not very well suitable. I would maybe suggest to rearrange the content a bit and have a section "Examples where RACK (and TLP) is beneficial" and another section on "Implementation considerations". At least for  the examples section I would recommend to use an own subsection.

12) Maybe rename section 8.3 to "Reasoning for basic reordering window size" (instead of " Adjusting the reordering window"...?

13) Section 8.4:
" Furthermore, RACK naturally works well with Tail Loss Probe [TLP]"
This reference should be removed as content was integrated into this document. Maybe the whole paragraph should be rephrased, removed entirely, or moved into the TLP section.

14) Section 8.5.1:
" Without RACK, the sender would time out, ..."
I suggest to say "Without RACK and TLP,..." as TLP is described as a separate optional algorithm but TLP is the important bit here. Also If you decide to have a separate example section, I would suggest to move this example there as well.

15) Should section 9 stay in the final RFC? If so, I recommend to move it into the appendix. However, it would also be nice to also provide results on the number of retransmissions (have they increased with RACK and TLP?). Alternatively I suggest removing this section and point to some paper maybe?

16) I think the security considerations could be more extensive, covering impact on reordering, spurious retransmission, and congestion control, as I said at the top of this mail.

Thanks!
Mirja



On 31.03.20, 00:28, "tcpm on behalf of Michael Tuexen" <[e-mail] on behalf of [e-mail]> wrote:

    Dear all,
    
    just a quick reminder:
    
    We are currently running a WGLC for TCP RACK until Tuesday, April 7th 2020.
    
    Please send any comments, including indications to support this document,
    to the TCMP mailing list by then.
    
    The ID is available at
    https://tools.ietf.org/html/draft-ietf-tcpm-rack-08
    
    Best regards
    Michael