[tcpm] Review of draft-zimmermann-tcpm-reordering-detection-02

[Michael Welzl <[e-mail]>]

Hi,

I just read https://tools.ietf.org/html/draft-zimmermann-tcpm-reordering-detection-02 <https://tools.ietf.org/html/draft-zimmermann-tcpm-reordering-detection-02>

Since I don’t fiddle with the TCP recovery code on a daily basis, I am a member of a large group of people who, in my opinion, can never make solid claims about the correctness of a mechanism like this one. All people like me can do is read and comment and hope for the best  :-)    because fully understanding this from simply reading, not looking at code, is REALLY hard. I don’t blame it on the authors: I appreciate that making this document easier to read must be very, very difficult.


Anyway. Here it goes - enjoy  :-)


Overall, this looks like a good mechanism to me, and I support the document. Details below:

***
A TCP connection that does not employ the Nagle algorithm SHOULD NOT use this methodology.
***
isn’t Nagle something that can be dynamically turned on or off? If so, I guess this statement should elaborate - do you want the whole mechanism to be disabled if Nagle is disabled once during the lifetime of a connection?


***
4.1 c.2 "If the TCP Timestamps option [RFC1122] has been negotiated,
         then the variable Timestamps MUST be activated and….”
***
I don’t think one can “activate” a variable. Since this is a boolean, this should probably read “MUST be set to true” ?

(below in c.2, “deactivated” is fine because it talks about a mechanism, not a variable)


4.3 S.1: skip to step A.4 is really weird here… so S.1 to S.5 is supposed to be a function call from within the sequence A.1 to A.4; I’d expect to see something equivalent to a “return” here, which would take us to A.3, not A.4


Also, when reaching S.4 and reading "Samples[SEG.SEQ].ReorExtR” I wondered where “ReorExtR” came from? Oh right, a sentence above in S.2 says it, they are the results of steps E.1 to E.4. But this code comes later - I think the fact that this “function call” returns ReorExtR and ReorExtA should be presented more prominently rather than just mentioning the variables in passing.

Maybe extra information could be added below all the “function calls”, i.e. the statements of the style:

"The TCP sender MUST execute steps (E.1) to (E.4)”

For this particular statement, the extra information could read:

“This is the reordering extent computation, yielding the relative reordering extent in variable ReorExtR and the absolute reordering extent in variable ReorExtA.”

This could (hopefully?) make the algorithm easier to understand.


4.4 D.2 and D.3: I think that the phrase "If a) the previous step (D.1) was not executed” isn’t as easy to parse as “If Dsack == false” of “If NOT DSACK” would be. If possible, you could replace it. However note that this is maybe not equivalent? My condition also fires if Dsack was set to true before, not only if D.1 was not executed in the same sequence. Clearly, only replace if that doesn’t matter  :)


Up to section 5, unless I overread it, there is no single statement that explains the underlying idea of the algorithm in simple terms. To me, this is all captured well by the following sentence in section 5:
"Whenever a received acceptable ACK or SACK closes a hole in the
  sequence number space of the SACK scoreboard either partially or
  completely, this is an indication of packet reordering in the network
  (step (A.2)).”

I think section 5 is fine where it is, and it’s good that it elaborates further after this sentence - but the message conveyed by this particular sentence should be brought very early, in the introduction, probably even in the abstract. From the abstract, it’s not exactly clear *how* the mechanism here achieves it’s magic - it only says: "The algorithm for the detection and quantification of reordering in this document uses information gathered from the TCP Timestamps Option, the TCP SACK Option and its DSACK extension.”  - I would strongly suggest to say already here that the underlying idea is that filling a hole can indicate reordering.

!!!! continue reading in sec 5 after this statement.

Just a style thing: I would replace "The important fact is that...”  with “What matters is that …”

IIUC the discussion in sections 6.1 / 6.2 assumes that reordering typically occurs as a function of time rather than as a fixed number of packets. While that seems like a reasonable assumption to me, is there any basis to it - anything that can be cited? (sorry if I missed it!)

section 6.2: “The scheme proposed in this document is to calculate the relative reordering by “  should be: “The scheme …. calculates the relative…”

Last paragraph of section 6.2:  Hmmm… I don’t get why this is so hard. Maybe this is just me being naive and stupid here, but then maybe the explanation should be improved: flightsize is the currently used window, which is transmitted per RTT. So the current rate is flightsize/RTT, no? Wouldn’t that suffice (say, using the latest RTT sample) as a rate, rather than a value that depends on the BDP?

If you don’t want to go down this path for some reason but if I’m right with this simple idea, then the statement "But since the calculation would be far from trivial and introducing more complexity, this is considered to be future research.” should perhaps be updated.

6.3 par 1: remove “a” in "even with a minor packet reordering”, and missing “the” in "From TCP sender's perspective”. Also, “persistent receiving” => “persistent reception”.

par2: I have trouble parsing the middle ("amount of data a reordered…") of the sentence:
 "If the transmission
   rate of the TCP sender, and therefore also the maximum amount of data
   a reordered segment can be received too late, changes significantly
   during its stay in the 'disorder' state, the actual amount of
   reordering is not accurately determined by the relative reordering
   extent.”

below: double “enter” in "if the amount of outstanding data is low when entering the 'disorder' state is entered”

section 7 par 1: missing “the” in "Because of retransmission ambiguity problem…”

same par: "the detection is
   usually conducted by detecting a closed hole in sequence number space
   of the SACK scoreboard.”  - what do you mean, “usually”? This is what the algorithm described in this draft does, right? Or is it also elsewhere? (if so, I suggest to add a reference)

section 10: broken sentence ("performance measurement tool, which give us a powerful tool”)


Cheers,
Michael