[v3] BPF path manager, part 1

[PATCH mptcp-next v3 5/9] mptcp: drop free_list for deleting entries

Posted by Geliang Tang 1 year, 3 months ago

From: Geliang Tang <tanggeliang@kylinos.cn>

mptcp_pm_remove_addrs() actually only deletes one address, which does
not match its name. This patch renames it to mptcp_pm_remove_addr_entry()
and changes the parameter "rm_list" to "entry".

With the help of mptcp_pm_remove_addr_entry(), it's no longer necessary to
move the entry to be deleted to free_list and then traverse the list to
delete the entry, which is not allowed in BPF. The entry can be directly
deleted through list_del_rcu() and sock_kfree_s() now.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 net/mptcp/pm_userspace.c | 33 ++++++++++++---------------------
 net/mptcp/protocol.h     |  3 ++-
 2 files changed, 14 insertions(+), 22 deletions(-)

diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index 737a07f5defe..a98da9a44bfa 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -286,26 +286,21 @@ 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)
+void mptcp_pm_remove_addr_entry(struct mptcp_sock *msk,
+				struct mptcp_pm_addr_entry *entry)
 {
 	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;
+	/* 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))
+		goto out;
 
-		alist.ids[alist.nr++] = entry->addr.id;
-	}
+	alist.ids[alist.nr++] = entry->addr.id;
 
+out:
 	if (alist.nr) {
 		spin_lock_bh(&msk->pm.lock);
 		msk->pm.add_addr_signaled -= anno_nr;
@@ -318,9 +313,7 @@ int mptcp_pm_nl_remove_doit(struct sk_buff *skb, struct genl_info *info)
 {
 	struct nlattr *id = info->attrs[MPTCP_PM_ATTR_LOC_ID];
 	struct mptcp_pm_addr_entry *match;
-	struct mptcp_pm_addr_entry *entry;
 	struct mptcp_sock *msk;
-	LIST_HEAD(free_list);
 	int err = -EINVAL;
 	struct sock *sk;
 	u8 id_val;
@@ -354,16 +347,14 @@ int mptcp_pm_nl_remove_doit(struct sk_buff *skb, struct genl_info *info)
 		goto out;
 	}
 
-	list_move(&match->list, &free_list);
+	list_del_rcu(&match->list);
 	spin_unlock_bh(&msk->pm.lock);
 
-	mptcp_pm_remove_addrs(msk, &free_list);
+	mptcp_pm_remove_addr_entry(msk, match);
 
 	release_sock(sk);
 
-	list_for_each_entry_safe(match, entry, &free_list, list) {
-		sock_kfree_s(sk, match, sizeof(*match));
-	}
+	sock_kfree_s(sk, match, sizeof(*match));
 
 	err = 0;
 out:
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 80d355c1dfb4..19a811220621 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -1042,7 +1042,8 @@ 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);
-void mptcp_pm_remove_addrs(struct mptcp_sock *msk, struct list_head *rm_list);
+void mptcp_pm_remove_addr_entry(struct mptcp_sock *msk,
+				struct mptcp_pm_addr_entry *entry);
 
 void mptcp_free_local_addr_list(struct mptcp_sock *msk);
 
-- 
2.45.2

Re: [PATCH mptcp-next v3 5/9] mptcp: drop free_list for deleting entries

Posted by Matthieu Baerts 1 year, 2 months ago

Hi Geliang,

On 07/11/2024 07:45, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> mptcp_pm_remove_addrs() actually only deletes one address, which does
> not match its name. This patch renames it to mptcp_pm_remove_addr_entry()
> and changes the parameter "rm_list" to "entry".
> 
> With the help of mptcp_pm_remove_addr_entry(), it's no longer necessary to
> move the entry to be deleted to free_list and then traverse the list to
> delete the entry, which is not allowed in BPF. The entry can be directly
> deleted through list_del_rcu() and sock_kfree_s() now.
> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  net/mptcp/pm_userspace.c | 33 ++++++++++++---------------------
>  net/mptcp/protocol.h     |  3 ++-
>  2 files changed, 14 insertions(+), 22 deletions(-)
> 
> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> index 737a07f5defe..a98da9a44bfa 100644
> --- a/net/mptcp/pm_userspace.c
> +++ b/net/mptcp/pm_userspace.c
> @@ -286,26 +286,21 @@ 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)
> +void mptcp_pm_remove_addr_entry(struct mptcp_sock *msk,
> +				struct mptcp_pm_addr_entry *entry)
>  {
>  	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;
> +	/* 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))
> +		goto out;

Here, you can 'return', no need to use this new 'out' label.

>  
> -		alist.ids[alist.nr++] = entry->addr.id;
> -	}
> +	alist.ids[alist.nr++] = entry->addr.id;
>  
> +out:
>  	if (alist.nr) {

If the 'out' label is removed, you can also remove this if-statement.

>  		spin_lock_bh(&msk->pm.lock);
>  		msk->pm.add_addr_signaled -= anno_nr;

(...)

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

Re: [PATCH mptcp-next v3 5/9] mptcp: drop free_list for deleting entries

Posted by Geliang Tang 1 year, 2 months ago

On Wed, 2024-12-04 at 18:49 +0100, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 07/11/2024 07:45, Geliang Tang wrote:
> > From: Geliang Tang <tanggeliang@kylinos.cn>
> > 
> > mptcp_pm_remove_addrs() actually only deletes one address, which
> > does
> > not match its name. This patch renames it to
> > mptcp_pm_remove_addr_entry()
> > and changes the parameter "rm_list" to "entry".
> > 
> > With the help of mptcp_pm_remove_addr_entry(), it's no longer
> > necessary to
> > move the entry to be deleted to free_list and then traverse the
> > list to
> > delete the entry, which is not allowed in BPF. The entry can be
> > directly
> > deleted through list_del_rcu() and sock_kfree_s() now.
> > 
> > Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> > ---
> >  net/mptcp/pm_userspace.c | 33 ++++++++++++---------------------
> >  net/mptcp/protocol.h     |  3 ++-
> >  2 files changed, 14 insertions(+), 22 deletions(-)
> > 
> > diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> > index 737a07f5defe..a98da9a44bfa 100644
> > --- a/net/mptcp/pm_userspace.c
> > +++ b/net/mptcp/pm_userspace.c
> > @@ -286,26 +286,21 @@ 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)
> > +void mptcp_pm_remove_addr_entry(struct mptcp_sock *msk,
> > +				struct mptcp_pm_addr_entry *entry)
> >  {
> >  	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;
> > +	/* 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))
> > +		goto out;
> 
> Here, you can 'return', no need to use this new 'out' label.
> 
> >  
> > -		alist.ids[alist.nr++] = entry->addr.id;
> > -	}
> > +	alist.ids[alist.nr++] = entry->addr.id;
> >  
> > +out:
> >  	if (alist.nr) {
> 
> If the 'out' label is removed, you can also remove this if-statement.

Good idea! Thanks.

> 
> >  		spin_lock_bh(&msk->pm.lock);
> >  		msk->pm.add_addr_signaled -= anno_nr;
> 
> (...)
> 
> Cheers,
> Matt