Hi Loa,
Thanks for your comments. See below
On Sat, Jan 21, 2017 at 12:02 AM, Loa Andersson <[email protected]> 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 +1-508-333-2270 (cell)
155 Beaver Street, Milford, MA 01757 USA
[email protected]
/Loa
--
Loa Andersson email: [email protected]
Senior MPLS Expert [email protected]
Huawei Technologies (consultant) phone: +46 739 81 21 64
