Re: [trill] QA review of draft-ietf-trill-ecn-support

[Loa Andersson <[e-mail]>]

Donald,

I'm pretty happy with your response on my comments. I think that the 
agreed updates will go a long way to improve readability of the
document.

/Loa

On 2017-01-27 06:09, Donald Eastlake wrote:
> Hi Loa,
>
> Thanks for your comments. See below
>
> On Sat, Jan 21, 2017 at 12:02 AM, Loa Andersson <[e-mail]> 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 :).
>
> OK :-)
>
>> Other than the Comment/Minor Issues I find the document pretty solid,
>
> Thanks.
>
>> 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?
>
> I think the point of that paragraph is to overcome the presumption
> many readers might have that ECN marking would be useless if the
> Destination does not understand ECN marking.
>
>> 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].
>
> OK. How about this:
>
>    Explicit congestion notification (ECN) allows a forwarding element
>    to notify downstream devices, including the destination, of the
>    onset of congestion without having to drop packets. This can
>    improve network efficiency through better flow control without
>    packet drops. This document extends ECN to TRILL switches,
>    including integration with IP ECN, and provides for ECN marking in
>    the TRILL Header Extension Flags Word (see RFC 7179).
>
>> 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.
>
> OK.
>
>> 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
>
> I'm not sure how much explanation/definition needs to appear in this
> document for bits that are irrelevant to the document. All of your "no
> expansion found in document" comments are true and probably a bit more
> should be said but the draft does, in connection with bits other than
> those it specifies, say
>
>    See [RFC7780] and [RFC7179] for the meaning of the other
>    bits.
>
> The format seems moderately clear and is the same as in Section 10.2
> of RFC 7780. Adding a mention that the TRILL Header Extension Flags
> Word is 32-bits and the bit numbers are across the top might
> help. Also, for those wishing to see a more tabular presentation, a
> pointer to the IANA Registry
> http://www.iana.org/assignments/trill-parameters/trill-parameters.xhtml#extended-header-flags
> might help.
>
>> 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.
>
> At the beginning of Section 2, it says "a two-bit TRILL-ECN
> field". Since the name of the TRILL Header "Extension Flags Word" is
> now embedded in multiple RFCs, I don't think that should be changed
> but I wouldn't have any problem reviewing the draft to see that
> multi-bit regions of that 32-bit word are consistently referred to as
> fields.
>
>> 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.
>
> That distinction between ECT(0) and ECT(1) only applies if the PCN
> variant of ECN is in use which is why the text you quote is in a
> subsection of Section 4 (TRILL Support for ECN Variants). There is a
> reference to RFC 6660 which talks about this. Perhaps we can make that
> clearer.
>
>> 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.
>
> This distincition is in RFC 6660 on PCN (Pre-Congestion Notification).
>
>> 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)..."
>
> OK.
>
>> 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?
>
> Because the meaning of the other three code points is the same in
> TRILL and IP.
>
>> ECN SUpport
>> -----------
>>
>> The first paragraph has "logically" at three places, what would be the
>> difference if these "logically" are dropped?
>
> Probably not. I think that word was included to be careful not to
> overly constrain the order of processing within a TRILL switch.
>
>> 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:"
>
> OK.
>
>> 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".
>
> I think "set to zero" is clearer.
>
>> First line section 3,2
>> ----------------------
>> s/ahow/shown
>> Caveat I normally don't English grammar reviews, but sometimes I can't stop
>> myself :)
>
> OK.
>
>> Second line first main bullet in section 3.2
>> --------------------------------------------
>>
>> I prefer the format "guidelines in RFC 7567 [RFC7567]"
>
> I really don't see any benefit to that and I think "RFC xxxx
> [RFCxxxx]" is not used in TRILL RFCs.
>
>> 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);
>
> I agree with your earlier comment that "field" is generally better.
>
>> 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."
>
> Yes, although it might be even clearer in the first line to say
> "... ECN, that RBridge will ..."
>
>> TRILL Support for ECN Variants
>> ------------------------------
>> The second 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].
>
> Interestingly, although RFC Editor meta-data consistencly shows RFC
> 4301 as updating RFC 3168, this is not indicated on the first page of
> RFC 4301. While ECN is mentioned a number of times in RFC 4301 and
> that RFC could be viewed as extending ECN, I'm not quite sure what, if
> any, change RFC 4301 makes to the behavior standardized RFC in 3168.
>
> I'll look at this more closely but I think the text currently in the
> draft is OK and need not refer to either RFC 4301 or RFC 6040.
>
>> Nits:
>>
>> Codepoints
>> ----------
>> at several places "codepoint(s)" I think the words IANA and the
>> RFC Editor use is "code point(s)"
>
> While I didn't to an exhaustive search, everywhere I looked in the ECN
> and PCN RFCs, "codepoint" is used so I would prefer to leave this
> unchanged for now.
>
> Thanks,
> Donald
> ===============================
>  Donald E. Eastlake 3rd   [phone] (cell)
>  155 Beaver Street, Milford, MA 01757 USA
>  [e-mail]
>
>> /Loa
>> --
>>
>> Loa Andersson                        email: [e-mail]
>> Senior MPLS Expert                          [e-mail]
>> Huawei Technologies (consultant)     phone: [phone]

-- 


Loa Andersson                        email: [e-mail]
Senior MPLS Expert                          [e-mail]
Huawei Technologies (consultant)     phone: [phone]