[PATCH mptcp-next v8 00/12] BPF path manager, part 5

[PATCH mptcp-next v8 00/12] BPF path manager, part 5

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

v8:
 - address Matt's comments in v7.

v7:
 - addresss Matt's comments in v6 [1].
 - drop "type" from struct mptcp_pm_ops as Matt suggested.
 - map "pm_type" to new sysctl as Matt suggested.

Depends on:
 - mptcp: pm: code reorganisation, v2

Based-on: <20250228-mptcp-pm-reorg-code-v2-0-fa8b2542b7a5@kernel.org>

[1]
https://patchwork.kernel.org/project/mptcp/cover/cover.1740320007.git.tanggeliang@kylinos.cn/

v6:
 - add "name" in struct mptcp_pm_ops.
 - add some "sysctl" patches.
 - drop "struct mptcp_pm_param".
 - drop "pm_type" in mptcp_pm_data.

v5:
 - use "struct mptcp_pm_param *param" as unified parameters for all
   interfaces.
 - register in-kernel mptcp_pm_ops too.
 - only implement two interfaces "get_local_id" and "get_priority" in
   this set.

v4:
 - include a new patch "define BPF path manager type".

 - add new interfaces:
	created established closed
	listerner_created listener_closed

 - rename interfaces as:
	address_announced address_removed
	subflow_established subflow_closed
	get_priority set_priority

 - rename functions as:
	mptcp_pm_validate
	mptcp_pm_register
	mptcp_pm_unregister
	mptcp_pm_initialize
	mptcp_pm_release

v3:
 - rename the 2nd parameter of get_local_id() from 'local' to 'skc'.
 - keep the 'msk_sport' check in mptcp_userspace_pm_get_local_id().
 - return 'err' instead of '0' in userspace_pm_subflow_create().
 - drop 'ret' variable inmptcp_pm_data_reset().
 - fix typos in commit log.

v2:
 - update get_local_id interface in patch 2.

get_addr() and dump_addr() interfaces of BPF userspace pm are dropped
as Matt suggested.

In order to implement BPF path manager, it's necessary to unify the
interfaces of the path manager. This set contains some cleanups and
refactoring to unify the interfaces in kernel space. Finally, define
a struct mptcp_pm_ops for a path manager.

Geliang Tang (12):
  mptcp: pm: define struct mptcp_pm_ops
  mptcp: sysctl: new sysctl to set path manager by name
  mptcp: sysctl: map pm_type to path_manager
  mptcp: sysctl: map path_manager to pm_type
  mptcp: sysctl: add available_path_managers
  mptcp: pm: in-kernel: register mptcp_pm_kernel
  mptcp: pm: userspace: register mptcp_pm_userspace
  mptcp: pm: initialize and release mptcp_pm_ops
  mptcp: pm: add get_local_id() interface
  mptcp: pm: add get_priority() interface
  selftests: mptcp: add pm_type mapping tests
  selftests: mptcp: add path_manager sysctl test

 Documentation/networking/mptcp-sysctl.rst     |  23 +++
 include/net/mptcp.h                           |  19 +++
 net/mptcp/ctrl.c                              | 112 +++++++++++++-
 net/mptcp/pm.c                                | 145 ++++++++++++++----
 net/mptcp/pm_kernel.c                         |  35 ++++-
 net/mptcp/pm_userspace.c                      |  36 ++++-
 net/mptcp/protocol.h                          |  18 ++-
 .../testing/selftests/net/mptcp/mptcp_join.sh |  49 +++++-
 8 files changed, 393 insertions(+), 44 deletions(-)

-- 
2.43.0

[PATCH mptcp-next v8 01/12] mptcp: pm: define struct mptcp_pm_ops

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

In order to allow users to develop their own BPF-based path manager,
this patch defines a struct ops "mptcp_pm_ops" for a userspace path
manager, which contains a set of interfaces.

Add a set of functions to register, unregister, find and validate a
given struct ops.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 include/net/mptcp.h  | 12 ++++++++++
 net/mptcp/pm.c       | 55 ++++++++++++++++++++++++++++++++++++++++++++
 net/mptcp/protocol.h |  5 ++++
 3 files changed, 72 insertions(+)

diff --git a/include/net/mptcp.h b/include/net/mptcp.h
index 72d6e6597add..aeb3b1e4d8f2 100644
--- a/include/net/mptcp.h
+++ b/include/net/mptcp.h
@@ -14,6 +14,7 @@
 
 struct mptcp_info;
 struct mptcp_sock;
+struct mptcp_pm_addr_entry;
 struct seq_file;
 
 /* MPTCP sk_buff extension data */
@@ -121,6 +122,17 @@ struct mptcp_sched_ops {
 	void (*release)(struct mptcp_sock *msk);
 } ____cacheline_aligned_in_smp;
 
+#define MPTCP_PM_NAME_MAX	16
+
+struct mptcp_pm_ops {
+	char			name[MPTCP_PM_NAME_MAX];
+	struct module		*owner;
+	struct list_head	list;
+
+	void (*init)(struct mptcp_sock *msk);
+	void (*release)(struct mptcp_sock *msk);
+} ____cacheline_aligned_in_smp;
+
 #ifdef CONFIG_MPTCP
 void mptcp_init(void);
 
diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index 04a156395aad..1af42c6a24c6 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -5,6 +5,8 @@
  */
 #define pr_fmt(fmt) "MPTCP: " fmt
 
+#include <linux/rculist.h>
+#include <linux/spinlock.h>
 #include "protocol.h"
 #include "mib.h"
 
@@ -18,6 +20,9 @@ struct mptcp_pm_add_entry {
 	struct mptcp_sock	*sock;
 };
 
