Re: [trill] Genart last call review of draft-ietf-trill-mtu-negotiation-05

[Donald Eastlake <[e-mail]>]

Hi Brian,

Thanks for the comments. See below.

On Fri, Jun 23, 2017 at 9:32 PM, Brian Carpenter
<[e-mail]> wrote:
> Reviewer: Brian Carpenter
> Review result: Ready with Issues
>
> 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?

It's somewhat similar to the originatingL1LSPBufferSize which is
talked about in Section 5 of RFC 7780, except that there is no reason
to worry about coordinating across the TRILL campus. How about adding
wording something like:

      The originatingSNPBufferSize for a port is the minimum of the
following two quantities, but not less than 1470 bytes: (1) the
maximum MTU of the port and (2) the maximum LSP size that the TRILL
IS-IS implementation can handle,

>> 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.

Yes, the "MUST" was just in reference to being in fragment zero, not
that it has to occur, so your wording seems better.

>> 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.

I don't want to say anything about that before checking with the primary author.

> 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?

I believe the sentences should be

"... In this document, a new RECOMMENDED link-wide minimum
inter-RBridge MTU size, Lz, is specified. By calculating and using Lz
as specified herein, ..."

>> 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.

OK.

> 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.

That seems reasonable.

> --

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