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_can_accept_new_subflow(). It seems
reasonable to add a mandatory .accept_new_subflow interface for struct
mptcp_pm_ops.
Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
include/net/mptcp.h | 1 +
net/mptcp/pm.c | 31 +++++++++++--------------------
net/mptcp/pm_kernel.c | 6 ++++++
net/mptcp/pm_userspace.c | 6 ++++++
net/mptcp/subflow.c | 4 +---
5 files changed, 25 insertions(+), 23 deletions(-)
diff --git a/include/net/mptcp.h b/include/net/mptcp.h
index 18d3679a752c..8c1ac7368693 100644
--- a/include/net/mptcp.h
+++ b/include/net/mptcp.h
@@ -133,6 +133,7 @@ struct mptcp_pm_ops {
/* required */
bool (*add_addr_echo)(struct mptcp_sock *msk,
const struct mptcp_addr_info *addr);
+ bool (*accept_new_subflow)(const struct mptcp_sock *msk);
char name[MPTCP_PM_NAME_MAX];
struct module *owner;
diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index d37f89bf0180..ca105bbd03ea 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -454,33 +454,24 @@ bool mptcp_pm_allow_new_subflow(struct mptcp_sock *msk)
{
struct mptcp_pm_data *pm = &msk->pm;
unsigned int subflows_max;
- int ret = 0;
+ bool ret = true;
- if (mptcp_pm_is_userspace(msk)) {
- if (mptcp_userspace_pm_active(msk)) {
- spin_lock_bh(&pm->lock);
- pm->subflows++;
- spin_unlock_bh(&pm->lock);
- return true;
- }
+ if (!pm->ops->accept_new_subflow(msk))
return false;
- }
-
- subflows_max = mptcp_pm_get_subflows_max(msk);
- pr_debug("msk=%p subflows=%d max=%d allow=%d\n", msk, pm->subflows,
- subflows_max, READ_ONCE(pm->accept_subflow));
+ spin_lock_bh(&pm->lock);
+ if (!mptcp_pm_is_userspace(msk) && READ_ONCE(pm->accept_subflow)) {
+ subflows_max = mptcp_pm_get_subflows_max(msk);
- /* try to avoid acquiring the lock below */
- if (!READ_ONCE(pm->accept_subflow))
- return false;
+ pr_debug("msk=%p subflows=%d max=%d allow=%d\n", msk, pm->subflows,
+ subflows_max, READ_ONCE(pm->accept_subflow));
- spin_lock_bh(&pm->lock);
- if (READ_ONCE(pm->accept_subflow)) {
ret = pm->subflows < subflows_max;
- if (ret && ++pm->subflows == subflows_max)
+ if (ret && pm->subflows == subflows_max - 1)
WRITE_ONCE(pm->accept_subflow, false);
}
+ if (ret)
+ pm->subflows++;
spin_unlock_bh(&pm->lock);
return ret;
@@ -1057,7 +1048,7 @@ 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->add_addr_echo) {
+ !pm_ops->add_addr_echo || !pm_ops->accept_new_subflow) {
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 9d159196afe5..7ec8fafeda0e 100644
--- a/net/mptcp/pm_kernel.c
+++ b/net/mptcp/pm_kernel.c
@@ -1404,6 +1404,11 @@ static bool mptcp_pm_kernel_add_addr_echo(struct mptcp_sock *msk,
(addr->id > 0 && !READ_ONCE(msk->pm.accept_addr));
}
+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_init(struct mptcp_sock *msk)
{
bool subflows_allowed = !!mptcp_pm_get_subflows_max(msk);
@@ -1432,6 +1437,7 @@ struct mptcp_pm_ops mptcp_pm_kernel = {
.add_addr_received = mptcp_pm_kernel_add_addr_received,
.rm_addr_received = mptcp_pm_kernel_rm_addr_received,
.add_addr_echo = mptcp_pm_kernel_add_addr_echo,
+ .accept_new_subflow = mptcp_pm_kernel_accept_new_subflow,
.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 3f7778ab064b..d6301d809376 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -689,6 +689,11 @@ static bool mptcp_pm_userspace_add_addr_echo(struct mptcp_sock *msk,
return mptcp_userspace_pm_active(msk);
}
+static bool mptcp_pm_userspace_accept_new_subflow(const struct mptcp_sock *msk)
+{
+ return mptcp_userspace_pm_active(msk);
+}
+
static void mptcp_pm_userspace_release(struct mptcp_sock *msk)
{
mptcp_userspace_pm_free_local_addr_list(msk);
@@ -698,6 +703,7 @@ static struct mptcp_pm_ops mptcp_pm_userspace = {
.get_local_id = mptcp_pm_userspace_get_local_id,
.get_priority = mptcp_pm_userspace_get_priority,
.add_addr_echo = mptcp_pm_userspace_add_addr_echo,
+ .accept_new_subflow = mptcp_pm_userspace_accept_new_subflow,
.release = mptcp_pm_userspace_release,
.name = "userspace",
.owner = THIS_MODULE,
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 409bd415ef1d..be79940da424 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -61,9 +61,7 @@ static void subflow_generate_hmac(u64 key1, u64 key2, u32 nonce1, u32 nonce2,
static bool mptcp_can_accept_new_subflow(const struct mptcp_sock *msk)
{
return mptcp_is_fully_established((void *)msk) &&
- ((mptcp_pm_is_userspace(msk) &&
- mptcp_userspace_pm_active(msk)) ||
- READ_ONCE(msk->pm.accept_subflow));
+ msk->pm.ops->accept_new_subflow(msk);
}
/* validate received token and create truncated hmac and nonce for SYN-ACK */
--
2.43.0On 24/03/2025 09:19, 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_can_accept_new_subflow(). It seems
> reasonable to add a mandatory .accept_new_subflow interface for struct
> mptcp_pm_ops.
>
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
> include/net/mptcp.h | 1 +
> net/mptcp/pm.c | 31 +++++++++++--------------------
> net/mptcp/pm_kernel.c | 6 ++++++
> net/mptcp/pm_userspace.c | 6 ++++++
> net/mptcp/subflow.c | 4 +---
> 5 files changed, 25 insertions(+), 23 deletions(-)
>
> diff --git a/include/net/mptcp.h b/include/net/mptcp.h
> index 18d3679a752c..8c1ac7368693 100644
> --- a/include/net/mptcp.h
> +++ b/include/net/mptcp.h
> @@ -133,6 +133,7 @@ struct mptcp_pm_ops {
> /* required */
> bool (*add_addr_echo)(struct mptcp_sock *msk,
> const struct mptcp_addr_info *addr);
> + bool (*accept_new_subflow)(const struct mptcp_sock *msk);
Similar to get_local_id() and get_priority(), I guess this callback will
be triggered from the subflow context, and not the msk context, right?
Detail: probably we should gather them together in this structure, with
an additional comment clearly mentioning in which context the callbacks
will be called.
> char name[MPTCP_PM_NAME_MAX];
> struct module *owner;
> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index d37f89bf0180..ca105bbd03ea 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c
> @@ -454,33 +454,24 @@ bool mptcp_pm_allow_new_subflow(struct mptcp_sock *msk)
> {
> struct mptcp_pm_data *pm = &msk->pm;
> unsigned int subflows_max;
> - int ret = 0;
> + bool ret = true;
>
> - if (mptcp_pm_is_userspace(msk)) {
> - if (mptcp_userspace_pm_active(msk)) {
> - spin_lock_bh(&pm->lock);
> - pm->subflows++;
> - spin_unlock_bh(&pm->lock);
> - return true;
> - }
> + if (!pm->ops->accept_new_subflow(msk))
> return false;
> - }
> -
> - subflows_max = mptcp_pm_get_subflows_max(msk);
>
> - pr_debug("msk=%p subflows=%d max=%d allow=%d\n", msk, pm->subflows,
> - subflows_max, READ_ONCE(pm->accept_subflow));
> + spin_lock_bh(&pm->lock);
> + if (!mptcp_pm_is_userspace(msk) && READ_ONCE(pm->accept_subflow)) {
> + subflows_max = mptcp_pm_get_subflows_max(msk);
>
> - /* try to avoid acquiring the lock below */
> - if (!READ_ONCE(pm->accept_subflow))
> - return false;
> + pr_debug("msk=%p subflows=%d max=%d allow=%d\n", msk, pm->subflows,
> + subflows_max, READ_ONCE(pm->accept_subflow));
>
> - spin_lock_bh(&pm->lock);
> - if (READ_ONCE(pm->accept_subflow)) {
> ret = pm->subflows < subflows_max;
> - if (ret && ++pm->subflows == subflows_max)
> + if (ret && pm->subflows == subflows_max - 1)
> WRITE_ONCE(pm->accept_subflow, false);
> }
Maybe I missed something, but could we not move this code to
mptcp_pm_kernel_accept_new_subflow()?
There here, we would have something like:
if (pm->ops->accept_new_subflow(msk)) {
spin_lock_bh(&pm->lock);
pm->subflows++;
spin_unlock_bh(&pm->lock);
}
No?
EDIT: just noticed you are doing that in patch 9/9. Can you not do that
in the same callback, but passing an extra argument to it? Or is it an
issue with the locks?
bool (*accept_new_subflow)(const struct mptcp_sock *msk, bool allow);
> + if (ret)
> + pm->subflows++;
>
> spin_unlock_bh(&pm->lock);
>
> return ret;
> @@ -1057,7 +1048,7 @@ 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->add_addr_echo) {
> + !pm_ops->add_addr_echo || !pm_ops->accept_new_subflow) {
> 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 9d159196afe5..7ec8fafeda0e 100644
> --- a/net/mptcp/pm_kernel.c
> +++ b/net/mptcp/pm_kernel.c
> @@ -1404,6 +1404,11 @@ static bool mptcp_pm_kernel_add_addr_echo(struct mptcp_sock *msk,
> (addr->id > 0 && !READ_ONCE(msk->pm.accept_addr));
> }
>
> +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_init(struct mptcp_sock *msk)
> {
> bool subflows_allowed = !!mptcp_pm_get_subflows_max(msk);
> @@ -1432,6 +1437,7 @@ struct mptcp_pm_ops mptcp_pm_kernel = {
> .add_addr_received = mptcp_pm_kernel_add_addr_received,
> .rm_addr_received = mptcp_pm_kernel_rm_addr_received,
> .add_addr_echo = mptcp_pm_kernel_add_addr_echo,
> + .accept_new_subflow = mptcp_pm_kernel_accept_new_subflow,
> .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 3f7778ab064b..d6301d809376 100644
> --- a/net/mptcp/pm_userspace.c
> +++ b/net/mptcp/pm_userspace.c
> @@ -689,6 +689,11 @@ static bool mptcp_pm_userspace_add_addr_echo(struct mptcp_sock *msk,
> return mptcp_userspace_pm_active(msk);
> }
>
> +static bool mptcp_pm_userspace_accept_new_subflow(const struct mptcp_sock *msk)
> +{
> + return mptcp_userspace_pm_active(msk);
> +}
> +
> static void mptcp_pm_userspace_release(struct mptcp_sock *msk)
> {
> mptcp_userspace_pm_free_local_addr_list(msk);
> @@ -698,6 +703,7 @@ static struct mptcp_pm_ops mptcp_pm_userspace = {
> .get_local_id = mptcp_pm_userspace_get_local_id,
> .get_priority = mptcp_pm_userspace_get_priority,
> .add_addr_echo = mptcp_pm_userspace_add_addr_echo,
> + .accept_new_subflow = mptcp_pm_userspace_accept_new_subflow,
> .release = mptcp_pm_userspace_release,
> .name = "userspace",
> .owner = THIS_MODULE,
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index 409bd415ef1d..be79940da424 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -61,9 +61,7 @@ static void subflow_generate_hmac(u64 key1, u64 key2, u32 nonce1, u32 nonce2,
> static bool mptcp_can_accept_new_subflow(const struct mptcp_sock *msk)
> {
> return mptcp_is_fully_established((void *)msk) &&
> - ((mptcp_pm_is_userspace(msk) &&
> - mptcp_userspace_pm_active(msk)) ||
> - READ_ONCE(msk->pm.accept_subflow));
> + msk->pm.ops->accept_new_subflow(msk);
I think pm->ops should only be used from pm.c. In other words, I suggest
having a dedicated patch changing this helper to call a new one added in
pm.c, e.g.
return mptcp_is_fully_established((void *)msk) &&
mptcp_pm_accept_new_subflow(msk);
WDYT?
> }
>
> /* validate received token and create truncated hmac and nonce for SYN-ACK */
Cheers,
Matt
--
Sponsored by the NGI0 Core fund.
© 2016 - 2025 Red Hat, Inc.