[manet] Re: Opsdir early review of draft-ietf-manet-dlep-credit-flow-control-15

[[e-mail]]

Hi Eric, 

Thanks for the follow-up. I checked -16 and the replies you provided below.

It is long time since I reviewed the doc, so I may not have all set in my mind.

Please see inline.

Cheers,
Med

> -----Message d'origine-----
> De : Eric Kinzie <[e-mail]>
> Envoyé : jeudi 21 novembre 2024 19:36
> À : BOUCADAIR Mohamed INNOV/NET <[e-mail]>
> Cc : [e-mail]; draft-ietf-manet-dlep-credit-flow-
> [e-mail]; [e-mail]
> Objet : Re: [manet] Opsdir early review of draft-ietf-manet-dlep-
> credit-flow-control-15
> 
> 
> On Wed Jul 24 05:54:05 -0700 2024, Mohamed Boucadair via
> Datatracker wrote:
> > Reviewer: Mohamed Boucadair
> > Review result: Has Issues
> >
> > Hi all,
> >
> > Thanks for the authors for the effort put into this well-
> written document.
> >
> > First, I'm always impressed by authors who persevere and push
> for a
> > specification over many years. I checked the archives to see
> whether
> > there were contentious points that would justify the long
> development
> > process, but I didn't find something meaningful other than some
> > discussions on how the various pieces (this I-D,
> classification, etc.)
> > can be documented. That discussion answered a comment I had
> about the
> > need to separate the classification spec from this. I won't
> thus raise
> > that point in my review. However, it seems weird (at least to
> me) to
> > define objects and put in the abstract that future documents
> "will mandate the use".
> >

[Med] I still see this mention in -16. I see no clarification provided to this point in the draft.

> > I was also expecting to see a discussion on how this flow-
> control is
> > superior compared to the pause approach specified in RFC 8651,
> > including a discussion about co-existence considerations and
> which one will take precedence.

[Med] I still think a discussion about this is needed, and easily visible in the doc.

> >
> > Overall, the document (seems) to reason following the model in
> Figure
> > 1 of RFC 8175, while it should be applicable as well for the
> > configuration in Figure 2 of 8175. 

[Med] Likewise, I don't see any mention of the two models and how this is supposed to work for both.

For example, the FID
> uniqueness (at
> > the router side) should be associated with the link over which
> the packets will be sent.
> >
> > >From a protocol machinery standpoint, there are some few cases
> where
> > >I think
> > the MUST behavior is not justified. Please refer to the link
> below for
> > more details on this.
> >
> > >From an ops standpoint, the document includes a dedicated
> section on
> > management. However, I think that more concrete implementation
> > behavior should be provided, e.g.,
> >
> > * how to report errors?
> >
> > * expose configuration knobs to control many of the parameters
> there.
> > Also, exposing implementation default would be helpful when
> operating the system.
> >
> > * technically characterize some events (e.g., transient events)
> and
> > provide a minimum value for how frequent messages can be sent.
> >

[Med] I see that you removed "frequent", declared logging as implementation-specific. I still don't see however a discussion on default values for configurable parameters, nor how to access those.

> > More detailed comments can be found at:
> >
> >
> https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2
> Fgith
> > ub.com%2Fboucadair%2FIETF-Drafts-
> Reviews%2Fblob%2Fmaster%2F2024%2Fdraf
> > t-ietf-manet-dlep-credit-flow-control-15-
> rev%2520Med.pdf&data=05%7C02%
> >
> 7Cmohamed.boucadair%40orange.com%7C00faa9f5f9714f21ac6c08dd0a5b7a
> 2f%7C
> >
> 90c7a20af34b40bfbc48b9253b6f5d20%7C0%7C0%7C638678110636003197%7CU
> nknow
> >
> n%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiO
> iJXaW
> >
> 4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=AnLnRWl
> OWNH3
> > IHRhM4gs0IFZzolWnc04rEIxz3TpX3c%3D&reserved=0
> >
> > hope this helps
> >
> > Cheers,
> > Med
> 
> Hi Med,
> 
> I have taken on the role of editor for this document.  Responses
> to your comments are below.
> 
> Thanks,
> Eric
> 
> 
> 
> Commenté [BMI1]: As this is not well-known per
> 	Accepted
> 
> 	Old text: DLEP Credit-Based Flow Control Messages and Data
> Items
> 
> 	New text: Dynamic Link Exchange Protocol (DLEP) Credit-Based
> 	Flow Control Messages and Data Items
> 

