Re: [manet] WGLC comments on draft-ietf-manet-dlep-credit-flow-control

[Lou Berger <[e-mail]>]

Hi Ronald,

Sorry for not getting this out earlier.  See below for inline responses.

On 7/12/2021 1:44 PM, Velt, R. (Ronald) in 't wrote:
>
> Hi Authors, MANET WG participants,
>
> At long last, here is my review of 
> draft-ietf-manet-dlep-credit-flow-control (with chair mask off). 
> Reviews of the other flow control related I-Ds in WGLC will follow 
> shortly.
>
> Thanks,
>
> Ronald
>
> --
>
> Major Issues
>
> ============
>
> None
>
> Minor Issues
>
> ============
>
> Section 1, Introduction, contains the somewhat puzzling sentence 
> "Traffic sent by a router will use traffic flow classification 
> information provided by the modem as defined in 
> [I-D.ietf-manet-dlep-traffic-classification] to identify which traffic 
> is associated with each credit window." I would think it is the 
> router, as opposed to the traffic, that uses the classification 
> information.
>
Very True!
>
> The traffic, which may originate from an upstream source, is what it 
> is. But then the question is HOW the router should make use of the 
> classification information. Should it e.g. re-mark certain traffic 
> flows (i.e., alter their DSCPs) to ensure that they receive 
> preferential treatment (i.e., are throttled last in case of 
> congestion)? Can the router assume that the modem understands Per-Hop 
> Behaviors? In my opinion, at the very least the sentence quoted above 
> should be rephrased; in addition some further clarification on the 
> router-side mechanism foreseen could be helpful.
>
How about: s/Traffic sent by a router will use/A router will use

> Section 2.1, Data Plane Considerations, states: "Note that routers 
> will typically view a DLEP destination as the next hop MAC address." 
> Does this still hold true, now that we have the Link Identifier 
> extension as specified in RFC8703?
>
Sure -- assuming most implementations don't support 8703. Perhaps it's 
just best to remove this sentence given 8703. (I looked at adding 
reverences to 8703 and decided the resulting text was more confusing 
then helpful.)


> Both the Credit Window Initialization Data Item (section 2.3.1) and 
> the Credit Window Status Data Item (section 2.3.4) have a field named 
> Credit Window Size, but these have different meanings: in the former 
> case, it represents a maximum window size, whereas in the latter case, 
> it holds the actual (current) window size. I would suggest renaming 
> either of these, e.g. to Credit Window Limit in the Credit Window 
> Initialization Data Item or to Current Credit Window Size in the 
> Credit Window Status Data Item.
>
I'll change it to Current Credit Window Size


> A special Flow Identifier value of 0xFFFF, indicating all FIDs, is 
> defined in the specification of the Credit Window Request Data Item. 
> Should it be stated, that if this special value is used, it MUST be 
> the only FID present in the Data Item?
>
I can go either way.  Following the principle of 'be liberal in what you 
accept' it does no harm to carry others, so I'd suggest noting or a 
should.  How  about adding:

Note that when the special value is included, all other FID values 
included in the Data Item are redundant as the special value indicates 
all FIDs.

> Nits
>
> ====
>
> Section 1
>
> 1st para: "... does not include any flow identification beyond DLEP 
> endpoints or flow control capability" => "... does not include any 
> flow identification beyond DLEP endpoints nor flow control capability" (?)
>
> 1st para: replace [I-D.ietf-manet-dlep-pause-extension] by [RFC8651]
>
agreed to the above
>
> 2nd para: Redundant full stop after 
> [I-D.ietf-manet-dlep-traffic-classification] (but see comment under 
> Minor Issues on the sentence as a whole).
>
changed to ,
>
> Section 2
>
> 2nd para: "negotiate the use of extension" => "negotiate the use of 
> this extension"
>
> 3rd pare: "per logical "Credit Windows" basis" => "per logical "Credit 
> Window" basis"
>
agreed to the above
>
> 3rd para: DSCP, expand on first use
>
not according to https://www.rfc-editor.org/materials/abbrev.expansion.txt
>
> 3rd para: FID, expand on first use
>
done
>
> 3rd para, sentence before last: "In addition to the traffic 
> classification information associated with an FID, routers provide an 
> initial credit window size, as well as ..." should presumably be: "In 
> addition to the traffic classification information associated with an 
> FID, modems provide an initial credit window size, as well as ..."
>
great catch!
>
> 7th para: "a modem "Associates" ..." => "a modem "associates" ..."
>
> 7th para: TID, expand on first use
>
> Section 2.1
>
> 1st para: "... including any MAC headers ..." => "... including any 
> MAC headers and trailers ..." (i.e., take Frame Check Sequence into 
> account?)
>
Ohh nice one! what about preamble and trailer?

