Subject: [tcpm] A review of draft-ietf-tcpm-rack-10

From: Yi Huang <[email-masked]>
Date: Mon, 21 Sep 2020 01:16:20 +0000

Hi,

I reviewed the latest version of RACK-TLP draft and have a few comments/questions.

1. In section 5.1, "Segment.retransmitted is true if it was retransmitted in the most recent transmission."
There are also pseudo-code explaining how retransmitted flag is set. AFAICT, retransmitted is set if the segment is ever retransmitted since the flag is never reset. Is this variable supposed to mean ever retransmitted?

2. In section 6.2, p14,
"   If the sender has not yet observed any reordering based on the
   previous step, then RACK prioritizes quick loss recovery by using
   setting RACK.reo_wnd to 0 when the number of SACKed segments exceeds
   DupThresh, or during loss recovery.

   Aside from those special conditions, RACK starts with a conservative
   reordering window of RACK.min_RTT/4.  This value was chosen because
   Linux TCP used the same factor in its implementation to delay Early
   Retransmit [RFC5827] to reduce spurious loss detections in the
   presence of reordering, and experience showed this worked reasonably
   well [DMCG11]."

If I understand this correctly, if no reordering observed, TCP should still enter loss recovery when DupACK >= DupThresh (RFC6675). If reordering has been seen, this RFC6675 rule is not honored and TCP only enters loss recovery when RACK detects loss? I think it would be great to clarify this in the spec.

3. [NIT] In section 8, " Zero window probe (ZWP) timer" -> "Zero Window Probe (ZWP) timer".

Thanks,

Yi

---

From: Yuchung Cheng <[email-masked]>
Date: Mon, 21 Sep 2020 14:52:55 -0700

> There are also pseudo-code explaining how retransmitted flag is set. AFAICT, retransmitted is set if the segment is ever retransmitted since the flag is never reset. Is this variable supposed to me…
Thanks for catching that. Segment.retransmitted MUST be reset upon a
(re)transmission so it does not ever-retransmitted (i.e. it's only
meant for the very last transmission)

   RACK_transmit_data(Segment):
           Segment.xmit_ts = Now()
           Segment.lost = FALSE
           Segment.retransmitted = FALSE

   RACK_retransmit_data(Segment):
           RACK_transmit_data(Segment)
           Segment.retransmitted = TRUE

> If I understand this correctly, if no reordering observed, TCP should still enter loss recovery when DupACK >= DupThresh (RFC6675). If reordering has been seen, this RFC6675 rule is not honored and…

Your understanding is correct. will this text revision improves clarity"
""
If no reordering has been observed based on the previous step, the
sender enters Fast Recovery when the number of SACKed segments exceeds
DupThresh (similar to RFC6675). The RACK.reo_wnd is set to 0 both upon
entering and during the loss recovery.

Otherwise if some reordering has been observed, RACK does not honor
RFC6675 rule and starts with a conservative RACK.reo_wnd of
RACK.min_RTT/4. This .... reasonably well [DMCG11].
""

> 3. [NIT] In section 8, " Zero window probe (ZWP) timer" -> "Zero Window Probe (ZWP) timer".
will fix thanks
>
> Thanks,
>
> Yi
>
> _______________________________________________
> tcpm mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/tcpm

---

From: Yi Huang <[email-masked]>
Date: Mon, 21 Sep 2020 22:18:00 +0000

Thanks Yuchung, I responded inline.

-----Original Message-----
From: tcpm <[email-masked]> On Behalf Of Yuchung Cheng
Sent: Monday, September 21, 2020 2:53 PM
To: Yi Huang <huanyi=[email-masked]>
Cc: [email-masked]
Subject: [EXTERNAL] Re: [tcpm] A review of draft-ietf-tcpm-rack-10

