Re: [tcpm] Detailed review of AccECN rev -04

I'd like to pick up on your replies. Please see inline.

Gorry

On 05/03/2018 14:09, Mirja Kühlewind wrote:
> Hi Gorry,
> 
> say for your feedback. Please see below.
> 
>> Am 20.11.2017 um 08:15 schrieb Gorry Fairhurst <gorry@erg.abdn.ac.uk>:
>>
>> tcpm@ietf.org
>>
>> I've read the -04 rev of teh above ID, and I have the following comments for the TCPM WG.
>>
>> Overall,
>>
>> * This document reads well, and seems mature.
>>
>> * There are several references to other IETF activities, and the text concerning these appears to need to be revised before this is ready to proceed. See below.
>>
>> * Since this document was adopted, work on ECN++ appears to have become inter-twined with this ID. I did not find this helpful, and made comments on ECN++ today that voiced this concern and requested we consider as a WG to best frame the proposed modification on the TCP SYN permitting ECT(?) to be set. See comment (6).
>>
>> * I wonder if this document updates any other RFCs - I'd assume not -  but it may if a future work proceeds along the standards track. I think it would be good to specifically identify in one section of the intro what these implications are.
>> For instance:
>> - The discussion of ECN on SYN is predecated by ECN-Experimentation (targeting publication as PS)
>> - The discussion of ECN on SYN has implications on ECN++, which recommends AccECN is implemented.
> 
> This document does NOT enable ECN on SYN. However, it does specify how to provide feedback on what was observed in the SYN (also because middleboxes might remark wrongly). These are two independent things.
> 
>> - A future standards-track document based on the AccECN experimental RFC could update RFC3168.
> 
> No, I don’t it would update RFC3168. It does not change anything in that RFC. It also specified an additional meachismen (which we hope will deploy as the default). The only thing we change is the use of the NS bit in the SYN but that was previously unused; with or without ECN Nonce.
> 
OK that makes sense.
> 
>> - etc.
> 
> I don’t think there is any update. I thin we’ve been trying to be careful about that.
> 
>>
>> * The document may intend a proposed change to RFC 5681 (4.2.  Generating Acknowledgments). See comment (8).
> 
> I also don’t think so because RFC5681 says "The delayed ACK algorithm specified in [RFC1122] SHOULD be used by a TCP receiver.“ and not MUST. And delay ACK SHOULD still because but there are further things to consider.
> 
> 
>>
>> Gorry
>>
>> ---
>>
>> Some more specific comments on the text in the -04 AccECN draft:
>>
>> (1) The introduction - "until the AccECN experiment suceeds, [RFC3168] will remain as the standards track specification for adding ECN to TCP."
>> - This seems odd. I think this is not the case:
>> Quite simply: "RFC3168 defines the current TCP feedback method."
>> - It may be you think a  future standards-track document based on the AccECN experimental RFC could update RFC3168, that is possible, but there is no need for an EXP to make that direct claim against as PS.
> 
> I added any „only“ here based on Micheal’s comment:
> 
> "Until the AccECN experiment succeeds, [RFC3168] will
>        remain as the only standards track specification for adding ECN to TCP.“
> 
> This sentenced was just mention to introduce the term „classic ECN“. It was not meant to say anything about a status change of RFC3168, which we also do plan to do as explained to Micheal. AccECN is implicitly backward compatible to ECN therefore even AccECN as PS will not change, nor update anything that is said in RFC3168.
> 
OK - so I think this actually should say the "only standards track 
method for providing ECN feedback" ... or something similar.

