diff mbox series

[mptcp-next,v1,4/6] mptcp: pm: in-kernel: drop is_userspace in remove_id_zero

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

Checks

Context Check Description
matttbe/checkpatch success total: 0 errors, 0 warnings, 0 checks, 108 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 Feb. 27, 2025, 6:43 a.m. UTC 
From: Geliang Tang <tanggeliang@kylinos.cn>

There're duplicate operations in mptcp_nl_remove_subflow_and_signal_addr()
and mptcp_nl_remove_id_zero_address(), both of which traverse all mptcp
sockets in the net namespace. This patch drops the traversal operation in
the latter and reuse the traversal loop of the former to do the removal of
id zero address.

An additional benefit is that the check for mptcp_pm_is_userspace() in
mptcp_nl_remove_id_zero_address() is dropped, which reduces the path
manager's reliance on mptcp_pm_is_userspace() and mptcp_pm_is_kernel()
helpers.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 net/mptcp/pm_netlink.c | 77 ++++++++++++++++++++----------------------
 1 file changed, 37 insertions(+), 40 deletions(-)

Comments

Matthieu Baerts Feb. 27, 2025, 12:10 p.m. UTC | #1 
Hi Geliang,

On 27/02/2025 07:43, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> There're duplicate operations in mptcp_nl_remove_subflow_and_signal_addr()
> and mptcp_nl_remove_id_zero_address(), both of which traverse all mptcp
> sockets in the net namespace. This patch drops the traversal operation in
> the latter and reuse the traversal loop of the former to do the removal of
> id zero address.

I'm hesitating here. It "feels" wrong to deal with an endpoint having 0
for the ID in the in-kernel manager code. Currently, the code is
separated from the beginning, and even if there is bit of duplication,
it looks clearer to deal with this special case.

(Also, I don't really see a use-case to delete only the first subflows
of all connections, but that's a different topic).

> An additional benefit is that the check for mptcp_pm_is_userspace() in
> mptcp_nl_remove_id_zero_address() is dropped, which reduces the path
> manager's reliance on mptcp_pm_is_userspace() and mptcp_pm_is_kernel()
> helpers.

I think these checks here make sense: we are iterating over all
connections, but we just need the ones handled by the in-kernel PM.

When the pm->ops will be there, can you not simply replace all these
checks under a mptcp_token_iter_next() by something like:

  if (&msk->pm.ops != &mptcp_pm_netlink)
      continue;

or:

  if (mptcp_pm_ops(msk) != mptcp_pm_netlink)

or with a macro for the iterator:

  mptcp_pm_for_each_msk(net, &s_slot, &s_num, &mptcp_pm_netlink) {

WDYT?

Cheers,
Matt
diff mbox series

Patch

diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index 0d98c2df72f7..e546388070b4 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -1514,6 +1514,28 @@  static void __mark_subflow_endp_available(struct mptcp_sock *msk, u8 id)
 		msk->pm.local_addr_used--;
 }
 
+static void mptcp_nl_remove_id_zero_address(struct mptcp_sock *msk,
+					    const struct mptcp_addr_info *addr)
+{
+	struct mptcp_rm_list list = { .nr = 0 };
+	struct mptcp_addr_info msk_local;
+
+	if (list_empty(&msk->conn_list))
+		return;
+
+	mptcp_local_address((struct sock_common *)msk, &msk_local);
+	if (!mptcp_addresses_equal(&msk_local, addr, addr->port))
+		return;
+
+	list.ids[list.nr++] = 0;
+
+	spin_lock_bh(&msk->pm.lock);
+	mptcp_pm_remove_addr(msk, &list);
+	mptcp_pm_nl_rm_subflow_received(msk, &list);
+	__mark_subflow_endp_available(msk, 0);
+	spin_unlock_bh(&msk->pm.lock);
+}
+
 static int mptcp_nl_remove_subflow_and_signal_addr(struct net *net,
 						   const struct mptcp_pm_addr_entry *entry)
 {
@@ -1532,6 +1554,11 @@  static int mptcp_nl_remove_subflow_and_signal_addr(struct net *net,
 			goto next;
 
 		lock_sock(sk);
+		if (entry->addr.id == 0) {
+			mptcp_nl_remove_id_zero_address(msk, &entry->addr);
+			goto out;
+		}
+
 		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));
@@ -1551,42 +1578,7 @@  static int mptcp_nl_remove_subflow_and_signal_addr(struct net *net,
 
 		if (msk->mpc_endpoint_id == entry->addr.id)
 			msk->mpc_endpoint_id = 0;
-		release_sock(sk);
-
-next:
-		sock_put(sk);
-		cond_resched();
-	}
-
-	return 0;
-}
-
-static int mptcp_nl_remove_id_zero_address(struct net *net,
-					   struct mptcp_addr_info *addr)
-{
-	struct mptcp_rm_list list = { .nr = 0 };
-	long s_slot = 0, s_num = 0;
-	struct mptcp_sock *msk;
-
-	list.ids[list.nr++] = 0;
-
-	while ((msk = mptcp_token_iter_next(net, &s_slot, &s_num)) != NULL) {
-		struct sock *sk = (struct sock *)msk;
-		struct mptcp_addr_info msk_local;
-
-		if (list_empty(&msk->conn_list) || mptcp_pm_is_userspace(msk))
-			goto next;
-
-		mptcp_local_address((struct sock_common *)msk, &msk_local);
-		if (!mptcp_addresses_equal(&msk_local, addr, addr->port))
-			goto next;
-
-		lock_sock(sk);
-		spin_lock_bh(&msk->pm.lock);
-		mptcp_pm_remove_addr(msk, &list);
-		mptcp_pm_nl_rm_subflow_received(msk, &list);
-		__mark_subflow_endp_available(msk, 0);
-		spin_unlock_bh(&msk->pm.lock);
+out:
 		release_sock(sk);
 
 next:
@@ -1618,8 +1610,10 @@  int mptcp_pm_nl_del_addr_doit(struct sk_buff *skb, struct genl_info *info)
 	 * id addresses. Additionally zero id is not accounted for in id_bitmap.
 	 * Let's use an 'mptcp_rm_list' instead of the common remove code.
 	 */
-	if (addr.addr.id == 0)
-		return mptcp_nl_remove_id_zero_address(sock_net(skb->sk), &addr.addr);
+	if (addr.addr.id == 0) {
+		entry = &addr;
+		goto del_addr;
+	}
 
 	spin_lock_bh(&pernet->lock);
 	entry = __lookup_addr_by_id(pernet, addr.addr.id);
@@ -1642,9 +1636,12 @@  int mptcp_pm_nl_del_addr_doit(struct sk_buff *skb, struct genl_info *info)
 	__clear_bit(entry->addr.id, pernet->id_bitmap);
 	spin_unlock_bh(&pernet->lock);
 
+del_addr:
 	mptcp_nl_remove_subflow_and_signal_addr(sock_net(skb->sk), entry);
-	synchronize_rcu();
-	__mptcp_pm_release_addr_entry(entry);
+	if (entry->addr.id) {
+		synchronize_rcu();
+		__mptcp_pm_release_addr_entry(entry);
+	}
 
 	return ret;
 }