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

[Yuchung Cheng <[e-mail]>]

On Mon, Apr 6, 2020 at 2:01 PM Ilpo Järvinen
<[e-mail]> wrote:
>
> Hi Yuchung,
>
> Certainly improved on many things, however, I'm surprised that the lost
> rexmit comment was not addressed at all. Aren't the rexmit losses
> supposed to be detected by RACK or have I misunderstood something?
>
> I don't understand how lost rexmits are detected by the algorithms in
> the draft. I think the algorithms are either flawed or lack something
> (mainly RACK_detect_loss is relevant I think). My original comment
> from below.
>
>        Is RACK_detect_loss() 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?

Sorry to miss this point earlier -- RACK definitely detects
lost-retransmits, but the current algorithm does not properly define
some variables. Here is how I propose to address these by adding
these:


Per-packet variables


Theses variables indicate the status of the most recent transmission
of a data packet:
        “Packet.lost” is true if the most recent (re)transmission of
the Packet has been considered lost and needs to be retransmitted.
False otherwise.
        “Packet.retransmitted” is tue if its sequence numbers were
retransmitted in the most recent transmission. False otherwise.
        “Packet.xmit_ts” is the time of the last transmission of a
data packet, including retransmissions, if any. The sender needs to
record the transmission time for each packet sent and not yet
acknowledged. The time MUST be stored at millisecond granularity or
finer.


Transmitting a data packet
Upon transmitting a new packet or retransmitting an old packet, record
the time in Packet.xmit_ts.

--- src

RACK_transmit_data(Packet):

Packet.time_ts = Now()

Packet.lost = FALSE


RACK_retransmit_data(Packet)

Packet.retransmitted = TRUE
      RACK_transmit_data(Packet)

---

When a packet is retransmitted again, its Packet.lost is reset and
transmission timestamp is renewed. Therefore, when its transmission
time falls out of the RACK reordering window in RACK_detect_loss(), it
will be marked as lost (again).




>
> Also my timeout "some" vs "all" internal inconsistency comment relates to
> this same algorithm:
>
>          "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_loss()
>        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).

Comment 1.2: good catch. We changed to: 'For timely loss detection,
the sender is RECOMMENDED to install a "reordering settling" timer.
The timer expires at the earliest moment when it is safe to conclude
that all the unacknowledged packets within the reordering window are
lost.'


>
>
> Then there are some other comments I made which seem also unaddressed but
> I'll not duplicate them all here.
>
>
> --
>  i.
>
>
> On Mon, 9 Mar 2020, Yuchung Cheng wrote:
>
> > 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.
> >
> >
> >