Message-ID: <239B3FF8-90AE-4C1B-B791-3A13964B7B12@fb.com>
Date:   Sun, 1 Jul 2018 04:09:36 +0000
From:   Lawrence Brakmo <brakmo@...com>
To:     Neal Cardwell <ncardwell@...gle.com>
CC:     Netdev <netdev@...r.kernel.org>, Kernel Team <Kernel-team@...com>,
        "Blake Matheny" <bmatheny@...com>, Alexei Starovoitov <ast@...com>,
        Yuchung Cheng <ycheng@...gle.com>,
        Steve Ibanez <sibanez@...nford.edu>,
        Eric Dumazet <eric.dumazet@...il.com>,
        Yousuk Seung <ysseung@...gle.com>
Subject: Re: [PATCH net-next 0/2] tcp: fix high tail latencies in DCTCP

On 6/30/18, 5:26 PM, "netdev-owner@...r.kernel.org on behalf of Neal Cardwell" <netdev-owner@...r.kernel.org on behalf of ncardwell@...gle.com> wrote:

    On Fri, Jun 29, 2018 at 9:48 PM Lawrence Brakmo <brakmo@...com> wrote:
    >
    > When have observed high tail latencies when using DCTCP for RPCs as
    > compared to using Cubic. For example, in one setup there are 2 hosts
    > sending to a 3rd one, with each sender having 3 flows (1 stream,
    > 1 1MB back-to-back RPCs and 1 10KB back-to-back RPCs). The following
    > table shows the 99% and 99.9% latencies for both Cubic and dctcp:
    >
    >            Cubic 99%  Cubic 99.9%   dctcp 99%    dctcp 99.9%
    >   1MB RPCs    2.6ms       5.5ms         43ms          208ms
    >  10KB RPCs    1.1ms       1.3ms         53ms          212ms
    >
    > Looking at tcpdump traces showed that there are two causes for the
    > latency.
    >
    >   1) RTOs caused by the receiver sending a dup ACK and not ACKing
    >      the last (and only) packet sent.
    >   2) Delaying ACKs when the sender has a cwnd of 1, so everything
    >      pauses for the duration of the delayed ACK.
    >
    > The first patch fixes the cause of the dup ACKs, not updating DCTCP
    > state when an ACK that was initially delayed has been sent with a
    > data packet.
    >
    > The second patch insures that an ACK is sent immediately when a
    > CWR marked packet arrives.
    >
    > With the patches the latencies for DCTCP now look like:
    >
    >            dctcp 99%  dctcp 99.9%
    >   1MB RPCs    4.8ms       6.5ms
    >  10KB RPCs    143us       184us
    >
    > Note that while the 1MB RPCs tail latencies are higher than Cubic's,
    > the 10KB latencies are much smaller than Cubic's. These patches fix
    > issues on the receiver, but tcpdump traces indicate there is an
    > opportunity to also fix an issue at the sender that adds about 3ms
    > to the tail latencies.
    >
    > The following trace shows the issue that tiggers an RTO (fixed by these patches):
    >
    >    Host A sends the last packets of the request
    >    Host B receives them, and the last packet is marked with congestion (CE)
    >    Host B sends ACKs for packets not marked with congestion
    >    Host B sends data packet with reply and ACK for packet marked with
    >           congestion (TCP flag ECE)
    >    Host A receives ACKs with no ECE flag
    >    Host A receives data packet with ACK for the last packet of request
    >           and which has TCP ECE bit set
    >    Host A sends 1st data packet of the next request with TCP flag CWR
    >    Host B receives the packet (as seen in tcpdump at B), no CE flag
    >    Host B sends a dup ACK that also has the TCP ECE flag
    >    Host A RTO timer fires!
    >    Host A to send the next packet
    >    Host A receives an ACK for everything it has sent (i.e. Host B
    >           did receive 1st packet of request)
    >    Host A send more packets…
    >
    > [PATCH net-next 1/2] tcp: notify when a delayed ack is sent
    > [PATCH net-next 2/2] tcp: ack immediately when a cwr packet arrives
    
    Thanks, Larry!
    
    I suspect there is a broader problem with "DCTCP-style precise ECE
    ACKs" that this patch series does not address.
    
    AFAICT the DCTCP "historical" ACKs for ECE precision, generated in
    dctcp_ce_state_0_to_1() and dctcp_ce_state_1_to_0(), violate the
    assumptions of the pre-existing delayed ACK state machine. They
    violate those assumptions by rewinding tp->rcv_nxt backwards and then
    calling tcp_send_ack(sk). But the existing code path to send an ACK
    assumes that any ACK transmission always sends an ACK that accounts
    for all received data, so that after sending an ACK we can cancel the
    delayed ACK timer. So it seems with DCTCP we can have buggy sequences
    where the DCTCP historical ACK causes us to forget that we need to
    schedule a delayed ACK (which will force the sender to RTO, as shown
    in the trace):
    
    tcp_event_data_recv()
     inet_csk_schedule_ack()  // remember that we need an ACK
     tcp_ecn_check_ce()
      tcp_ca_event() // with CA_EVENT_ECN_IS_CE or CA_EVENT_ECN_NO_CE
        dctcp_cwnd_event()
          dctcp_ce_state_0_to_1() or dctcp_ce_state_1_to_0()
           if (... && ca->delayed_ack_reserved)
              tp->rcv_nxt = ca->prior_rcv_nxt;
              tcp_send_ack(sk);     // send an ACK, but for old data!
                tcp_transmit_skb()
                  tcp_event_ack_sent()
                    inet_csk_clear_xmit_timer(sk, ICSK_TIME_DACK);
                        // forget that we need a delayed ACK!
    
    AFAICT the first patch in this series, to force an immediate ACK on
    any packet with a CWR, papers over this issue of the forgotten delayed
    ACK by forcing an immediate ack in __tcp_ack_snd_check(). This ensures
    that at least for CWR packets the forgotten delayed ACK does not
    matter. But for packets without CWR I believe the buggy "forgotten
    delayed ACK" sequence above is still possible, and could still plague
    DCTCP with high tail latencies in some cases. I suspect that after
    your CWR-forces-ACK patch it may not be jumping out in performance
    test results because (a) if there is no CWR involved then usually the
    cwnd is high enough that missing delayed ACK does not cause a stall,
    and (b) if there is a CWR involved then your patch forces an immediate
    ACK. But AFAICT that forgotten delayed ACK bug is still there.
    
    What do folks think?
    
    neal

    