[Med] ACK

> 
> Commenté [BMI2]: I expect their use to be defined in this
> document.
> I guess the authors refer to I-D.ietf-manet-dlep-da-credit-
> extension?
> 
> 	Yes, that is the referenced document.  Left unchanged for
> now.
> 

[Med] I don't think that it is good to let the readers guess ;-). Please fix this so that the intent is explicit enough. Thanks.

> 
> 
> Commenté [BMI3]: I failed to find a section in rfc8175 where this
> is discussed. It would be helpful to include a reference of this.
> Thanks.
> 
> 	I don't think this implies that there is any discussion of
> flow
> 	control techniques in RFC8175.	However, I have removed the
> 	reference to other flow-control draft.
> 
> 	Old text: There are various flow control techniques
> theoretically
> 	possible with DLEP. For example, a credit-window scheme for
> 	destination-specific flow control which provides aggregate
> 	flow control for both modem and routers has been proposed in
> 	[I-D.ietf-manet-credit-window], and a control plane pause
> based
> 	mechanism is defined in [RFC8651].
> 
> 	New text: There are various flow control techniques
> theoretically
> 	possible with DLEP. For example, a credit-window scheme for
> 	destination-specific flow control which provides aggregate
> flow
> 	control for modems is proposed in this document, and a
> control
> 	plane pause based mechanism is defined in [RFC8651].
> 
> 

[Med] Delete the ref is definitely an option. Thanks.

> 
> Commenté [BMI4]: A router may be connected to multiple modems
> (figure 2 of 8175, for example), the spec seems to not include
> that when reasoning about FID uniqueness, etc. Some control
> checks should be also per peer.
> 
> 	draft-ietf-manet-dlep-traffic-classification explains that
> "TID
> 	and FID values have modem-local scope."  It is not a problem
> 	for two modems to use the same numerical FID values.
> 

[Med] It is not easy for readers to cross check the docs. I expect the applicability the one or both modes to be discussed in this document. I still think a change is needed here. 

As a side comment, with your explanation I feel like draft-ietf-manet-dlep-traffic-classification is needed to implement this extension. This smells more like a normative reference here. 

> 
> 
> Commenté [BMI5]: I would simplify and delete this sentence as the
> first sentence of the para right after conveys the message with
> more precision
> 	Removed.
> 
> 
[Med] ACK.

> 
> Commenté [BMI6]: Already mentioned.
> 	I don't think it is stated explicitly in the preceeding
> material.
> 	I have left this unchanged for now.
> 
> 

[Med] I see that you deleted it :-) So, all OK.


> 
> Commenté [BMI7]: To be consistent with the use in 8175
> 	Old text: Both types of DLEP endpoints, i.e., a router and a
> modem, . . .
> 	New text: Both types of DLEP peers, i.e., a router and a
> modem, . . .
> 
> 

[Med] ACK

> 
> Commenté [BMI8]: Which extension?
> The one on this document, traffic classification, or both?
> 	Old text: Both types of DLEP endpoints, router and modem,
> 	negotiate the use of this extension during session
> initialization,
> 	e.g., see [I-D.ietf-manet-dlep-da-credit-extension].
> 
> 	New text: Both types of DLEP peers, router and modem,
> 	negotiate the use of an extension employing this mechanism
> 	during session initialization as required, for example, by
> 	[I-D.ietf-manet-dlep-da-credit-extension].
> 
> 

[Med] ACK. Thanks.

> 
> Commenté [BMI9]: How those are identified?
> Can we technically characterize those?
> 	This statement is no longer accurate.  Changes combined with
> BMI10.
> 

[Med] ACK

