[v2] Squash to "mptcp: add last time fields in mptcp_info"

[PATCH mptcp-next v2] Squash to "mptcp: add last time fields in mptcp_info"

Posted by Geliang Tang 4 weeks ago

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.

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);
+	mptcp_data_unlock(sk);
 }
 EXPORT_SYMBOL_GPL(mptcp_diag_fill_info);
 
-- 
2.40.1

Re: [PATCH mptcp-next v2] Squash to "mptcp: add last time fields in mptcp_info"

Posted by Matthieu Baerts 3 weeks, 6 days ago

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
-- 
Sponsored by the NGI0 Core fund.

Re: [PATCH mptcp-next v2] Squash to "mptcp: add last time fields in mptcp_info"

Posted by Matthieu Baerts 3 weeks, 6 days ago

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
-- 
Sponsored by the NGI0 Core fund.

Re: [PATCH mptcp-next v2] Squash to "mptcp: add last time fields in mptcp_info"

Posted by Geliang Tang 3 weeks, 6 days ago

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