| Message ID | 1733279773-32536-1-git-send-email-mengensun@tencent.com (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | tcp: replace head->tstamp with head->skb_mstamp_ns in tcp_tso_should_defer() | expand |
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 :)
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
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.
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;