diff mbox series

[mptcp-next,v1,3/6] mptcp: pm: add subflow_check_next() interface

Message ID 2e7dd2f26852e4d5b5cd0434e13069b9461a3f32.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, 117 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_subflow_check_next(). It seems
reasonable to add a mandatory .subflow_check_next interface for struct
mptcp_pm_ops.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 include/net/mptcp.h      |  3 +++
 net/mptcp/pm.c           | 31 +++----------------------------
 net/mptcp/pm_kernel.c    | 20 ++++++++++++++++++++
 net/mptcp/pm_userspace.c | 15 +++++++++++++++
 4 files changed, 41 insertions(+), 28 deletions(-)

Comments

Matthieu Baerts (NGI0) March 11, 2025, 11:14 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_subflow_check_next(). It seems
> reasonable to add a mandatory .subflow_check_next interface for struct
> mptcp_pm_ops.
> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  include/net/mptcp.h      |  3 +++
>  net/mptcp/pm.c           | 31 +++----------------------------
>  net/mptcp/pm_kernel.c    | 20 ++++++++++++++++++++
>  net/mptcp/pm_userspace.c | 15 +++++++++++++++
>  4 files changed, 41 insertions(+), 28 deletions(-)
> 
> diff --git a/include/net/mptcp.h b/include/net/mptcp.h
> index 6f9bc0ca4d37..5f2d94b2f97f 100644
> --- a/include/net/mptcp.h
> +++ b/include/net/mptcp.h
> @@ -15,6 +15,7 @@
>  struct mptcp_info;
>  struct mptcp_sock;
>  struct mptcp_pm_addr_entry;
> +struct mptcp_subflow_context;
>  struct seq_file;
>  
>  /* MPTCP sk_buff extension data */
> @@ -125,6 +126,8 @@ struct mptcp_pm_ops {
>  
>  	bool (*allow_new_subflow)(struct mptcp_sock *msk);
>  	bool (*accept_new_subflow)(const struct mptcp_sock *msk);
> +	void (*subflow_check_next)(struct mptcp_sock *msk,

Maybe we should rename this: subflow_established?

'check_next' is not really explicit.

> +				   const struct mptcp_subflow_context *subflow);
>  
>  	char			name[MPTCP_PM_NAME_MAX];
>  	struct module		*owner;
> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index 31a2c66a1af3..d8a5c2c06500 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c
> @@ -526,33 +526,7 @@ void mptcp_pm_subflow_established(struct mptcp_sock *msk)
>  void mptcp_pm_subflow_check_next(struct mptcp_sock *msk,
>  				 const struct mptcp_subflow_context *subflow)
>  {
> -	struct mptcp_pm_data *pm = &msk->pm;
> -	bool update_subflows;
> -
> -	update_subflows = subflow->request_join || subflow->mp_join;
> -	if (mptcp_pm_is_userspace(msk)) {
> -		if (update_subflows) {
> -			spin_lock_bh(&pm->lock);
> -			pm->subflows--;
> -			spin_unlock_bh(&pm->lock);
> -		}

I wonder if... this 'if' should not be done for all PMs here. I guess
mptcp_pm_close_subflow(msk) could then be called.

Then I suggest scheduling the worker if msk->pm.ops->subflow_established
is set, and ... I'm not sure how to deal with work_pending. Maybe a new
callback? Or maybe we can deal with that later, but the current name is
not good :)

Anyway: then msk->pm.ops->subflow_check_next(), when the msk lock is
held. I think all these ops should only be called when the msk lock is
held, no? I don't think a BPF PM should schedule the worker.

> -		return;
> -	}
> -
> -	if (!READ_ONCE(pm->work_pending) && !update_subflows)
> -		return;
> -
> -	spin_lock_bh(&pm->lock);
> -	if (update_subflows)
> -		__mptcp_pm_close_subflow(msk);
> -
> -	/* Even if this subflow is not really established, tell the PM to try
> -	 * to pick the next ones, if possible.
> -	 */
> -	if (mptcp_pm_nl_check_work_pending(msk))
> -		mptcp_pm_schedule_work(msk, MPTCP_PM_SUBFLOW_ESTABLISHED);
> -
> -	spin_unlock_bh(&pm->lock);
> +	msk->pm.ops->subflow_check_next(msk, subflow);
>  }
>  
>  void mptcp_pm_add_addr_received(const struct sock *ssk,
> @@ -1017,7 +991,8 @@ struct mptcp_pm_ops *mptcp_pm_find(const char *name)
>  int mptcp_pm_validate(struct mptcp_pm_ops *pm_ops)
>  {
>  	if (!pm_ops->get_local_id || !pm_ops->get_priority ||
> -	    !pm_ops->allow_new_subflow || !pm_ops->accept_new_subflow) {
> +	    !pm_ops->allow_new_subflow || !pm_ops->accept_new_subflow ||
> +	    !pm_ops->subflow_check_next) {

See above: this new callback would not be mandatory then.

Cheers,
Matt
diff mbox series

Patch

diff --git a/include/net/mptcp.h b/include/net/mptcp.h
index 6f9bc0ca4d37..5f2d94b2f97f 100644
--- a/include/net/mptcp.h
+++ b/include/net/mptcp.h
@@ -15,6 +15,7 @@ 
 struct mptcp_info;
 struct mptcp_sock;
 struct mptcp_pm_addr_entry;
+struct mptcp_subflow_context;
 struct seq_file;
 
 /* MPTCP sk_buff extension data */
@@ -125,6 +126,8 @@  struct mptcp_pm_ops {
 
 	bool (*allow_new_subflow)(struct mptcp_sock *msk);
 	bool (*accept_new_subflow)(const struct mptcp_sock *msk);
+	void (*subflow_check_next)(struct mptcp_sock *msk,
+				   const struct mptcp_subflow_context *subflow);
 
 	char			name[MPTCP_PM_NAME_MAX];
 	struct module		*owner;
diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index 31a2c66a1af3..d8a5c2c06500 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -526,33 +526,7 @@  void mptcp_pm_subflow_established(struct mptcp_sock *msk)
 void mptcp_pm_subflow_check_next(struct mptcp_sock *msk,
 				 const struct mptcp_subflow_context *subflow)
 {
-	struct mptcp_pm_data *pm = &msk->pm;
-	bool update_subflows;
-
-	update_subflows = subflow->request_join || subflow->mp_join;
-	if (mptcp_pm_is_userspace(msk)) {
-		if (update_subflows) {
-			spin_lock_bh(&pm->lock);
-			pm->subflows--;
-			spin_unlock_bh(&pm->lock);
-		}
-		return;
-	}
-
-	if (!READ_ONCE(pm->work_pending) && !update_subflows)
-		return;
-
-	spin_lock_bh(&pm->lock);
-	if (update_subflows)
-		__mptcp_pm_close_subflow(msk);
-
-	/* Even if this subflow is not really established, tell the PM to try
-	 * to pick the next ones, if possible.
-	 */
-	if (mptcp_pm_nl_check_work_pending(msk))
-		mptcp_pm_schedule_work(msk, MPTCP_PM_SUBFLOW_ESTABLISHED);
-
-	spin_unlock_bh(&pm->lock);
+	msk->pm.ops->subflow_check_next(msk, subflow);
 }
 
 void mptcp_pm_add_addr_received(const struct sock *ssk,
@@ -1017,7 +991,8 @@  struct mptcp_pm_ops *mptcp_pm_find(const char *name)
 int mptcp_pm_validate(struct mptcp_pm_ops *pm_ops)
 {
 	if (!pm_ops->get_local_id || !pm_ops->get_priority ||
-	    !pm_ops->allow_new_subflow || !pm_ops->accept_new_subflow) {
+	    !pm_ops->allow_new_subflow || !pm_ops->accept_new_subflow ||
+	    !pm_ops->subflow_check_next) {
 		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 708a96951cba..b40d39232f70 100644
--- a/net/mptcp/pm_kernel.c
+++ b/net/mptcp/pm_kernel.c
@@ -1430,6 +1430,25 @@  static bool mptcp_pm_kernel_accept_new_subflow(const struct mptcp_sock *msk)
 	return READ_ONCE(msk->pm.accept_subflow);
 }
 
+static void mptcp_pm_kernel_subflow_check_next(struct mptcp_sock *msk,
+					       const struct mptcp_subflow_context *subflow)
+{
+	struct mptcp_pm_data *pm = &msk->pm;
+	bool update_subflows;
+
+	update_subflows = subflow->request_join || subflow->mp_join;
+
+	if (!READ_ONCE(pm->work_pending) && !update_subflows)
+		return;
+
+	spin_lock_bh(&pm->lock);
+	if (update_subflows)
+		__mptcp_pm_close_subflow(msk);
+	spin_unlock_bh(&pm->lock);
+
+	mptcp_pm_subflow_established(msk);
+}
+
 static void mptcp_pm_kernel_init(struct mptcp_sock *msk)
 {
 	bool subflows_allowed = !!mptcp_pm_get_subflows_max(msk);
@@ -1455,6 +1474,7 @@  struct mptcp_pm_ops mptcp_pm_kernel = {
 	.get_priority		= mptcp_pm_kernel_get_priority,
 	.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,
 	.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 4cd9a84477c8..68247ec4cdaa 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -701,6 +701,20 @@  static bool mptcp_pm_userspace_accept_new_subflow(const struct mptcp_sock *msk)
 	return mptcp_userspace_pm_active(msk);
 }
 
+static void mptcp_pm_userspace_subflow_check_next(struct mptcp_sock *msk,
+						  const struct mptcp_subflow_context *subflow)
+{
+	struct mptcp_pm_data *pm = &msk->pm;
+	bool update_subflows;
+
+	update_subflows = subflow->request_join || subflow->mp_join;
+	if (update_subflows) {
+		spin_lock_bh(&pm->lock);
+		pm->subflows--;
+		spin_unlock_bh(&pm->lock);
+	}
+}
+
 static void mptcp_pm_userspace_release(struct mptcp_sock *msk)
 {
 	mptcp_userspace_pm_free_local_addr_list(msk);
@@ -711,6 +725,7 @@  static struct mptcp_pm_ops mptcp_pm_userspace = {
 	.get_priority		= mptcp_pm_userspace_get_priority,
 	.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,
 	.release		= mptcp_pm_userspace_release,
 	.name			= "userspace",
 	.owner			= THIS_MODULE,