[PATCH mptcp-next v4 0/9] BPF path manager, part 6

[PATCH mptcp-next v4 0/9] BPF path manager, part 6

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

v4:
 - address Matt's comments in v3.
 - update pm locks in mptcp_pm_worker.
 - move the lock inside mptcp_pm_create_subflow_or_signal_addr.
 - move the lock inside mptcp_pm_nl_add_addr_received.
 - invoke add_addr_received interface from mptcp_pm_worker.
 - invoke rm_addr_received interface from mptcp_pm_rm_addr_or_subflow.
 - simply call mptcp_pm_close_subflow() in mptcp_pm_subflow_check_next.

v3:
 - merge 'bugfixes for "BPF path manager, part 6, v2"' into this set.
 - https://patchwork.kernel.org/project/mptcp/cover/cover.1742521397.git.tanggeliang@kylinos.cn/

v2:
 - address Matt's comments in v1.
 - add add_addr_received and rm_addr_received interfaces.
 - drop subflow_check_next interface.
 - add a "required" or "optional" comment for a group of interfaces in
   struct mptcp_pm_ops.

v1:
- https://patchwork.kernel.org/project/mptcp/cover/cover.1741685260.git.tanggeliang@kylinos.cn/

New interfaces for struct mptcp_pm_ops.

Geliang Tang (9):
  Squash to "mptcp: pm: add get_local_id() interface"
  mptcp: pm: add established interfaces
  mptcp: pm: drop is_userspace in subflow_check_next
  mptcp: pm: drop redundant MPTCP_MIB_ADDADDRDROP
  mptcp: pm: add add_addr_received() interface
  mptcp: pm: add rm_addr_received() interface
  mptcp: pm: add add_addr_echo() interface
  mptcp: pm: add accept_new_subflow() interface
  mptcp: pm: add allow_new_subflow() interface

 include/net/mptcp.h      |  15 ++++++
 net/mptcp/pm.c           | 114 ++++++++++++++++++---------------------
 net/mptcp/pm_kernel.c    |  91 +++++++++++++++++++++----------
 net/mptcp/pm_userspace.c |  13 +++++
 net/mptcp/protocol.h     |   3 +-
 net/mptcp/subflow.c      |   4 +-
 6 files changed, 148 insertions(+), 92 deletions(-)

-- 
2.43.0

[PATCH mptcp-next v4 1/9] Squash to "mptcp: pm: add get_local_id() interface"

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

Add /* required */ comment for get_local_id and get_priority.

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

