diff mbox series

[mptcp-next,v5,4/7] mptcp: pm: update pm lock order in mptcp_pm_worker

Message ID 739e3667698915fa690d08c751c1846bd9442df2.1743054942.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 success total: 0 errors, 0 warnings, 0 checks, 39 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 27, 2025, 6:04 a.m. UTC 
From: Geliang Tang <tanggeliang@kylinos.cn>

Later functions that cannot hold the mptcp pm lock will be called from
the PM worker, so this patch modifies the order of holding the lock
at the beginning of this function and releasing the lock at the end.

The new order is to obtain a copy of pm->status while holding the mptcp
pm lock, then read the copy after releasing the lock.

For each PM status flag, hold the lock, clear this flag of pm->status,
and then call each handling function before or after releasing the lock
as needed.

Finally, hold the lock before calling __mptcp_pm_kernel_worker() and
release it afterwards.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 net/mptcp/pm.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

Comments

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

On 27/03/2025 07:04, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> Later functions that cannot hold the mptcp pm lock will be called from
> the PM worker, so this patch modifies the order of holding the lock
> at the beginning of this function and releasing the lock at the end.
> 
> The new order is to obtain a copy of pm->status while holding the mptcp
> pm lock, then read the copy after releasing the lock.
> 
> For each PM status flag, hold the lock, clear this flag of pm->status,
> and then call each handling function before or after releasing the lock
> as needed.
> 
> Finally, hold the lock before calling __mptcp_pm_kernel_worker() and
> release it afterwards.
> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  net/mptcp/pm.c | 19 +++++++++++++------
>  1 file changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index c72d2fade555..1e681acaad7f 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c
> @@ -932,26 +932,33 @@ void mptcp_pm_subflow_chk_stale(const struct mptcp_sock *msk, struct sock *ssk)
>  void mptcp_pm_worker(struct mptcp_sock *msk)
>  {
>  	struct mptcp_pm_data *pm = &msk->pm;
> +	u8 status;
>  
>  	msk_owned_by_me(msk);
>  
>  	if (!(pm->status & MPTCP_PM_WORK_MASK))
>  		return;
>  
> -	spin_lock_bh(&msk->pm.lock);
> +	spin_lock_bh(&pm->lock);
> +	status = READ_ONCE(pm->status);

Do you think it would be OK to clear all the bits that are going to be
checked here (or once at the end), using MPTCP_PM_WORK_MASK? Before the
PM lock was released temporally in some sections, I guess it should be fine.

Also you don't need READ_ONCE() here as pm->status is protected only by
the pm lock.

> +	spin_unlock_bh(&pm->lock);
>  
> -	pr_debug("msk=%p status=%x\n", msk, pm->status);
> -	if (pm->status & BIT(MPTCP_PM_ADD_ADDR_SEND_ACK)) {
> +	pr_debug("msk=%p status=%x\n", msk, status);
> +	if (status & BIT(MPTCP_PM_ADD_ADDR_SEND_ACK)) {
> +		spin_lock_bh(&pm->lock);

I *think* we should change the way the PM lock is currently handled. For
the moment, we lock very big sections, and not only when it is needed.
That is probably easier somehow, but it might not be clear when it is
locked or not, and required unlock/lock in some other cases.

Changing the lock might have a cost in some cases, but probably a gain
in others. Concretely, here, we should not lock now, and let the callee
the responsibility to do so only when required.

By doing that, all PM callbacks will not be called with the PM locks,
which would make easier to deal with kfunc with BPF.

If you are not comfortable doing such modifications, do not hesitate to
tell us to get some helps. I can try to look when I have a bit of time
if you prefer.

>  		pm->status &= ~BIT(MPTCP_PM_ADD_ADDR_SEND_ACK);

(so this should be done before/after)

>  		mptcp_pm_addr_send_ack(msk);
> +		spin_unlock_bh(&pm->lock);
>  	}
> -	if (pm->status & BIT(MPTCP_PM_RM_ADDR_RECEIVED)) {
> +	if (status & BIT(MPTCP_PM_RM_ADDR_RECEIVED)) {
> +		spin_lock_bh(&pm->lock);

(same here)

>  		pm->status &= ~BIT(MPTCP_PM_RM_ADDR_RECEIVED);

(same here)

>  		mptcp_pm_rm_addr_recv(msk);
> +		spin_unlock_bh(&pm->lock);
>  	}
> +	spin_lock_bh(&pm->lock);

(same here and in the next patches)

>  	__mptcp_pm_kernel_worker(msk);
> -
> -	spin_unlock_bh(&msk->pm.lock);
> +	spin_unlock_bh(&pm->lock);
>  }
>  
>  static void mptcp_pm_ops_init(struct mptcp_sock *msk,
Cheers,
Matt
diff mbox series

Patch

diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index c72d2fade555..1e681acaad7f 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -932,26 +932,33 @@  void mptcp_pm_subflow_chk_stale(const struct mptcp_sock *msk, struct sock *ssk)
 void mptcp_pm_worker(struct mptcp_sock *msk)
 {
 	struct mptcp_pm_data *pm = &msk->pm;
+	u8 status;
 
 	msk_owned_by_me(msk);
 
 	if (!(pm->status & MPTCP_PM_WORK_MASK))
 		return;
 
-	spin_lock_bh(&msk->pm.lock);
+	spin_lock_bh(&pm->lock);
+	status = READ_ONCE(pm->status);
+	spin_unlock_bh(&pm->lock);
 
-	pr_debug("msk=%p status=%x\n", msk, pm->status);
-	if (pm->status & BIT(MPTCP_PM_ADD_ADDR_SEND_ACK)) {
+	pr_debug("msk=%p status=%x\n", msk, status);
+	if (status & BIT(MPTCP_PM_ADD_ADDR_SEND_ACK)) {
+		spin_lock_bh(&pm->lock);
 		pm->status &= ~BIT(MPTCP_PM_ADD_ADDR_SEND_ACK);
 		mptcp_pm_addr_send_ack(msk);
+		spin_unlock_bh(&pm->lock);
 	}
-	if (pm->status & BIT(MPTCP_PM_RM_ADDR_RECEIVED)) {
+	if (status & BIT(MPTCP_PM_RM_ADDR_RECEIVED)) {
+		spin_lock_bh(&pm->lock);
 		pm->status &= ~BIT(MPTCP_PM_RM_ADDR_RECEIVED);
 		mptcp_pm_rm_addr_recv(msk);
+		spin_unlock_bh(&pm->lock);
 	}
+	spin_lock_bh(&pm->lock);
 	__mptcp_pm_kernel_worker(msk);
-
-	spin_unlock_bh(&msk->pm.lock);
+	spin_unlock_bh(&pm->lock);
 }
 
 static void mptcp_pm_ops_init(struct mptcp_sock *msk,