This helper is confusing. It is in pm.c, but it is specific to the
in-kernel PM and it cannot be used by the userspace one. Also, it simply
calls one in-kernel specific function with the PM lock, while the
similar mptcp_pm_remove_addr() helper requires the PM lock.

What's left is the pr_debug(), which is not that useful, because a
similar one is present in the only function called by this helper:

  mptcp_pm_nl_rm_subflow_received()

After these modifications, this helper can now be marked as 'static'.

Note that it is not really a bug, but it will help backporting the
following commits.

Fixes: 0ee4261a3681 ("mptcp: implement mptcp_pm_remove_subflow")
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 net/mptcp/pm.c         | 10 ----------
 net/mptcp/pm_netlink.c | 16 +++++++++++-----
 net/mptcp/protocol.h   |  3 ---
 3 files changed, 11 insertions(+), 18 deletions(-)

diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index 55406720c607..1f1b2617d0f5 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -60,16 +60,6 @@ int mptcp_pm_remove_addr(struct mptcp_sock *msk, const struct mptcp_rm_list *rm_
 	return 0;
 }
 
-int mptcp_pm_remove_subflow(struct mptcp_sock *msk, const struct mptcp_rm_list *rm_list)
-{
-	pr_debug("msk=%p, rm_list_nr=%d", msk, rm_list->nr);
-
-	spin_lock_bh(&msk->pm.lock);
-	mptcp_pm_nl_rm_subflow_received(msk, rm_list);
-	spin_unlock_bh(&msk->pm.lock);
-	return 0;
-}
-
 /* path manager event handlers */
 
 void mptcp_pm_new_connection(struct mptcp_sock *msk, const struct sock *ssk, int server_side)
diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index 44092246259c..96336a87973f 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -858,8 +858,8 @@ static void mptcp_pm_nl_rm_addr_received(struct mptcp_sock *msk)
 	mptcp_pm_nl_rm_addr_or_subflow(msk, &msk->pm.rm_list_rx, MPTCP_MIB_RMADDR);
 }
 
-void mptcp_pm_nl_rm_subflow_received(struct mptcp_sock *msk,
-				     const struct mptcp_rm_list *rm_list)
+static void mptcp_pm_nl_rm_subflow_received(struct mptcp_sock *msk,
+					    const struct mptcp_rm_list *rm_list)
 {
 	mptcp_pm_nl_rm_addr_or_subflow(msk, rm_list, MPTCP_MIB_RMSUBFLOW);
 }
