Hi Brian, Thanks for the comments. See below.
On Fri, Jun 23, 2017 at 9:32 PM, Brian Carpenter <[email protected]> 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 +1-508-333-2270 (cell) 155 Beaver Street, Milford, MA 01757 USA [email protected] _______________________________________________ trill mailing list [email protected] https://www.ietf.org/mailman/listinfo/trill
