Subject: [tcpm] Review of draft-ietf-tcpm-dctcp-01

From: Bob Briscoe <[email-masked]>
Date: Fri, 6 Nov 2015 18:27:03 +0000

Praveen & co-authors,

As promised, here's my review of draft-ietf-tcpm-dctcp-01

_*General Comments*__*
*_
In general, in the comments below I have tried to reduce the multiple 
assumptions about controlled environments down to the one assumption 
that they are controlled by a single authority. I believe DCTCP can work 
without the other assumptions, so the following do not apply in all 
controlled environments:
   * not always low RTT (e.g. inter-DC scenarios, or global private 
networks)
   * not always no risk of traffic attacks (e.g. internally arranged 
attacks)
I may have missed some other occurrences of these assumptions, so pls 
feel free to seek out them all.

Many of the comments argue with your choices of MUST, SHOULD, MAY etc. 
This might seem nit-picking, given the primary purpose is to document 
the algo. However, it would be wrong to require things that aren't 
required or to allow exceptions when exceptions would not be 
interoperable. An alternative approach would be to just remove all 
capitalised language.

_*Section-by-section review.*__*
*_
*Abstract**
*
I think it needs to have an applicability statement in the abstract (and 
introduction) that refers to the deployment and implementation status 
sections at the end. Something like:

"This is an informational specification of the implementation of DCTCP 
in Microsoft Windows Server 2012. It is applicable to deployments in 
controlled environments like data centres but it MUST NOT be deployed 
over the public Internet without additional measures, as detailed in 
sections 4-6. "

*1. Intro**
*s/its many servers/
  /their many servers/

"...limited queue capacities..."
I understand that the motivation has been abbreviated, but I think this 
has lost too much. Perhaps mention memory shared between interfaces, so 
either bloated buffers or too little, making traffic susceptible to 
packet losses?

    [RFC3168] describes a mechanism for using Explicit Congestion
    Notification (ECN) from the switches for early detection of
    congestion, rather than waiting for packet loss to occur.

This isn't correct. 3168 requires ECN marking to occur no earlier than 
drop. It is precisely that 3168 does not allow early marking unless 
there is also early drop that is the problem.

    It is recommended that DCTCP be deployed...

"recommended" is too weak. I suggest you use something like the 
applicability text given above, both here and in the abstract.

*2. Terminology**
*
Suggest you explain that capitalised words are not quite the same as in 
most RFCs:
"Normative language is used to describe how necessary the various 
aspects of the Microsoft implementation are for interoperability, but 
even compliant implementations without the measures in sections 4-6 
would still only be safe to deploy in controlled environments."

*3. DCTCP Algo**
*
The three bullets say no more than "this is normal stuff". Would it be 
better to supplement each sentence with a brief phrase saying what is 
different about each aspect compared to RFC3168?

*3.1 Marking**
*
This needs to say something about how you mark the IP header from L2. I 
suggest you refer to the up-and-forward mode of draft-ietf-tsvwg-ecn-encap.

s/sending rate/
  /link rate/

*3.2 **
*
CURRENT:

    1.  If the CE codepoint is set and DCTCP.CE is false, send an ACK for
        any previously unacknowledged packets and set DCTCP.CE to true.

    2.  If the CE codepoint is not set and DCTCP.CE is true, send an ACK
        for any previously unacknowledged packets and set DCTCP.CE to
        false.

SUGGESTED:

    1.  If the CE codepoint is set and DCTCP.CE is false, set DCTCP.CE to
        true and send an ACK for any previously unacknowledged packets.

    2.  If the CE codepoint is not set and DCTCP.CE is true, set DCTCP.CE
        to false and send an ACK for any previously unacknowledged packets.

     The figure would have to be changed to be consistent too.
RATIONALE:
If the receiver changes DCTCP.CE /after/ sending the ACK like this, it 
delays each signal until the following ACK is sent. That could add a 
delay of anything from 1 to m packets. I've never understood why the 
order of these operations was specified in this way. If there is no 
reason, then I suggest that it is specified in the opposite order, as I 
describe above.

