Re: [tcpm] FW: Review of draft-bagnulo-tcpm-generalized-ecn (David Black)

[Bob Briscoe <[e-mail]>]

David,

Thank you for these useful comments.
Detailed responses inline.

In summary, I think all your proposed changes improve clarity. I don't 
think you've disagreed with any technical effect of the proposed protocol.


On 07/04/17 16:46, Black, David wrote:
> I sent this review to the draft authors privately, and they asked that I forward to the TCPM list; this version had been edited slightly from what I sent to the draft authors.  Please cc: me directly on any responses or follow-up emails, as I'm not currently on the TCPM list.
>
> In general, I think this draft is headed in the right direction.  I've limited this review to items with at least moderate technical impact.  I apologize that it's taken over 4 months to get this review done - the day job has been putting much more pressure on my IETF activity that expected since the start of this year.
>
> Overall concern: Abstract and Introduction should make it clear  that this draft is primarily about Accurate ECN, with applicability to vanilla RFC 3168 ECN being an open question for further discussion.
[BB] We obviously need to clarify that AccECN is solely relevant to 
SYNs, and all other control packets could be made ECN-capable whether or 
not AccECN was being used. Actually, AccECN is only mentioned in the 
sections about SYNs. However, I accept that it would help to point that 
out explicitly. And to explain why. Here's why:

In RFC3168 ECN feedback, the Echo Congestion Experienced (ECE) flag is 
used for capability negotiation during the 3WHS. So, even if the client 
makes the SYN ECN-capable and then it's marked CE, the SYN-ACK has 
nowhere for the server to feed this back. In contrast, an AccECN server 
supports feedback in the SYN/ACK of any CE on the SYN.

The relevant sections of AccECN are:
https://tools.ietf.org/html/draft-ietf-tcpm-accurate-ecn-02#section-2.5
https://tools.ietf.org/html/draft-ietf-tcpm-accurate-ecn-02#section-3.1

BTW, there is one mention of AccECN in the section on Pure ACKs as well 
- necessary to consider whether AccECN would count CE on Pure ACKs. 
We'll clarify that this is not saying that AccECN is necessary.

> It's also not clear to me whether applicability to vanilla RFC 3168 ECN is a single global decision for all packet types vs. a per-packet-type decision.
[BB] This is a consequence of the process of reaching consensus.

Initially, we had to assume that someone might come up with a good 
argument against setting ECT on one of the types. So it is written as if 
the decision on each type is independent. Nonetheless, I think it would 
now make sense to add a sentence saying:

To comply with this spec, a TCP sender will set ECT on all types of 
control packets.

However, RFC3168 allows an ECN sender to choose not to set ECT on any 
individual packet (although it gives no example reasons why a sender 
would not set ECT). So I don't think we don't need to /require/ a TCP 
sender to set all types of packet to ECT.

