From: Geliang Tang <tanggeliang@kylinos.cn>

Since mptcp_pm_remove_addrs is only called from the userspace PM, this
patch moves it into pm_userspace.c.

For this, lookup_subflow_by_saddr() and remove_anno_list_by_saddr()
helpers need to be exported in protocol.h. Also add "mptcp_" prefix for
these helpers.

This patch doesn't change the behaviour of the code, just refactoring.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 net/mptcp/pm_netlink.c   | 45 +++++++---------------------------------
 net/mptcp/pm_userspace.c | 28 +++++++++++++++++++++++++
 net/mptcp/protocol.h     |  4 ++++
 3 files changed, 40 insertions(+), 37 deletions(-)

diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index 618289aac0ab..8aba7670345d 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -107,8 +107,8 @@ static void remote_address(const struct sock_common *skc,
 #endif
 }
 
-static bool lookup_subflow_by_saddr(const struct list_head *list,
-				    const struct mptcp_addr_info *saddr)
+bool mptcp_lookup_subflow_by_saddr(const struct list_head *list,
+				   const struct mptcp_addr_info *saddr)
 {
 	struct mptcp_subflow_context *subflow;
 	struct mptcp_addr_info cur;
@@ -1453,8 +1453,8 @@ int mptcp_pm_nl_add_addr_doit(struct sk_buff *skb, struct genl_info *info)
 	return ret;
 }
 
