Re: [PATCH mptcp-next v10 04/12] mptcp: add struct_group in mptcp_pm_data

[Matthieu Baerts <matttbe@kernel.org>]

Hi Geliang,

On 06/03/2025 12:01, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> This patch adds a "struct_group(reset, ...)" in struct mptcp_pm_data to
> simplify the reset, and make sure we don't miss any.

Do you mind checking if, before this patch, we didn't already miss the
reset of some fields? (I think I quickly checked last time and
everything was reset here, or a bit later (e.g. server_side). If we
missed something, we will need a dedicated patch reseting the missing
fields first, with a Fixes tag, for -net, then this patch using
'struct_group' to ease the reset.

> Suggested-by: Matthieu Baerts <matttbe@kernel.org>
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  net/mptcp/pm.c       | 14 +-------------
>  net/mptcp/protocol.h |  4 ++++
>  2 files changed, 5 insertions(+), 13 deletions(-)
> 
> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index eefed554dcc9..1400bfed4b0d 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c
> @@ -983,12 +983,7 @@ void mptcp_pm_data_reset(struct mptcp_sock *msk)
>  	u8 pm_type = mptcp_get_pm_type(sock_net((struct sock *)msk));
>  	struct mptcp_pm_data *pm = &msk->pm;
>  
> -	pm->add_addr_signaled = 0;
> -	pm->add_addr_accepted = 0;
> -	pm->local_addr_used = 0;
> -	pm->subflows = 0;
> -	pm->rm_list_tx.nr = 0;
> -	pm->rm_list_rx.nr = 0;

Just to be sure, is it OK to reset the list with memset(0)? (I didn't check)

> +	memset(&pm->reset, 0, sizeof(pm->reset));
>  	WRITE_ONCE(pm->pm_type, pm_type);
>  
>  	if (pm_type == MPTCP_PM_TYPE_KERNEL) {
> @@ -1005,15 +1000,8 @@ void mptcp_pm_data_reset(struct mptcp_sock *msk)
>  			   !!mptcp_pm_get_add_addr_accept_max(msk) &&
>  			   subflows_allowed);
>  		WRITE_ONCE(pm->accept_subflow, subflows_allowed);
> -	} else {
> -		WRITE_ONCE(pm->work_pending, 0);
> -		WRITE_ONCE(pm->accept_addr, 0);
> -		WRITE_ONCE(pm->accept_subflow, 0);
>  	}
>  
> -	WRITE_ONCE(pm->addr_signal, 0);
> -	WRITE_ONCE(pm->remote_deny_join_id0, false);
> -	pm->status = 0;
>  	bitmap_fill(pm->id_avail_bitmap, MPTCP_PM_MAX_ADDR_ID + 1);
>  }
>  
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index 0ef758d233b7..47710db243f4 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -223,6 +223,8 @@ struct mptcp_pm_data {
>  
>  	spinlock_t	lock;		/*protects the whole PM data */
>  
> +	struct_group(reset,
> +
>  	u8		addr_signal;
>  	bool		server_side;
>  	bool		work_pending;
> @@ -238,6 +240,8 @@ struct mptcp_pm_data {
>  	DECLARE_BITMAP(id_avail_bitmap, MPTCP_PM_MAX_ADDR_ID + 1);

Maybe better to move this bitmap after the reset group, because it is
large, and it will be overwritten with 1 just after the reset.

>  	struct mptcp_rm_list rm_list_tx;
>  	struct mptcp_rm_list rm_list_rx;
> +
> +	);
>  };
>  
>  struct mptcp_pm_local {
Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.