Re: [trill] Adam Roach's Discuss on draft-ietf-trill-ecn-support-05: (with DISCUSS and COMMENT)

[Donald Eastlake <[e-mail]>]

Hi Adam,

On Wed, Feb 7, 2018 at 8:12 PM, Adam Roach <[e-mail]> wrote:
> Adam Roach has entered the following ballot position for
> draft-ietf-trill-ecn-support-05: Discuss
>
> ...
>
> ----------------------------------------------------------------------
> DISCUSS:
> ----------------------------------------------------------------------
>
> Thanks to the authors, chairs, shepherd, and working group for the effort
that
> has been put into this document.
>
> I have concerns about some ambiguity and/or self-contradiction in this
> specification, but I suspect these should be easy to fix. In particular,
the
> behavior defined in Table 3 doesn't seem to be consistent with the
behavior
> described in the prose.
>
> For easy reference, I've copied Table 3 here:
>
>>       +---------+----------------------------------------------+
>>       | Inner   |  Arriving TRILL 3-bit ECN Codepoint Name     |
>>       | Native  +---------+------------+------------+----------+
>>       | Header  | Not-ECT | ECT(0)     | ECT(1)     |     CE   |
>>       +---------+---------+------------+------------+----------+
>>       | Not-ECT | Not-ECT | Not-ECT(*) | Not-ECT(*) |  <drop>  |
>>       |  ECT(0) |  ECT(0) |  ECT(0)    |  ECT(1)    |     CE   |
>>       |  ECT(1) |  ECT(1) |  ECT(1)(*) |  ECT(1)    |     CE   |
>>       |    CE   |      CE |      CE    |      CE(*) |     CE   |
>>       +---------+---------+------------+------------+----------+
>>
>>                      Table 3. Egress ECN Behavior
>>
>>  An asterisk in the above table indicates a currently unused
>>  combination that SHOULD be logged. In contrast to [RFC6040], in TRILL
>>  the drop condition is the result of a valid combination of events and
>>  need not be logged.
>
> The prose in this document indicates:
>
>  1. Ingress gateway either copies the native header value to the TRILL ECN
>     codepoint (resulting in any of the four values above) or doesn't
insert
>     any ECN information in the TRILL header.
>
>  2. Intermediate gateways can set the CCE flag, resulting in "CE" in the
>     table above.
>
> Based on the above, a packet arriving at an egress gateway can only be in
one of
> the following states:
>
>  A. TRILL header is Not-ECT because no TRILL node inserted ECN
information.
>
>  B. TRILL header value == Native header value because the ingress gateway
>     copied it from native to TRILL.
>
>  C. TRILL header is "CE" because an intermediate node indicated
congestion.

Sort of... But note that the TRILL header ECN bit s can indicate non-ECT
while the CCE bit is set making the overall TRILL Header "CE".

> If that's correct, I would think that any state other than those three
needs
> to be marked with an (*). In particular, these two states fall into that
> classification, and seem to require an asterisk:
>
>   - Native==CE && TRILL==ECT(0)
>
>   - Native==ECT(0) && TRILL==ECT(1)

I would defer to my co-author Bob Briscoe but it looks to me like you have
a good point.

> In addition, the behavior this table defines for Native==ECT(0) &&
TRILL==ECT(1)
> is somewhat perplexing: for this case, the value in the TRILL header takes
> precedence; however, when Native==ECT(1) && TRILL==ECT(0) the Native
header
> takes precedence. Or, put another way, this table defines ECT(1) to always
> override ECT(0). I don't find any prose in here to indicate why this
needs to be
> treated differentially, so I'm left to conclude that this is a
typographical
> error. If that's not the case, please add motivating text to Table 3
explaining
> why ECT(1) is treated differently than ECT(0) for baseline ECN behavior.

As noted in Section 4.1, there is an ECN variation where ECT(0) just
indicates ECT while ECT(1) indicates congestion of a lesser severity level
has been encountered than that indicated by CE. I believe the dominance of
ECT(1) over ECT(0) is designed to not interfere with this variation.

> ----------------------------------------------------------------------
> COMMENT:
> ----------------------------------------------------------------------
>
> I also have a small handful of editorial suggestions and nits to propose.

I am fine with all the changes below .

Thanks,
Donald
=============================
 Donald E. Eastlake 3rd   [phone] (cell)
 155 Beaver Street, Milford, MA 01757 USA
 [e-mail]

> Please expand "TRILL" upon first use and in the title; see
> https://www.rfc-editor.org/materials/abbrev.expansion.txt for guidance.
>
>
---------------------------------------------------------------------------
> §1:
>
>>  In [RFC3168] it was recognized that tunnels and lower layer protocols
>
> "In [RFC3168], it was..."
> (insert comma)
>
>
---------------------------------------------------------------------------
>
> §2:
>
>>  These fields are show in Figure 2 as "ECN" and "CCE". The TRILL-ECN
>
> "...are shown..."
>
>
>>  The CRItE bit is the critical Ingress-to-Egress summary
>>  bit and will be one if and only if any of the bits in the CItE range
>>  (21-26) is one or there is a critical feature invoked in some further
>
> "...if any of the bits... are one or..."
> (replace "is" with "are")
>
>
>>   The first three have the same meaning as the corresponding ECN field
>>   codepoints in the IPv4 or IPv6 header as defined in [RFC3168].
>
> Section 1.1 defines "IP" to mean both IPv4 and IPv6. It would seem
cleaner and
> easier to read if the document were to leverage that definition here.
>
>
>>   However codepoint 0b11 is called Non-Critical Congestion Experienced
>
> "However, codepoint..."
> (insert comma)
>
>
---------------------------------------------------------------------------
>
> §3.3.2:
>
>>  If an RBridge supports ECN, for the two cases of an IP and a non-IPR
>
> "...non-IP"
>
>
---------------------------------------------------------------------------
>
> §4:
>
>>  Section 3 specifies interworking between TRILL and the original
>>  standardized form of ECN in IP [RFC3168].
>
> Please indicate this at the top of Section 3. When I was puzzling over
Table 3,
> I spent some time trying to figure out whether the behavior I describe in
my
> DISCUSS above was due to behavior described in RFC 8311 or the
experiments it
> contemplates.
>
>
---------------------------------------------------------------------------
>
> Appendix A:
>
>>  o  the meaning of CE markings applied by an L4S queue is not the same
>>     as the meaning of a drop by a "Classic" queue (contrary to the
>>     original requirement for ECN [RFC3168]).
>
> I think, when citing this exception, it makes much more sense to point to
RFC
> 8311 (where the exception to RFC 3168's requirement is defined) than to
RFC 3168
> in a vacuum.
>
>>     Instead the likelihood
>
> Insert a comma after "Instead".
>
>>     that the Classic queue drops packets is defined as the square of
>>     the likelihood that the L4S queue marks packets (e.g. when there
>
> Insert a comma after "e.g.,"