Re: [PATCH net-next v2 2/5] mptcp: let the retrans scheduler do its job

On 7/31/26 4:24 PM, Matthieu Baerts (NGI0) wrote:
> @@ -2851,14 +2849,88 @@ static void __mptcp_retrans(struct sock *sk)
>  	struct mptcp_sock *msk = mptcp_sk(sk);
>  	struct mptcp_subflow_context *subflow;
>  	struct mptcp_data_frag *dfrag;
> +	bool need_retrans;
> +	u64 retrans_seq;
>  	int err, len;
>  
> -	mptcp_clean_una_wakeup(sk);
> -
> -	/* first check ssk: need to kick "stale" logic */
> -	err = mptcp_sched_get_retrans(msk);
> +	mptcp_data_lock(sk);
> +	__mptcp_clean_una_wakeup(sk);
> +	retrans_seq = msk->snd_una;
>  	dfrag = mptcp_rtx_head(sk);
> -	if (!dfrag) {
> +	need_retrans = !!dfrag;
> +	mptcp_data_unlock(sk);
> +	if (!dfrag)
> +		goto check_data_fin;

Clashiko notes that the above will not kick the stale logic anymore when
all send data has been acked, and there is pending, unsent data.

That is intentional and safe: if no data is in-flight, we don't need
stale subflows detection.

> +
> +	for (;;) {
> +		bool already_retrans;
> +		u64 sent_seq;
> +
> +		/* The default scheduler will kick "stale" logic, that in
> +		 * turn can process incoming acks and clean the RTX queue;
> +		 * ensure that the current dfrag will still be around
> +		 * afterwards.
> +		 */
> +		get_page(dfrag->page);
> +		err = mptcp_sched_get_retrans(msk);

Clashiko noted that the above can cause bad accounting for the stale
logic. This needs to be fixed.

> +		if (err) {
> +			put_page(dfrag->page);
> +			break;
> +		}
> +
> +		/* Incoming acks can have moved retrans sequence after
> +		 * the current dfrag, if so try to start again from RTX head.
> +		 */
> +		mptcp_data_lock(sk);
> +		already_retrans = !before64(msk->snd_una, dfrag->data_seq +
> +					    dfrag->already_sent);
> +		put_page(dfrag->page);
> +		if (already_retrans) {
> +			__mptcp_clean_una_wakeup(sk);
> +			retrans_seq = msk->snd_una;
> +			dfrag = mptcp_rtx_head(sk);
> +			need_retrans = !!dfrag;
> +		} else if (after64(msk->snd_una, retrans_seq)) {
> +			retrans_seq = msk->snd_una;
> +		}
> +		mptcp_data_unlock(sk);
> +
> +		/* `already_sent` can be 0 for `dfrag` belonging to the RTX
> +		 *  queue due to __mptcp_retransmit_pending_data().
> +		 */
> +		if (!dfrag || !dfrag->already_sent)
> +			break;
> +
> +		/* Can fail only in case of fallback. */
> +		len = __mptcp_push_retrans(sk, dfrag, retrans_seq);
> +		if (len < 0)
> +			goto clear_scheduled;
> +
> +		retrans_seq += len;
> +		msk->bytes_retrans += len;
> +		dfrag->already_sent = max_t(u16, dfrag->already_sent,
> +					    retrans_seq - dfrag->data_seq);
> +
> +		/* With csum enabled retransmission can send new data. */
> +		sent_seq = dfrag->already_sent + dfrag->data_seq;
> +		if (after64(sent_seq, msk->snd_nxt))
> +			WRITE_ONCE(msk->snd_nxt, sent_seq);
> +
> +		/* Attempt the next fragment only if the current one is
> +		 * completely retransmitted.
> +		 */
> +		if (before64(retrans_seq, dfrag->data_seq + dfrag->data_len))
> +			break;
> +
> +		dfrag = list_is_last(&dfrag->list, &msk->rtx_queue) ?
> +				NULL : list_next_entry(dfrag, list);
> +		if (!dfrag)
> +			break;
> +	}
> +
> +	/* Attempt data-fin retransmission only when the RTX queue is empty. */
> +	if (!need_retrans) {

Clashiko noted that an error path could reach here with an outdated
`need_retrans` value. It needs to be fixed.

/P