-static bool remove_anno_list_by_saddr(struct mptcp_sock *msk,
-				      const struct mptcp_addr_info *addr)
+bool mptcp_remove_anno_list_by_saddr(struct mptcp_sock *msk,
+				     const struct mptcp_addr_info *addr)
 {
 	struct mptcp_pm_add_entry *entry;
 
@@ -1482,7 +1482,7 @@ static bool mptcp_pm_remove_anno_addr(struct mptcp_sock *msk,
 
 	list.ids[list.nr++] = mptcp_endp_get_local_id(msk, addr);
 
-	ret = remove_anno_list_by_saddr(msk, addr);
+	ret = mptcp_remove_anno_list_by_saddr(msk, addr);
 	if (ret || force) {
 		spin_lock_bh(&msk->pm.lock);
 		if (ret) {
@@ -1526,7 +1526,7 @@ static int mptcp_nl_remove_subflow_and_signal_addr(struct net *net,
 		}
 
 		lock_sock(sk);
-		remove_subflow = lookup_subflow_by_saddr(&msk->conn_list, addr);
+		remove_subflow = mptcp_lookup_subflow_by_saddr(&msk->conn_list, addr);
 		mptcp_pm_remove_anno_addr(msk, addr, remove_subflow &&
 					  !(entry->flags & MPTCP_PM_ADDR_FLAG_IMPLICIT));
 
@@ -1639,35 +1639,6 @@ int mptcp_pm_nl_del_addr_doit(struct sk_buff *skb, struct genl_info *info)
 	return ret;
 }
 
-/* Called from the userspace PM only */
-void mptcp_pm_remove_addrs(struct mptcp_sock *msk, struct list_head *rm_list)
-{
-	struct mptcp_rm_list alist = { .nr = 0 };
-	struct mptcp_pm_addr_entry *entry;
-	int anno_nr = 0;
-
-	list_for_each_entry(entry, rm_list, list) {
-		if (alist.nr >= MPTCP_RM_IDS_MAX)
-			break;
-
-		/* only delete if either announced or matching a subflow */
-		if (remove_anno_list_by_saddr(msk, &entry->addr))
-			anno_nr++;
-		else if (!lookup_subflow_by_saddr(&msk->conn_list,
-						  &entry->addr))
-			continue;
-
-		alist.ids[alist.nr++] = entry->addr.id;
-	}
-
-	if (alist.nr) {
-		spin_lock_bh(&msk->pm.lock);
-		msk->pm.add_addr_signaled -= anno_nr;
-		mptcp_pm_remove_addr(msk, &alist);
-		spin_unlock_bh(&msk->pm.lock);
-	}
-}
-
 /* Called from the in-kernel PM only */
 static void mptcp_pm_flush_addrs_and_subflows(struct mptcp_sock *msk,
 					      struct list_head *rm_list)
@@ -1677,11 +1648,11 @@ static void mptcp_pm_flush_addrs_and_subflows(struct mptcp_sock *msk,
 
 	list_for_each_entry(entry, rm_list, list) {
 		if (slist.nr < MPTCP_RM_IDS_MAX &&
-		    lookup_subflow_by_saddr(&msk->conn_list, &entry->addr))
+		    mptcp_lookup_subflow_by_saddr(&msk->conn_list, &entry->addr))
 			slist.ids[slist.nr++] = mptcp_endp_get_local_id(msk, &entry->addr);
 
 		if (alist.nr < MPTCP_RM_IDS_MAX &&
-		    remove_anno_list_by_saddr(msk, &entry->addr))
+		    mptcp_remove_anno_list_by_saddr(msk, &entry->addr))
 			alist.ids[alist.nr++] = mptcp_endp_get_local_id(msk, &entry->addr);
 	}
 
diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index a6de837d8958..737a07f5defe 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -286,6 +286,34 @@ static int mptcp_userspace_pm_remove_id_zero_address(struct mptcp_sock *msk,
 	return err;
 }
 
+void mptcp_pm_remove_addrs(struct mptcp_sock *msk, struct list_head *rm_list)
+{
+	struct mptcp_rm_list alist = { .nr = 0 };
+	struct mptcp_pm_addr_entry *entry;
+	int anno_nr = 0;
+
+	list_for_each_entry(entry, rm_list, list) {
+		if (alist.nr >= MPTCP_RM_IDS_MAX)
+			break;
+
+		/* only delete if either announced or matching a subflow */
+		if (mptcp_remove_anno_list_by_saddr(msk, &entry->addr))
+			anno_nr++;
+		else if (!mptcp_lookup_subflow_by_saddr(&msk->conn_list,
+							&entry->addr))
+			continue;
+
+		alist.ids[alist.nr++] = entry->addr.id;
+	}
+
+	if (alist.nr) {
+		spin_lock_bh(&msk->pm.lock);
+		msk->pm.add_addr_signaled -= anno_nr;
+		mptcp_pm_remove_addr(msk, &alist);
+		spin_unlock_bh(&msk->pm.lock);
+	}
+}
+
 int mptcp_pm_nl_remove_doit(struct sk_buff *skb, struct genl_info *info)
 {
 	struct nlattr *id = info->attrs[MPTCP_PM_ATTR_LOC_ID];
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index b4c72a73594f..80d355c1dfb4 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -1031,6 +1031,10 @@ mptcp_pm_del_add_timer(struct mptcp_sock *msk,
 struct mptcp_pm_add_entry *
 mptcp_lookup_anno_list_by_saddr(const struct mptcp_sock *msk,
 				const struct mptcp_addr_info *addr);
+bool mptcp_lookup_subflow_by_saddr(const struct list_head *list,
+				   const struct mptcp_addr_info *saddr);
+bool mptcp_remove_anno_list_by_saddr(struct mptcp_sock *msk,
+				     const struct mptcp_addr_info *addr);
 int mptcp_pm_set_flags(struct sk_buff *skb, struct genl_info *info);
 int mptcp_pm_nl_set_flags(struct sk_buff *skb, struct genl_info *info);
 int mptcp_userspace_pm_set_flags(struct sk_buff *skb, struct genl_info *info);
-- 
2.45.2

Hi Geliang,

On 07/11/2024 07:45, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> Since mptcp_pm_remove_addrs is only called from the userspace PM, this
> patch moves it into pm_userspace.c.
> 
> For this, lookup_subflow_by_saddr() and remove_anno_list_by_saddr()
> helpers need to be exported in protocol.h. Also add "mptcp_" prefix for
> these helpers.
> 
> This patch doesn't change the behaviour of the code, just refactoring.
> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  net/mptcp/pm_netlink.c   | 45 +++++++---------------------------------
>  net/mptcp/pm_userspace.c | 28 +++++++++++++++++++++++++
>  net/mptcp/protocol.h     |  4 ++++
>  3 files changed, 40 insertions(+), 37 deletions(-)
> 
> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index 618289aac0ab..8aba7670345d 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c

(...)

> @@ -1639,35 +1639,6 @@ int mptcp_pm_nl_del_addr_doit(struct sk_buff *skb, struct genl_info *info)
>  	return ret;
>  }
>  
> -/* Called from the userspace PM only */
> -void mptcp_pm_remove_addrs(struct mptcp_sock *msk, struct list_head *rm_list)
> -{
> -	struct mptcp_rm_list alist = { .nr = 0 };
> -	struct mptcp_pm_addr_entry *entry;
> -	int anno_nr = 0;
> -
> -	list_for_each_entry(entry, rm_list, list) {
> -		if (alist.nr >= MPTCP_RM_IDS_MAX)
> -			break;
> -
> -		/* only delete if either announced or matching a subflow */
> -		if (remove_anno_list_by_saddr(msk, &entry->addr))
> -			anno_nr++;
> -		else if (!lookup_subflow_by_saddr(&msk->conn_list,
> -						  &entry->addr))
> -			continue;
> -
> -		alist.ids[alist.nr++] = entry->addr.id;
> -	}
> -
> -	if (alist.nr) {
> -		spin_lock_bh(&msk->pm.lock);
> -		msk->pm.add_addr_signaled -= anno_nr;
> -		mptcp_pm_remove_addr(msk, &alist);
> -		spin_unlock_bh(&msk->pm.lock);
> -	}
> -}
> -
>  /* Called from the in-kernel PM only */

I guess we can remove this command as well.

(Something I can change when applying the patch if there is nothing else.)

>  static void mptcp_pm_flush_addrs_and_subflows(struct mptcp_sock *msk,
>  					      struct list_head *rm_list)
> @@ -1677,11 +1648,11 @@ static void mptcp_pm_flush_addrs_and_subflows(struct mptcp_sock *msk,
>  
>  	list_for_each_entry(entry, rm_list, list) {
>  		if (slist.nr < MPTCP_RM_IDS_MAX &&
> -		    lookup_subflow_by_saddr(&msk->conn_list, &entry->addr))
> +		    mptcp_lookup_subflow_by_saddr(&msk->conn_list, &entry->addr))
>  			slist.ids[slist.nr++] = mptcp_endp_get_local_id(msk, &entry->addr);
>  
>  		if (alist.nr < MPTCP_RM_IDS_MAX &&
> -		    remove_anno_list_by_saddr(msk, &entry->addr))
> +		    mptcp_remove_anno_list_by_saddr(msk, &entry->addr))
>  			alist.ids[alist.nr++] = mptcp_endp_get_local_id(msk, &entry->addr);
>  	}
>  
> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> index a6de837d8958..737a07f5defe 100644
> --- a/net/mptcp/pm_userspace.c
> +++ b/net/mptcp/pm_userspace.c
> @@ -286,6 +286,34 @@ static int mptcp_userspace_pm_remove_id_zero_address(struct mptcp_sock *msk,
>  	return err;
>  }
>  
> +void mptcp_pm_remove_addrs(struct mptcp_sock *msk, struct list_head *rm_list)

Can it not be 'static' now that it is only used in the userspace pm?

(...)

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

Hi Matt,

Thanks for the review.

On Wed, 2024-12-04 at 18:48 +0100, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 07/11/2024 07:45, Geliang Tang wrote:
> > From: Geliang Tang <tanggeliang@kylinos.cn>
> > 
> > Since mptcp_pm_remove_addrs is only called from the userspace PM,
> > this
> > patch moves it into pm_userspace.c.
> > 
> > For this, lookup_subflow_by_saddr() and remove_anno_list_by_saddr()
> > helpers need to be exported in protocol.h. Also add "mptcp_" prefix
> > for
> > these helpers.
> > 
> > This patch doesn't change the behaviour of the code, just
> > refactoring.
> > 
> > Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> > ---
> >  net/mptcp/pm_netlink.c   | 45 +++++++-----------------------------
> > ----
> >  net/mptcp/pm_userspace.c | 28 +++++++++++++++++++++++++
> >  net/mptcp/protocol.h     |  4 ++++
> >  3 files changed, 40 insertions(+), 37 deletions(-)
> > 
> > diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> > index 618289aac0ab..8aba7670345d 100644
> > --- a/net/mptcp/pm_netlink.c
> > +++ b/net/mptcp/pm_netlink.c
> 
> (...)
> 
> > @@ -1639,35 +1639,6 @@ int mptcp_pm_nl_del_addr_doit(struct sk_buff
> > *skb, struct genl_info *info)
> >  	return ret;
> >  }
> >  
> > -/* Called from the userspace PM only */
> > -void mptcp_pm_remove_addrs(struct mptcp_sock *msk, struct
> > list_head *rm_list)
> > -{
> > -	struct mptcp_rm_list alist = { .nr = 0 };
> > -	struct mptcp_pm_addr_entry *entry;
> > -	int anno_nr = 0;
> > -
> > -	list_for_each_entry(entry, rm_list, list) {
> > -		if (alist.nr >= MPTCP_RM_IDS_MAX)
> > -			break;
> > -
> > -		/* only delete if either announced or matching a
> > subflow */
> > -		if (remove_anno_list_by_saddr(msk, &entry->addr))
> > -			anno_nr++;
> > -		else if (!lookup_subflow_by_saddr(&msk->conn_list,
> > -						  &entry->addr))
> > -			continue;
> > -
> > -		alist.ids[alist.nr++] = entry->addr.id;
> > -	}
> > -
> > -	if (alist.nr) {
> > -		spin_lock_bh(&msk->pm.lock);
> > -		msk->pm.add_addr_signaled -= anno_nr;
> > -		mptcp_pm_remove_addr(msk, &alist);
> > -		spin_unlock_bh(&msk->pm.lock);
> > -	}
> > -}
> > -
> >  /* Called from the in-kernel PM only */
> 
> I guess we can remove this command as well.

Dropped this in v4.

> 
> (Something I can change when applying the patch if there is nothing
> else.)
> 
> >  static void mptcp_pm_flush_addrs_and_subflows(struct mptcp_sock
> > *msk,
> >  					      struct list_head
> > *rm_list)
> > @@ -1677,11 +1648,11 @@ static void
> > mptcp_pm_flush_addrs_and_subflows(struct mptcp_sock *msk,
> >  
> >  	list_for_each_entry(entry, rm_list, list) {
> >  		if (slist.nr < MPTCP_RM_IDS_MAX &&
> > -		    lookup_subflow_by_saddr(&msk->conn_list,
> > &entry->addr))
> > +		    mptcp_lookup_subflow_by_saddr(&msk->conn_list,
> > &entry->addr))
> >  			slist.ids[slist.nr++] =
> > mptcp_endp_get_local_id(msk, &entry->addr);
> >  
> >  		if (alist.nr < MPTCP_RM_IDS_MAX &&
> > -		    remove_anno_list_by_saddr(msk, &entry->addr))
> > +		    mptcp_remove_anno_list_by_saddr(msk, &entry-
> > >addr))
> >  			alist.ids[alist.nr++] =
> > mptcp_endp_get_local_id(msk, &entry->addr);
> >  	}
> >  
> > diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> > index a6de837d8958..737a07f5defe 100644
> > --- a/net/mptcp/pm_userspace.c
> > +++ b/net/mptcp/pm_userspace.c
> > @@ -286,6 +286,34 @@ static int
> > mptcp_userspace_pm_remove_id_zero_address(struct mptcp_sock *msk,
> >  	return err;
> >  }
> >  
> > +void mptcp_pm_remove_addrs(struct mptcp_sock *msk, struct
> > list_head *rm_list)
> 
> Can it not be 'static' now that it is only used in the userspace pm?

