netdev - Re: [PATCH net-next v2 0/5] tcp

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

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

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

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