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

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 2 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] Commit message claim "It fits in an existing hole,
  sizeof(struct tcp_sock) is unchanged" is wrong for the chosen position.
- [Medium] The invariant the commit message and include/net/tcp.h:787-793
  claim to maintain — "SOCK_NOSPACE set => tp->tcp_nospace set" — is not…
- [Low] The new field comment in include/linux/tcp.h states an invariant
  that in-tree code added by the same patch deliberately violates.

--

tcp: add tp->tcp_nospace

This adds a u8 mirror of SOCK_NOSPACE to struct tcp_sock so that
tcp_check_space() no longer has to touch sk->sk_socket->flags on every
incoming ACK and every transmitted packet.  The mirror is maintained by
sk_set_nospace()/sk_clear_nospace(), __tcp_check_space() keeps the
authoritative SOCK_NOSPACE test, and MPTCP subflows pin the mirror in
subflow_ulp_init().

A few questions below about the claimed layout and about the claimed
invariant.

[ ... ]

> diff --git a/include/linux/tcp.h b/include/linux/tcp.h
> index 6a8c77719322f..d51aae60aa45b 100644
> --- a/include/linux/tcp.h
> +++ b/include/linux/tcp.h
> @@ -307,6 +307,10 @@ struct tcp_sock {
>  		accecn_opt_demand:2,/* Demand AccECN option for n next ACKs */
>  		prev_ecnfield:2; /* ECN bits from the previous segment */
>  	__be32	pred_flags;
> +	u8	tcp_nospace;	/* mirrors SOCK_NOSPACE, but in a cache line
> +				 * that tcp_check_space() already needs.

[Severity: Medium]

The changelog says:

    Add tp->tcp_nospace, a mirror of SOCK_NOSPACE placed in the
    tcp_sock_write_txrx group [...] It fits in an existing hole,
    sizeof(struct tcp_sock) is unchanged.

Is there actually a hole at this particular position?

Counting from the 64-byte aligned start of the tcp_sock_write_tx group
(the __cacheline_group_begin/end markers are zero-length u8 arrays and
consume no space), ecn_flags ends at group-relative offset 145, so the
three u8 bitfield bytes that open tcp_sock_write_txrx occupy 145, 146 and
147:

	u8	nonagle : 4,
		rate_app_limited:1;			/* byte 145 */
	u8	received_ce_pending:4, ...		/* byte 146 */
	u8	accecn_minlen:2, ...			/* byte 147 */
	__be32	pred_flags;				/* 148..151 */
	u64	tcp_clock_cache;			/* 152 */

pred_flags then lands exactly at 148..151 and the 8-byte aligned
tcp_clock_cache exactly at 152, with no padding in between.  The 2-byte
hole that used to precede pred_flags was consumed by the AccECN u8
bitfields that now sit in front of it.

Doesn't inserting a u8 between pred_flags and tcp_clock_cache therefore
add 7 bytes of padding and push tcp_clock_cache, tcp_mstamp, rcv_nxt and
everything up to segs_out 8 bytes further into the hottest group, rather
than filling a hole?

Two places in the same cache line look genuinely free: the spare
unused2:3 bits in the received_ce_pending bitfield right above, or the
3-byte hole after u8 chrono_type in the tx group.

Would it be worth adding a CACHELINE_ASSERT_GROUP_SIZE() for struct
tcp_sock as well?  tcp_struct_check() only uses
CACHELINE_ASSERT_GROUP_MEMBER(), so an 8-byte growth of
tcp_sock_write_txrx is not caught at build time.  A pahole diff of
struct tcp_sock before and after would settle the "sizeof(struct
tcp_sock) is unchanged" claim either way.

> +				 * Can only be set if SOCK_NOSPACE is set.
> +				 */

[Severity: Low]

This comment says the mirror "Can only be set if SOCK_NOSPACE is set",
but subflow_ulp_init() in net/mptcp/subflow.c sets tp->tcp_nospace = 1
at ULP init time, when the shared struct socket has SOCK_NOSPACE clear,
and tcp_clear_nospace() then refuses to ever clear it again for MPTCP
subflows.

The direction the rest of the patch relies on is the reverse one
(SOCK_NOSPACE set => mirror set), which is what the comment block above
tcp_set_nospace() in include/net/tcp.h and the changelog describe.
Could this field comment be reworded to state that direction instead?
As written, the two headers document opposite contracts for the same
field, and a reader trusting this one might drop the SOCK_NOSPACE
re-test just added to __tcp_check_space(), or add a warning for
"mirror set but SOCK_NOSPACE clear", and break MPTCP.

