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

[Eric Kinzie <[e-mail]>]

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".
> 
> 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.
> 
> 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. 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.
> 
> More detailed comments can be found at:
> 
> https://github.com/boucadair/IETF-Drafts-Reviews/blob/master/2024/draft-ietf-manet-dlep-credit-flow-control-15-rev%20Med.pdf
> 
> 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



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.
	


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



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.



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.



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



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



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



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



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



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.



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



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.



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.



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.



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.



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.



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.



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.
	https://datatracker.ietf.org/doc/html/rfc8175#section-12.1
	specifies how an invalid message is handled.
	A router may send Credit Control messages at an agressive rate,
	but there is not benefit; the Security Considerations section
	comments on the possibility of a malicious third-party doing
	such a thing.




Commenté [BMI21]: Where?
	XREF added



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.



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



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



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.



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.



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.



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.



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



Commenté [BMI33]: Where?
	XREF added



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



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.



Commenté [BMI36]: Where?
	XREF added



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



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.



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



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.