| Message ID | 4de819beea2e553b61718a3b4c9b6180979961e0.1743054942.git.tanggeliang@kylinos.cn (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Matthieu Baerts |
| Headers | show |
| Series | BPF path manager, part 6 | expand |
| Context | Check | Description |
|---|---|---|
| matttbe/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 59 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! ✅ |
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
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); + 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); + + pr_debug("msk=%p subflows=%d max=%d allow=%d\n", msk, pm->subflows, + subflows_max, READ_ONCE(pm->accept_subflow)); + if (READ_ONCE(pm->accept_subflow)) { if (allow) return true; + + ret = pm->subflows < subflows_max; + if (ret && pm->subflows == subflows_max - 1) + WRITE_ONCE(pm->accept_subflow, false); } return ret;