Message ID | 7930751788d3bc8155898d4a22bdd6efca8cfde0.1712486293.git.tanggeliang@kylinos.cn (mailing list archive) |
---|---|
State | Accepted, archived |
Commit | d0ef49f9fc1638e3e41ff4f1299b249c10a1ad15 |
Delegated to: | Matthieu Baerts |
Headers | show |
Series | [mptcp-next,v2] Squash to "mptcp: add last time fields in mptcp_info" | expand |
Context | Check | Description |
---|---|---|
matttbe/build | success | Build and static analysis OK |
matttbe/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 39 lines checked |
matttbe/shellcheck | success | MPTCP selftests files have not been modified |
matttbe/KVM_Validation__normal | success | Success! ✅ |
matttbe/KVM_Validation__debug | success | Success! ✅ |
matttbe/KVM_Validation__btf__only_bpftest_all_ | success | Success! ✅ |
Hi Geliang, Thank you for your modifications, that's great! Our CI did some validations and here is its report: - KVM Validation: normal: Success! ✅ - KVM Validation: debug: Success! ✅ - KVM Validation: btf (only bpftest_all): Success! ✅ - Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/8588196493 Initiator: Patchew Applier Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/ebdea5824333 Patchwork: https://patchwork.kernel.org/project/mptcp/list/?series=842144 If there are some issues, you can reproduce them using the same environment as the one used by the CI thanks to a docker image, e.g.: $ cd [kernel source code] $ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \ --pull always mptcp/mptcp-upstream-virtme-docker:latest \ auto-normal For more details: https://github.com/multipath-tcp/mptcp-upstream-virtme-docker Please note that despite all the efforts that have been already done to have a stable tests suite when executed on a public CI like here, it is possible some reported issues are not due to your modifications. Still, do not hesitate to help us improve that ;-) Cheers, MPTCP GH Action bot Bot operated by Matthieu Baerts (NGI0 Core)
Hi Geliang, On 07/04/2024 12:38, Geliang Tang wrote: > From: Geliang Tang <tanggeliang@kylinos.cn> > > Please append the following lines into the commit log: > > ''' > Since msk->last_ack_recv needs to be protected by mptcp_data_lock/unlock, > and lock_sock_fast can sleep and be quite slow, move the entire > mptcp_data_lock/unlock block after the lock/unlock_sock_fast block. > Then mptcpi_last_data_sent and mptcpi_last_data_recv are set in > lock/unlock_sock_fast block, while mptcpi_last_ack_recv is set in > mptcp_data_lock/unlock block. > ''' > > v2: > - move the entire mptcp_data_lock()/mptcp_data_unlock() block after > the lock_sock_fast()/unlock_sock_fast() block. > - can't move it inside lock_sock_fast()/unlock_sock_fast() block, this > causes a deadlock. Thank you for this v2, it looks good to me! Reviewed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> > Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn> > --- > net/mptcp/sockopt.c | 16 +++++++++------- > 1 file changed, 9 insertions(+), 7 deletions(-) > > diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c > index 2ec2fdf9f4af..40b262b2c24a 100644 > --- a/net/mptcp/sockopt.c > +++ b/net/mptcp/sockopt.c > @@ -896,9 +896,9 @@ static int mptcp_getsockopt_first_sf_only(struct mptcp_sock *msk, int level, int > void mptcp_diag_fill_info(struct mptcp_sock *msk, struct mptcp_info *info) > { > struct sock *sk = (struct sock *)msk; > - u32 now = tcp_jiffies32; > u32 flags = 0; > bool slow; > + u32 now; > > memset(info, 0, sizeof(*info)); > > @@ -927,12 +927,6 @@ void mptcp_diag_fill_info(struct mptcp_sock *msk, struct mptcp_info *info) > if (READ_ONCE(msk->can_ack)) > flags |= MPTCP_INFO_FLAG_REMOTE_KEY_RECEIVED; > info->mptcpi_flags = flags; > - mptcp_data_lock(sk); > - info->mptcpi_snd_una = msk->snd_una; > - info->mptcpi_rcv_nxt = msk->ack_seq; > - info->mptcpi_bytes_acked = msk->bytes_acked; > - info->mptcpi_last_ack_recv = jiffies_to_msecs(now - msk->last_ack_recv); > - mptcp_data_unlock(sk); > > slow = lock_sock_fast(sk); > info->mptcpi_csum_enabled = READ_ONCE(msk->csum_enabled); > @@ -944,9 +938,17 @@ void mptcp_diag_fill_info(struct mptcp_sock *msk, struct mptcp_info *info) > info->mptcpi_bytes_retrans = msk->bytes_retrans; > info->mptcpi_subflows_total = info->mptcpi_subflows + > __mptcp_has_initial_subflow(msk); > + now = tcp_jiffies32; > info->mptcpi_last_data_sent = jiffies_to_msecs(now - msk->last_data_sent); > info->mptcpi_last_data_recv = jiffies_to_msecs(now - msk->last_data_recv); > unlock_sock_fast(sk, slow); > + > + mptcp_data_lock(sk); > + info->mptcpi_snd_una = msk->snd_una; > + info->mptcpi_rcv_nxt = msk->ack_seq; > + info->mptcpi_bytes_acked = msk->bytes_acked; > + info->mptcpi_last_ack_recv = jiffies_to_msecs(now - msk->last_ack_recv); Detail: I suggest moving this line just after 'mptcp_data_lock(sk)', just to have all the "last time" counters next to each others. I can do that when applying the patch. > + mptcp_data_unlock(sk); > } > EXPORT_SYMBOL_GPL(mptcp_diag_fill_info); > Cheers, Matt
On Mon, 2024-04-08 at 10:23 +0200, Matthieu Baerts wrote: > Hi Geliang, > > On 07/04/2024 12:38, Geliang Tang wrote: > > From: Geliang Tang <tanggeliang@kylinos.cn> > > > > Please append the following lines into the commit log: > > > > ''' > > Since msk->last_ack_recv needs to be protected by > > mptcp_data_lock/unlock, > > and lock_sock_fast can sleep and be quite slow, move the entire > > mptcp_data_lock/unlock block after the lock/unlock_sock_fast block. > > Then mptcpi_last_data_sent and mptcpi_last_data_recv are set in > > lock/unlock_sock_fast block, while mptcpi_last_ack_recv is set in > > mptcp_data_lock/unlock block. > > ''' > > > > v2: > > - move the entire mptcp_data_lock()/mptcp_data_unlock() block > > after > > the lock_sock_fast()/unlock_sock_fast() block. > > - can't move it inside lock_sock_fast()/unlock_sock_fast() block, > > this > > causes a deadlock. > > Thank you for this v2, it looks good to me! > > Reviewed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> > > > Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn> > > --- > > net/mptcp/sockopt.c | 16 +++++++++------- > > 1 file changed, 9 insertions(+), 7 deletions(-) > > > > diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c > > index 2ec2fdf9f4af..40b262b2c24a 100644 > > --- a/net/mptcp/sockopt.c > > +++ b/net/mptcp/sockopt.c > > @@ -896,9 +896,9 @@ static int > > mptcp_getsockopt_first_sf_only(struct mptcp_sock *msk, int level, > > int > > void mptcp_diag_fill_info(struct mptcp_sock *msk, struct > > mptcp_info *info) > > { > > struct sock *sk = (struct sock *)msk; > > - u32 now = tcp_jiffies32; > > u32 flags = 0; > > bool slow; > > + u32 now; > > > > memset(info, 0, sizeof(*info)); > > > > @@ -927,12 +927,6 @@ void mptcp_diag_fill_info(struct mptcp_sock > > *msk, struct mptcp_info *info) > > if (READ_ONCE(msk->can_ack)) > > flags |= MPTCP_INFO_FLAG_REMOTE_KEY_RECEIVED; > > info->mptcpi_flags = flags; > > - mptcp_data_lock(sk); > > - info->mptcpi_snd_una = msk->snd_una; > > - info->mptcpi_rcv_nxt = msk->ack_seq; > > - info->mptcpi_bytes_acked = msk->bytes_acked; > > - info->mptcpi_last_ack_recv = jiffies_to_msecs(now - msk- > > >last_ack_recv); > > - mptcp_data_unlock(sk); > > > > slow = lock_sock_fast(sk); > > info->mptcpi_csum_enabled = READ_ONCE(msk->csum_enabled); > > @@ -944,9 +938,17 @@ void mptcp_diag_fill_info(struct mptcp_sock > > *msk, struct mptcp_info *info) > > info->mptcpi_bytes_retrans = msk->bytes_retrans; > > info->mptcpi_subflows_total = info->mptcpi_subflows + > > __mptcp_has_initial_subflow(msk); > > + now = tcp_jiffies32; > > info->mptcpi_last_data_sent = jiffies_to_msecs(now - msk- > > >last_data_sent); > > info->mptcpi_last_data_recv = jiffies_to_msecs(now - msk- > > >last_data_recv); > > unlock_sock_fast(sk, slow); > > + > > + mptcp_data_lock(sk); > > + info->mptcpi_snd_una = msk->snd_una; > > + info->mptcpi_rcv_nxt = msk->ack_seq; > > + info->mptcpi_bytes_acked = msk->bytes_acked; > > + info->mptcpi_last_ack_recv = jiffies_to_msecs(now - msk- > > >last_ack_recv); > > Detail: I suggest moving this line just after 'mptcp_data_lock(sk)', > just to have all the "last time" counters next to each others. > > I can do that when applying the patch. Thanks Matt. I agree, it's much better. -Geliang > > > + mptcp_data_unlock(sk); > > } > > EXPORT_SYMBOL_GPL(mptcp_diag_fill_info); > > > > Cheers, > Matt
Hi Geliang, On 07/04/2024 12:38, Geliang Tang wrote: > From: Geliang Tang <tanggeliang@kylinos.cn> > > Please append the following lines into the commit log: > > ''' > Since msk->last_ack_recv needs to be protected by mptcp_data_lock/unlock, > and lock_sock_fast can sleep and be quite slow, move the entire > mptcp_data_lock/unlock block after the lock/unlock_sock_fast block. > Then mptcpi_last_data_sent and mptcpi_last_data_recv are set in > lock/unlock_sock_fast block, while mptcpi_last_ack_recv is set in > mptcp_data_lock/unlock block. > ''' Now in our tree: - d0ef49f9fc16: "squashed" in "mptcp: add last time fields in mptcp_info" (your patch) - df2c358813a1: "squashed" in "mptcp: add last time fields in mptcp_info" (move counter) - fb5d63ae1831: tg:msg: update after the recent squash-to patch - Results: eeb8d59ecaf4..dc4002d18eb8 (export) Tests are now in progress: - export: https://github.com/multipath-tcp/mptcp_net-next/commit/2c0573e1fc19ac66ed37910de7345f52038536df/checks Cheers, Matt
diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c index 2ec2fdf9f4af..40b262b2c24a 100644 --- a/net/mptcp/sockopt.c +++ b/net/mptcp/sockopt.c @@ -896,9 +896,9 @@ static int mptcp_getsockopt_first_sf_only(struct mptcp_sock *msk, int level, int void mptcp_diag_fill_info(struct mptcp_sock *msk, struct mptcp_info *info) { struct sock *sk = (struct sock *)msk; - u32 now = tcp_jiffies32; u32 flags = 0; bool slow; + u32 now; memset(info, 0, sizeof(*info)); @@ -927,12 +927,6 @@ void mptcp_diag_fill_info(struct mptcp_sock *msk, struct mptcp_info *info) if (READ_ONCE(msk->can_ack)) flags |= MPTCP_INFO_FLAG_REMOTE_KEY_RECEIVED; info->mptcpi_flags = flags; - mptcp_data_lock(sk); - info->mptcpi_snd_una = msk->snd_una; - info->mptcpi_rcv_nxt = msk->ack_seq; - info->mptcpi_bytes_acked = msk->bytes_acked; - info->mptcpi_last_ack_recv = jiffies_to_msecs(now - msk->last_ack_recv); - mptcp_data_unlock(sk); slow = lock_sock_fast(sk); info->mptcpi_csum_enabled = READ_ONCE(msk->csum_enabled); @@ -944,9 +938,17 @@ void mptcp_diag_fill_info(struct mptcp_sock *msk, struct mptcp_info *info) info->mptcpi_bytes_retrans = msk->bytes_retrans; info->mptcpi_subflows_total = info->mptcpi_subflows + __mptcp_has_initial_subflow(msk); + now = tcp_jiffies32; info->mptcpi_last_data_sent = jiffies_to_msecs(now - msk->last_data_sent); info->mptcpi_last_data_recv = jiffies_to_msecs(now - msk->last_data_recv); unlock_sock_fast(sk, slow); + + mptcp_data_lock(sk); + info->mptcpi_snd_una = msk->snd_una; + info->mptcpi_rcv_nxt = msk->ack_seq; + info->mptcpi_bytes_acked = msk->bytes_acked; + info->mptcpi_last_ack_recv = jiffies_to_msecs(now - msk->last_ack_recv); + mptcp_data_unlock(sk); } EXPORT_SYMBOL_GPL(mptcp_diag_fill_info);