Last Call Review of draft-ietf-trill-mtu-negotiation-05
review-ietf-trill-mtu-negotiation-05-genart-lc-carpenter-2017-06-23-00

• Status
• Email expansions
• History

• Versions
• 00

Review
review-ietf-trill-mtu-negotiation-05-genart-lc-carpenter-2017-06-23

Gen-ART Last Call review of draft-ietf-trill-mtu-negotiation-05

I am the assigned Gen-ART reviewer for this draft. The General Area
Review Team (Gen-ART) reviews all IETF documents being processed
by the IESG for the IETF Chair.  Please treat these comments just
like any other last call comments.

For more information, please see the FAQ at
<http://wiki.tools.ietf.org/area/gen/trac/wiki/GenArtfaq>.

Document: draft-ietf-trill-mtu-negotiation-05.txt
Reviewer: Brian Carpenter
Review Date: 2017-06-24
IETF LC End Date: 2017-06-28
IESG Telechat date: 2017-07-06

Summary: Ready with issues
--------

Minor issues: 
-------------

> 2. Link-Wide TRILL MTU Size
...
>   ...RBridges MUST support the Extended L1 Circuit-Scoped
>   (E-L1CS) flooding scope LSP (FS-LSP). They use that flooding to
>   exchange their maximally supportable value of "Lz".

Where does that value come from? Is it configured, derived from
the interface in some way, or discovered?

> 2.1. Operations
>
>   Lz is reported using a originatingSNPBufferSize TLV that MUST occur
>   in fragment zero of the RBridge's E-L1CS FS-LSP. An
>   originatingSNPBufferSize APPsub-TLV occurring in any other fragment
>   is ignored.

Is that really what you mean? I thought Lz was an optional extra. So I think
you mean:

2.1. Operations

   Lz MAY be reported using a originatingSNPBufferSize TLV that occurs
   in fragment zero of the RBridge's E-L1CS FS-LSP. An
   originatingSNPBufferSize APPsub-TLV occurring in any other fragment
   MUST be ignored.

> 3. Link MTU Size Testing
...
>   Step 0:
...
>      b) Link MTU size is set to 1470, lowerBound is set to 1470,
>         upperBound is set to the link-wide Lz, linkMtuSize is set to
>         [(lowerBound + upperBound)/2] (Operation "[...]" returns the
>         fraction-rounded-up integer.).  

This is confusing. "linkMtuSize" was defined as a local variable.
But what is "Link MTU size"? Is that another local variable?
If so, how is it different from "linkMtuSize"?
It is used again in part 2) of step 2 below.

Also, I assume "Lz" is the value previously agreed among the nodes,
but that should be made clear to the reader.

Nits:
-----

> 1. Introduction
...
>   topology. While in this document, a new RECOMMENDED link-wide minimum
>   inter-RBridge MTU size, Lz, is specified. By calculating a using Lz
>   as specified herein, link-scoped PDUs can be formatted greater than
>   the campus-wide Sz up to the link-wide minimum acceptable inter-
>   RBridge MTU size potentially improving the efficiency of link
>   utilization and speeding link state convergence.

I cannot parse those two sentences. What does the "While" refer to? 
What does "By calculating a using Lz" mean?

> 3. Link MTU Size Testing
...
>      b) Link MTU size is set to 1470, lowerBound is set to 1470,
>         upperBound is set to the link-wide Lz, linkMtuSize is set to
>         [(lowerBound + upperBound)/2] (Operation "[...]" returns the
>         fraction-rounded-up integer.).  

This would be easier to understand:

3. Link MTU Size Testing
...
      b) Link MTU size is set to 1470, lowerBound is set to 1470,
         upperBound is set to the link-wide Lz, linkMtuSize is set to
         [(lowerBound + upperBound)/2], rounded up to the nearest integer.

Repeat this in the following two cases; a normal reader will not
remember the rounding rule:

...
   1) If RB1 fails to receive an MTU-ack from RB2 after k tries: 

         upperBound is set to linkMtuSize and linkMtuSize is set to
         [(lowerBound + upperBound)/2], rounded up to the nearest integer.

   2) If RB1 receives an MTU-ack to a probe of size linkMtuSize from
      RB2:

         link MTU size is set to linkMtuSize, lowerBound is set to
         linkMtuSize and linkMtuSize is set to [(lowerBound +
         upperBound)/2], rounded up to the nearest integer.

--