On Sun, Sep 20, 2020 at 6:16 PM Yi Huang <huanyi=[email-masked]> wrote:
>Thanks for catching that. Segment.retransmitted MUST be reset upon a (re)transmission so it does not ever-retransmitted (i.e. it's only meant for the very last transmission)

>           Segment.retransmitted = TRUE

Sorry, I still don't quite get it. RACK_transmit_data is for previously unsent data (aka new data) right? If so, it will be only called once for a given segment. Then, RACK_retransmit_data only sets it to TRUE. So where do we reset it? Looks like we still just initialize it to FALSE and set it to TRUE upon retransmission.

> If no reordering has been observed based on the previous step, the sender enters Fast Recovery when the number of SACKed segments exceeds DupThresh (similar to RFC6675). The RACK.reo_wnd is set to 0 both upon entering and during the loss recovery.

> Otherwise if some reordering has been observed, RACK does not honor
RFC6675 rule and starts with a conservative RACK.reo_wnd of RACK.min_RTT/4. This .... reasonably well [DMCG11].

Yeah, that's pretty clear now.

will fix thanks
> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.
> ietf.org%2Fmailman%2Flistinfo%2Ftcpm&amp;data=02%7C01%7Chuanyi%40micro
> soft.com%7C71748af3b9ae4232346008d85e78d063%7C72f988bf86f141af91ab2d7c
> d011db47%7C1%7C0%7C637363220297828477&amp;sdata=eHYh0FXRGNS8T2LFq8szgH
> x43Hsfwlrog%2BZxQwJyiq0%3D&amp;reserved=0

_______________________________________________
tcpm mailing list
[email-masked]
https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.ietf.org%2Fmailman%2Flistinfo%2Ftcpm&amp;data=02%7C01%7Chuanyi%40microsoft.com%7C71748af3b9ae4232346008d85e78d063%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637363220297828477&amp;sdata=eHYh0FXRGNS8T2LFq8szgHx43Hsfwlrog%2BZxQwJyiq0%3D&amp;reserved=0

---

From: Yuchung Cheng <[email-masked]>
Date: Mon, 21 Sep 2020 16:12:00 -0700

> Sorry, I still don't quite get it. RACK_transmit_data is for previously unsent data (aka new data) right? If so, it will be only called once for a given segment. Then, RACK_retransmit_data only set…

Apologize for this confusion. I now see better how the text and code
confuse you. Segment.retransmitted is meant to reflect whether the
most recent send is a retransmission or not. But since a segment can
be (only) retransmitted N times consecutively, the flag is sticky
throughout once set. To avoid future confusion, how about having two
separate routines.

/* Send newly unsent data */
RACK_transmit_new_data(Segment):
           Segment.xmit_ts = Now()
           Segment.lost = FALSE
           Segment.retransmitted = FALSE

/* Retransmit previously sent data */
RACK_retransmit_data(Segment)
          Segment.xmit_ts = Now()
          Segment.lost = FALSE
          Segment.retransmitted = TRUE

---

From: Praveen Balasubramanian <[email-masked]>
Date: Mon, 21 Sep 2020 23:37:31 +0000

>> Segment.retransmitted is meant to reflect whether the most recent send is a retransmission or not
The "most recent send" here is confusing. Why not just state that this reflects whether the segment was ever retransmitted?

-----Original Message-----
From: tcpm <[email-masked]> On Behalf Of Yuchung Cheng
Sent: Monday, September 21, 2020 4:12 PM
To: Yi Huang <huanyi=[email-masked]>
Cc: Yuchung Cheng <ycheng=[email-masked]>; [email-masked]
Subject: Re: [tcpm] [EXTERNAL] Re: A review of draft-ietf-tcpm-rack-10

> Sorry, I still don't quite get it. RACK_transmit_data is for previously unsent data (aka new data) right? If so, it will be only called once for a given segment. Then, RACK_retransmit_data only set…

Apologize for this confusion. I now see better how the text and code confuse you. Segment.retransmitted is meant to reflect whether the most recent send is a retransmission or not. But since a segment can be (only) retransmitted N times consecutively, the flag is sticky throughout once set. To avoid future confusion, how about having two separate routines.

> ietf.org%2Fmailman%2Flistinfo%2Ftcpm&amp;data=02%7C01%7Cpravb%40micros
> oft.com%7C4cc0adb558a64fedcfac08d85e83dca9%7C72f988bf86f141af91ab2d7cd
> 011db47%7C1%7C0%7C637363267745119455&amp;sdata=GdXrtwnYavfLpVIYqrOTrDx
> U%2FOrpC7ayXhtZSuzM%2B2U%3D&amp;reserved=0
>
> ietf.org%2Fmailman%2Flistinfo%2Ftcpm&amp;data=02%7C01%7Cpravb%40micros
> oft.com%7C4cc0adb558a64fedcfac08d85e83dca9%7C72f988bf86f141af91ab2d7cd
> 011db47%7C1%7C0%7C637363267745119455&amp;sdata=GdXrtwnYavfLpVIYqrOTrDx
> U%2FOrpC7ayXhtZSuzM%2B2U%3D&amp;reserved=0

_______________________________________________
tcpm mailing list
[email-masked]
https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.ietf.org%2Fmailman%2Flistinfo%2Ftcpm&amp;data=02%7C01%7Cpravb%40microsoft.com%7C4cc0adb558a64fedcfac08d85e83dca9%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637363267745119455&amp;sdata=GdXrtwnYavfLpVIYqrOTrDxU%2FOrpC7ayXhtZSuzM%2B2U%3D&amp;reserved=0

---

From: Yi Huang <[email-masked]>
Date: Mon, 21 Sep 2020 23:40:34 +0000

The new pseudocode looks great and Praveen already asked what I wanted to ask. It seems to be more accurate to call it ever retransmitted.

-----Original Message-----
From: tcpm <[email-masked]> On Behalf Of Yuchung Cheng
Sent: Monday, September 21, 2020 4:12 PM
To: Yi Huang <huanyi=[email-masked]>
Cc: Yuchung Cheng <ycheng=[email-masked]>; [email-masked]
Subject: Re: [tcpm] [EXTERNAL] Re: A review of draft-ietf-tcpm-rack-10

_______________________________________________
tcpm mailing list
[email-masked]
https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.ietf.org%2Fmailman%2Flistinfo%2Ftcpm&amp;data=02%7C01%7Chuanyi%40microsoft.com%7Cd3da167125f14f08fc9f08d85e875607%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637363282644029020&amp;sdata=msvNOdkbMtyFfB5pep34eo%2BMdKiDh73hVFGX503kgjA%3D&amp;reserved=0

---

From: Yuchung Cheng <[email-masked]>
Date: Tue, 22 Sep 2020 17:36:13 -0700

Hi Yi and Praveen,

Neal and I are auditing the algorithm in full on this state variable
change. Will get back in a day or two.

---

From: Yuchung Cheng <[email-masked]>
Date: Thu, 24 Sep 2020 09:30:20 -0700

> The "most recent send" here is confusing. Why not just state that this reflects whether the segment was ever retransmitted?
We'll make the change based on your suggestions:

“Segment.retransmitted” is true if the segment has ever been
retransmitted. False otherwise.

Also RACK_detect_loss() can be further simplified by removing the
checks of Segment.retransmitted, as long as Segment.xmit_ts is
invalidated upon loss.

        “Segment.xmit_ts” is the time of the last transmission of a
data segment, including retransmissions, if any, with a clock
granularity specified in the Requirements section. A maximum value
INFINITE_TS indicates an invalid timestamp that represents that the
Segment is not currently in flight.

New RACK_detect_lost():
    timeout = 0
    RACK.reo_wnd = RACK_update_reo_wnd()

    For each segment, Segment, not acknowledged yet:
        // removed the old lines to check Segment.retransmitted
        //
        If RACK_sent_after(RACK.xmit_ts, RACK.end_seq,
                           Segment.xmit_ts, Segment.end_seq):
            remaining = Segment.xmit_ts + RACK.rtt +
                        RACK.reo_wnd - Now()
            If remaining <= 0:
                Segment.lost = TRUE
                Segment.xmit_ts = INFINITE_TS //<--- new line
            Else:
                timeout = max(remaining, timeout)
    Return timeout

So it's more clear and simpler :-)

---

From: Yi Huang <[email-masked]>
Date: Thu, 24 Sep 2020 20:14:56 +0000

Yes, this is much better. You could also change the loop condition to "For each unacknowledged and not lost segment, Segment:" and you wouldn't need INFINITE_TS, which sounds a bit more intuitive to me. Or even simpler "For each segment, Segment, not lost yet" since it is usually assumed that cumulatively ACKed segments are no longer being tracked. Anyway, the new pseudocode and text look pretty good.

-----Original Message-----
From: Yuchung Cheng <[email-masked]> 
Sent: Thursday, September 24, 2020 9:30 AM
To: Praveen Balasubramanian <pravb=[email-masked]>
Cc: Yi Huang <[email-masked]>; [email-masked]
Subject: Re: [tcpm] [EXTERNAL] Re: A review of draft-ietf-tcpm-rack-10

“Segment.retransmitted” is true if the segment has ever been retransmitted. False otherwise.

Also RACK_detect_loss() can be further simplified by removing the checks of Segment.retransmitted, as long as Segment.xmit_ts is invalidated upon loss.

        “Segment.xmit_ts” is the time of the last transmission of a data segment, including retransmissions, if any, with a clock granularity specified in the Requirements section. A maximum value INFINITE_TS indicates an invalid timestamp that represents that the Segment is not currently in flight.

New RACK_detect_lost():
    timeout = 0

> > >Thanks for catching that. Segment.retransmitted MUST be reset upon 
> > >a (re)transmission so it does not ever-retransmitted (i.e. it's 
> > >only meant for the very last transmission)
> >
> > > ietf.org%2Fmailman%2Flistinfo%2Ftcpm&amp;data=02%7C01%7Chuanyi%40m
> > > ic
> > > ro
> > > soft.com%7C71748af3b9ae4232346008d85e78d063%7C72f988bf86f141af91ab
> > > 2d
> > > 7c
> > > d011db47%7C1%7C0%7C637363220297828477&amp;sdata=eHYh0FXRGNS8T2LFq8
> > > sz
> > > gH
> soft.com%7C5ea6a38c3c0b4fb518a908d860a7379b%7C72f988bf86f141af91ab2d7c
> d011db47%7C1%7C0%7C637365619031782885&amp;sdata=BZRLTHuxck9GSyKvC%2BSA
> PIPbcEUU3MS0oC7%2Bd08Idgc%3D&amp;reserved=0

---

From: Neal Cardwell <[email-masked]>
Date: Thu, 24 Sep 2020 16:27:19 -0400

On Thu, Sep 24, 2020 at 4:15 PM Yi Huang <huanyi=
[email-masked]> wrote:

> Yes, this is much better. You could also change the loop condition to "For
> each unacknowledged and not lost segment, Segment:" and you wouldn't need
> INFINITE_TS, which sounds a bit more intuitive to me. Or even simpler "For
> each segment, Segment, not lost yet" since it is usually assumed that
> cumulatively ACKed segments are no longer being tracked. Anyway, the new
> pseudocode and text look pretty good.
>

Expressing this as "For each unacknowledged and not lost segment, Segment:"
or "For each segment, Segment, not lost yet" might be a little unclear or
ambiguous. The issue is that we do want to consider in this loop the
segments that were marked lost but then later retransmitted.  The way
Yuchung expressed the loop condition avoids that issue.

best,
neal

> > The "most recent send" here is confusing. Why not just state that this
> reflects whether the segment was ever retransmitted?
> Also RACK_detect_loss() can be further simplified by removing the checks
> of Segment.retransmitted, as long as Segment.xmit_ts is invalidated upon
> loss.
>
>         “Segment.xmit_ts” is the time of the last transmission of a data
> segment, including retransmissions, if any, with a clock granularity
> specified in the Requirements section. A maximum value INFINITE_TS
> indicates an invalid timestamp that represents that the Segment is not
> currently in flight.
>
> New RACK_detect_lost():
>     timeout = 0
> > > >> 1. In section 5.1, "Segment.retransmitted is true if it was
> retransmitted in the most recent transmission."
> > > >> There are also pseudo-code explaining how retransmitted flag is
> set. AFAICT, retransmitted is set if the segment is ever retransmitted
> since the flag is never reset. Is this variable supposed to mean ever
> retransmitted?
> > > Sorry, I still don't quite get it. RACK_transmit_data is for
> previously unsent data (aka new data) right? If so, it will be only called
> once for a given segment. Then, RACK_retransmit_data only sets it to TRUE.
> So where do we reset it? Looks like we still just initialize it to FALSE
> and set it to TRUE upon retransmission.
> >
> confuse you. Segment.retransmitted is meant to reflect whether the most
> recent send is a retransmission or not. But since a segment can be (only)
> retransmitted N times consecutively, the flag is sticky throughout once
> set. To avoid future confusion, how about having two separate routines.
> >
> > > >> If I understand this correctly, if no reordering observed, TCP
> should still enter loss recovery when DupACK >= DupThresh (RFC6675). If
> reordering has been seen, this RFC6675 rule is not honored and TCP only
> enters loss recovery when RACK detects loss? I think it would be great to
> clarify this in the spec.
> > >