Re: [tcpm] RFC793bis AD review

[Wesley Eddy <[e-mail]>]

Here are my replies on each finding, with proposed changes to make sure 
you agree, before cutting another revision of the document:


On 6/24/2021 4:12 PM, Martin Duke wrote:
>
> (Document Header) Please update the Intended Status to "Internet 
> Standard".

There doesn't seem to be a way to force this in xml2rfc3; the 'category' 
attribute is set to 'std' (Standards Track) and there's no distinction 
(this looks consistent with other recent Internet Standard RFCs).

So, I think no change is needed on this.


> (3.8.3) R1 and R2 can be measured in retransmissions or seconds; the 
> proposed bounds are in retransmissions for R1 and seconds for R2. So 
> if I have a time-based R1, should that be 3? If I have a 
> retransmission based R2, should I compare that to the value of RTO 
> somehow?

This is heritage from RFC 1122.  The text on R1 mentions "at the current 
RTO", so I think RO is definitely the intended conversion factor, but 
since you asked, maybe it's not clear enough?  Do you think maybe in 
part (a) of 3.8.3 we could just change:

     R1 and R2 might be measured in time units or as a count of 
retransmissions.

to:

     R1 and R2 might be measured in time units or as a count of 
retransmissions (with the current RTO as a conversion factor, if needed).

>
> (3.8.5)  This section is unclear as to whether the sender can cause 
> the Urgent Pointer to recede (move to the left), and if so, if that 
> MUST be reported to the application like a pointer advance.

Receding looks the same as advancing so much that it wraps.  So, if not 
already in urgent mode, that would seem to cause a change to urgent 
mode, while if not already in urgent mode, that would fall under the 
existing rule of:

     If the urgent pointer is updated while the user is in "urgent 
mode", the update will be invisible to the user.

So ... also given that the use of urgent pointer is deprecated anyways 
and we don't want to cause changes to TCP in this doc, I'm proposing we 
do nothing to clarify this, if you agree?


> (3.8.6.1)
> The sending TCP peer must be prepared to accept from the user and
>     send at least one octet of new data even if the send window is zero.
>     The sending TCP peer must regularly retransmit to the receiving TCP
>     peer even when the window is zero
> Please clarify that these probes only happen when there is new data, 
> if that's the intent.

The "accept from the user" part means that new data was written already, 
right?  If the user doesn't write data into the socket, there's nothing 
to be sent.


