netdev - Re: [PATCH net-next 3/4] tcp: add SACK compression

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: <CAK6E8=cEOPQqThGqbwB1Q0p5r8N0GcqV2hOT7CRa8Rdf2BWcCw@mail.gmail.com>
Date:   Thu, 17 May 2018 10:15:05 -0700
From:   Yuchung Cheng <ycheng@...gle.com>
To:     Neal Cardwell <ncardwell@...gle.com>
Cc:     Eric Dumazet <eric.dumazet@...il.com>,
        Eric Dumazet <edumazet@...gle.com>,
        David Miller <davem@...emloft.net>,
        Netdev <netdev@...r.kernel.org>,
        Toke Høiland-Jørgensen <toke@...e.dk>,
        Soheil Hassas Yeganeh <soheil@...gle.com>,
        Christoph Paasch <cpaasch@...le.com>
Subject: Re: [PATCH net-next 3/4] tcp: add SACK compression

On Thu, May 17, 2018 at 9:59 AM, Yuchung Cheng <ycheng@...gle.com> wrote:
> On Thu, May 17, 2018 at 9:41 AM, Neal Cardwell <ncardwell@...gle.com> wrote:
>>
>> On Thu, May 17, 2018 at 11:40 AM Eric Dumazet <eric.dumazet@...il.com>
>> wrote:
>> > On 05/17/2018 08:14 AM, Neal Cardwell wrote:
>> > > Is there a particular motivation for the cap of 127? IMHO 127 ACKs is
>> quite
>> > > a few to compress. Experience seems to show that it works well to have
>> one
>> > > GRO ACK for ~64KBytes that triggers a single TSO skb of ~64KBytes. It
>> might
>> > > be nice to try to match those dynamics in this SACK compression case,
>> so it
>> > > might be nice to cap the number of compressed ACKs at something like 44?
>> > > (0xffff / 1448 - 1).  That way for high-speed paths we could try to keep
>> > > the ACK clock going with ACKs for ~64KBytes that trigger a single TSO
>> skb
>> > > of ~64KBytes, no matter whether we are sending SACKs or cumulative ACKs.
>>
>> > 127 was chosen because the field is u8, and since skb allocation for the
>> ACK
>> > can fail, we could have cases were the field goes above 127.
>>
>> > Ultimately, I believe a followup patch would add a sysctl, so that we can
>> fine-tune
>> > this, and eventually disable ACK compression if this sysctl is set to 0
>>
>> OK, a sysctl sounds good. I would still vote for a default of 44.  :-)
>>
>>
>> > >> +       if (hrtimer_is_queued(&tp->compressed_ack_timer))
>> > >> +               return;
>> > >> +
>> > >> +       /* compress ack timer : 5 % of srtt, but no more than 2.5 ms */
>> > >> +
>> > >> +       delay = min_t(unsigned long, 2500 * NSEC_PER_USEC,
>> > >> +                     tp->rcv_rtt_est.rtt_us * (NSEC_PER_USEC >>
>> 3)/20);
>> > >
>> > > Any particular motivation for the 2.5ms here? It might be nice to match
>> the
>> > > existing TSO autosizing dynamics and use 1ms here instead of having a
>> > > separate new constant of 2.5ms. Smaller time scales here should lead to
>> > > less burstiness and queue pressure from data packets in the network,
>> and we
>> > > know from experience that the CPU overhead of 1ms chunks is acceptable.
>>
>> > This came from my tests on wifi really :)
>>
>> > I also had the idea to make this threshold adjustable for wifi, like we
>> did for sk_pacing_shift.
>>
>> > (On wifi, we might want to increase the max delay between ACK)
>>
>> > So maybe use 1ms delay, when sk_pacing_shift == 10, but increase it if
>> sk_pacing_shift has been lowered.
>>
>> Sounds good to me.
>>
>> Thanks for implementing this! Overall this patch seems nice to me.
>>
>> Acked-by: Neal Cardwell <ncardwell@...gle.com>
>>
>> BTW, I guess we should spread the word to maintainers of other major TCP
>> stacks that they need to be prepared for what may be a much higher degree
>> of compression/aggregation in the SACK stream. Linux stacks going back many
>> years should be fine with this, but I'm not sure about the other major OSes
>> (they may only allow sending one MSS per ACK-with-SACKs received).
> Patch looks really good but Neal's comment just reminds me a potential
> legacy issue.
>
> I recall at least Apple and Windows TCP stacks still need 3+ DUPACKs
> (!= a SACK covering 3+ packets) to trigger fast recovery. Will we have
> an issue there interacting w/ these stacks?
Offline chat w/ Eric: actually the problem already exists with GRO: a
Linux receiver could receive a OOO skb of say 5 pkts and returns one
(DUP)ACK w/ sack option covering 5 pkts.

Since no issues have been reported my concern is probably not big
deal. Hopefully other stacks can improve their sack / recovery
handling there.

>
>>
>> neal