It will be invoked in BPF path manager, 'static' doesn't work.

> 
> (...)
> 
> Cheers,
> Matt

Hi Geliang,

On 05/12/2024 08:26, Geliang Tang wrote:
> On Wed, 2024-12-04 at 18:48 +0100, Matthieu Baerts wrote:
>> On 07/11/2024 07:45, Geliang Tang wrote:

(...)

>>> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
>>> index a6de837d8958..737a07f5defe 100644
>>> --- a/net/mptcp/pm_userspace.c
>>> +++ b/net/mptcp/pm_userspace.c
>>> @@ -286,6 +286,34 @@ static int
>>> mptcp_userspace_pm_remove_id_zero_address(struct mptcp_sock *msk,
>>>  	return err;
>>>  }
>>>  
>>> +void mptcp_pm_remove_addrs(struct mptcp_sock *msk, struct
>>> list_head *rm_list)
>>
>> Can it not be 'static' now that it is only used in the userspace pm?
> 
> It will be invoked in BPF path manager, 'static' doesn't work.

OK. Hopefully people will not send a patch in between to add 'static'.

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

On Thu, 2024-12-05 at 10:27 +0100, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 05/12/2024 08:26, Geliang Tang wrote:
> > On Wed, 2024-12-04 at 18:48 +0100, Matthieu Baerts wrote:
> > > On 07/11/2024 07:45, Geliang Tang wrote:
> 
> (...)
> 
> > > > diff --git a/net/mptcp/pm_userspace.c
> > > > b/net/mptcp/pm_userspace.c
> > > > index a6de837d8958..737a07f5defe 100644
> > > > --- a/net/mptcp/pm_userspace.c
> > > > +++ b/net/mptcp/pm_userspace.c
> > > > @@ -286,6 +286,34 @@ static int
> > > > mptcp_userspace_pm_remove_id_zero_address(struct mptcp_sock
> > > > *msk,
> > > >  	return err;
> > > >  }
> > > >  
> > > > +void mptcp_pm_remove_addrs(struct mptcp_sock *msk, struct
> > > > list_head *rm_list)
> > > 
> > > Can it not be 'static' now that it is only used in the userspace
> > > pm?
> > 
> > It will be invoked in BPF path manager, 'static' doesn't work.
> 
> OK. Hopefully people will not send a patch in between to add
> 'static'.