+static DEFINE_SPINLOCK(mptcp_pm_list_lock);
+static LIST_HEAD(mptcp_pm_list);
+
 /* path manager helpers */
 
 /* if sk is ipv4 or ipv6_only allows only same-family local and remote addresses,
@@ -1025,3 +1030,53 @@ void __init mptcp_pm_init(void)
 	mptcp_pm_kernel_register();
 	mptcp_pm_nl_init();
 }
+
+/* Must be called with rcu read lock held */
+struct mptcp_pm_ops *mptcp_pm_find(const char *name)
+{
+	struct mptcp_pm_ops *pm;
+
+	list_for_each_entry_rcu(pm, &mptcp_pm_list, list) {
+		if (!strcmp(pm->name, name))
+			return pm;
+	}
+
+	return NULL;
+}
+
+int mptcp_pm_validate(struct mptcp_pm_ops *pm)
+{
+	if (!pm->init) {
+		pr_err("%s does not implement required ops\n", pm->name);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+int mptcp_pm_register(struct mptcp_pm_ops *pm)
+{
+	int ret;
+
+	ret = mptcp_pm_validate(pm);
+	if (ret)
+		return ret;
+
+	spin_lock(&mptcp_pm_list_lock);
+	if (mptcp_pm_find(pm->name)) {
+		spin_unlock(&mptcp_pm_list_lock);
+		return -EEXIST;
+	}
+	list_add_tail_rcu(&pm->list, &mptcp_pm_list);
+	spin_unlock(&mptcp_pm_list_lock);
+
+	pr_debug("%s registered\n", pm->name);
+	return 0;
+}
+
+void mptcp_pm_unregister(struct mptcp_pm_ops *pm)
+{
+	spin_lock(&mptcp_pm_list_lock);
+	list_del_rcu(&pm->list);
+	spin_unlock(&mptcp_pm_list_lock);
+}
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 99b848de5229..e1cd69d376d1 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -1049,6 +1049,11 @@ int mptcp_pm_remove_addr(struct mptcp_sock *msk, const struct mptcp_rm_list *rm_
 void mptcp_pm_remove_addr_entry(struct mptcp_sock *msk,
 				struct mptcp_pm_addr_entry *entry);
 
+struct mptcp_pm_ops *mptcp_pm_find(const char *name);
+int mptcp_pm_validate(struct mptcp_pm_ops *pm);
+int mptcp_pm_register(struct mptcp_pm_ops *pm);
+void mptcp_pm_unregister(struct mptcp_pm_ops *pm);
+
 void mptcp_userspace_pm_free_local_addr_list(struct mptcp_sock *msk);
 
 void mptcp_event(enum mptcp_event_type type, const struct mptcp_sock *msk,
-- 
2.43.0

Re: [PATCH mptcp-next v8 01/12] mptcp: pm: define struct mptcp_pm_ops

[Matthieu Baerts]

Hi Geliang,

On 04/03/2025 12:40, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> In order to allow users to develop their own BPF-based path manager,
> this patch defines a struct ops "mptcp_pm_ops" for a userspace path
> manager, which contains a set of interfaces.
> 
> Add a set of functions to register, unregister, find and validate a
> given struct ops.

(...)

> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index 04a156395aad..1af42c6a24c6 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c

(...)

> +int mptcp_pm_validate(struct mptcp_pm_ops *pm)
> +{
> +	if (!pm->init) {

Maybe the init part should not be mandatory, see my comment in patch 7/12.

(...)

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

[PATCH mptcp-next v8 02/12] mptcp: sysctl: new sysctl to set path manager by name

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

A new net.mptcp.path_manager sysctl is added to determine which path
manager will be used by each newly-created MPTCP socket by setting the
name of it.

This sysctl makes the old one "pm_type" deprecated.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 Documentation/networking/mptcp-sysctl.rst | 19 +++++++++
 net/mptcp/ctrl.c                          | 50 +++++++++++++++++++++++
 net/mptcp/protocol.h                      |  1 +
 3 files changed, 70 insertions(+)

diff --git a/Documentation/networking/mptcp-sysctl.rst b/Documentation/networking/mptcp-sysctl.rst
index 03e1d3610333..b78a2254d452 100644
--- a/Documentation/networking/mptcp-sysctl.rst
+++ b/Documentation/networking/mptcp-sysctl.rst
@@ -72,6 +72,23 @@ enabled - BOOLEAN
 
 	Default: 1 (enabled)
 
+path_manager - STRING
+	Set the default path manager name to use for each new MPTCP
+	socket. In-kernel path management will control subflow
+	connections and address advertisements according to
+	per-namespace values configured over the MPTCP netlink
+	API. Userspace path management puts per-MPTCP-connection subflow
+	connection decisions and address advertisements under control of
+	a privileged userspace program, at the cost of more netlink
+	traffic to propagate all of the related events and commands.
+
+	This is a per-namespace sysctl.
+
+	* "kernel"          - In-kernel path manager
+	* "userspace"       - Userspace path manager
+
+	Default: "kernel"
+
 pm_type - INTEGER
 	Set the default path manager type to use for each new MPTCP
 	socket. In-kernel path management will control subflow
@@ -84,6 +101,8 @@ pm_type - INTEGER
 
 	This is a per-namespace sysctl.
 
+	Deprecated since v6.15, use path_manager instead.
+
 	* 0 - In-kernel path manager
 	* 1 - Userspace path manager
 
diff --git a/net/mptcp/ctrl.c b/net/mptcp/ctrl.c
index be6c0237e10b..d64e6b4f6d1d 100644
--- a/net/mptcp/ctrl.c
+++ b/net/mptcp/ctrl.c
@@ -39,6 +39,7 @@ struct mptcp_pernet {
 	u8 allow_join_initial_addr_port;
 	u8 pm_type;
 	char scheduler[MPTCP_SCHED_NAME_MAX];
+	char path_manager[MPTCP_PM_NAME_MAX];
 };
 
 static struct mptcp_pernet *mptcp_get_pernet(const struct net *net)
@@ -83,6 +84,11 @@ int mptcp_get_pm_type(const struct net *net)
 	return mptcp_get_pernet(net)->pm_type;
 }
 
+const char *mptcp_get_path_manager(const struct net *net)
+{
+	return mptcp_get_pernet(net)->path_manager;
+}
+
 const char *mptcp_get_scheduler(const struct net *net)
 {
 	return mptcp_get_pernet(net)->scheduler;
@@ -101,6 +107,7 @@ static void mptcp_pernet_set_defaults(struct mptcp_pernet *pernet)
 	pernet->stale_loss_cnt = 4;
 	pernet->pm_type = MPTCP_PM_TYPE_KERNEL;
 	strscpy(pernet->scheduler, "default", sizeof(pernet->scheduler));
+	strscpy(pernet->path_manager, "kernel", sizeof(pernet->path_manager));
 }
 
 #ifdef CONFIG_SYSCTL
@@ -174,6 +181,42 @@ static int proc_blackhole_detect_timeout(const struct ctl_table *table,
 	return ret;
 }
 
+static int mptcp_set_path_manager(char *path_manager, const char *name)
+{
+	struct mptcp_pm_ops *pm;
+	int ret = 0;
+
+	rcu_read_lock();
+	pm = mptcp_pm_find(name);
+	if (pm)
+		strscpy(path_manager, name, MPTCP_PM_NAME_MAX);
+	else
+		ret = -ENOENT;
+	rcu_read_unlock();
+
+	return ret;
+}
+
+static int proc_path_manager(const struct ctl_table *ctl, int write,
+			     void *buffer, size_t *lenp, loff_t *ppos)
+{
+	char (*path_manager)[MPTCP_PM_NAME_MAX] = ctl->data;
+	char val[MPTCP_PM_NAME_MAX];
+	const struct ctl_table tbl = {
+		.data = val,
+		.maxlen = MPTCP_PM_NAME_MAX,
+	};
+	int ret;
+
+	strscpy(val, *path_manager, MPTCP_PM_NAME_MAX);
+
+	ret = proc_dostring(&tbl, write, buffer, lenp, ppos);
+	if (write && ret == 0)
+		ret = mptcp_set_path_manager(*path_manager, val);
+
+	return ret;
+}
+
 static struct ctl_table mptcp_sysctl_table[] = {
 	{
 		.procname = "enabled",
@@ -253,6 +296,12 @@ static struct ctl_table mptcp_sysctl_table[] = {
 		.mode = 0644,
 		.proc_handler = proc_dou8vec_minmax,
 	},
+	{
+		.procname = "path_manager",
+		.maxlen	= MPTCP_PM_NAME_MAX,
+		.mode = 0644,
+		.proc_handler = proc_path_manager,
+	},
 };
 
 static int mptcp_pernet_new_table(struct net *net, struct mptcp_pernet *pernet)
@@ -278,6 +327,7 @@ static int mptcp_pernet_new_table(struct net *net, struct mptcp_pernet *pernet)
 	table[8].data = &pernet->close_timeout;
 	table[9].data = &pernet->blackhole_timeout;
 	table[10].data = &pernet->syn_retrans_before_tcp_fallback;
+	table[11].data = &pernet->path_manager;
 
 	hdr = register_net_sysctl_sz(net, MPTCP_SYSCTL_PATH, table,
 				     ARRAY_SIZE(mptcp_sysctl_table));
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index e1cd69d376d1..ff14d43bf8a9 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -694,6 +694,7 @@ int mptcp_allow_join_id0(const struct net *net);
 unsigned int mptcp_stale_loss_cnt(const struct net *net);
 unsigned int mptcp_close_timeout(const struct sock *sk);
 int mptcp_get_pm_type(const struct net *net);
+const char *mptcp_get_path_manager(const struct net *net);
 const char *mptcp_get_scheduler(const struct net *net);
 
 void mptcp_active_disable(struct sock *sk);
-- 
2.43.0

[PATCH mptcp-next v8 03/12] mptcp: sysctl: map pm_type to path_manager

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

This patch adds a new proc_handler "proc_pm_type" for "pm_type" to
map old path manager sysctl "pm_type" to the newly added "path_manager".

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

diff --git a/net/mptcp/ctrl.c b/net/mptcp/ctrl.c
index d64e6b4f6d1d..d425fcbd036a 100644
--- a/net/mptcp/ctrl.c
+++ b/net/mptcp/ctrl.c
@@ -217,6 +217,32 @@ static int proc_path_manager(const struct ctl_table *ctl, int write,
 	return ret;
 }
 
+static int proc_pm_type(const struct ctl_table *ctl, int write,
+			void *buffer, size_t *lenp, loff_t *ppos)
+{
+	struct mptcp_pernet *pernet = container_of(ctl->data,
+						   struct mptcp_pernet,
+						   pm_type);
+	u8 pm_type = READ_ONCE(*(u8 *)ctl->data);
+	const struct ctl_table tbl = {
+		.maxlen = sizeof(pm_type),
+		.data = &pm_type,
+	};
+	int ret;
+
+	ret = proc_dou8vec_minmax(&tbl, write, buffer, lenp, ppos);
+	if (write && ret == 0) {
+		char *path_manager = "kernel";
+
+		if (pm_type == MPTCP_PM_TYPE_USERSPACE)
+			path_manager = "userspace";
+		mptcp_set_path_manager(pernet->path_manager, path_manager);
+		WRITE_ONCE(*(u8 *)ctl->data, pm_type);
+	}
+
+	return ret;
+}
+
 static struct ctl_table mptcp_sysctl_table[] = {
 	{
 		.procname = "enabled",
@@ -261,7 +287,7 @@ static struct ctl_table mptcp_sysctl_table[] = {
 		.procname = "pm_type",
 		.maxlen = sizeof(u8),
 		.mode = 0644,
-		.proc_handler = proc_dou8vec_minmax,
+		.proc_handler = proc_pm_type,
 		.extra1       = SYSCTL_ZERO,
 		.extra2       = &mptcp_pm_type_max
 	},
-- 
2.43.0

Re: [PATCH mptcp-next v8 03/12] mptcp: sysctl: map pm_type to path_manager

[Matthieu Baerts]

Hi Geliang,

On 04/03/2025 12:40, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> This patch adds a new proc_handler "proc_pm_type" for "pm_type" to
> map old path manager sysctl "pm_type" to the newly added "path_manager".
> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  net/mptcp/ctrl.c | 28 +++++++++++++++++++++++++++-
>  1 file changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/net/mptcp/ctrl.c b/net/mptcp/ctrl.c
> index d64e6b4f6d1d..d425fcbd036a 100644
> --- a/net/mptcp/ctrl.c
> +++ b/net/mptcp/ctrl.c
> @@ -217,6 +217,32 @@ static int proc_path_manager(const struct ctl_table *ctl, int write,
>  	return ret;
>  }
>  
> +static int proc_pm_type(const struct ctl_table *ctl, int write,
> +			void *buffer, size_t *lenp, loff_t *ppos)
> +{
> +	struct mptcp_pernet *pernet = container_of(ctl->data,
> +						   struct mptcp_pernet,
> +						   pm_type);
> +	u8 pm_type = READ_ONCE(*(u8 *)ctl->data);
> +	const struct ctl_table tbl = {
> +		.maxlen = sizeof(pm_type),
> +		.data = &pm_type,
> +	};
> +	int ret;
> +
> +	ret = proc_dou8vec_minmax(&tbl, write, buffer, lenp, ppos);


I missed that in my previous review. Do you need tbl here? Can you not
use "ctl" here instead? If you need tbl, you will need to move the
"extra[12]" fields there I suppose, otherwise the limits will not be
checked, no?

In the proc_handler, a new ctl_table structure is needed when it is
required to have a way to prevent the writing of the data after having
called proc_do(...). Here, that's not the case: if the new value is
between 0 and mptcp_pm_type_max, we will always write ctl->data.

In other words, I think you can remove tlb, use ctl instead, and remove
the 'WRITE_ONCE(*(u8 *)ctl->data, pm_type)' here below.

> +	if (write && ret == 0) {
> +		char *path_manager = "kernel";
> +
> +		if (pm_type == MPTCP_PM_TYPE_USERSPACE)

pm_type should be read here in the 'if' statement, after having
potentially be modified in proc_dou8vec_minmax(). Or use "buffer" directly.

> +			path_manager = "userspace";
> +		mptcp_set_path_manager(pernet->path_manager, path_manager);
> +		WRITE_ONCE(*(u8 *)ctl->data, pm_type);
> +	}
> +
> +	return ret;
> +}
> +
>  static struct ctl_table mptcp_sysctl_table[] = {
>  	{
>  		.procname = "enabled",
Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.

[PATCH mptcp-next v8 04/12] mptcp: sysctl: map path_manager to pm_type

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

This patch maps the newly added path manager sysctl "path_manager"
to the old one "pm_type".

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

diff --git a/net/mptcp/ctrl.c b/net/mptcp/ctrl.c
index d425fcbd036a..a158a337cdb5 100644
--- a/net/mptcp/ctrl.c
+++ b/net/mptcp/ctrl.c
@@ -200,6 +200,9 @@ static int mptcp_set_path_manager(char *path_manager, const char *name)
 static int proc_path_manager(const struct ctl_table *ctl, int write,
 			     void *buffer, size_t *lenp, loff_t *ppos)
 {
+	struct mptcp_pernet *pernet = container_of(ctl->data,
+						   struct mptcp_pernet,
+						   path_manager);
 	char (*path_manager)[MPTCP_PM_NAME_MAX] = ctl->data;
 	char val[MPTCP_PM_NAME_MAX];
 	const struct ctl_table tbl = {
@@ -211,8 +214,14 @@ static int proc_path_manager(const struct ctl_table *ctl, int write,
 	strscpy(val, *path_manager, MPTCP_PM_NAME_MAX);
 
 	ret = proc_dostring(&tbl, write, buffer, lenp, ppos);
-	if (write && ret == 0)
+	if (write && ret == 0) {
+		u8 pm_type = MPTCP_PM_TYPE_KERNEL;
+
+		if (!strncmp(val, "userspace", MPTCP_PM_NAME_MAX))
+			pm_type = MPTCP_PM_TYPE_USERSPACE;
+		pernet->pm_type = pm_type;
 		ret = mptcp_set_path_manager(*path_manager, val);
+	}
 
 	return ret;
 }
-- 
2.43.0

Re: [PATCH mptcp-next v8 04/12] mptcp: sysctl: map path_manager to pm_type

[Matthieu Baerts]

Hi Geliang,

On 04/03/2025 12:40, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> This patch maps the newly added path manager sysctl "path_manager"
> to the old one "pm_type".
> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  net/mptcp/ctrl.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/net/mptcp/ctrl.c b/net/mptcp/ctrl.c
> index d425fcbd036a..a158a337cdb5 100644
> --- a/net/mptcp/ctrl.c
> +++ b/net/mptcp/ctrl.c
> @@ -200,6 +200,9 @@ static int mptcp_set_path_manager(char *path_manager, const char *name)
>  static int proc_path_manager(const struct ctl_table *ctl, int write,
>  			     void *buffer, size_t *lenp, loff_t *ppos)
>  {
> +	struct mptcp_pernet *pernet = container_of(ctl->data,
> +						   struct mptcp_pernet,
> +						   path_manager);
>  	char (*path_manager)[MPTCP_PM_NAME_MAX] = ctl->data;
>  	char val[MPTCP_PM_NAME_MAX];
>  	const struct ctl_table tbl = {
> @@ -211,8 +214,14 @@ static int proc_path_manager(const struct ctl_table *ctl, int write,
>  	strscpy(val, *path_manager, MPTCP_PM_NAME_MAX);
>  
>  	ret = proc_dostring(&tbl, write, buffer, lenp, ppos);
> -	if (write && ret == 0)
> +	if (write && ret == 0) {
> +		u8 pm_type = MPTCP_PM_TYPE_KERNEL;
> +
> +		if (!strncmp(val, "userspace", MPTCP_PM_NAME_MAX))

(detail: please use '== 0', that feels more natural than "not strncmp")

> +			pm_type = MPTCP_PM_TYPE_USERSPACE;
> +		pernet->pm_type = pm_type;

Should we not already cope with the future BPF PMs?

  if "kernel":
    pm_type = MPTCP_PM_TYPE_KERNEL;
  elif "userspace:
    pm_type = MPTCP_PM_TYPE_USERSPACE;
  else:
    pm_type = __MPTCP_PM_TYPE_NR;

Please check that this doesn't cause issue to display "pm_type" to 2
here -- note that we should not change the current mptcp_pm_type_max:
the userspace **cannot** set "net.mptcp.pm_type=2".

In other words, please check that this is OK:

  # sysctl net.mptcp.pm_type=2
  sysctl: setting key "net.mptcp.pm_type": Invalid argument
  # sysctl -q net.mptcp.path_manager = "$BPF_PM"
  # sysctl -n net.mptcp.pm_type
  2

>  		ret = mptcp_set_path_manager(*path_manager, val);
> +	}
>  
>  	return ret;
>  }

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

[PATCH mptcp-next v8 05/12] mptcp: sysctl: add available_path_managers

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

Similarly to net.mptcp.available_schedulers, this patch adds a new one
net.mptcp.available_path_managers to list the available path managers.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 Documentation/networking/mptcp-sysctl.rst |  4 ++++
 include/net/mptcp.h                       |  2 ++
 net/mptcp/ctrl.c                          | 25 +++++++++++++++++++++++
 net/mptcp/pm.c                            | 19 +++++++++++++++++
 net/mptcp/protocol.h                      |  1 +
 5 files changed, 51 insertions(+)

diff --git a/Documentation/networking/mptcp-sysctl.rst b/Documentation/networking/mptcp-sysctl.rst
index b78a2254d452..5bfab01eff5a 100644
--- a/Documentation/networking/mptcp-sysctl.rst
+++ b/Documentation/networking/mptcp-sysctl.rst
@@ -30,6 +30,10 @@ allow_join_initial_addr_port - BOOLEAN
 
 	Default: 1
 
+available_path_managers - STRING
+	Shows the available path managers choices that are registered. More
+	path managers may be available, but not loaded.
+
 available_schedulers - STRING
 	Shows the available schedulers choices that are registered. More packet
 	schedulers may be available, but not loaded.
diff --git a/include/net/mptcp.h b/include/net/mptcp.h
index aeb3b1e4d8f2..c53ffff0a4cd 100644
--- a/include/net/mptcp.h
+++ b/include/net/mptcp.h
@@ -123,6 +123,8 @@ struct mptcp_sched_ops {
 } ____cacheline_aligned_in_smp;
 
 #define MPTCP_PM_NAME_MAX	16
+#define MPTCP_PM_MAX		128
+#define MPTCP_PM_BUF_MAX	(MPTCP_PM_NAME_MAX * MPTCP_PM_MAX)
 
 struct mptcp_pm_ops {
 	char			name[MPTCP_PM_NAME_MAX];
diff --git a/net/mptcp/ctrl.c b/net/mptcp/ctrl.c
index a158a337cdb5..c11edcb683d0 100644
--- a/net/mptcp/ctrl.c
+++ b/net/mptcp/ctrl.c
@@ -252,6 +252,24 @@ static int proc_pm_type(const struct ctl_table *ctl, int write,
 	return ret;
 }
 
+static int proc_available_path_managers(const struct ctl_table *ctl,
+					int write, void *buffer,
+					size_t *lenp, loff_t *ppos)
+{
+	struct ctl_table tbl = { .maxlen = MPTCP_PM_BUF_MAX, };
+	int ret;
+
+	tbl.data = kmalloc(tbl.maxlen, GFP_USER);
+	if (!tbl.data)
+		return -ENOMEM;
+
+	mptcp_pm_get_available(tbl.data, MPTCP_PM_BUF_MAX);
+	ret = proc_dostring(&tbl, write, buffer, lenp, ppos);
+	kfree(tbl.data);
+
+	return ret;
+}
+
 static struct ctl_table mptcp_sysctl_table[] = {
 	{
 		.procname = "enabled",
@@ -337,6 +355,12 @@ static struct ctl_table mptcp_sysctl_table[] = {
 		.mode = 0644,
 		.proc_handler = proc_path_manager,
 	},
+	{
+		.procname = "available_path_managers",
+		.maxlen	= MPTCP_PM_BUF_MAX,
+		.mode = 0444,
+		.proc_handler = proc_available_path_managers,
+	},
 };
 
 static int mptcp_pernet_new_table(struct net *net, struct mptcp_pernet *pernet)
@@ -363,6 +387,7 @@ static int mptcp_pernet_new_table(struct net *net, struct mptcp_pernet *pernet)
 	table[9].data = &pernet->blackhole_timeout;
 	table[10].data = &pernet->syn_retrans_before_tcp_fallback;
 	table[11].data = &pernet->path_manager;
+	/* table[12] is for available_path_managers which is read-only info */
 
 	hdr = register_net_sysctl_sz(net, MPTCP_SYSCTL_PATH, table,
 				     ARRAY_SIZE(mptcp_sysctl_table));
diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index 1af42c6a24c6..a2b210873b23 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -1080,3 +1080,22 @@ void mptcp_pm_unregister(struct mptcp_pm_ops *pm)
 	list_del_rcu(&pm->list);
 	spin_unlock(&mptcp_pm_list_lock);
 }
+
+/* Build string with list of available path manager values.
+ * Similar to tcp_get_available_congestion_control()
+ */
+void mptcp_pm_get_available(char *buf, size_t maxlen)
+{
+	struct mptcp_pm_ops *pm;
+	size_t offs = 0;
+
+	rcu_read_lock();
+	list_for_each_entry_rcu(pm, &mptcp_pm_list, list) {
+		offs += snprintf(buf + offs, maxlen - offs, "%s%s",
+				 offs == 0 ? "" : " ", pm->name);
+
+		if (WARN_ON_ONCE(offs >= maxlen))
+			break;
+	}
+	rcu_read_unlock();
+}
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index ff14d43bf8a9..246b44db9775 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -1054,6 +1054,7 @@ struct mptcp_pm_ops *mptcp_pm_find(const char *name);
 int mptcp_pm_validate(struct mptcp_pm_ops *pm);
 int mptcp_pm_register(struct mptcp_pm_ops *pm);
 void mptcp_pm_unregister(struct mptcp_pm_ops *pm);
+void mptcp_pm_get_available(char *buf, size_t maxlen);
 
 void mptcp_userspace_pm_free_local_addr_list(struct mptcp_sock *msk);
 
-- 
2.43.0

[PATCH mptcp-next v8 06/12] mptcp: pm: in-kernel: register mptcp_pm_kernel

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

This patch defines the original in-kernel netlink path manager as a
new struct mptcp_pm_ops named "mptcp_pm_kernel", and register it in
mptcp_pm_kernel_register().

This mptcp_pm_ops will be skipped in mptcp_pm_unregister().

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 net/mptcp/pm.c        |  4 ++++
 net/mptcp/pm_kernel.c | 26 ++++++++++++++++++++++++++
 net/mptcp/protocol.h  |  3 +++
 3 files changed, 33 insertions(+)

diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index a2b210873b23..28ea8bdaa8b0 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -1076,6 +1076,10 @@ int mptcp_pm_register(struct mptcp_pm_ops *pm)
 
 void mptcp_pm_unregister(struct mptcp_pm_ops *pm)
 {
+	/* skip unregistering the default path manager */
+	if (pm == &mptcp_pm_kernel)
+		return;
+
 	spin_lock(&mptcp_pm_list_lock);
 	list_del_rcu(&pm->list);
 	spin_unlock(&mptcp_pm_list_lock);
diff --git a/net/mptcp/pm_kernel.c b/net/mptcp/pm_kernel.c
index 806a9b5b3c07..e6a1aef738a8 100644
--- a/net/mptcp/pm_kernel.c
+++ b/net/mptcp/pm_kernel.c
@@ -1398,8 +1398,34 @@ static struct pernet_operations mptcp_pm_pernet_ops = {
 	.size = sizeof(struct pm_nl_pernet),
 };
 
+static void mptcp_pm_nl_initialize(struct mptcp_sock *msk)
+{
+	bool subflows_allowed = !!mptcp_pm_get_subflows_max(msk);
+	struct mptcp_pm_data *pm = &msk->pm;
+
+	/* pm->work_pending must be only be set to 'true' when
+	 * pm is the default path manager
+	 */
+	WRITE_ONCE(pm->work_pending,
+		   (!!mptcp_pm_get_local_addr_max(msk) &&
+		    subflows_allowed) ||
+		   !!mptcp_pm_get_add_addr_signal_max(msk));
+	WRITE_ONCE(pm->accept_addr,
+		   !!mptcp_pm_get_add_addr_accept_max(msk) &&
+		   subflows_allowed);
+	WRITE_ONCE(pm->accept_subflow, subflows_allowed);
+}
+
+struct mptcp_pm_ops mptcp_pm_kernel = {
+	.init			= mptcp_pm_nl_initialize,
+	.name			= "kernel",
+	.owner			= THIS_MODULE,
+};
+
 void __init mptcp_pm_kernel_register(void)
 {
 	if (register_pernet_subsys(&mptcp_pm_pernet_ops) < 0)
 		panic("Failed to register MPTCP PM pernet subsystem.\n");
+
+	mptcp_pm_register(&mptcp_pm_kernel);
 }
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 246b44db9775..f700cb55bf49 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -1050,6 +1050,9 @@ int mptcp_pm_remove_addr(struct mptcp_sock *msk, const struct mptcp_rm_list *rm_
 void mptcp_pm_remove_addr_entry(struct mptcp_sock *msk,
 				struct mptcp_pm_addr_entry *entry);
 
+/* the default path manager, used in mptcp_pm_unregister */
+extern struct mptcp_pm_ops mptcp_pm_kernel;
+
 struct mptcp_pm_ops *mptcp_pm_find(const char *name);
 int mptcp_pm_validate(struct mptcp_pm_ops *pm);
 int mptcp_pm_register(struct mptcp_pm_ops *pm);
-- 
2.43.0

Re: [PATCH mptcp-next v8 06/12] mptcp: pm: in-kernel: register mptcp_pm_kernel

[Geliang Tang]

Hi Matt,

On Tue, 2025-03-04 at 19:40 +0800, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> This patch defines the original in-kernel netlink path manager as a
> new struct mptcp_pm_ops named "mptcp_pm_kernel", and register it in
> mptcp_pm_kernel_register().
> 
> This mptcp_pm_ops will be skipped in mptcp_pm_unregister().
> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  net/mptcp/pm.c        |  4 ++++
>  net/mptcp/pm_kernel.c | 26 ++++++++++++++++++++++++++
>  net/mptcp/protocol.h  |  3 +++
>  3 files changed, 33 insertions(+)
> 
> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index a2b210873b23..28ea8bdaa8b0 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c
> @@ -1076,6 +1076,10 @@ int mptcp_pm_register(struct mptcp_pm_ops *pm)
>  
>  void mptcp_pm_unregister(struct mptcp_pm_ops *pm)
>  {
> +	/* skip unregistering the default path manager */
> +	if (pm == &mptcp_pm_kernel)
> +		return;
> +
>  	spin_lock(&mptcp_pm_list_lock);
>  	list_del_rcu(&pm->list);
>  	spin_unlock(&mptcp_pm_list_lock);
> diff --git a/net/mptcp/pm_kernel.c b/net/mptcp/pm_kernel.c
> index 806a9b5b3c07..e6a1aef738a8 100644
> --- a/net/mptcp/pm_kernel.c
> +++ b/net/mptcp/pm_kernel.c
> @@ -1398,8 +1398,34 @@ static struct pernet_operations
> mptcp_pm_pernet_ops = {
>  	.size = sizeof(struct pm_nl_pernet),
>  };
>  
> +static void mptcp_pm_nl_initialize(struct mptcp_sock *msk)
> +{
> +	bool subflows_allowed = !!mptcp_pm_get_subflows_max(msk);
> +	struct mptcp_pm_data *pm = &msk->pm;
> +
> +	/* pm->work_pending must be only be set to 'true' when
> +	 * pm is the default path manager
> +	 */
> +	WRITE_ONCE(pm->work_pending,
> +		   (!!mptcp_pm_get_local_addr_max(msk) &&
> +		    subflows_allowed) ||
> +		   !!mptcp_pm_get_add_addr_signal_max(msk));
> +	WRITE_ONCE(pm->accept_addr,
> +		   !!mptcp_pm_get_add_addr_accept_max(msk) &&
> +		   subflows_allowed);
> +	WRITE_ONCE(pm->accept_subflow, subflows_allowed);
> +}
> +
> +struct mptcp_pm_ops mptcp_pm_kernel = {
> +	.init			= mptcp_pm_nl_initialize,

A better name would be mptcp_pm_nl_init, but this is already used in
pm_netlink.c. I would like to rename mptcp_pm_nl_init in pm_netlink.c
to mptcp_pm_genl_init, do you think this is a good idea?

If so, is it better to squash this renaming into "mptcp: pm: split
netlink and in-kernel init" or make it a separate patch?

Thanks,
-Geliang

> +	.name			= "kernel",
> +	.owner			= THIS_MODULE,
> +};
> +
>  void __init mptcp_pm_kernel_register(void)
>  {
>  	if (register_pernet_subsys(&mptcp_pm_pernet_ops) < 0)
>  		panic("Failed to register MPTCP PM pernet
> subsystem.\n");
> +
> +	mptcp_pm_register(&mptcp_pm_kernel);
>  }
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index 246b44db9775..f700cb55bf49 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -1050,6 +1050,9 @@ int mptcp_pm_remove_addr(struct mptcp_sock
> *msk, const struct mptcp_rm_list *rm_
>  void mptcp_pm_remove_addr_entry(struct mptcp_sock *msk,
>  				struct mptcp_pm_addr_entry *entry);
>  
> +/* the default path manager, used in mptcp_pm_unregister */
> +extern struct mptcp_pm_ops mptcp_pm_kernel;
> +
>  struct mptcp_pm_ops *mptcp_pm_find(const char *name);
>  int mptcp_pm_validate(struct mptcp_pm_ops *pm);
>  int mptcp_pm_register(struct mptcp_pm_ops *pm);

Re: [PATCH mptcp-next v8 06/12] mptcp: pm: in-kernel: register mptcp_pm_kernel

[Matthieu Baerts]

Hi Geliang,

On 05/03/2025 02:35, Geliang Tang wrote:
> Hi Matt,
> 
> On Tue, 2025-03-04 at 19:40 +0800, Geliang Tang wrote:
>> From: Geliang Tang <tanggeliang@kylinos.cn>
>>
>> This patch defines the original in-kernel netlink path manager as a
>> new struct mptcp_pm_ops named "mptcp_pm_kernel", and register it in
>> mptcp_pm_kernel_register().
>>
>> This mptcp_pm_ops will be skipped in mptcp_pm_unregister().
>>
>> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
>> ---
>>  net/mptcp/pm.c        |  4 ++++
>>  net/mptcp/pm_kernel.c | 26 ++++++++++++++++++++++++++
>>  net/mptcp/protocol.h  |  3 +++
>>  3 files changed, 33 insertions(+)
>>
>> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
>> index a2b210873b23..28ea8bdaa8b0 100644
>> --- a/net/mptcp/pm.c
>> +++ b/net/mptcp/pm.c
>> @@ -1076,6 +1076,10 @@ int mptcp_pm_register(struct mptcp_pm_ops *pm)
>>  
>>  void mptcp_pm_unregister(struct mptcp_pm_ops *pm)
>>  {
>> +	/* skip unregistering the default path manager */
>> +	if (pm == &mptcp_pm_kernel)
>> +		return;
>> +
>>  	spin_lock(&mptcp_pm_list_lock);
>>  	list_del_rcu(&pm->list);
>>  	spin_unlock(&mptcp_pm_list_lock);
>> diff --git a/net/mptcp/pm_kernel.c b/net/mptcp/pm_kernel.c
>> index 806a9b5b3c07..e6a1aef738a8 100644
>> --- a/net/mptcp/pm_kernel.c
>> +++ b/net/mptcp/pm_kernel.c
>> @@ -1398,8 +1398,34 @@ static struct pernet_operations
>> mptcp_pm_pernet_ops = {
>>  	.size = sizeof(struct pm_nl_pernet),
>>  };
>>  
>> +static void mptcp_pm_nl_initialize(struct mptcp_sock *msk)
>> +{
>> +	bool subflows_allowed = !!mptcp_pm_get_subflows_max(msk);
>> +	struct mptcp_pm_data *pm = &msk->pm;
>> +
>> +	/* pm->work_pending must be only be set to 'true' when
>> +	 * pm is the default path manager
>> +	 */
>> +	WRITE_ONCE(pm->work_pending,
>> +		   (!!mptcp_pm_get_local_addr_max(msk) &&
>> +		    subflows_allowed) ||
>> +		   !!mptcp_pm_get_add_addr_signal_max(msk));
>> +	WRITE_ONCE(pm->accept_addr,
>> +		   !!mptcp_pm_get_add_addr_accept_max(msk) &&
>> +		   subflows_allowed);
>> +	WRITE_ONCE(pm->accept_subflow, subflows_allowed);
>> +}
>> +
>> +struct mptcp_pm_ops mptcp_pm_kernel = {
>> +	.init			= mptcp_pm_nl_initialize,
> 
> A better name would be mptcp_pm_nl_init, but this is already used in
> pm_netlink.c. I would like to rename mptcp_pm_nl_init in pm_netlink.c
> to mptcp_pm_genl_init, do you think this is a good idea?
> 
> If so, is it better to squash this renaming into "mptcp: pm: split
> netlink and in-kernel init" or make it a separate patch?

What about calling the one here mptcp_pm_kernel_init()?

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

Re: [PATCH mptcp-next v8 06/12] mptcp: pm: in-kernel: register mptcp_pm_kernel

[Geliang Tang]

On Wed, 2025-03-05 at 10:11 +0100, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 05/03/2025 02:35, Geliang Tang wrote:
> > Hi Matt,
> > 
> > On Tue, 2025-03-04 at 19:40 +0800, Geliang Tang wrote:
> > > From: Geliang Tang <tanggeliang@kylinos.cn>
> > > 
> > > This patch defines the original in-kernel netlink path manager as
> > > a
> > > new struct mptcp_pm_ops named "mptcp_pm_kernel", and register it
> > > in
> > > mptcp_pm_kernel_register().
> > > 
> > > This mptcp_pm_ops will be skipped in mptcp_pm_unregister().
> > > 
> > > Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> > > ---
> > >  net/mptcp/pm.c        |  4 ++++
> > >  net/mptcp/pm_kernel.c | 26 ++++++++++++++++++++++++++
> > >  net/mptcp/protocol.h  |  3 +++
> > >  3 files changed, 33 insertions(+)
> > > 
> > > diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> > > index a2b210873b23..28ea8bdaa8b0 100644
> > > --- a/net/mptcp/pm.c
> > > +++ b/net/mptcp/pm.c
> > > @@ -1076,6 +1076,10 @@ int mptcp_pm_register(struct mptcp_pm_ops
> > > *pm)
> > >  
> > >  void mptcp_pm_unregister(struct mptcp_pm_ops *pm)
> > >  {
> > > +	/* skip unregistering the default path manager */
> > > +	if (pm == &mptcp_pm_kernel)
> > > +		return;
> > > +
> > >  	spin_lock(&mptcp_pm_list_lock);
> > >  	list_del_rcu(&pm->list);
> > >  	spin_unlock(&mptcp_pm_list_lock);
> > > diff --git a/net/mptcp/pm_kernel.c b/net/mptcp/pm_kernel.c
> > > index 806a9b5b3c07..e6a1aef738a8 100644
> > > --- a/net/mptcp/pm_kernel.c
> > > +++ b/net/mptcp/pm_kernel.c
> > > @@ -1398,8 +1398,34 @@ static struct pernet_operations
> > > mptcp_pm_pernet_ops = {
> > >  	.size = sizeof(struct pm_nl_pernet),
> > >  };
> > >  
> > > +static void mptcp_pm_nl_initialize(struct mptcp_sock *msk)
> > > +{
> > > +	bool subflows_allowed =
> > > !!mptcp_pm_get_subflows_max(msk);
> > > +	struct mptcp_pm_data *pm = &msk->pm;
> > > +
> > > +	/* pm->work_pending must be only be set to 'true' when
> > > +	 * pm is the default path manager
> > > +	 */
> > > +	WRITE_ONCE(pm->work_pending,
> > > +		   (!!mptcp_pm_get_local_addr_max(msk) &&
> > > +		    subflows_allowed) ||
> > > +		   !!mptcp_pm_get_add_addr_signal_max(msk));
> > > +	WRITE_ONCE(pm->accept_addr,
> > > +		   !!mptcp_pm_get_add_addr_accept_max(msk) &&
> > > +		   subflows_allowed);
> > > +	WRITE_ONCE(pm->accept_subflow, subflows_allowed);
> > > +}
> > > +
> > > +struct mptcp_pm_ops mptcp_pm_kernel = {
> > > +	.init			= mptcp_pm_nl_initialize,
> > 
> > A better name would be mptcp_pm_nl_init, but this is already used
> > in
> > pm_netlink.c. I would like to rename mptcp_pm_nl_init in
> > pm_netlink.c
> > to mptcp_pm_genl_init, do you think this is a good idea?
> > 
> > If so, is it better to squash this renaming into "mptcp: pm: split
> > netlink and in-kernel init" or make it a separate patch?
> 
> What about calling the one here mptcp_pm_kernel_init()?

mptcp_pm_kernel_init is not good, because other functions start with
mptcp_pm_nl_:

struct mptcp_pm_ops mptcp_pm_kernel = {
        .get_local_id           = mptcp_pm_nl_get_local_id,
        .get_priority           = mptcp_pm_nl_is_backup,
        .established            = mptcp_pm_nl_fully_established,
        .subflow_established    = mptcp_pm_nl_subflow_established,
        .add_addr_echo          = mptcp_pm_nl_add_addr_echo,
        .add_addr_received      = mptcp_pm_nl_add_addr_received,
        .rm_addr_received       = mptcp_pm_nl_rm_addr_received,
        .rm_subflow_received    = mptcp_pm_nl_rm_subflow_received,
        .add_addr               = mptcp_pm_nl_add_addr,
        .del_addr               = mptcp_pm_nl_del_addr,
        .flush_addrs            = mptcp_pm_nl_flush_addrs,
        .set_priority           = mptcp_pm_nl_set_priority,
        .init                   = mptcp_pm_nl_init,
        .name                   = "kernel",
        .owner                  = THIS_MODULE,
};

Thanks,
-Geliang

> 
> Cheers,
> Matt

Re: [PATCH mptcp-next v8 06/12] mptcp: pm: in-kernel: register mptcp_pm_kernel

[Matthieu Baerts]

Hi Geliang,

On 05/03/2025 10:14, Geliang Tang wrote:
> On Wed, 2025-03-05 at 10:11 +0100, Matthieu Baerts wrote:
>> Hi Geliang,
>>
>> On 05/03/2025 02:35, Geliang Tang wrote:
>>> Hi Matt,
>>>
>>> On Tue, 2025-03-04 at 19:40 +0800, Geliang Tang wrote:
>>>> From: Geliang Tang <tanggeliang@kylinos.cn>
>>>>
>>>> This patch defines the original in-kernel netlink path manager as
>>>> a
>>>> new struct mptcp_pm_ops named "mptcp_pm_kernel", and register it
>>>> in
>>>> mptcp_pm_kernel_register().
>>>>
>>>> This mptcp_pm_ops will be skipped in mptcp_pm_unregister().
>>>>
>>>> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
>>>> ---
>>>>  net/mptcp/pm.c        |  4 ++++
>>>>  net/mptcp/pm_kernel.c | 26 ++++++++++++++++++++++++++
>>>>  net/mptcp/protocol.h  |  3 +++
>>>>  3 files changed, 33 insertions(+)
>>>>
>>>> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
>>>> index a2b210873b23..28ea8bdaa8b0 100644
>>>> --- a/net/mptcp/pm.c
>>>> +++ b/net/mptcp/pm.c
>>>> @@ -1076,6 +1076,10 @@ int mptcp_pm_register(struct mptcp_pm_ops
>>>> *pm)
>>>>  
>>>>  void mptcp_pm_unregister(struct mptcp_pm_ops *pm)
>>>>  {
>>>> +	/* skip unregistering the default path manager */
>>>> +	if (pm == &mptcp_pm_kernel)
>>>> +		return;
>>>> +
>>>>  	spin_lock(&mptcp_pm_list_lock);
>>>>  	list_del_rcu(&pm->list);
>>>>  	spin_unlock(&mptcp_pm_list_lock);
>>>> diff --git a/net/mptcp/pm_kernel.c b/net/mptcp/pm_kernel.c
>>>> index 806a9b5b3c07..e6a1aef738a8 100644
>>>> --- a/net/mptcp/pm_kernel.c
>>>> +++ b/net/mptcp/pm_kernel.c
>>>> @@ -1398,8 +1398,34 @@ static struct pernet_operations
>>>> mptcp_pm_pernet_ops = {
>>>>  	.size = sizeof(struct pm_nl_pernet),
>>>>  };
>>>>  
>>>> +static void mptcp_pm_nl_initialize(struct mptcp_sock *msk)
>>>> +{
>>>> +	bool subflows_allowed =
>>>> !!mptcp_pm_get_subflows_max(msk);
>>>> +	struct mptcp_pm_data *pm = &msk->pm;
>>>> +
>>>> +	/* pm->work_pending must be only be set to 'true' when
>>>> +	 * pm is the default path manager
>>>> +	 */
>>>> +	WRITE_ONCE(pm->work_pending,
>>>> +		   (!!mptcp_pm_get_local_addr_max(msk) &&
>>>> +		    subflows_allowed) ||
>>>> +		   !!mptcp_pm_get_add_addr_signal_max(msk));
>>>> +	WRITE_ONCE(pm->accept_addr,
>>>> +		   !!mptcp_pm_get_add_addr_accept_max(msk) &&
>>>> +		   subflows_allowed);
>>>> +	WRITE_ONCE(pm->accept_subflow, subflows_allowed);
>>>> +}
>>>> +
>>>> +struct mptcp_pm_ops mptcp_pm_kernel = {
>>>> +	.init			= mptcp_pm_nl_initialize,
>>>
>>> A better name would be mptcp_pm_nl_init, but this is already used
>>> in
>>> pm_netlink.c. I would like to rename mptcp_pm_nl_init in
>>> pm_netlink.c
>>> to mptcp_pm_genl_init, do you think this is a good idea?
>>>
>>> If so, is it better to squash this renaming into "mptcp: pm: split
>>> netlink and in-kernel init" or make it a separate patch?
>>
>> What about calling the one here mptcp_pm_kernel_init()?
> 
> mptcp_pm_kernel_init is not good, because other functions start with
> mptcp_pm_nl_:
> 
> struct mptcp_pm_ops mptcp_pm_kernel = {
>         .get_local_id           = mptcp_pm_nl_get_local_id,
>         .get_priority           = mptcp_pm_nl_is_backup,
>         .established            = mptcp_pm_nl_fully_established,
>         .subflow_established    = mptcp_pm_nl_subflow_established,
>         .add_addr_echo          = mptcp_pm_nl_add_addr_echo,
>         .add_addr_received      = mptcp_pm_nl_add_addr_received,
>         .rm_addr_received       = mptcp_pm_nl_rm_addr_received,
>         .rm_subflow_received    = mptcp_pm_nl_rm_subflow_received,
>         .add_addr               = mptcp_pm_nl_add_addr,
>         .del_addr               = mptcp_pm_nl_del_addr,
>         .flush_addrs            = mptcp_pm_nl_flush_addrs,
>         .set_priority           = mptcp_pm_nl_set_priority,
>         .init                   = mptcp_pm_nl_init,
>         .name                   = "kernel",
>         .owner                  = THIS_MODULE,
> };

What about switching to the "mptcp_pm_kernel_" prefix when switching to
the new ops? For some of them, I guess there will be modifications
around the declaration of the function because 'static' will be added, no?

The "mptcp_pm_nl_" was making sense before because everything was in
pm_netlink.c. But now with the split, it might be good to take this
opportunity to rename the functions here to clearly mention it is from
the kernel PM, no?

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

Re: [PATCH mptcp-next v8 06/12] mptcp: pm: in-kernel: register mptcp_pm_kernel

[Geliang Tang]

On Wed, 2025-03-05 at 10:22 +0100, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 05/03/2025 10:14, Geliang Tang wrote:
> > On Wed, 2025-03-05 at 10:11 +0100, Matthieu Baerts wrote:
> > > Hi Geliang,
> > > 
> > > On 05/03/2025 02:35, Geliang Tang wrote:
> > > > Hi Matt,
> > > > 
> > > > On Tue, 2025-03-04 at 19:40 +0800, Geliang Tang wrote:
> > > > > From: Geliang Tang <tanggeliang@kylinos.cn>
> > > > > 
> > > > > This patch defines the original in-kernel netlink path
> > > > > manager as
> > > > > a
> > > > > new struct mptcp_pm_ops named "mptcp_pm_kernel", and register
> > > > > it
> > > > > in
> > > > > mptcp_pm_kernel_register().
> > > > > 
> > > > > This mptcp_pm_ops will be skipped in mptcp_pm_unregister().
> > > > > 
> > > > > Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> > > > > ---
> > > > >  net/mptcp/pm.c        |  4 ++++
> > > > >  net/mptcp/pm_kernel.c | 26 ++++++++++++++++++++++++++
> > > > >  net/mptcp/protocol.h  |  3 +++
> > > > >  3 files changed, 33 insertions(+)
> > > > > 
> > > > > diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> > > > > index a2b210873b23..28ea8bdaa8b0 100644
> > > > > --- a/net/mptcp/pm.c
> > > > > +++ b/net/mptcp/pm.c
> > > > > @@ -1076,6 +1076,10 @@ int mptcp_pm_register(struct
> > > > > mptcp_pm_ops
> > > > > *pm)
> > > > >  
> > > > >  void mptcp_pm_unregister(struct mptcp_pm_ops *pm)
> > > > >  {
> > > > > +	/* skip unregistering the default path manager */
> > > > > +	if (pm == &mptcp_pm_kernel)
> > > > > +		return;
> > > > > +
> > > > >  	spin_lock(&mptcp_pm_list_lock);
> > > > >  	list_del_rcu(&pm->list);
> > > > >  	spin_unlock(&mptcp_pm_list_lock);
> > > > > diff --git a/net/mptcp/pm_kernel.c b/net/mptcp/pm_kernel.c
> > > > > index 806a9b5b3c07..e6a1aef738a8 100644
> > > > > --- a/net/mptcp/pm_kernel.c
> > > > > +++ b/net/mptcp/pm_kernel.c
> > > > > @@ -1398,8 +1398,34 @@ static struct pernet_operations
> > > > > mptcp_pm_pernet_ops = {
> > > > >  	.size = sizeof(struct pm_nl_pernet),
> > > > >  };
> > > > >  
> > > > > +static void mptcp_pm_nl_initialize(struct mptcp_sock *msk)
> > > > > +{
> > > > > +	bool subflows_allowed =
> > > > > !!mptcp_pm_get_subflows_max(msk);
> > > > > +	struct mptcp_pm_data *pm = &msk->pm;
> > > > > +
> > > > > +	/* pm->work_pending must be only be set to 'true'
> > > > > when
> > > > > +	 * pm is the default path manager
> > > > > +	 */
> > > > > +	WRITE_ONCE(pm->work_pending,
> > > > > +		   (!!mptcp_pm_get_local_addr_max(msk) &&
> > > > > +		    subflows_allowed) ||
> > > > > +		   !!mptcp_pm_get_add_addr_signal_max(msk));
> > > > > +	WRITE_ONCE(pm->accept_addr,
> > > > > +		   !!mptcp_pm_get_add_addr_accept_max(msk)
> > > > > &&
> > > > > +		   subflows_allowed);
> > > > > +	WRITE_ONCE(pm->accept_subflow, subflows_allowed);
> > > > > +}
> > > > > +
> > > > > +struct mptcp_pm_ops mptcp_pm_kernel = {
> > > > > +	.init			= mptcp_pm_nl_initialize,
> > > > 
> > > > A better name would be mptcp_pm_nl_init, but this is already
> > > > used
> > > > in
> > > > pm_netlink.c. I would like to rename mptcp_pm_nl_init in
> > > > pm_netlink.c
> > > > to mptcp_pm_genl_init, do you think this is a good idea?
> > > > 
> > > > If so, is it better to squash this renaming into "mptcp: pm:
> > > > split
> > > > netlink and in-kernel init" or make it a separate patch?
> > > 
> > > What about calling the one here mptcp_pm_kernel_init()?
> > 
> > mptcp_pm_kernel_init is not good, because other functions start
> > with
> > mptcp_pm_nl_:
> > 
> > struct mptcp_pm_ops mptcp_pm_kernel = {
> >         .get_local_id           = mptcp_pm_nl_get_local_id,
> >         .get_priority           = mptcp_pm_nl_is_backup,
> >         .established            = mptcp_pm_nl_fully_established,
> >         .subflow_established    = mptcp_pm_nl_subflow_established,
> >         .add_addr_echo          = mptcp_pm_nl_add_addr_echo,
> >         .add_addr_received      = mptcp_pm_nl_add_addr_received,
> >         .rm_addr_received       = mptcp_pm_nl_rm_addr_received,
> >         .rm_subflow_received    = mptcp_pm_nl_rm_subflow_received,
> >         .add_addr               = mptcp_pm_nl_add_addr,
> >         .del_addr               = mptcp_pm_nl_del_addr,
> >         .flush_addrs            = mptcp_pm_nl_flush_addrs,
> >         .set_priority           = mptcp_pm_nl_set_priority,
> >         .init                   = mptcp_pm_nl_init,
> >         .name                   = "kernel",
> >         .owner                  = THIS_MODULE,
> > };
> 
> What about switching to the "mptcp_pm_kernel_" prefix when switching
> to
> the new ops? For some of them, I guess there will be modifications
> around the declaration of the function because 'static' will be
> added, no?
> 
> The "mptcp_pm_nl_" was making sense before because everything was in
> pm_netlink.c. But now with the split, it might be good to take this
> opportunity to rename the functions here to clearly mention it is
> from
> the kernel PM, no?

Sure, I can do that in v9.

Thanks for your suggestion.

-Geliang

> 
> Cheers,
> Matt

Re: [PATCH mptcp-next v8 06/12] mptcp: pm: in-kernel: register mptcp_pm_kernel

[Matthieu Baerts]

Hi Geliang,

On 04/03/2025 12:40, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> This patch defines the original in-kernel netlink path manager as a
> new struct mptcp_pm_ops named "mptcp_pm_kernel", and register it in
> mptcp_pm_kernel_register().
> 
> This mptcp_pm_ops will be skipped in mptcp_pm_unregister().
> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  net/mptcp/pm.c        |  4 ++++
>  net/mptcp/pm_kernel.c | 26 ++++++++++++++++++++++++++
>  net/mptcp/protocol.h  |  3 +++
>  3 files changed, 33 insertions(+)
> 
> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index a2b210873b23..28ea8bdaa8b0 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c
> @@ -1076,6 +1076,10 @@ int mptcp_pm_register(struct mptcp_pm_ops *pm)
>  
>  void mptcp_pm_unregister(struct mptcp_pm_ops *pm)
>  {
> +	/* skip unregistering the default path manager */

Please see my questions from v7: why this skip?

When looking at this, I can understand that we don't want to unregister
built-in modules and the default one, but:

- When are we going to that? mptcp_pm_unregister() is still unused in
this series.

- Why would we want to unregister the userspace PM as well?

It makes sense to have an exception for the default one, but it feels
like we should simply not try to unregister the in-kernel ones. In other
words, there is probably no need to have such exceptions because
mptcp_pm_unregister() should never be called with the built-in PMs. In
this case, maybe we could add a WARN_ON_ONCE()?

  if (WARN_ON_ONCE(pm == &mptcp_pm_kernel))
       return;

(or something else if we need to catch the userspace PM as well, e.g.
pm->built_in, but that should not be needed)


> +	if (pm == &mptcp_pm_kernel)
> +		return;
> +
>  	spin_lock(&mptcp_pm_list_lock);
>  	list_del_rcu(&pm->list);
>  	spin_unlock(&mptcp_pm_list_lock);
> diff --git a/net/mptcp/pm_kernel.c b/net/mptcp/pm_kernel.c
> index 806a9b5b3c07..e6a1aef738a8 100644
> --- a/net/mptcp/pm_kernel.c
> +++ b/net/mptcp/pm_kernel.c
> @@ -1398,8 +1398,34 @@ static struct pernet_operations mptcp_pm_pernet_ops = {
>  	.size = sizeof(struct pm_nl_pernet),
>  };
>  
> +static void mptcp_pm_nl_initialize(struct mptcp_sock *msk)
> +{
> +	bool subflows_allowed = !!mptcp_pm_get_subflows_max(msk);
> +	struct mptcp_pm_data *pm = &msk->pm;
> +
> +	/* pm->work_pending must be only be set to 'true' when
> +	 * pm is the default path manager
> +	 */
> +	WRITE_ONCE(pm->work_pending,
> +		   (!!mptcp_pm_get_local_addr_max(msk) &&
> +		    subflows_allowed) ||
> +		   !!mptcp_pm_get_add_addr_signal_max(msk));
> +	WRITE_ONCE(pm->accept_addr,
> +		   !!mptcp_pm_get_add_addr_accept_max(msk) &&
> +		   subflows_allowed);
> +	WRITE_ONCE(pm->accept_subflow, subflows_allowed);

It might feel clearer to add this helper in patch 8 ("mptcp: pm:
initialize and release mptcp_pm_ops"), to understand you are moving
existing code here.

If you do that, then maybe better to squash the existing patches 6
("mptcp: pm: in-kernel: register mptcp_pm_kernel") and 7 ("mptcp: pm:
userspace: register mptcp_pm_userspace"), no?

  mptcp: pm: register in-kernel and userspace PM


> +}
> +
> +struct mptcp_pm_ops mptcp_pm_kernel = {
> +	.init			= mptcp_pm_nl_initialize,
> +	.name			= "kernel",
> +	.owner			= THIS_MODULE,
> +};
> +
>  void __init mptcp_pm_kernel_register(void)
>  {
>  	if (register_pernet_subsys(&mptcp_pm_pernet_ops) < 0)
>  		panic("Failed to register MPTCP PM pernet subsystem.\n");
> +
> +	mptcp_pm_register(&mptcp_pm_kernel);
>  }
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index 246b44db9775..f700cb55bf49 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -1050,6 +1050,9 @@ int mptcp_pm_remove_addr(struct mptcp_sock *msk, const struct mptcp_rm_list *rm_
>  void mptcp_pm_remove_addr_entry(struct mptcp_sock *msk,
>  				struct mptcp_pm_addr_entry *entry);
>  
> +/* the default path manager, used in mptcp_pm_unregister */
(to be adapted if it is no longer used there. Or: mptcp_pm_initialize)

Or maybe better: it could be exported in patch 8 ("mptcp: pm: initialize
and release mptcp_pm_ops").

> +extern struct mptcp_pm_ops mptcp_pm_kernel;
> +
>  struct mptcp_pm_ops *mptcp_pm_find(const char *name);
>  int mptcp_pm_validate(struct mptcp_pm_ops *pm);
>  int mptcp_pm_register(struct mptcp_pm_ops *pm);

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

Re: [PATCH mptcp-next v8 06/12] mptcp: pm: in-kernel: register mptcp_pm_kernel

[Geliang Tang]

Hi Matt,

Thanks for the review.

On Wed, 2025-03-05 at 12:51 +0100, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 04/03/2025 12:40, Geliang Tang wrote:
> > From: Geliang Tang <tanggeliang@kylinos.cn>
> > 
> > This patch defines the original in-kernel netlink path manager as a
> > new struct mptcp_pm_ops named "mptcp_pm_kernel", and register it in
> > mptcp_pm_kernel_register().
> > 
> > This mptcp_pm_ops will be skipped in mptcp_pm_unregister().
> > 
> > Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> > ---
> >  net/mptcp/pm.c        |  4 ++++
> >  net/mptcp/pm_kernel.c | 26 ++++++++++++++++++++++++++
> >  net/mptcp/protocol.h  |  3 +++
> >  3 files changed, 33 insertions(+)
> > 
> > diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> > index a2b210873b23..28ea8bdaa8b0 100644
> > --- a/net/mptcp/pm.c
> > +++ b/net/mptcp/pm.c
> > @@ -1076,6 +1076,10 @@ int mptcp_pm_register(struct mptcp_pm_ops
> > *pm)
> >  
> >  void mptcp_pm_unregister(struct mptcp_pm_ops *pm)
> >  {
> > +	/* skip unregistering the default path manager */
> 
> Please see my questions from v7: why this skip?

mptcp_pm_kernel is the default pm, skip it to ensure that there's
always a valid path manager available.

> 
> When looking at this, I can understand that we don't want to
> unregister
> built-in modules and the default one, but:
> 
> - When are we going to that? mptcp_pm_unregister() is still unused in
> this series.

mptcp_pm_unregister is not used in this set, but will be invoked
in .unreg of struct bpf_struct_ops.

> 
> - Why would we want to unregister the userspace PM as well?

The default one is mptcp_pm_kernel, not mptcp_pm_userspace, we set it
in mptcp_pm_ops_init when the input pm_ops is invalid.

> 
> It makes sense to have an exception for the default one, but it feels
> like we should simply not try to unregister the in-kernel ones. In
> other
> words, there is probably no need to have such exceptions because
> mptcp_pm_unregister() should never be called with the built-in PMs.
> In
> this case, maybe we could add a WARN_ON_ONCE()?
> 
>   if (WARN_ON_ONCE(pm == &mptcp_pm_kernel))
>        return;

Added this in v10.

> 
> (or something else if we need to catch the userspace PM as well, e.g.
> pm->built_in, but that should not be needed)
> 
> 
> > +	if (pm == &mptcp_pm_kernel)
> > +		return;
> > +
> >  	spin_lock(&mptcp_pm_list_lock);
> >  	list_del_rcu(&pm->list);
> >  	spin_unlock(&mptcp_pm_list_lock);
> > diff --git a/net/mptcp/pm_kernel.c b/net/mptcp/pm_kernel.c
> > index 806a9b5b3c07..e6a1aef738a8 100644
> > --- a/net/mptcp/pm_kernel.c
> > +++ b/net/mptcp/pm_kernel.c
> > @@ -1398,8 +1398,34 @@ static struct pernet_operations
> > mptcp_pm_pernet_ops = {
> >  	.size = sizeof(struct pm_nl_pernet),
> >  };
> >  
> > +static void mptcp_pm_nl_initialize(struct mptcp_sock *msk)
> > +{
> > +	bool subflows_allowed = !!mptcp_pm_get_subflows_max(msk);
> > +	struct mptcp_pm_data *pm = &msk->pm;
> > +
> > +	/* pm->work_pending must be only be set to 'true' when
> > +	 * pm is the default path manager
> > +	 */
> > +	WRITE_ONCE(pm->work_pending,
> > +		   (!!mptcp_pm_get_local_addr_max(msk) &&
> > +		    subflows_allowed) ||
> > +		   !!mptcp_pm_get_add_addr_signal_max(msk));
> > +	WRITE_ONCE(pm->accept_addr,
> > +		   !!mptcp_pm_get_add_addr_accept_max(msk) &&
> > +		   subflows_allowed);
> > +	WRITE_ONCE(pm->accept_subflow, subflows_allowed);
> 
> It might feel clearer to add this helper in patch 8 ("mptcp: pm:
> initialize and release mptcp_pm_ops"), to understand you are moving
> existing code here.
> 
> If you do that, then maybe better to squash the existing patches 6
> ("mptcp: pm: in-kernel: register mptcp_pm_kernel") and 7 ("mptcp: pm:
> userspace: register mptcp_pm_userspace"), no?
> 
>   mptcp: pm: register in-kernel and userspace PM

Done.

> 
> 
> > +}
> > +
> > +struct mptcp_pm_ops mptcp_pm_kernel = {
> > +	.init			= mptcp_pm_nl_initialize,
> > +	.name			= "kernel",
> > +	.owner			= THIS_MODULE,
> > +};
> > +
> >  void __init mptcp_pm_kernel_register(void)
> >  {
> >  	if (register_pernet_subsys(&mptcp_pm_pernet_ops) < 0)
> >  		panic("Failed to register MPTCP PM pernet
> > subsystem.\n");
> > +
> > +	mptcp_pm_register(&mptcp_pm_kernel);
> >  }
> > diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> > index 246b44db9775..f700cb55bf49 100644
> > --- a/net/mptcp/protocol.h
> > +++ b/net/mptcp/protocol.h
> > @@ -1050,6 +1050,9 @@ int mptcp_pm_remove_addr(struct mptcp_sock
> > *msk, const struct mptcp_rm_list *rm_
> >  void mptcp_pm_remove_addr_entry(struct mptcp_sock *msk,
> >  				struct mptcp_pm_addr_entry
> > *entry);
> >  
> > +/* the default path manager, used in mptcp_pm_unregister */
> (to be adapted if it is no longer used there. Or:
> mptcp_pm_initialize)
> 
> Or maybe better: it could be exported in patch 8 ("mptcp: pm:
> initialize
> and release mptcp_pm_ops").

Done.

Thanks,
-Geliang

> 
> > +extern struct mptcp_pm_ops mptcp_pm_kernel;
> > +
> >  struct mptcp_pm_ops *mptcp_pm_find(const char *name);
> >  int mptcp_pm_validate(struct mptcp_pm_ops *pm);
> >  int mptcp_pm_register(struct mptcp_pm_ops *pm);
> 
> Cheers,
> Matt

Re: [PATCH mptcp-next v8 06/12] mptcp: pm: in-kernel: register mptcp_pm_kernel

[Matthieu Baerts]

Hi Geliang,

Thank you for the reply!

On 06/03/2025 12:09, Geliang Tang wrote:
> On Wed, 2025-03-05 at 12:51 +0100, Matthieu Baerts wrote:
>> On 04/03/2025 12:40, Geliang Tang wrote:

(...)

>> When looking at this, I can understand that we don't want to
>> unregister
>> built-in modules and the default one, but:
>>
>> - When are we going to that? mptcp_pm_unregister() is still unused in
>> this series.
> 
> mptcp_pm_unregister is not used in this set, but will be invoked
> in .unreg of struct bpf_struct_ops.

OK, that's clearer. So mptcp_pm_unregister() will only be called for the
BPF PM, and then not with the kernel/userspace PMs.

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

[PATCH mptcp-next v8 07/12] mptcp: pm: userspace: register mptcp_pm_userspace

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

This patch defines the original userspace path manager as a new
struct mptcp_pm_ops named "mptcp_userspace_pm", and register it
in mptcp_pm_init(). mptcp_userspace_pm_is_release() is a wrapper
of mptcp_userspace_pm_free_local_addr_list().

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 net/mptcp/pm.c           |  1 +
 net/mptcp/pm_userspace.c | 26 ++++++++++++++++++++++++++
 net/mptcp/protocol.h     |  1 +
 3 files changed, 28 insertions(+)

diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index 28ea8bdaa8b0..5018ed3c575f 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -1028,6 +1028,7 @@ void mptcp_pm_data_init(struct mptcp_sock *msk)
 void __init mptcp_pm_init(void)
 {
 	mptcp_pm_kernel_register();
+	mptcp_pm_userspace_register();
 	mptcp_pm_nl_init();
 }
 
diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index 13856df22673..412d6c912148 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -682,3 +682,29 @@ int mptcp_userspace_pm_get_addr(u8 id, struct mptcp_pm_addr_entry *addr,
 	sock_put(sk);
 	return ret;
 }
+
+static void mptcp_userspace_pm_init(struct mptcp_sock *msk)
+{
+	struct mptcp_pm_data *pm = &msk->pm;
+
+	WRITE_ONCE(pm->work_pending, 0);
+	WRITE_ONCE(pm->accept_addr, 0);
+	WRITE_ONCE(pm->accept_subflow, 0);
+}
+
+static void mptcp_userspace_pm_release(struct mptcp_sock *msk)
+{
+	mptcp_userspace_pm_free_local_addr_list(msk);
+}
+
+static struct mptcp_pm_ops mptcp_pm_userspace = {
+	.init			= mptcp_userspace_pm_init,
+	.release		= mptcp_userspace_pm_release,
+	.name			= "userspace",
+	.owner			= THIS_MODULE,
+};
+
+void __init mptcp_pm_userspace_register(void)
+{
+	mptcp_pm_register(&mptcp_pm_userspace);
+}
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index f700cb55bf49..658bc60d4cd8 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -1162,6 +1162,7 @@ static inline u8 subflow_get_local_id(const struct mptcp_subflow_context *subflo
 }
 
 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);
-- 
2.43.0

Re: [PATCH mptcp-next v8 07/12] mptcp: pm: userspace: register mptcp_pm_userspace

[Matthieu Baerts]

Hi Geliang,

On 04/03/2025 12:40, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> This patch defines the original userspace path manager as a new
> struct mptcp_pm_ops named "mptcp_userspace_pm", and register it
> in mptcp_pm_init(). mptcp_userspace_pm_is_release() is a wrapper
> of mptcp_userspace_pm_free_local_addr_list().

(...)

> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> index 13856df22673..412d6c912148 100644
> --- a/net/mptcp/pm_userspace.c
> +++ b/net/mptcp/pm_userspace.c
> @@ -682,3 +682,29 @@ int mptcp_userspace_pm_get_addr(u8 id, struct mptcp_pm_addr_entry *addr,
>  	sock_put(sk);
>  	return ret;
>  }
> +
> +static void mptcp_userspace_pm_init(struct mptcp_sock *msk)
> +{
> +	struct mptcp_pm_data *pm = &msk->pm;
> +
> +	WRITE_ONCE(pm->work_pending, 0);
> +	WRITE_ONCE(pm->accept_addr, 0);
> +	WRITE_ONCE(pm->accept_subflow, 0);

I would not do that here: these variables are not used by the userspace
PM, that doesn't make sense for this PM to do that. I think these
variables should be reset all the time, then the init callback is called
if set.

In other words, only the in-kernel PM needs this init callback.

> +}
> +
> +static void mptcp_userspace_pm_release(struct mptcp_sock *msk)
> +{
> +	mptcp_userspace_pm_free_local_addr_list(msk);
> +}

Same as in patch 6, I would also move this release part in patch 8
("mptcp: pm: initialize and release mptcp_pm_ops"), to show that you are
moving code. Otherwise, it is difficult to know from where it comes from.

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

[PATCH mptcp-next v8 08/12] mptcp: pm: initialize and release mptcp_pm_ops

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

Add a struct mptcp_pm_ops pointer "ops" in struct mptcp_pm_data, and two
functions mptcp_pm_initialize() and mptcp_pm_release(), to set and release
this pointer. mptcp_pm_initialize() is invoked in mptcp_pm_data_reset(),
while mptcp_pm_release() is invoked in mptcp_pm_destroy().

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

diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index 5018ed3c575f..e4d84aad3795 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -970,16 +970,45 @@ void mptcp_pm_worker(struct mptcp_sock *msk)
 	spin_unlock_bh(&msk->pm.lock);
 }
 
+static void mptcp_pm_initialize(struct mptcp_sock *msk,
+				struct mptcp_pm_ops *pm)
+{
+	if (!pm || !bpf_try_module_get(pm, pm->owner)) {
+		pr_warn_once("pm %s fails, fallback to default pm",
+			     pm->name);
+		pm = &mptcp_pm_kernel;
+	}
+
+	msk->pm.ops = pm;
+	if (msk->pm.ops->init)
+		msk->pm.ops->init(msk);
+
+	pr_debug("pm %s initialized\n", pm->name);
+}
+
+static void mptcp_pm_release(struct mptcp_sock *msk)
+{
+	struct mptcp_pm_ops *pm = msk->pm.ops;
+
+	if (!pm)
+		return;
+
+	msk->pm.ops = NULL;
+	if (pm->release)
+		pm->release(msk);
+
+	bpf_module_put(pm, pm->owner);
+}
+
 void mptcp_pm_destroy(struct mptcp_sock *msk)
 {
 	mptcp_pm_free_anno_list(msk);
-
-	if (mptcp_pm_is_userspace(msk))
-		mptcp_userspace_pm_free_local_addr_list(msk);
+	mptcp_pm_release(msk);
 }
 
 void mptcp_pm_data_reset(struct mptcp_sock *msk)
 {
+	const char *path_manager = mptcp_get_path_manager(sock_net((struct sock *)msk));
 	u8 pm_type = mptcp_get_pm_type(sock_net((struct sock *)msk));
 	struct mptcp_pm_data *pm = &msk->pm;
 
@@ -991,25 +1020,9 @@ void mptcp_pm_data_reset(struct mptcp_sock *msk)
 	pm->rm_list_rx.nr = 0;
 	WRITE_ONCE(pm->pm_type, pm_type);
 
-	if (pm_type == MPTCP_PM_TYPE_KERNEL) {
-		bool subflows_allowed = !!mptcp_pm_get_subflows_max(msk);
-
-		/* pm->work_pending must be only be set to 'true' when
-		 * pm->pm_type is set to MPTCP_PM_TYPE_KERNEL
-		 */
-		WRITE_ONCE(pm->work_pending,
-			   (!!mptcp_pm_get_local_addr_max(msk) &&
-			    subflows_allowed) ||
-			   !!mptcp_pm_get_add_addr_signal_max(msk));
-		WRITE_ONCE(pm->accept_addr,
-			   !!mptcp_pm_get_add_addr_accept_max(msk) &&
-			   subflows_allowed);
-		WRITE_ONCE(pm->accept_subflow, subflows_allowed);
-	} else {
-		WRITE_ONCE(pm->work_pending, 0);
-		WRITE_ONCE(pm->accept_addr, 0);
-		WRITE_ONCE(pm->accept_subflow, 0);
-	}
+	rcu_read_lock();
+	mptcp_pm_initialize(msk, mptcp_pm_find(path_manager));
+	rcu_read_unlock();
 
 	WRITE_ONCE(pm->addr_signal, 0);
 	WRITE_ONCE(pm->remote_deny_join_id0, false);
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 658bc60d4cd8..2a264124cb17 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -220,6 +220,7 @@ struct mptcp_pm_data {
 	struct mptcp_addr_info remote;
 	struct list_head anno_list;
 	struct list_head userspace_pm_local_addr_list;
+	struct mptcp_pm_ops *ops;
 
 	spinlock_t	lock;		/*protects the whole PM data */
 
-- 
2.43.0

Re: [PATCH mptcp-next v8 08/12] mptcp: pm: initialize and release mptcp_pm_ops

[Matthieu Baerts]

Hi Geliang,

On 04/03/2025 12:40, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> Add a struct mptcp_pm_ops pointer "ops" in struct mptcp_pm_data, and two
> functions mptcp_pm_initialize() and mptcp_pm_release(), to set and release
> this pointer. mptcp_pm_initialize() is invoked in mptcp_pm_data_reset(),
> while mptcp_pm_release() is invoked in mptcp_pm_destroy().
> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  net/mptcp/pm.c       | 57 +++++++++++++++++++++++++++-----------------
>  net/mptcp/protocol.h |  1 +
>  2 files changed, 36 insertions(+), 22 deletions(-)
> 
> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index 5018ed3c575f..e4d84aad3795 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c
> @@ -970,16 +970,45 @@ void mptcp_pm_worker(struct mptcp_sock *msk)
>  	spin_unlock_bh(&msk->pm.lock);
>  }
>  
> +static void mptcp_pm_initialize(struct mptcp_sock *msk,
> +				struct mptcp_pm_ops *pm)

To avoid confusions, probably best to use "pm_ops" for "mptcp_pm_ops"
structures, and keep "pm" for "mptcp_pm_data" ones.

> +{
> +	if (!pm || !bpf_try_module_get(pm, pm->owner)) {
> +		pr_warn_once("pm %s fails, fallback to default pm",
> +			     pm->name);
> +		pm = &mptcp_pm_kernel;
> +	}
> +
> +	msk->pm.ops = pm;
> +	if (msk->pm.ops->init)

Note: this should be kept like that if 'init' is no longer mandatory
(see my comment on patch 7 -- but if it was mandatory because of what is
done in mptcp_pm_validate(), no need to check if it is set then)

> +		msk->pm.ops->init(msk);
> +
> +	pr_debug("pm %s initialized\n", pm->name);
> +}
> +
> +static void mptcp_pm_release(struct mptcp_sock *msk)
> +{
> +	struct mptcp_pm_ops *pm = msk->pm.ops;

Same here: pm_ops?

> +
> +	if (!pm)
When can we have this case? If we can have this case, that means each
time we will want to use "pm->ops", we will need to check if it is not
NULL. That feels wrong. The PM should be released only once.

We could add a WARN_ON_ONCE() here, but still, that feels wrong: why
adding a check only there? And if it is not needed, why adding it?

> +		return;
> +
> +	msk->pm.ops = NULL;
> +	if (pm->release)
> +		pm->release(msk);
> +
> +	bpf_module_put(pm, pm->owner);
Probably good to add this after:

  pr_debug("pm %s released\n", pm->name);

(...)

> @@ -991,25 +1020,9 @@ void mptcp_pm_data_reset(struct mptcp_sock *msk)
>  	pm->rm_list_rx.nr = 0;
>  	WRITE_ONCE(pm->pm_type, pm_type);
>  
> -	if (pm_type == MPTCP_PM_TYPE_KERNEL) {
> -		bool subflows_allowed = !!mptcp_pm_get_subflows_max(msk);
> -
> -		/* pm->work_pending must be only be set to 'true' when
> -		 * pm->pm_type is set to MPTCP_PM_TYPE_KERNEL
> -		 */
> -		WRITE_ONCE(pm->work_pending,
> -			   (!!mptcp_pm_get_local_addr_max(msk) &&
> -			    subflows_allowed) ||
> -			   !!mptcp_pm_get_add_addr_signal_max(msk));
> -		WRITE_ONCE(pm->accept_addr,
> -			   !!mptcp_pm_get_add_addr_accept_max(msk) &&
> -			   subflows_allowed);
> -		WRITE_ONCE(pm->accept_subflow, subflows_allowed);
> -	} else {
> -		WRITE_ONCE(pm->work_pending, 0);
> -		WRITE_ONCE(pm->accept_addr, 0);
> -		WRITE_ONCE(pm->accept_subflow, 0);
This should be kept here, for all PM, then calling pm->ops->init(), see
my comment in patch 7.


Also, not directly related to this patch, it feels like we should add a
"struct_group(reset, ...)" in struct mptcp_pm_data to simplify the
reset, and make sure we don't miss any. This should be done in a
dedicated patch, and can be done later.
Note that mptcp_pm_data_reset() is called when the msk is created, and
after a disconnect, to be able to re-use the socket again after. We
should make sure everything has been reset, that's not something we
heavily test. In fact, maybe we already missed the reset of some fields
that could cause some issues? Can you check that please?

> -	}
> +	rcu_read_lock();
> +	mptcp_pm_initialize(msk, mptcp_pm_find(path_manager));
> +	rcu_read_unlock();
>  
>  	WRITE_ONCE(pm->addr_signal, 0);
>  	WRITE_ONCE(pm->remote_deny_join_id0, false);
Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.

[PATCH mptcp-next v8 09/12] mptcp: pm: add get_local_id() interface

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

Now mptcp_pm_get_local_id() can directly invoke get_local_id() interface
through "ops" of "msk->pm". Instead of using mptcp_pm_is_userspace() to
check which get_local_id() helper to invoke.

Then mptcp_pm_nl_get_local_id() and mptcp_userspace_pm_get_local_id()
helpers can be static.

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

diff --git a/include/net/mptcp.h b/include/net/mptcp.h
index c53ffff0a4cd..6090a9853f45 100644
--- a/include/net/mptcp.h
+++ b/include/net/mptcp.h
@@ -127,6 +127,9 @@ struct mptcp_sched_ops {
 #define MPTCP_PM_BUF_MAX	(MPTCP_PM_NAME_MAX * MPTCP_PM_MAX)
 
 struct mptcp_pm_ops {
+	int (*get_local_id)(struct mptcp_sock *msk,
+			    struct mptcp_pm_addr_entry *skc);
+
 	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 e4d84aad3795..1e92bd470fdc 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -872,9 +872,7 @@ int mptcp_pm_get_local_id(struct mptcp_sock *msk, struct sock_common *skc)
 	skc_local.addr.id = 0;
 	skc_local.flags = MPTCP_PM_ADDR_FLAG_IMPLICIT;
 
-	if (mptcp_pm_is_userspace(msk))
-		return mptcp_userspace_pm_get_local_id(msk, &skc_local);
-	return mptcp_pm_nl_get_local_id(msk, &skc_local);
+	return msk->pm.ops->get_local_id(msk, &skc_local);
 }
 
 bool mptcp_pm_is_backup(struct mptcp_sock *msk, struct sock_common *skc)
@@ -1060,7 +1058,7 @@ struct mptcp_pm_ops *mptcp_pm_find(const char *name)
 
 int mptcp_pm_validate(struct mptcp_pm_ops *pm)
 {
-	if (!pm->init) {
+	if (!pm->init || !pm->get_local_id) {
 		pr_err("%s does not implement required ops\n", pm->name);
 		return -EINVAL;
 	}
diff --git a/net/mptcp/pm_kernel.c b/net/mptcp/pm_kernel.c
index e6a1aef738a8..f134acc39101 100644
--- a/net/mptcp/pm_kernel.c
+++ b/net/mptcp/pm_kernel.c
@@ -693,8 +693,8 @@ static int mptcp_pm_nl_create_listen_socket(struct sock *sk,
 	return err;
 }
 
-int mptcp_pm_nl_get_local_id(struct mptcp_sock *msk,
-			     struct mptcp_pm_addr_entry *skc)
+static int mptcp_pm_nl_get_local_id(struct mptcp_sock *msk,
+				    struct mptcp_pm_addr_entry *skc)
 {
 	struct mptcp_pm_addr_entry *entry;
 	struct pm_nl_pernet *pernet;
@@ -1417,6 +1417,7 @@ static void mptcp_pm_nl_initialize(struct mptcp_sock *msk)
 }
 
 struct mptcp_pm_ops mptcp_pm_kernel = {
+	.get_local_id		= mptcp_pm_nl_get_local_id,
 	.init			= mptcp_pm_nl_initialize,
 	.name			= "kernel",
 	.owner			= THIS_MODULE,
diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index 412d6c912148..c41ae9cb41df 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -126,8 +126,8 @@ mptcp_userspace_pm_lookup_addr_by_id(struct mptcp_sock *msk, unsigned int id)
 	return NULL;
 }
 
-int mptcp_userspace_pm_get_local_id(struct mptcp_sock *msk,
-				    struct mptcp_pm_addr_entry *skc)
+static int mptcp_userspace_pm_get_local_id(struct mptcp_sock *msk,
+					   struct mptcp_pm_addr_entry *skc)
 {
 	__be16 msk_sport =  ((struct inet_sock *)
 			     inet_sk((struct sock *)msk))->inet_sport;
@@ -698,6 +698,7 @@ static void mptcp_userspace_pm_release(struct mptcp_sock *msk)
 }
 
 static struct mptcp_pm_ops mptcp_pm_userspace = {
+	.get_local_id		= mptcp_userspace_pm_get_local_id,
 	.init			= mptcp_userspace_pm_init,
 	.release		= mptcp_userspace_pm_release,
 	.name			= "userspace",
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 2a264124cb17..85c5a5bd3657 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -1137,10 +1137,6 @@ bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, const struct sk_buff *skb,
 bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
 			     struct mptcp_rm_list *rm_list);
 int mptcp_pm_get_local_id(struct mptcp_sock *msk, struct sock_common *skc);
-int mptcp_pm_nl_get_local_id(struct mptcp_sock *msk,
-			     struct mptcp_pm_addr_entry *skc);
-int mptcp_userspace_pm_get_local_id(struct mptcp_sock *msk,
-				    struct mptcp_pm_addr_entry *skc);
 bool mptcp_pm_is_backup(struct mptcp_sock *msk, struct sock_common *skc);
 bool mptcp_pm_nl_is_backup(struct mptcp_sock *msk, struct mptcp_addr_info *skc);
 bool mptcp_userspace_pm_is_backup(struct mptcp_sock *msk, struct mptcp_addr_info *skc);
-- 
2.43.0

[PATCH mptcp-next v8 10/12] mptcp: pm: add get_priority() interface

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

Now mptcp_pm_is_backup() can directly invoke get_priority() interface
through "ops" of "msk->pm". Instead of using mptcp_pm_is_userspace()
to check which is_backup() helper to invoke.

Then mptcp_pm_nl_is_backup() and mptcp_userspace_pm_is_backup() helpers
can be static.

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

diff --git a/include/net/mptcp.h b/include/net/mptcp.h
index 6090a9853f45..83977fe3dd30 100644
--- a/include/net/mptcp.h
+++ b/include/net/mptcp.h
@@ -129,6 +129,8 @@ struct mptcp_sched_ops {
 struct mptcp_pm_ops {
 	int (*get_local_id)(struct mptcp_sock *msk,
 			    struct mptcp_pm_addr_entry *skc);
+	bool (*get_priority)(struct mptcp_sock *msk,
+			     struct mptcp_addr_info *skc);
 
 	char			name[MPTCP_PM_NAME_MAX];
 	struct module		*owner;
diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index 1e92bd470fdc..51866bf16d02 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -881,10 +881,7 @@ bool mptcp_pm_is_backup(struct mptcp_sock *msk, struct sock_common *skc)
 
 	mptcp_local_address((struct sock_common *)skc, &skc_local);
 
-	if (mptcp_pm_is_userspace(msk))
-		return mptcp_userspace_pm_is_backup(msk, &skc_local);
-
-	return mptcp_pm_nl_is_backup(msk, &skc_local);
+	return msk->pm.ops->get_priority(msk, &skc_local);
 }
 
 static void mptcp_pm_subflows_chk_stale(const struct mptcp_sock *msk, struct sock *ssk)
@@ -1058,7 +1055,7 @@ struct mptcp_pm_ops *mptcp_pm_find(const char *name)
 
 int mptcp_pm_validate(struct mptcp_pm_ops *pm)
 {
-	if (!pm->init || !pm->get_local_id) {
+	if (!pm->init || !pm->get_local_id || !pm->get_priority) {
 		pr_err("%s does not implement required ops\n", pm->name);
 		return -EINVAL;
 	}
diff --git a/net/mptcp/pm_kernel.c b/net/mptcp/pm_kernel.c
index f134acc39101..8d661084daf3 100644
--- a/net/mptcp/pm_kernel.c
+++ b/net/mptcp/pm_kernel.c
@@ -722,7 +722,8 @@ static int mptcp_pm_nl_get_local_id(struct mptcp_sock *msk,
 	return ret;
 }
 
-bool mptcp_pm_nl_is_backup(struct mptcp_sock *msk, struct mptcp_addr_info *skc)
+static bool mptcp_pm_nl_is_backup(struct mptcp_sock *msk,
+				  struct mptcp_addr_info *skc)
 {
 	struct pm_nl_pernet *pernet = pm_nl_get_pernet_from_msk(msk);
 	struct mptcp_pm_addr_entry *entry;
@@ -1418,6 +1419,7 @@ static void mptcp_pm_nl_initialize(struct mptcp_sock *msk)
 
 struct mptcp_pm_ops mptcp_pm_kernel = {
 	.get_local_id		= mptcp_pm_nl_get_local_id,
+	.get_priority		= mptcp_pm_nl_is_backup,
 	.init			= mptcp_pm_nl_initialize,
 	.name			= "kernel",
 	.owner			= THIS_MODULE,
diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index c41ae9cb41df..e219a2d37429 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -145,8 +145,8 @@ static int mptcp_userspace_pm_get_local_id(struct mptcp_sock *msk,
 	return mptcp_userspace_pm_append_new_local_addr(msk, skc, true);
 }
 
-bool mptcp_userspace_pm_is_backup(struct mptcp_sock *msk,
-				  struct mptcp_addr_info *skc)
+static bool mptcp_userspace_pm_is_backup(struct mptcp_sock *msk,
+					 struct mptcp_addr_info *skc)
 {
 	struct mptcp_pm_addr_entry *entry;
 	bool backup;
@@ -699,6 +699,7 @@ static void mptcp_userspace_pm_release(struct mptcp_sock *msk)
 
 static struct mptcp_pm_ops mptcp_pm_userspace = {
 	.get_local_id		= mptcp_userspace_pm_get_local_id,
+	.get_priority		= mptcp_userspace_pm_is_backup,
 	.init			= mptcp_userspace_pm_init,
 	.release		= mptcp_userspace_pm_release,
 	.name			= "userspace",
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 85c5a5bd3657..776a4bc3e5af 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -1138,8 +1138,6 @@ bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
 			     struct mptcp_rm_list *rm_list);
 int mptcp_pm_get_local_id(struct mptcp_sock *msk, struct sock_common *skc);
 bool mptcp_pm_is_backup(struct mptcp_sock *msk, struct sock_common *skc);
-bool mptcp_pm_nl_is_backup(struct mptcp_sock *msk, struct mptcp_addr_info *skc);
-bool mptcp_userspace_pm_is_backup(struct mptcp_sock *msk, struct mptcp_addr_info *skc);
 int mptcp_pm_nl_dump_addr(struct sk_buff *msg,
 			  struct netlink_callback *cb);
 int mptcp_userspace_pm_dump_addr(struct sk_buff *msg,
-- 
2.43.0

[PATCH mptcp-next v8 11/12] selftests: mptcp: add pm_type mapping tests

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

This patch adds a new helper set_path_manager() to set the newly added
net.mptcp.path_manager, and test it inside a userspace pm test.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 tools/testing/selftests/net/mptcp/mptcp_join.sh | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index 13a3b68181ee..fa1929ca6ad4 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -3572,6 +3572,21 @@ userspace_tests()
 	if reset_with_events "userspace pm add & remove address" &&
 	   continue_if mptcp_lib_has_file '/proc/sys/net/mptcp/pm_type'; then
 		set_userspace_pm $ns1
+		if continue_if mptcp_lib_has_file '/proc/sys/net/mptcp/path_manager'; then
+			local pm1 pm2
+
+			pm1=$(ip netns exec ${ns1} sysctl -n net.mptcp.path_manager)
+			if [ "$pm1" != "userspace" ]; then
+				mptcp_lib_pr_fail "ns1 pm_type mapping fails"
+				return 1
+			fi
+
+			pm2=$(ip netns exec ${ns2} sysctl -n net.mptcp.path_manager)
+			if [ "$pm2" != "kernel" ]; then
+				mptcp_lib_pr_fail "ns2 pm_type mapping fails"
+				return 1
+			fi
+		fi
 		pm_nl_set_limits $ns2 2 2
 		{ speed=5 \
 			run_tests $ns1 $ns2 10.0.1.1 & } 2>/dev/null
-- 
2.43.0

Re: [PATCH mptcp-next v8 11/12] selftests: mptcp: add pm_type mapping tests

[Matthieu Baerts]

Hi Geliang,

On 04/03/2025 12:40, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> This patch adds a new helper set_path_manager() to set the newly added
> net.mptcp.path_manager, and test it inside a userspace pm test.
> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  tools/testing/selftests/net/mptcp/mptcp_join.sh | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index 13a3b68181ee..fa1929ca6ad4 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -3572,6 +3572,21 @@ userspace_tests()
>  	if reset_with_events "userspace pm add & remove address" &&
>  	   continue_if mptcp_lib_has_file '/proc/sys/net/mptcp/pm_type'; then
>  		set_userspace_pm $ns1
> +		if continue_if mptcp_lib_has_file '/proc/sys/net/mptcp/path_manager'; then

You cannot have a "continue_if" here, that will mark the whole subtest
as skip if the condition is not met, then continue.

Why not having this in userspace_pm.sh as I suggested. It feels wrong to
have it here, in the "join" test, no?

https://lore.kernel.org/c49517d2-38e2-4848-9fb9-1c7748689cec@kernel.org

So in userspace_pm.sh, around "mptcp_lib_ns_init ns1 ns2", having
something like (not tested):

  print_title "Init"
  print_test "Created network namespaces ns1, ns2"
  mptcp_lib_ns_init ns1 ns2

  # check path_manager and pm_type sysctl mapping
  if [ -f /proc/sys/net/mptcp/path_manager ]; then
      ip netns exec "$ns1" sysctl -q net.mptcp.path_manager=userspace
      pm_type="$(ip netns exec "$ns1" sysctl -n net.mptcp.pm_type)"
      if [ "${pm_type}" != "1" ]; then
          test_fail "unexpected pm_type: ${pm_type}"
          mptcp_lib_result_print_all_tap
          exit ${KSFT_FAIL}
      fi

      ip netns exec "$ns1" sysctl -q net.mptcp.pm_type=0
      pm="$(ip netns exec "$ns1" sysctl -n net.mptcp.path_manager)"
      if [ "${pm}" != "kernel" ]; then
          test_fail "unexpected path-manager: ${pm}"
          mptcp_lib_result_print_all_tap
          exit ${KSFT_FAIL}
      fi
  fi

  for i in "$ns1" "$ns2" ;do
      ip netns exec "$i" sysctl -q net.mptcp.pm_type=1
  done

  (...)

WDYT?

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

[PATCH mptcp-next v8 12/12] selftests: mptcp: add path_manager sysctl test

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

This patch adds a new helper set_path_manager() to set the newly added
net.mptcp.path_manager, and test it inside a userspace pm test.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 .../testing/selftests/net/mptcp/mptcp_join.sh | 34 ++++++++++++++++++-
 1 file changed, 33 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index fa1929ca6ad4..3b43b42f9abe 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -91,6 +91,19 @@ CBPF_MPTCP_SUBOPTION_ADD_ADDR="14,
 			       6 0 0 65535,
 			       6 0 0 0"
 
+set_path_manager()
+{
+	local ns=$1
+	local pm=$2
+
+	if ! ip netns exec ${ns} sysctl net.mptcp.available_path_managers |
+	     grep -wq "${pm}"; then
+		mptcp_lib_pr_fail "path manager ${pm} not found"
+		return 1
+	fi
+	ip netns exec ${ns} sysctl -q net.mptcp.path_manager="${pm}"
+}
+
 init_partial()
 {
 	capout=$(mktemp)
@@ -3619,7 +3632,26 @@ userspace_tests()
 	# userspace pm create destroy subflow
 	if reset_with_events "userspace pm create destroy subflow" &&
 	   continue_if mptcp_lib_has_file '/proc/sys/net/mptcp/pm_type'; then
-		set_userspace_pm $ns2
+		if continue_if mptcp_lib_has_file '/proc/sys/net/mptcp/path_manager'; then
+			local pm1 pm2
+
+			set_path_manager $ns1 "kernel"
+			set_path_manager $ns2 "userspace"
+
+			pm1=$(ip netns exec ${ns1} sysctl -n net.mptcp.pm_type)
+			if [ "$pm1" != "0" ]; then
+				mptcp_lib_pr_fail "ns1 pm_type mapping fails"
+				return 1
+			fi
+
+			pm2=$(ip netns exec ${ns2} sysctl -n net.mptcp.pm_type)
+			if [ "$pm2" != "1" ]; then
+				mptcp_lib_pr_fail "ns2 pm_type mapping fails"
+				return 1
+			fi
+		else
+			set_userspace_pm $ns2
+		fi
 		pm_nl_set_limits $ns1 0 1
 		{ speed=5 \
 			run_tests $ns1 $ns2 10.0.1.1 & } 2>/dev/null
-- 
2.43.0

Re: [PATCH mptcp-next v8 12/12] selftests: mptcp: add path_manager sysctl test

[Matthieu Baerts]

Hi Geliang,

On 04/03/2025 12:40, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> This patch adds a new helper set_path_manager() to set the newly added
> net.mptcp.path_manager, and test it inside a userspace pm test.
> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  .../testing/selftests/net/mptcp/mptcp_join.sh | 34 ++++++++++++++++++-
>  1 file changed, 33 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index fa1929ca6ad4..3b43b42f9abe 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -91,6 +91,19 @@ CBPF_MPTCP_SUBOPTION_ADD_ADDR="14,
>  			       6 0 0 65535,
>  			       6 0 0 0"
>  
> +set_path_manager()
> +{
> +	local ns=$1
> +	local pm=$2
> +
> +	if ! ip netns exec ${ns} sysctl net.mptcp.available_path_managers |
> +	     grep -wq "${pm}"; then
> +		mptcp_lib_pr_fail "path manager ${pm} not found"
> +		return 1
> +	fi
> +	ip netns exec ${ns} sysctl -q net.mptcp.path_manager="${pm}"
> +}
> +
>  init_partial()
>  {
>  	capout=$(mktemp)
> @@ -3619,7 +3632,26 @@ userspace_tests()
>  	# userspace pm create destroy subflow
>  	if reset_with_events "userspace pm create destroy subflow" &&
>  	   continue_if mptcp_lib_has_file '/proc/sys/net/mptcp/pm_type'; then
> -		set_userspace_pm $ns2
> +		if continue_if mptcp_lib_has_file '/proc/sys/net/mptcp/path_manager'; then

Same here, we cannot have continue_if here.

> +			local pm1 pm2
> +
> +			set_path_manager $ns1 "kernel"
> +			set_path_manager $ns2 "userspace"
> +
> +			pm1=$(ip netns exec ${ns1} sysctl -n net.mptcp.pm_type)
> +			if [ "$pm1" != "0" ]; then
> +				mptcp_lib_pr_fail "ns1 pm_type mapping fails"
> +				return 1
> +			fi
> +
> +			pm2=$(ip netns exec ${ns2} sysctl -n net.mptcp.pm_type)
> +			if [ "$pm2" != "1" ]; then
> +				mptcp_lib_pr_fail "ns2 pm_type mapping fails"
> +				return 1
> +			fi

I think it would be better to do that in userspace_pm.sh as suggested in
my review from patch 11/12. So replacing patches 11 and 12 by this one. No?

> +		else
> +			set_userspace_pm $ns2
> +		fi
>  		pm_nl_set_limits $ns1 0 1
>  		{ speed=5 \
>  			run_tests $ns1 $ns2 10.0.1.1 & } 2>/dev/null

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

Re: [PATCH mptcp-next v8 00/12] BPF path manager, part 5

[Matthieu Baerts]

Hi Geliang,

On 04/03/2025 12:40, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> v8:
>  - address Matt's comments in v7.

Thank you for the new version 8.

Please next time add an individual changelog, it is hard to follow the
modifications between versions without that.

I have some comments, please see my individual reviews.

> v7:
>  - addresss Matt's comments in v6 [1].
>  - drop "type" from struct mptcp_pm_ops as Matt suggested.
>  - map "pm_type" to new sysctl as Matt suggested.
> 
> Depends on:
>  - mptcp: pm: code reorganisation, v2
> 
> Based-on: <20250228-mptcp-pm-reorg-code-v2-0-fa8b2542b7a5@kernel.org>

Please drop this line next time, the CI didn't manage to apply and
validate this series because of that.

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