diff --git a/include/net/mptcp.h b/include/net/mptcp.h
index 6a08ac862bbe..9f28ef550e10 100644
--- a/include/net/mptcp.h
+++ b/include/net/mptcp.h
@@ -118,6 +118,7 @@ struct mptcp_sched_ops {
 #define MPTCP_PM_BUF_MAX	(MPTCP_PM_NAME_MAX * MPTCP_PM_MAX)
 
 struct mptcp_pm_ops {
+	/* required */
 	int (*get_local_id)(struct mptcp_sock *msk,
 			    struct mptcp_pm_addr_entry *skc);
 	bool (*get_priority)(struct mptcp_sock *msk,
-- 
2.43.0

[PATCH mptcp-next v4 2/9] mptcp: pm: add established interfaces

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

This patch adds .established and .subflow_established interfaces for
struct mptcp_pm_ops, and calls pm->ops->established/subflow_established
in from mptcp_pm_worker(). Then get rid of the corresponding code from
__mptcp_pm_kernel_worker().

Since mptcp_pm_addr_send_ack() is a sleepable kfunc, which is invoked
by mptcp_pm_create_subflow_or_signal_addr(), .established() and
.subflow_established() interfaces of BPF PM should be invoked by
__bpf_prog_enter_sleepable(), which can't be invoked under a lock.
This patch unlocks the pm lock before invoking this interface in
mptcp_pm_worker(), while holding this lock in mptcp_pm_kernel_established()
and mptcp_pm_kernel_subflow_established().

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 include/net/mptcp.h   |  4 ++++
 net/mptcp/pm.c        | 32 ++++++++++++++++++++++++--------
 net/mptcp/pm_kernel.c | 25 +++++++++++--------------
 3 files changed, 39 insertions(+), 22 deletions(-)

diff --git a/include/net/mptcp.h b/include/net/mptcp.h
index 9f28ef550e10..4ac936e4ce0d 100644
--- a/include/net/mptcp.h
+++ b/include/net/mptcp.h
@@ -124,6 +124,10 @@ struct mptcp_pm_ops {
 	bool (*get_priority)(struct mptcp_sock *msk,
 			     struct mptcp_addr_info *skc);
 
+	/* optional */
+	void (*established)(struct mptcp_sock *msk);
+	void (*subflow_established)(struct mptcp_sock *msk);
+
 	char			name[MPTCP_PM_NAME_MAX];
 	struct module		*owner;
 	struct list_head	list;
diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index ba7424582ebf..e2b2c874a9f8 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -516,7 +516,8 @@ 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) &&
-	    !(pm->status & BIT(MPTCP_PM_ALREADY_ESTABLISHED)))
+	    !(pm->status & BIT(MPTCP_PM_ALREADY_ESTABLISHED)) &&
+	    pm->ops->established)
 		mptcp_pm_schedule_work(msk, MPTCP_PM_ESTABLISHED);
 
 	if ((pm->status & BIT(MPTCP_PM_ALREADY_ESTABLISHED)) == 0)
@@ -543,7 +544,7 @@ void mptcp_pm_subflow_established(struct mptcp_sock *msk)
 
 	pr_debug("msk=%p\n", msk);
 
-	if (!READ_ONCE(pm->work_pending))
+	if (!READ_ONCE(pm->work_pending) || !pm->ops->subflow_established)
 		return;
 
 	spin_lock_bh(&pm->lock);
@@ -570,7 +571,8 @@ void mptcp_pm_subflow_check_next(struct mptcp_sock *msk,
 		return;
 	}
 
-	if (!READ_ONCE(pm->work_pending) && !update_subflows)
+	if (!pm->ops->subflow_established ||
+	    (!READ_ONCE(pm->work_pending) && !update_subflows))
 		return;
 
 	spin_lock_bh(&pm->lock);
@@ -628,7 +630,7 @@ void mptcp_pm_add_addr_echoed(struct mptcp_sock *msk,
 
 	pr_debug("msk=%p\n", msk);
 
-	if (!READ_ONCE(pm->work_pending))
+	if (!READ_ONCE(pm->work_pending) || !pm->ops->subflow_established)
 		return;
 
 	spin_lock_bh(&pm->lock);
@@ -949,20 +951,34 @@ void mptcp_pm_worker(struct mptcp_sock *msk)
 	if (!(pm->status & MPTCP_PM_WORK_MASK))
 		return;
 
-	spin_lock_bh(&msk->pm.lock);
-
 	pr_debug("msk=%p status=%x\n", msk, pm->status);
 	if (pm->status & BIT(MPTCP_PM_ADD_ADDR_SEND_ACK)) {
+		spin_lock_bh(&pm->lock);
 		pm->status &= ~BIT(MPTCP_PM_ADD_ADDR_SEND_ACK);
 		mptcp_pm_addr_send_ack(msk);
+		spin_unlock_bh(&pm->lock);
 	}
 	if (pm->status & BIT(MPTCP_PM_RM_ADDR_RECEIVED)) {
+		spin_lock_bh(&pm->lock);
 		pm->status &= ~BIT(MPTCP_PM_RM_ADDR_RECEIVED);
 		mptcp_pm_rm_addr_recv(msk);
+		spin_unlock_bh(&pm->lock);
+	}
+	if (pm->status & BIT(MPTCP_PM_ESTABLISHED)) {
+		spin_lock_bh(&pm->lock);
+		pm->status &= ~BIT(MPTCP_PM_ESTABLISHED);
+		spin_unlock_bh(&pm->lock);
+		pm->ops->established(msk);
+	}
+	if (pm->status & BIT(MPTCP_PM_SUBFLOW_ESTABLISHED)) {
+		spin_lock_bh(&pm->lock);
+		pm->status &= ~BIT(MPTCP_PM_SUBFLOW_ESTABLISHED);
+		spin_unlock_bh(&pm->lock);
+		pm->ops->subflow_established(msk);
 	}
+	spin_lock_bh(&pm->lock);
 	__mptcp_pm_kernel_worker(msk);
-
-	spin_unlock_bh(&msk->pm.lock);
+	spin_unlock_bh(&pm->lock);
 }
 
 static void mptcp_pm_ops_init(struct mptcp_sock *msk,
diff --git a/net/mptcp/pm_kernel.c b/net/mptcp/pm_kernel.c
index 7ec81d5195d4..2e181224bccb 100644
--- a/net/mptcp/pm_kernel.c
+++ b/net/mptcp/pm_kernel.c
@@ -269,6 +269,7 @@ static void mptcp_pm_create_subflow_or_signal_addr(struct mptcp_sock *msk)
 	local_addr_max = mptcp_pm_get_local_addr_max(msk);
 	subflows_max = mptcp_pm_get_subflows_max(msk);
 
+	spin_lock_bh(&msk->pm.lock);
 	/* do lazy endpoint usage accounting for the MPC subflows */
 	if (unlikely(!(msk->pm.status & BIT(MPTCP_PM_MPC_ENDPOINT_ACCOUNTED))) && msk->first) {
 		struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(msk->first);
@@ -307,7 +308,7 @@ static void mptcp_pm_create_subflow_or_signal_addr(struct mptcp_sock *msk)
 		 * current address announce will be completed.
 		 */
 		if (msk->pm.addr_signal & BIT(MPTCP_ADD_ADDR_SIGNAL))
-			return;
+			goto out;
 
 		if (!select_signal_address(pernet, msk, &local))
 			goto subflow;
@@ -316,7 +317,7 @@ static void mptcp_pm_create_subflow_or_signal_addr(struct mptcp_sock *msk)
 		 * continuing, and trying to create subflows.
 		 */
 		if (!mptcp_pm_alloc_anno_list(msk, &local.addr))
-			return;
+			goto out;
 
 		__clear_bit(local.addr.id, msk->pm.id_avail_bitmap);
 		msk->pm.add_addr_signaled++;
@@ -365,14 +366,16 @@ static void mptcp_pm_create_subflow_or_signal_addr(struct mptcp_sock *msk)
 		spin_lock_bh(&msk->pm.lock);
 	}
 	mptcp_pm_nl_check_work_pending(msk);
+out:
+	spin_unlock_bh(&msk->pm.lock);
 }
 
-static void mptcp_pm_nl_fully_established(struct mptcp_sock *msk)
+static void mptcp_pm_kernel_established(struct mptcp_sock *msk)
 {
 	mptcp_pm_create_subflow_or_signal_addr(msk);
 }
 
-static void mptcp_pm_nl_subflow_established(struct mptcp_sock *msk)
+static void mptcp_pm_kernel_subflow_established(struct mptcp_sock *msk)
 {
 	mptcp_pm_create_subflow_or_signal_addr(msk);
 }
@@ -758,8 +761,8 @@ static int mptcp_nl_add_subflow_or_signal_addr(struct net *net,
 		spin_lock_bh(&msk->pm.lock);
 		if (mptcp_addresses_equal(addr, &mpc_addr, addr->port))
 			msk->mpc_endpoint_id = addr->id;
-		mptcp_pm_create_subflow_or_signal_addr(msk);
 		spin_unlock_bh(&msk->pm.lock);
+		mptcp_pm_create_subflow_or_signal_addr(msk);
 		release_sock(sk);
 
 next:
@@ -1243,8 +1246,8 @@ static void mptcp_pm_nl_fullmesh(struct mptcp_sock *msk,
 	spin_lock_bh(&msk->pm.lock);
 	mptcp_pm_rm_subflow(msk, &list);
 	__mark_subflow_endp_available(msk, list.ids[0]);
-	mptcp_pm_create_subflow_or_signal_addr(msk);
 	spin_unlock_bh(&msk->pm.lock);
+	mptcp_pm_create_subflow_or_signal_addr(msk);
 }
 
 static void mptcp_pm_nl_set_flags_all(struct net *net,
@@ -1348,14 +1351,6 @@ void __mptcp_pm_kernel_worker(struct mptcp_sock *msk)
 		pm->status &= ~BIT(MPTCP_PM_ADD_ADDR_RECEIVED);
 		mptcp_pm_nl_add_addr_received(msk);
 	}
-	if (pm->status & BIT(MPTCP_PM_ESTABLISHED)) {
-		pm->status &= ~BIT(MPTCP_PM_ESTABLISHED);
-		mptcp_pm_nl_fully_established(msk);
-	}
-	if (pm->status & BIT(MPTCP_PM_SUBFLOW_ESTABLISHED)) {
-		pm->status &= ~BIT(MPTCP_PM_SUBFLOW_ESTABLISHED);
-		mptcp_pm_nl_subflow_established(msk);
-	}
 }
 
 static int __net_init pm_nl_init_net(struct net *net)
@@ -1422,6 +1417,8 @@ static void mptcp_pm_kernel_init(struct mptcp_sock *msk)
 struct mptcp_pm_ops mptcp_pm_kernel = {
 	.get_local_id		= mptcp_pm_kernel_get_local_id,
 	.get_priority		= mptcp_pm_kernel_get_priority,
+	.established		= mptcp_pm_kernel_established,
+	.subflow_established	= mptcp_pm_kernel_subflow_established,
 	.init			= mptcp_pm_kernel_init,
 	.name			= "kernel",
 	.owner			= THIS_MODULE,
-- 
2.43.0

[PATCH mptcp-next v4 3/9] mptcp: pm: drop is_userspace in subflow_check_next

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

This patch moves mptcp_pm_close_subflow() forward to let it be used by both
the userspace PM and the in-kernel PM in mptcp_pm_subflow_check_next().Then
mptcp_pm_is_userspace() here can be dropped.

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

diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index e2b2c874a9f8..906c558aef0b 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -562,22 +562,14 @@ void mptcp_pm_subflow_check_next(struct mptcp_sock *msk,
 	bool update_subflows;
 
 	update_subflows = subflow->request_join || subflow->mp_join;
-	if (mptcp_pm_is_userspace(msk)) {
-		if (update_subflows) {
-			spin_lock_bh(&pm->lock);
-			pm->subflows--;
-			spin_unlock_bh(&pm->lock);
-		}
-		return;
-	}
+	if (update_subflows)
+		mptcp_pm_close_subflow(msk);
 
 	if (!pm->ops->subflow_established ||
 	    (!READ_ONCE(pm->work_pending) && !update_subflows))
 		return;
 
 	spin_lock_bh(&pm->lock);
-	if (update_subflows)
-		__mptcp_pm_close_subflow(msk);
 
 	/* Even if this subflow is not really established, tell the PM to try
 	 * to pick the next ones, if possible.
-- 
2.43.0

[PATCH mptcp-next v4 4/9] mptcp: pm: drop redundant MPTCP_MIB_ADDADDRDROP

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

MPTCP_MIB_ADDADDRDROP MIB counter is incremented from both the in-kernel PM
and the userspace PM. This can be called only once to reduce redundant
code.

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

diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index 906c558aef0b..8efb47331f79 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -586,6 +586,7 @@ void mptcp_pm_add_addr_received(const struct sock *ssk,
 	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk);
 	struct mptcp_sock *msk = mptcp_sk(subflow->conn);
 	struct mptcp_pm_data *pm = &msk->pm;
+	int ret = 0;
 
 	pr_debug("msk=%p remote_id=%d accept=%d\n", msk, addr->id,
 		 READ_ONCE(pm->accept_addr));
@@ -599,7 +600,7 @@ void mptcp_pm_add_addr_received(const struct sock *ssk,
 			mptcp_pm_announce_addr(msk, addr, true);
 			mptcp_pm_add_addr_send_ack(msk);
 		} else {
-			__MPTCP_INC_STATS(sock_net((struct sock *)msk), MPTCP_MIB_ADDADDRDROP);
+			ret = -EINVAL;
 		}
 	/* id0 should not have a different address */
 	} else if ((addr->id == 0 && !mptcp_pm_is_init_remote_addr(msk, addr)) ||
@@ -609,9 +610,12 @@ void mptcp_pm_add_addr_received(const struct sock *ssk,
 	} else if (mptcp_pm_schedule_work(msk, MPTCP_PM_ADD_ADDR_RECEIVED)) {
 		pm->remote = *addr;
 	} else {
-		__MPTCP_INC_STATS(sock_net((struct sock *)msk), MPTCP_MIB_ADDADDRDROP);
+		ret = -EINVAL;
 	}
 
+	if (ret)
+		__MPTCP_INC_STATS(sock_net((struct sock *)msk), MPTCP_MIB_ADDADDRDROP);
+
 	spin_unlock_bh(&pm->lock);
 }
 
-- 
2.43.0

[PATCH mptcp-next v4 5/9] mptcp: pm: add add_addr_received() interface

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

This patch adds an optional .add_addr_received interface for struct
mptcp_pm_ops and invokes it in mptcp_pm_worker().

This interface is only implemented in the in-kernel PM as a wrapper
of mptcp_pm_nl_add_addr_received().

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 include/net/mptcp.h   |  1 +
 net/mptcp/pm.c        | 18 +++++++++++-------
 net/mptcp/pm_kernel.c | 24 +++++++++++-------------
 net/mptcp/protocol.h  |  1 -
 4 files changed, 23 insertions(+), 21 deletions(-)

diff --git a/include/net/mptcp.h b/include/net/mptcp.h
index 4ac936e4ce0d..5118d11d2ee9 100644
--- a/include/net/mptcp.h
+++ b/include/net/mptcp.h
@@ -127,6 +127,7 @@ struct mptcp_pm_ops {
 	/* optional */
 	void (*established)(struct mptcp_sock *msk);
 	void (*subflow_established)(struct mptcp_sock *msk);
+	void (*add_addr_received)(struct mptcp_sock *msk);
 
 	char			name[MPTCP_PM_NAME_MAX];
 	struct module		*owner;
diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index 8efb47331f79..71589cd5dee7 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -607,10 +607,11 @@ void mptcp_pm_add_addr_received(const struct sock *ssk,
 		   (addr->id > 0 && !READ_ONCE(pm->accept_addr))) {
 		mptcp_pm_announce_addr(msk, addr, true);
 		mptcp_pm_add_addr_send_ack(msk);
-	} else if (mptcp_pm_schedule_work(msk, MPTCP_PM_ADD_ADDR_RECEIVED)) {
-		pm->remote = *addr;
-	} else {
-		ret = -EINVAL;
+	} else if (pm->ops->add_addr_received) {
+		if (mptcp_pm_schedule_work(msk, MPTCP_PM_ADD_ADDR_RECEIVED))
+			pm->remote = *addr;
+		else
+			ret = -EINVAL;
 	}
 
 	if (ret)
@@ -948,6 +949,12 @@ void mptcp_pm_worker(struct mptcp_sock *msk)
 		return;
 
 	pr_debug("msk=%p status=%x\n", msk, pm->status);
+	if (pm->status & BIT(MPTCP_PM_ADD_ADDR_RECEIVED)) {
+		spin_lock_bh(&pm->lock);
+		pm->status &= ~BIT(MPTCP_PM_ADD_ADDR_RECEIVED);
+		spin_unlock_bh(&pm->lock);
+		pm->ops->add_addr_received(msk);
+	}
 	if (pm->status & BIT(MPTCP_PM_ADD_ADDR_SEND_ACK)) {
 		spin_lock_bh(&pm->lock);
 		pm->status &= ~BIT(MPTCP_PM_ADD_ADDR_SEND_ACK);
@@ -972,9 +979,6 @@ void mptcp_pm_worker(struct mptcp_sock *msk)
 		spin_unlock_bh(&pm->lock);
 		pm->ops->subflow_established(msk);
 	}
-	spin_lock_bh(&pm->lock);
-	__mptcp_pm_kernel_worker(msk);
-	spin_unlock_bh(&pm->lock);
 }
 
 static void mptcp_pm_ops_init(struct mptcp_sock *msk,
diff --git a/net/mptcp/pm_kernel.c b/net/mptcp/pm_kernel.c
index 2e181224bccb..4f4791620072 100644
--- a/net/mptcp/pm_kernel.c
+++ b/net/mptcp/pm_kernel.c
@@ -461,12 +461,13 @@ static void mptcp_pm_nl_add_addr_received(struct mptcp_sock *msk)
 		 msk->pm.add_addr_accepted, add_addr_accept_max,
 		 msk->pm.remote.family);
 
+	spin_lock_bh(&msk->pm.lock);
 	remote = msk->pm.remote;
 	mptcp_pm_announce_addr(msk, &remote, true);
 	mptcp_pm_addr_send_ack(msk);
 
 	if (lookup_subflow_by_daddr(&msk->conn_list, &remote))
-		return;
+		goto out;
 
 	/* pick id 0 port, if none is provided the remote address */
 	if (!remote.port)
@@ -477,7 +478,7 @@ static void mptcp_pm_nl_add_addr_received(struct mptcp_sock *msk)
 	 */
 	nr = fill_local_addresses_vec(msk, &remote, locals);
 	if (nr == 0)
-		return;
+		goto out;
 
 	spin_unlock_bh(&msk->pm.lock);
 	for (i = 0; i < nr; i++)
@@ -493,6 +494,8 @@ static void mptcp_pm_nl_add_addr_received(struct mptcp_sock *msk)
 		    msk->pm.subflows >= subflows_max)
 			WRITE_ONCE(msk->pm.accept_addr, false);
 	}