I've mentioned this in the commit log of v4:

'''
Here, mptcp_pm_remove_addrs() is not changed to a static function
because it will be used in BPF Path Manager.
''' 
Thanks,
-Geliang

> 
> Cheers,
> Matt

On 05/12/2024 10:36, Geliang Tang wrote:
> On Thu, 2024-12-05 at 10:27 +0100, Matthieu Baerts wrote:
>> Hi Geliang,
>>
>> On 05/12/2024 08:26, Geliang Tang wrote:
>>> On Wed, 2024-12-04 at 18:48 +0100, Matthieu Baerts wrote:
>>>> On 07/11/2024 07:45, Geliang Tang wrote:
>>
>> (...)
>>
>>>>> diff --git a/net/mptcp/pm_userspace.c
>>>>> b/net/mptcp/pm_userspace.c
>>>>> index a6de837d8958..737a07f5defe 100644
>>>>> --- a/net/mptcp/pm_userspace.c
>>>>> +++ b/net/mptcp/pm_userspace.c
>>>>> @@ -286,6 +286,34 @@ static int
>>>>> mptcp_userspace_pm_remove_id_zero_address(struct mptcp_sock
>>>>> *msk,
>>>>>  	return err;
>>>>>  }
>>>>>  
>>>>> +void mptcp_pm_remove_addrs(struct mptcp_sock *msk, struct
>>>>> list_head *rm_list)
>>>>
>>>> Can it not be 'static' now that it is only used in the userspace
>>>> pm?
>>>
>>> It will be invoked in BPF path manager, 'static' doesn't work.
>>
>> OK. Hopefully people will not send a patch in between to add
>> 'static'.
> 
> I've mentioned this in the commit log of v4:
> 
> '''
> Here, mptcp_pm_remove_addrs() is not changed to a static function
> because it will be used in BPF Path Manager.
> '''

Good, thank you (I saw that after I sent my previous email).

If someone sends a patch, we can refer to this commit then.

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