diff mbox series

[mptcp-next,v2,05/36] mptcp: add lookup_addr for userspace pm

Message ID 6d30431fd64e8c17d163ab5656838e1c3be78b97.1729588019.git.tanggeliang@kylinos.cn (mailing list archive)
State Superseded, archived
Delegated to: Matthieu Baerts
Headers show
Series BPF path manager | expand

Checks

Context Check Description
matttbe/checkpatch success total: 0 errors, 0 warnings, 0 checks, 86 lines checked
matttbe/shellcheck success MPTCP selftests files have not been modified
matttbe/build warning Build error with: make C=1 net/mptcp/bpf.o
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 Oct. 22, 2024, 9:14 a.m. UTC 
From: Geliang Tang <tanggeliang@kylinos.cn>

Like __lookup_addr() helper in pm_netlink.c, a new helper
mptcp_userspace_pm_lookup_addr() is also defined in pm_userspace.c.
It looks up the corresponding mptcp_pm_addr_entry address in
userspace_pm_local_addr_list through the passed "addr" parameter
and returns it.

This helper can be used in mptcp_userspace_pm_delete_local_addr(),
mptcp_userspace_pm_get_local_id() and mptcp_userspace_pm_is_backup()
to simplify the code.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 net/mptcp/pm_userspace.c | 56 +++++++++++++++++++++-------------------
 1 file changed, 29 insertions(+), 27 deletions(-)

Comments

Matthieu Baerts Oct. 30, 2024, 6:21 p.m. UTC | #1 
Hi Geliang,

On 22/10/2024 11:14, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> Like __lookup_addr() helper in pm_netlink.c, a new helper
> mptcp_userspace_pm_lookup_addr() is also defined in pm_userspace.c.
> It looks up the corresponding mptcp_pm_addr_entry address in
> userspace_pm_local_addr_list through the passed "addr" parameter
> and returns it.
> 
> This helper can be used in mptcp_userspace_pm_delete_local_addr(),
> mptcp_userspace_pm_get_local_id() and mptcp_userspace_pm_is_backup()
> to simplify the code.
> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  net/mptcp/pm_userspace.c | 56 +++++++++++++++++++++-------------------
>  1 file changed, 29 insertions(+), 27 deletions(-)
> 
> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> index 3fb5713cd988..ce0f7131c701 100644
> --- a/net/mptcp/pm_userspace.c
> +++ b/net/mptcp/pm_userspace.c
> @@ -26,6 +26,18 @@ void mptcp_free_local_addr_list(struct mptcp_sock *msk)
>  	}
>  }
>  
> +static struct mptcp_pm_addr_entry *
> +mptcp_userspace_pm_lookup_addr(struct mptcp_sock *msk, const struct mptcp_addr_info *addr)

When possible, can you try to limit to 80 chars per line?

See: https://github.com/linux-netdev/nipa/pull/41

Using more than 80 is allowed, but it should be restricted to cases
where using less than 80 chars affects the readability, e.g. not to
break 'entry->flags & MY_SPECIFIC_FLAG' in two lines, etc. The idea is
not to abuse of that.

Here for example, it is easy to go to the new line after the ','.