A note could be added to say the Windows Server 12 implementation does 
these steps in reverse order, but the order specified is preferred 
because it reduces signalling delay and has no interoperability issues 
with the original Windows implementation.

    The handling of the "Congestion Window Reduced" (CWR) bit is also
    exactly as per [RFC3168 <https://tools.ietf.org/html/rfc3168>] including [RFC3168-ERRATA3639 
<https://tools.ietf.org/html/draft-ietf-tcpm-dctcp-01#ref-RFC3168-ERRATA3639>].  That is, on
    receipt of a segment with both the CE and CWR bits set, CWR is
    processed first and then ECE is processed.

Even tho this is the receiver section, I suggest:
s/The handling/Receiver handling/

Whatever, this cannot be correct. A DCTCP receiver surely does nothing 
on receipt of CWR, whereas an RFC3168 receiver does something. A DCTCP 
receiver certainly cannot do exactly what RFC3168 says, because that 
says stop setting ECE on receipt of CWR until the next CE arrives, which 
would surely break this feedback protocol.

*3.3 **
*

    DCTCP.Alpha, which is initialized to 1 and MUST be updated
    as follows:

s/MUST/SHOULD/
And add "An alternative congestion avoidance algorithm MAY be used, but 
it MUST lead to flow rate inversely proportional to 1/M".

Rationale: There are many ways to implement interoperable 1/p cc. This 
is just one.

    DCTCP.BytesSent

This variable actually counts bytes received. it's pretty confusing to 
call it BytesSent.

s/TCP SACK options [RFC2018] are ignored./
  /The sender MUST ignore TCP SACK options [RFC2018] for this computation./
RATIONALE:
   a) I think the idea is to ignore CE marks after a gap because, if the 
gap becomes considered a loss, the response to loss will override any 
need to count incoming CE marks for the following round trip. However, 
if the gap is filled before it is considered a loss {Note 1}, then CE 
marks that were ignored because they arrived after a gap will need to be 
"unignored".
   b) We need to make clear that SACK is not ignored altogether, only 
for this computation.

{Note 1}: I can think of two cases:
* Less than three dupacks then the gap is filled
* The gap is filled by a delayed segment so that an earlier 
retransmission was spurious

the sender MUST update cwnd as follows:
       cwnd = cwnd * (1 - DCTCP.Alpha / 2)

s/MUST/SHOULD/
Rationale: Alternative interoperable response functions are possible. 
For instance an algorithm like Relentless TCP that simply reduces cwnd 
by half the size of any CE-marked segment. Then there is no Alpha and no 
g variable.

