| Message ID | 20241003082231.759759-1-dongml2@chinatelecom.cn (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 269084f748524fa1a3fb8eb530eb70f77e7c3e4a |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: tcp: refresh tcp_mstamp for compressed ack in timer | expand |
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 > >
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!
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 {
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(+)