Re: [PATCH net] tcp: initialize send sequence numbers before deferring TFO

[Neal Cardwell]

On Tue, Nov 3, 2026 at 12:32 AM Kuniyuki Iwashima <kuniyu@xxxxxxxxxx> wrote:
>
> On Mon, 02 Nov 2026 09:20:41 +0100, Eric Dumazet wrote:
> > Le lun. 2 nov. 2026 à 09:13, Qingfang Deng <qingfang.deng@xxxxxxxxx> a écrit :
> > >
> > > On 2026/11/2 16:00, Eric Dumazet wrote:
> > > > Le lun. 2 nov. 2026 à 08:41, Qingfang Deng <qingfang.deng@xxxxxxxxx> a écrit :
> > > >> On 2026/11/2 14:11, Jiayuan Chen wrote:
> > > >>> On 11/2/26 11:59 AM, Qingfang Deng wrote:
> > > >>>> TCP_FASTOPEN_CONNECT can return from connect() before tcp_connect_init()
> > > >>>> when a cached cookie or cookieless Fast Open allows deferring the SYN.
> > > >>>> At this point write_seq contains the initial sequence number, while
> > > >>>> snd_una and snd_nxt have not been initialized to that sequence number.
> > > >>>>
> > > >>>> Consequently, inet_diag and /proc/net/tcp{,6} report the initial
> > > >>>> sequence number as the send queue length despite no SYN or payload being
> > > >>>> queued.
> > > >>>>
> > > >>>> Initialize snd_una and snd_nxt to write_seq before marking the
> > > >>>> connection as deferred. Both sequence-based queue lengths then remain
> > > >>>> zero until the first write starts the connection and runs
> > > >>>> tcp_connect_init().
> > > >>> This patch looks good to me. But idiag_wqueue can still go negative
> > > >>> on the normal connect path, since the socket is already hashed and
> > > >>> diag does not take the socket lock. We always set write_seq first,
> > > >>> then init snd_una later:
> > > >>>
> > > >>>     tcp_v4_connect
> > > >>>       WRITE_ONCE(tp->write_seq, st.seq)
> > > >>>       tcp_connect
> > > >>>         tcp_connect_init
> > > >>>           WRITE_ONCE(tp->snd_una, tp->write_seq)
> > > >>>
> > > >>> Can you try this instead?
> > > >>>
> > > >>>     r->idiag_wqueue = READ_ONCE(sk->sk_wmem_queued) ?
> > > >>>                       READ_ONCE(tp->write_seq) - READ_ONCE(tp->snd_una) : 0;
> > > >> This does not guarantee the load order on a weakly-ordered CPU. LLM
> > > >> suggests using this load-acquire/store-release sequence:
> > > >>
> > > >> r->idiag_wqueue = smp_load_acquire(&sk->sk_wmem_queued) ?
> > > >> READ_ONCE(tp->write_seq) - READ_ONCE(tp->snd_una) : 0;
> > > >>
> > > >> Then in tcp_connect_queue_skb(), replace sk_wmem_queued_add()
> > > >> with smp_store_release():
> > > >>
> > > >> /* Paired with smp_load_acquire() in tcp_diag_get_info(). */
> > > >> smp_store_release(&sk->sk_wmem_queued, sk->sk_wmem_queued + skb->truesize);
> > > >>
> > > >> What do you think?
> > > > Please do not do this.
> > > >
> > > > /proc/net/tcp and inet_diag dump sockets without holding the socket
> > > > lock. They provide a best effort snapshot, and transient inconsistent
> > > > values are expected (see the comment about rx_queue in get_tcp4_sock()).
> > > >
> > > > The window in tcp_v4_connect()/tcp_v6_connect() is a few usec, with the
> > > > socket lock held by the caller. Adding memory barriers in the TCP stack,
> > > > on a field updated in many places, to cover this is not worth it.
> > >
> > > tcp_connect_queue_skb(), as the name suggests, only queues the
> > > connecting SYN. So this adds a small setup cost on weakly-ordered CPUs
> > > (on x86, smp_store_release degrades to a compiler barrier).
> > >
> > > If strong memory ordering is not required, do you prefer Jiayuan's approach?
> >
> > Neither, let's keep v1 as is.
> >
> > I know tcp_connect_queue_skb() only queues the SYN. The cost is not the
> > problem; sk_wmem_queued has many other writers, and one release store
> > among them is a special ordering rule that nobody will remember the
> > next time this code is changed. This is too much complexity to hide a
> > transient value in a best-effort dump.
>
> +1.  v1 looks good to me too.
>
> Reviewed-by: Kuniyuki Iwashima <kuniyu@xxxxxxxxxx>

v1 LGTM as well.

Reviewed-by: Neal Cardwell <ncardwell@xxxxxxxxxx>

thanks,
neal