> 
> 
> Commenté [BMI10]: I suggest to expose a configuration knob to
> control that window. Also, exposing the implepmentation default
> would be helpful.
> 	The modem waits for the previously-issued credits

[Med] How these are set? Who controls that? This is not easily available from the various specs I checked.

 to be
> 	drained, rather than allowing the credit window the be
> exceeded.
> 	As such, I don't think configuration is needed here.
> 
> 	Old text (9 & 10): When using credit windows, data traffic
> is only allowed
> 	to be sent by the router to the modem when there are credits
> 	available except during transients when the credit window
> has
> 	been reduced.  Implementations should allow exceeding the
> credit
> 	window during the short time that a router might take to
> respect
> 	the new credit window.
> 
> 	New text (9 & 10): When using credit windows, data traffic
> is only
> 	allowed to be sent by the router to the modem when there are
> 	credits available.
> 
> 
> Commenté [BMI11]: Where? Please cite the section
> 	XREF added.
> 

[Med] Thanks.

> 
> 
> Commenté [BMI12]: How that one is set? Is it configurable?
> 	I think the initial credit window size set by the modem is
> 	implementation-specific (depends on queue capacity).  I'm
> nore
> 	sure that requiring a configuration knob makes sense here.
> 
> 

[Med] I think having some text to echo what you said would be helpful. If you can explain why it is not needed to control that initial window, that would helpful to understand the rationale as well. Thanks. 

> 
> Commenté [BMI13]: Idem as previous comment
> 	Same response as to BMI12.
> 
> 

[Med] Still how that minimum is made available? Is it controllable? Etc.

> 
> Commenté [BMI14]: Do we need to keep this? This assumes that
> there will be always in place a matching CW for a flow. Is there
> a case where there is no "associated CW" for a flow?
> 	There will always be an associated CW.
> 
> 

[Med] This may be trivial for you, but can we have some text to explain this? Thanks.

> 
> Commenté [BMI15]: I'm afraid the normative language is not
> justified here, unless we expect the router to "reject" or err
> when it receives such window compared to typical packet sizes it
> may send.
> I think this is more a requirement on the (configuration of)
> value that will be sent by the modem.
> 	Reworded to be a requirement, placed on the router, that
> must
> 	be met before transmitting a packet.
> 
> 	Old text: A credit window value MUST be larger than the
> number
> 	of octets contained in a packet, including any MAC overhead
> 	(e.g., framing, headers, and trailers) used between the
> router
> 	and the modem, in order for the router to send the packet to
> a
> 	modem for forwarding.
> 
> 	New text: Additionally, a router MUST NOT send a data packet
> 	to the modem when there are fewer credits available in the
> 	associated Credit Window than there are octets in the
> packet.
> 	The count of octets in the packet includes MAC overhead,
> such as
> 	framing, header and trailer.
> 

[Med] Thanks, this is better.

> 
> 
> Commenté [BMI16]: As it may not be sent if there is not enough
> credits
> 	Accepted
> 
> 	Old text: A router MUST identify the credit window
> associated
> 	with traffic sent to a modem based on the traffic
> classification
> 	information provided in the Data Items defined in this
> document.
> 
> 	New text: A router MUST identify the credit window
> associated with
> 	traffic to be sent to a modem based on the traffic
> classification
> 	information provided in the Data Items defined in this
> document.
> 

[Med] Thanks

> 
> 
> Commenté [BMI17]: 2 messages
> 	Accepted
> 
> 	Old text: Two new messages are defined in support for credit
> 	window control: the Credit Control and the Credit Control
> 	Response Message.
> 
> 	New text: Two new messages are defined in support for credit
> 	window control: the Credit Control and the Credit Control
> 	Response Messages.
> 
> 

[Med] ACK

> 
> Commenté [BMI18]: This is likely to be dynamic, but I guess a
> minimum frequency guard can be enforced. Can we learn that min or
> control it by configuration?
> 	Removed "frequent".  This is just an observation.
> 
> 	Old text: Modems will need to balance the load generated by
> 	sending and processing frequent credit window increases
> against
> 	a router having data traffic available to send, but no
> credits
> 	available.
> 
> 	New text: Modems will need to balance the load generated by
> 	sending and processing credit window increases against a
> router
> 	having data traffic available to send, but no credits
> available.
> 

