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

["Susan Hares" <[e-mail]>]

Loa:

Thank you for providing this review to the authors.   

Sue Hares 
Trill co-chair

-----Original Message-----
From: trill [mailto:[e-mail]] On Behalf Of Loa Andersson
Sent: Saturday, January 21, 2017 12:02 AM
To: [e-mail]; [e-mail];
[e-mail]
Cc: 'Yemin (Amy'; [e-mail]
Subject: [trill] QA review of draft-ietf-trill-ecn-support

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]

_______________________________________________
trill mailing list
[e-mail]
https://www.ietf.org/mailman/listinfo/trill