Authors, I have been asked to do a Routing Area Directorate QA review of draft-ietf-trill-ecn-support. Caveat - I'm not a congestion control expert, and this will show up in my comments. The place where I ask for clarifications might be perfectly clear for a reader that is up to speed in the area. Summary: I think the document is on the right track, for a reader not an expert in the area there are a lot of abbreviations that are not properly expanded. I also think that it would be a good idea to more clearly make the the case why the document is needed (abstract and/or introduction). After a while I stop trying to distinguish between "Minor issues" and "Nits" and placed everything as Minor Issues. I guess I could have done everything as Nits :). Other than the Comment/Minor Issues I find the document pretty solid, though I sometimes found it hard to parse sentences. If you want I can take a look at that aspect once what is in this review has been addressed. Comments: Last paragraph of the Introduction ---------------------------------- Whichever RBridges do not support ECN, this specification ensures congestion notification will propagate safely to Destination because the packet will be dropped if explicit congestion notification cannot be propagated and drop is itself an implicit form of congestion notification. Is this logic really watertight? What if the packet is dropped because of a checksum error? Major Issues: Minor Issues: Abstract -------- I find the Abstract a bit meager, a little more context would be good. Maybe lead with some short words about what ECN is good for. And maybe use this from the Introduction: This specification provides for any ECN marking in the traffic at the ingress to be copied into the TRILL Extension Header Flags Word. It also enables congestion marking by a congested RBridge such as RBn or RB1 above in the TRILL Header Extension Flags Word [RFC7179]. ECNencapGuide ------------- This reference has expired, probably not a problem since Bob is a co-author of both documents. TRILL Header ------------ Referred to in section in the Introduction, I think a reference would be good. The ECN Specific Extended Header Flags -------------------------------------- The pictures is less than intuitive, it took me quite some time de-code it. I did the following: Critical (?) flags 0 - CRHbH (no expansion found in document) 1 - CRItE (no expansion found in document) 2 - CRRsv (no expansion found in document) CHbH flags (Critical Hop by Hop flags - no expansion found in document) 3 - unassigned 4 - unassigned 5 - unassigned 6 - unassigned 7 - CRCAF (no expansion found in document) NCHbH flags = Non-Critical Hop-by-Hop flags 8 - NCCAF (no expansion found in document) 9 - unassigned 10 - unassigned 11 - unassigned ------------------------------------------- 12 - ECN = Explicit Congestion Notification 13 (two bit flags) ------------------------------------------- CRSV flags (no expansion found in document) ------------------------------------------- 14 - Ext Hop Cnt (no expansion found in document) 15 three bit field 16 ------------------------------------------ NCRSV flags (no expansion found in document) 17 - unassigned 18 - unassigned 19 - unassigned 20 - unassigned ------------------------------------------ CItE flags = Critical Ingress-to-Egress ------------------------------------------ 21 - unassigned 22 - unassigned 23 - unassigned 24 - unassigned 25 - unassigned 26 - CCE = Critical Congestion Experienced ------------------------------------------ NCItE flags = Non Critical Ingress-to-Egress -------------------------------------------- 27 - Ext Clr (no expansion found in document) 28 two bit field -------------------------------------------- 29 - unassigned 30 - unassigned 31 - unassigned Multi-bit flags --------------- In the context I've been active "flags" are one bit, if there are multiple bits they are called fields. I understand that in the context this is written there are multiple bit flags. Bit 11 and 12 ------------- Bit 11 and 12 has the following code points: Binary Name Meaning ------ ------- ----------------------------------- 00 Not-ECT Not ECN-Capable Transport 01 ECT(1) ECN-Capable Transport (1) 10 ECT(0) ECN-Capable Transport (0) 11 NCCE Non-Critical Congestion Experienced Table 1. TRILL-ECN Field Codepoints There is no good explanation what ECT(0) and ECT(1) means, even though it is (page 9) said that "ECT(1) as a lesser severity level, termed the Threshold-Marked (ThM) codepoint". It could be inferred that ECT(0) is a higher severity level, but this should be clearly spelled out. RFC 3168 does not make the distinction between ECT(0) and ECT(1), but says that it will be done in future RFCs; since this is about 3000 RFCs ago it might have happened but I couldn't find it. If this has been done I think a reference would be good. Code Point 0b11 --------------- The text above Table 1 says: OLD "However codepoint 11 is called Non-Critical Congestion Experienced (NCCE)..." I think this should be: However code point 0b11 is called Non-Critical Congestion Experienced (NCCE)..." The text further says that the code point is call NCCE to distinguish it from Congestion Experienced in IP. The question I have is why it is necessary to distinguish code point 0b11, but not 0b00, 0b01 and 0b10? ECN SUpport ----------- The first paragraph has "logically" at three places, what would be the difference if these "logically" are dropped? First sentence in sectuion 3.1 ------------------------------ The sentence says: "The ingress behavior is as follows:" I would say "The behavior of an ingress RBridge is as follows:" or even "The behavior of an ingress RBridge MUST be as follows:" cleared vs set to zero ---------------------- The last sub-bullet in the first main bullet of section 3.1 says: "ensure the CCE flag is cleared to zero (Flags Word bit 26)." I would have used "cleared" or "swt to zero". First line section 3,2 ---------------------- s/ahow/shown Caveat I normally don't English grammar reviews, but sometimes I can't stop myself :) Second line first main bullet in section 3.2 -------------------------------------------- I prefer the format "guidelines in RFC 7567 [RFC7567]" Third sub-bullet in the third main bullet of section 3.2 --------------------------------------------------------- It says: "+ set the TRILL-ECN field to Not-ECT (00);" Here you use "field" instead of "flag", which is fine, but the document should be consistent. Either: + set the TRILL-ECN field to Not-ECT (0b00); or + set the TRILL-ECN flag to Not-ECT (0b00); Egress ECN Support ------------------ First sentence: "If the egress RBridge does not support ECN, it will ignore bits 12 and 13 of any Flags Word that is present, because it does not contain any special ECN logic." in "it will ignore" what does "it" refer to? SHould it be: "If the egress RBridge does not support ECN, the RBridge will ignore the TRILL-ECN field (bits 12 and 13) if a Flags Word that is present, because it has no ECN logic." TRILL Support for ECN Variants ------------------------------ The sedond sentence of section four says: Section 3 specifies interworking between TRILL and the original standardized form of ECN in IP [RFC3168]. RFC 3168 is updated by RFC 4301, RFC 6040, does section 3 relate to RFC 3168 or does the updates come into plan. IF the updates are in scope I think the sentence should say: Section 3 specifies interworking between TRILL and the original standardized form of ECN in IP RFC 3168 [RFC3168], and the updates in RFC4310 [RFC4301] and RFC 6040 [6040]. Nits: Codepoints ---------- at several places "codepoint(s)" I think the words IANA and the RFC Editor use is "code point(s)" /Loa -- Loa Andersson email: loa@mail01.huawei.com Senior MPLS Expert loa@pi.nu Huawei Technologies (consultant) phone: +46 739 81 21 64