[Med] OK with removing the mention. However, wouldn't be useful to learn that min or control it by configuration?

> 
> 
> Commenté [BMI19]: Idem as previous comment
> 
> 	Old text: Routers will need to balance the load generated by
> 	sending and processing frequent credit window requests
> against
> 	having data traffic available to send, but no credits
> available.
> 
> 	New text: Routers will need to balance the load generated by
> 	sending and processing credit window requests against having
> data
> 	traffic available to send, but no credits available.
> 
> 

[Med] ACK.

> 
> Commenté [BMI20]: Are we sure MUST is justified here?
> What if the message was invalid? the router uses an aggressive
> rate when sending Credit Control messages? Etc.
> I think SHOULD is more appropriate
> 
> 	Since only one Credit Control can be outstanding, the
> response
> 	is necessary.  "MUST" seems appropriate.

[Med] .. assuming all validation checks pass.  Still don't think unconditional MUST is not justified here.

> 	https://eur03.safelinks.protection.outlook.com/?url=https%3A
> %2F%2Fdatatracker.ietf.org%2Fdoc%2Fhtml%2Frfc8175%23section-
> 12.1&data=05%7C02%7Cmohamed.boucadair%40orange.com%7C00faa9f5f971
> 4f21ac6c08dd0a5b7a2f%7C90c7a20af34b40bfbc48b9253b6f5d20%7C0%7C0%7
> C638678110636026029%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRyd
> WUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ
> %3D%3D%7C0%7C%7C%7C&sdata=0qcmwEjqoahMVvGU%2Bq9Lz6Mnli%2BRcbSr3YD
> sylJI1RU%3D&reserved=0
> 	specifies how an invalid message is handled.
> 	A router may send Credit Control messages at an agressive
> rate,
> 	but there is not benefit

[Med] I was raising a misuse/bug/etc. I think guards are needed here.

; the Security Considerations
> section
> 	comments on the possibility of a malicious third-party doing
> 	such a thing.
> 
> 
> 
> 
> Commenté [BMI21]: Where?
> 	XREF added
> 
> 

[Med] Thanks

> 
> Commenté [BMI22]: Includes a provision for local policies to
> modems for protecting itself for being overloaded, queue
> availability, etc.
> 	Since this document does not define such policies, I'm not
> sure
> 	it makes sense to mention them here.  Text left unchanged
> for now.
> 

[Med] Given the various MUST out there, I still think a discussion is worth in the draft.

> 
> 
> Commenté [BMI24]: For a specific router/modem association.
> 	Not sure this adds anything.  Left unchanged for now.
> 
> 

[Med] Indicating the unicity scope is useful, IMO. 

> 
> Commenté [BMI25]: Indicate how reporting can be done Commenté
> [BMI26]: It may do both?
> Commenté [BMI27]: May rate-limit the log per FID
> 	Removed "report" since this is used elsewhere to describe
> 	protocol behavior.

[Med] Can you please clarify where? Thanks.


  This now states explicitly that "how" is
> an
> 	implementation detail.
> 
> 	Old text: If the FID cannot be found the router SHOULD
> report
> 	or log this information.
> 
> 	New text: If the FID cannot be found the router SHOULD log
> 	this information. The method of logging is left to the
> router
> 	implementation.
> 
> 

[Med] ACK.

> 
> Commenté [BMI28]: Just to confirm, this will lead to terminating
> the session, right?
> Can this be logged?
> 
> 	Yes, this will terminate the session.  The referenced
> description
> 	recommends logging.
> 

[Med] OK.

> 
> 
> Commenté [BMI29]: Only one item per FID must be present. Right?
> Can this be mentioned and associated behavior described?
> 	Old text: Multiple Credit Window Grant Data Items in a
> single
> 	message are used to indicate different credit values for
> different
> 	credit windows.
> 
> 	New text: Multiple Credit Window Grant Data Items may be
> present
> 	in a single message.  Each item grants credits to a
> different
> 	credit window and, therefor, references a different FID.
> 