>
> -- Section 1.3
>
> OLD
>     RFC3168 does not prevents from using ECN in TCP
>     control packets lightly.  It provides a number of specific reasons
>     for each packet type.  In this Section 4, we revisit each of the
>     arguments provided by RFC3168 and explore possibilities to enable the
>     ECN capability in the different packet types.
> NEW
>     RFC3168 does not prohibit use of ECN for TCP
>     control packets lightly.  It provides a number of specific reasons why
>     ECN should not be used for each packet type.  In this Section 4, we analyze
>     each of the reasons provided by RFC3168 as part of explaining why this
>     experiment is reasonable to conduct for each of the different packet types.
>
> Last sentence in old text is too weak - "explore possibilities" doesn't sound like providing a solid rationale for experimentation.
>
> -- Section 2
>
> OLD
>     Retransmission: A TCP segment that has been retransmitted by the TCP
>     sender because it determined that the original segment was lost,
>     which may or may not be the case.
> NEW
>     Retransmission: A TCP segment that has been retransmitted by the TCP
>     sender.
>
> Shortened definition to improve clarity.
[BB] AOK so far
> -- Section 3.1
>
> This section is problematic as written, as it appears to require a router to parse TCP packets and maintain enough state to detect retransmissions.  That's surely not what was intended ;-).
[BB] Indeed. We intended this to apply to any middlebox (e.g. firewall) 
that already parses the TCP header.
> This section should be rewritten and dramatically shorted to require that routers treat ECT-marked TCP control packets and retransmissions just like any other ECT-marked packet,
[BB] I agree (and I hope my co-author does) that it would be clearer 
just to say this.
> as specified in RFC 3168 (and don't repeat lengthy RFC 3168 text here).

[BB] Unfortunately, RFC3168 does not specify anything about network node 
forwarding behaviour wrt ECT. So some firewall policies could have 
decided to block any TCP control packets that contravene RFC3168. That's 
why we wrote this section.

However I admit that, by not explaining that DPI was the problem we were 
trying to solve, it reads like we are advocating DPI, rather than 
removing any ambiguity that might have been interpreted as a need for 
DPI. We'll fix this.
>
> The reason to rewrite this section to point to RFC 3168 is that any divergence from RFC 3168 network node behavior for other ECT-marked packets is likely to not only be a bad idea, but to also be fatal to this proposal.  It's ok to cite RFCs beyond 3168 if needed to specify  the network node behavior.
[BB] As above, no network node behaviour wrt ECT is specified in RFC 
3168 (unless you can point to it - I just checked again, and I couldn't 
find it).
>
> -- Section 3.2.1.1
>
>     The server SHOULD not set any of the ECT codepoints if
>     the server is included in the cache as not supporting ECN in the SYN
>     packet.
>
> Which cache?  The natural reading of this text is "the client's cache" in which case this requirement is unimplementable as written because the server can't see the client's cache.  I suspect that the actual requirement is that the server not change its ECN behavior until any client cache entry for it is certain to have aged out (how long would that be? provide an answer ...).
[BB] This is a simple typo. I'm sure the sentence was intended to start 
"The client SHOULD...", not "The server SHOULD..." (The section is 
entitled "TCP client behaviour"). Sorry about that.
> There appears to be an analogous concern with the server cache in 3.2.1.2.
[BB] I'm not sure I agree with the need for a server cache about clients 
at all. The incoming SYNs unambiguously say what type of client they 
came from.

I will check with my co-author why this sentence is here.
>
> --Section 3.2.2.1
>
>     Also, it should be noted than if an ACK
>     is dropped due to congestion the sender of the ACK does not react by
>     reducing the load in any way.
>
> Given that statement, much of the discussion of how to reduce sender load on the network may be irrelevant.
[BB] I agree. We will put this sentence first. Then say that ECT on pure 
ACKs could be used for ACK CC and refer to RFC 5690 rather than 
describing a possible ACK CC scheme here. Then make the point that an 
ECN connection not implementing ACK CC is hardly worse than a non-ECN 
connection not implementing ACK CC, both of which are the norm.
> In the quoted sentence, at  least the first instance of "ACK" should be "pure ACK".
[BB] Yup.
>
> -- Section 3.2.3.1
>
> I think the reduction in offered load discussion in this section is pointless;
[BB] The text is sort-of saying that. But I admit it could be clearer - 
we'll sort it.
> the sender should behave as if the Zero Window Probe were dropped.
[BB] Well, I don't know that any TCP implementation does anything 
specific in this case. And I certainly can't find anything written in 
RFC793 or RFC5681 about this.
>
> -- End of Section 3.2
>
> This would be a good place to summarize sender and receiver behavior changes.  That summary is needed anyhow, as the changes to RFC 3168 need to be clearly called out, and preferably summarized in one place.
[BB] Good, yes.
>
> -- Section 4.1
>
>     In particular, setting the CE
>     codepoint in the very same packet seem to fulfill this criteria,
>     since either the packet is delivered and the CE codepoint signal is
>     delivered to the endpoint, or the packet is dropped, so the original
>     congestion signal through the packet loss is delivered to the
>     endpoint.
>
> That assumes that "the very same packet" will be retransmitted, which is correct for data packets, but not control packets (e.g., a pure ACK  or a zero window probe).  What may be missing (or not well stated in the rest of this paragraph) is that drops of these packets don't cause congestion reactions.
[BB] Yes, we'll make this point clearer.
>
>     As currently specified, ECN adoption implies an increased reliability
>     of the ECN congestion signal and a decrease in the reliability in the
>     TCP control packets.  We believe that it is possible and desirable to
>     restore the tradeoff existent in non ECN capable networks in terms of
>     reliability, where the congestion signal delivery is as reliable as
>     in a non ECN capable network and so it is the delivery of TCP control
>     packets.
>
> Add a section reference for decrease in reliability - to Section 1.1 ??
[BB] Will do.

Thanks again,



Bob
>
> Thanks, --David
> --------------------------------------------------------
> David L. Black, Distinguished Engineer
> Dell EMC, 176 South St., Hopkinton, MA  01748
> [phone]     Cell: [phone]
> [e-mail]  <=== NEW ===
> --------------------------------------------------------
>
> _______________________________________________
> tcpm mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/tcpm
>

-- 
________________________________________________________________
Bob Briscoe                               http://bobbriscoe.net/