[PATCH mptcp-next v5 0/9] BPF path manager, part 4

[PATCH mptcp-next v5 0/9] BPF path manager, part 4

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

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 (9):
  mptcp: pm: use addr entry for get_local_id
  mptcp: pm: add struct mptcp_pm_param
  mptcp: pm: pass pm_param to get_local_id
  mptcp: pm: define struct mptcp_pm_ops
  mptcp: pm: in-kernel: register mptcp_netlink_pm
  mptcp: pm: userspace: register mptcp_userspace_pm
  mptcp: pm: initialize and release mptcp_pm_ops
  mptcp: pm: drop get_local_id helpers
  mptcp: pm: drop is_backup helpers

 include/net/mptcp.h      |  42 +++++++++++++
 net/mptcp/pm.c           | 131 ++++++++++++++++++++++++++++++++++++---
 net/mptcp/pm_netlink.c   |  23 +++++--
 net/mptcp/pm_userspace.c |  37 ++++++-----
 net/mptcp/protocol.c     |   1 +
 net/mptcp/protocol.h     |  24 +++----
 6 files changed, 216 insertions(+), 42 deletions(-)

-- 
2.43.0

[PATCH mptcp-next v5 1/9] mptcp: pm: use addr entry for get_local_id

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

The following code in mptcp_userspace_pm_get_local_id() that assigns "skc"
to "new_entry" is not allowed in BPF if we use the same code to implement
the get_local_id() interface of a BFP path manager:

	memset(&new_entry, 0, sizeof(struct mptcp_pm_addr_entry));
	new_entry.addr = *skc;
	new_entry.addr.id = 0;
	new_entry.flags = MPTCP_PM_ADDR_FLAG_IMPLICIT;

To solve the issue, this patch moves this assignment to "new_entry" forward
to mptcp_pm_get_local_id(), and then passing "new_entry" as a parameter to
both mptcp_pm_nl_get_local_id() and mptcp_userspace_pm_get_local_id().

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

diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index 16cacce6c10f..94620ab172b7 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -403,20 +403,25 @@ bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
 
 int mptcp_pm_get_local_id(struct mptcp_sock *msk, struct sock_common *skc)
 {
-	struct mptcp_addr_info skc_local;
+	struct mptcp_pm_addr_entry skc_local;
 	struct mptcp_addr_info msk_local;
 
 	if (WARN_ON_ONCE(!msk))
 		return -1;
 
+	memset(&skc_local, 0, sizeof(struct mptcp_pm_addr_entry));
+
 	/* The 0 ID mapping is defined by the first subflow, copied into the msk
 	 * addr
 	 */
 	mptcp_local_address((struct sock_common *)msk, &msk_local);
-	mptcp_local_address((struct sock_common *)skc, &skc_local);
-	if (mptcp_addresses_equal(&msk_local, &skc_local, false))
+	mptcp_local_address((struct sock_common *)skc, &skc_local.addr);
+	if (mptcp_addresses_equal(&msk_local, &skc_local.addr, false))
 		return 0;
 
+	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);
diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index d4328443d844..0a0fe890c53d 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -1139,7 +1139,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_addr_info *skc)
+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;
@@ -1148,7 +1149,7 @@ int mptcp_pm_nl_get_local_id(struct mptcp_sock *msk, struct mptcp_addr_info *skc
 	pernet = pm_nl_get_pernet_from_msk(msk);
 
 	rcu_read_lock();
-	entry = __lookup_addr(pernet, skc);
+	entry = __lookup_addr(pernet, &skc->addr);
 	ret = entry ? entry->addr.id : -1;
 	rcu_read_unlock();
 	if (ret >= 0)
@@ -1159,11 +1160,9 @@ int mptcp_pm_nl_get_local_id(struct mptcp_sock *msk, struct mptcp_addr_info *skc
 	if (!entry)
 		return -ENOMEM;
 
-	entry->addr = *skc;
-	entry->addr.id = 0;
+	*entry = *skc;
 	entry->addr.port = 0;
 	entry->ifindex = 0;
-	entry->flags = MPTCP_PM_ADDR_FLAG_IMPLICIT;
 	entry->lsk = NULL;
 	ret = mptcp_pm_nl_append_new_local_addr(pernet, entry, true);
 	if (ret < 0)
diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index 6bf6a20ef7f3..5b3ee43130be 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -131,27 +131,22 @@ mptcp_userspace_pm_lookup_addr_by_id(struct mptcp_sock *msk, unsigned int id)
 }
 
 int mptcp_userspace_pm_get_local_id(struct mptcp_sock *msk,
-				    struct mptcp_addr_info *skc)
+				    struct mptcp_pm_addr_entry *skc)
 {
-	struct mptcp_pm_addr_entry *entry = NULL, new_entry;
 	__be16 msk_sport =  ((struct inet_sock *)
 			     inet_sk((struct sock *)msk))->inet_sport;
+	struct mptcp_pm_addr_entry *entry;
 
 	spin_lock_bh(&msk->pm.lock);
-	entry = mptcp_userspace_pm_lookup_addr(msk, skc);
+	entry = mptcp_userspace_pm_lookup_addr(msk, &skc->addr);
 	spin_unlock_bh(&msk->pm.lock);
 	if (entry)
 		return entry->addr.id;
 
-	memset(&new_entry, 0, sizeof(struct mptcp_pm_addr_entry));
-	new_entry.addr = *skc;
-	new_entry.addr.id = 0;
-	new_entry.flags = MPTCP_PM_ADDR_FLAG_IMPLICIT;
-
-	if (new_entry.addr.port == msk_sport)
-		new_entry.addr.port = 0;
+	if (skc->addr.port == msk_sport)
+		skc->addr.port = 0;
 
-	return mptcp_userspace_pm_append_new_local_addr(msk, &new_entry, true);
+	return mptcp_userspace_pm_append_new_local_addr(msk, skc, true);
 }
 
 bool mptcp_userspace_pm_is_backup(struct mptcp_sock *msk,
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 3d72ca155322..ef1d43406f9b 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -1126,8 +1126,10 @@ 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_addr_info *skc);
-int mptcp_userspace_pm_get_local_id(struct mptcp_sock *msk, struct mptcp_addr_info *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

Re: [PATCH mptcp-next v5 1/9] mptcp: pm: use addr entry for get_local_id

[Matthieu Baerts]

Hi Geliang,

On 20/02/2025 03:57, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> The following code in mptcp_userspace_pm_get_local_id() that assigns "skc"
> to "new_entry" is not allowed in BPF if we use the same code to implement
> the get_local_id() interface of a BFP path manager:
> 
> 	memset(&new_entry, 0, sizeof(struct mptcp_pm_addr_entry));
> 	new_entry.addr = *skc;
> 	new_entry.addr.id = 0;
> 	new_entry.flags = MPTCP_PM_ADDR_FLAG_IMPLICIT;
> 
> To solve the issue, this patch moves this assignment to "new_entry" forward
> to mptcp_pm_get_local_id(), and then passing "new_entry" as a parameter to
> both mptcp_pm_nl_get_local_id() and mptcp_userspace_pm_get_local_id().
> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  net/mptcp/pm.c           | 11 ++++++++---
>  net/mptcp/pm_netlink.c   |  9 ++++-----
>  net/mptcp/pm_userspace.c | 17 ++++++-----------
>  net/mptcp/protocol.h     |  6 ++++--
>  4 files changed, 22 insertions(+), 21 deletions(-)
> 
> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index 16cacce6c10f..94620ab172b7 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c
> @@ -403,20 +403,25 @@ bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
>  
>  int mptcp_pm_get_local_id(struct mptcp_sock *msk, struct sock_common *skc)
>  {
> -	struct mptcp_addr_info skc_local;
> +	struct mptcp_pm_addr_entry skc_local;
>  	struct mptcp_addr_info msk_local;
>  
>  	if (WARN_ON_ONCE(!msk))
>  		return -1;
>  
> +	memset(&skc_local, 0, sizeof(struct mptcp_pm_addr_entry));

Detail: do we need memset? Can you not initialise it to 0 instead?

  struct mptcp_pm_addr_entry skc_local = { 0 };

> +
>  	/* The 0 ID mapping is defined by the first subflow, copied into the msk
>  	 * addr
>  	 */
>  	mptcp_local_address((struct sock_common *)msk, &msk_local);
> -	mptcp_local_address((struct sock_common *)skc, &skc_local);
> -	if (mptcp_addresses_equal(&msk_local, &skc_local, false))
> +	mptcp_local_address((struct sock_common *)skc, &skc_local.addr);
> +	if (mptcp_addresses_equal(&msk_local, &skc_local.addr, false))
>  		return 0;
>  
> +	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);
> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index d4328443d844..0a0fe890c53d 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c

(...)

> @@ -1159,11 +1160,9 @@ int mptcp_pm_nl_get_local_id(struct mptcp_sock *msk, struct mptcp_addr_info *skc
>  	if (!entry)
>  		return -ENOMEM;
>  
> -	entry->addr = *skc;
> -	entry->addr.id = 0;
> +	*entry = *skc;
>  	entry->addr.port = 0;
>  	entry->ifindex = 0;
> -	entry->flags = MPTCP_PM_ADDR_FLAG_IMPLICIT;
>  	entry->lsk = NULL;
Small detail: is it still needed to reset all these info (except the
port number)?

If I'm not mistaken, now all the "entry" should be set to 0, no?

>  	ret = mptcp_pm_nl_append_new_local_addr(pernet, entry, true);
>  	if (ret < 0)
Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.

[PATCH mptcp-next v5 2/9] mptcp: pm: add struct mptcp_pm_param

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

Generally, in the path manager interfaces, the local address is defined
as an mptcp_pm_addr_entry type address, while the remote address is
defined as an mptcp_addr_info type one:

    (struct mptcp_pm_addr_entry *local, struct mptcp_addr_info *remote)

In order to make these interfaces more flexible and extensible, a struct
mptcp_pm_param is defined here to pass parameters. "entry" can be used
as the local address entry, and "addr" can be used as the remote address.

Also add a new helper mptcp_pm_param_set_contexts() to set a struct
mptcp_pm_param type parameter.

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

diff --git a/include/net/mptcp.h b/include/net/mptcp.h
index 72d6e6597add..a41d6c74760f 100644
--- a/include/net/mptcp.h
+++ b/include/net/mptcp.h
@@ -121,6 +121,19 @@ struct mptcp_sched_ops {
 	void (*release)(struct mptcp_sock *msk);
 } ____cacheline_aligned_in_smp;
 
+struct mptcp_pm_addr_entry {
+	struct list_head	list;
+	struct mptcp_addr_info	addr;
+	u8			flags;
+	int			ifindex;
+	struct socket		*lsk;
+};
+
+struct mptcp_pm_param {
+	struct mptcp_pm_addr_entry	entry;
+	struct mptcp_addr_info		addr;
+};
+
 #ifdef CONFIG_MPTCP
 void mptcp_init(void);
 
diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index 94620ab172b7..6a504c870e1a 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -401,6 +401,16 @@ bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
 	return ret;
 }
 
+void mptcp_pm_param_set_contexts(struct mptcp_pm_param *param,
+				 const struct mptcp_pm_addr_entry *entry,
+				 const struct mptcp_addr_info *addr)
+{
+	if (entry)
+		param->entry = *entry;
+	if (addr)
+		param->addr = *addr;
+}
+
 int mptcp_pm_get_local_id(struct mptcp_sock *msk, struct sock_common *skc)
 {
 	struct mptcp_pm_addr_entry skc_local;
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index ef1d43406f9b..dbcf4b84e0f0 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -246,14 +246,6 @@ struct mptcp_pm_local {
 	int			ifindex;
 };
 
-struct mptcp_pm_addr_entry {
-	struct list_head	list;
-	struct mptcp_addr_info	addr;
-	u8			flags;
-	int			ifindex;
-	struct socket		*lsk;
-};
-
 struct mptcp_data_frag {
 	struct list_head list;
 	u64 data_seq;
@@ -1125,6 +1117,9 @@ bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, const struct sk_buff *skb,
 			      bool *drop_other_suboptions);
 bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
 			     struct mptcp_rm_list *rm_list);
+void mptcp_pm_param_set_contexts(struct mptcp_pm_param *param,
+				 const struct mptcp_pm_addr_entry *entry,
+				 const struct mptcp_addr_info *addr);
 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);
