Last Call Review of draft-ietf-tcpm-dctcp-07
review-ietf-tcpm-dctcp-07-opsdir-lc-clarke-2017-06-08-00

Request Review of draft-ietf-tcpm-dctcp
Requested rev. no specific revision (document currently at 10)
Type Last Call Review
Team Ops Directorate (opsdir)
Deadline 2017-06-15
Requested 2017-06-01
Draft last updated 2017-06-08
Completed reviews Genart Last Call review of -07 by Orit Levin (diff)
Secdir Last Call review of -07 by Catherine Meadows (diff)
Opsdir Last Call review of -07 by Joe Clarke (diff)
Assignment Reviewer Joe Clarke
State Completed
Review review-ietf-tcpm-dctcp-07-opsdir-lc-clarke-2017-06-08
Reviewed rev. 07 (document currently at 10)
Review result Has Nits
Review completed: 2017-06-08

Review
review-ietf-tcpm-dctcp-07-opsdir-lc-clarke-2017-06-08

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.

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?