[PATCH mptcp-next v1 0/6] BPF path manager, part 5

[PATCH mptcp-next v1 0/6] BPF path manager, part 5

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

Some path manager related refactoring and cleanups.
 - patch 1, a cleanup.
 - patches 2-4, drop mptcp_pm_is_userspace() and mptcp_pm_is_kernel().
 - patches 5-6, change remote of set_flags as mptcp_pm_addr_entry type.

Geliang Tang (6):
  mptcp: pm: use pm variable instead of msk->pm
  mptcp: pm: userspace: drop is_userspace in free_local_addr_list
  mptcp: pm: drop is_kernel in alloc_anno_list
  mptcp: pm: in-kernel: drop is_userspace in remove_id_zero
  mptcp: pm: add remote parameter for set_flags
  mptcp: pm: in-kernel: drop changed parameter of set_flags

 net/mptcp/pm.c           | 28 ++++++++----
 net/mptcp/pm_netlink.c   | 93 +++++++++++++++++++++-------------------
 net/mptcp/pm_userspace.c | 25 +++--------
 net/mptcp/protocol.h     |  5 ++-
 4 files changed, 77 insertions(+), 74 deletions(-)

-- 
2.43.0

[PATCH mptcp-next v1 1/6] mptcp: pm: use pm variable instead of msk->pm

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

The variable "pm" has been defined in mptcp_pm_fully_established()
and mptcp_pm_data_reset() as "sk->pm", so use "pm" directly instead
of using "sk->pm".

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

diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index ac7b39148bd3..1844107f6f93 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -139,13 +139,13 @@ void mptcp_pm_fully_established(struct mptcp_sock *msk, const struct sock *ssk)
 	 * be sure to serve this event only once.
 	 */
 	if (READ_ONCE(pm->work_pending) &&
-	    !(msk->pm.status & BIT(MPTCP_PM_ALREADY_ESTABLISHED)))
+	    !(pm->status & BIT(MPTCP_PM_ALREADY_ESTABLISHED)))
 		mptcp_pm_schedule_work(msk, MPTCP_PM_ESTABLISHED);
 
-	if ((msk->pm.status & BIT(MPTCP_PM_ALREADY_ESTABLISHED)) == 0)
+	if ((pm->status & BIT(MPTCP_PM_ALREADY_ESTABLISHED)) == 0)
 		announce = true;
 
-	msk->pm.status |= BIT(MPTCP_PM_ALREADY_ESTABLISHED);
+	pm->status |= BIT(MPTCP_PM_ALREADY_ESTABLISHED);
 	spin_unlock_bh(&pm->lock);
 
 	if (announce)
@@ -632,7 +632,7 @@ void mptcp_pm_data_reset(struct mptcp_sock *msk)
 	WRITE_ONCE(pm->addr_signal, 0);
 	WRITE_ONCE(pm->remote_deny_join_id0, false);
 	pm->status = 0;
-	bitmap_fill(msk->pm.id_avail_bitmap, MPTCP_PM_MAX_ADDR_ID + 1);
+	bitmap_fill(pm->id_avail_bitmap, MPTCP_PM_MAX_ADDR_ID + 1);
 }
 
 void mptcp_pm_data_init(struct mptcp_sock *msk)
-- 
2.43.0

[PATCH mptcp-next v1 2/6] mptcp: pm: userspace: drop is_userspace in free_local_addr_list

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

To reduce the path manager's reliance on mptcp_pm_is_userspace()
and mptcp_pm_is_kernel() helpers, this patch drops the check for
mptcp_pm_is_userspace() in mptcp_free_local_addr_list() and
replaces it with a check to see if userspace_pm_local_addr_list
is empty.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 net/mptcp/pm_userspace.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index 5b3ee43130be..98fe8808d1e1 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -18,7 +18,7 @@ void mptcp_free_local_addr_list(struct mptcp_sock *msk)
 	struct sock *sk = (struct sock *)msk;
 	LIST_HEAD(free_list);
 
