Re: [PATCH v3 net-next 9/9] tcp: add tp->tcp_nospace

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message says struct socket lives in a cache line
  "which the TCP fast paths do not otherwise touch".
- [Low] A child socket inherits the listener's tp->tcp_nospace and
  nothing resets it.

--

tcp: add tp->tcp_nospace

This commit adds tp->tcp_nospace, a copy of SOCK_NOSPACE that lives in the
tcp_sock_write_tx cache line group. With it, tcp_check_space() no longer
has to read sk->sk_socket->flags on every ACK and every transmit.
sk_set_nospace() and sk_clear_nospace() keep the rule that SOCK_NOSPACE
set implies tp->tcp_nospace set. MPTCP subflows pin tp->tcp_nospace to 1
in subflow_ulp_init().

> struct socket lives in its own cache line, which the TCP fast paths do
> not otherwise touch, so testing sk->sk_socket->flags pulls in an extra
> cache line that is cold when the working set is large.

[Severity: Low]
Is this still accurate after this patch? Only tcp_check_space() was
converted to use the new field.

tcp_write_xmit() calls tcp_cwnd_validate() whenever it has sent packets.
If the flow is not cwnd limited and the write queue is empty,
tcp_cwnd_validate() still reads sk->sk_socket->flags:

net/ipv4/tcp_output.c:tcp_cwnd_validate() {
    ...
		if (tcp_write_queue_empty(sk) && sk->sk_socket &&
		    test_bit(SOCK_NOSPACE, &sk->sk_socket->flags) &&
		    (1 << sk->sk_state) & (TCPF_ESTABLISHED | TCPF_CLOSE_WAIT))
			tcp_chrono_start(sk, TCP_CHRONO_SNDBUF_LIMITED);
    ...
}

tcp_cwnd_application_limited() also reads it on the
slow_start_after_idle path:

net/ipv4/tcp_output.c:tcp_cwnd_application_limited() {
    ...
	if (inet_csk(sk)->icsk_ca_state == TCP_CA_Open &&
	    sk->sk_socket && !test_bit(SOCK_NOSPACE, &sk->sk_socket->flags)) {
    ...
}

Request/response senders limited by the application empty the write
queue on every transmit. Their transmits would still pull in the struct
socket cache line. The microbenchmark only times tcp_check_space(), so it
would not show this.

The new rule (SOCK_NOSPACE set => tp->tcp_nospace set) means both sites
could check READ_ONCE(tp->tcp_nospace) first without changing behavior.
Would that be worth doing? If not, could the commit message avoid saying
the fast paths no longer touch struct socket?

[ ... ]

> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index f0a6725d2c376..e297c88be503b 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -2001,6 +2001,11 @@ static int subflow_ulp_init(struct sock *sk)
>  	pr_debug("subflow=%p, family=%d\n", ctx, sk->sk_family);
>  
>  	tp->is_mptcp = 1;
> +	/* Subflows share the MPTCP socket, and thus its SOCK_NOSPACE bit,
> +	 * which tcp_check_space() can not mirror. Pin the mirror so that
> +	 * __tcp_check_space() always tests the shared bit.
> +	 */
> +	tp->tcp_nospace = 1;

[Severity: Low]
Do passive children of an MPTCP listener inherit this pinned value, even
when they fall back to plain TCP?

The listener subflow gets tcp_nospace = 1 here through
mptcp_subflow_create_socket()->tcp_set_ulp()->subflow_ulp_init(). Its
is_mptcp stays 1, so tcp_clear_nospace() never clears it.

tcp_create_openreq_child()->inet_csk_clone_lock()->sk_clone_lock() then
copies the tcp_sock, and nothing resets tcp_nospace.

When the SYN comes from a peer without MPTCP,
subflow_ulp_clone()->subflow_ulp_fallback() makes the child plain TCP:

	tcp_sk(sk)->is_mptcp = 0;

mptcp_stream_accept() then grafts it as a plain TCP socket. Its new
struct socket has SOCK_NOSPACE clear. This new early return in
__tcp_check_space() then applies:

	if (!sock || !test_bit(SOCK_NOSPACE, &sock->flags))
		return;

So tcp_new_space()->sk_stream_write_space()->sk_clear_nospace() is never
reached, and that is the only place the hint gets cleared. It stays set
until the application first fills its send buffer.

Until then, every ACK and every transmit calls __tcp_check_space() out of
line and reads sk->sk_socket->flags. These connections would not get the
optimization.

The rule still holds, so no EPOLLOUT is lost. Could tcp_nospace be reset
for the child, for example in tcp_create_openreq_child() or in
subflow_ulp_fallback()?

The same can happen with a plain TCP listener. If a socket was connected,
ended up with tcp_nospace = 1, was disconnected and then put into
listen(), its children inherit the stale value too. tcp_disconnect() does
not reset it either.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260929071743.23624-1-edumazet%40kernel.org