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

Request Review of draft-ietf-trill-mtu-negotiation
Requested rev. no specific revision (document currently at 08)
Type Last Call Review
Team General Area Review Team (Gen-ART) (genart)
Deadline 2017-06-28
Requested 2017-06-14
Draft last updated 2017-06-23
Completed reviews Rtgdir Early review of -02 by Julien Meuric (diff)
Secdir Last Call review of -05 by Vincent Roca (diff)
Genart Last Call review of -05 by Brian Carpenter (diff)
Genart Telechat review of -06 by Brian Carpenter (diff)
Tsvart Telechat review of -06 by Magnus Westerlund (diff)
Assignment Reviewer Brian Carpenter
State Completed
Review review-ietf-trill-mtu-negotiation-05-genart-lc-carpenter-2017-06-23
Reviewed rev. 05 (document currently at 08)
Review result Ready with Issues
Review completed: 2017-06-23

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.

--