diff mbox series

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

Message ID 9133981edb83438ee10e6da9ef4e5cc6bf7f188b.1741258415.git.tanggeliang@kylinos.cn (mailing list archive)
State Superseded
Headers show
Series BPF path manager, part 5 | expand

Checks

Context Check Description
matttbe/checkpatch warning total: 0 errors, 0 warnings, 1 checks, 44 lines checked
matttbe/shellcheck success MPTCP selftests files have not been modified
matttbe/build success Build and static analysis OK
matttbe/KVM_Validation__normal success Success! ✅
matttbe/KVM_Validation__debug success Success! ✅
matttbe/KVM_Validation__btf-normal__only_bpftest_all_ success Success! ✅
matttbe/KVM_Validation__btf-debug__only_bpftest_all_ success Success! ✅

Commit Message

Geliang Tang March 6, 2025, 11:01 a.m. UTC 
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.

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

Comments

Matthieu Baerts (NGI0) March 10, 2025, 11:17 p.m. UTC | #1 
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
Geliang Tang March 11, 2025, 4:24 a.m. UTC | #2 
Hi Matt,

Thanks for the review.

On Tue, 2025-03-11 at 00:17 +0100, Matthieu Baerts wrote:
> 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

Yes, I did check this, and nothing is missing.

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

I kept this unchanged in v11.

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

I agree. I moved this, together with rm_list_tx and rm_list_rx, after
the reset group in v11.

Thanks,
-Geliang

> 
> >  	struct mptcp_rm_list rm_list_tx;
> >  	struct mptcp_rm_list rm_list_rx;
> > +
> > +	);
> >  };
> >  
> >  struct mptcp_pm_local {
> Cheers,
> Matt
Matthieu Baerts (NGI0) March 11, 2025, 8:53 a.m. UTC | #3 
Hi Geliang,

On 11/03/2025 05:24, Geliang Tang wrote:
> Hi Matt,
> 
> Thanks for the review.
> 
> On Tue, 2025-03-11 at 00:17 +0100, Matthieu Baerts wrote:
>> 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
> 
> Yes, I did check this, and nothing is missing.

Great, thank you for having checked!

Cheers,
Matt
diff mbox series

Patch

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;
+	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);
 	struct mptcp_rm_list rm_list_tx;
 	struct mptcp_rm_list rm_list_rx;
+
+	);
 };
 
 struct mptcp_pm_local {