> (3.8.6.1) the first paragraph says
> Two minutes is recommended for the retransmission interval when the
>     window is zero.
> but the last one says
> The transmitting host SHOULD send the first zero-window probe when a
>     zero window has existed for the retransmission timeout period (SHLD-
>     29) (Section 3.8.1  <https://datatracker.ietf.org/doc/html/draft-ietf-tcpm-rfc793bis-23#section-3.8.1>), and SHOULD increase exponentially the interval
>     between successive probes (SHLD-30).
> so is it 2 min or the RTO?

Good question ... The first sentence comes from 793 and the other from 
1122.  I think we should erase the first sentence, since the 1122 intent 
seems to be updating this (replacing the hard-coded 2 minute heuristic 
with the RTO one which adapts to reality).  Do you agree?


> (3.8.6.2.1) If I read this algorithm correctly, an un-pushed send 
> buffer of < MSS, if D < Fs*Max(SND.WND), will sit there indefinitely 
> without being transmitted; is this intentional?

This comes from 1122.  I think the key to preventing what you mention is 
this part of the preceding paragraph:

   To avoid a resulting deadlock, it is necessary to have a
    timeout to force transmission of data, overriding the SWS avoidance
    algorithm.

So, SWS avoidance is not supposed to keep a sender waiting forever for 
the receiver to possibly advertise a bigger window; the sender is 
supposed to wake up and just send eventually.


> (3.8.2) and (3.8.6.3) Should RFC 5681 be a normative reference, given 
> the congestion control requirement? If not, (3.8.6.3) shouldn't 
> require the reader to go there to fully implement delayed ACKs. I suggest
>
> s/an ACK for at least every second segment/an ACK for least every 
> 2*MSS bytes of data
>
> to fully state what is in 5681.

Yeah, I think 5681 should be normative (since we also rely on the for 
descriptions of slow start, etc. that we didn't try to duplicate within 
793bis), but we should also fix the "second segment" sentence to match 
more exactly the text in 5681 about 2*RMSS.


> (3.10) I understand the desire to conform to RFC 793 formatting, but 
> this section is very long and has quite a bit of hierarchy. Some 
> subsections would be very helpful for some needed cross-referencing, 
> as described in the nits.

I completely agree.  Numbering these sounds good to me.


> (3.10, OPEN/LISTEN)
> If active and the remote socket is specified, then change the
>           connection from passive to active,
> I think you mean "If passive"?

Actually, I think to be more clear this should say:

"If the OPEN call is active ...", since it's referring to the case where 
an active OPEN is called when the socket is in LISTEN (passive).

It's technically correct currently (just not very clear), if you read 
the "If active" as referring to the OPEN call rather than the socket 
(and it must be that way since a socket in the LISTEN state is the 
result of a passive OPEN).


> (3.10, SEGMENT ARRIVES/LISTEN) Step 4 mentions RST but those have 
> already been handled in Step 1.

Yeah, you're definitely right, and in both cases the action is to ignore 
it, so this part of step 4 is irrelevant, but at least consistent.

It looks like this is an errata to file on 793; should we do that 
quickly and then fix this doc accordingly?


> _*NITS:*_
>
> (3.1, data offset section) s/integral number/integer multiple
>
> (3.1, URG and ACK) s/field significant/field is significant

Both of these are fine to change with me; they're just carry over from 793.


>
> (3.1, Window) It would be useful to observe here that RFC 7323 can 
> change the meaning of this field.

Agreed.


> (3.1, Options) "
>     Options: [TCP Option]; Options#Size == (DOffset-5)*32; present only
>     when DOffset > 5.
> What is this notation?
It is a little odd, but comes from reference [61] for 
draft-mcquistin-augmented-ascii-diagrams-08.

> (3.4)  s/l0/10 (switch letter L with numeral 1)

Good eye; that looks like it was wrong in 793!


> (3.7.2) In the first paragraph it says
> RFC 1122  <https://datatracker.ietf.org/doc/html/rfc1122>  recommends an IP-layer default
>     effective MTU of less than or equal to 576 for destinations not
>     directly connected
> In the last it says
> RFC 1191  <https://datatracker.ietf.org/doc/html/rfc1191>  discusses this implication of many older TCP
>     implementations setting MSS to 536 for non-local destinations, rather
>     than deriving it from the MTUs of connected interfaces as
>     recommended.
> Could we unify these comments in the same location? Implementers 
> browsing the spec should see the old recommendation and its drawback 
> in the same place.

Good idea; I think we can.


> (3.8) "push function" should include a reference to Section 3.9.1.
>
> (3.8.2) s/endpoint may implement/endpoint MAY implement
>
> (3.8.6.2.1) s/min(D.U)/min(D,U)
>
> (3.9.2.2) Is "> a stray mark, or does it mean something?
>
> (3.10) s/and it the segment/and the segment
>
> (3.10, RECEIVE/CLOSE_WAIT) s/text/data
>
> (3.10, SEGMENT ARRIVES/LISTEN) s/text/data
>
> (3.10, SEGMENT ARRIVES/SYN_SENT) Step 4 directs the reader to the 
> "sixth step below" but the algorithm has only 5 steps! IIUC it is 
> referring to step 6 of the "other states" procedure, which is several 
> pages later. This needs to be clearer! (and is where having 
> subsections would be helpful).
>
> (3.11) There are redundant definitions of "Type of Service" and "TOS"

I'm good with changing all of these above.

Thank you for the detailed review!