Last Call Review of draft-ietf-manet-dlep-pause-extension-05
review-ietf-manet-dlep-pause-extension-05-genart-lc-worley-2019-03-20-00

Request Review of draft-ietf-manet-dlep-pause-extension
Requested rev. no specific revision (document currently at 08)
Type Last Call Review
Team General Area Review Team (Gen-ART) (genart)
Deadline 2019-04-03
Requested 2019-03-14
Draft last updated 2019-03-20
Completed reviews Rtgdir Last Call review of -05 by Andy Smith (diff)
Secdir Last Call review of -05 by Stephen Kent (diff)
Genart Last Call review of -05 by Dale Worley (diff)
Tsvart Last Call review of -06 by Bob Briscoe (diff)
Assignment Reviewer Dale Worley
State Completed
Review review-ietf-manet-dlep-pause-extension-05-genart-lc-worley-2019-03-20
Reviewed rev. 05 (document currently at 08)
Review result Ready with Nits
Review completed: 2019-03-20

Review
review-ietf-manet-dlep-pause-extension-05-genart-lc-worley-2019-03-20

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.

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.

   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.

I note that the Sub Data Items are not padded to a multiple of 4
octets.  I assume this is intended.

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

Also, it seems that recent RFCs have tended to capitalize the phrase
"Differentiated Services Code Point".  (Probably check this point with
the 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 ...".

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 ...".

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.)

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"."

   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.

         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)

   [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".)

--

   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":

      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."

[END]