Hi Loa, Thanks for the update.
Donald =============================== Donald E. Eastlake 3rd +1-508-333-2270 (cell) 155 Beaver Street, Milford, MA 01757 USA [email protected] On Mon, Jun 19, 2017 at 6:00 AM, Loa Andersson <[email protected]> wrote: > Authors, Working Group, > > Sorry for the late reply. > > I've been asked to re-review the document since it is (was) in working > group last call. > > I've reviewed again > > I find that > - my earlier comments has been satisfactorily addressed > - I have no further comments on this document > - I believe we are ready to ask for publication. > > /Loa > > > On 2017-01-21 06:02, Loa Andersson wrote: > >> 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: [email protected] > Senior MPLS Expert [email protected] > Huawei Technologies (consultant) phone: +46 739 81 21 64 >
_______________________________________________ trill mailing list [email protected] https://www.ietf.org/mailman/listinfo/trill
