Re: [PATCH v2 net] tcp: Enable header prediction for active open connections with MD5.

[Kuniyuki Iwashima <kuniyu@amazon.com>]

From: Eric Dumazet <edumazet@google.com>
Date: Thu, 3 Aug 2023 08:44:14 +0200
> On Thu, Aug 3, 2023 at 6:22 AM Kuniyuki Iwashima <kuniyu@amazon.com> wrote:
> >
> > TCP socket saves the minimum required header length in tcp_header_len
> > of struct tcp_sock, and later the value is used in __tcp_fast_path_on()
> > to generate a part of TCP header in tcp_sock(sk)->pred_flags.
> >
> > In tcp_rcv_established(), if the incoming packet has the same pattern
> > with pred_flags, we enter the fast path and skip full option parsing.
> >
> > The MD5 option is parsed in tcp_v[46]_rcv(), so we need not parse it
> > again later in tcp_rcv_established() unless other options exist.  Thus,
> > MD5 should add TCPOLEN_MD5SIG_ALIGNED to tcp_header_len and avoid the
> > slow path.
> >
> > For passive open connections with MD5, we add TCPOLEN_MD5SIG_ALIGNED
> > to tcp_header_len in tcp_create_openreq_child() after 3WHS.
> >
> > On the other hand, we do it in tcp_connect_init() for active open
> > connections.  However, the value is overwritten while processing
> > SYN+ACK or crossed SYN in tcp_rcv_synsent_state_process().
> >
> >   1) SYN+ACK
> >
> >     tcp_rcv_synsent_state_process
> >       tp->tcp_header_len = sizeof(struct tcphdr) or
> >                            sizeof(struct tcphdr) + TCPOLEN_TSTAMP_ALIGNED
> >       tcp_finish_connect
> >         __tcp_fast_path_on
> >       tcp_send_ack
> >
> >   2) Crossed SYN and the following ACK
> >
> >     tcp_rcv_synsent_state_process
> >       tp->tcp_header_len = sizeof(struct tcphdr) or
> >                            sizeof(struct tcphdr) + TCPOLEN_TSTAMP_ALIGNED
> >       tcp_set_state(sk, TCP_SYN_RECV)
> >       tcp_send_synack
> >
> >     -- ACK received --
> >     tcp_v4_rcv
> >       tcp_v4_do_rcv
> >         tcp_rcv_state_process
> >           tcp_fast_path_on
> >             __tcp_fast_path_on
> >
> > So these two cases will have the wrong value in pred_flags and never
> > go into the fast path.
> >
> > Let's add TCPOLEN_MD5SIG_ALIGNED in tcp_rcv_synsent_state_process()
> > to enable header prediction for active open connections.
> 
> I do not think we want to slow down fast path (no MD5), for 'header
> prediction' of MD5 flows,
> considering how slow MD5 is anyway (no GSO/GRO), and add yet another
> ugly #ifdef CONFIG_TCP_MD5SIG
> in already convoluted code base.
> 
> The case of cross-syn is kind of hilarious, if you ask me.
> 
> >
> > Fixes: cfb6eeb4c860 ("[TCP]: MD5 Signature Option (RFC2385) support.")
> 
> This would be net-next material anyway, unless you show a huge
> improvement after this patch,
> which I doubt very much.

I just noticed the MD5 does not add TCPOLEN_MD5SIG_ALIGNED and thought
it would benefit from the fast path, but sorry, you were correct.

I have tested with slightly modified netperf that uses MD5 for each
flow and found that the patch does not improve MD5 perf at all.  Rather,
without all TCPOLEN_MD5SIG_ALIGNED addition in tcp_connect_init() and
tcp_create_openreq_child(), the slow path became the faster path for MD5.

  On c5.4xlarge EC2 instance (16 vCPU, 32 GiB mem)

  $ for i in {1..10}; do
  ./super_netperf $(nproc) -H localhost -l 10 -- -m 256 -M 256 2>/dev/null;
  done

  - 36e68eadd303  : 10.376 Gbps
  - all fast path : 10.374 Gbps
  - all slow path : 10.394 Gbps

I'll post v3 that disable fast path for MD5 instead.

Thanks!