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]
