On Thu, Jun 8, 2017 at 11:53 AM, Joe Clarke <jclarke(_at_)cisco(_dot_)com>
wrote:
Reviewer: Joe Clarke
Review result: Has Nits
Hello, WG and authors. I have reviewed rev -07 of the draft-ietf-tcpm-dctcp
as
requested by the OPS-DIR. This review focuses on improving operational
aspects
as well as any nits found in the text.
... and I'd quickly like to thank Joe for his review. Fixing issues
like this make the documents progress through the IESG with much less
drama...
W
This document is an informational draft that describes Data Center TCP
(DCTCP),
a congestion control mechanism for TCP in Data Center environments.
Overall, I believe this document to be ready, with some nits and perhaps small
areas for improved clarity and readability. First, I'd like to say that I
appreciate the fact that this has been implemented on a number of kernels, and
the authors included real-world implementation results and thoughts. From an
operational perspective, that is very helpful. I also appreciated the fact
that there are interoperability challenges, and those were called out in the
document. My specific comments are below.
There are a lot of abbreviations, variables and other terminology used
throughout this document. It might be helpful for the reader to have an
expanded terminology section at the top that one can refer to for all of these
things. Some of the abbreviations are called out in the description of the
algorithm, but not all (e.g., DCTCP.Alpha, CWR, RTT, etc.).
===
Section 3.2:
You refer to DCTCP.Alpha before defining it. While you refer to Section 3.3
here, the impact of an incorrect Alpha value is not fully appreciated in this
text. Perhaps this could be changed to reflect the impact the incorrect Alpha
value would have?
===
Section 3.2:
My abbreviating DCTCP.CE as CE in your state machine diagram, it is a bit
confusing as to the difference between CE and DCTCP.CE. The description of
the
state machine above requires the CE codepoint to have a certain value in order
for DCTCP.CE to change. Perhaps you can use D.CE as an abbreviation to be a
bit clearer here.
===
Section 3.3:
It is not clear if 'g' can be inclusive of 0 and 1.
===
Section 3.3:
You define DCTCP.WindowEnd as the threshold for beginning a new observation
window, but maybe to complement the state variable name, you should define it
as the following:
The TCP sequence number threshold when one observation window ends and other
is
to begin; initialized to SND.UNA.
===
Section 3.3:
You state:
Thus, when no bytes sent experienced congestion, DCTCP.Alpha equals
zero, and cwnd is left unchanged
But if I use a value of 1/16 for g, with DCTCP.Alpha initialized to 1 as you
say, I get a value of DCTCP.Alpha == 15/16 when there is no congestion (i.e.,
M
== 0).
===
Section 3.5:
You have an extra space here before the comma:
If SYN , SYN-ACK and RST packets for DCTCP connections have ECT set
This should be:
If SYN, SYN-ACK and RST packets for DCTCP connections have ECT set
===
Section 3.5:
You do not define ECT before using it.
===
Section 4.1:
Can you provide a reference for NewReno?
===
Section 5:
Can you reference or define AQM and RED?
_______________________________________________
OPS-DIR mailing list
OPS-DIR(_at_)ietf(_dot_)org
https://www.ietf.org/mailman/listinfo/ops-dir
--
I don't think the execution is relevant when it was obviously a bad
idea in the first place.
This is like putting rabid weasels in your pants, and later expressing
regret at having chosen those particular rabid weasels and that pair
of pants.
---maf