Re: [PATCH mptcp-next v8 07/12] mptcp: pm: userspace: register mptcp_pm_userspace

[Matthieu Baerts <matttbe@kernel.org>]

Hi Geliang,

On 04/03/2025 12:40, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> This patch defines the original userspace path manager as a new
> struct mptcp_pm_ops named "mptcp_userspace_pm", and register it
> in mptcp_pm_init(). mptcp_userspace_pm_is_release() is a wrapper
> of mptcp_userspace_pm_free_local_addr_list().

(...)

> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> index 13856df22673..412d6c912148 100644
> --- a/net/mptcp/pm_userspace.c
> +++ b/net/mptcp/pm_userspace.c
> @@ -682,3 +682,29 @@ int mptcp_userspace_pm_get_addr(u8 id, struct mptcp_pm_addr_entry *addr,
>  	sock_put(sk);
>  	return ret;
>  }
> +
> +static void mptcp_userspace_pm_init(struct mptcp_sock *msk)
> +{
> +	struct mptcp_pm_data *pm = &msk->pm;
> +
> +	WRITE_ONCE(pm->work_pending, 0);
> +	WRITE_ONCE(pm->accept_addr, 0);
> +	WRITE_ONCE(pm->accept_subflow, 0);

I would not do that here: these variables are not used by the userspace
PM, that doesn't make sense for this PM to do that. I think these
variables should be reset all the time, then the init callback is called
if set.

In other words, only the in-kernel PM needs this init callback.

> +}
> +
> +static void mptcp_userspace_pm_release(struct mptcp_sock *msk)
> +{
> +	mptcp_userspace_pm_free_local_addr_list(msk);
> +}

Same as in patch 6, I would also move this release part in patch 8
("mptcp: pm: initialize and release mptcp_pm_ops"), to show that you are
moving code. Otherwise, it is difficult to know from where it comes from.

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