+out:
+	spin_unlock_bh(&msk->pm.lock);
 }
 
 void mptcp_pm_nl_rm_addr(struct mptcp_sock *msk, u8 rm_id)
@@ -1342,17 +1345,6 @@ bool mptcp_pm_nl_check_work_pending(struct mptcp_sock *msk)
 	return true;
 }
 
-/* Called under PM lock */
-void __mptcp_pm_kernel_worker(struct mptcp_sock *msk)
-{
-	struct mptcp_pm_data *pm = &msk->pm;
-
-	if (pm->status & BIT(MPTCP_PM_ADD_ADDR_RECEIVED)) {
-		pm->status &= ~BIT(MPTCP_PM_ADD_ADDR_RECEIVED);
-		mptcp_pm_nl_add_addr_received(msk);
-	}
-}
-
 static int __net_init pm_nl_init_net(struct net *net)
 {
 	struct pm_nl_pernet *pernet = pm_nl_get_pernet(net);
@@ -1394,6 +1386,11 @@ static struct pernet_operations mptcp_pm_pernet_ops = {
 	.size = sizeof(struct pm_nl_pernet),
 };
 
+static void mptcp_pm_kernel_add_addr_received(struct mptcp_sock *msk)
+{
+	mptcp_pm_nl_add_addr_received(msk);
+}
+
 static void mptcp_pm_kernel_init(struct mptcp_sock *msk)
 {
 	bool subflows_allowed = !!mptcp_pm_get_subflows_max(msk);
@@ -1419,6 +1416,7 @@ struct mptcp_pm_ops mptcp_pm_kernel = {
 	.get_priority		= mptcp_pm_kernel_get_priority,
 	.established		= mptcp_pm_kernel_established,
 	.subflow_established	= mptcp_pm_kernel_subflow_established,
+	.add_addr_received	= mptcp_pm_kernel_add_addr_received,
 	.init			= mptcp_pm_kernel_init,
 	.name			= "kernel",
 	.owner			= THIS_MODULE,
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 64aa091cb685..7fa26c49fbed 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -1164,7 +1164,6 @@ void __init mptcp_pm_kernel_register(void);
 void __init mptcp_pm_userspace_register(void);
 void __init mptcp_pm_nl_init(void);
 void mptcp_pm_worker(struct mptcp_sock *msk);
-void __mptcp_pm_kernel_worker(struct mptcp_sock *msk);
 unsigned int mptcp_pm_get_add_addr_signal_max(const struct mptcp_sock *msk);
 unsigned int mptcp_pm_get_add_addr_accept_max(const struct mptcp_sock *msk);
 unsigned int mptcp_pm_get_subflows_max(const struct mptcp_sock *msk);
-- 
2.43.0

[PATCH mptcp-next v4 6/9] mptcp: pm: add rm_addr_received() interface

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

This patch adds an optional .rm_addr_received interface for struct
mptcp_pm_ops and invokes it in mptcp_pm_rm_addr_or_subflow().

This interface is only implemented in the in-kernel PM as a wrapper
of mptcp_pm_nl_rm_addr().

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 include/net/mptcp.h   | 1 +
 net/mptcp/pm.c        | 4 ++--
 net/mptcp/pm_kernel.c | 6 ++++++
 3 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/include/net/mptcp.h b/include/net/mptcp.h
index 5118d11d2ee9..3f06fbd2a908 100644
--- a/include/net/mptcp.h
+++ b/include/net/mptcp.h
@@ -128,6 +128,7 @@ struct mptcp_pm_ops {
 	void (*established)(struct mptcp_sock *msk);
 	void (*subflow_established)(struct mptcp_sock *msk);
 	void (*add_addr_received)(struct mptcp_sock *msk);
+	void (*rm_addr_received)(struct mptcp_sock *msk, u8 id);
 
 	char			name[MPTCP_PM_NAME_MAX];
 	struct module		*owner;
diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index 71589cd5dee7..bf3c19defe98 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -703,8 +703,8 @@ static void mptcp_pm_rm_addr_or_subflow(struct mptcp_sock *msk,
 
 		if (rm_type == MPTCP_MIB_RMADDR) {
 			__MPTCP_INC_STATS(sock_net(sk), rm_type);
-			if (removed && mptcp_pm_is_kernel(msk))
-				mptcp_pm_nl_rm_addr(msk, rm_id);
+			if (removed && msk->pm.ops->rm_addr_received)
+				msk->pm.ops->rm_addr_received(msk, rm_id);
 		}
 	}
 }
diff --git a/net/mptcp/pm_kernel.c b/net/mptcp/pm_kernel.c
index 4f4791620072..1a71676bdf43 100644
--- a/net/mptcp/pm_kernel.c
+++ b/net/mptcp/pm_kernel.c
@@ -1391,6 +1391,11 @@ static void mptcp_pm_kernel_add_addr_received(struct mptcp_sock *msk)
 	mptcp_pm_nl_add_addr_received(msk);
 }
 
+static void mptcp_pm_kernel_rm_addr_received(struct mptcp_sock *msk, u8 id)
+{
+	mptcp_pm_nl_rm_addr(msk, id);
+}
+
 static void mptcp_pm_kernel_init(struct mptcp_sock *msk)
 {
 	bool subflows_allowed = !!mptcp_pm_get_subflows_max(msk);
@@ -1417,6 +1422,7 @@ struct mptcp_pm_ops mptcp_pm_kernel = {
 	.established		= mptcp_pm_kernel_established,
 	.subflow_established	= mptcp_pm_kernel_subflow_established,
 	.add_addr_received	= mptcp_pm_kernel_add_addr_received,
+	.rm_addr_received	= mptcp_pm_kernel_rm_addr_received,
 	.init			= mptcp_pm_kernel_init,
 	.name			= "kernel",
 	.owner			= THIS_MODULE,
-- 
2.43.0

[PATCH mptcp-next v4 7/9] mptcp: pm: add add_addr_echo() interface

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

The helper mptcp_pm_is_userspace() is used to distinguish userspace PM
operations from in-kernel PM in mptcp_pm_add_addr_received(). It seems
reasonable to add a mandatory .add_addr_echo interface for struct
mptcp_pm_ops.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 include/net/mptcp.h      |  4 ++++
 net/mptcp/pm.c           | 20 +++++++-------------
 net/mptcp/pm_kernel.c    |  9 +++++++++
 net/mptcp/pm_userspace.c |  7 +++++++
 net/mptcp/protocol.h     |  2 ++
 5 files changed, 29 insertions(+), 13 deletions(-)

diff --git a/include/net/mptcp.h b/include/net/mptcp.h
index 3f06fbd2a908..18d3679a752c 100644
--- a/include/net/mptcp.h
+++ b/include/net/mptcp.h
@@ -130,6 +130,10 @@ struct mptcp_pm_ops {
 	void (*add_addr_received)(struct mptcp_sock *msk);
 	void (*rm_addr_received)(struct mptcp_sock *msk, u8 id);
 
+	/* required */
+	bool (*add_addr_echo)(struct mptcp_sock *msk,
+			      const struct mptcp_addr_info *addr);
+
 	char			name[MPTCP_PM_NAME_MAX];
 	struct module		*owner;
 	struct list_head	list;
diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index bf3c19defe98..d37f89bf0180 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -104,8 +104,8 @@ void mptcp_remote_address(const struct sock_common *skc,
 #endif
 }
 
-static bool mptcp_pm_is_init_remote_addr(struct mptcp_sock *msk,
-					 const struct mptcp_addr_info *remote)
+bool mptcp_pm_is_init_remote_addr(struct mptcp_sock *msk,
+				  const struct mptcp_addr_info *remote)
 {
 	struct mptcp_addr_info mpc_remote;
 
@@ -595,16 +595,7 @@ void mptcp_pm_add_addr_received(const struct sock *ssk,
 
 	spin_lock_bh(&pm->lock);
 
-	if (mptcp_pm_is_userspace(msk)) {
-		if (mptcp_userspace_pm_active(msk)) {
-			mptcp_pm_announce_addr(msk, addr, true);
-			mptcp_pm_add_addr_send_ack(msk);
-		} else {
-			ret = -EINVAL;
-		}
-	/* id0 should not have a different address */
-	} else if ((addr->id == 0 && !mptcp_pm_is_init_remote_addr(msk, addr)) ||
-		   (addr->id > 0 && !READ_ONCE(pm->accept_addr))) {
+	if (pm->ops->add_addr_echo(msk, addr)) {
 		mptcp_pm_announce_addr(msk, addr, true);
 		mptcp_pm_add_addr_send_ack(msk);
 	} else if (pm->ops->add_addr_received) {
@@ -612,6 +603,8 @@ void mptcp_pm_add_addr_received(const struct sock *ssk,
 			pm->remote = *addr;
 		else
 			ret = -EINVAL;
+	} else {
+		ret = -EINVAL;
 	}
 
 	if (ret)
@@ -1063,7 +1056,8 @@ struct mptcp_pm_ops *mptcp_pm_find(const char *name)
 
 int mptcp_pm_validate(struct mptcp_pm_ops *pm_ops)
 {
-	if (!pm_ops->get_local_id || !pm_ops->get_priority) {
+	if (!pm_ops->get_local_id || !pm_ops->get_priority ||
+	    !pm_ops->add_addr_echo) {
 		pr_err("%s does not implement required ops\n", pm_ops->name);
 		return -EINVAL;
 	}
diff --git a/net/mptcp/pm_kernel.c b/net/mptcp/pm_kernel.c
index 1a71676bdf43..9d159196afe5 100644
--- a/net/mptcp/pm_kernel.c
+++ b/net/mptcp/pm_kernel.c
@@ -1396,6 +1396,14 @@ static void mptcp_pm_kernel_rm_addr_received(struct mptcp_sock *msk, u8 id)
 	mptcp_pm_nl_rm_addr(msk, id);
 }
 
+static bool mptcp_pm_kernel_add_addr_echo(struct mptcp_sock *msk,
+					  const struct mptcp_addr_info *addr)
+{
+	/* id0 should not have a different address */
+	return (addr->id == 0 && !mptcp_pm_is_init_remote_addr(msk, addr)) ||
+	       (addr->id > 0 && !READ_ONCE(msk->pm.accept_addr));
+}
+
 static void mptcp_pm_kernel_init(struct mptcp_sock *msk)
 {
 	bool subflows_allowed = !!mptcp_pm_get_subflows_max(msk);
@@ -1423,6 +1431,7 @@ struct mptcp_pm_ops mptcp_pm_kernel = {
 	.subflow_established	= mptcp_pm_kernel_subflow_established,
 	.add_addr_received	= mptcp_pm_kernel_add_addr_received,
 	.rm_addr_received	= mptcp_pm_kernel_rm_addr_received,
+	.add_addr_echo		= mptcp_pm_kernel_add_addr_echo,
 	.init			= mptcp_pm_kernel_init,
 	.name			= "kernel",
 	.owner			= THIS_MODULE,
diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index 7fc19b844384..3f7778ab064b 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -683,6 +683,12 @@ int mptcp_userspace_pm_get_addr(u8 id, struct mptcp_pm_addr_entry *addr,
 	return ret;
 }
 
+static bool mptcp_pm_userspace_add_addr_echo(struct mptcp_sock *msk,
+					     const struct mptcp_addr_info *addr)
+{
+	return mptcp_userspace_pm_active(msk);
+}
+
 static void mptcp_pm_userspace_release(struct mptcp_sock *msk)
 {
 	mptcp_userspace_pm_free_local_addr_list(msk);
@@ -691,6 +697,7 @@ static void mptcp_pm_userspace_release(struct mptcp_sock *msk)
 static struct mptcp_pm_ops mptcp_pm_userspace = {
 	.get_local_id		= mptcp_pm_userspace_get_local_id,
 	.get_priority		= mptcp_pm_userspace_get_priority,
+	.add_addr_echo		= mptcp_pm_userspace_add_addr_echo,
 	.release		= mptcp_pm_userspace_release,
 	.name			= "userspace",
 	.owner			= THIS_MODULE,
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 7fa26c49fbed..a886e89a806c 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -1014,6 +1014,8 @@ void mptcp_pm_subflow_established(struct mptcp_sock *msk);
 bool mptcp_pm_nl_check_work_pending(struct mptcp_sock *msk);
 void mptcp_pm_subflow_check_next(struct mptcp_sock *msk,
 				 const struct mptcp_subflow_context *subflow);
+bool mptcp_pm_is_init_remote_addr(struct mptcp_sock *msk,
+				  const struct mptcp_addr_info *remote);
 void mptcp_pm_add_addr_received(const struct sock *ssk,
 				const struct mptcp_addr_info *addr);
 void mptcp_pm_add_addr_echoed(struct mptcp_sock *msk,
-- 
2.43.0

[PATCH mptcp-next v4 8/9] mptcp: pm: add accept_new_subflow() interface

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

The helper mptcp_pm_is_userspace() is used to distinguish userspace PM
operations from in-kernel PM in mptcp_can_accept_new_subflow(). It seems
reasonable to add a mandatory .accept_new_subflow interface for struct
mptcp_pm_ops.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 include/net/mptcp.h      |  1 +
 net/mptcp/pm.c           | 31 +++++++++++--------------------
 net/mptcp/pm_kernel.c    |  6 ++++++
 net/mptcp/pm_userspace.c |  6 ++++++
 net/mptcp/subflow.c      |  4 +---
 5 files changed, 25 insertions(+), 23 deletions(-)

diff --git a/include/net/mptcp.h b/include/net/mptcp.h
index 18d3679a752c..8c1ac7368693 100644
--- a/include/net/mptcp.h
+++ b/include/net/mptcp.h
@@ -133,6 +133,7 @@ struct mptcp_pm_ops {
 	/* required */
 	bool (*add_addr_echo)(struct mptcp_sock *msk,
 			      const struct mptcp_addr_info *addr);
+	bool (*accept_new_subflow)(const struct mptcp_sock *msk);
 
 	char			name[MPTCP_PM_NAME_MAX];
 	struct module		*owner;
diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index d37f89bf0180..ca105bbd03ea 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -454,33 +454,24 @@ bool mptcp_pm_allow_new_subflow(struct mptcp_sock *msk)
 {
 	struct mptcp_pm_data *pm = &msk->pm;
 	unsigned int subflows_max;
-	int ret = 0;
+	bool ret = true;
 
-	if (mptcp_pm_is_userspace(msk)) {
-		if (mptcp_userspace_pm_active(msk)) {
-			spin_lock_bh(&pm->lock);
-			pm->subflows++;
-			spin_unlock_bh(&pm->lock);
-			return true;
-		}
+	if (!pm->ops->accept_new_subflow(msk))
 		return false;
-	}
-
-	subflows_max = mptcp_pm_get_subflows_max(msk);
 
-	pr_debug("msk=%p subflows=%d max=%d allow=%d\n", msk, pm->subflows,
-		 subflows_max, READ_ONCE(pm->accept_subflow));
+	spin_lock_bh(&pm->lock);
+	if (!mptcp_pm_is_userspace(msk) && READ_ONCE(pm->accept_subflow)) {
+		subflows_max = mptcp_pm_get_subflows_max(msk);
 
-	/* try to avoid acquiring the lock below */
-	if (!READ_ONCE(pm->accept_subflow))
-		return false;
+		pr_debug("msk=%p subflows=%d max=%d allow=%d\n", msk, pm->subflows,
+			 subflows_max, READ_ONCE(pm->accept_subflow));
 
-	spin_lock_bh(&pm->lock);
-	if (READ_ONCE(pm->accept_subflow)) {
 		ret = pm->subflows < subflows_max;
-		if (ret && ++pm->subflows == subflows_max)
+		if (ret && pm->subflows == subflows_max - 1)
 			WRITE_ONCE(pm->accept_subflow, false);
 	}
+	if (ret)
+		pm->subflows++;
 	spin_unlock_bh(&pm->lock);
 
 	return ret;
@@ -1057,7 +1048,7 @@ struct mptcp_pm_ops *mptcp_pm_find(const char *name)
 int mptcp_pm_validate(struct mptcp_pm_ops *pm_ops)
 {
 	if (!pm_ops->get_local_id || !pm_ops->get_priority ||
-	    !pm_ops->add_addr_echo) {
+	    !pm_ops->add_addr_echo || !pm_ops->accept_new_subflow) {
 		pr_err("%s does not implement required ops\n", pm_ops->name);
 		return -EINVAL;
 	}
diff --git a/net/mptcp/pm_kernel.c b/net/mptcp/pm_kernel.c
index 9d159196afe5..7ec8fafeda0e 100644
--- a/net/mptcp/pm_kernel.c
+++ b/net/mptcp/pm_kernel.c
@@ -1404,6 +1404,11 @@ static bool mptcp_pm_kernel_add_addr_echo(struct mptcp_sock *msk,
 	       (addr->id > 0 && !READ_ONCE(msk->pm.accept_addr));
 }
 
+static bool mptcp_pm_kernel_accept_new_subflow(const struct mptcp_sock *msk)
+{
+	return READ_ONCE(msk->pm.accept_subflow);
+}
+
 static void mptcp_pm_kernel_init(struct mptcp_sock *msk)
 {
 	bool subflows_allowed = !!mptcp_pm_get_subflows_max(msk);
@@ -1432,6 +1437,7 @@ struct mptcp_pm_ops mptcp_pm_kernel = {
 	.add_addr_received	= mptcp_pm_kernel_add_addr_received,
 	.rm_addr_received	= mptcp_pm_kernel_rm_addr_received,
 	.add_addr_echo		= mptcp_pm_kernel_add_addr_echo,
+	.accept_new_subflow	= mptcp_pm_kernel_accept_new_subflow,
 	.init			= mptcp_pm_kernel_init,
 	.name			= "kernel",
 	.owner			= THIS_MODULE,
diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index 3f7778ab064b..d6301d809376 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -689,6 +689,11 @@ static bool mptcp_pm_userspace_add_addr_echo(struct mptcp_sock *msk,
 	return mptcp_userspace_pm_active(msk);
 }
 
+static bool mptcp_pm_userspace_accept_new_subflow(const struct mptcp_sock *msk)
+{
+	return mptcp_userspace_pm_active(msk);
+}
+
 static void mptcp_pm_userspace_release(struct mptcp_sock *msk)
 {
 	mptcp_userspace_pm_free_local_addr_list(msk);
@@ -698,6 +703,7 @@ static struct mptcp_pm_ops mptcp_pm_userspace = {
 	.get_local_id		= mptcp_pm_userspace_get_local_id,
 	.get_priority		= mptcp_pm_userspace_get_priority,
 	.add_addr_echo		= mptcp_pm_userspace_add_addr_echo,
+	.accept_new_subflow	= mptcp_pm_userspace_accept_new_subflow,
 	.release		= mptcp_pm_userspace_release,
 	.name			= "userspace",
 	.owner			= THIS_MODULE,
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 409bd415ef1d..be79940da424 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -61,9 +61,7 @@ static void subflow_generate_hmac(u64 key1, u64 key2, u32 nonce1, u32 nonce2,
 static bool mptcp_can_accept_new_subflow(const struct mptcp_sock *msk)
 {
 	return mptcp_is_fully_established((void *)msk) &&
-		((mptcp_pm_is_userspace(msk) &&
-		  mptcp_userspace_pm_active(msk)) ||
-		 READ_ONCE(msk->pm.accept_subflow));
+		msk->pm.ops->accept_new_subflow(msk);
 }
 
 /* validate received token and create truncated hmac and nonce for SYN-ACK */
-- 
2.43.0

[PATCH mptcp-next v4 9/9] mptcp: pm: add allow_new_subflow() interface

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

The helper mptcp_pm_is_userspace() is used to distinguish userspace PM
operations from in-kernel PM in mptcp_pm_allow_new_subflow(). It seems
reasonable to add a mandatory .allow_new_subflow interface for struct
mptcp_pm_ops.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 include/net/mptcp.h   |  3 +++
 net/mptcp/pm.c        | 13 ++-----------
 net/mptcp/pm_kernel.c | 21 +++++++++++++++++++++
 3 files changed, 26 insertions(+), 11 deletions(-)

diff --git a/include/net/mptcp.h b/include/net/mptcp.h
index 8c1ac7368693..aedabc7f4190 100644
--- a/include/net/mptcp.h
+++ b/include/net/mptcp.h
@@ -135,6 +135,9 @@ struct mptcp_pm_ops {
 			      const struct mptcp_addr_info *addr);
 	bool (*accept_new_subflow)(const struct mptcp_sock *msk);
 
+	/* optional */
+	bool (*allow_new_subflow)(struct mptcp_sock *msk);
+
 	char			name[MPTCP_PM_NAME_MAX];
 	struct module		*owner;
 	struct list_head	list;
diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index ca105bbd03ea..215b3a4d24be 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -453,23 +453,14 @@ void mptcp_pm_new_connection(struct mptcp_sock *msk, const struct sock *ssk, int
 bool mptcp_pm_allow_new_subflow(struct mptcp_sock *msk)
 {
 	struct mptcp_pm_data *pm = &msk->pm;
-	unsigned int subflows_max;
 	bool ret = true;
 
 	if (!pm->ops->accept_new_subflow(msk))
 		return false;
 
 	spin_lock_bh(&pm->lock);
-	if (!mptcp_pm_is_userspace(msk) && READ_ONCE(pm->accept_subflow)) {
-		subflows_max = mptcp_pm_get_subflows_max(msk);
-
-		pr_debug("msk=%p subflows=%d max=%d allow=%d\n", msk, pm->subflows,
-			 subflows_max, READ_ONCE(pm->accept_subflow));
-
-		ret = pm->subflows < subflows_max;
-		if (ret && pm->subflows == subflows_max - 1)
-			WRITE_ONCE(pm->accept_subflow, false);
-	}
+	if (pm->ops->allow_new_subflow)
+		ret = pm->ops->allow_new_subflow(msk);
 	if (ret)
 		pm->subflows++;
 	spin_unlock_bh(&pm->lock);
diff --git a/net/mptcp/pm_kernel.c b/net/mptcp/pm_kernel.c
index 7ec8fafeda0e..5ae64392a316 100644
--- a/net/mptcp/pm_kernel.c
+++ b/net/mptcp/pm_kernel.c
@@ -1409,6 +1409,26 @@ static bool mptcp_pm_kernel_accept_new_subflow(const struct mptcp_sock *msk)
 	return READ_ONCE(msk->pm.accept_subflow);
 }
 
+static bool mptcp_pm_kernel_allow_new_subflow(struct mptcp_sock *msk)
+{
+	struct mptcp_pm_data *pm = &msk->pm;
+	unsigned int subflows_max;
+	bool ret = false;
+
+	subflows_max = mptcp_pm_get_subflows_max(msk);
+
+	pr_debug("msk=%p subflows=%d max=%d allow=%d\n", msk, pm->subflows,
+		 subflows_max, READ_ONCE(pm->accept_subflow));
+
+	if (READ_ONCE(pm->accept_subflow)) {
+		ret = pm->subflows < subflows_max;
+		if (ret && pm->subflows == subflows_max - 1)
+			WRITE_ONCE(pm->accept_subflow, false);
+	}
+
+	return ret;
+}
+
 static void mptcp_pm_kernel_init(struct mptcp_sock *msk)
 {
 	bool subflows_allowed = !!mptcp_pm_get_subflows_max(msk);
@@ -1438,6 +1458,7 @@ struct mptcp_pm_ops mptcp_pm_kernel = {
 	.rm_addr_received	= mptcp_pm_kernel_rm_addr_received,
 	.add_addr_echo		= mptcp_pm_kernel_add_addr_echo,
 	.accept_new_subflow	= mptcp_pm_kernel_accept_new_subflow,
+	.allow_new_subflow	= mptcp_pm_kernel_allow_new_subflow,
 	.init			= mptcp_pm_kernel_init,
 	.name			= "kernel",
 	.owner			= THIS_MODULE,
-- 
2.43.0

Re: [PATCH mptcp-next v4 1/9] Squash to "mptcp: pm: add get_local_id() interface"

[Matthieu Baerts]

Hi Geliang,

On 24/03/2025 09:19, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> Add /* required */ comment for get_local_id and get_priority.

Thank you for this patch. I just applied it not to have to carry this
simple Squash-to patch again. I will continue the review of the other
patches later.

New patches for t/upstream:
- d9ff94b6854e: Squash to "mptcp: pm: add get_local_id() interface"
- Results: 72b4c5fecd91..38f8291cd31d (export)

Tests are now in progress:

- export:
https://github.com/multipath-tcp/mptcp_net-next/commit/148e4f30be3b3ca9fcb647cc656ebf736925ee5d/checks

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

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

[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/14030669230

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


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 v4 6/9] mptcp: pm: add rm_addr_received() interface

[Geliang Tang]

On Mon, 2025-03-24 at 16:19 +0800, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> This patch adds an optional .rm_addr_received interface for struct
> mptcp_pm_ops and invokes it in mptcp_pm_rm_addr_or_subflow().
> 
> This interface is only implemented in the in-kernel PM as a wrapper
> of mptcp_pm_nl_rm_addr().
> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  include/net/mptcp.h   | 1 +
>  net/mptcp/pm.c        | 4 ++--
>  net/mptcp/pm_kernel.c | 6 ++++++
>  3 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/include/net/mptcp.h b/include/net/mptcp.h
> index 5118d11d2ee9..3f06fbd2a908 100644
> --- a/include/net/mptcp.h
> +++ b/include/net/mptcp.h
> @@ -128,6 +128,7 @@ struct mptcp_pm_ops {
>  	void (*established)(struct mptcp_sock *msk);
>  	void (*subflow_established)(struct mptcp_sock *msk);
>  	void (*add_addr_received)(struct mptcp_sock *msk);
> +	void (*rm_addr_received)(struct mptcp_sock *msk, u8 id);
>  
>  	char			name[MPTCP_PM_NAME_MAX];
>  	struct module		*owner;
> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index 71589cd5dee7..bf3c19defe98 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c
> @@ -703,8 +703,8 @@ static void mptcp_pm_rm_addr_or_subflow(struct
> mptcp_sock *msk,
>  
>  		if (rm_type == MPTCP_MIB_RMADDR) {
>  			__MPTCP_INC_STATS(sock_net(sk), rm_type);
> -			if (removed && mptcp_pm_is_kernel(msk))
> -				mptcp_pm_nl_rm_addr(msk, rm_id);
> +			if (removed && msk->pm.ops-
> >rm_addr_received)
> +				msk->pm.ops->rm_addr_received(msk,
> rm_id);
>  		}
>  	}
>  }
> diff --git a/net/mptcp/pm_kernel.c b/net/mptcp/pm_kernel.c
> index 4f4791620072..1a71676bdf43 100644
> --- a/net/mptcp/pm_kernel.c
> +++ b/net/mptcp/pm_kernel.c
> @@ -1391,6 +1391,11 @@ static void
> mptcp_pm_kernel_add_addr_received(struct mptcp_sock *msk)
>  	mptcp_pm_nl_add_addr_received(msk);
>  }
>  
> +static void mptcp_pm_kernel_rm_addr_received(struct mptcp_sock *msk,
> u8 id)
> +{
> +	mptcp_pm_nl_rm_addr(msk, id);

Now we can make mptcp_pm_nl_rm_addr static.
Will update this in v5.

Thanks,
-Geliang

> +}
> +
>  static void mptcp_pm_kernel_init(struct mptcp_sock *msk)
>  {
>  	bool subflows_allowed = !!mptcp_pm_get_subflows_max(msk);
> @@ -1417,6 +1422,7 @@ struct mptcp_pm_ops mptcp_pm_kernel = {
>  	.established		= mptcp_pm_kernel_established,
>  	.subflow_established	=
> mptcp_pm_kernel_subflow_established,
>  	.add_addr_received	= mptcp_pm_kernel_add_addr_received,
> +	.rm_addr_received	= mptcp_pm_kernel_rm_addr_received,
>  	.init			= mptcp_pm_kernel_init,
>  	.name			= "kernel",
>  	.owner			= THIS_MODULE,

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

[Matthieu Baerts]

Hi Geliang,

On 24/03/2025 09:19, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> v4:
>  - address Matt's comments in v3.
>  - update pm locks in mptcp_pm_worker.
>  - move the lock inside mptcp_pm_create_subflow_or_signal_addr.
>  - move the lock inside mptcp_pm_nl_add_addr_received.
>  - invoke add_addr_received interface from mptcp_pm_worker.
>  - invoke rm_addr_received interface from mptcp_pm_rm_addr_or_subflow.
>  - simply call mptcp_pm_close_subflow() in mptcp_pm_subflow_check_next.

Thank you for the v4. I have some comments, please see my individual
replies.

After this series, do you still have any mptcp_pm_is_userspace() and
mptcp_pm_is_kernel()? Can we eventually get rid of them? Same for
pm_type from "struct mptcp_pm_data" and from "struct mptcp_pernet", no?

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

Re: [PATCH mptcp-next v4 2/9] mptcp: pm: add established interfaces

[Matthieu Baerts]

Hi Geliang,

On 24/03/2025 09:19, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> This patch adds .established and .subflow_established interfaces for
> struct mptcp_pm_ops, and calls pm->ops->established/subflow_established
> in from mptcp_pm_worker(). Then get rid of the corresponding code from
> __mptcp_pm_kernel_worker().
> 
> Since mptcp_pm_addr_send_ack() is a sleepable kfunc, which is invoked
> by mptcp_pm_create_subflow_or_signal_addr(), .established() and
> .subflow_established() interfaces of BPF PM should be invoked by
> __bpf_prog_enter_sleepable(), which can't be invoked under a lock.
> This patch unlocks the pm lock before invoking this interface in
> mptcp_pm_worker(), while holding this lock in mptcp_pm_kernel_established()
> and mptcp_pm_kernel_subflow_established().

(...)

> @@ -949,20 +951,34 @@ void mptcp_pm_worker(struct mptcp_sock *msk)
>  	if (!(pm->status & MPTCP_PM_WORK_MASK))
>  		return;
>  
> -	spin_lock_bh(&msk->pm.lock);
> -
>  	pr_debug("msk=%p status=%x\n", msk, pm->status);
>  	if (pm->status & BIT(MPTCP_PM_ADD_ADDR_SEND_ACK)) {

This should probably be read under the lock or with a READ_ONCE. Or
manipulating pm->status before, then running the different actions. I
need to think about that.

I also think it might be good to have a dedicated patch moving the
locking mechanisms first, then introducing the new callbacks.

I will discuss that with Mat on Wednesday. Do you mind holding new
versions for this series until then please?

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

Re: [PATCH mptcp-next v4 3/9] mptcp: pm: drop is_userspace in subflow_check_next

[Matthieu Baerts]

Hi Geliang,

On 24/03/2025 09:19, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> This patch moves mptcp_pm_close_subflow() forward to let it be used by both
> the userspace PM and the in-kernel PM in mptcp_pm_subflow_check_next().Then

(space missing after the '.')

> mptcp_pm_is_userspace() here can be dropped.
> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  net/mptcp/pm.c | 12 ++----------
>  1 file changed, 2 insertions(+), 10 deletions(-)
> 
> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index e2b2c874a9f8..906c558aef0b 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c
> @@ -562,22 +562,14 @@ void mptcp_pm_subflow_check_next(struct mptcp_sock *msk,
>  	bool update_subflows;
>  
>  	update_subflows = subflow->request_join || subflow->mp_join;
> -	if (mptcp_pm_is_userspace(msk)) {
> -		if (update_subflows) {
> -			spin_lock_bh(&pm->lock);
> -			pm->subflows--;
> -			spin_unlock_bh(&pm->lock);
> -		}
> -		return;
> -	}
> +	if (update_subflows)
> +		mptcp_pm_close_subflow(msk);
>  
>  	if (!pm->ops->subflow_established ||
>  	    (!READ_ONCE(pm->work_pending) && !update_subflows))

I didn't check, but can we then not drop "!update_subflows" check here?

>  		return;
>  
>  	spin_lock_bh(&pm->lock);
> -	if (update_subflows)
> -		__mptcp_pm_close_subflow(msk);
>  
>  	/* Even if this subflow is not really established, tell the PM to try
>  	 * to pick the next ones, if possible.

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

Re: [PATCH mptcp-next v4 5/9] mptcp: pm: add add_addr_received() interface

[Matthieu Baerts]

Hi Geliang,

On 24/03/2025 09:19, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> This patch adds an optional .add_addr_received interface for struct
> mptcp_pm_ops and invokes it in mptcp_pm_worker().
> 
> This interface is only implemented in the in-kernel PM as a wrapper
> of mptcp_pm_nl_add_addr_received().
> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  include/net/mptcp.h   |  1 +
>  net/mptcp/pm.c        | 18 +++++++++++-------
>  net/mptcp/pm_kernel.c | 24 +++++++++++-------------
>  net/mptcp/protocol.h  |  1 -
>  4 files changed, 23 insertions(+), 21 deletions(-)
> 
> diff --git a/include/net/mptcp.h b/include/net/mptcp.h
> index 4ac936e4ce0d..5118d11d2ee9 100644
> --- a/include/net/mptcp.h
> +++ b/include/net/mptcp.h
> @@ -127,6 +127,7 @@ struct mptcp_pm_ops {
>  	/* optional */
>  	void (*established)(struct mptcp_sock *msk);
>  	void (*subflow_established)(struct mptcp_sock *msk);
> +	void (*add_addr_received)(struct mptcp_sock *msk);
>  
>  	char			name[MPTCP_PM_NAME_MAX];
>  	struct module		*owner;
> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index 8efb47331f79..71589cd5dee7 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c
> @@ -607,10 +607,11 @@ void mptcp_pm_add_addr_received(const struct sock *ssk,
>  		   (addr->id > 0 && !READ_ONCE(pm->accept_addr))) {

How do you plan to remove the 'if (mptcp_pm_is_userspace(msk))' here above.

I guess this code is there to force the other peer to retransmit the
ADD_ADDR, hoping a userspace will be launched in between. Either we
remove this exception for the userspace PM (other events will not be
retransmitted: RM_ADDR, subflow closed, etc.), or we have another hook
but it feels wrong.

EDIT: I just saw your patch 7/9. Maybe we should avoid adding this
add_addr_echo hook, no? It is not clear what should be done here. I need
to think about that too.

An alternative is to send the ADD_ADDR echo from the worker, if
pm->ops->add_addr_received() returned true. If
pm->ops->add_addr_received is not implemented, then the ADD_ADDR echo is
scheduled from here. WDYT?

>  		mptcp_pm_announce_addr(msk, addr, true);
>  		mptcp_pm_add_addr_send_ack(msk);
> -	} else if (mptcp_pm_schedule_work(msk, MPTCP_PM_ADD_ADDR_RECEIVED)) {
> -		pm->remote = *addr;
> -	} else {
> -		ret = -EINVAL;
> +	} else if (pm->ops->add_addr_received) {
> +		if (mptcp_pm_schedule_work(msk, MPTCP_PM_ADD_ADDR_RECEIVED))
> +			pm->remote = *addr;
> +		else
> +			ret = -EINVAL;
>  	}
>  
>  	if (ret)
> @@ -948,6 +949,12 @@ void mptcp_pm_worker(struct mptcp_sock *msk)
>  		return;
>  
>  	pr_debug("msk=%p status=%x\n", msk, pm->status);
> +	if (pm->status & BIT(MPTCP_PM_ADD_ADDR_RECEIVED)) {
> +		spin_lock_bh(&pm->lock);
> +		pm->status &= ~BIT(MPTCP_PM_ADD_ADDR_RECEIVED);
> +		spin_unlock_bh(&pm->lock);
> +		pm->ops->add_addr_received(msk);
> +	}
>  	if (pm->status & BIT(MPTCP_PM_ADD_ADDR_SEND_ACK)) {
>  		spin_lock_bh(&pm->lock);
>  		pm->status &= ~BIT(MPTCP_PM_ADD_ADDR_SEND_ACK);
> @@ -972,9 +979,6 @@ void mptcp_pm_worker(struct mptcp_sock *msk)
>  		spin_unlock_bh(&pm->lock);
>  		pm->ops->subflow_established(msk);
>  	}
> -	spin_lock_bh(&pm->lock);
> -	__mptcp_pm_kernel_worker(msk);
> -	spin_unlock_bh(&pm->lock);
>  }
>  
>  static void mptcp_pm_ops_init(struct mptcp_sock *msk,
> diff --git a/net/mptcp/pm_kernel.c b/net/mptcp/pm_kernel.c
> index 2e181224bccb..4f4791620072 100644
> --- a/net/mptcp/pm_kernel.c
> +++ b/net/mptcp/pm_kernel.c
> @@ -461,12 +461,13 @@ static void mptcp_pm_nl_add_addr_received(struct mptcp_sock *msk)
>  		 msk->pm.add_addr_accepted, add_addr_accept_max,
>  		 msk->pm.remote.family);
>  
> +	spin_lock_bh(&msk->pm.lock);
>  	remote = msk->pm.remote;
>  	mptcp_pm_announce_addr(msk, &remote, true);
>  	mptcp_pm_addr_send_ack(msk);
>  
>  	if (lookup_subflow_by_daddr(&msk->conn_list, &remote))
> -		return;
> +		goto out;
>  
>  	/* pick id 0 port, if none is provided the remote address */
>  	if (!remote.port)
> @@ -477,7 +478,7 @@ static void mptcp_pm_nl_add_addr_received(struct mptcp_sock *msk)
>  	 */
>  	nr = fill_local_addresses_vec(msk, &remote, locals);
>  	if (nr == 0)
> -		return;
> +		goto out;
>  
>  	spin_unlock_bh(&msk->pm.lock);
>  	for (i = 0; i < nr; i++)
> @@ -493,6 +494,8 @@ static void mptcp_pm_nl_add_addr_received(struct mptcp_sock *msk)
>  		    msk->pm.subflows >= subflows_max)
>  			WRITE_ONCE(msk->pm.accept_addr, false);
>  	}
> +out:
> +	spin_unlock_bh(&msk->pm.lock);
>  }
>  
>  void mptcp_pm_nl_rm_addr(struct mptcp_sock *msk, u8 rm_id)
> @@ -1342,17 +1345,6 @@ bool mptcp_pm_nl_check_work_pending(struct mptcp_sock *msk)
>  	return true;
>  }
>  
> -/* Called under PM lock */
> -void __mptcp_pm_kernel_worker(struct mptcp_sock *msk)
> -{
> -	struct mptcp_pm_data *pm = &msk->pm;
> -
> -	if (pm->status & BIT(MPTCP_PM_ADD_ADDR_RECEIVED)) {
> -		pm->status &= ~BIT(MPTCP_PM_ADD_ADDR_RECEIVED);
> -		mptcp_pm_nl_add_addr_received(msk);
> -	}
> -}
> -
>  static int __net_init pm_nl_init_net(struct net *net)
>  {
>  	struct pm_nl_pernet *pernet = pm_nl_get_pernet(net);
> @@ -1394,6 +1386,11 @@ static struct pernet_operations mptcp_pm_pernet_ops = {
>  	.size = sizeof(struct pm_nl_pernet),
>  };
>  
> +static void mptcp_pm_kernel_add_addr_received(struct mptcp_sock *msk)
> +{
> +	mptcp_pm_nl_add_addr_received(msk);

No need to add a new static function only calling another static
function with the same arguments.

Simply rename mptcp_pm_nl_add_addr_received() to
mptcp_pm_kernel_add_addr_received().

> +}
> +
>  static void mptcp_pm_kernel_init(struct mptcp_sock *msk)
>  {
>  	bool subflows_allowed = !!mptcp_pm_get_subflows_max(msk);
> @@ -1419,6 +1416,7 @@ struct mptcp_pm_ops mptcp_pm_kernel = {
>  	.get_priority		= mptcp_pm_kernel_get_priority,
>  	.established		= mptcp_pm_kernel_established,
>  	.subflow_established	= mptcp_pm_kernel_subflow_established,
> +	.add_addr_received	= mptcp_pm_kernel_add_addr_received,
>  	.init			= mptcp_pm_kernel_init,
>  	.name			= "kernel",
>  	.owner			= THIS_MODULE,
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index 64aa091cb685..7fa26c49fbed 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -1164,7 +1164,6 @@ void __init mptcp_pm_kernel_register(void);
>  void __init mptcp_pm_userspace_register(void);
>  void __init mptcp_pm_nl_init(void);
>  void mptcp_pm_worker(struct mptcp_sock *msk);
> -void __mptcp_pm_kernel_worker(struct mptcp_sock *msk);
>  unsigned int mptcp_pm_get_add_addr_signal_max(const struct mptcp_sock *msk);
>  unsigned int mptcp_pm_get_add_addr_accept_max(const struct mptcp_sock *msk);
>  unsigned int mptcp_pm_get_subflows_max(const struct mptcp_sock *msk);

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

Re: [PATCH mptcp-next v4 6/9] mptcp: pm: add rm_addr_received() interface

[Matthieu Baerts]

Hi Geliang,

On 24/03/2025 09:19, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> This patch adds an optional .rm_addr_received interface for struct
> mptcp_pm_ops and invokes it in mptcp_pm_rm_addr_or_subflow().
> 
> This interface is only implemented in the in-kernel PM as a wrapper
> of mptcp_pm_nl_rm_addr().

(...)

> diff --git a/net/mptcp/pm_kernel.c b/net/mptcp/pm_kernel.c
> index 4f4791620072..1a71676bdf43 100644
> --- a/net/mptcp/pm_kernel.c
> +++ b/net/mptcp/pm_kernel.c
> @@ -1391,6 +1391,11 @@ static void mptcp_pm_kernel_add_addr_received(struct mptcp_sock *msk)
>  	mptcp_pm_nl_add_addr_received(msk);
>  }
>  
> +static void mptcp_pm_kernel_rm_addr_received(struct mptcp_sock *msk, u8 id)
> +{
> +	mptcp_pm_nl_rm_addr(msk, id);

Same here: no need to add a new static function only calling another
static function (mptcp_pm_nl_rm_addr() should now be static) with the
same arguments.

Simply rename mptcp_pm_nl_rm_addr() to mptcp_pm_kernel_rm_addr_received().

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

Re: [PATCH mptcp-next v4 7/9] mptcp: pm: add add_addr_echo() interface

[Matthieu Baerts]

Hi Geliang,

On 24/03/2025 09:19, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> The helper mptcp_pm_is_userspace() is used to distinguish userspace PM
> operations from in-kernel PM in mptcp_pm_add_addr_received(). It seems
> reasonable to add a mandatory .add_addr_echo interface for struct
> mptcp_pm_ops.

As mentioned in patch 5/9, I'm not sure about that: it feels wrong to
have a hook there.

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

Re: [PATCH mptcp-next v4 8/9] mptcp: pm: add accept_new_subflow() interface

[Matthieu Baerts]

On 24/03/2025 09:19, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> The helper mptcp_pm_is_userspace() is used to distinguish userspace PM
> operations from in-kernel PM in mptcp_can_accept_new_subflow(). It seems
> reasonable to add a mandatory .accept_new_subflow interface for struct
> mptcp_pm_ops.
> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  include/net/mptcp.h      |  1 +
>  net/mptcp/pm.c           | 31 +++++++++++--------------------
>  net/mptcp/pm_kernel.c    |  6 ++++++
>  net/mptcp/pm_userspace.c |  6 ++++++
>  net/mptcp/subflow.c      |  4 +---
>  5 files changed, 25 insertions(+), 23 deletions(-)
> 
> diff --git a/include/net/mptcp.h b/include/net/mptcp.h
> index 18d3679a752c..8c1ac7368693 100644
> --- a/include/net/mptcp.h
> +++ b/include/net/mptcp.h
> @@ -133,6 +133,7 @@ struct mptcp_pm_ops {
>  	/* required */
>  	bool (*add_addr_echo)(struct mptcp_sock *msk,
>  			      const struct mptcp_addr_info *addr);
> +	bool (*accept_new_subflow)(const struct mptcp_sock *msk);

Similar to get_local_id() and get_priority(), I guess this callback will
be triggered from the subflow context, and not the msk context, right?

Detail: probably we should gather them together in this structure, with
an additional comment clearly mentioning in which context the callbacks
will be called.

>  	char			name[MPTCP_PM_NAME_MAX];
>  	struct module		*owner;
> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index d37f89bf0180..ca105bbd03ea 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c
> @@ -454,33 +454,24 @@ bool mptcp_pm_allow_new_subflow(struct mptcp_sock *msk)
>  {
>  	struct mptcp_pm_data *pm = &msk->pm;
>  	unsigned int subflows_max;
> -	int ret = 0;
> +	bool ret = true;
>  
> -	if (mptcp_pm_is_userspace(msk)) {
> -		if (mptcp_userspace_pm_active(msk)) {
> -			spin_lock_bh(&pm->lock);
> -			pm->subflows++;
> -			spin_unlock_bh(&pm->lock);
> -			return true;
> -		}
> +	if (!pm->ops->accept_new_subflow(msk))
>  		return false;
> -	}
> -
> -	subflows_max = mptcp_pm_get_subflows_max(msk);
>  
> -	pr_debug("msk=%p subflows=%d max=%d allow=%d\n", msk, pm->subflows,
> -		 subflows_max, READ_ONCE(pm->accept_subflow));
> +	spin_lock_bh(&pm->lock);
> +	if (!mptcp_pm_is_userspace(msk) && READ_ONCE(pm->accept_subflow)) {
> +		subflows_max = mptcp_pm_get_subflows_max(msk);
>  
> -	/* try to avoid acquiring the lock below */
> -	if (!READ_ONCE(pm->accept_subflow))
> -		return false;
> +		pr_debug("msk=%p subflows=%d max=%d allow=%d\n", msk, pm->subflows,
> +			 subflows_max, READ_ONCE(pm->accept_subflow));
>  
> -	spin_lock_bh(&pm->lock);
> -	if (READ_ONCE(pm->accept_subflow)) {
>  		ret = pm->subflows < subflows_max;
> -		if (ret && ++pm->subflows == subflows_max)
> +		if (ret && pm->subflows == subflows_max - 1)
>  			WRITE_ONCE(pm->accept_subflow, false);
>  	}

Maybe I missed something, but could we not move this code to
mptcp_pm_kernel_accept_new_subflow()?

There here, we would have something like:

  if (pm->ops->accept_new_subflow(msk)) {
      spin_lock_bh(&pm->lock);
      pm->subflows++;
      spin_unlock_bh(&pm->lock);
  }

No?

EDIT: just noticed you are doing that in patch 9/9. Can you not do that
in the same callback, but passing an extra argument to it? Or is it an
issue with the locks?

  bool (*accept_new_subflow)(const struct mptcp_sock *msk, bool allow);

> +	if (ret)
> +		pm->subflows++;
>  
>  	spin_unlock_bh(&pm->lock);
>  
>  	return ret;
> @@ -1057,7 +1048,7 @@ struct mptcp_pm_ops *mptcp_pm_find(const char *name)
>  int mptcp_pm_validate(struct mptcp_pm_ops *pm_ops)
>  {
>  	if (!pm_ops->get_local_id || !pm_ops->get_priority ||
> -	    !pm_ops->add_addr_echo) {
> +	    !pm_ops->add_addr_echo || !pm_ops->accept_new_subflow) {
>  		pr_err("%s does not implement required ops\n", pm_ops->name);
>  		return -EINVAL;
>  	}
> diff --git a/net/mptcp/pm_kernel.c b/net/mptcp/pm_kernel.c
> index 9d159196afe5..7ec8fafeda0e 100644
> --- a/net/mptcp/pm_kernel.c
> +++ b/net/mptcp/pm_kernel.c
> @@ -1404,6 +1404,11 @@ static bool mptcp_pm_kernel_add_addr_echo(struct mptcp_sock *msk,
>  	       (addr->id > 0 && !READ_ONCE(msk->pm.accept_addr));
>  }
>  
> +static bool mptcp_pm_kernel_accept_new_subflow(const struct mptcp_sock *msk)
> +{
> +	return READ_ONCE(msk->pm.accept_subflow);
> +}
> +
>  static void mptcp_pm_kernel_init(struct mptcp_sock *msk)
>  {
>  	bool subflows_allowed = !!mptcp_pm_get_subflows_max(msk);
> @@ -1432,6 +1437,7 @@ struct mptcp_pm_ops mptcp_pm_kernel = {
>  	.add_addr_received	= mptcp_pm_kernel_add_addr_received,
>  	.rm_addr_received	= mptcp_pm_kernel_rm_addr_received,
>  	.add_addr_echo		= mptcp_pm_kernel_add_addr_echo,
> +	.accept_new_subflow	= mptcp_pm_kernel_accept_new_subflow,
>  	.init			= mptcp_pm_kernel_init,
>  	.name			= "kernel",
>  	.owner			= THIS_MODULE,
> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> index 3f7778ab064b..d6301d809376 100644
> --- a/net/mptcp/pm_userspace.c
> +++ b/net/mptcp/pm_userspace.c
> @@ -689,6 +689,11 @@ static bool mptcp_pm_userspace_add_addr_echo(struct mptcp_sock *msk,
>  	return mptcp_userspace_pm_active(msk);
>  }
>  
> +static bool mptcp_pm_userspace_accept_new_subflow(const struct mptcp_sock *msk)
> +{
> +	return mptcp_userspace_pm_active(msk);
> +}
> +
>  static void mptcp_pm_userspace_release(struct mptcp_sock *msk)
>  {
>  	mptcp_userspace_pm_free_local_addr_list(msk);
> @@ -698,6 +703,7 @@ static struct mptcp_pm_ops mptcp_pm_userspace = {
>  	.get_local_id		= mptcp_pm_userspace_get_local_id,
>  	.get_priority		= mptcp_pm_userspace_get_priority,
>  	.add_addr_echo		= mptcp_pm_userspace_add_addr_echo,
> +	.accept_new_subflow	= mptcp_pm_userspace_accept_new_subflow,
>  	.release		= mptcp_pm_userspace_release,
>  	.name			= "userspace",
>  	.owner			= THIS_MODULE,
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index 409bd415ef1d..be79940da424 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -61,9 +61,7 @@ static void subflow_generate_hmac(u64 key1, u64 key2, u32 nonce1, u32 nonce2,
>  static bool mptcp_can_accept_new_subflow(const struct mptcp_sock *msk)
>  {
>  	return mptcp_is_fully_established((void *)msk) &&
> -		((mptcp_pm_is_userspace(msk) &&
> -		  mptcp_userspace_pm_active(msk)) ||
> -		 READ_ONCE(msk->pm.accept_subflow));
> +		msk->pm.ops->accept_new_subflow(msk);

I think pm->ops should only be used from pm.c. In other words, I suggest
having a dedicated patch changing this helper to call a new one added in
pm.c, e.g.

  return mptcp_is_fully_established((void *)msk) &&
         mptcp_pm_accept_new_subflow(msk);

WDYT?

>  }
>  
>  /* validate received token and create truncated hmac and nonce for SYN-ACK */

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

Re: [PATCH mptcp-next v4 9/9] mptcp: pm: add allow_new_subflow() interface

[Matthieu Baerts]

Hi Geliang,

On 24/03/2025 09:19, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> The helper mptcp_pm_is_userspace() is used to distinguish userspace PM
> operations from in-kernel PM in mptcp_pm_allow_new_subflow(). It seems
> reasonable to add a mandatory .allow_new_subflow interface for struct
> mptcp_pm_ops.
> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  include/net/mptcp.h   |  3 +++
>  net/mptcp/pm.c        | 13 ++-----------
>  net/mptcp/pm_kernel.c | 21 +++++++++++++++++++++
>  3 files changed, 26 insertions(+), 11 deletions(-)
> 
> diff --git a/include/net/mptcp.h b/include/net/mptcp.h
> index 8c1ac7368693..aedabc7f4190 100644
> --- a/include/net/mptcp.h
> +++ b/include/net/mptcp.h
> @@ -135,6 +135,9 @@ struct mptcp_pm_ops {
>  			      const struct mptcp_addr_info *addr);
>  	bool (*accept_new_subflow)(const struct mptcp_sock *msk);
>  
> +	/* optional */
> +	bool (*allow_new_subflow)(struct mptcp_sock *msk);

Maybe not needed, see my comment in patch 8/9.

> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index ca105bbd03ea..215b3a4d24be 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c
> @@ -453,23 +453,14 @@ void mptcp_pm_new_connection(struct mptcp_sock *msk, const struct sock *ssk, int
>  bool mptcp_pm_allow_new_subflow(struct mptcp_sock *msk)
>  {
>  	struct mptcp_pm_data *pm = &msk->pm;
> -	unsigned int subflows_max;
>  	bool ret = true;
>  
>  	if (!pm->ops->accept_new_subflow(msk))
>  		return false;
>  
>  	spin_lock_bh(&pm->lock);
> -	if (!mptcp_pm_is_userspace(msk) && READ_ONCE(pm->accept_subflow)) {
> -		subflows_max = mptcp_pm_get_subflows_max(msk);
> -
> -		pr_debug("msk=%p subflows=%d max=%d allow=%d\n", msk, pm->subflows,
> -			 subflows_max, READ_ONCE(pm->accept_subflow));
> -
> -		ret = pm->subflows < subflows_max;
> -		if (ret && pm->subflows == subflows_max - 1)
> -			WRITE_ONCE(pm->accept_subflow, false);
> -	}
> +	if (pm->ops->allow_new_subflow)
> +		ret = pm->ops->allow_new_subflow(msk);

From what I understood, callbacks should not be called under a lock. But
maybe this new callback is not needed, see my comment on patch 8/9.

>  	if (ret)
>  		pm->subflows++;
>  	spin_unlock_bh(&pm->lock);

(...)

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