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      |  2 ++
 net/mptcp/pm.c           | 36 +++---------------------------------
 net/mptcp/pm_kernel.c    | 27 +++++++++++++++++++++++++++
 net/mptcp/pm_userspace.c | 14 ++++++++++++++
 4 files changed, 46 insertions(+), 33 deletions(-)

diff --git a/include/net/mptcp.h b/include/net/mptcp.h
index 6a08ac862bbe..af995387248b 100644
--- a/include/net/mptcp.h
+++ b/include/net/mptcp.h
@@ -123,6 +123,8 @@ struct mptcp_pm_ops {
 	bool (*get_priority)(struct mptcp_sock *msk,
 			     struct mptcp_addr_info *skc);
 
+	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 ba7424582ebf..80293c823a3a 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -452,38 +452,7 @@ 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;
-	int ret = 0;
-
-	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;
-		}
-		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));
-
-	/* try to avoid acquiring the lock below */
-	if (!READ_ONCE(pm->accept_subflow))
-		return false;
-
-	spin_lock_bh(&pm->lock);
-	if (READ_ONCE(pm->accept_subflow)) {
-		ret = pm->subflows < subflows_max;
-		if (ret && ++pm->subflows == subflows_max)
-			WRITE_ONCE(pm->accept_subflow, false);
-	}
-	spin_unlock_bh(&pm->lock);
-
-	return ret;
+	return msk->pm.ops->allow_new_subflow(msk);
 }
 
 /* return true if the new status bit is currently cleared, that is, this event
@@ -1047,7 +1016,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->allow_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 7ec81d5195d4..af69f736c7c7 100644
--- a/net/mptcp/pm_kernel.c
+++ b/net/mptcp/pm_kernel.c
@@ -1399,6 +1399,32 @@ static struct pernet_operations mptcp_pm_pernet_ops = {
 	.size = sizeof(struct pm_nl_pernet),
 };
 
+static bool mptcp_pm_kernel_allow_new_subflow(struct mptcp_sock *msk)
+{
+	struct mptcp_pm_data *pm = &msk->pm;
+	unsigned int subflows_max;
+	int ret = 0;
+
+	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));
+
+	/* try to avoid acquiring the lock below */
+	if (!READ_ONCE(pm->accept_subflow))
+		return false;
+
+	spin_lock_bh(&pm->lock);
+	if (READ_ONCE(pm->accept_subflow)) {
+		ret = pm->subflows < subflows_max;
+		if (ret && ++pm->subflows == subflows_max)
+			WRITE_ONCE(pm->accept_subflow, false);
+	}
+	spin_unlock_bh(&pm->lock);
+
+	return ret;
+}
+
 static void mptcp_pm_kernel_init(struct mptcp_sock *msk)
 {
 	bool subflows_allowed = !!mptcp_pm_get_subflows_max(msk);
@@ -1422,6 +1448,7 @@ 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,
+	.allow_new_subflow	= mptcp_pm_kernel_allow_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 7fc19b844384..3a9962ac77b2 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -683,6 +683,19 @@ int mptcp_userspace_pm_get_addr(u8 id, struct mptcp_pm_addr_entry *addr,
 	return ret;
 }
 
+static bool mptcp_pm_userspace_allow_new_subflow(struct mptcp_sock *msk)
+{
+	struct mptcp_pm_data *pm = &msk->pm;
+
+	if (mptcp_userspace_pm_active(msk)) {
+		spin_lock_bh(&pm->lock);
+		pm->subflows++;
+		spin_unlock_bh(&pm->lock);
+		return true;
+	}
+	return false;
+}
+
 static void mptcp_pm_userspace_release(struct mptcp_sock *msk)
 {
 	mptcp_userspace_pm_free_local_addr_list(msk);
@@ -691,6 +704,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,
+	.allow_new_subflow	= mptcp_pm_userspace_allow_new_subflow,
 	.release		= mptcp_pm_userspace_release,
 	.name			= "userspace",
 	.owner			= THIS_MODULE,
-- 
2.43.0

Hi Geliang,

On 11/03/2025 10:32, 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      |  2 ++
>  net/mptcp/pm.c           | 36 +++---------------------------------
>  net/mptcp/pm_kernel.c    | 27 +++++++++++++++++++++++++++
>  net/mptcp/pm_userspace.c | 14 ++++++++++++++
>  4 files changed, 46 insertions(+), 33 deletions(-)
> 
> diff --git a/include/net/mptcp.h b/include/net/mptcp.h
> index 6a08ac862bbe..af995387248b 100644
> --- a/include/net/mptcp.h
> +++ b/include/net/mptcp.h
> @@ -123,6 +123,8 @@ struct mptcp_pm_ops {
>  	bool (*get_priority)(struct mptcp_sock *msk,
>  			     struct mptcp_addr_info *skc);
>  
> +	bool (*allow_new_subflow)(struct mptcp_sock *msk);

(When looking at "struct tcp_congestion_ops", I wonder if we should not
add a comment here to know if it is required or optional.)

(...)

> +static bool mptcp_pm_userspace_allow_new_subflow(struct mptcp_sock *msk)
> +{
> +	struct mptcp_pm_data *pm = &msk->pm;
> +
> +	if (mptcp_userspace_pm_active(msk)) {
> +		spin_lock_bh(&pm->lock);
> +		pm->subflows++;
> +		spin_unlock_bh(&pm->lock);

I wonder if it still makes sense to let each PM updating pm->subflows.
It sounds like it would be better to do that from pm.c, e.g. if this
callback returns true.

(Same when creating a new subflow, maybe something could be done there
with a new helper, but maybe not because different locks are needed...
Anyway, we can look at that later I suppose)

> +		return true;
> +	}
> +	return false;
> +}
> +
>  static void mptcp_pm_userspace_release(struct mptcp_sock *msk)
>  {
>  	mptcp_userspace_pm_free_local_addr_list(msk);
> @@ -691,6 +704,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,
> +	.allow_new_subflow	= mptcp_pm_userspace_allow_new_subflow,
>  	.release		= mptcp_pm_userspace_release,
>  	.name			= "userspace",
>  	.owner			= THIS_MODULE,

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