netdev - Re: [PATCH net-next 2/2] tcp: ack immediately when a cwr packet arrives

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <B5239D76-DB07-45F3-BC18-BF984D1D3861@fb.com>
Date:   Mon, 2 Jul 2018 21:24:27 +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>
Subject: Re: [PATCH net-next 2/2] tcp: ack immediately when a cwr packet
 arrives

On 7/2/18, 7:57 AM, "Neal Cardwell" <ncardwell@...gle.com> wrote:

    On Sat, Jun 30, 2018 at 9:47 PM Lawrence Brakmo <brakmo@...com> wrote:
    > I see two issues, one is that entering quickack mode as you
    > mentioned does not insure that it will still be on when the CWR
    > arrives. The second issue is that the problem occurs right after the
    > receiver sends a small reply which results in entering pingpong mode
    > right before the sender starts the new request by sending just one
    > packet (i.e. forces delayed ack).
    >
    > I compiled and tested your patch. Both 99 and 99.9 percentile
    > latencies are around 40ms. Looking at the packet traces shows that
    > some CWR marked packets are not being ack immediately (delayed by
    > 40ms).
    
    Thanks, Larry! So your tests provide nice, specific evidence that it
    is good to force an immediate ACK when a receiver receives a packet
    with CWR marked. Given that, I am wondering what the simplest way is
    to achieve that goal.
    
    What if, rather than plumbing a new specific signal into
    __tcp_ack_snd_check(), we use the existing general quick-ack
    mechanism, where various parts of the TCP stack (like
    __tcp_ecn_check_ce())  are already using the quick-ack mechanism to
    "remotely" signal to __tcp_ack_snd_check() that they want an immediate
    ACK.
    
    For example, would it work to do something like:
    
    diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
    index c53ae5fc834a5..8168d1938b376 100644
    --- a/net/ipv4/tcp_input.c
    +++ b/net/ipv4/tcp_input.c
    @@ -262,6 +262,12 @@ static void __tcp_ecn_check_ce(struct sock *sk,
    const struct sk_buff *skb)
     {
            struct tcp_sock *tp = tcp_sk(sk);
    
    +       /* If the sender is telling us it has entered CWR, then its cwnd may be
    +        * very low (even just 1 packet), so we should ACK immediately.
    +        */
    +       if (tcp_hdr(skb)->cwr)
    +               tcp_enter_quickack_mode(sk, 2);
    +
            switch (TCP_SKB_CB(skb)->ip_dsfield & INET_ECN_MASK) {
            case INET_ECN_NOT_ECT:
                    /* Insure that GCN will not continue to mark packets. */
    
    And then since that broadens the mission of this function beyond
    checking just the ECT/CE bits, I supposed we could rename the
    __tcp_ecn_check_ce() and tcp_ecn_check_ce() functions to
    __tcp_ecn_check() and tcp_ecn_check(), or something like that.
    
    Would that work for this particular issue?
    
    Neal

Thanks Neal, it does work and is cleaner than what I was doing. I will submit a revised patch set.