-- 
2.43.0

Re: [PATCH mptcp-next v5 2/9] mptcp: pm: add struct mptcp_pm_param

[Matthieu Baerts]

Hi Geliang,

On 20/02/2025 03:57, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> Generally, in the path manager interfaces, the local address is defined
> as an mptcp_pm_addr_entry type address, while the remote address is
> defined as an mptcp_addr_info type one:
> 
>     (struct mptcp_pm_addr_entry *local, struct mptcp_addr_info *remote)
> 
> In order to make these interfaces more flexible and extensible, a struct
> mptcp_pm_param is defined here to pass parameters. "entry" can be used
> as the local address entry, and "addr" can be used as the remote address.

Mmh, it is not clear to me why you cannot use only the parameters that
are needed per interfaces, e.g. "local" and "remote". These parameters
are not even always needed, e.g. when an address has been announced,
"remote" doesn't make sense here. Same for get_local_id and
get_priority. Similarly, when an address has been removed, only the ID
is needed for the PM, etc.

I'm not convinced by the "flexible and extensible" reasons for the
future: "local" and "remote" will likely not change. If we want to add
more parameters, will we require them for all interfaces?

Also, if you use similar signatures as the ones being used, no need to
do many changes in the current in-kernel and userspace PM, e.g. no need
to introduce the new helpers in patches 5 and 6. Instead, these patches
could directly set the available helpers, and we can reduce the number
of patches, e.g. patches 8 and 9 (and more) could all come in one, no?

Plus, it feels strange to copy data from one structure to another just
to pass parameters, no?

Are you sure we need this patch? (and the next one?)

> Also add a new helper mptcp_pm_param_set_contexts() to set a struct
> mptcp_pm_param type parameter.
> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  include/net/mptcp.h  | 13 +++++++++++++
>  net/mptcp/pm.c       | 10 ++++++++++
>  net/mptcp/protocol.h | 11 +++--------
>  3 files changed, 26 insertions(+), 8 deletions(-)
> 
> diff --git a/include/net/mptcp.h b/include/net/mptcp.h
> index 72d6e6597add..a41d6c74760f 100644
> --- a/include/net/mptcp.h
> +++ b/include/net/mptcp.h
> @@ -121,6 +121,19 @@ struct mptcp_sched_ops {
>  	void (*release)(struct mptcp_sock *msk);
>  } ____cacheline_aligned_in_smp;
>  
> +struct mptcp_pm_addr_entry {
> +	struct list_head	list;
> +	struct mptcp_addr_info	addr;
> +	u8			flags;
> +	int			ifindex;
> +	struct socket		*lsk;
> +};
> +
> +struct mptcp_pm_param {
> +	struct mptcp_pm_addr_entry	entry;
> +	struct mptcp_addr_info		addr;
If this structure is really needed, it seems clearer to use "local" and
"remote" instead of "entry" and "addr".

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

[PATCH mptcp-next v5 3/9] mptcp: pm: pass pm_param to get_local_id

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

This patch changes the 2nd parameter of get_local_id() interface as
"struct mptcp_pm_param" type. Only "entry" member of this struct is
used.

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

diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index 6a504c870e1a..e3457f34621c 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -415,6 +415,7 @@ int mptcp_pm_get_local_id(struct mptcp_sock *msk, struct sock_common *skc)
 {
 	struct mptcp_pm_addr_entry skc_local;
 	struct mptcp_addr_info msk_local;
+	struct mptcp_pm_param param;
 
 	if (WARN_ON_ONCE(!msk))
 		return -1;
@@ -432,9 +433,10 @@ 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;
 
+	mptcp_pm_param_set_contexts(&param, &skc_local, NULL);
 	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 mptcp_userspace_pm_get_local_id(msk, &param);
+	return mptcp_pm_nl_get_local_id(msk, &param);
 }
 
 bool mptcp_pm_is_backup(struct mptcp_sock *msk, struct sock_common *skc)
diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index 0a0fe890c53d..3709ad2c06ef 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -1140,8 +1140,9 @@ static int mptcp_pm_nl_create_listen_socket(struct sock *sk,
 }
 
 int mptcp_pm_nl_get_local_id(struct mptcp_sock *msk,
-			     struct mptcp_pm_addr_entry *skc)
+			     struct mptcp_pm_param *param)
 {
+	struct mptcp_pm_addr_entry *skc = &param->entry;
 	struct mptcp_pm_addr_entry *entry;
 	struct pm_nl_pernet *pernet;
 	int ret;
diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index 5b3ee43130be..bd235b2d18b3 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -131,10 +131,11 @@ mptcp_userspace_pm_lookup_addr_by_id(struct mptcp_sock *msk, unsigned int id)
 }
 
 int mptcp_userspace_pm_get_local_id(struct mptcp_sock *msk,
-				    struct mptcp_pm_addr_entry *skc)
+				    struct mptcp_pm_param *param)
 {
 	__be16 msk_sport =  ((struct inet_sock *)
 			     inet_sk((struct sock *)msk))->inet_sport;
+	struct mptcp_pm_addr_entry *skc = &param->entry;
 	struct mptcp_pm_addr_entry *entry;
 
 	spin_lock_bh(&msk->pm.lock);
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index dbcf4b84e0f0..7987beaa730e 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -1122,9 +1122,9 @@ void mptcp_pm_param_set_contexts(struct mptcp_pm_param *param,
 				 const struct mptcp_addr_info *addr);
 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);
