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

[Donald Eastlake <[e-mail]>]

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]