>>
>> (2) Section 1.5. First para. (On loss v drop using ECN)
>> - I'd feel happier also noting that a router implementing AQM is still allowed to drop packets marked as ECT() when this is needed - and you could refer to the BCP to say so: /RFC/7567.
> 
> I didn’t think the section say anything else. Also this is just the ECN recap…
> 
> This says:
> 
> "When a network node experiences congestion, it
>     will occasionally either drop or mark a packet, with the choice
>     depending on the packet's ECN codepoint.  If the codepoint is Not-
>     ECT, only drop is appropriate.  If the codepoint is ECT(0) or ECT(1),
>     the node can mark the packet by setting both ECN bits,…“
> 
> So it explicitly say „mark or drop“ and „can mark“...
> 
> 
>>
>> (3) Section 1.5. Last para.
>> - Please remove the discussion of Nonce from here!
> 
> I think it is still important to mention that this bit is not assigned completely newish but has been assigned for ECN purported previously. Based on Micheal’s comment I added a reference to the ecn-exp draft and explicitly say that NS has been marked as „revered“.
> 
>> - It is OK simply to refer to the PS specifying ECN Experimentation - or the process document to do this.
>> - If it is really necessary to discuss this, please use the form of words expressed in that draft, rather than saying "was never deployed“.
> 
> Done. Now it says
> "In the absence of widespread deployment RFC 3540 has been reclassified as historic [RFC8311]."
> 
OK
>>
>> (4) Section 3.1.1
>> "is being reclassified as historic."
>> - Again this text is now out-of-date, see comment (3).
> 
> s/is being/has been/
OK
>>
>> (5) Section 3.1.1, ECN Nonce.
>> - This definition also contains text later that say "does not need...". It's not that it doesn't need, more that the IETF has changed the use of this field. I think it would be easy to simply remove this definition - but the WG wants to keep it please align the text precisely with the ECN Experimentation draft. The last para of section 4 repeats this and has the smae problem.
> 
> Good catch. Changed to
> 
> "With AccECN implementation, there is no need to for the ECN Nonce feedback mode [RFC3540], which has also been reclassified as historic [RFC8311], as AccECN is compatible with an alternative ECN feedback integrity approach…"
> 
> Does that work for you?
> 
It would, with changing /as/ to /because/
>>
>> (6) Section 3.1.2. ECN on SYN
>> - This section seems to be a variant of the section expressed in the ECN++ draft (sect 4.2). That draft rather awkwardly refers back to this draft recommending AccECN is implemented - but the two specs differ.
> 
> Section 3.1.2? Not sure what you mean…
> 
> However, we have been to only specify a feedback scheme (that includes SYN feedback) and not say anything about when or if to use ECN (in general not only for the SYN). Please let me know if that is anywhere not the case.

My feeling was that the two specs differ for how ECN is used with SYN.
If that's not the case (as you say), it would be good to hear from the 
ECN++ people that they agree with the spec as written here - so we know 
that they are not going to be surprised when people refer back to this 
during the WGLC of their draft.

>>
>> I do encourage the WG to think about where the update on ECN for SYN is best described.
> 
> As I just said we clearly want to separate feedback and usage of ECN as this is also one of the problems with RFC3168.
> 
Separation is good. It would help to state (in section 3.1??) that this 
does not specify setting of the IP ECN field, only how feedback is 
negotiated.