how about:

s/headers/overhead (e.g., framing, header and trailer)

> Section 2.2.1
>
> 2nd para: "... the credit window value previous provided to the 
> router." => "... the credit window value previously provided to the 
> router."
>
yes
>
> 3rd para: "... and processing frequent credit window requests against 
> a having data traffic available to send, ..." => "... and processing 
> frequent credit window requests against a router having data traffic 
> available to send, ..."
>
how about "against having"
>
> Section 2.2.2
>
> 2nd para: "A Data Item for every Credit Window Request Data Item 
> contained in the corresponding Credit Control Response Message 
> received by the modem MUST be included." => "A Data Item for every 
> Credit Window Request Data Item contained in the corresponding Credit 
> Control Message received by the modem MUST be included."
>
thanks
>
> Section 2.3
>
> 1st para: "The Credit Request is used by a router to request 
> additional credits." => "The Credit Window Request is used by a router 
> to request additional credits."
>
sure


> Section 2.3.1
>
> Scale field definition: "... used in the Credit Window Size fields." 
> => "... used in the Credit Window Size field."
>
> last para: "... that the FID field value has been provided by the 
> modem in a Traffic Classification Data Item carried in either the 
> current or previous message." => "... that the FID field value has 
> been provided by the modem in a Traffic Classification Data Item 
> carried in either the current or a previous message." (not completely 
> sure about this one)
>
okay


> Section 2.3.2
>
> Throughout this section (including its title), replace "Credit Window 
> Associate" by "Credit Window Association"
>
thanks
>
> Section 2.3.3
>
> 1st para: "... or are listed in a Credit Window Initialization Data 
> Item carried in the same messages as the Data Item." => "... or are 
> listed in a Credit Window Initialization Data Item carried in the same 
> message as the Data Item."
>
okay

>
> Add definition of 'Reserved' field, as was done for the Credit Window 
> Initialization Data Item in section 2.3.1
>
I usually just give the definition of Reserved once in the document , 
but okay.

> Section 2.3.4
>
> Add definition of 'Reserved' field, as was done for the Credit Window 
> Initialization Data Item in section 2.3.1
>
sure.
>
> last para: "... at the time the most Credit Window Initialization or 
> Grant Data Item associated with the indicated FID was sent." => "... 
> at the time the most recent Credit Window Initialization or Grant Data 
> Item associated with the indicated FID was sent."
>
sure
>
> Section 2.3.5
>
> 2nd para: "Credit windows identified using a FID as defined above in 
> Section 2.3.1." => "Credit windows are identified using a FID as 
> defined above in Section 2.3.1."
>
thanks
>
> Section 3
>
> "The data items defined in this document will only be used when 
> extensions require their use." =>  "The messages and data items 
> defined in this document will only be used when extensions require 
> their use."
>
agreed
>
> Section 6.2
>
> Replace informative reference [I-D.ietf-manet-dlep-pause-extension] by 
> [RFC8651]
>
yes
>
> Appendix A
>
> 1st para: "We morn the loss ..." => "We mourn the loss ..."
>
Thank you very much for the detailed review!

I'll have these changes posted shortly!

Lou

PS it would be nice to say what changes in the  line in the future, 
visual diff isn't really working well for me... In my reviews I usually 
do section, original text, OLD/NEW or list specific change. I probably 
spent an extra hour on this doing visual diff processing.


> This message may contain information that is not intended for you. If 
> you are not the addressee or if this message was sent to you by 
> mistake, you are requested to inform the sender and delete the 
> message. TNO accepts no liability for the content of this e-mail, for 
> the manner in which you use it and for damage of any kind resulting 
> from the risks inherent to the electronic transmission of messages.
>