Hi, Donald, On 6/25/2017 5:07 PM, Donald Eastlake wrote: > Hi Magnus, > > Thanks for the extensive review. See my responses below. > > On Thu, Jun 15, 2017 at 1:32 PM, Magnus Westerlund > <magnus.westerlund@xxxxxxxxxxxx> wrote: >> Reviewer: Magnus Westerlund >> Review result: Not Ready >> >> Early review of draft-ietf-trill-over-ip-10 >> Reviewer: Magnus Westerlund >> Review result: Not Ready >> >> TSV-ART review comments: >> >> I have set this to not ready as there are several issues, some significant that >> could affect the protocol realization significantly. Some may be me missing >> things in TRILL, I was not that familiar with it before this review and I have >> only tried looking up things, not reading the whole earlier specifications. So >> don't hesitate to push back and provide pointers to things that can resolve >> issues. The authors and the WG clearly have thought about a lot of issues and >> dealt with much already. > OK. Hopefully we can resolve these one way or the other. > > ... >> TCP Encapsulation issue >> ----------------------- >> >> Section 5.6: >> >> The TCP encapsulation appear to be missing an delimiter format allowing each >> individual TRILL packet/payload to be read out of the TCP's byte stream. In >> other words, a normal implementation has no way of ensuring that the TCP >> payload starts with the start of a new TRILL payload. Multiple small TRILL >> payloads may be included in the same TCP payload, and also only parts as TCP is >> one way of dealing with TRILL packets that are larger than the IP+Encapsulation >> MTU that actually will work. >> >> This comment is based on that there appear to be no length fields included in >> the TRILL header. The most straight forward delimiter is a 2-byte length field >> for the TRILL payload to be encapsulated. > Right. It might also be useful to include some sort of check field, as > is done in BGP, to detect if you are out of sync in parsing the TCP > stream. There is nothing in BGP that ever assumes that TCP write boundaries are preserved. BGP uses markers and length fields to create message boundaries in TCP's bytestream. The same is needed here. Note that BGP also never claims to craft TCP packets by 'encapsulating' a BGP message in a TCP segment. That part of this document needs to be removed - it not how TCP is ever used. > Another point is that, while with UDP it seems fine to send packets > with assorted QoS, you don't want to encourage re-ordering of TCP > packets in a stream. So if TCP encapsulation is being used, Again - please, NO. NEVER use this term. > you want > to use the same DSCP value for the packets in a particular TCP stream. Again, this is nonsensical. TCP would set a DSCP for the connection, never in different ways for individual segments of a connection. > So, generally, you need to have a TCP connection per priority handling > category. Mapping the 8 priority levels into a smaller number of > handling categories is a normal thing to do so you certainly don't > necessarily need 8 TCP connections. Adding material on this should not > be too hard. Perhaps, but please - again, please - omit any mention or implication that this occurs via encapsulation. If you want to use TCP, please use it properly. > >> Section 5.6: >> >> TCP endpoint requirements. I do wonder if an application like TRILL actual >> would need to discuss performance impacting implementation choices or >> limitations. For example use of NAGLE, the requirements on buffer sizes in >> relation to Bandwidth delay products, as buffer memory in a RBridge will impact >> performance. > Well, I'm not sure how deeply this document should get into such > performance issues. What about just saying something about > consideration being given to tuning TCP for performance and pointing > to one or a few other RFCs that talk about this? Because your use of TCP (even if changed to describe it correctly) isn't listed in those TCP RFCs. And it's not so simple - NAGLE helps performance for interactive systems that use single-byte messages (e.g., telnet) and reduces the number of outstanding "less than full" segments. When used for encapsulation, turning NAGLE off is the right thing for multibyte messages (e.g., TRILL messages) and can avoid the "gathering" delay (200 ms stalls when there isn't enough source data - i.e., incoming TRILL packets - to keep up with the outgoing segments), but could also generate a large number of small segments (which can interfere with segment-based congestion control, vs. ABC). Unless you want a very poorly performing result, *THIS* is what you need to drill down into. Joe