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

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
>

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
> >