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

From: Brian Carpenter <[email-masked]>
Date: Fri, 23 Jun 2017 18:32:55 -0700

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?

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

--

---

From: Donald Eastlake <[email-masked]>
Date: Mon, 26 Jun 2017 23:02:33 -0400

Hi Brian,

Thanks for the comments. See below.

> Summary: Ready with issues

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,

>    originatingSNPBufferSize APPsub-TLV occurring in any other fragment

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

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

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

>          [(lowerBound + upperBound)/2], rounded up to the nearest integer.

OK.

>          upperBound)/2], rounded up to the nearest integer.

That seems reasonable.

Thanks,
Donald
===============================
 Donald E. Eastlake 3rd   +1-508-333-2270 (cell)
 155 Beaver Street, Milford, MA 01757 USA
 [email-masked]

---

From: "Zhangmingui (Martin)" <[email-masked]>
Date: Tue, 27 Jun 2017 08:06:53 +0000

Hi Brian and Donald,

Thanks a lot for the comments. Please see the responses as inline below.

> -----Original Message-----
> From: Donald Eastlake [mailto:[email-masked]]
> Sent: Tuesday, June 27, 2017 11:03 AM
> To: Brian Carpenter
> Cc: [email-masked] Review Team; [email-masked];
> [email-masked]
> Subject: Re: Genart last call review of draft-ietf-trill-mtu-negotiation-05
> 
> Hi Brian,
> 
> > 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.
> >
> > 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,

[Mingui] OK.

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

[Mingui] OK.

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

[Mingui] As specified in the text leading the Steps, "linkMtuSize" is a local integer variable for a specific RBridge while "link MTU size" is not a variable but a value that is agreed by two connected RBridges. To avoid the confusion, I changed "linkMtuSize" to "X" and add the text to explain that link MTU size is a value that is agreed by two connected RBridges.

> > but that should be made clear to the reader.

[Mingui] Agree. Added the word "agreed" in Section 2.

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

[Mingui] OK.

> > remember the rounding rule:

[Mingui] Changed three occurrences in the document.

>  155 Beaver Street, Milford, MA 01757 USA  [email-masked]

Thanks,
Mingui

---

From: Brian E Carpenter <[email-masked]>
Date: Wed, 28 Jun 2017 08:58:01 +1200

Thanks, all those changes seem good to me. If I am asked to review
the next version for the IESG telechat, I expect to say "Ready".

Regards
   Brian