Re: [PATCH net-next 1/4] tcp: sync MSS after enabling MTU probing on passive connections

[Kuniyuki Iwashima]

On Sat, Nov 7, 2026 at 7:17 PM Neal Cardwell <ncardwell@xxxxxxxxxx> wrote:
>
> On Sat, Nov 7, 2026 at 2:15 PM Kuniyuki Iwashima <kuniyu@xxxxxxxxxx> wrote:
> >
> > On Sat, 07 Nov 2026 12:53:36 -0500, Neal Cardwell wrote:
> > > On Tue, Nov 3, 2026 at 2:09 AM Yongjian Wang via B4 Relay
> > > <devnull+echoowall.gmail.com@xxxxxxxxxx> wrote:
> > > >
> > > > From: Yongjian Wang <echoowall@xxxxxxxxx>
> > > >
> > > > tcp_v4_syn_recv_sock() and tcp_v6_syn_recv_sock() call tcp_sync_mss()
> > > > before MTU probing is enabled, and tcp_init_transfer() enables it later
> > > > without syncing the MSS again.
> > > >
> > > > With net.ipv4.tcp_mtu_probing=2, a passive connection therefore starts
> > > > at the full MSS instead of tcp_base_mss as documented, while an active
> > > > connection starts at tcp_base_mss. This has two visible effects, shown
> > > > here with tcp_base_mss=1024:
> > > >
> > > >  - The MSS drops when the first probe is acknowledged, from 1460 to 1242,
> > > >    although that probe has just shown the path can carry more.
> > > >
> > > >  - On a black hole path the MSS never tries tcp_base_mss. It goes from
> > > >    1460 straight to 512, since MTU probing is already enabled when black
> > > >    hole detection triggers.
> > > >
> > > > Sync the MSS right after tcp_mtup_init(). tcp_sync_mss() only applies
> > > > search_low when MTU probing is enabled, so the other tcp_mtu_probing
> > > > values keep their behaviour.
> > > >
> > > > The Fixes commit kept tcp_mtup_init() in a single place on purpose, so
> > > > sync the MSS next to it instead of moving it back into the
> > > > syn_recv_sock() handlers.
> > > >
> > > > Fixes: 4d83e1773031 ("tcp: delete redundant calls of tcp_mtup_init()")
> > > > Signed-off-by: Yongjian Wang <echoowall@xxxxxxxxx>
> > > > ---
> > > >  net/ipv4/tcp_input.c | 1 +
> > > >  1 file changed, 1 insertion(+)
> > > >
> > > > diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> > > > index e1f2d9765..c13f005b2 100644
> > > > --- a/net/ipv4/tcp_input.c
> > > > +++ b/net/ipv4/tcp_input.c
> > > > @@ -6760,6 +6760,7 @@ void tcp_init_transfer(struct sock *sk, int bpf_op, struct sk_buff *skb)
> > > >         struct tcp_sock *tp = tcp_sk(sk);
> > > >
> > > >         tcp_mtup_init(sk);
> > > > +       tcp_sync_mss(sk, icsk->icsk_pmtu_cookie);
> > > >         icsk->icsk_af_ops->rebuild_header(sk);
> > > >         tcp_init_metrics(sk);
> > > >
> > > >
> > > > --
> > >
> > > Functionally this seems correct. But I'm wondering about the
> > > performance impact, and whether there is a way to achieve this without
> > > adding this call to the code path for every connection establishment?
> > >
> > > Eric and Kuniyuki, WDYT?
> >
> > Looking at history, initially it seems we properly called
> > tcp_mtup_init() right before tcp_sync_mss() in ->syn_recv_sock(),
> > but 4d83e1773031 removed them.
> >
> > The dup tcp_mtup_init() mentioned in the commit is the one in
> > tcp_init_transfer() today.
> >
> > Then, 27204aaa9dc6 moved only tcp_mtup_init() to tcp_init_transfer()
> > without tcp_sync_mss().
> >
> > The dup exists in tcp_rcv_state_process() from day 0 (5d424d5a674f),
> > and 168a8f58059a copy-and-pasted the unnecessary tcp_mtup_init().
> >
> > So, 4d83e1773031 removed the correct one.
> >
> > Instead of restoring the 2nd set of calls, I think we should
> > revert 4d83e1773031 and remove tcp_mtup_init() in tcp_init_transfer().
> >
> > ---8<---
> > diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> > index e1f2d9765f81..8d009c0d9359 100644
> > --- a/net/ipv4/tcp_input.c
> > +++ b/net/ipv4/tcp_input.c
> > @@ -6759,7 +6759,6 @@ void tcp_init_transfer(struct sock *sk, int bpf_op, struct sk_buff *skb)
> >         struct inet_connection_sock *icsk = inet_csk(sk);
> >         struct tcp_sock *tp = tcp_sk(sk);
> >
> > -       tcp_mtup_init(sk);
> >         icsk->icsk_af_ops->rebuild_header(sk);
> >         tcp_init_metrics(sk);
> >
> > diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
> > index a7f9242f1e42..579a88472abf 100644
> > --- a/net/ipv4/tcp_ipv4.c
> > +++ b/net/ipv4/tcp_ipv4.c
> > @@ -1740,6 +1740,7 @@ struct sock *tcp_v4_syn_recv_sock(const struct sock *sk, struct sk_buff *skb,
> >
> >         tcp_ca_openreq_child(newsk, dst);
> >
> > +       tcp_mtup_init(newsk);
> >         tcp_sync_mss(newsk, dst4_mtu(dst));
> >         newtp->advmss = tcp_mss_clamp(tcp_sk(sk), tcp_dst_advmss(dst));
> >
> > diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
> > index e006a30360ea..c465fd97baec 100644
> > --- a/net/ipv6/tcp_ipv6.c
> > +++ b/net/ipv6/tcp_ipv6.c
> > @@ -1489,6 +1489,7 @@ static struct sock *tcp_v6_syn_recv_sock(const struct sock *sk, struct sk_buff *
> >
> >         tcp_ca_openreq_child(newsk, dst);
> >
> > +       tcp_mtup_init(newsk);
> >         tcp_sync_mss(newsk, dst6_mtu(dst));
> >         newtp->advmss = tcp_mss_clamp(tcp_sk(sk), tcp_dst_advmss(dst));
> >
> > ---8<---
>
> Thanks, Kuniyuki. I agree that seems like a good approach. Though
> since 27204aaa9dc6 also moved the tcp_mtup_init() out of
> tcp_rcv_synsent_state_process(), I think with your nice approach we'd
> also want to place a call to tcp_mtup_init(sk) right before the
> tcp_sync_mss() call on the active connect code path as well, something
> like:
>
> --- a/net/ipv4/tcp_input.c
> +++ b/net/ipv4/tcp_input.c
> @@ -7015,6 +7015,7 @@ static int tcp_rcv_synsent_state_process(struct
> sock *sk, struct sk_buff *skb,
>                   tp->tcp_header_len = sizeof(struct tcphdr);
>           }
>
> +        tcp_mtup_init(sk);
>           tcp_sync_mss(sk, icsk->icsk_pmtu_cookie);
>           tcp_initialize_rcv_mss(sk);
>
> WDYT?

Ah exactly, thanks for catching :)