Re: [PATCH v2 mptcp-next] Squash-to: "mptcp: leverage the backlog for RX packet processing"

[Matthieu Baerts <matttbe@kernel.org>]

Hi Paolo,

On 11/11/2025 17:21, Matthieu Baerts wrote:
> Hi Paolo,
> 
> On 09/11/2025 14:53, Paolo Abeni wrote:
>> If a subflow receives data before gaining the memcg while the msk
>> socket lock is held at accept time, or the PM locks the msk socket
>> while still unaccepted and subflows push data to it at the same time,
>> the mptcp_graph_subflows() can complete with a non empty backlog.
>>
>> The msk will try to borrow such memory, but (some) of the skbs there
>> where not memcg charged. When the msk finally will return such accounted
>> memory, we should hit the same splat of #597.
>> [even if so far I was unable to replicate this scenario]
>>
>> This patch tries to address such potential issue by:
>> - preventing the subflow from queuing data into the backlog after
>>   gaining the memcg. This ensure that at the end of the look all the
>>   skbs in the backlog (if any) are _not_ memory accounted.
>> - mem charge the backlog to msk
>> - 'restart' the subflow and spool any data waiting there.
>>
>> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
>> ---
>>  net/mptcp/protocol.c | 46 ++++++++++++++++++++++++++++++++++++++++++--
>>  1 file changed, 44 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
>> index 5e9325c7ea9c..d6b08e1de358 100644
>> --- a/net/mptcp/protocol.c
>> +++ b/net/mptcp/protocol.c
>> @@ -4082,10 +4082,12 @@ static void mptcp_graph_subflows(struct sock *sk)
>>  {
>>  	struct mptcp_subflow_context *subflow;
>>  	struct mptcp_sock *msk = mptcp_sk(sk);
>> +	struct sock *ssk;
>> +	int old_amt, amt;
>> +	bool slow;
>>  
>>  	mptcp_for_each_subflow(msk, subflow) {
>> -		struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
>> -		bool slow;
>> +		ssk = mptcp_subflow_tcp_sock(subflow);
>>  
>>  		slow = lock_sock_fast(ssk);
>>  
>> @@ -4095,8 +4097,48 @@ static void mptcp_graph_subflows(struct sock *sk)
>>  		if (!ssk->sk_socket)
>>  			mptcp_sock_graft(ssk, sk->sk_socket);
>>  
>> +		if (!mem_cgroup_from_sk(sk))
> 
> Should we not call mem_cgroup_sk_enabled() instead? It does this:
> 
>   return mem_cgroup_sockets_enabled && mem_cgroup_from_sk(sk);
> 
> That's what is done in net/core/sock.c and net/ipv4/tcp_output.c. Not in
> __inet_accept(), because mem_cgroup_sockets_enabled() is checked before.
> Maybe we should do the same here?
> 
> (Note that it is not clear to me if mem_cgroup can be enabled later on,
> and if yes, what should be done with existing connections.)

Also, do you not still need to call __mptcp_inherit_cgrp_data() even if
!mem_cgroup_sockets_enabled() (or !mem_cgroup_from_sk())?

I guess the two are often linked, but can they not be used independently?

Cheers,
Matt

> 
>> +			goto unlock;
>> +
>>  		__mptcp_inherit_cgrp_data(sk, ssk);
>>  		__mptcp_inherit_memcg(sk, ssk, GFP_KERNEL);
>> +
>> +		/* Prevent subflows from queueing data into the backlog
>> +		 * as soon as cg is set; note that we can't race
>> +		 * with __mptcp_close_ssk setting this bit for a really
>> +		 * closing socket, because we hold the msk socket lock here.
>> +		 */
>> +		subflow->closing = 1;
>> +
>> +unlock:
>> +		unlock_sock_fast(ssk, slow);
>> +	}
>> +
>> +	if (!mem_cgroup_from_sk(sk))
> 
> Same here?
> 
>> +		return;
>> +
>> +	/* Charge the bl memory, note that __sk_charge accounted for
>> +	 * fwd memory and rmem only
>> +	 */
>> +	mptcp_data_lock(sk);
>> +	old_amt = sk_mem_pages(sk->sk_forward_alloc +
>> +			       atomic_read(&sk->sk_rmem_alloc));
>> +	amt = sk_mem_pages(msk->backlog_len + sk->sk_forward_alloc +
>> +		     atomic_read(&sk->sk_rmem_alloc));
> 
> (Same as Geliang for the alignment here, and eventually calling
> kmem_cache_charge() like in __inet_accept())
> 
>> +	amt -= old_amt;
>> +	if (amt)
>> +		mem_cgroup_sk_charge(sk, amt, GFP_ATOMIC | __GFP_NOFAIL);
> 
> Just to be sure: no need to check if there was an error? It is not done
> in __inet_accept() either, so I guess no?
> 
>> +	mptcp_data_unlock(sk);
>> +
>> +	/* Finally let the subflow restart queuing data. */
>> +	mptcp_for_each_subflow(msk, subflow) {
>> +		ssk = mptcp_subflow_tcp_sock(subflow);
>> +
>> +		slow = lock_sock_fast(ssk);
>> +		subflow->closing = 0;
>> +
>> +		if (mptcp_subflow_data_available(ssk))
>> +			mptcp_data_ready(sk, ssk);
>>  		unlock_sock_fast(ssk, slow);
>>  	}
>>  }
-- 
Sponsored by the NGI0 Core fund.