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

On Thu, May 17, 2018 at 11:40 AM Eric Dumazet [off-list ref]
wrote:

On 05/17/2018 08:14 AM, Neal Cardwell wrote:

quoted

Is there a particular motivation for the cap of 127? IMHO 127 ACKs is

quite

quoted

a few to compress. Experience seems to show that it works well to have

one

quoted

GRO ACK for ~64KBytes that triggers a single TSO skb of ~64KBytes. It

might

quoted

be nice to try to match those dynamics in this SACK compression case,

so it

quoted

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

quoted

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.  :-)

quoted

quoted

+       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);

quoted

Any particular motivation for the 2.5ms here? It might be nice to match

the

quoted

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

quoted

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@google.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).

neal