[Med] OK, thanks

> 
> 
> Commenté [BMI30]: How?
> 	Old text: If the FID is not known to the router, it SHOULD
> report
> 	or log this information and discard the Data Item.
> 
> 	New text: If the FID is not known to the router, it SHOULD
> log
> 	this information and discard the Data Item. The method of
> logging
> 	is left to the router implementation.
> 

[Med] ACK

> 
> 
> Commenté [BMI31]: If not set to zero
> 	If the grant is zero, increasing the credit window size by
> the
> 	value contained in the Additional Credits field still
> produces
> 	the correct outcome.
> 
> 

[Med] ACK.

> 
> Commenté [BMI32]: Such as?
> 	Old text: No response is sent by the router to a modem after
> 	processing a Credit Window Grant Data Item received in a
> Credit
> 	Control Response Message. In other cases, the receiving
> router
> 	MUST send a Credit Window Status Data Item or items
> reflecting the
> 	resulting Credit Window value of the updated credit window.
> When
> 	the Credit Grant
> 
> 	New text: No response is sent by the router to a modem after
> 	processing a Credit Window Grant Data Item received in a
> Credit
> 	Control Response Message. For other message types, the
> receiving
> 	router MUST send a Credit Window Status Data Item or items
> 	reflecting the resulting Credit Window value of the updated
> 	credit window. When the Credit Grant
> 
> 

[Med] ACK.

> 
> Commenté [BMI33]: Where?
> 	XREF added
> 
 
[Med] Thanks.

> 
> Commenté [BMI34]: Do we really need to mention this?
> That spec expired since 2016!
> 	Sentence removed.
> 

[Med] Thanks

> 
> Commenté [BMI35]: How?
> 	Old text: If the FID is not known to the modem, it SHOULD
> report
> 	or log this information and discard the Data Item.
> 
> 	New text: If the FID is not known to the modem, it SHOULD
> log
> 	this information and discard the Data Item.  The method of
> 	logging is left to the modem implementation.
> 
> 

[Med] OK

> 
> Commenté [BMI36]: Where?
> 	XREF added
> 
> 
[Med] Thanks 

> 
> Commenté [BMI37]: So they should not be included?
> Right?
> 	They are not necessary, but are harmless.
> 
> 
[Med] OK

> 
> Commenté [BMI38]: Absent parsing/validation errors.
> 	True, but it seems unnecessary to state this explicitly.
> 	Parsing error and validation error handling is described by
> DLEP.
> 

[Med] I think it is needed here because otherwise the MUST is unconditional.

> 
> 
> Commenté [BMI39]: What if it returns 0 for some FIDs? Is that
> still considered as an increment?
> 	Zero is a valid response.  "Each Credit Grant Data Item MAY
> 	provide zero or more additional credits based on the modem's
> 	transmission or local queue availability."
> 

[Med] thanks for pointing to this.

> 
> 
> Commenté [BMI40]: How?
> 	Old text: Unknown FID values SHOULD be reported or logged
> and
> 	then ignored by the modem.
> 
> 	New text: Unknown FID values SHOULD be logged and then
> ignored
> 	by the modem.  The method of logging is left to the modem
> 	implementation.

[Med] ACK.
____________________________________________________________________________________________________________
Ce message et ses pieces jointes peuvent contenir des informations confidentielles ou privilegiees et ne doivent donc
pas etre diffuses, exploites ou copies sans autorisation. Si vous avez recu ce message par erreur, veuillez le signaler
a l'expediteur et le detruire ainsi que les pieces jointes. Les messages electroniques etant susceptibles d'alteration,
Orange decline toute responsabilite si ce message a ete altere, deforme ou falsifie. Merci.

This message and its attachments may contain confidential or privileged information that may be protected by law;
they should not be distributed, used or copied without authorisation.
If you have received this email in error, please notify the sender and delete this message and its attachments.
As emails may be altered, Orange is not liable for messages that have been modified, changed or falsified.
Thank you.