diff mbox series

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

Message ID 0fd4e42c04c60d63c11463c792084b751c4e2e48.1741088339.git.tanggeliang@kylinos.cn (mailing list archive)
State Superseded, archived
Delegated to: Matthieu Baerts
Headers show
Series BPF path manager, part 5 | expand

Commit Message

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

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

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

Comments

Matthieu Baerts March 5, 2025, 11:57 a.m. UTC | #1 
Hi Geliang,

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

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

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

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

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

Same here: pm_ops?

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

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

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

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

(...)

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


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

> -	}
> +	rcu_read_lock();
> +	mptcp_pm_initialize(msk, mptcp_pm_find(path_manager));
> +	rcu_read_unlock();
>  
>  	WRITE_ONCE(pm->addr_signal, 0);
>  	WRITE_ONCE(pm->remote_deny_join_id0, false);
Cheers,
Matt
diff mbox series

Patch

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