-	if (!mptcp_pm_is_userspace(msk))
+	if (list_empty(&msk->pm.userspace_pm_local_addr_list))
 		return;
 
 	spin_lock_bh(&msk->pm.lock);
-- 
2.43.0

[PATCH mptcp-next v1 3/6] mptcp: pm: drop is_kernel in alloc_anno_list

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

To reduce the path manager's reliance on mptcp_pm_is_userspace()
and mptcp_pm_is_kernel() helpers, this patch drops the check for
mptcp_pm_is_kernel() in the function mptcp_pm_alloc_anno_list().
Instead, add a new parameter "reissue" for this function, pass
"false" to this function in the in-kernel PM while pass "true"
to it in the userspace PM.

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

diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index 23c28e37ab8f..0d98c2df72f7 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -358,7 +358,8 @@ mptcp_pm_del_add_timer(struct mptcp_sock *msk,
 }
 
 bool mptcp_pm_alloc_anno_list(struct mptcp_sock *msk,
-			      const struct mptcp_addr_info *addr)
+			      const struct mptcp_addr_info *addr,
+			      bool reissue)
 {
 	struct mptcp_pm_add_entry *add_entry = NULL;
 	struct sock *sk = (struct sock *)msk;
@@ -369,7 +370,7 @@ bool mptcp_pm_alloc_anno_list(struct mptcp_sock *msk,
 	add_entry = mptcp_lookup_anno_list_by_saddr(msk, addr);
 
 	if (add_entry) {
-		if (WARN_ON_ONCE(mptcp_pm_is_kernel(msk)))
+		if (WARN_ON_ONCE(!reissue))
 			return false;
 
 		sk_reset_timer(sk, &add_entry->add_timer,
@@ -595,7 +596,7 @@ static void mptcp_pm_create_subflow_or_signal_addr(struct mptcp_sock *msk)
 		/* If the alloc fails, we are on memory pressure, not worth
 		 * continuing, and trying to create subflows.
 		 */
-		if (!mptcp_pm_alloc_anno_list(msk, &local.addr))
+		if (!mptcp_pm_alloc_anno_list(msk, &local.addr, false))
 			return;
 
 		__clear_bit(local.addr.id, msk->pm.id_avail_bitmap);
diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index 98fe8808d1e1..05d59ad1a0bc 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -232,7 +232,7 @@ int mptcp_pm_nl_announce_doit(struct sk_buff *skb, struct genl_info *info)
 	lock_sock(sk);
 	spin_lock_bh(&msk->pm.lock);
 
-	if (mptcp_pm_alloc_anno_list(msk, &addr_val.addr)) {
+	if (mptcp_pm_alloc_anno_list(msk, &addr_val.addr, true)) {
 		msk->pm.add_addr_signaled++;
 		mptcp_pm_announce_addr(msk, &addr_val.addr, false);
 		mptcp_pm_nl_addr_send_ack(msk);
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index ef1d43406f9b..0b6695fbb645 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -1023,7 +1023,8 @@ int mptcp_pm_nl_mp_prio_send_ack(struct mptcp_sock *msk,
 				 struct mptcp_addr_info *rem,
 				 u8 bkup);
 bool mptcp_pm_alloc_anno_list(struct mptcp_sock *msk,
-			      const struct mptcp_addr_info *addr);
+			      const struct mptcp_addr_info *addr,
+			      bool reissue);
 void mptcp_pm_free_anno_list(struct mptcp_sock *msk);
 bool mptcp_pm_sport_in_anno_list(struct mptcp_sock *msk, const struct sock *sk);
 struct mptcp_pm_add_entry *
-- 
2.43.0

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

[Geliang Tang]

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(-)

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;
 }
-- 
2.43.0

[PATCH mptcp-next v1 5/6] mptcp: pm: add remote parameter for set_flags

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

The remote address of set_flags() interface is useful for userspace PM,
but unused in in-kernel PM.

But an additional "changed" parameter needs to be passed to set_flags() of
in-kernel PM. One option is to add a "u8 changed" parameter to set_flags()
interface:

	set_flags(struct mptcp_pm_addr_entry *local,
		  struct mptcp_addr_info *remote,
		  u8 changed)

A better option is to add a struct mptcp_pm_addr_entry "remote" parameter
for set_flags(), so that "remote->addr" can be used for userspace PM, and
"remote->flags" can be used for in-kernel PM to replace the additional
"changed" parameter.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 net/mptcp/pm.c           | 20 ++++++++++++++++----
 net/mptcp/pm_netlink.c   |  1 +
 net/mptcp/pm_userspace.c | 21 +++------------------
 net/mptcp/protocol.h     |  2 ++
 4 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index 1844107f6f93..adb2735c4131 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -531,7 +531,8 @@ int mptcp_pm_nl_get_addr_dumpit(struct sk_buff *msg,
 static int mptcp_pm_set_flags(struct genl_info *info)
 {
 	struct mptcp_pm_addr_entry loc = { .addr = { .family = AF_UNSPEC }, };
-	struct nlattr *attr_loc;
+	struct mptcp_pm_addr_entry rem = { .addr = { .family = AF_UNSPEC }, };
+	struct nlattr *attr_loc, *attr_rem;
 	int ret = -EINVAL;
 
 	if (GENL_REQ_ATTR_CHECK(info, MPTCP_PM_ATTR_ADDR))
@@ -542,9 +543,20 @@ static int mptcp_pm_set_flags(struct genl_info *info)
 	if (ret < 0)
 		return ret;
 
-	if (info->attrs[MPTCP_PM_ATTR_TOKEN])
-		return mptcp_userspace_pm_set_flags(&loc, info);
-	return mptcp_pm_nl_set_flags(&loc, info);
+	if (info->attrs[MPTCP_PM_ATTR_TOKEN]) {
+		attr_rem = info->attrs[MPTCP_PM_ATTR_ADDR_REMOTE];
+		ret = mptcp_pm_parse_entry(attr_rem, info, false, &rem);
+		if (ret < 0)
+			return ret;
+
+		if (rem.addr.family == AF_UNSPEC) {
+			NL_SET_ERR_MSG_ATTR(info->extack, attr_rem,
+					    "invalid remote address family");
+			return -EINVAL;
+		}
+		return mptcp_userspace_pm_set_flags(&loc, &rem, info);
+	}
+	return mptcp_pm_nl_set_flags(&loc, &rem, info);
 }
 
 int mptcp_pm_nl_set_flags_doit(struct sk_buff *skb, struct genl_info *info)
diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index e546388070b4..053f2bec9042 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -1949,6 +1949,7 @@ static void mptcp_nl_set_flags(struct net *net,
 }
 
 int mptcp_pm_nl_set_flags(struct mptcp_pm_addr_entry *local,
+			  struct mptcp_pm_addr_entry *remote,
 			  struct genl_info *info)
 {
 	struct nlattr *attr = info->attrs[MPTCP_PM_ATTR_ADDR];
diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index 05d59ad1a0bc..dddd2daed650 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -553,19 +553,16 @@ int mptcp_pm_nl_subflow_destroy_doit(struct sk_buff *skb, struct genl_info *info
 }
 
 int mptcp_userspace_pm_set_flags(struct mptcp_pm_addr_entry *local,
+				 struct mptcp_pm_addr_entry *remote,
 				 struct genl_info *info)
 {
-	struct mptcp_addr_info rem = { .family = AF_UNSPEC, };
 	struct mptcp_pm_addr_entry *entry;
-	struct nlattr *attr, *attr_rem;
 	struct mptcp_sock *msk;
+	struct nlattr *attr;
 	int ret = -EINVAL;
 	struct sock *sk;
 	u8 bkup = 0;
 
-	if (GENL_REQ_ATTR_CHECK(info, MPTCP_PM_ATTR_ADDR_REMOTE))
-		return ret;
-
 	msk = mptcp_userspace_pm_get_sock(info);
 	if (!msk)
 		return ret;
@@ -580,18 +577,6 @@ int mptcp_userspace_pm_set_flags(struct mptcp_pm_addr_entry *local,
 		goto set_flags_err;
 	}
 
-	attr_rem = info->attrs[MPTCP_PM_ATTR_ADDR_REMOTE];
-	ret = mptcp_pm_parse_addr(attr_rem, info, &rem);
-	if (ret < 0)
-		goto set_flags_err;
-
-	if (rem.family == AF_UNSPEC) {
-		NL_SET_ERR_MSG_ATTR(info->extack, attr_rem,
-				    "invalid remote address family");
-		ret = -EINVAL;
-		goto set_flags_err;
-	}
-
 	if (local->flags & MPTCP_PM_ADDR_FLAG_BACKUP)
 		bkup = 1;
 
@@ -606,7 +591,7 @@ int mptcp_userspace_pm_set_flags(struct mptcp_pm_addr_entry *local,
 	spin_unlock_bh(&msk->pm.lock);
 
 	lock_sock(sk);
-	ret = mptcp_pm_nl_mp_prio_send_ack(msk, &local->addr, &rem, bkup);
+	ret = mptcp_pm_nl_mp_prio_send_ack(msk, &local->addr, &remote->addr, bkup);
 	release_sock(sk);
 
 	/* mptcp_pm_nl_mp_prio_send_ack() only fails in one case */
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 0b6695fbb645..5adb791898ec 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -1038,8 +1038,10 @@ bool mptcp_lookup_subflow_by_saddr(const struct list_head *list,
 bool mptcp_remove_anno_list_by_saddr(struct mptcp_sock *msk,
 				     const struct mptcp_addr_info *addr);
 int mptcp_pm_nl_set_flags(struct mptcp_pm_addr_entry *local,
+			  struct mptcp_pm_addr_entry *remote,
 			  struct genl_info *info);
 int mptcp_userspace_pm_set_flags(struct mptcp_pm_addr_entry *local,
+				 struct mptcp_pm_addr_entry *remote,
 				 struct genl_info *info);
 int mptcp_pm_announce_addr(struct mptcp_sock *msk,
 			   const struct mptcp_addr_info *addr,
-- 
2.43.0

[PATCH mptcp-next v1 6/6] mptcp: pm: in-kernel: drop changed parameter of set_flags

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

To drop the additional "changed" parameter of mptcp_nl_set_flags(),
store "entry->flags" to "remote->flags" before modifying it in
mptcp_pm_nl_set_flags(), so that "changed" value can be obtained by
comparing "local->flags" and "remote->flags" in mptcp_nl_set_flags().

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

diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index 053f2bec9042..2f22c3bf88e2 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -1916,13 +1916,16 @@ static void mptcp_pm_nl_fullmesh(struct mptcp_sock *msk,
 
 static void mptcp_nl_set_flags(struct net *net,
 			       struct mptcp_pm_addr_entry *local,
-			       u8 changed)
+			       struct mptcp_pm_addr_entry *remote)
 {
 	u8 is_subflow = !!(local->flags & MPTCP_PM_ADDR_FLAG_SUBFLOW);
 	u8 bkup = !!(local->flags & MPTCP_PM_ADDR_FLAG_BACKUP);
+	u8 changed, mask = MPTCP_PM_ADDR_FLAG_BACKUP |
+			   MPTCP_PM_ADDR_FLAG_FULLMESH;
 	long s_slot = 0, s_num = 0;
 	struct mptcp_sock *msk;
 
+	changed = (local->flags ^ remote->flags) & mask;
 	if (changed == MPTCP_PM_ADDR_FLAG_FULLMESH && !is_subflow)
 		return;
 
@@ -1987,12 +1990,13 @@ int mptcp_pm_nl_set_flags(struct mptcp_pm_addr_entry *local,
 		return -EINVAL;
 	}
 
+	remote->flags = entry->flags;
 	changed = (local->flags ^ entry->flags) & mask;
 	entry->flags = (entry->flags & ~mask) | (local->flags & mask);
 	*local = *entry;
 	spin_unlock_bh(&pernet->lock);
 
-	mptcp_nl_set_flags(net, local, changed);
+	mptcp_nl_set_flags(net, local, remote);
 	return 0;
 }
 
-- 
2.43.0

Re: [PATCH mptcp-next v1 0/6] BPF path manager, part 5

[MPTCP CI]

Hi Geliang,

Thank you for your modifications, that's great!

But sadly, our CI spotted some issues with it when trying to build it.

You can find more details there:

  https://github.com/multipath-tcp/mptcp_net-next/actions/runs/13560646576

Status: failure
Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/2fc0cce80faf
Patchwork: https://patchwork.kernel.org/project/mptcp/list/?series=938355

Feel free to reply to this email if you cannot access logs, if you need
some support to fix the error, if this doesn't seem to be caused by your
modifications or if the error is a false positive one.

Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (NGI0 Core)

Re: [PATCH mptcp-next v1 0/6] BPF path manager, part 5

[MPTCP CI]

Hi Geliang,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal: Success! ✅
- KVM Validation: debug: Success! ✅
- KVM Validation: btf-normal (only bpftest_all): Success! ✅
- KVM Validation: btf-debug (only bpftest_all): Success! ✅
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/13560646466

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/2fc0cce80faf
Patchwork: https://patchwork.kernel.org/project/mptcp/list/?series=938355


If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:

    $ cd [kernel source code]
    $ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
        --pull always mptcp/mptcp-upstream-virtme-docker:latest \
        auto-normal

For more details:

    https://github.com/multipath-tcp/mptcp-upstream-virtme-docker


Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)

Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (NGI0 Core)

Re: [PATCH mptcp-next v1 2/6] mptcp: pm: userspace: drop is_userspace in free_local_addr_list

[Matthieu Baerts]

Hi Geliang,

On 27/02/2025 07:43, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> To reduce the path manager's reliance on mptcp_pm_is_userspace()
> and mptcp_pm_is_kernel() helpers, this patch drops the check for
> mptcp_pm_is_userspace() in mptcp_free_local_addr_list() and
> replaces it with a check to see if userspace_pm_local_addr_list
> is empty.

The existing design feels wrong: I see that mptcp_destroy_common()
directly calls specific PM code.

I think in general, MPTCP core should only call code from pm.c, then
redirected to the right PM (later using the 'pm->ops').

For the moment, I see mptcp_destroy_common() from protocol.c is calling:

 - mptcp_pm_free_anno_list(msk);
 - mptcp_free_local_addr_list(msk);

Instead, I suggest mptcp_destroy_common() calling mptcp_pm_destroy(),
which will always call mptcp_pm_free_anno_list() and only call
mptcp_free_local_addr_list() for the userspace pm.

Later on, mptcp_free_local_addr_list() could be called via
'pm->ops->destroy' I suppose, no?

BTW, mptcp_pm_free_anno_list() seems to be used by all PMs, right? I
think it would be better to move this code to pm.c. Same for all
"common" code. I can look at that if you prefer. But maybe you prefer to
do this after your changed or is that OK to rebase after?

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

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

[Matthieu Baerts]

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
-- 
Sponsored by the NGI0 Core fund.

Re: [PATCH mptcp-next v1 5/6] mptcp: pm: add remote parameter for set_flags

[Matthieu Baerts]

Hi Geliang,

On 27/02/2025 07:43, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> The remote address of set_flags() interface is useful for userspace PM,
> but unused in in-kernel PM.
> 
> But an additional "changed" parameter needs to be passed to set_flags() of
> in-kernel PM. One option is to add a "u8 changed" parameter to set_flags()
> interface:
> 
> 	set_flags(struct mptcp_pm_addr_entry *local,
> 		  struct mptcp_addr_info *remote,
> 		  u8 changed)
> 
> A better option is to add a struct mptcp_pm_addr_entry "remote" parameter
> for set_flags(), so that "remote->addr" can be used for userspace PM, and
> "remote->flags" can be used for in-kernel PM to replace the additional
> "changed" parameter.

I don't think I understand why we need this modification.

The 'set_flags' netlink command is there to change the behaviour of the
in-kernel PM and the userspace PM. It will certainly not be useful for a
BPF PM because all its configurations will be done via BPF, not via
Netlink. If the userspace send this Netlink command when a BPF PM will
be used, nothing will happen:
- if a token is specified, the userspace PM should not do anything if it
is not the designated PM for this connection.
- if not, the in-kernel PM will only modify the connections handled by
the in-kernel PM.

The in-kernel and userspace PMs use the same command, but for different
things, hence requiring different parameters: e.g. the userspace one
requires the token because the modification will be only for one
connection, while the in-kernel will be for a specific endpoint.

So the current code looks OK: in the common part (pm.c), only the common
attributes are parsed. On the userspace side, extra parameter will be
parsed, that's fine. On the in-kernel side, 'changed' will be looked at
simply because there is no need to iterate over all the connections if
nothing has changed on the corresponding endpoint side. In other words,
I don't think the code should be changed, no?

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

Re: [PATCH mptcp-next v1 6/6] mptcp: pm: in-kernel: drop changed parameter of set_flags

[Matthieu Baerts]

Hi Geliang,

On 27/02/2025 07:43, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> To drop the additional "changed" parameter of mptcp_nl_set_flags(),
> store "entry->flags" to "remote->flags" before modifying it in
> mptcp_pm_nl_set_flags(), so that "changed" value can be obtained by
> comparing "local->flags" and "remote->flags" in mptcp_nl_set_flags().

Here as well, no need to change the code: it doesn't make sense to deal
with 'remote' as a workaround to have the same interface that is not
needed from what I understood. No?

> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  net/mptcp/pm_netlink.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index 053f2bec9042..2f22c3bf88e2 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c
> @@ -1916,13 +1916,16 @@ static void mptcp_pm_nl_fullmesh(struct mptcp_sock *msk,
>  
>  static void mptcp_nl_set_flags(struct net *net,
>  			       struct mptcp_pm_addr_entry *local,
> -			       u8 changed)
> +			       struct mptcp_pm_addr_entry *remote)
>  {
>  	u8 is_subflow = !!(local->flags & MPTCP_PM_ADDR_FLAG_SUBFLOW);
>  	u8 bkup = !!(local->flags & MPTCP_PM_ADDR_FLAG_BACKUP);
> +	u8 changed, mask = MPTCP_PM_ADDR_FLAG_BACKUP |
> +			   MPTCP_PM_ADDR_FLAG_FULLMESH;
>  	long s_slot = 0, s_num = 0;
>  	struct mptcp_sock *msk;
>  
> +	changed = (local->flags ^ remote->flags) & mask;
>  	if (changed == MPTCP_PM_ADDR_FLAG_FULLMESH && !is_subflow)
>  		return;
>  
> @@ -1987,12 +1990,13 @@ int mptcp_pm_nl_set_flags(struct mptcp_pm_addr_entry *local,
>  		return -EINVAL;
>  	}
>  
> +	remote->flags = entry->flags;

(I guess you wanted to use "remote->flags = changed" or something like
that, but this code should not be modified I think, it doesn't make
sense to deal with a "remote" here)

>  	changed = (local->flags ^ entry->flags) & mask;
>  	entry->flags = (entry->flags & ~mask) | (local->flags & mask);
>  	*local = *entry;
>  	spin_unlock_bh(&pernet->lock);
>  
> -	mptcp_nl_set_flags(net, local, changed);
> +	mptcp_nl_set_flags(net, local, remote);
>  	return 0;
>  }
>  
Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.

Re: [PATCH mptcp-next v1 6/6] mptcp: pm: in-kernel: drop changed parameter of set_flags

[kernel test robot]

Hi Geliang,

kernel test robot noticed the following build warnings:

[auto build test WARNING on mptcp/export]
[cannot apply to mptcp/export-net linus/master v6.14-rc4 next-20250227]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Geliang-Tang/mptcp-pm-use-pm-variable-instead-of-msk-pm/20250227-144528
base:   https://github.com/multipath-tcp/mptcp_net-next.git export
patch link:    https://lore.kernel.org/r/1e2f2a9bcdf69bd241dec4c5455c4aa135927cb6.1740638334.git.tanggeliang%40kylinos.cn
patch subject: [PATCH mptcp-next v1 6/6] mptcp: pm: in-kernel: drop changed parameter of set_flags
config: riscv-randconfig-002-20250228 (https://download.01.org/0day-ci/archive/20250228/202502281544.ZIQZO9cy-lkp@intel.com/config)
compiler: riscv32-linux-gcc (GCC) 14.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250228/202502281544.ZIQZO9cy-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202502281544.ZIQZO9cy-lkp@intel.com/

All warnings (new ones prefixed by >>):

   net/mptcp/pm_netlink.c: In function 'mptcp_pm_nl_set_flags':
>> net/mptcp/pm_netlink.c:1959:12: warning: variable 'changed' set but not used [-Wunused-but-set-variable]
    1959 |         u8 changed, mask = MPTCP_PM_ADDR_FLAG_BACKUP |
         |            ^~~~~~~


vim +/changed +1959 net/mptcp/pm_netlink.c

0f9f696a502e1b0 Geliang Tang           2021-01-08  1953  
c7f25f7987c060b Geliang Tang           2025-02-07  1954  int mptcp_pm_nl_set_flags(struct mptcp_pm_addr_entry *local,
de258815006a162 Geliang Tang           2025-02-27  1955  			  struct mptcp_pm_addr_entry *remote,
c7f25f7987c060b Geliang Tang           2025-02-07  1956  			  struct genl_info *info)
6ba7ce89905c5d5 Geliang Tang           2023-06-08  1957  {
c7f25f7987c060b Geliang Tang           2025-02-07  1958  	struct nlattr *attr = info->attrs[MPTCP_PM_ATTR_ADDR];
6ba7ce89905c5d5 Geliang Tang           2023-06-08 @1959  	u8 changed, mask = MPTCP_PM_ADDR_FLAG_BACKUP |
6ba7ce89905c5d5 Geliang Tang           2023-06-08  1960  			   MPTCP_PM_ADDR_FLAG_FULLMESH;
2c8971c04f745de Geliang Tang           2025-02-07  1961  	struct net *net = genl_info_net(info);
6ba7ce89905c5d5 Geliang Tang           2023-06-08  1962  	struct mptcp_pm_addr_entry *entry;
6a42477fe4491e4 Geliang Tang           2024-03-05  1963  	struct pm_nl_pernet *pernet;
6ba7ce89905c5d5 Geliang Tang           2023-06-08  1964  	u8 lookup_by_id = 0;
8cdc56f99e6c33a Matthieu Baerts (NGI0  2025-02-07  1965) 
6a42477fe4491e4 Geliang Tang           2024-03-05  1966  	pernet = pm_nl_get_pernet(net);
6ba7ce89905c5d5 Geliang Tang           2023-06-08  1967  
c7f25f7987c060b Geliang Tang           2025-02-07  1968  	if (local->addr.family == AF_UNSPEC) {
6ba7ce89905c5d5 Geliang Tang           2023-06-08  1969  		lookup_by_id = 1;
c7f25f7987c060b Geliang Tang           2025-02-07  1970  		if (!local->addr.id) {
a25a8b10491bb94 Matthieu Baerts (NGI0  2025-02-07  1971) 			NL_SET_ERR_MSG_ATTR(info->extack, attr,
a25a8b10491bb94 Matthieu Baerts (NGI0  2025-02-07  1972) 					    "missing address ID");
6ba7ce89905c5d5 Geliang Tang           2023-06-08  1973  			return -EOPNOTSUPP;
6ba7ce89905c5d5 Geliang Tang           2023-06-08  1974  		}
a4d68b160240815 Geliang Tang           2024-03-05  1975  	}
6ba7ce89905c5d5 Geliang Tang           2023-06-08  1976  
6ba7ce89905c5d5 Geliang Tang           2023-06-08  1977  	spin_lock_bh(&pernet->lock);
c7f25f7987c060b Geliang Tang           2025-02-07  1978  	entry = lookup_by_id ? __lookup_addr_by_id(pernet, local->addr.id) :
c7f25f7987c060b Geliang Tang           2025-02-07  1979  			       __lookup_addr(pernet, &local->addr);
6ba7ce89905c5d5 Geliang Tang           2023-06-08  1980  	if (!entry) {
6ba7ce89905c5d5 Geliang Tang           2023-06-08  1981  		spin_unlock_bh(&pernet->lock);
a25a8b10491bb94 Matthieu Baerts (NGI0  2025-02-07  1982) 		NL_SET_ERR_MSG_ATTR(info->extack, attr, "address not found");
6ba7ce89905c5d5 Geliang Tang           2023-06-08  1983  		return -EINVAL;
6ba7ce89905c5d5 Geliang Tang           2023-06-08  1984  	}
c7f25f7987c060b Geliang Tang           2025-02-07  1985  	if ((local->flags & MPTCP_PM_ADDR_FLAG_FULLMESH) &&
1bb0d1348546ad0 Matthieu Baerts (NGI0  2025-01-23  1986) 	    (entry->flags & (MPTCP_PM_ADDR_FLAG_SIGNAL |
1bb0d1348546ad0 Matthieu Baerts (NGI0  2025-01-23  1987) 			     MPTCP_PM_ADDR_FLAG_IMPLICIT))) {
6ba7ce89905c5d5 Geliang Tang           2023-06-08  1988  		spin_unlock_bh(&pernet->lock);
a25a8b10491bb94 Matthieu Baerts (NGI0  2025-02-07  1989) 		NL_SET_ERR_MSG_ATTR(info->extack, attr, "invalid addr flags");
6ba7ce89905c5d5 Geliang Tang           2023-06-08  1990  		return -EINVAL;
6ba7ce89905c5d5 Geliang Tang           2023-06-08  1991  	}
6ba7ce89905c5d5 Geliang Tang           2023-06-08  1992  
c029cf028bb79d2 Geliang Tang           2025-02-27  1993  	remote->flags = entry->flags;
c7f25f7987c060b Geliang Tang           2025-02-07  1994  	changed = (local->flags ^ entry->flags) & mask;
c7f25f7987c060b Geliang Tang           2025-02-07  1995  	entry->flags = (entry->flags & ~mask) | (local->flags & mask);
c7f25f7987c060b Geliang Tang           2025-02-07  1996  	*local = *entry;
6ba7ce89905c5d5 Geliang Tang           2023-06-08  1997  	spin_unlock_bh(&pernet->lock);
6ba7ce89905c5d5 Geliang Tang           2023-06-08  1998  
c029cf028bb79d2 Geliang Tang           2025-02-27  1999  	mptcp_nl_set_flags(net, local, remote);
6ba7ce89905c5d5 Geliang Tang           2023-06-08  2000  	return 0;
6ba7ce89905c5d5 Geliang Tang           2023-06-08  2001  }
6ba7ce89905c5d5 Geliang Tang           2023-06-08  2002  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki