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

[Neal Cardwell <[e-mail]>]

On Thu, Sep 24, 2020 at 4:15 PM Yi Huang <huanyi=
[e-mail]> 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



>
> -----Original Message-----
> From: Yuchung Cheng <[e-mail]>
> Sent: Thursday, September 24, 2020 9:30 AM
> To: Praveen Balasubramanian <pravb=[e-mail]>
> Cc: Yi Huang <[e-mail]>; [e-mail]
> Subject: Re: [tcpm] [EXTERNAL] Re: A review of draft-ietf-tcpm-rack-10
>
> On Mon, Sep 21, 2020 at 4:37 PM Praveen Balasubramanian <pravb=
> [e-mail]> wrote:
> >
> > >> 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?
> 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 :-)
>
> >
> > -----Original Message-----
> > From: tcpm <[e-mail]> On Behalf Of Yuchung Cheng
> > Sent: Monday, September 21, 2020 4:12 PM
> > To: Yi Huang <huanyi=[e-mail]>
> > Cc: Yuchung Cheng <ycheng=[e-mail]>; [e-mail]
> > Subject: Re: [tcpm] [EXTERNAL] Re: A review of draft-ietf-tcpm-rack-10
> >
> > On Mon, Sep 21, 2020 at 3:18 PM Yi Huang <huanyi=
> [e-mail]> wrote:
> > >
> > > Thanks Yuchung, I responded inline.
> > >
> > > -----Original Message-----
> > > From: tcpm <[e-mail]> On Behalf Of Yuchung Cheng
> > > Sent: Monday, September 21, 2020 2:53 PM
> > > To: Yi Huang <huanyi=[e-mail]>
> > > Cc: [e-mail]
> > > Subject: [EXTERNAL] Re: [tcpm] A review of draft-ietf-tcpm-rack-10
> > >
> > > On Sun, Sep 20, 2020 at 6:16 PM Yi Huang <huanyi=
> [e-mail]> wrote:
> > > >>
> > > >> 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?
> > > >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
> > >
> > > 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.
> >
> > 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
> >
> >
> > >
> > > >>
> > > >> 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.
> > >
> > > > 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].
> > > > ""
> > >
> > > Yeah, that's pretty clear now.
> > >
> > > > "   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]."
> > > >
> > > > 3. [NIT] In section 8, " Zero window probe (ZWP) timer" -> "Zero
> Window Probe (ZWP) timer".
> > > will fix thanks
> > > >
> > > > Thanks,
> > > >
> > > > Yi
> > > >
> > > > _______________________________________________
> > > > tcpm mailing list
> > > > [e-mail]
> > > >
> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.
> > > > 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
> > > > x43Hsfwlrog%2BZxQwJyiq0%3D&amp;reserved=0
> > >
> > > _______________________________________________
> > > tcpm mailing list
> > > [e-mail]
> > > https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.
> > > ietf.org%2Fmailman%2Flistinfo%2Ftcpm&amp;data=02%7C01%7Cpravb%40micr
> > > os
> > > oft.com%7C4cc0adb558a64fedcfac08d85e83dca9%7C72f988bf86f141af91ab2d7
> > > cd
> > > 011db47%7C1%7C0%7C637363267745119455&amp;sdata=GdXrtwnYavfLpVIYqrOTr
> > > Dx
> > > U%2FOrpC7ayXhtZSuzM%2B2U%3D&amp;reserved=0
> > >
> > > _______________________________________________
> > > tcpm mailing list
> > > [e-mail]
> > > https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.
> > > ietf.org%2Fmailman%2Flistinfo%2Ftcpm&amp;data=02%7C01%7Cpravb%40micr
> > > os
> > > oft.com%7C4cc0adb558a64fedcfac08d85e83dca9%7C72f988bf86f141af91ab2d7
> > > cd
> > > 011db47%7C1%7C0%7C637363267745119455&amp;sdata=GdXrtwnYavfLpVIYqrOTr
> > > Dx
> > > U%2FOrpC7ayXhtZSuzM%2B2U%3D&amp;reserved=0
> >
> > _______________________________________________
> > tcpm mailing list
> > [e-mail]
> > https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.
> > ietf.org%2Fmailman%2Flistinfo%2Ftcpm&amp;data=02%7C01%7Chuanyi%40micro
> > soft.com%7C5ea6a38c3c0b4fb518a908d860a7379b%7C72f988bf86f141af91ab2d7c
> > d011db47%7C1%7C0%7C637365619031782885&amp;sdata=BZRLTHuxck9GSyKvC%2BSA
> > PIPbcEUU3MS0oC7%2Bd08Idgc%3D&amp;reserved=0
> _______________________________________________
> tcpm mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/tcpm
>