+			     struct mptcp_pm_param *param);
 int mptcp_userspace_pm_get_local_id(struct mptcp_sock *msk,
-				    struct mptcp_pm_addr_entry *skc);
+				    struct mptcp_pm_param *param);
 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 v5 4/9] 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  | 29 ++++++++++++++++++++++
 net/mptcp/pm.c       | 59 ++++++++++++++++++++++++++++++++++++++++++++
 net/mptcp/protocol.h |  5 ++++
 3 files changed, 93 insertions(+)

diff --git a/include/net/mptcp.h b/include/net/mptcp.h
index a41d6c74760f..f51e75d3882d 100644
--- a/include/net/mptcp.h
+++ b/include/net/mptcp.h
@@ -134,6 +134,35 @@ struct mptcp_pm_param {
 	struct mptcp_addr_info		addr;
 };
 
+struct mptcp_pm_ops {
+	int (*created)(struct mptcp_sock *msk);
+	int (*established)(struct mptcp_sock *msk);
+	int (*closed)(struct mptcp_sock *msk);
+	int (*address_announced)(struct mptcp_sock *msk,
+				 struct mptcp_pm_param *param);
+	int (*address_removed)(struct mptcp_sock *msk,
+			       struct mptcp_pm_param *param);
+	int (*subflow_established)(struct mptcp_sock *msk,
+				   struct mptcp_pm_param *param);
+	int (*subflow_closed)(struct mptcp_sock *msk,
+			      struct mptcp_pm_param *param);
+	int (*get_local_id)(struct mptcp_sock *msk,
+			    struct mptcp_pm_param *param);
+	bool (*get_priority)(struct mptcp_sock *msk,
+			     struct mptcp_pm_param *param);
+	int (*set_priority)(struct mptcp_sock *msk,
+			    struct mptcp_pm_param *param);
+	int (*listener_created)(struct mptcp_sock *msk);
+	int (*listener_closed)(struct mptcp_sock *msk);
+
+	u8			type;
+	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 e3457f34621c..f56b2d1e3409 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -6,12 +6,17 @@
 #define pr_fmt(fmt) "MPTCP: " fmt
 
 #include <linux/kernel.h>
+#include <linux/rculist.h>
+#include <linux/spinlock.h>
 #include <net/mptcp.h>
 #include "protocol.h"
 
 #include "mib.h"
 #include "mptcp_pm_gen.h"
 
+static DEFINE_SPINLOCK(mptcp_pm_list_lock);
+static LIST_HEAD(mptcp_pm_list);
+
 /* path manager command handlers */
 
 int mptcp_pm_announce_addr(struct mptcp_sock *msk,
@@ -661,3 +666,57 @@ void __init mptcp_pm_init(void)
 {
 	mptcp_pm_nl_init();
 }
+
+/* Must be called with rcu read lock held */
+struct mptcp_pm_ops *mptcp_pm_find(enum mptcp_pm_type type)
+{
+	struct mptcp_pm_ops *pm;
+
+	list_for_each_entry_rcu(pm, &mptcp_pm_list, list) {
+		if (pm->type == type)
+			return pm;
+	}
+
+	return NULL;
+}
+
+int mptcp_pm_validate(struct mptcp_pm_ops *pm)
+{
+	if (!pm->created && !pm->established && !pm->closed &&
+	    !pm->address_announced && !pm->address_removed &&
+	    !pm->subflow_established && !pm->subflow_closed &&
+	    !pm->get_local_id && !pm->get_priority && !pm->set_priority &&
+	    !pm->listener_created && !pm->listener_closed) {
+		pr_err("%u does not implement required ops\n", pm->type);
+		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->type)) {
+		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("userspace_pm type %u registered\n", pm->type);
+	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 7987beaa730e..f3e04927e214 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -1039,6 +1039,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(enum mptcp_pm_type type);
+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_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 v5 4/9] mptcp: pm: define struct mptcp_pm_ops

[Matthieu Baerts]

Hi Geliang,

On 20/02/2025 03:57, 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.
> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  include/net/mptcp.h  | 29 ++++++++++++++++++++++
>  net/mptcp/pm.c       | 59 ++++++++++++++++++++++++++++++++++++++++++++
>  net/mptcp/protocol.h |  5 ++++
>  3 files changed, 93 insertions(+)
> 
> diff --git a/include/net/mptcp.h b/include/net/mptcp.h
> index a41d6c74760f..f51e75d3882d 100644
> --- a/include/net/mptcp.h
> +++ b/include/net/mptcp.h
> @@ -134,6 +134,35 @@ struct mptcp_pm_param {
>  	struct mptcp_addr_info		addr;
>  };
>  
> +struct mptcp_pm_ops {
> +	int (*created)(struct mptcp_sock *msk);
> +	int (*established)(struct mptcp_sock *msk);
> +	int (*closed)(struct mptcp_sock *msk);
> +	int (*address_announced)(struct mptcp_sock *msk,
> +				 struct mptcp_pm_param *param);
> +	int (*address_removed)(struct mptcp_sock *msk,
> +			       struct mptcp_pm_param *param);
> +	int (*subflow_established)(struct mptcp_sock *msk,
> +				   struct mptcp_pm_param *param);
> +	int (*subflow_closed)(struct mptcp_sock *msk,
> +			      struct mptcp_pm_param *param);
> +	int (*get_local_id)(struct mptcp_sock *msk,
> +			    struct mptcp_pm_param *param);
> +	bool (*get_priority)(struct mptcp_sock *msk,
> +			     struct mptcp_pm_param *param);
> +	int (*set_priority)(struct mptcp_sock *msk,
> +			    struct mptcp_pm_param *param);
> +	int (*listener_created)(struct mptcp_sock *msk);
> +	int (*listener_closed)(struct mptcp_sock *msk);
> +
> +	u8			type;

I guess the type matches net.mptcp.pm_type sysctl knob, right?

I wonder if we should not deprecate this sysctl, and use a string like
with the scheduler. So instead, we could have:

  char			name[MPTCP_PM_NAME_MAX];

And on ctrl.c, we could map pm_type for the moment with a custom
proc_handler (and even remove this sysctl knob in a few releases):

 - 0  → in-kernel
 - 1  → userspace
 - >1 → bpf

WDYT? Would it not be clearer for devs and users?

Note that if for the implementation, if it is easier to keep this "type"
entry for the moment for the sysctl stuff, I'm fine with that. But if we
don't need it, let's not introduce it.

> +	struct module		*owner;
> +	struct list_head	list;
> +
> +	void (*init)(struct mptcp_sock *msk);
> +	void (*release)(struct mptcp_sock *msk);

To answer my question from the v3 review: the init/release is done for
each MPTCP connection handled by this PM.

> +} ____cacheline_aligned_in_smp;
> +
>  #ifdef CONFIG_MPTCP
>  void mptcp_init(void);
>  
> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index e3457f34621c..f56b2d1e3409 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c
> @@ -6,12 +6,17 @@
>  #define pr_fmt(fmt) "MPTCP: " fmt
>  
>  #include <linux/kernel.h>
> +#include <linux/rculist.h>
> +#include <linux/spinlock.h>
>  #include <net/mptcp.h>
>  #include "protocol.h"
>  
>  #include "mib.h"
>  #include "mptcp_pm_gen.h"
>  
> +static DEFINE_SPINLOCK(mptcp_pm_list_lock);
> +static LIST_HEAD(mptcp_pm_list);
> +
>  /* path manager command handlers */
>  
>  int mptcp_pm_announce_addr(struct mptcp_sock *msk,
> @@ -661,3 +666,57 @@ void __init mptcp_pm_init(void)
>  {
>  	mptcp_pm_nl_init();
>  }
> +
> +/* Must be called with rcu read lock held */
> +struct mptcp_pm_ops *mptcp_pm_find(enum mptcp_pm_type type)
> +{
> +	struct mptcp_pm_ops *pm;
> +
> +	list_for_each_entry_rcu(pm, &mptcp_pm_list, list) {
> +		if (pm->type == type)
> +			return pm;
> +	}
> +
> +	return NULL;
> +}
> +
> +int mptcp_pm_validate(struct mptcp_pm_ops *pm)
> +{
> +	if (!pm->created && !pm->established && !pm->closed &&
> +	    !pm->address_announced && !pm->address_removed &&
> +	    !pm->subflow_established && !pm->subflow_closed &&
> +	    !pm->get_local_id && !pm->get_priority && !pm->set_priority &&
> +	    !pm->listener_created && !pm->listener_closed) {

I'm not sure to understand the purpose of this validation.

Why not forcing some or all of them?

(...)

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

Re: [PATCH mptcp-next v5 4/9] mptcp: pm: define struct mptcp_pm_ops

[Geliang Tang]

Hi Matt,

Thanks for the review.

On Fri, 2025-02-21 at 18:23 +0100, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 20/02/2025 03:57, 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.
> > 
> > Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> > ---
> >  include/net/mptcp.h  | 29 ++++++++++++++++++++++
> >  net/mptcp/pm.c       | 59
> > ++++++++++++++++++++++++++++++++++++++++++++
> >  net/mptcp/protocol.h |  5 ++++
> >  3 files changed, 93 insertions(+)
> > 
> > diff --git a/include/net/mptcp.h b/include/net/mptcp.h
> > index a41d6c74760f..f51e75d3882d 100644
> > --- a/include/net/mptcp.h
> > +++ b/include/net/mptcp.h
> > @@ -134,6 +134,35 @@ struct mptcp_pm_param {
> >  	struct mptcp_addr_info		addr;
> >  };
> >  
> > +struct mptcp_pm_ops {
> > +	int (*created)(struct mptcp_sock *msk);
> > +	int (*established)(struct mptcp_sock *msk);
> > +	int (*closed)(struct mptcp_sock *msk);
> > +	int (*address_announced)(struct mptcp_sock *msk,
> > +				 struct mptcp_pm_param *param);
> > +	int (*address_removed)(struct mptcp_sock *msk,
> > +			       struct mptcp_pm_param *param);
> > +	int (*subflow_established)(struct mptcp_sock *msk,
> > +				   struct mptcp_pm_param *param);
> > +	int (*subflow_closed)(struct mptcp_sock *msk,
> > +			      struct mptcp_pm_param *param);
> > +	int (*get_local_id)(struct mptcp_sock *msk,
> > +			    struct mptcp_pm_param *param);
> > +	bool (*get_priority)(struct mptcp_sock *msk,
> > +			     struct mptcp_pm_param *param);
> > +	int (*set_priority)(struct mptcp_sock *msk,
> > +			    struct mptcp_pm_param *param);
> > +	int (*listener_created)(struct mptcp_sock *msk);
> > +	int (*listener_closed)(struct mptcp_sock *msk);
> > +
> > +	u8			type;
> 
> I guess the type matches net.mptcp.pm_type sysctl knob, right?
> 
> I wonder if we should not deprecate this sysctl, and use a string
> like
> with the scheduler. So instead, we could have:
> 
>   char			name[MPTCP_PM_NAME_MAX];
> 
> And on ctrl.c, we could map pm_type for the moment with a custom
> proc_handler (and even remove this sysctl knob in a few releases):
> 
>  - 0  → in-kernel
>  - 1  → userspace
>  - >1 → bpf

Do we need to restrict BPF path manager to only extend the userspace
type path manager? That is, for all BPF path managers,
mptcp_pm_is_userspace() returns true. If this is the case, there is no
need to add "type" field in struct mptcp_pm_ops.

Is it necessary to allow BPF to create kernel type path managers, that
is, path managers for which mptcp_pm_is_userspace() returns false?
Since we registered mptcp_netlink_pm from this version, can the in-
kernel type path manager also be extended by BPF? If this is the case,
we need to add "type" field in struct mptcp_pm_ops to distinguish
whether it is a userspace type path manager.

> 
> WDYT? Would it not be clearer for devs and users?
> 
> Note that if for the implementation, if it is easier to keep this
> "type"
> entry for the moment for the sysctl stuff, I'm fine with that. But if
> we
> don't need it, let's not introduce it.
> 
> > +	struct module		*owner;
> > +	struct list_head	list;
> > +
> > +	void (*init)(struct mptcp_sock *msk);
> > +	void (*release)(struct mptcp_sock *msk);
> 
> To answer my question from the v3 review: the init/release is done
> for
> each MPTCP connection handled by this PM.

In-kernel pm is for each MPTCP connection handled by this PM while
userspace pm is only for this one being initialized. How should I deal
with this? Please give me more detailed suggestions.

Thanks,
-Geliang

> 
> > +} ____cacheline_aligned_in_smp;
> > +
> >  #ifdef CONFIG_MPTCP
> >  void mptcp_init(void);
> >  
> > diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> > index e3457f34621c..f56b2d1e3409 100644
> > --- a/net/mptcp/pm.c
> > +++ b/net/mptcp/pm.c
> > @@ -6,12 +6,17 @@
> >  #define pr_fmt(fmt) "MPTCP: " fmt
> >  
> >  #include <linux/kernel.h>
> > +#include <linux/rculist.h>
> > +#include <linux/spinlock.h>
> >  #include <net/mptcp.h>
> >  #include "protocol.h"
> >  
> >  #include "mib.h"
> >  #include "mptcp_pm_gen.h"
> >  
> > +static DEFINE_SPINLOCK(mptcp_pm_list_lock);
> > +static LIST_HEAD(mptcp_pm_list);
> > +
> >  /* path manager command handlers */
> >  
> >  int mptcp_pm_announce_addr(struct mptcp_sock *msk,
> > @@ -661,3 +666,57 @@ void __init mptcp_pm_init(void)
> >  {
> >  	mptcp_pm_nl_init();
> >  }
> > +
> > +/* Must be called with rcu read lock held */
> > +struct mptcp_pm_ops *mptcp_pm_find(enum mptcp_pm_type type)
> > +{
> > +	struct mptcp_pm_ops *pm;
> > +
> > +	list_for_each_entry_rcu(pm, &mptcp_pm_list, list) {
> > +		if (pm->type == type)
> > +			return pm;
> > +	}
> > +
> > +	return NULL;
> > +}
> > +
> > +int mptcp_pm_validate(struct mptcp_pm_ops *pm)
> > +{
> > +	if (!pm->created && !pm->established && !pm->closed &&
> > +	    !pm->address_announced && !pm->address_removed &&
> > +	    !pm->subflow_established && !pm->subflow_closed &&
> > +	    !pm->get_local_id && !pm->get_priority && !pm-
> > >set_priority &&
> > +	    !pm->listener_created && !pm->listener_closed) {
> 
> I'm not sure to understand the purpose of this validation.
> 
> Why not forcing some or all of them?
> 
> (...)
> 
> Cheers,
> Matt

Re: [PATCH mptcp-next v5 4/9] mptcp: pm: define struct mptcp_pm_ops

[Matthieu Baerts]

Hi Geliang,

On 24/02/2025 07:54, Geliang Tang wrote:
> Hi Matt,
> 
> Thanks for the review.
> 
> On Fri, 2025-02-21 at 18:23 +0100, Matthieu Baerts wrote:
>> Hi Geliang,
>>
>> On 20/02/2025 03:57, 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.
>>>
>>> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
>>> ---
>>>  include/net/mptcp.h  | 29 ++++++++++++++++++++++
>>>  net/mptcp/pm.c       | 59
>>> ++++++++++++++++++++++++++++++++++++++++++++
>>>  net/mptcp/protocol.h |  5 ++++
>>>  3 files changed, 93 insertions(+)
>>>
>>> diff --git a/include/net/mptcp.h b/include/net/mptcp.h
>>> index a41d6c74760f..f51e75d3882d 100644
>>> --- a/include/net/mptcp.h
>>> +++ b/include/net/mptcp.h
>>> @@ -134,6 +134,35 @@ struct mptcp_pm_param {
>>>  	struct mptcp_addr_info		addr;
>>>  };
>>>  
>>> +struct mptcp_pm_ops {
>>> +	int (*created)(struct mptcp_sock *msk);
>>> +	int (*established)(struct mptcp_sock *msk);
>>> +	int (*closed)(struct mptcp_sock *msk);
>>> +	int (*address_announced)(struct mptcp_sock *msk,
>>> +				 struct mptcp_pm_param *param);
>>> +	int (*address_removed)(struct mptcp_sock *msk,
>>> +			       struct mptcp_pm_param *param);
>>> +	int (*subflow_established)(struct mptcp_sock *msk,
>>> +				   struct mptcp_pm_param *param);
>>> +	int (*subflow_closed)(struct mptcp_sock *msk,
>>> +			      struct mptcp_pm_param *param);
>>> +	int (*get_local_id)(struct mptcp_sock *msk,
>>> +			    struct mptcp_pm_param *param);
>>> +	bool (*get_priority)(struct mptcp_sock *msk,
>>> +			     struct mptcp_pm_param *param);
>>> +	int (*set_priority)(struct mptcp_sock *msk,
>>> +			    struct mptcp_pm_param *param);
>>> +	int (*listener_created)(struct mptcp_sock *msk);
>>> +	int (*listener_closed)(struct mptcp_sock *msk);
>>> +
>>> +	u8			type;
>>
>> I guess the type matches net.mptcp.pm_type sysctl knob, right?
>>
>> I wonder if we should not deprecate this sysctl, and use a string
>> like
>> with the scheduler. So instead, we could have:
>>
>>   char			name[MPTCP_PM_NAME_MAX];
>>
>> And on ctrl.c, we could map pm_type for the moment with a custom
>> proc_handler (and even remove this sysctl knob in a few releases):
>>
>>  - 0  → in-kernel
>>  - 1  → userspace
>>  - >1 → bpf
> 
> Do we need to restrict BPF path manager to only extend the userspace
> type path manager? That is, for all BPF path managers,
> mptcp_pm_is_userspace() returns true. If this is the case, there is no
> need to add "type" field in struct mptcp_pm_ops.

To me, mptcp_pm_is_userspace() should no longer be needed, because
pm->ops will be used instead. So the "type" field should no longer be
needed. Only the name should be stored, and the net.mptcp.pm_type sysctl
knob should depend only on the name. (I guess a lock will be needed to
handle the case where both the "pm_type" and "path_manager" will be set
at the same time.)

> Is it necessary to allow BPF to create kernel type path managers, that
> is, path managers for which mptcp_pm_is_userspace() returns false?
> Since we registered mptcp_netlink_pm from this version, can the in-
> kernel type path manager also be extended by BPF? If this is the case,
> we need to add "type" field in struct mptcp_pm_ops to distinguish
> whether it is a userspace type path manager.

I guess a BPF path-manager will have its own mptcp_pm_ops structure, and
it will not extend anything. Of course, it is possible to add kfunc to
share some code. No?

>> WDYT? Would it not be clearer for devs and users?
>>
>> Note that if for the implementation, if it is easier to keep this
>> "type"
>> entry for the moment for the sysctl stuff, I'm fine with that. But if
>> we
>> don't need it, let's not introduce it.
>>
>>> +	struct module		*owner;
>>> +	struct list_head	list;
>>> +
>>> +	void (*init)(struct mptcp_sock *msk);
>>> +	void (*release)(struct mptcp_sock *msk);
>>
>> To answer my question from the v3 review: the init/release is done
>> for
>> each MPTCP connection handled by this PM.
> 
> In-kernel pm is for each MPTCP connection handled by this PM while
> userspace pm is only for this one being initialized. How should I deal
> with this? Please give me more detailed suggestions.

Sorry, I'm not sure to understand your question. If one PM doesn't need
to init and/or release anything, these ops can be set to NULL, no?

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

Re: [PATCH mptcp-next v5 4/9] mptcp: pm: define struct mptcp_pm_ops

[Geliang Tang]

Hi Matt,

On Mon, 2025-02-24 at 09:26 +0100, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 24/02/2025 07:54, Geliang Tang wrote:
> > Hi Matt,
> > 
> > Thanks for the review.
> > 
> > On Fri, 2025-02-21 at 18:23 +0100, Matthieu Baerts wrote:
> > > Hi Geliang,
> > > 
> > > On 20/02/2025 03:57, 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.
> > > > 
> > > > Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> > > > ---
> > > >  include/net/mptcp.h  | 29 ++++++++++++++++++++++
> > > >  net/mptcp/pm.c       | 59
> > > > ++++++++++++++++++++++++++++++++++++++++++++
> > > >  net/mptcp/protocol.h |  5 ++++
> > > >  3 files changed, 93 insertions(+)
> > > > 
> > > > diff --git a/include/net/mptcp.h b/include/net/mptcp.h
> > > > index a41d6c74760f..f51e75d3882d 100644
> > > > --- a/include/net/mptcp.h
> > > > +++ b/include/net/mptcp.h
> > > > @@ -134,6 +134,35 @@ struct mptcp_pm_param {
> > > >  	struct mptcp_addr_info		addr;
> > > >  };
> > > >  
> > > > +struct mptcp_pm_ops {
> > > > +	int (*created)(struct mptcp_sock *msk);
> > > > +	int (*established)(struct mptcp_sock *msk);
> > > > +	int (*closed)(struct mptcp_sock *msk);
> > > > +	int (*address_announced)(struct mptcp_sock *msk,
> > > > +				 struct mptcp_pm_param
> > > > *param);
> > > > +	int (*address_removed)(struct mptcp_sock *msk,
> > > > +			       struct mptcp_pm_param *param);
> > > > +	int (*subflow_established)(struct mptcp_sock *msk,
> > > > +				   struct mptcp_pm_param
> > > > *param);
> > > > +	int (*subflow_closed)(struct mptcp_sock *msk,
> > > > +			      struct mptcp_pm_param *param);
> > > > +	int (*get_local_id)(struct mptcp_sock *msk,
> > > > +			    struct mptcp_pm_param *param);
> > > > +	bool (*get_priority)(struct mptcp_sock *msk,
> > > > +			     struct mptcp_pm_param *param);
> > > > +	int (*set_priority)(struct mptcp_sock *msk,
> > > > +			    struct mptcp_pm_param *param);
> > > > +	int (*listener_created)(struct mptcp_sock *msk);
> > > > +	int (*listener_closed)(struct mptcp_sock *msk);
> > > > +
> > > > +	u8			type;
> > > 
> > > I guess the type matches net.mptcp.pm_type sysctl knob, right?
> > > 
> > > I wonder if we should not deprecate this sysctl, and use a string
> > > like
> > > with the scheduler. So instead, we could have:
> > > 
> > >   char			name[MPTCP_PM_NAME_MAX];
> > > 
> > > And on ctrl.c, we could map pm_type for the moment with a custom
> > > proc_handler (and even remove this sysctl knob in a few
> > > releases):
> > > 
> > >  - 0  → in-kernel
> > >  - 1  → userspace
> > >  - >1 → bpf
> > 
> > Do we need to restrict BPF path manager to only extend the
> > userspace
> > type path manager? That is, for all BPF path managers,
> > mptcp_pm_is_userspace() returns true. If this is the case, there is
> > no
> > need to add "type" field in struct mptcp_pm_ops.
> 
> To me, mptcp_pm_is_userspace() should no longer be needed, because
> pm->ops will be used instead. So the "type" field should no longer be

Currently, the path manager must rely on mptcp_pm_is_userspace().
mptcp_pm_is_userspace() is not needed for get_local_id() and
get_priority(), but it's useful for address_announced(),
address_removed(), subflow_created() and subflow_closed(). 

Take address_announced() as an example:

For userspace pm, address_announced() is invoked in
mptcp_pm_nl_announce_doit(), which is the handler of the command
MPTCP_PM_CMD_ANNOUNCE [1].

While for in-kernel pm, address_announced() is invoked in
mptcp_pm_nl_add_addr_doit(), which is the handler of the command
MPTCP_PM_CMD_ADD_ADDR [2].

When we implement a BPF path manager, we must tell the PM core which
path to use to call its address_announced() interface. So I added
"type" in struct mptcp_pm_ops, and each path manager needs to set it to
tell the PM core whether it is a userspace type path manager, and check
this type in mptcp_pm_is_userspace().

[1]
https://patchwork.kernel.org/project/mptcp/patch/6d39ed9364b41f84b273598f198fa1aa226a2cbc.1740047738.git.tanggeliang@kylinos.cn/
[2]
https://patchwork.kernel.org/project/mptcp/patch/5881dc057b4927f30070193bde21703f0079e233.1740047738.git.tanggeliang@kylinos.cn/

> needed. Only the name should be stored, and the net.mptcp.pm_type
> sysctl
> knob should depend only on the name. (I guess a lock will be needed
> to
> handle the case where both the "pm_type" and "path_manager" will be
> set
> at the same time.)
> 
> > Is it necessary to allow BPF to create kernel type path managers,
> > that
> > is, path managers for which mptcp_pm_is_userspace() returns false?
> > Since we registered mptcp_netlink_pm from this version, can the in-
> > kernel type path manager also be extended by BPF? If this is the
> > case,
> > we need to add "type" field in struct mptcp_pm_ops to distinguish
> > whether it is a userspace type path manager.
> 
> I guess a BPF path-manager will have its own mptcp_pm_ops structure,
> and
> it will not extend anything. Of course, it is possible to add kfunc
> to
> share some code. No?
> 
> > > WDYT? Would it not be clearer for devs and users?
> > > 
> > > Note that if for the implementation, if it is easier to keep this
> > > "type"
> > > entry for the moment for the sysctl stuff, I'm fine with that.
> > > But if
> > > we
> > > don't need it, let's not introduce it.
> > > 
> > > > +	struct module		*owner;
> > > > +	struct list_head	list;
> > > > +
> > > > +	void (*init)(struct mptcp_sock *msk);
> > > > +	void (*release)(struct mptcp_sock *msk);
> > > 
> > > To answer my question from the v3 review: the init/release is
> > > done
> > > for
> > > each MPTCP connection handled by this PM.
> > 
> > In-kernel pm is for each MPTCP connection handled by this PM while
> > userspace pm is only for this one being initialized. How should I
> > deal
> > with this? Please give me more detailed suggestions.
> 
> Sorry, I'm not sure to understand your question. If one PM doesn't
> need
> to init and/or release anything, these ops can be set to NULL, no?
> 
> Cheers,
> Matt

Re: [PATCH mptcp-next v5 4/9] mptcp: pm: define struct mptcp_pm_ops

[Matthieu Baerts]

Hi Geliang,

On 24/02/2025 10:11, Geliang Tang wrote:
> Hi Matt,
> 
> On Mon, 2025-02-24 at 09:26 +0100, Matthieu Baerts wrote:
>> Hi Geliang,
>>
>> On 24/02/2025 07:54, Geliang Tang wrote:
>>> Hi Matt,
>>>
>>> Thanks for the review.
>>>
>>> On Fri, 2025-02-21 at 18:23 +0100, Matthieu Baerts wrote:
>>>> Hi Geliang,
>>>>
>>>> On 20/02/2025 03:57, 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.
>>>>>
>>>>> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
>>>>> ---
>>>>>  include/net/mptcp.h  | 29 ++++++++++++++++++++++
>>>>>  net/mptcp/pm.c       | 59
>>>>> ++++++++++++++++++++++++++++++++++++++++++++
>>>>>  net/mptcp/protocol.h |  5 ++++
>>>>>  3 files changed, 93 insertions(+)
>>>>>
>>>>> diff --git a/include/net/mptcp.h b/include/net/mptcp.h
>>>>> index a41d6c74760f..f51e75d3882d 100644
>>>>> --- a/include/net/mptcp.h
>>>>> +++ b/include/net/mptcp.h
>>>>> @@ -134,6 +134,35 @@ struct mptcp_pm_param {
>>>>>  	struct mptcp_addr_info		addr;
>>>>>  };
>>>>>  
>>>>> +struct mptcp_pm_ops {
>>>>> +	int (*created)(struct mptcp_sock *msk);
>>>>> +	int (*established)(struct mptcp_sock *msk);
>>>>> +	int (*closed)(struct mptcp_sock *msk);
>>>>> +	int (*address_announced)(struct mptcp_sock *msk,
>>>>> +				 struct mptcp_pm_param
>>>>> *param);
>>>>> +	int (*address_removed)(struct mptcp_sock *msk,
>>>>> +			       struct mptcp_pm_param *param);
>>>>> +	int (*subflow_established)(struct mptcp_sock *msk,
>>>>> +				   struct mptcp_pm_param
>>>>> *param);
>>>>> +	int (*subflow_closed)(struct mptcp_sock *msk,
>>>>> +			      struct mptcp_pm_param *param);
>>>>> +	int (*get_local_id)(struct mptcp_sock *msk,
>>>>> +			    struct mptcp_pm_param *param);
>>>>> +	bool (*get_priority)(struct mptcp_sock *msk,
>>>>> +			     struct mptcp_pm_param *param);
>>>>> +	int (*set_priority)(struct mptcp_sock *msk,
>>>>> +			    struct mptcp_pm_param *param);
>>>>> +	int (*listener_created)(struct mptcp_sock *msk);
>>>>> +	int (*listener_closed)(struct mptcp_sock *msk);
>>>>> +
>>>>> +	u8			type;
>>>>
>>>> I guess the type matches net.mptcp.pm_type sysctl knob, right?
>>>>
>>>> I wonder if we should not deprecate this sysctl, and use a string
>>>> like
>>>> with the scheduler. So instead, we could have:
>>>>
>>>>   char			name[MPTCP_PM_NAME_MAX];
>>>>
>>>> And on ctrl.c, we could map pm_type for the moment with a custom
>>>> proc_handler (and even remove this sysctl knob in a few
>>>> releases):
>>>>
>>>>  - 0  → in-kernel
>>>>  - 1  → userspace
>>>>  - >1 → bpf
>>>
>>> Do we need to restrict BPF path manager to only extend the
>>> userspace
>>> type path manager? That is, for all BPF path managers,
>>> mptcp_pm_is_userspace() returns true. If this is the case, there is
>>> no
>>> need to add "type" field in struct mptcp_pm_ops.
>>
>> To me, mptcp_pm_is_userspace() should no longer be needed, because
>> pm->ops will be used instead. So the "type" field should no longer be
> 
> Currently, the path manager must rely on mptcp_pm_is_userspace().
> mptcp_pm_is_userspace() is not needed for get_local_id() and
> get_priority(), but it's useful for address_announced(),
> address_removed(), subflow_created() and subflow_closed(). 

Yes but once all ops are implemented, it should be fine, no?

> Take address_announced() as an example:
> 
> For userspace pm, address_announced() is invoked in
> mptcp_pm_nl_announce_doit(), which is the handler of the command
> MPTCP_PM_CMD_ANNOUNCE [1].
> 
> While for in-kernel pm, address_announced() is invoked in
> mptcp_pm_nl_add_addr_doit(), which is the handler of the command
> MPTCP_PM_CMD_ADD_ADDR [2].
> 
> When we implement a BPF path manager, we must tell the PM core which
> path to use to call its address_announced() interface. So I added
> "type" in struct mptcp_pm_ops, and each path manager needs to set it to
> tell the PM core whether it is a userspace type path manager, and check
> this type in mptcp_pm_is_userspace().

I think there is a bit of confusion here: address_announced() will be
called when the other peer has announced an address. In other words,
when an ADD_ADDR is received, pm->ops->address_announced() will be
called from mptcp_pm_add_addr_received(), replacing:

  if (mptcp_pm_is_userspace(msk)) {
     (...)
  } else (...) {
     (...)
  }

address_announced() could then either:
- return a boolean: handled or drop → the PM will be in charged of
sending the ADD_ADDR echo.

- Or, if it is easier for the BPF PM, an enum could be returned, and in
pm.c, the kernel will either:
  - send add addr echo
  - drop
  - do nothing (the in-kernel PM would schedule the PM worker)

WDYT?

Similar for address_removed(), subflow_created() and subflow_closed().
If I'm not mistaken, the userspace PM does nothing here, because that's
the userspace daemon to react with the events that will be sent.

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

[PATCH mptcp-next v5 5/9] mptcp: pm: in-kernel: register mptcp_netlink_pm

[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_netlink_pm", and register it in
mptcp_pm_nl_init().

Only get_local_id() and get_priority() interfaces are implemented here.
mptcp_pm_nl_is_backup() becomes a wrapper of get_priority().

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

diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index 3709ad2c06ef..e0f8754e261e 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -1172,9 +1172,11 @@ 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_get_priority(struct mptcp_sock *msk,
+				     struct mptcp_pm_param *param)
 {
 	struct pm_nl_pernet *pernet = pm_nl_get_pernet_from_msk(msk);
+	struct mptcp_addr_info *skc = &param->addr;
 	struct mptcp_pm_addr_entry *entry;
 	bool backup;
 
@@ -1186,6 +1188,14 @@ bool mptcp_pm_nl_is_backup(struct mptcp_sock *msk, struct mptcp_addr_info *skc)
 	return backup;
 }
 
+bool mptcp_pm_nl_is_backup(struct mptcp_sock *msk, struct mptcp_addr_info *skc)
+{
+	struct mptcp_pm_param param;
+
+	mptcp_pm_param_set_contexts(&param, NULL, skc);
+	return mptcp_pm_nl_get_priority(msk, &param);
+}
+
 #define MPTCP_PM_CMD_GRP_OFFSET       0
 #define MPTCP_PM_EV_GRP_OFFSET        1
 
@@ -2373,6 +2383,13 @@ static struct pernet_operations mptcp_pm_pernet_ops = {
 	.size = sizeof(struct pm_nl_pernet),
 };
 
+static struct mptcp_pm_ops mptcp_netlink_pm = {
+	.get_local_id		= mptcp_pm_nl_get_local_id,
+	.get_priority		= mptcp_pm_nl_get_priority,
+	.type			= MPTCP_PM_TYPE_KERNEL,
+	.owner			= THIS_MODULE,
+};
+
 void __init mptcp_pm_nl_init(void)
 {
 	if (register_pernet_subsys(&mptcp_pm_pernet_ops) < 0)
@@ -2380,4 +2397,6 @@ void __init mptcp_pm_nl_init(void)
 
 	if (genl_register_family(&mptcp_genl_family))
 		panic("Failed to register MPTCP PM netlink family\n");
+
+	mptcp_pm_register(&mptcp_netlink_pm);
 }
-- 
2.43.0

[PATCH mptcp-next v5 6/9] mptcp: pm: userspace: register mptcp_userspace_pm

[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_data_init().

Only get_local_id() and get_priority() interfaces are implemented here.
mptcp_userspace_pm_is_backup() becomes a wrapper of get_priority().

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, 26 insertions(+), 2 deletions(-)

diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index f56b2d1e3409..91e377058243 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -665,6 +665,7 @@ void mptcp_pm_data_init(struct mptcp_sock *msk)
 void __init mptcp_pm_init(void)
 {
 	mptcp_pm_nl_init();
+	mptcp_userspace_pm_init();
 }
 
 /* Must be called with rcu read lock held */
diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index bd235b2d18b3..6e51335c8bc1 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -150,9 +150,10 @@ 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_get_priority(struct mptcp_sock *msk,
+					    struct mptcp_pm_param *param)
 {
+	struct mptcp_addr_info *skc = &param->addr;
 	struct mptcp_pm_addr_entry *entry;
 	bool backup;
 
@@ -164,6 +165,15 @@ bool mptcp_userspace_pm_is_backup(struct mptcp_sock *msk,
 	return backup;
 }
 
+bool mptcp_userspace_pm_is_backup(struct mptcp_sock *msk,
+				  struct mptcp_addr_info *skc)
+{
+	struct mptcp_pm_param param;
+
+	mptcp_pm_param_set_contexts(&param, NULL, skc);
+	return mptcp_userspace_pm_get_priority(msk, &param);
+}
+
 static struct mptcp_sock *mptcp_userspace_pm_get_sock(const struct genl_info *info)
 {
 	struct mptcp_sock *msk;
@@ -687,3 +697,15 @@ int mptcp_userspace_pm_get_addr(u8 id, struct mptcp_pm_addr_entry *addr,
 	sock_put(sk);
 	return ret;
 }
+
+static struct mptcp_pm_ops mptcp_userspace_pm = {
+	.get_local_id		= mptcp_userspace_pm_get_local_id,
+	.get_priority		= mptcp_userspace_pm_get_priority,
+	.type			= MPTCP_PM_TYPE_USERSPACE,
+	.owner			= THIS_MODULE,
+};
+
+void __init mptcp_userspace_pm_init(void)
+{
+	mptcp_pm_register(&mptcp_userspace_pm);
+}
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index f3e04927e214..021c0f87da1b 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -1152,6 +1152,7 @@ static inline u8 subflow_get_local_id(const struct mptcp_subflow_context *subflo
 }
 
 void __init mptcp_pm_nl_init(void);
+void __init mptcp_userspace_pm_init(void);
 void mptcp_pm_nl_work(struct mptcp_sock *msk);
 unsigned int mptcp_pm_get_add_addr_signal_max(const struct mptcp_sock *msk);
 unsigned int mptcp_pm_get_add_addr_accept_max(const struct mptcp_sock *msk);
-- 
2.43.0

[PATCH mptcp-next v5 7/9] 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_destroy_sock().

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

diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index 91e377058243..9d42aed440a0 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -627,6 +627,9 @@ void mptcp_pm_data_reset(struct mptcp_sock *msk)
 	pm->rm_list_tx.nr = 0;
 	pm->rm_list_rx.nr = 0;
 	WRITE_ONCE(pm->pm_type, pm_type);
+	rcu_read_lock();
+	mptcp_pm_initialize(msk, mptcp_pm_find(pm_type));
+	rcu_read_unlock();
 
 	if (pm_type == MPTCP_PM_TYPE_KERNEL) {
 		bool subflows_allowed = !!mptcp_pm_get_subflows_max(msk);
@@ -721,3 +724,33 @@ void mptcp_pm_unregister(struct mptcp_pm_ops *pm)
 	list_del_rcu(&pm->list);
 	spin_unlock(&mptcp_pm_list_lock);
 }
+
+int mptcp_pm_initialize(struct mptcp_sock *msk, struct mptcp_pm_ops *pm)
+{
+	if (!pm)
+		return -EINVAL;
+
+	if (!bpf_try_module_get(pm, pm->owner))
+		return -EBUSY;
+
+	msk->pm.ops = pm;
+	if (msk->pm.ops->init)
+		msk->pm.ops->init(msk);
+
+	pr_debug("userspace_pm type %u initialized\n", msk->pm.ops->type);
+	return 0;
+}
+
+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);
+}
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 2b48cf648346..de8eb3ec8cdd 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -2944,6 +2944,7 @@ static void __mptcp_destroy_sock(struct sock *sk)
 	sk_stop_timer(sk, &sk->sk_timer);
 	msk->pm.status = 0;
 	mptcp_release_sched(msk);
+	mptcp_pm_release(msk);
 
 	sk->sk_prot->destroy(sk);
 
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 021c0f87da1b..cddb919fc120 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 */
 
@@ -1043,6 +1044,8 @@ struct mptcp_pm_ops *mptcp_pm_find(enum mptcp_pm_type type);
 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);
+int mptcp_pm_initialize(struct mptcp_sock *msk, struct mptcp_pm_ops *pm);
+void mptcp_pm_release(struct mptcp_sock *msk);
 
 void mptcp_free_local_addr_list(struct mptcp_sock *msk);
 
-- 
2.43.0

[PATCH mptcp-next v5 8/9] mptcp: pm: drop get_local_id helpers

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

Now mptcp_pm_nl_get_local_id() and mptcp_userspace_pm_get_local_id()
helpers can be dropped, and mptcp_pm_get_local_id() can directly invoke
get_local_id() interface through "ops" of "msk->pm".

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

diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index 9d42aed440a0..d2cc93e21bee 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -438,10 +438,10 @@ 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 (!msk->pm.ops || !msk->pm.ops->get_local_id)
+		return -ENOTSUPP;
 	mptcp_pm_param_set_contexts(&param, &skc_local, NULL);
-	if (mptcp_pm_is_userspace(msk))
-		return mptcp_userspace_pm_get_local_id(msk, &param);
-	return mptcp_pm_nl_get_local_id(msk, &param);
+	return msk->pm.ops->get_local_id(msk, &param);
 }
 
 bool mptcp_pm_is_backup(struct mptcp_sock *msk, struct sock_common *skc)
diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index e0f8754e261e..33b19ff7a313 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -1139,8 +1139,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_param *param)
+static int mptcp_pm_nl_get_local_id(struct mptcp_sock *msk,
+				    struct mptcp_pm_param *param)
 {
 	struct mptcp_pm_addr_entry *skc = &param->entry;
 	struct mptcp_pm_addr_entry *entry;
diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index 6e51335c8bc1..b233d8469a48 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -130,8 +130,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_param *param)
+static int mptcp_userspace_pm_get_local_id(struct mptcp_sock *msk,
+					   struct mptcp_pm_param *param)
 {
 	__be16 msk_sport =  ((struct inet_sock *)
 			     inet_sk((struct sock *)msk))->inet_sport;
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index cddb919fc120..d06add105df5 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -1129,10 +1129,6 @@ void mptcp_pm_param_set_contexts(struct mptcp_pm_param *param,
 				 const struct mptcp_pm_addr_entry *entry,
 				 const struct mptcp_addr_info *addr);
 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_param *param);
-int mptcp_userspace_pm_get_local_id(struct mptcp_sock *msk,
-				    struct mptcp_pm_param *param);
 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

Re: [PATCH mptcp-next v5 8/9] mptcp: pm: drop get_local_id helpers

[Matthieu Baerts]

On 20/02/2025 03:57, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> Now mptcp_pm_nl_get_local_id() and mptcp_userspace_pm_get_local_id()
> helpers can be dropped, and mptcp_pm_get_local_id() can directly invoke
> get_local_id() interface through "ops" of "msk->pm".
> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  net/mptcp/pm.c           | 6 +++---
>  net/mptcp/pm_netlink.c   | 4 ++--
>  net/mptcp/pm_userspace.c | 4 ++--
>  net/mptcp/protocol.h     | 4 ----
>  4 files changed, 7 insertions(+), 11 deletions(-)
> 
> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index 9d42aed440a0..d2cc93e21bee 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c
> @@ -438,10 +438,10 @@ 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 (!msk->pm.ops || !msk->pm.ops->get_local_id)
Mmh, do we really need to check the "no pm ops" case? If we have to deal
with that, that might be annoying.

Should we not impose the implementation of get_local_id()?

> +		return -ENOTSUPP;
>  	mptcp_pm_param_set_contexts(&param, &skc_local, NULL);
> -	if (mptcp_pm_is_userspace(msk))
> -		return mptcp_userspace_pm_get_local_id(msk, &param);
> -	return mptcp_pm_nl_get_local_id(msk, &param);
> +	return msk->pm.ops->get_local_id(msk, &param);
>  }
>  
>  bool mptcp_pm_is_backup(struct mptcp_sock *msk, struct sock_common *skc)
> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index e0f8754e261e..33b19ff7a313 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c
> @@ -1139,8 +1139,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_param *param)
> +static int mptcp_pm_nl_get_local_id(struct mptcp_sock *msk,
> +				    struct mptcp_pm_param *param)
>  {
>  	struct mptcp_pm_addr_entry *skc = &param->entry;
>  	struct mptcp_pm_addr_entry *entry;
> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> index 6e51335c8bc1..b233d8469a48 100644
> --- a/net/mptcp/pm_userspace.c
> +++ b/net/mptcp/pm_userspace.c
> @@ -130,8 +130,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_param *param)
> +static int mptcp_userspace_pm_get_local_id(struct mptcp_sock *msk,
> +					   struct mptcp_pm_param *param)
>  {
>  	__be16 msk_sport =  ((struct inet_sock *)
>  			     inet_sk((struct sock *)msk))->inet_sport;
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index cddb919fc120..d06add105df5 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -1129,10 +1129,6 @@ void mptcp_pm_param_set_contexts(struct mptcp_pm_param *param,
>  				 const struct mptcp_pm_addr_entry *entry,
>  				 const struct mptcp_addr_info *addr);
>  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_param *param);
> -int mptcp_userspace_pm_get_local_id(struct mptcp_sock *msk,
> -				    struct mptcp_pm_param *param);
>  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);

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

[PATCH mptcp-next v5 9/9] mptcp: pm: drop is_backup helpers

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

Now mptcp_pm_nl_is_backup() and mptcp_userspace_pm_is_backup()
helpers can be dropped, and mptcp_pm_is_backup() can directly
invoke get_priority() interface through "ops" of "msk->pm".

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

diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index d2cc93e21bee..ea92e72c383f 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -447,13 +447,14 @@ 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)
 {
 	struct mptcp_addr_info skc_local;
+	struct mptcp_pm_param param;
 
 	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);
+	if (!msk->pm.ops || !msk->pm.ops->get_priority)
+		return -ENOTSUPP;
+	mptcp_pm_param_set_contexts(&param, NULL, &skc_local);
+	return msk->pm.ops->get_priority(msk, &param);
 }
 
 static int mptcp_pm_get_addr(u8 id, struct mptcp_pm_addr_entry *addr,
diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index 33b19ff7a313..3859642792db 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -1188,14 +1188,6 @@ static bool mptcp_pm_nl_get_priority(struct mptcp_sock *msk,
 	return backup;
 }
 
-bool mptcp_pm_nl_is_backup(struct mptcp_sock *msk, struct mptcp_addr_info *skc)
-{
-	struct mptcp_pm_param param;
-
-	mptcp_pm_param_set_contexts(&param, NULL, skc);
-	return mptcp_pm_nl_get_priority(msk, &param);
-}
-
 #define MPTCP_PM_CMD_GRP_OFFSET       0
 #define MPTCP_PM_EV_GRP_OFFSET        1
 
diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index b233d8469a48..be578f5c3195 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -165,15 +165,6 @@ static bool mptcp_userspace_pm_get_priority(struct mptcp_sock *msk,
 	return backup;
 }
 
-bool mptcp_userspace_pm_is_backup(struct mptcp_sock *msk,
-				  struct mptcp_addr_info *skc)
-{
-	struct mptcp_pm_param param;
-
-	mptcp_pm_param_set_contexts(&param, NULL, skc);
-	return mptcp_userspace_pm_get_priority(msk, &param);
-}
-
 static struct mptcp_sock *mptcp_userspace_pm_get_sock(const struct genl_info *info)
 {
 	struct mptcp_sock *msk;
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index d06add105df5..45c948142839 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -1130,8 +1130,6 @@ void mptcp_pm_param_set_contexts(struct mptcp_pm_param *param,
 				 const struct mptcp_addr_info *addr);
 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

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

[MPTCP CI]

Hi Geliang,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal: Success! ✅
- KVM Validation: debug: Success! ✅
- KVM Validation: btf-normal (only bpftest_all): Success! ✅
- KVM Validation: btf-debug (only bpftest_all): Unstable: 1 failed test(s): bpftest_test_progs-no_alu32_mptcp 🔴
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/13427116230

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


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

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

For more details:

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


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

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

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

[Matthieu Baerts]

Hi Geliang,

On 20/02/2025 03:57, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> 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

Thank you for the new PM ops, and the different modifications, that
looks clearer!

I have some comments, mainly about the new "struct mptcp_pm_param
*param" that seems causing more issues (noises) than solving some I think.

Please see my individual comments.

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