@@ -1454,8 +1454,11 @@ static int mptcp_nl_remove_subflow_and_signal_addr(struct net *net,
 		remove_subflow = lookup_subflow_by_saddr(&msk->conn_list, addr);
 		mptcp_pm_remove_anno_addr(msk, addr, remove_subflow &&
 					  !(entry->flags & MPTCP_PM_ADDR_FLAG_IMPLICIT));
+
 		if (remove_subflow) {
-			mptcp_pm_remove_subflow(msk, &list);
+			spin_lock_bh(&msk->pm.lock);
+			mptcp_pm_nl_rm_subflow_received(msk, &list);
+			spin_unlock_bh(&msk->pm.lock);
 		} else if (entry->flags & MPTCP_PM_ADDR_FLAG_SUBFLOW) {
 			/* If the subflow has been used, but now closed */
 			spin_lock_bh(&msk->pm.lock);
@@ -1608,8 +1611,11 @@ static void mptcp_pm_flush_addrs_and_subflows(struct mptcp_sock *msk,
 		spin_unlock_bh(&msk->pm.lock);
 	}
 
-	if (slist.nr)
-		mptcp_pm_remove_subflow(msk, &slist);
+	if (slist.nr) {
+		spin_lock_bh(&msk->pm.lock);
+		mptcp_pm_nl_rm_subflow_received(msk, &slist);
+		spin_unlock_bh(&msk->pm.lock);
+	}
 
 	/* Reset counters: maybe some subflows have been removed before */
 	spin_lock_bh(&msk->pm.lock);
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 19d60b6d5b45..f2eb5273d752 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -1030,7 +1030,6 @@ int mptcp_pm_announce_addr(struct mptcp_sock *msk,
 			   const struct mptcp_addr_info *addr,
 			   bool echo);
 int mptcp_pm_remove_addr(struct mptcp_sock *msk, const struct mptcp_rm_list *rm_list);
-int mptcp_pm_remove_subflow(struct mptcp_sock *msk, const struct mptcp_rm_list *rm_list);
 void mptcp_pm_remove_addrs(struct mptcp_sock *msk, struct list_head *rm_list);
 
 void mptcp_free_local_addr_list(struct mptcp_sock *msk);
@@ -1134,8 +1133,6 @@ static inline u8 subflow_get_local_id(const struct mptcp_subflow_context *subflo
 
 void __init mptcp_pm_nl_init(void);
 void mptcp_pm_nl_work(struct mptcp_sock *msk);
-void mptcp_pm_nl_rm_subflow_received(struct mptcp_sock *msk,
-				     const struct mptcp_rm_list *rm_list);
 unsigned int mptcp_pm_get_add_addr_signal_max(const struct mptcp_sock *msk);
 unsigned int mptcp_pm_get_add_addr_accept_max(const struct mptcp_sock *msk);
 unsigned int mptcp_pm_get_subflows_max(const struct mptcp_sock *msk);

-- 
2.45.2

On Mon, 2024-07-15 at 12:09 +0200, Matthieu Baerts (NGI0) wrote:
> This helper is confusing. It is in pm.c, but it is specific to the
> in-kernel PM and it cannot be used by the userspace one. Also, it
> simply
> calls one in-kernel specific function with the PM lock, while the
> similar mptcp_pm_remove_addr() helper requires the PM lock.
> 
> What's left is the pr_debug(), which is not that useful, because a
> similar one is present in the only function called by this helper:
> 
>   mptcp_pm_nl_rm_subflow_received()
> 
> After these modifications, this helper can now be marked as 'static'.
> 
> Note that it is not really a bug, but it will help backporting the
> following commits.
> 
> Fixes: 0ee4261a3681 ("mptcp: implement mptcp_pm_remove_subflow")
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> ---
>  net/mptcp/pm.c         | 10 ----------
>  net/mptcp/pm_netlink.c | 16 +++++++++++-----
>  net/mptcp/protocol.h   |  3 ---
>  3 files changed, 11 insertions(+), 18 deletions(-)
> 
> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index 55406720c607..1f1b2617d0f5 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c
> @@ -60,16 +60,6 @@ int mptcp_pm_remove_addr(struct mptcp_sock *msk,
> const struct mptcp_rm_list *rm_
>  	return 0;
>  }
>  
> -int mptcp_pm_remove_subflow(struct mptcp_sock *msk, const struct
> mptcp_rm_list *rm_list)
> -{
> -	pr_debug("msk=%p, rm_list_nr=%d", msk, rm_list->nr);
> -
> -	spin_lock_bh(&msk->pm.lock);
> -	mptcp_pm_nl_rm_subflow_received(msk, rm_list);
> -	spin_unlock_bh(&msk->pm.lock);
> -	return 0;
> -}
> -
>  /* path manager event handlers */
>  
>  void mptcp_pm_new_connection(struct mptcp_sock *msk, const struct
> sock *ssk, int server_side)
> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index 44092246259c..96336a87973f 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c
> @@ -858,8 +858,8 @@ static void mptcp_pm_nl_rm_addr_received(struct
> mptcp_sock *msk)
>  	mptcp_pm_nl_rm_addr_or_subflow(msk, &msk->pm.rm_list_rx,
> MPTCP_MIB_RMADDR);
>  }
>  
> -void mptcp_pm_nl_rm_subflow_received(struct mptcp_sock *msk,
> -				     const struct mptcp_rm_list
> *rm_list)
> +static void mptcp_pm_nl_rm_subflow_received(struct mptcp_sock *msk,
> +					    const struct
> mptcp_rm_list *rm_list)
>  {
>  	mptcp_pm_nl_rm_addr_or_subflow(msk, rm_list,
> MPTCP_MIB_RMSUBFLOW);
>  }
> @@ -1454,8 +1454,11 @@ static int
> mptcp_nl_remove_subflow_and_signal_addr(struct net *net,
>  		remove_subflow = lookup_subflow_by_saddr(&msk-
> >conn_list, addr);
>  		mptcp_pm_remove_anno_addr(msk, addr, remove_subflow
> &&
>  					  !(entry->flags &
> MPTCP_PM_ADDR_FLAG_IMPLICIT));
> +

I think no need to add a new line here.

>  		if (remove_subflow) {
> -			mptcp_pm_remove_subflow(msk, &list);
> +			spin_lock_bh(&msk->pm.lock);
> +			mptcp_pm_nl_rm_subflow_received(msk, &list);
> +			spin_unlock_bh(&msk->pm.lock);

The subsequent code holds msk->pm.lock too, both of them can be placed
in the same block holding msk->pm.lock once.

>  		} else if (entry->flags &
> MPTCP_PM_ADDR_FLAG_SUBFLOW) {
>  			/* If the subflow has been used, but now
> closed */
>  			spin_lock_bh(&msk->pm.lock);
> @@ -1608,8 +1611,11 @@ static void
> mptcp_pm_flush_addrs_and_subflows(struct mptcp_sock *msk,
>  		spin_unlock_bh(&msk->pm.lock);
>  	}
>  
> -	if (slist.nr)
> -		mptcp_pm_remove_subflow(msk, &slist);
> +	if (slist.nr) {
> +		spin_lock_bh(&msk->pm.lock);
> +		mptcp_pm_nl_rm_subflow_received(msk, &slist);
> +		spin_unlock_bh(&msk->pm.lock);
> +	}

The same here. Holding msk->pm.lock once.

WDYT?

>  
>  	/* Reset counters: maybe some subflows have been removed
> before */
>  	spin_lock_bh(&msk->pm.lock);
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index 19d60b6d5b45..f2eb5273d752 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -1030,7 +1030,6 @@ int mptcp_pm_announce_addr(struct mptcp_sock
> *msk,
>  			   const struct mptcp_addr_info *addr,
>  			   bool echo);
>  int mptcp_pm_remove_addr(struct mptcp_sock *msk, const struct
> mptcp_rm_list *rm_list);
> -int mptcp_pm_remove_subflow(struct mptcp_sock *msk, const struct
> mptcp_rm_list *rm_list);
>  void mptcp_pm_remove_addrs(struct mptcp_sock *msk, struct list_head
> *rm_list);
>  
>  void mptcp_free_local_addr_list(struct mptcp_sock *msk);
> @@ -1134,8 +1133,6 @@ static inline u8 subflow_get_local_id(const
> struct mptcp_subflow_context *subflo
>  
>  void __init mptcp_pm_nl_init(void);
>  void mptcp_pm_nl_work(struct mptcp_sock *msk);
> -void mptcp_pm_nl_rm_subflow_received(struct mptcp_sock *msk,
> -				     const struct mptcp_rm_list
> *rm_list);
>  unsigned int mptcp_pm_get_add_addr_signal_max(const struct
> mptcp_sock *msk);
>  unsigned int mptcp_pm_get_add_addr_accept_max(const struct
> mptcp_sock *msk);
>  unsigned int mptcp_pm_get_subflows_max(const struct mptcp_sock
> *msk);
> 

Hi Geliang,

Thank you for the review!

On 16/07/2024 05:40, Geliang Tang wrote:
> On Mon, 2024-07-15 at 12:09 +0200, Matthieu Baerts (NGI0) wrote:
>> This helper is confusing. It is in pm.c, but it is specific to the
>> in-kernel PM and it cannot be used by the userspace one. Also, it
>> simply
>> calls one in-kernel specific function with the PM lock, while the
>> similar mptcp_pm_remove_addr() helper requires the PM lock.
>>
>> What's left is the pr_debug(), which is not that useful, because a
>> similar one is present in the only function called by this helper:
>>
>>   mptcp_pm_nl_rm_subflow_received()
>>
>> After these modifications, this helper can now be marked as 'static'.
>>
>> Note that it is not really a bug, but it will help backporting the
>> following commits.

(...)

>> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
>> index 44092246259c..96336a87973f 100644
>> --- a/net/mptcp/pm_netlink.c
>> +++ b/net/mptcp/pm_netlink.c
>> @@ -1454,8 +1454,11 @@ static int
>> mptcp_nl_remove_subflow_and_signal_addr(struct net *net,
>>  		remove_subflow = lookup_subflow_by_saddr(&msk-
>>> conn_list, addr);
>>  		mptcp_pm_remove_anno_addr(msk, addr, remove_subflow
>> &&
>>  					  !(entry->flags &
>> MPTCP_PM_ADDR_FLAG_IMPLICIT));
>> +
> 
> I think no need to add a new line here.

The block below started to be a bit big, and was doing a different
action that the one above (removing ADD_ADDR). Maybe I should have added
it in patch 10/17?

>>  		if (remove_subflow) {
>> -			mptcp_pm_remove_subflow(msk, &list);
>> +			spin_lock_bh(&msk->pm.lock);
>> +			mptcp_pm_nl_rm_subflow_received(msk, &list);
>> +			spin_unlock_bh(&msk->pm.lock);
> 
> The subsequent code holds msk->pm.lock too, both of them can be placed
> in the same block holding msk->pm.lock once.

Yes indeed, but there are cases where it is not needed to lock the PM:
if we are removing a 'signal' endpoint. In this case, I thought it would
be better to lock only if needed, and surround the helper requiring the
lock. No?

>>  		} else if (entry->flags &
>> MPTCP_PM_ADDR_FLAG_SUBFLOW) {
>>  			/* If the subflow has been used, but now
>> closed */
>>  			spin_lock_bh(&msk->pm.lock);
>> @@ -1608,8 +1611,11 @@ static void
>> mptcp_pm_flush_addrs_and_subflows(struct mptcp_sock *msk,
>>  		spin_unlock_bh(&msk->pm.lock);
>>  	}
>>  
>> -	if (slist.nr)
>> -		mptcp_pm_remove_subflow(msk, &slist);
>> +	if (slist.nr) {
>> +		spin_lock_bh(&msk->pm.lock);
>> +		mptcp_pm_nl_rm_subflow_received(msk, &slist);
>> +		spin_unlock_bh(&msk->pm.lock);
>> +	}
> 
> The same here. Holding msk->pm.lock once.

Yes, indeed, here we can probably lock once before "if (alist.nr)". If
there is only this change needed, I can probably do the modification
when applying the patches. (I already did the modifications locally).

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

On Tue, 2024-07-16 at 10:35 +0200, Matthieu Baerts wrote:
> Hi Geliang,
> 
> Thank you for the review!
> 
> On 16/07/2024 05:40, Geliang Tang wrote:
> > On Mon, 2024-07-15 at 12:09 +0200, Matthieu Baerts (NGI0) wrote:
> > > This helper is confusing. It is in pm.c, but it is specific to
> > > the
> > > in-kernel PM and it cannot be used by the userspace one. Also, it
> > > simply
> > > calls one in-kernel specific function with the PM lock, while the
> > > similar mptcp_pm_remove_addr() helper requires the PM lock.
> > > 
> > > What's left is the pr_debug(), which is not that useful, because
> > > a
> > > similar one is present in the only function called by this
> > > helper:
> > > 
> > >   mptcp_pm_nl_rm_subflow_received()
> > > 
> > > After these modifications, this helper can now be marked as
> > > 'static'.
> > > 
> > > Note that it is not really a bug, but it will help backporting
> > > the
> > > following commits.
> 
> (...)
> 
> > > diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> > > index 44092246259c..96336a87973f 100644
> > > --- a/net/mptcp/pm_netlink.c
> > > +++ b/net/mptcp/pm_netlink.c
> > > @@ -1454,8 +1454,11 @@ static int
> > > mptcp_nl_remove_subflow_and_signal_addr(struct net *net,
> > >  		remove_subflow = lookup_subflow_by_saddr(&msk-
> > > > conn_list, addr);
> > >  		mptcp_pm_remove_anno_addr(msk, addr,
> > > remove_subflow
> > > &&
> > >  					  !(entry->flags &
> > > MPTCP_PM_ADDR_FLAG_IMPLICIT));
> > > +
> > 
> > I think no need to add a new line here.
> 
> The block below started to be a bit big, and was doing a different
> action that the one above (removing ADD_ADDR). Maybe I should have
> added
> it in patch 10/17?

Yes, it's better in 10/17.

> 
> > >  		if (remove_subflow) {
> > > -			mptcp_pm_remove_subflow(msk, &list);
> > > +			spin_lock_bh(&msk->pm.lock);
> > > +			mptcp_pm_nl_rm_subflow_received(msk,
> > > &list);
> > > +			spin_unlock_bh(&msk->pm.lock);
> > 
> > The subsequent code holds msk->pm.lock too, both of them can be
> > placed
> > in the same block holding msk->pm.lock once.
> 
> Yes indeed, but there are cases where it is not needed to lock the
> PM:
> if we are removing a 'signal' endpoint. In this case, I thought it
> would
> be better to lock only if needed, and surround the helper requiring
> the
> lock. No?

Yes, I agree.

> 
> > >  		} else if (entry->flags &
> > > MPTCP_PM_ADDR_FLAG_SUBFLOW) {
> > >  			/* If the subflow has been used, but now
> > > closed */
> > >  			spin_lock_bh(&msk->pm.lock);
> > > @@ -1608,8 +1611,11 @@ static void
> > > mptcp_pm_flush_addrs_and_subflows(struct mptcp_sock *msk,
> > >  		spin_unlock_bh(&msk->pm.lock);
> > >  	}
> > >  
> > > -	if (slist.nr)
> > > -		mptcp_pm_remove_subflow(msk, &slist);
> > > +	if (slist.nr) {
> > > +		spin_lock_bh(&msk->pm.lock);
> > > +		mptcp_pm_nl_rm_subflow_received(msk, &slist);
> > > +		spin_unlock_bh(&msk->pm.lock);
> > > +	}
> > 
> > The same here. Holding msk->pm.lock once.
> 
> Yes, indeed, here we can probably lock once before "if (alist.nr)".
> If
> there is only this change needed, I can probably do the modification
> when applying the patches. (I already did the modifications locally).
> 
> Cheers,
> Matt