> +{
> +	struct mptcp_pm_addr_entry *entry, *tmp;
> +
> +	mptcp_for_each_address_safe(msk, entry, tmp) {

Why do you need the '_safe' alternative here? You only return an entry
from the list, and you stop: no need to continue after having modified
the list here as far as I can see, no?

Also, something very important: here you are presenting the modification
as a simple refactoring, but it does change the behaviour: the '_safe'
version is used everywhere, which was not the case before. When you do
something like that, please mention it in the commit message! Without
that, a reviewer might not notice it "OK, just a refactoring", and
developers might wonder later why this was done. I then recommend to
always add either something like:
- "No behaviour change intended here."
- or "Please note that now <something different is done> for <these
cases>, but that's OK to do so <because ...>."

> +		if (mptcp_addresses_equal(&entry->addr, addr, false))
> +			return entry;
> +	}
> +	return NULL;
> +}
> +
>  static int mptcp_userspace_pm_append_new_local_addr(struct mptcp_sock *msk,
>  						    struct mptcp_pm_addr_entry *entry,
>  						    bool needs_id)
> @@ -90,22 +102,20 @@ static int mptcp_userspace_pm_append_new_local_addr(struct mptcp_sock *msk,
>  static int mptcp_userspace_pm_delete_local_addr(struct mptcp_sock *msk,
>  						struct mptcp_pm_addr_entry *addr)
>  {
> -	struct mptcp_pm_addr_entry *entry, *tmp;
>  	struct sock *sk = (struct sock *)msk;
> +	struct mptcp_pm_addr_entry *entry;
>  
> -	mptcp_for_each_address_safe(msk, entry, tmp) {
> -		if (mptcp_addresses_equal(&entry->addr, &addr->addr, false)) {
> -			/* TODO: a refcount is needed because the entry can
> -			 * be used multiple times (e.g. fullmesh mode).
> -			 */
> -			list_del_rcu(&entry->list);
> -			sock_kfree_s(sk, entry, sizeof(*entry));
> -			msk->pm.local_addr_used--;
> -			return 0;
> -		}
> -	}
> -
> -	return -EINVAL;
> +	entry = mptcp_userspace_pm_lookup_addr(msk, &addr->addr);
> +	if (!entry)
> +		return -EINVAL;
> +
> +	/* TODO: a refcount is needed because the entry can
> +	 * be used multiple times (e.g. fullmesh mode).
> +	 */

(Not related to this commit: I wonder if the TODO still makes sense. We
had some discussions with Mat, and I think the conclusion was that it
was OK, but I don't remember why)

> +	list_del_rcu(&entry->list);
> +	sock_kfree_s(sk, entry, sizeof(*entry));
> +	msk->pm.local_addr_used--;
> +	return 0;
>  }
>  
>  static struct mptcp_pm_addr_entry *
> @@ -123,17 +133,12 @@ mptcp_userspace_pm_lookup_addr_by_id(struct mptcp_sock *msk, unsigned int id)
>  int mptcp_userspace_pm_get_local_id(struct mptcp_sock *msk,
>  				    struct mptcp_addr_info *skc)
>  {
> -	struct mptcp_pm_addr_entry *entry = NULL, *e, new_entry;
> +	struct mptcp_pm_addr_entry *entry = NULL, new_entry;
>  	__be16 msk_sport =  ((struct inet_sock *)
>  			     inet_sk((struct sock *)msk))->inet_sport;
>  
>  	spin_lock_bh(&msk->pm.lock);
> -	mptcp_for_each_address(msk, e) {
> -		if (mptcp_addresses_equal(&e->addr, skc, false)) {
> -			entry = e;
> -			break;
> -		}
> -	}
> +	entry = mptcp_userspace_pm_lookup_addr(msk, skc);
>  	spin_unlock_bh(&msk->pm.lock);
>  	if (entry)
>  		return entry->addr.id;
> @@ -156,12 +161,9 @@ bool mptcp_userspace_pm_is_backup(struct mptcp_sock *msk,
>  	bool backup = false;
>  
>  	spin_lock_bh(&msk->pm.lock);
> -	mptcp_for_each_address(msk, entry) {
> -		if (mptcp_addresses_equal(&entry->addr, skc, false)) {
> -			backup = !!(entry->flags & MPTCP_PM_ADDR_FLAG_BACKUP);
> -			break;
> -		}
> -	}
> +	entry = mptcp_userspace_pm_lookup_addr(msk, skc);
> +	if (entry)
> +		backup = !!(entry->flags & MPTCP_PM_ADDR_FLAG_BACKUP);
>  	spin_unlock_bh(&msk->pm.lock);
>  
>  	return backup;

Cheers,
Matt
diff mbox series

Patch

diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index 3fb5713cd988..ce0f7131c701 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -26,6 +26,18 @@  void mptcp_free_local_addr_list(struct mptcp_sock *msk)
 	}
 }
 
+static struct mptcp_pm_addr_entry *
+mptcp_userspace_pm_lookup_addr(struct mptcp_sock *msk, const struct mptcp_addr_info *addr)
+{
+	struct mptcp_pm_addr_entry *entry, *tmp;
+
+	mptcp_for_each_address_safe(msk, entry, tmp) {
+		if (mptcp_addresses_equal(&entry->addr, addr, false))
+			return entry;
+	}
+	return NULL;
+}
+
 static int mptcp_userspace_pm_append_new_local_addr(struct mptcp_sock *msk,
 						    struct mptcp_pm_addr_entry *entry,
 						    bool needs_id)
@@ -90,22 +102,20 @@  static int mptcp_userspace_pm_append_new_local_addr(struct mptcp_sock *msk,
 static int mptcp_userspace_pm_delete_local_addr(struct mptcp_sock *msk,
 						struct mptcp_pm_addr_entry *addr)
 {
-	struct mptcp_pm_addr_entry *entry, *tmp;
 	struct sock *sk = (struct sock *)msk;
+	struct mptcp_pm_addr_entry *entry;
 
-	mptcp_for_each_address_safe(msk, entry, tmp) {
-		if (mptcp_addresses_equal(&entry->addr, &addr->addr, false)) {
-			/* TODO: a refcount is needed because the entry can
-			 * be used multiple times (e.g. fullmesh mode).
-			 */
-			list_del_rcu(&entry->list);
-			sock_kfree_s(sk, entry, sizeof(*entry));
-			msk->pm.local_addr_used--;
-			return 0;
-		}
-	}
-
-	return -EINVAL;
+	entry = mptcp_userspace_pm_lookup_addr(msk, &addr->addr);
+	if (!entry)
+		return -EINVAL;
+
+	/* TODO: a refcount is needed because the entry can
+	 * be used multiple times (e.g. fullmesh mode).
+	 */
+	list_del_rcu(&entry->list);
+	sock_kfree_s(sk, entry, sizeof(*entry));
+	msk->pm.local_addr_used--;
+	return 0;
 }
 
 static struct mptcp_pm_addr_entry *
@@ -123,17 +133,12 @@  mptcp_userspace_pm_lookup_addr_by_id(struct mptcp_sock *msk, unsigned int id)
 int mptcp_userspace_pm_get_local_id(struct mptcp_sock *msk,
 				    struct mptcp_addr_info *skc)
 {
-	struct mptcp_pm_addr_entry *entry = NULL, *e, new_entry;
+	struct mptcp_pm_addr_entry *entry = NULL, new_entry;
 	__be16 msk_sport =  ((struct inet_sock *)
 			     inet_sk((struct sock *)msk))->inet_sport;
 
 	spin_lock_bh(&msk->pm.lock);
-	mptcp_for_each_address(msk, e) {
-		if (mptcp_addresses_equal(&e->addr, skc, false)) {
-			entry = e;
-			break;
-		}
-	}
+	entry = mptcp_userspace_pm_lookup_addr(msk, skc);
 	spin_unlock_bh(&msk->pm.lock);
 	if (entry)
 		return entry->addr.id;
@@ -156,12 +161,9 @@  bool mptcp_userspace_pm_is_backup(struct mptcp_sock *msk,
 	bool backup = false;
 
 	spin_lock_bh(&msk->pm.lock);
-	mptcp_for_each_address(msk, entry) {
-		if (mptcp_addresses_equal(&entry->addr, skc, false)) {
-			backup = !!(entry->flags & MPTCP_PM_ADDR_FLAG_BACKUP);
-			break;
-		}
-	}
+	entry = mptcp_userspace_pm_lookup_addr(msk, skc);
+	if (entry)
+		backup = !!(entry->flags & MPTCP_PM_ADDR_FLAG_BACKUP);
 	spin_unlock_bh(&msk->pm.lock);
 
 	return backup;