Dale, thank you for your review. Lou, thanks for addressing the review comments. I entered a DISCUSS ballot on a process point not raised here.
Alissa > On Apr 3, 2019, at 10:38 PM, Lou Berger <lber...@labn.net> wrote: > > Hi, > > Thanks you for the comments, please see below. > > On 3/20/2019 10:33 PM, Dale Worley via Datatracker wrote: >> Reviewer: Dale Worley >> Review result: Ready with Nits >> >> 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 >> >> <https://trac.ietf.org/trac/gen/wiki/GenArtfaq>. >> >> Document: draft-ietf-manet-dlep-pause-extension-05 >> Reviewer: Dale R. Worley >> Review Date: 2019-03-20 >> IETF LC End Date: 2019-04-03 >> IESG Telechat date: [not scheduled] >> >> Summary: >> >> This draft is basically ready for publication, but has nits >> that should be fixed before publication. There are a number of >> issues with technical content, but they appear to stem from >> editorial issues, rather than being unsettled technical >> decisions. >> >> * Technical issues: >> >> I notice that the Pause Extension cannot be used by a router to tell a >> modem to not send. I assume that the WG has considered this and has a >> good reason for this asymmetry. > > This is correct. It was deemed unneeded complexity for the typical use case > where modems have very narrow bandwidth available for transmissions in > comparison to other router attached links. > >> >> 3.1.1. Queue Parameter Sub Data Item >> >> There need not be a Sub Data Item for a particular queue index. Such >> a queue has no declared size. OTOH, it has no DSCPs, and so no >> traffic can be sent for it, either. >> >> Queue Parameter Sub Data Items are an unordered list composed of sub >> data items with a common format. The first sub data item is assigned >> a Queue Index value of 1, and subsequent data items are numbered >> incrementally. The format of the Queue Parameter Sub Data Item is >> >> ... >> >> Queue Index: >> >> An 8-bit field indicating the queue index of the queue parameter >> represented in the sub data item. >> >> These passages are contradictory. Either the Sub Data Items are an >> ordered list and indexes are assigned to them sequentially, they are >> unordered and the indexes are given in the Queue Index subfield, or >> the Sub Data Items are required to be in order by their Queue Index >> fields. > > Good catch - and you are correct. This is text wasn't modified when we made > ordering explicit. > > The following will be removed: > > The first sub data item is assigned > a Queue Index value of 1, and subsequent data items are numbered > incrementally. > > >> DS Field Qn: >> >> The data item contains a sequence of 8 bit DS Fields. The >> position in the sequence identifies the associated queue index. >> The number of DS Fields present MUST equal the sum of all Num >> DSCPs field values. >> >> This doesn't seem to match the defined format of the Sub Data Items. >> The "DS Field Qn" fields contain exactly as many DS Fields as the >> value of the Num DSCPs Qn field by definition. And all of them are >> associated with the one Queue Index in the Sub Data Item containing >> the DS Field Qn. > > Same issue. Drop: > > The > position in the sequence identifies the associated queue index. > >> I note that the Sub Data Items are not padded to a multiple of 4 >> octets. I assume this is intended. > > Correct, this is consistent with rfc8175. > > >> >> 3.3. Restart >> >> The sending of this data item parallels the Pause Data Item, see the >> previous section, and follows the same rules. This includes that to >> indicate that transmission can resume to all destinations, a modem >> MUST send the Restart Data Item in a Session Update Message. It also >> includes that to indicate that transmission can resume to a >> particular destination a modem MUST send the Pause Restart Item in a >> Destination Update Message. >> >> Read literally, this means that there is a pause/transmit bit for each >> destination/DSCP combination, and that the various messages (pause >> vs. restart * Session Update vs. Destination Update * queue index >> vs. 255) set some subset of the bits to "pause" or "transmit". This >> is opposed to the model where (to simplify) there is an overall >> pause/transmit bit for all traffic and an independent pause/transmit >> bit for each destination, and traffic may be sent for a destination >> only if both the overall bit and the destination bit are "transmit". >> >> * Editorial issues: >> >> 1. Introduction >> >> The >> extension also optionally supports DSCP (differentiated services >> codepoint) aware, see [RFC2475], flow control. >> >> The phrasing of this sentence is awkward because of the number of >> interpolated phrases. I suggest something like: >> >> The extension also optionally supports flow control that is DSCP >> (differentiated services codepoint) aware (see [RFC2475]). > > I'll break it into two sentences. > > >> Also, it seems that recent RFCs have tended to capitalize the phrase >> "Differentiated Services Code Point". (Probably check this point with >> the Editor.) > I'll defer to the RFC editor. >> Note >> that this mechanism only controls traffic that is to be transmitted >> on the modem's attached data channel and not to DLEP control messages >> themselves. >> >> The parallelism is not correct here, as it would read "this mechanism >> ... controls ... to DLEP". Perhaps change to: "this mechanism only >> applies to traffic ...". > > Thanks! > > >> 2. Extension Usage and Identification >> >> To indicate that the Control Plane Based Pause >> Extension is to be used, an implementation MUST include the Control >> Plane Based Pause Extension Type Value in the Extensions Supported >> Data Item. >> >> If I am reading RFC 8175 correctly, this is not exactly true. Sending >> the Value does not compel the peer device to use the Extension. >> Instead, "To indicate that the implementation accepts use of the >> C.P.B.P.E., an implementation includes ...". > > thanks, but will s/accepts/supports. > >> 3. Extension Data Items >> >> The Queue Parameters >> Data Item is used by a modem to provide information on the DSCPs it >> uses in forwarding. >> >> Suggest s/information on/information about/. To me, "information on >> the DSCPs" suggests that it will provide a listing of all the DSCPs >> that it uses, whereas "information about the DSCPs" suggests that it >> will provide attributes of some, but perhaps not all, of the DSCPs. >> (Section 3.1 makes clear that the Queue Parameters does not need to >> list all DSCPs that are used.) > > Done! > > >> >> 3.1. Queue Parameters >> >> The Queue Parameters Data Item identifies DSCPs based on groups of >> logical queues, each of which is referred to via a "Queue Index". >> >> "groups of logical queues" isn't correct. Suggest "The Queue >> Parameters Data Item groups DSCPs into logical queues, each of which >> is identified by a "Queue Index"." > I like it - thanks! >> >> An implementation that does not support DSCPs would indicate 1 queue >> with 0 DSCPs, and the number of bytes that may be in its associated >> link transmit queue. >> >> "with 0 DSCPs" is not really correct, since the Queue Parameter >> doesn't specify how many DSCPs are included in queue index 0, and >> traffic with all values of the DSCP field (which is unexamined by the >> device) will be in queue index 0. I think this phrase should be >> omitted. > > I think it's cleaner to remove Q0 at this point, it too is a bit vestigial > and I think this highlights that having essentially two ways to cover the > > >> Value Scale >> ------------ >> 0 B - Bytes (Octets) >> 1 KB - Kilobytes (B/1024) >> 2 MB - Megabytes (KB/1024) >> 3 GB - Gigabytes (MB/1024) >> >> I would expect these items to be "1024 B", "1024 KB", etc. But >> perhaps that's because it is ambiguous whether "scale" means the units >> in which the measurement is expressed, or the factor by which the >> measurement is multiplied by before it is reported. I would replace >> "scale" with "unit", which does not have that ambiguity. I would also >> use the IEC abbreviations: >> >> Value Unit >> ----------- >> 0 B - Bytes (Octets) >> 1 KiB - Kilobytes (1024 B) >> 2 MiB - Megabytes (1024 KiB) >> 3 GiB - Gigabytes (1024 GiB) > > Sure. > > >> [ISQ-13] International Electrotechnical Commission, "International >> Standard -- Quantities and units -- Part 13: Information >> science and technology", IEC 80000-13, March 2008. >> >> 3.2. Pause >> >> The special value of 255 is used to >> indicate that all traffic is to be suppressed. >> >> This implies that a queue index of 255 cannot be used, which is also >> implied by the fact that Num Queues = 255 means that only indexes 0 to >> 254 are in use. It would be helpful to update 3.1 to make this more >> explicit: >> >> Queue Indexes are numbered sequentially from zero to a maximum of >> 254, where queue index zero is a special case covering DSCPs which >> are not otherwise associated with Queue Index. (Value 255 is used >> to indicate "all queues".) > I'll add text that 255 MUST NOT be used in the Queue Index field. >> >> -- >> >> Queue Index: >> >> ... The special value of 255 indicates >> all traffic is to be suppressed to the modem, when the data item >> is carried in a Session Update Message, or a destination, when the >> data item is carried in Destination Update Message. >> >> The parallelism is awkward here. I think you want to explicitly >> parallel "traffic to the modem" and "traffic to a destination": > okay, will make them parallel. >> The special value of 255 indicates all traffic to the modem is >> to be suppressed, when the data item is carried in a Session >> Update Message, or all traffic to a destination, when the data >> item is carried in Destination Update Message. >> >> 3.3. Restart >> >> Finally, the same rules apply to queue indexes. >> >> Probably better as "Queue indexes are interpreted in the same way as >> in the Pause Data Item." > > Done. > > Look for an update to be published shortly. > > Thank you for the comments! > > Lou > >> [END] >> >> >> > > _______________________________________________ > Gen-art mailing list > Gen-art@ietf.org <mailto:Gen-art@ietf.org> > https://www.ietf.org/mailman/listinfo/gen-art > <https://www.ietf.org/mailman/listinfo/gen-art>
_______________________________________________ Gen-art mailing list Gen-art@ietf.org https://www.ietf.org/mailman/listinfo/gen-art
