[PATCH net-next] net: tcp: refresh tcp_mstamp for compressed ack in timer

[PATCH net-next] net: tcp: refresh tcp_mstamp for compressed ack in timer

[Menglong Dong]

For now, we refresh the tcp_mstamp for delayed acks and keepalives, but
not for the compressed ack in tcp_compressed_ack_kick().

I have not found out the effact of the tcp_mstamp when sending ack, but
we can still refresh it for the compressed ack to keep consistent.

Signed-off-by: Menglong Dong <dongml2@chinatelecom.cn>
---
 net/ipv4/tcp_timer.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/ipv4/tcp_timer.c b/net/ipv4/tcp_timer.c
index 79064580c8c0..1f37a37f9c82 100644
--- a/net/ipv4/tcp_timer.c
+++ b/net/ipv4/tcp_timer.c
@@ -851,6 +851,7 @@ static enum hrtimer_restart tcp_compressed_ack_kick(struct hrtimer *timer)
 			 * LINUX_MIB_TCPACKCOMPRESSED accurate.
 			 */
 			tp->compressed_ack--;
+			tcp_mstamp_refresh(tp);
 			tcp_send_ack(sk);
 		}
 	} else {
-- 
2.39.5

Re: [PATCH net-next] net: tcp: refresh tcp_mstamp for compressed ack in timer

[Eric Dumazet]

On Thu, Oct 3, 2024 at 10:23 AM Menglong Dong <menglong8.dong@gmail.com> wrote:
>
> For now, we refresh the tcp_mstamp for delayed acks and keepalives, but
> not for the compressed ack in tcp_compressed_ack_kick().
>
> I have not found out the effact of the tcp_mstamp when sending ack, but
> we can still refresh it for the compressed ack to keep consistent.

This was a choice I made for the following reason :

delayed ack timer can happen sometime 40ms later. Thus the
tcp_mstamp_refresh(tp) was probably welcome.

Compressed ack timer is scheduled for min( 5% of RTT, 1ms). It is
usually in the 200 usec range.

So sending the prior tsval (for flow using TCP TS) was ok (and right
most of the time), and not changing PAWS or EDT logic.

Although I do not object to your patch, there is no strong argument
for it or against it.

Reviewed-by: Eric Dumazet <edumazet@google.com>

>
> Signed-off-by: Menglong Dong <dongml2@chinatelecom.cn>
> ---
>  net/ipv4/tcp_timer.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/net/ipv4/tcp_timer.c b/net/ipv4/tcp_timer.c
> index 79064580c8c0..1f37a37f9c82 100644
> --- a/net/ipv4/tcp_timer.c
> +++ b/net/ipv4/tcp_timer.c
> @@ -851,6 +851,7 @@ static enum hrtimer_restart tcp_compressed_ack_kick(struct hrtimer *timer)
>                          * LINUX_MIB_TCPACKCOMPRESSED accurate.
>                          */
>                         tp->compressed_ack--;
> +                       tcp_mstamp_refresh(tp);
>                         tcp_send_ack(sk);
>                 }
>         } else {
> --
> 2.39.5
>

Re: [PATCH net-next] net: tcp: refresh tcp_mstamp for compressed ack in timer

[Menglong Dong]

On Thu, Oct 3, 2024 at 4:47 PM Eric Dumazet <edumazet@google.com> wrote:
>
> On Thu, Oct 3, 2024 at 10:23 AM Menglong Dong <menglong8.dong@gmail.com> wrote:
> >
> > For now, we refresh the tcp_mstamp for delayed acks and keepalives, but
> > not for the compressed ack in tcp_compressed_ack_kick().
> >
> > I have not found out the effact of the tcp_mstamp when sending ack, but
> > we can still refresh it for the compressed ack to keep consistent.
>
> This was a choice I made for the following reason :
>
> delayed ack timer can happen sometime 40ms later. Thus the
> tcp_mstamp_refresh(tp) was probably welcome.
>
> Compressed ack timer is scheduled for min( 5% of RTT, 1ms). It is
> usually in the 200 usec range.
>

Thanks for the explanation! I'm writing a tool, which tries to capture
the latency from __tcp_transmit_skb() to dev_hard_start_xmit() according
to the skb->tstamp, and the compressed ack case confuses my
application.

Maybe someone else can do similar things, and they can benefit from
this patch too.

Thanks!
Menglong Dong

> So sending the prior tsval (for flow using TCP TS) was ok (and right
> most of the time), and not changing PAWS or EDT logic.
>
> Although I do not object to your patch, there is no strong argument
> for it or against it.
>
> Reviewed-by: Eric Dumazet <edumazet@google.com>
>
> >
> > Signed-off-by: Menglong Dong <dongml2@chinatelecom.cn>
> > ---
> >  net/ipv4/tcp_timer.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/net/ipv4/tcp_timer.c b/net/ipv4/tcp_timer.c
> > index 79064580c8c0..1f37a37f9c82 100644
> > --- a/net/ipv4/tcp_timer.c
> > +++ b/net/ipv4/tcp_timer.c
> > @@ -851,6 +851,7 @@ static enum hrtimer_restart tcp_compressed_ack_kick(struct hrtimer *timer)
> >                          * LINUX_MIB_TCPACKCOMPRESSED accurate.
> >                          */
> >                         tp->compressed_ack--;
> > +                       tcp_mstamp_refresh(tp);
> >                         tcp_send_ack(sk);
> >                 }
> >         } else {
> > --
> > 2.39.5
> >

Re: [PATCH net-next] net: tcp: refresh tcp_mstamp for compressed ack in timer

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Thu,  3 Oct 2024 16:22:31 +0800 you wrote:
> For now, we refresh the tcp_mstamp for delayed acks and keepalives, but
> not for the compressed ack in tcp_compressed_ack_kick().
> 
> I have not found out the effact of the tcp_mstamp when sending ack, but
> we can still refresh it for the compressed ack to keep consistent.
> 
> Signed-off-by: Menglong Dong <dongml2@chinatelecom.cn>
> 
> [...]

Here is the summary with links:
  - [net-next] net: tcp: refresh tcp_mstamp for compressed ack in timer
    https://git.kernel.org/netdev/net-next/c/269084f74852

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html