Re: [PATCH mptcp-next v5 5/7] mptcp: defer read_sock cleanup to mptcp_worker

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Inverted order of `mptcp_cleanup_rbuf()` and `mptcp_rcv_space_adjust()` causes delayed receive window expansion.
--

--- Patch [5]: [PATCH mptcp-next v5 5/7] mptcp: defer read_sock cleanup to mptcp_worker ---
commit 7c6cfb1e14c2505035ad95612907c5f1c34d25e5
Author: Paolo Abeni <[email protected]>

mptcp: defer read_sock cleanup to mptcp_worker

This commit defers sk->sk_data_ready(sk) cleanup tasks to the MPTCP worker
using a new MPTCP_WORK_READ_COMPLETE flag. This avoids locking recursions and
context-related issues when MPTCP carries TLS.

> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 6006193388f12..648eba89278f6 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -3133,6 +3133,15 @@ static void mptcp_backlog_purge(struct sock *sk)
>  	sk_mem_reclaim(sk);
>  }
>  
> +static void mptcp_read_complete(struct sock *sk)
> +{
> +	struct mptcp_sock *msk = mptcp_sk(sk);
> +
> +	mptcp_cleanup_rbuf(msk, msk->read_copied);
> +	mptcp_rcv_space_adjust(msk, msk->read_copied);

[Severity: Medium]
Does the inverted order of these function calls delay receive window expansion?

Before this patch in __mptcp_read_sock(), mptcp_rcv_space_adjust() was called
before mptcp_cleanup_rbuf(). Because mptcp_cleanup_rbuf() evaluates the
current socket receive buffer capacity to decide whether to send a window
update ACK, calling mptcp_rcv_space_adjust() afterward means the buffer
capacity is expanded after the cleanup check has already run.

Could this cause the ACK sent to advertise the old, smaller window size,
delaying the expanded window advertisement until the next cycle?

> +	msk->read_copied = 0;
> +}
> +
>  static void mptcp_do_fastclose(struct sock *sk)

[ ... ]

> @@ -4641,11 +4651,14 @@ static int __mptcp_read_sock(struct sock *sk, read_descriptor_t *desc,
>  	if (noack)
>  		goto out;
>  
> -	mptcp_rcv_space_adjust(msk, copied);
> -
> +	/* The backlog flushing is only needed when some data is actually
> +	 * moved and will take place in the workers's release callback.
> +	 */
>  	if (copied > 0) {
>  		mptcp_recv_skb(sk, &offset);
> -		mptcp_cleanup_rbuf(msk, copied);
> +		msk->read_copied += copied;
> +		set_bit(MPTCP_WORK_READ_COMPLETE, &msk->flags);
> +		mptcp_schedule_work(sk);
>  	}
>  out:
>  	return copied;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5