[PATCH] tcp: replace head->tstamp with head->skb_mstamp_ns in tcp_tso_should_defer()

[PATCH] tcp: replace head->tstamp with head->skb_mstamp_ns in tcp_tso_should_defer()

[mengensun88]

From: MengEn Sun <mengensun@tencent.com>

The tstamp field of sk_buff is intended to implement SO_TIMESTAMP*.
However, the skb in the RTX queue does not have this field set.
Using this field in tcp_tso_should_defer() can confuse readers of
the code.

Therefore, in this function, we replace tstamp with skb_mstamp_ns
to obtain the timestamp of when a packet is sent.

Reviewed-by: YueHong Wu <yuehongwu@tencent.com>
Signed-off-by: MengEn Sun <mengensun@tencent.com>
---
 net/ipv4/tcp_output.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index 68804fd01daf..d1d167c93a4f 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -2274,7 +2274,7 @@ static bool tcp_tso_should_defer(struct sock *sk, struct sk_buff *skb,
 	head = tcp_rtx_queue_head(sk);
 	if (!head)
 		goto send_now;
-	delta = tp->tcp_clock_cache - head->tstamp;
+	delta = tp->tcp_clock_cache - head->skb_mstamp_ns;
 	/* If next ACK is likely to come too late (half srtt), do not defer */
 	if ((s64)(delta - (u64)NSEC_PER_USEC * (tp->srtt_us >> 4)) < 0)
 		goto send_now;
-- 
2.43.5

Re: [PATCH] tcp: replace head->tstamp with head->skb_mstamp_ns in tcp_tso_should_defer()

[Eric Dumazet]

On Wed, Dec 4, 2024 at 3:36 AM <mengensun88@gmail.com> wrote:
>
> From: MengEn Sun <mengensun@tencent.com>
>
> The tstamp field of sk_buff is intended to implement SO_TIMESTAMP*.
> However, the skb in the RTX queue does not have this field set.
> Using this field in tcp_tso_should_defer() can confuse readers of
> the code.
>
> Therefore, in this function, we replace tstamp with skb_mstamp_ns
> to obtain the timestamp of when a packet is sent.
>

I do not see why this patch would be needed, we have many places in
TCP using skb->tstamp just fine.

Do not be confused by unions, this would be my advice :)

Re: [PATCH] tcp: replace head->tstamp with head->skb_mstamp_ns in tcp_tso_should_defer()

[MengEn Sun]

Thank you very much for your reply!

There is no functional issue with using tstamp here.

TCP does indeed use tstamp in many places, but it seems that most of
them are related to SO_TIMESTAMP*. The main interface functions that
set tstamp in the code include the following:
- __net_timestamp
- net_timestamp_set
- __skb_tstamp_tx
The points where this tstamp is modified are documented in the kernel
documentation: Documentation/networking/timestamping.txt.


The functions that mainly modify skb_mstamp_ns include the following:
- tcp_add_tx_delay
- __tcp_transmit_skb
- tcp_write_xmit
- tcp_make_synack
- tcp_send_syn_data
The tstamp in an skb on the RTX queue has indeed not been modified by
the first group of functions mentioned above; instead, it is set by
the second group of functions before cloning the skb using skb_mstamp_ns.

I spent quite a bit of time reading the code before realizing that
the use of tstamp here is actually intended to retrieve the meaning of
skb_mstamp_ns. Therefore, I think if skb_mstamp_ns is used here, it
might give newcomers reading the TCP code a hint that this is actually
the value set by the second group of functions.

Best regards
MengEn

Re: [PATCH] tcp: replace head->tstamp with head->skb_mstamp_ns in tcp_tso_should_defer()

[Eric Dumazet]

On Wed, Dec 4, 2024 at 12:00 PM MengEn Sun <mengensun88@gmail.com> wrote:
>
> Thank you very much for your reply!
>
> There is no functional issue with using tstamp here.
>
> TCP does indeed use tstamp in many places, but it seems that most of
> them are related to SO_TIMESTAMP*.

Not at all. TCP switched to EDT model back in 2018, the goal had
nothing to do with SO_TIMESTAMP

commit d3edd06ea8ea9e03de6567fda80b8be57e21a537 tcp: provide earliest
departure time in skb->tstamp

Note how a prior field (skb->skb_mstamp) has been renamed to
skb->skb_mstamp_ns to express
the fact that a change in units happened at that time, because suddenly
TCP was providing skb->tstamp to lower stack (fq qdisc for instance
makes use of it) in ns units.

Starting from this point, skb->tstamp and skb->skb_mstamp_ns had the
same meaning as far as TCP is concerned.

Note how it is absolutely clear in the doc:

include/linux/skbuff.h:762: *   @skb_mstamp_ns: (aka @tstamp) earliest
departure time; start point
include/linux/skbuff.h:892:             u64             skb_mstamp_ns;
/* earliest departure time */

I actually had in my TODO list a _removal_ of skb_mstamp_ns, mostly because of
an unfortunate naming (mstamp would imply millisecond units, which is
simply not true)

Same remark for tp->tcp_mstamp :  quite a bad name, but unfortunately
changing it would make future backports more difficult.