>  	u64	tcp_clock_cache; /* cache last tcp_clock_ns() (see tcp_mstamp_refresh()) */
>  	u64	tcp_mstamp;	/* most recent packet received/sent */
>  	u32	rcv_nxt;	/* What we want to receive next		*/
> diff --git a/include/net/tcp.h b/include/net/tcp.h
> index 5e5f5f9b89a38..1e1950dd184ec 100644
> --- a/include/net/tcp.h
> +++ b/include/net/tcp.h
> @@ -783,14 +783,35 @@ void tcp_done_with_error(struct sock *sk, int err);
>  void tcp_reset(struct sock *sk, struct sk_buff *skb);
>  void tcp_fin(struct sock *sk);
>  void __tcp_check_space(struct sock *sk);
> +
> +/* Mirror of SOCK_NOSPACE in tcp_sock, maintained by sk_set_nospace()
> + * and sk_clear_nospace().
> + *
> + * MPTCP subflows share the parent socket, and thus its SOCK_NOSPACE bit.
> + * Keep their mirror always set (see subflow_ulp_init()) so that they
> + * always reach __tcp_check_space() and behave as before.
> + */
> +static inline void tcp_set_nospace(struct sock *sk)
> +{
> +	if (sk_is_tcp(sk))
> +		WRITE_ONCE(tcp_sk(sk)->tcp_nospace, 1);
> +}
> +
> +static inline void tcp_clear_nospace(struct sock *sk)
> +{
> +	if (sk_is_tcp(sk) && !sk_is_mptcp(sk))
> +		WRITE_ONCE(tcp_sk(sk)->tcp_nospace, 0);
> +}
> +
>  static inline void tcp_check_space(struct sock *sk)
>  {
>  	/* pairs with tcp_poll() */
>  	smp_mb();
>  
> -	if (sk->sk_socket && test_bit(SOCK_NOSPACE, &sk->sk_socket->flags))
> +	if (unlikely(READ_ONCE(tcp_sk(sk)->tcp_nospace)))
>  		__tcp_check_space(sk);
>  }
> diff --git a/net/core/sock.c b/net/core/sock.c
> index 11a22aec7e414..dce4e8e4e2bbe 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -2969,8 +2969,13 @@ void sk_set_nospace(struct sock *sk)
>  {
>  	struct socket *sock = sk->sk_socket;
>  
> -	if (sock)
> -		set_bit(SOCK_NOSPACE, &sock->flags);
> +	if (!sock)
> +		return;
> +	/* Mirror first: callers relying on the barrier implied by
> +	 * set_bit() + smp_mb__after_atomic() are then also covered.
> +	 */
> +	tcp_set_nospace(sk);
> +	set_bit(SOCK_NOSPACE, &sock->flags);
>  }
>  EXPORT_SYMBOL(sk_set_nospace);
>  
> @@ -2985,8 +2990,13 @@ void sk_clear_nospace(struct sock *sk)
>  {
>  	struct socket *sock = sk->sk_socket;
>  
> -	if (sock)
> -		clear_bit(SOCK_NOSPACE, &sock->flags);
> +	if (!sock)
> +		return;
> +	clear_bit(SOCK_NOSPACE, &sock->flags);
> +	/* Mirror last: a stale mirror only costs a slow path, while a
> +	 * stale SOCK_NOSPACE would cost a missed EPOLLOUT.
> +	 */
> +	tcp_clear_nospace(sk);
>  }
>  EXPORT_SYMBOL(sk_clear_nospace);

[Severity: Medium]

Can the invariant the changelog states actually hold?

    The two flags are now only changed from sk_set_nospace() and
    sk_clear_nospace(), which maintain this invariant:

            SOCK_NOSPACE set  =>  tp->tcp_nospace set

The set side stores the mirror first and then the bit, while the clear
side clears the bit first and then the mirror, so the two non-atomic
stores are issued in opposite order on the two sides.  sk_set_nospace()
runs without the socket lock from tcp_poll(), while sk_clear_nospace()
for TCP runs under the socket lock from
tcp_check_space()->__tcp_check_space()->tcp_new_space()->sk_stream_write_space().

CPU0, in tcp_poll(), no socket lock:
	tcp_set_nospace(sk);			/* mirror = 1 */
	/* delayed here */

CPU1, socket lock held, processing an ACK:
	sk_clear_nospace(sk)
		clear_bit(SOCK_NOSPACE, &sock->flags);
		tcp_clear_nospace(sk);		/* mirror = 0 */

CPU0 resumes:
	set_bit(SOCK_NOSPACE, &sock->flags);	/* bit = 1, mirror = 0 */

Doesn't that leave SOCK_NOSPACE set with tp->tcp_nospace clear, the
combination the changelog declares impossible?  It looks like a lost
update across two separate locations, so no barrier or WRITE_ONCE
placement fixes it, and reversing the store order in sk_clear_nospace()
just makes the symmetric interleaving possible instead.

While that state holds, tcp_check_space() short-circuits on every ACK
and every transmitted packet, so tcp_new_space() and
sk->sk_write_space() are never reached: no SOCK_NOSPACE clearing, no
EPOLLOUT/SIGIO wake, no tcp_sndbuf_expand() autotuning, and no
tcp_chrono_stop(TCP_CHRONO_SNDBUF_LIMITED).  Before this patch the same
race could only end up in the safe combination, since the bit being set
always took the slow path.

For poll()/epoll() this seems to self-heal: sk_clear_nospace() in
sk_stream_write_space() is immediately followed by
wake_up_interruptible_poll(EPOLLOUT ...) while the waiter is already
queued, and tcp_poll() re-arms the mirror whenever it finds the socket
not writeable.  sendmsg() callers take the socket lock and cannot race
at all.

The case that does not look self-healing is in-kernel users whose only
re-arm path is the suppressed ->sk_write_space() callback and whose
sk_set_nospace() is lock-free, for example send_to_sock() in
fs/dlm/lowcomms.c:

	sk_set_nospace(con->sock->sk);
	con->sock->sk->sk_write_pending++;

whose retry comes from lowcomms_write_space() requeueing con->swork, and
xs_nospace() in net/sunrpc/xprtsock.c, whose retry comes from
xs_write_space()->xprt_write_space().  Can a transmit stall there until
an unrelated event or an RPC timeout?

Would re-testing SOCK_NOSPACE after the clear (with a full barrier) and
re-arming the mirror, or only clearing the mirror together with a
re-validation by the socket owner, be enough to restore the stated
invariant?

[ ... ]

> 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;
>  	ctx->icsk_af_ops = icsk->icsk_af_ops;
>  	icsk->icsk_af_ops = subflow_default_af_ops(sk);
>  	ctx->tcp_state_change = sk->sk_state_change;

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260922122721.3568295-1-edumazet%40google.com