Re: [tcpm] Review of draft-ietf-tcpm-rack-07

[Yuchung Cheng <[e-mail]>]

Hi Ilpo,

Thanks for a detailed review and the great suggestions. Here's all the
edits we made.
https://www.ietf.org/rfcdiff?url2=draft-ietf-tcpm-rack-08

 We defined the missing terms and addressed the nits, removed the less
critical parts like recommending PRR, and edited TLP algorithms (sections
7.4 to 7.6). I hope you find it's better now and would be happy to hear any
improvements you'd like to make.

Yuchung

On Fri, Feb 14, 2020 at 4:22 AM Ilpo Järvinen <[e-mail]>
wrote:

> In general, I find the draft quite good shape already, it's quite easy
> to read and follow (that is, except the TLP sections that are specifically
> mentioned below).
>
> Here's the list of issues I came across:
>
> Sect 6.1
>
> "RACK.pkts_sacked" lacks the empty line separator.
>
> Sect 7.2
>
>   "This
>    heuristic would fail to update RACK stats if the packet is spuriously
>    retransmitted because of a recent minimum RTT decrease (e.g. path
>    change)."
> This defies my logic as RTT decrease should not result in spurious
> retransmissions but help to avoid them.
>
>
>   "Note that
>    extensions that require additional TCP features (e.g.  DSACK) would
>    work if the feature functions simply return false."
> Please rephrase this. I suspect you meant that if some functions relate
> to TCP features that are not available, the given code still works
> as long as the functions related to such features return false when
> the TCP feature is not available but that surely isn't what the
> sentence currently says.
>
> Sect 7.2 Step 5
>
>   "For timely loss detection, the sender MAY
>    install a "reordering settling" timer set to fire at the earliest
>    moment at which it is safe to conclude that some packet is lost.  The
>    earliest moment is the time it takes to expire the reordering window
>    of the earliest unacked packet in flight."
> vs
>    RACK_detect_loss():
>       ...
>           timeout = max(remaining, timeout)
> Which way it is, "some" or "all"? The use of max() in RACK_detect_lost()
> seems to indicate that the timeout will be set according to the longest
> time, that is, the latest moment rather than earliest moments. Which
> is the intented behavior here? (I can there might be merits in both
> approaches).
>
>
> In general, I didn't like the presentation order here as this form looks
> the most obvious to me:
>    now >= Packet.xmit_ts + RACK.reo_wnd + (RACK.ack_ts - RACK.xmit_ts)
> ...and I find the derivation of it just complicating things rather
> than helping but that's perhaps just my opinion.
>
>
>    "It is also useful when the timestamp clock granularity is close to
>     or longer than the actual round trip time."
> Given that some things based on timestamps will obviously stop working
> if the clock is slower than one tick per RTT I found this fragment a bit
> odd. RFC 7323 says "the clock SHOULD tick at least once per window's worth
> of data". I can understand though that a host might find clock tick
> granularity challenging due to extensive RTT range but on the same
> I'd not want such a "too slow clock" behavior to be endorsed as if
> it would be just normal. That is, I'm not against RACK handling
> also this case nicely but it would be nice to somehow downplay the
> normalness tone in it with something along the lines of "This check is
> also useful if the recommended one tick per RTT cannot be achieved due
> to timestamp clock granularity."
>
> Is RACK_detect_lost() supposed to detect also lost rexmits? How?
> The rexmit code itself is not in the draft so it's hard to evaluate
> how the Packet booleans are supposed to work in the first place.
> I'm not saying the rexmit code should be included but currently it
> surely looks like any rexmitted packet just remains .lost = TRUE and
> .retransmitted = TRUE state until RTO?
>
>
> Sect 7.4
>
> "PTO" just appears out of nowhere (and is not even written open
> in the first instance).
>
>    "2. ... in step (2) of the algorithm."
> "step (2)" is forward-refing, it would naturally refer back to
> "Step 2: Update RACK stats" so it makes very little sense as is.
>
> Sect 7.5
>
> Separator missing for PTO: and TLPRxtOut:
>
> Sect 7.5.1
>
>   "3.  Upon receiving a SACK that selectively acknowledges data that was
>        last sent before the segment with SEG.SEQ=SND.UNA was last
>        (re)transmitted"
> This is hard to understand, perhaps rephrase it somehow?
>
>
>   "But choosing PTO to be
>    exactly an SRTT is likely to generate spurious probes given that
>    network delay variance and even end-system timings can easily push an
>    ACK to be above an SRTT.  We chose PTO to be the next integral
>    multiple of SRTT.
>
>    Similarly, network delay variations and end-system processing
>    latencies and timer granularities can easily delay ACKs beyond
>    2*SRTT, so senders SHOULD add at least 2ms to a computed PTO value
>    (and MAY add more if the sending host OS timer granularity is more
>    coarse than 1ms)."
> Does this later paragraph depend on some assumptions (such as very
> low RTT?) as I don't otherwise understand why you say what you say
> in the given order. First you said x & y can push ACK over SRTT so
> we pick 2* but then you say the same things can push over 2*SRTT
> too?
>
> WCDelAckT to terminology?
>
> Sect 7.5.2
>
> TLP_send_probe():
>         - Perhaps set TLPRxtOut somewhere?
>         - Add "Arm RTO" to the end?
>
>   "In the case where
>    there is only one original outstanding segment of data (N=1), the
>    same logic (trivially) applies: an ACK for a single outstanding
>    segment tells the sender the N-1=0 segments preceding that segment
>    were lost.  Furthermore, whether there are N>1 or N=1 outstanding
>    segments,"
> If this distinction is useful to make at all, maybe just simply say:
> "Applies also in case N=1."
>
> Sect 7.6
>
>   "SND.NXT at the time the episode started."
> Isn't this TLPHighRxt, why not use it here?
>
> Sect 7.6.2
>
>   "Upon sending a TLP probe that is a retransmission, the sender sets
>    TLPRxtOut to true and TLPHighRxt to SND.NXT."
> This should appear in TLP_send_probe().
>
> "Detecting recoveries accomplished by loss probes"
> Did you want to start another section here?
>
> "Step 1: Track ACKs ..." until what point? (It's told on later but
> this question comes into mind when reading the draft in order).
>
>
> Sect 7.4 - 7.6.2
>
> In general, this particular part of the draft seems to require some
> further editing. After reading it all through, I feel most bits are
> present but the text is very hard to follow. I think the presentation
> order needs significant work. There are random bits here and there
> that are explained only later. Also, algorithm lacks some parts
> discussed only in the text but variable like items should definitely
> be included to the algorithm.
>
>
> Sect 8.4
>
>   "RACK is compatible with and does not interfere with the the standard
>    RTO [RFC6298], RTO-restart [RFC7765], F-RTO [RFC5682] and Eifel
>    algorithms [RFC3522]. ...
>    It neither changes the RTO timer calculation nor detects spurious
>    timeouts."
> For F-RTO, also send order is significant.
>
> Sect 8.5
>
> SHOULDs PRR (and also placed to normative references) that is only
> experimental and RACK aims to standards track. I feel that PRR is
> disjoint enough anyway, which I think was also your intention when
> maintaining the decoupling CC and RACK, that it's bit odd to take
> this strong stance on PRR (indepedent of the stds track vs
> experimental thing).
>
>   "due to congestion window validation."
> Does this refer to CWV as in RFC? Or some other mechanism in which
> case you shouldn't use the same term here.
>
> I find the described scenario somewhat unconvincing becase
> the description ends with:
>   "...
>    The ending congestion window after recovery also impacts
>    subsequent data transfer."
> ...First there wasn't enough data to fill cwnd of 20 packets and
> then suddenly transferring that non-existing data is impacted.
>
> References
>
> QUIC-LR is very old ref and even the filename has changed a few
> times from draft-tsvwg-...
>
>
> Nits
>
> "runt" made the sentence incomprehendable for me, I had to look up
> that word (and only second source was descriptive enough that I
> think I could understand the meaning, and I'd guessed wrong because
> I thought there was some special intent here because of the quotes).
>
> is dicussed -> is discussed
> RACT.RTT -> RACK.rtt
> First "TSO" -> TCP Segmentation Offload (TSO)
> grainularity -> granularity
> higher-sequenc -> higher-sequence
>
>
> --
>  i.
>