Re: [PATCH mptcp-next] mptcp: pm: userspace: avoid scheduling in-kernel PM worker

[Matthieu Baerts <matttbe@kernel.org>]

Hi Geliang,

Thank you for your review!

On 25/02/2025 05:21, Geliang Tang wrote:
> Hi Matt,
> 
> On Mon, 2025-02-24 at 20:01 +0100, Matthieu Baerts (NGI0) wrote:
>> When the userspace PM is used, there is no need to schedule the PM
>> worker for in-kernel specific tasks, e.g. creating new subflows, or
>> sending more ADD_ADDR.
>>
>> Now, these tasks will be done only if the in-kernel PM is being used.
>>
>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
>> ---
>> Notes:
>>  - I don't know if this should be seen as a fix. Maybe yes because it
>>    was not supposed to do that from the beginning, and it feels less
>>    risky not to schedule the worker when it is not needed. I'm open
>> to
>>    suggestions here.
>>  - I'm mainly sharing this patch now because Geliang is going to
>> modify
>>    these helpers to separate PM specific code.
>>    @Geliang: please rebase your series on top of this one (or include
>>    this patch if it is easier).
>> Cc: Geliang Tang <tanggeliang@kylinos.cn>
>> ---
>>  net/mptcp/pm.c | 10 ++++++++--
>>  1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
>> index
>> 16cacce6c10fe86467aa7ef8e588f9f535b586fb..4eabb83328905676759768fb44c
>> 859f5682721e3 100644
>> --- a/net/mptcp/pm.c
>> +++ b/net/mptcp/pm.c
>> @@ -138,7 +138,7 @@ void mptcp_pm_fully_established(struct mptcp_sock
>> *msk, const struct sock *ssk)
>>  	 * racing paths - accept() and check_fully_established()
>>  	 * be sure to serve this event only once.
>>  	 */
>> -	if (READ_ONCE(pm->work_pending) &&
>> +	if (mptcp_pm_is_kernel(msk) && READ_ONCE(pm->work_pending)
>> &&
> 
> I think there's no need to modify this. work_pending is set to 0 by 
> userspace pm, for all cases where work_pending is 1, the path manager
> must be an in-kernel pm.

Good point, I missed that!

'pm->work_pending' doesn't look like a good name for what it does.
Hopefully, this will be clearer when pm->ops will be used, which will
clearly separate what is done by the different PMs.

Also, is it possible we never set 'pm->work_pending' back to 'true' once
it has been set to 'false'? Does it mean when we reach the limits, and
something happens (e.g. a removed subflow, limits are changed, etc.),
the worker is no longer used to establish new subflows or react when an
ADD_ADDR is received?

> So there is no need to add mptcp_pm_is_kernel() check here. Just
> checking work_pending is sufficient.

Indeed.

>>  	    !(msk->pm.status & BIT(MPTCP_PM_ALREADY_ESTABLISHED)))
>>  		mptcp_pm_schedule_work(msk, MPTCP_PM_ESTABLISHED);
>>  
>> @@ -166,7 +166,7 @@ void mptcp_pm_subflow_established(struct
>> mptcp_sock *msk)
>>  
>>  	pr_debug("msk=%p\n", msk);
>>  
>> -	if (!READ_ONCE(pm->work_pending))
>> +	if (!mptcp_pm_is_kernel(msk) || !READ_ONCE(pm-
>>> work_pending))
>>  		return;
> 
> No need to modify this too, work_pending has been checked later in
> mptcp_pm_subflow_established:
> 
>     if (READ_ONCE(pm->work_pending))
>         mptcp_pm_schedule_work(msk, MPTCP_PM_SUBFLOW_ESTABLISHED);

Indeed.

>>  
>>  	spin_lock_bh(&pm->lock);
>> @@ -251,6 +251,9 @@ void mptcp_pm_add_addr_echoed(struct mptcp_sock
>> *msk,
>>  
>>  	pr_debug("msk=%p\n", msk);
>>  
>> +	if (!mptcp_pm_is_kernel(msk) || !READ_ONCE(pm-
>>> work_pending))
>> +		return;
>> +
> 
> Same here, work_pending has been checked later in
> mptcp_pm_add_addr_echoed:
> 
> if (mptcp_lookup_anno_list_by_saddr(msk, addr) && READ_ONCE(pm-
>> work_pending))
>     mptcp_pm_schedule_work(msk, MPTCP_PM_SUBFLOW_ESTABLISHED);

Indeed.

>>  	spin_lock_bh(&pm->lock);
>>  
>>  	if (mptcp_lookup_anno_list_by_saddr(msk, addr) &&
>> READ_ONCE(pm->work_pending))
>> @@ -278,6 +281,9 @@ void mptcp_pm_rm_addr_received(struct mptcp_sock
>> *msk,
>>  	for (i = 0; i < rm_list->nr; i++)
>>  		mptcp_event_addr_removed(msk, rm_list->ids[i]);
>>  
>> +	if (!mptcp_pm_is_kernel(msk))
>> +		return;
> 
> This mptcp_pm_is_kernel() is indeed needed.

Yes it is. And 'pm->work_pending' cannot be used, because it will be set
to false when the limits are reached, but a RM_ADDR will potentially
decrement counters.

I will send a v2 with only this change.

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.