>> There are specific issues for experimentation and for caching in ECN++ and other issues here. I'd like a cleaner spec so that **IF** there are results from the experiments we know how to then publish a PS and declare the experiment as historic. At the moment this seems awkard.
> 
> I think the nice part of this separation is actually that AccECN is independent of the results of the ECN++ experiments.
See above.
>>
>> - Section 3.2.7.2 is also about the SYN negotiation. And similar to ECN++.
> 
> I don’t know where the ECN+ doc says anything about negotiation. negoation and option fallback belongs in this doc. Setting and testing of the IP ECN codepoint below in ECN++.
> 
>>
>> (7) Section 3.2.3 (First segment after SYN)
>> - TCP segments can be re-ordered, so saying the "first segment after SYN" should (I think) probably be the segment numbered after the SYN segment - noting that this is not necessarily the next segment received (in time order).
> 
> I don’t see where this is written. Actually the text says:
> 
> "More precisely, the "first segment with
>     SYN=0" is defined as: the segment with SYN=0 that i) acknowledges
>     sequence space at least covering the initial sequence number (ISN)
>     plus 1; and ii) arrives before any other segments with SYN=0 so it is
>     unlikely to be a retransmission.“
> 
OK - somehow I didn't focus on that as I read, so that seems fine.
> 
>>
>> (8) Section 3.2.5
>> - The modified delayed ACK policy stated here is an experimental update to the IETF recommended procedure in a PS (RFC5681, 4.2.  Generating Acknowledgments). I think it is important that the change is referenced back to this RFC. The proposed change could have implications on the capacity used for the ACK stream - and this should be noted.
> 
> I disagree as explained above.
> 
>>
>> (9) RFC2119:  Section 3.2.7.1, para 1
>> - The ID says the client MUST NOT include the AccECN option on the SYN. What does a server do that receives such an option?
> 
> I guess as long as the option is correct, there is no problem and therefore no further specification is needed; it would be treated like every other AccECN option. Not sure if that needs to be explicitly mentioned...
>>
>> (10) RFC2119:   Section 3.2.7.1, para 2
>> - "A TCP server that confirms its support ... SHOULD also"
>> - I couldn't parse that sentence... Is that to imply there are valid reasons why the server could be implemented in a way that does not do this? (That doesn't seem to be explained).
> 
> Yes, it's the endpoints decision to use options at all. If it does not want to support feedback in options it should not send it in the SYN/ACK. I just realized that this not stated anyway explicitly, so I added a sentence at the end of in section 3.2.6.:
> 
> "The use of the AccECN option is optional for the Data Receiver. If the Data Receiver intents to use the AccECN option at any time during the rest of the connection it strongly recommended to also test its path traversal by including it in the SYN/ACK as specified in the next section. By default the use of the AccECN option is RECOMMENDED.“
> 
> 
>> - What is the meaning of "also" here. Does that mean the support is already confirmed in some other way? (I'm not sure also after SHOULD is a helpful construction of a clause).
> 
> This referred to in addition to the information provided in the ACE field. But I just removed the „also".
>>
>> (11) RFC2119: Section 3.2.7.1, para 3
>> - I think the sentence starting "A TCP Client" also contains two SHOULD clauses without saying what are the conditions for not doing this.  I think it is often wiser to start these sort of clauses by saying something like this order to avoid this:
>>
>> ... A host that receives an AccECN option SHOULD respond by returning the AccECN option in the first ACK at the end of the 3WHS. ... There are three cases when the host could decide not to …
> 
> No this is not the intention here. Option travel may be independent in both direction and should be tested independently.
>>
>> When the TCP client returns the ECN option, it SHOULD also include an AccECN option on the first data segment it sends... This is to ensure that...
>>
>> (12) RFC2119: Please also avoid "MAY NOT" as a construct, since this can be mis-interpretted by different varients of English usage.
> 
> What else would we say? This is the correctly RFC2119 language.
> 
OLD:
    A host MAY NOT include an AccECN Option in any of these three cases
    if it has cached knowledge that the packet would be likely to be
NEW:
    A host MAY decide to NOT include an AccECN Option in any of
     these  three cases, when it has cached knowledge that the
     packet would be likely to be

- To me that more clearly states it is permitted to not include.

>>
>> (13) Section 3.3 appears to make string normative requirements (MUST) on the operation of middleboxes. This may be the intention, but if so, I think it would be much wider to then make this evident in the section title and the document introduction.
> 
> Yes, that is intended. I renamed the section to
> 
> "Requirements for TCP Proxies, Offload Engines and other Middleboxes on AccECN Compliance“
> 
> Does that help?
> 
Yes.

>>
>> (14) RFC2119: Section 3.3 appears to make mixed normative requirements (MUST attempt) on offload. I don't know what that means. I think this could be equivalent to "SHOULD" with an explanation of WHY this is so needed (which is sort of there).
> 
> s/MUST attempt to/SHOULD/
> 
> Also added
> 
> „… as these can be used by the Data Sender to infer further information about the path congestion level.“
> 
OK
>>
>> (15) Section 3.3 on offload.
>> - If this is intended to change offload behaviour, I think it needs a separate section to say this, mentioned in the intro, otherewise people implementing offload will not see and read this.
> 
> Not really. How offload works with AccECN is a question I often get. My current understanding is that everything likely would still work but be less effective (at least when there are high level of path congestion). I would expect that is AccECN gets widely deployed offloading could be further optimized but there doesn’t see to be an urgent need now. However, knowing if what I actually said is all true, might also be part of the experiment…
> 
>>
>> (16) Section 4, Section 5, Appendix A, intro.
>> - You could remove (not normative) to be even clearer.
> 
> I don’t really see a point in removing these parts. As there are non-normative this obviously says that the spec would still be complete without them, however, I think the still have some useful information and I don’t the what we gain by removing them…?
>>
OK
>> (17) For the avoidance of doubt it may be wise to state the standards status of the TCP AO and Conex work... e.g., "TCP AO is a standards-track method to …"
> 
> Where should we do that?
> 
You could here:

The TCP authentication option (TCP-AO [RFC5925]) is a standards track 
method that can be used to detect any tampering with AccECN

> Mirja
> 
> 
>>
>> Best wishes,
>>
>> Gorry
>>
>> _______________________________________________
>> tcpm mailing list
>> tcpm@ietf.org
>> https://www.ietf.org/mailman/listinfo/tcpm
> 