diff mbox series

[mptcp-next,v1,4/6] mptcp: pm: add add_addr_echo() interface

Message ID a550107394e989fb5557ce16fbb54050f4a20551.1741685260.git.tanggeliang@kylinos.cn (mailing list archive)
State Superseded
Delegated to: Matthieu Baerts
Headers show
Series BPF path manager, part 6 | expand

Checks

Context Check Description
matttbe/checkpatch warning total: 0 errors, 2 warnings, 0 checks, 113 lines checked
matttbe/shellcheck success MPTCP selftests files have not been modified
matttbe/build success Build and static analysis OK
matttbe/KVM_Validation__normal success Success! ✅
matttbe/KVM_Validation__debug success Success! ✅
matttbe/KVM_Validation__btf-normal__only_bpftest_all_ success Success! ✅
matttbe/KVM_Validation__btf-debug__only_bpftest_all_ success Success! ✅

Commit Message

Geliang Tang March 11, 2025, 9:32 a.m. UTC 
From: Geliang Tang <tanggeliang@kylinos.cn>

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

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

Comments

Matthieu Baerts (NGI0) March 11, 2025, 11:31 p.m. UTC | #1 
Hi Geliang,

On 11/03/2025 10:32, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> The helper mptcp_pm_is_userspace() is used to distinguish userspace PM
> operations from in-kernel PM in mptcp_pm_add_addr_received(). It seems
> reasonable to add a mandatory .add_addr_echo interface for struct
> mptcp_pm_ops.
> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  include/net/mptcp.h      |  2 ++
>  net/mptcp/pm.c           | 36 ++++++++++++++++++------------------
>  net/mptcp/pm_kernel.c    |  7 +++++++
>  net/mptcp/pm_userspace.c | 12 ++++++++++++
>  net/mptcp/protocol.h     |  2 ++
>  5 files changed, 41 insertions(+), 18 deletions(-)
> 
> diff --git a/include/net/mptcp.h b/include/net/mptcp.h
> index 5f2d94b2f97f..1f4d0d3988c1 100644
> --- a/include/net/mptcp.h
> +++ b/include/net/mptcp.h
> @@ -128,6 +128,8 @@ struct mptcp_pm_ops {
>  	bool (*accept_new_subflow)(const struct mptcp_sock *msk);
>  	void (*subflow_check_next)(struct mptcp_sock *msk,
>  				   const struct mptcp_subflow_context *subflow);
> +	void (*add_addr_echo)(struct mptcp_sock *msk,

add_addr_received instead?

> +			      const struct mptcp_addr_info *addr);
>  
>  	char			name[MPTCP_PM_NAME_MAX];
>  	struct module		*owner;
> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index d8a5c2c06500..dc7ab696c24b 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c
> @@ -529,6 +529,22 @@ void mptcp_pm_subflow_check_next(struct mptcp_sock *msk,
>  	msk->pm.ops->subflow_check_next(msk, subflow);
>  }
>  
> +void mptcp_pm_add_addr_echo(struct mptcp_sock *msk,
> +			    const struct mptcp_addr_info *addr)

Why is this here if it is only used by the kernel PM?

> +{
> +	struct mptcp_pm_data *pm = &msk->pm;
> +
> +	if ((addr->id == 0 && !mptcp_pm_is_init_remote_addr(msk, addr)) ||
> +	    (addr->id > 0 && !READ_ONCE(pm->accept_addr))) {
> +		mptcp_pm_announce_addr(msk, addr, true);
> +		mptcp_pm_add_addr_send_ack(msk);
> +	} else if (mptcp_pm_schedule_work(msk, MPTCP_PM_ADD_ADDR_RECEIVED)) {
> +		pm->remote = *addr;
> +	} else {
> +		__MPTCP_INC_STATS(sock_net((struct sock *)msk), MPTCP_MIB_ADDADDRDROP);
> +	}
> +}
> +
>  void mptcp_pm_add_addr_received(const struct sock *ssk,
>  				const struct mptcp_addr_info *addr)
>  {
> @@ -543,23 +559,7 @@ void mptcp_pm_add_addr_received(const struct sock *ssk,
>  
>  	spin_lock_bh(&pm->lock);
>  
> -	if (mptcp_pm_is_userspace(msk)) {
> -		if (mptcp_userspace_pm_active(msk)) {
> -			mptcp_pm_announce_addr(msk, addr, true);
> -			mptcp_pm_add_addr_send_ack(msk);
> -		} else {
> -			__MPTCP_INC_STATS(sock_net((struct sock *)msk), MPTCP_MIB_ADDADDRDROP);
> -		}
> -	/* id0 should not have a different address */
> -	} else if ((addr->id == 0 && !mptcp_pm_is_init_remote_addr(msk, addr)) ||
> -		   (addr->id > 0 && !READ_ONCE(pm->accept_addr))) {
> -		mptcp_pm_announce_addr(msk, addr, true);
> -		mptcp_pm_add_addr_send_ack(msk);
> -	} else if (mptcp_pm_schedule_work(msk, MPTCP_PM_ADD_ADDR_RECEIVED)) {

Here as well, I think the worker scheduling should be done from pm.c if
an optional callback is set, and this callback should be called from the
worker.

Maybe we need 2 callbacks here? One to know if there is a need to be
called from a worker, then add_addr_received? Or: can we not always send
the ADD_ADDR echo here, and schedule the worker if there is a need to?
So we would drop the "if (mptcp_userspace_pm_active(msk))" condition
above, but maybe that's OK? (We would need a dedicate patch for that)

> -		pm->remote = *addr;
> -	} else {
> -		__MPTCP_INC_STATS(sock_net((struct sock *)msk), MPTCP_MIB_ADDADDRDROP);

I think this MIB counter should be incremented from here, not from each PM.

Cheers,
Matt
diff mbox series

Patch

diff --git a/include/net/mptcp.h b/include/net/mptcp.h
index 5f2d94b2f97f..1f4d0d3988c1 100644
--- a/include/net/mptcp.h
+++ b/include/net/mptcp.h
@@ -128,6 +128,8 @@  struct mptcp_pm_ops {
 	bool (*accept_new_subflow)(const struct mptcp_sock *msk);
 	void (*subflow_check_next)(struct mptcp_sock *msk,
 				   const struct mptcp_subflow_context *subflow);
+	void (*add_addr_echo)(struct mptcp_sock *msk,
+			      const struct mptcp_addr_info *addr);
 
 	char			name[MPTCP_PM_NAME_MAX];
 	struct module		*owner;
diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index d8a5c2c06500..dc7ab696c24b 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -529,6 +529,22 @@  void mptcp_pm_subflow_check_next(struct mptcp_sock *msk,
 	msk->pm.ops->subflow_check_next(msk, subflow);
 }
 
+void mptcp_pm_add_addr_echo(struct mptcp_sock *msk,
+			    const struct mptcp_addr_info *addr)
+{
+	struct mptcp_pm_data *pm = &msk->pm;
+
+	if ((addr->id == 0 && !mptcp_pm_is_init_remote_addr(msk, addr)) ||
+	    (addr->id > 0 && !READ_ONCE(pm->accept_addr))) {
+		mptcp_pm_announce_addr(msk, addr, true);
+		mptcp_pm_add_addr_send_ack(msk);
+	} else if (mptcp_pm_schedule_work(msk, MPTCP_PM_ADD_ADDR_RECEIVED)) {
+		pm->remote = *addr;
+	} else {
+		__MPTCP_INC_STATS(sock_net((struct sock *)msk), MPTCP_MIB_ADDADDRDROP);
+	}
+}
+
 void mptcp_pm_add_addr_received(const struct sock *ssk,
 				const struct mptcp_addr_info *addr)
 {
@@ -543,23 +559,7 @@  void mptcp_pm_add_addr_received(const struct sock *ssk,
 
 	spin_lock_bh(&pm->lock);
 
-	if (mptcp_pm_is_userspace(msk)) {
-		if (mptcp_userspace_pm_active(msk)) {
-			mptcp_pm_announce_addr(msk, addr, true);
-			mptcp_pm_add_addr_send_ack(msk);
-		} else {
-			__MPTCP_INC_STATS(sock_net((struct sock *)msk), MPTCP_MIB_ADDADDRDROP);
-		}
-	/* id0 should not have a different address */
-	} else if ((addr->id == 0 && !mptcp_pm_is_init_remote_addr(msk, addr)) ||
-		   (addr->id > 0 && !READ_ONCE(pm->accept_addr))) {
-		mptcp_pm_announce_addr(msk, addr, true);
-		mptcp_pm_add_addr_send_ack(msk);
-	} else if (mptcp_pm_schedule_work(msk, MPTCP_PM_ADD_ADDR_RECEIVED)) {
-		pm->remote = *addr;
-	} else {
-		__MPTCP_INC_STATS(sock_net((struct sock *)msk), MPTCP_MIB_ADDADDRDROP);
-	}
+	pm->ops->add_addr_echo(msk, addr);
 
 	spin_unlock_bh(&pm->lock);
 }
@@ -992,7 +992,7 @@  int mptcp_pm_validate(struct mptcp_pm_ops *pm_ops)
 {
 	if (!pm_ops->get_local_id || !pm_ops->get_priority ||
 	    !pm_ops->allow_new_subflow || !pm_ops->accept_new_subflow ||
-	    !pm_ops->subflow_check_next) {
+	    !pm_ops->subflow_check_next || !pm_ops->add_addr_echo) {
 		pr_err("%s does not implement required ops\n", pm_ops->name);
 		return -EINVAL;
 	}
diff --git a/net/mptcp/pm_kernel.c b/net/mptcp/pm_kernel.c
index b40d39232f70..d5002236fe6e 100644
--- a/net/mptcp/pm_kernel.c
+++ b/net/mptcp/pm_kernel.c
@@ -1449,6 +1449,12 @@  static void mptcp_pm_kernel_subflow_check_next(struct mptcp_sock *msk,
 	mptcp_pm_subflow_established(msk);
 }
 
+static void mptcp_pm_kernel_add_addr_echo(struct mptcp_sock *msk,
+					  const struct mptcp_addr_info *addr)
+{
+	mptcp_pm_add_addr_echo(msk, addr);
+}
+
 static void mptcp_pm_kernel_init(struct mptcp_sock *msk)
 {
 	bool subflows_allowed = !!mptcp_pm_get_subflows_max(msk);
@@ -1475,6 +1481,7 @@  struct mptcp_pm_ops mptcp_pm_kernel = {
 	.allow_new_subflow	= mptcp_pm_kernel_allow_new_subflow,
 	.accept_new_subflow	= mptcp_pm_kernel_accept_new_subflow,
 	.subflow_check_next	= mptcp_pm_kernel_subflow_check_next,
+	.add_addr_echo		= mptcp_pm_kernel_add_addr_echo,
 	.init			= mptcp_pm_kernel_init,
 	.name			= "kernel",
 	.owner			= THIS_MODULE,
diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index 68247ec4cdaa..848102bd0f4b 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -715,6 +715,17 @@  static void mptcp_pm_userspace_subflow_check_next(struct mptcp_sock *msk,
 	}
 }
 
+static void mptcp_pm_userspace_add_addr_echo(struct mptcp_sock *msk,
+					     const struct mptcp_addr_info *addr)
+{
+	if (mptcp_userspace_pm_active(msk)) {
+		mptcp_pm_announce_addr(msk, addr, true);
+		mptcp_pm_add_addr_send_ack(msk);
+	} else {
+		__MPTCP_INC_STATS(sock_net((struct sock *)msk), MPTCP_MIB_ADDADDRDROP);
+	}
+}
+
 static void mptcp_pm_userspace_release(struct mptcp_sock *msk)
 {
 	mptcp_userspace_pm_free_local_addr_list(msk);
@@ -726,6 +737,7 @@  static struct mptcp_pm_ops mptcp_pm_userspace = {
 	.allow_new_subflow	= mptcp_pm_userspace_allow_new_subflow,
 	.accept_new_subflow	= mptcp_pm_userspace_accept_new_subflow,
 	.subflow_check_next	= mptcp_pm_userspace_subflow_check_next,
+	.add_addr_echo		= mptcp_pm_userspace_add_addr_echo,
 	.release		= mptcp_pm_userspace_release,
 	.name			= "userspace",
 	.owner			= THIS_MODULE,
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index d9ca3a19a218..8ca8eea1795b 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -1013,6 +1013,8 @@  void mptcp_pm_subflow_established(struct mptcp_sock *msk);
 bool mptcp_pm_nl_check_work_pending(struct mptcp_sock *msk);
 void mptcp_pm_subflow_check_next(struct mptcp_sock *msk,
 				 const struct mptcp_subflow_context *subflow);
+void mptcp_pm_add_addr_echo(struct mptcp_sock *msk,
+			    const struct mptcp_addr_info *addr);
 void mptcp_pm_add_addr_received(const struct sock *ssk,
 				const struct mptcp_addr_info *addr);
 void mptcp_pm_add_addr_echoed(struct mptcp_sock *msk,