Re: [manet] [Gen-art] Genart last call review of draft-ietf-manet-dlep-pause-extension-05

[Alissa Cooper <[e-mail]>]

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 <[e-mail]> 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
> [e-mail] <mailto:[e-mail]>
> https://www.ietf.org/mailman/listinfo/gen-art <https://www.ietf.org/mailman/listinfo/gen-art>