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 PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Fri, 27 Dec 2019 12:32:26 -0500 From: Neal Cardwell <ncardwell@...gle.com> To: Eric Dumazet <edumazet@...gle.com> Cc: "David S . Miller" <davem@...emloft.net>, netdev <netdev@...r.kernel.org>, Eric Dumazet <eric.dumazet@...il.com>, Soheil Hassas Yeganeh <soheil@...gle.com>, Yuchung Cheng <ycheng@...gle.com>, Martin KaFai Lau <kafai@...com> Subject: Re: [PATCH net-next v2 0/5] tcp_cubic: various fixes On Mon, Dec 23, 2019 at 3:28 PM Eric Dumazet <edumazet@...gle.com> wrote: > > This patch series converts tcp_cubic to usec clock resolution > for Hystart logic. > > This makes Hystart more relevant for data-center flows. > Prior to this series, Hystart was not kicking, or was > kicking without good reason, since the 1ms clock was too coarse. > > Last patch also fixes an issue with Hystart vs TCP pacing. > > v2: removed a last-minute debug chunk from last patch > > Eric Dumazet (5): > tcp_cubic: optimize hystart_update() > tcp_cubic: remove one conditional from hystart_update() > tcp_cubic: switch bictcp_clock() to usec resolution > tcp_cubic: tweak Hystart detection for short RTT flows > tcp_cubic: make Hystart aware of pacing > > net/ipv4/tcp_cubic.c | 82 +++++++++++++++++++++++++++----------------- > 1 file changed, 51 insertions(+), 31 deletions(-) Thanks for this very nice patch series, Eric. In reviewing these patches and thinking about the Hystart ACK train heuristic, I am thinking that another behavior that could fool this heuristic and cause a spurious/early Hystart exit of slow start would be application-limited flights of data. In other words, just as pacing can cause inter-ACK spacing increases that could spuriously trigger the Hystart ACK train heuristic, AFAICT gaps between application writes could also cause inter-ACK gaps that could spuriously trigger the Hystart ACK train heuristic. AFAICT to avoid such spurious exits of slow start we ought to pass in the is_app_limited bool in the struct ack_sample, and thereby pass it through pkts_acked(), bictcp_acked(), and hystart_update(). @@ -3233,9 +3233,12 @@ static int tcp_clean_rtx_queue(struct sock *sk, u32 prior_fack, } if (icsk->icsk_ca_ops->pkts_acked) { - struct ack_sample sample = { .pkts_acked = pkts_acked, - .rtt_us = sack->rate->rtt_us, - .in_flight = last_in_flight }; + struct ack_sample sample = { + .pkts_acked = pkts_acked, + .rtt_us = sack->rate->rtt_us, + .in_flight = last_in_flight, + .is_app_limited = sack->rate->is_app_limited, + }; icsk->icsk_ca_ops->pkts_acked(sk, &sample); } ...and then only trigger the HYSTART_ACK_TRAIN heuristic to exit slow start if !sample->is_app_limited. This could be a follow-on patch after this series, or an additional patch at the end of this series. WDYT? neal
Powered by blists - more mailing lists