Neal, just to clarify, the 2nd patch is the one that ACKs immediately after receiving a CWR marked packet. The 1st patch eliminates the problem that was causing RTOs due to DCTCP being in the wrong state (thinking there was a delayed ACK pending when there was not). With just the 1st patch I do not see any RTOs, only delays due to delayed ACKs (which are fixed with the 2nd patch).

You are correct in that the dctcp code to send old ACKs can result in missing sending an ACK. However, it is for the new packet not for the previously delayed ACK since that is the ACK that is sent when the dctcp code (dctcp_ce_state_..()) calls tcp_send_ack().

When a new data packet arrives, tcp_event_data_recv() is called. This calls inet_csk_schedule_ack(sk) which sets icsk_ack.pending to remember that an ack needs to be sent. If dctcp_ce_state_..() ends up calling tcp_send_ack(), it will ultimately clear icsk_ack.pending (it is done in inet_csk_clear_xmit_timer()). As a result, tcp_ack_snd_check(), which is called after dctcp_ce_state_..() is called, will just return after checking that icsk_ack.pending is zero. That is, not ACK will be sent.

If the cwnd is greater than 1, then missing one ACK will not have a major impact because the next packet will trigger sending an ACK (although it may be delayed). This is why my original patch of making sure the cwnd is at least 2 solved the problem (but did not fix the main causes).

This patch set fixes two issues in the current code that were causing the high percentile latencies when using DCTCP and RPCs.

The bug you pointed out does exists, but is orthogonal to the issues fixed by my current patch set. I will leave this patch set as is and do another patch for the bug you pointed out.

Thanks Neal!

Larry

Powered by blists - more mailing lists