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

[Donald Eastlake <[e-mail]>]

Hi Loa,

The -02 version of draft-ietf-trill-ecn-support, just posted, included the
improvements we have discussed.

Thanks,
Donald
===============================
 Donald E. Eastlake 3rd   [phone] (cell)
 155 Beaver Street, Milford, MA 01757 USA
 [e-mail]

On Sat, Jan 28, 2017 at 3:44 AM, Loa Andersson <[e-mail]> wrote:

> 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]
>