Re: [PATCH mptcp-next v7 08/11] mptcp: pm: drop pm_type in mptcp_pm_data

[Matthieu Baerts <matttbe@kernel.org>]

Hi Geliang,

On 03/03/2025 05:22, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> Now pm->pm_type can be replaced by pm->ops->name, then "pm_type" filed
> of struct mptcp_pm_data can be dropped.
> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  net/mptcp/pm.c       | 4 +---
>  net/mptcp/protocol.h | 5 ++---
>  2 files changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index e8b34f2ecb35..1ce58d16370a 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c
> @@ -979,7 +979,6 @@ void mptcp_pm_destroy(struct mptcp_sock *msk)
>  void mptcp_pm_data_reset(struct mptcp_sock *msk)
>  {
>  	const char *path_manager = mptcp_get_path_manager(sock_net((struct sock *)msk));
> -	u8 pm_type = mptcp_get_pm_type(sock_net((struct sock *)msk));
>  	struct mptcp_pm_data *pm = &msk->pm;
>  	int ret;
>  
> @@ -989,7 +988,6 @@ void mptcp_pm_data_reset(struct mptcp_sock *msk)
>  	pm->subflows = 0;
>  	pm->rm_list_tx.nr = 0;
>  	pm->rm_list_rx.nr = 0;
> -	WRITE_ONCE(pm->pm_type, pm_type);
>  
>  	rcu_read_lock();
>  	ret = mptcp_pm_initialize(msk, mptcp_pm_find(path_manager));
> @@ -997,7 +995,7 @@ void mptcp_pm_data_reset(struct mptcp_sock *msk)
>  	if (ret)
>  		return;
>  
> -	if (pm_type == MPTCP_PM_TYPE_KERNEL) {
> +	if (mptcp_pm_is_kernel(msk)) {

The code here could be done in the new init() callback maybe? So what
you introduced in the previous patch 7/11.

>  		bool subflows_allowed = !!mptcp_pm_get_subflows_max(msk);
>  
>  		/* pm->work_pending must be only be set to 'true' when
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index 172450455c2a..56eeee1cbccc 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -233,7 +233,6 @@ struct mptcp_pm_data {
>  	u8		add_addr_signaled;
>  	u8		add_addr_accepted;
>  	u8		local_addr_used;
> -	u8		pm_type;
>  	u8		subflows;
>  	u8		status;
>  	DECLARE_BITMAP(id_avail_bitmap, MPTCP_PM_MAX_ADDR_ID + 1);
> @@ -1101,12 +1100,12 @@ static inline bool mptcp_pm_should_rm_signal(struct mptcp_sock *msk)
>  
>  static inline bool mptcp_pm_is_userspace(const struct mptcp_sock *msk)
>  {
> -	return READ_ONCE(msk->pm.pm_type) == MPTCP_PM_TYPE_USERSPACE;
> +	return !strncmp(msk->pm.ops->name, "userspace", MPTCP_PM_NAME_MAX);

Please don't do a string comparison here.

I think it is better to drop msk->pm.pm_type when mptcp_pm_is_userspace
and mptcp_pm_is_kernel are both dropped, no?

If you want to drop pm_type before, then you could compare &msk->pm.ops,
but I think that's something that should be done later. I guess
mptcp_pm_is_userspace() will still be needed, but only from
pm_userspace.c with mptcp_userspace_pm_get_sock(). No?

Same for mptcp_pm_is_kernel(), only from pm_kernel.c when iterating over
all connections.  Except if you use introduce a new macro like
mptcp_pm_for_each_msk() taking in argument "&mptcp_pm_kernel"?

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