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

[Lou Berger <[e-mail]>]

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]
>
>
>