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

[Brian E Carpenter <[e-mail]>]

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

On 27/06/2017 20:06, Zhangmingui (Martin) wrote:
> Hi Brian and Donald,
> 
> Thanks a lot for the comments. Please see the responses as inline below.
> 
>> -----Original Message-----
>> From: Donald Eastlake [mailto:[e-mail]]
>> Sent: Tuesday, June 27, 2017 11:03 AM
>> To: Brian Carpenter
>> Cc: [e-mail] Review Team; [e-mail];
>> [e-mail]
>> Subject: Re: Genart last call review of draft-ietf-trill-mtu-negotiation-05
>>
>> 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,
> 
> [Mingui] OK.
> 
>>
>>>> 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.
> 
> [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:
>>> ...
>>>>      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.
> 
> [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.
> 
>>
>>> Also, I assume "Lz" is the value previously agreed among the nodes,
>>> but that should be made clear to the reader.
> 
> [Mingui] Agree. Added the word "agreed" in Section 2.
> 
>>>
>>> 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, ..."
> 
> [Mingui] OK.
> 
>>
>>>> 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:
> 
> [Mingui] Changed three occurrences in the document.
> 
> 
>>>
>>> ...
>>>    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]
> 
> 
> 
> Thanks,
> Mingui
>