Re: [PATCH mptcp-next v3 2/4] mptcp: sched: penalise a slow subflow by halving its cwnd

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] A 64-bit integer overflow in the EWMA `avg_pacing_rate` calculation incorrectly cripples highly performant subflows.
- [Medium] Bypassing TCP Congestion Control API corrupts internal state and renders the penalty ineffective.
- [Medium] Unsynchronized lockless reads of TCP subflow internal states and bitfields cause data races.
--

--- Patch [2]: [PATCH mptcp-next v3 2/4] mptcp: sched: penalise a slow subflow by halving its cwnd ---
commit 07a5e01113da906e5797c2902f8c9a2e4065ed23
Author: Shardul Bankar <shardul.b@mpiricsoftware.com>
Subject: mptcp: sched: penalise a slow subflow by halving its cwnd

This commit introduces a mechanism to penalize poorly performing subflows
by halving their congestion window. It flags subflows for penalization in
the scheduler when their pacing rate falls significantly behind the fastest
path, and applies the reduction under the subflow socket lock.

Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/345

> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
[ ... ]
> +static void mptcp_penalise_cwnd(struct sock *ssk)
> +{
> +	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk);
> +	struct tcp_sock *tp = tcp_sk(ssk);
> +	u32 cwnd = tcp_snd_cwnd(tp);
> +
> +	subflow->penalise = false;
> +	/* CA state may have left TCP_CA_Open since get_send flagged this */
> +	if (inet_csk(ssk)->icsk_ca_state != TCP_CA_Open)
> +		return;
> +	subflow->last_penalise = tcp_jiffies32;
> +	tcp_snd_cwnd_set(tp, max_t(u32, cwnd >> 1, MPTCP_PENALISE_MIN_CWND));
> +	if (cwnd >= tp->snd_ssthresh)
> +		tp->snd_ssthresh = max_t(u32, tp->snd_ssthresh >> 1, 2);

[Severity: Medium]
Will bypassing the TCP congestion control API work for modern algorithms?

Algorithms like BBR or CUBIC explicitly ignore external modifications to
snd_cwnd and might immediately overwrite or inflate the window back up on the
next ACK. 

BBR tracks its own window internally, so the penalty here might be instantly
undone and rendered ineffective.

> +}
[ ... ]
> @@ -1685,12 +1727,24 @@ struct sock *mptcp_subflow_get_send(struct mptcp_sock *msk)
>  	if (!ssk || !sk_stream_memory_free(ssk))
>  		return NULL;
>  
> +	/* Flag a slow subflow for cwnd halving, applied in the push path */
> +	subflow = mptcp_subflow_ctx(ssk);
> +	penal_cand = fastest && ssk != fastest &&
> +		     subflow->avg_pacing_rate < max_pace / MPTCP_PENALISE_RATE_RATIO;
> +	subflow->penalise = penal_cand &&
> +			    tcp_snd_cwnd(tcp_sk(ssk)) > MPTCP_PENALISE_MIN_CWND &&
> +			    inet_csk(ssk)->icsk_ca_state == TCP_CA_Open &&
> +			    tcp_is_cwnd_limited(fastest) &&

[Severity: Medium]
Are we safe from data races when reading tcp_is_cwnd_limited(fastest)
locklessly?

The scheduler is running under the MPTCP connection lock, but does not hold
the individual subflow socket locks. tcp_is_cwnd_limited is a bitfield
sharing a byte with other heavily modified TCP flags.

Concurrent updates by the TCP stack to adjacent flags in the bitfield during
ACK processing require Read-Modify-Write operations. This lockless read
might trigger KCSAN splats.

> +			    mptcp_penalise_throttle_ok(subflow);
> +
>  	burst = min(MPTCP_SEND_BURST_SIZE, mptcp_wnd_end(msk) - msk->snd_nxt);
>  	wmem = READ_ONCE(ssk->sk_wmem_queued);
> -	if (!burst)
> +	if (!burst) {
> +		/* no window room to send; skip the penalty */
> +		subflow->penalise = false;
>  		return ssk;
> +	}
>  
> -	subflow = mptcp_subflow_ctx(ssk);
>  	subflow->avg_pacing_rate = div_u64((u64)subflow->avg_pacing_rate * wmem +

[Severity: High]
Could this EWMA calculation overflow a 64-bit integer on high-throughput
links?

If a highly performant subflow, like 100Gbps (around 12.5 GB/s), has a large
write queue (wmem around 1.5 GB), the product of (u64)avg_pacing_rate * wmem
might exceed the u64 maximum of ~18.44 * 10^18.

This would wrap around modulo 2^64 and drastically reduce avg_pacing_rate.
The scheduler might then falsely flag this highly performant subflow as
a penalty candidate and repeatedly halve its congestion window.

>  					   (u64)READ_ONCE(ssk->sk_pacing_rate) * burst,
>  					   burst + wmem);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260817-mptcp_penalise_send_v2-v3-0-e6a2ad2f1b82@mpiricsoftware.com?part=2