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

[Loa Andersson <[e-mail]>]

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: [e-mail]
Senior MPLS Expert                          [e-mail]
Huawei Technologies (consultant)     phone: [phone]