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

[Geliang Tang <geliang@kernel.org>]

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.

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

>  	    !(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);

>  
>  	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);

>  	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.

WDYT?

Thanks,
-Geliang

> +
>  	spin_lock_bh(&pm->lock);
>  	if (mptcp_pm_schedule_work(msk, MPTCP_PM_RM_ADDR_RECEIVED))
>  		pm->rm_list_rx = *rm_list;
> 
> ---
> base-commit: 3716d837622ee1baf6fbe7e6a6a7a3df116e75fe
> change-id: 20250224-mptcp-userspace-avoid-worker-93f367e39ca6
> 
> Best regards,