Subject: [RTG-DIR] QA review of draft-ietf-trill-ecn-support

From: Loa Andersson <[email-masked]>
Date: Sat, 21 Jan 2017 13:02:00 +0800

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: [email-masked]
Senior MPLS Expert                          [email-masked]
Huawei Technologies (consultant)     phone: +46 739 81 21 64

---

From: "Susan Hares" <[email-masked]>
Date: Sat, 21 Jan 2017 06:10:41 -0500

Loa:

Thank you for providing this review to the authors.   

Sue Hares 
Trill co-chair

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

Authors,

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:

Is this logic really watertight? What if the packet is dropped because of a
checksum error?

Major Issues:

Minor Issues:

Abstract
--------

ECNencapGuide
-------------

TRILL Header
------------

Multi-bit flags
---------------

Bit 11 and 12
-------------

           Binary  Name     Meaning

Code Point 0b11
---------------

ECN SUpport
-----------

The sentence says:

I would say

cleared vs set to zero
----------------------

First line section 3,2
----------------------
s/ahow/shown

It says:

Egress ECN Support
------------------
First sentence:

SHould it be:

Nits:

Codepoints
----------

/Loa
-- 

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

---

From: Donald Eastlake <[email-masked]>
Date: Thu, 26 Jan 2017 17:09:26 -0500

Hi Loa,

Thanks for your comments. See below

On Sat, Jan 21, 2017 at 12:02 AM, Loa Andersson <[email-masked]> wrote:
> 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.

> Last paragraph of the Introduction

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.

>    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

OK.

> The ECN Specific Extended Header Flags

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

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

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.

> I think a reference would be good.

This distincition is in RFC 6660 on PCN (Pre-Congestion Notification).

> Code Point 0b11

OK.

> 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

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

OK.

> cleared vs set 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 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

I agree with your earlier comment that "field" is generally better.

> Egress ECN Support

Yes, although it might be even clearer in the first line to say
"... ECN, that RBridge will ..."

> The second sentence of section four says:
>

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.

> Codepoints

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

---

From: Loa Andersson <[email-masked]>
Date: Sat, 28 Jan 2017 16:44:40 +0800

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

--