Hi Geliang,
On 27/03/2025 07:04, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
>
> This patch reduces the dependency on mptcp_pm_is_userspace() to identify
> userspace PM from in-kernel PM by using mptcp_pm_accept_new_subflow()
> helper in mptcp_pm_allow_new_subflow().
>
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
> net/mptcp/pm.c | 27 ++++-----------------------
> net/mptcp/pm_kernel.c | 10 ++++++++++
> 2 files changed, 14 insertions(+), 23 deletions(-)
>
> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index 1c8395d3baa9..c72d2fade555 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c
> @@ -461,34 +461,15 @@ bool mptcp_pm_accept_new_subflow(struct mptcp_sock *msk, bool allow)
> bool mptcp_pm_allow_new_subflow(struct mptcp_sock *msk)
> {
> struct mptcp_pm_data *pm = &msk->pm;
> - unsigned int subflows_max;
> int ret = 0;
>
> - 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;
> - }
> - 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));
> -
> - /* try to avoid acquiring the lock below */
> - if (!READ_ONCE(pm->accept_subflow))
> + if (!mptcp_pm_accept_new_subflow(msk, true))
> return false;
>
> spin_lock_bh(&pm->lock);
> - if (READ_ONCE(pm->accept_subflow)) {
> - ret = pm->subflows < subflows_max;
> - if (ret && ++pm->subflows == subflows_max)
> - WRITE_ONCE(pm->accept_subflow, false);
> - }
> + ret = mptcp_pm_accept_new_subflow(msk, false);
I think it would make more sense **not** to call pm->ops callback under
the PM lock.
I think a BPF PM should not modify this structure directly. If something
needs to be changed, it should be done via a kfunc that will
automatically take the PM spin lock if needed, and release it at the
end. Only the "PM core" (pm.c) should modify it and the in-kernel and
userspace PM because they require a way to store data. A BPF PM can
store data in a map elsewhere.
> + if (ret)
> + pm->subflows++;
> spin_unlock_bh(&pm->lock);
>
> return ret;
> diff --git a/net/mptcp/pm_kernel.c b/net/mptcp/pm_kernel.c
> index ee3915d33e04..5cc35ee122ff 100644
> --- a/net/mptcp/pm_kernel.c
> +++ b/net/mptcp/pm_kernel.c
> @@ -1403,11 +1403,21 @@ static bool mptcp_pm_kernel_accept_new_subflow(struct mptcp_sock *msk,
> bool allow)
> {
> struct mptcp_pm_data *pm = &msk->pm;
> + unsigned int subflows_max;
> bool ret = false;
>
> + subflows_max = mptcp_pm_get_subflows_max(msk);
this variable is only needed for pm->accept_subflow && !allow, maybe we
should declare it there, and assign it only when needed?
> + pr_debug("msk=%p subflows=%d max=%d allow=%d\n", msk, pm->subflows,
> + subflows_max, READ_ONCE(pm->accept_subflow));
Perhaps we can drop this pr_debug, or remove subflows_max: easy to get
the info from userspace.
> +
> if (READ_ONCE(pm->accept_subflow)) {
> if (allow)
> return true;
> +
> + ret = pm->subflows < subflows_max;
> + if (ret && pm->subflows == subflows_max - 1)
detail, I think it makes more sense to use "+ 1":
if (ret && (pm->subflows + 1) == subflows_max)
> + WRITE_ONCE(pm->accept_subflow, false);
> }
>
> return ret;
Cheers,
Matt
--
Sponsored by the NGI0 Core fund.