CURRENT:

    Just as specified in [RFC3168 <https://tools.ietf.org/html/rfc3168>], TCP should not react to congestion
    indications more than once for every window of data.

SUGGESTED:

    Just as specified in [RFC3168 <https://tools.ietf.org/html/rfc3168>], DCTCP does not react to congestion
    indications more than once for every window of data.

I don't know whether you intended this to be an upper-case SHOULD NOT. 
If so, I would argue against this being normative, because it isn't 
necessary for interop; therefore I've suggested avoiding he 'should' word.

    The setting of
    the "Congestion Window Reduced" (CWR) bit is also exactly as per
    [RFC3168 <https://tools.ietf.org/html/rfc3168>].

It is wrong to say "exactly". The sender certainly sets CWR when it does 
a congestion response. But in DCTCP the congestion response occurs every 
RTT, triggered by its own measurement-period logic, whereas in RFC3168 
it is triggered by the arrival of an ECE after the ACK of any previous 
segment it sent with CWR set.

I think it is worth saying that setting CWR is necessary for safety, 
otherwise in the case where there has been a configuration management 
error and the receiver complies with RFC 3168 not DCTCP, it will 
continue sending ECE for ever and starve the session.

It might help to explicitly confirm that:
"Other aspects of TCP congestion avoidance and control such as the slow 
start phase are no different to those in RFC5681."

*3.4 SYN, SYN-ACK, RST**
*
    [RFC3168] requires that a compliant TCP MUST NOT set ECT on SYN or
    SYN-ACK packets.

s/MUST NOT/'MUST NOT'/
Rationale: Best to make it clear this is quoting a normative statement 
in another RFC, because it is going to be contradicted.

CURRENT

    These RFCs, however, are
    intended for general Internet use, and do not directly apply to a
    controlled datacenter environment.
...
    For DCTCP connections, the sender
    SHOULD set ECT for SYN, SYN-ACK and RST packets.

SUGGESTED:

    Also, a SYN is sent before the ECN-capability has been
    negotiated, so if it is CE-marked, a non-ECN TCP server would not
    recognise the marking. RFC 3168 does not specify the ECN field on
    a RST. The security concerns addressed by these RFCs
    might not apply in controlled environments like data centres, and
    it might not be necessary to cater for the presence of non-ECN
    servers.
...

    Therefore, the sender MUST set ECT for SYN/ACK and RST packets and a
    configuration option SHOULD be provided to set ECT for SYNs.

RATIONALE:
    The draft should avoid assuming that security concerns do not apply 
in all controlled environments.
NOTE:
    This might not reflect the way your implementation is coded, so you 
might want to make that clear.

*4. Implementation Issues**
*
CURRENT:

    the implementation MUST choose a suitable
    estimation gain.  [DCTCP10 <https://tools.ietf.org/html/draft-ietf-tcpm-dctcp-01#ref-DCTCP10>] provides a theoretical basis

SUGGEST:

    the implementation will need to choose a suitable
    estimation gain.  [ADCTCP10 <https://tools.ietf.org/html/draft-ietf-tcpm-dctcp-01#ref-DCTCP10>] provides a theoretical basis ...

RATIONALE:
    a) As above, this is only one way to implement the cc, and others 
could well prove to be interoperable when tested.
    b) More particularly, it would be better for an implementation to 
adapt the estimation gain to the RTT, so the spec ought not to imply 
that one value has to be chosen.
    c) The later paper [ADCTCP10] provides a corrected way to determine 
the gain. From memory (I'm offline at the mo) it at least makes 
throughput inversely proportional to 1/RTT, whereas the approach in 
[DCTCP10] made it inversely proportional to 1/(RTT^2). I believe the 
Linux implementation uses the [ADCTCP10] approach.

    The implementation must also decide when to use DCTCP.
...
    likely in the same datacenter network.

The 2nd & 3rd paras are a mix of implementation and deployment issues, 
but mostly deployment, so they might be better moved to the deployment 
issues section.

CURRENT:

    It is RECOMMENDED that an implementation deal with loss episodes in
    the same way as conventional TCP.
...
    DCTCP implementation MAY also allow configuration of resetting the
    value of DCTCP.Alpha as part of processing any loss episodes.

SUGGESTED: Move this whole para to 3.3 Sender Behaviour section, and:

    An implementation MUST deal with loss episodes in
    the same way as conventional TCP.
...
    DCTCP implementation SHOULD also reset the
    value of DCTCP.Alpha as part of processing any loss episodes.
    This aspect of the behaviour MAY be configurable.

RATIONALE:
    a) There would never be a case for not falling back to conventional 
TCP  (or do you have one?), so "RECOMMENDED" is too weak.
    b) I have tried to interpret what you might have meant about 
resetting Alpha. I'm not sure why you think it might need to be 
configurable though?

    ...it is also RECOMMENDED that an implementation allow
    configuration of restarting the congestion window (cwnd) of idle
    DCTCP connections

Where special implementation measures are necessary for low RTT, these 
should be related to the measured RTT, not hard-coded. There is an 
assumption in a number of places  that "data centre" implies low RTT. Of 
course this is true in a single DC, but I believe the the single admin 
aspect of a DC is what characterises DCTCP, not the low RTT aspect. With 
the modifications in [ADCTCP], DCTCP generalises to a network with 
larger RTTs as long as it is still operated by a single admin.

CURRENT:
    [RFC3168] forbids the ECN-marking of pure ACK packets, because of the
    inability of TCP to mitigate ACK-path congestion and protocol-wise
    preferential treatment by routers.  However, dropping pure ACKs -
    rather than ECN marking them - has disadvantages for typical
    datacenter traffic patterns.  Because of the prevalence of bursty
    traffic patterns that feature transient congestion, dropping of ACKs
    causes subsequent retransmissions. It is RECOMMENDED that an
    implementation provide a configuration knob that will cause ECT to 
be set
    on such control packes, which can be used in environments where such
    concerns do not apply.
SUGGESTED:
    [RFC3168] forbids the ECN-marking of pure ACK packets, because of the
    inability of TCP to mitigate ACK-path congestion and the extra
    advantage to injection attackers that ECN is perceived to offer.
    For the latter reason RFC 3168 also forbids ECN-marking of
    retransmissions, window probes and RSTs.

    However, dropping all these control packets -
    rather than ECN marking them - has considerable performance
    disadvantages. It is RECOMMENDED that an
    implementation provide a configuration knob that will cause ECT to 
be set
    on such control packes, which can be used in environments where such
    concerns do not apply.
NOTE:
    This might not reflect the way your implementation is coded, so you 
might want to make that clear.

*5. Deployment Issues**
*

    If
    the traffic in the datacenter is a mix of conventional TCP and DCTCP,
    it is RECOMMENDED that DCTCP traffic be segregated from conventional
    TCP traffic.

In a data centre (or anywhere), is there any case where not segregating 
them would make sense? If not, this needs to be a MUST, not just a 
RECOMMENDED. I can imagine cases where there is no long-running 
conventional TCP or no long-running DCTCP, but that's covered by the 
"if" at the start of the sentence.

You might want to refer to draft-briscoe-aqm-dualq-coupled as another 
segregation approach here too.

    Since DCTCP relies on congestion marking by the switches, DCTCP can
    only be deployed in datacenters where the entire network
    infrastructure supports ECN.

Surely, the "fall-back to conventional TCP on loss" rule means you can 
delete this constraint.

    A
    variant of DCTCP that can be deployed unilaterally and only requires
    standard ECN behavior has been described in [ODCTCP <https://tools.ietf.org/html/draft-ietf-tcpm-dctcp-01#ref-ODCTCP>][BSDCAN],

Which part of "standard ECN behaviour" do you mean? The host part? The 
network part? The receiver part? And "can be deployed unilaterally" 
ought to be in the active voice not the passive, which would help 
resolve the ambiguity too.

I haven't read [ODCTCP] and [BSDCAN] (I'm offline on a plane at the mo). 
Are they similar to "Instant ECN" in [Wu12] listed below? If not, that 
could be referred to as well for a technique that uses the regular ECN 
host behaviour.

*6. Known Issues**
*
First para could refer to appendix A of RFC7560, which describes the 
problem precisely.

A method for improving the fairness of DCTCP has been proposed
    in [ADCTCP <https://tools.ietf.org/html/draft-ietf-tcpm-dctcp-01#ref-ADCTCP>], but requires additional experimental evaluation.

s/fairness/RTT fairness/

(In our experiments, it's much better than without.)

*11. References**
*
I think the following are cited normatively, not just informatively:
RFC5681, RFC5562.

Additional informational Reference:

[Wu12] Wu, H., Ju, J., Lu, G., Guo, C., Xiong, Y. & Zhang, Y., "Tuning 
ECN for Data Center Networks," In: Proceedings of the 8th International 
Conference on Emerging Networking Experiments and Technologies CoNEXT 
'12 pp.25-36 ACM (2012)

That's it. HTH.

Bob

-- 
________________________________________________________________
Bob Briscoe                               http://bobbriscoe.net/