* [PATCH net] tcp: initialize skb tx timestamp key before cloning
@ 2026-09-15 21:44 David Wei
2026-09-15 22:21 ` Eric Dumazet
0 siblings, 1 reply; 5+ messages in thread
From: David Wei @ 2026-09-15 21:44 UTC (permalink / raw)
To: netdev
Cc: Eric Dumazet, Neal Cardwell, Kuniyuki Iwashima, David S. Miller,
Jakub Kicinski, Paolo Abeni, Simon Horman, Jason Xing
tcp_tx_timestamp() can select skbs from the rtx queue when all the
copied data has been sent in tcp_sendmsg_locked(). These skbs may be
already cloned, sharing the same shinfo, and handed off into the lower
tx layers.
tcp_tx_timestamp() sets tx_flags before setting skb tskey, racing with
any reader of both. It is possible to observe a valid tx_flag, but an
uninitialized tskey, which produces a large underflow after subtracting
the socket tskey.
Initialize skb tskey just before cloning while its shinfo is still
private. In tcp_tx_timestamp(), if it selects a cloned skb from the rtx
queue from the current tcp_sendmsg_locked() call (i.e. tx_flags and
txstamp_ack are 0) and the pre-init skb tskey matches, then tx_flags can
be set safely.
If the skb is not shared, coming from the write queue, then both tskey
and tx_flags are set, as before.
Fixes: 838eb9687691 ("tcp: tcp_tx_timestamp() must look at the rtx queue")
Assisted-by: LLM
Signed-off-by: David Wei <dw@davidwei.uk>
---
net/ipv4/tcp.c | 21 ++++++++++++++++-----
net/ipv4/tcp_output.c | 20 +++++++++++++-------
2 files changed, 29 insertions(+), 12 deletions(-)
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 562752352afe..0d6bbb41d5f8 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -481,20 +481,31 @@ void tcp_init_sock(struct sock *sk)
static void tcp_tx_timestamp(struct sock *sk, struct sockcm_cookie *sockc)
{
struct sk_buff *skb = tcp_write_queue_tail(sk);
+ struct skb_shared_info *shinfo;
u32 tsflags = sockc->tsflags;
+ struct tcp_skb_cb *tcb;
+ u32 tskey;
if (unlikely(!skb))
skb = skb_rb_last(&sk->tcp_rtx_queue);
+ if (unlikely(!skb))
+ return;
- if (tsflags && skb) {
- struct skb_shared_info *shinfo = skb_shinfo(skb);
- struct tcp_skb_cb *tcb = TCP_SKB_CB(skb);
+ shinfo = skb_shinfo(skb);
+ tcb = TCP_SKB_CB(skb);
+ tskey = tcb->seq + skb->len - 1;
+ if (skb_cloned(skb) &&
+ !shinfo->tx_flags && !tcb->txstamp_ack) {
+ if (shinfo->tskey != tskey)
+ return;
+ } else if (tsflags & SOF_TIMESTAMPING_TX_RECORD_MASK) {
+ shinfo->tskey = tskey;
+ }
+ if (tsflags) {
sock_tx_timestamp(sk, sockc, &shinfo->tx_flags);
if (tsflags & SOF_TIMESTAMPING_TX_ACK)
tcb->txstamp_ack |= TSTAMP_ACK_SK;
- if (tsflags & SOF_TIMESTAMPING_TX_RECORD_MASK)
- shinfo->tskey = TCP_SKB_CB(skb)->seq + skb->len - 1;
}
if (cgroup_bpf_enabled(CGROUP_SOCK_OPS) &&
diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index 00417a429222..ee3efcbbdf3a 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -1522,6 +1522,12 @@ static void tcp_v6_send_check(struct sock *sk, struct sk_buff *skb)
}
#endif
+static bool tcp_has_tx_tstamp(const struct sk_buff *skb)
+{
+ return TCP_SKB_CB(skb)->txstamp_ack ||
+ (skb_shinfo(skb)->tx_flags & SKBTX_ANY_TSTAMP);
+}
+
/* This routine actually transmits TCP packets queued in by
* tcp_do_sendmsg(). This is used by both the initial
* transmission and possible later retransmissions.
@@ -1559,8 +1565,14 @@ static int __tcp_transmit_skb(struct sock *sk, struct sk_buff *skb,
tcp_skb_tsorted_save(oskb) {
if (unlikely(skb_cloned(oskb)))
skb = pskb_copy(oskb, gfp_mask);
- else
+ else {
+ if (oskb->len &&
+ !(TCP_SKB_CB(oskb)->tcp_flags & TCPHDR_SYN) &&
+ !tcp_has_tx_tstamp(oskb))
+ skb_shinfo(oskb)->tskey =
+ TCP_SKB_CB(oskb)->seq + oskb->len - 1;
skb = skb_clone(oskb, gfp_mask);
+ }
} tcp_skb_tsorted_restore(oskb);
if (unlikely(!skb))
@@ -1795,12 +1807,6 @@ static void tcp_adjust_pcount(struct sock *sk, const struct sk_buff *skb, int de
tcp_verify_left_out(tp);
}
-static bool tcp_has_tx_tstamp(const struct sk_buff *skb)
-{
- return TCP_SKB_CB(skb)->txstamp_ack ||
- (skb_shinfo(skb)->tx_flags & SKBTX_ANY_TSTAMP);
-}
-
static void tcp_fragment_tstamp(struct sk_buff *skb, struct sk_buff *skb2)
{
struct skb_shared_info *shinfo = skb_shinfo(skb);
--
2.53.0-Meta
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH net] tcp: initialize skb tx timestamp key before cloning
2026-09-15 21:44 [PATCH net] tcp: initialize skb tx timestamp key before cloning David Wei
@ 2026-09-15 22:21 ` Eric Dumazet
2026-09-16 17:23 ` David Wei
0 siblings, 1 reply; 5+ messages in thread
From: Eric Dumazet @ 2026-09-15 22:21 UTC (permalink / raw)
To: David Wei
Cc: netdev, Neal Cardwell, Kuniyuki Iwashima, David S. Miller,
Jakub Kicinski, Paolo Abeni, Simon Horman, Jason Xing
On Tue, Sep 15, 2026 at 2:44 PM David Wei <dw@davidwei.uk> wrote:
>
> tcp_tx_timestamp() can select skbs from the rtx queue when all the
> copied data has been sent in tcp_sendmsg_locked(). These skbs may be
> already cloned, sharing the same shinfo, and handed off into the lower
> tx layers.
>
> tcp_tx_timestamp() sets tx_flags before setting skb tskey, racing with
> any reader of both. It is possible to observe a valid tx_flag, but an
> uninitialized tskey, which produces a large underflow after subtracting
> the socket tskey.
>
> Initialize skb tskey just before cloning while its shinfo is still
> private. In tcp_tx_timestamp(), if it selects a cloned skb from the rtx
> queue from the current tcp_sendmsg_locked() call (i.e. tx_flags and
> txstamp_ack are 0) and the pre-init skb tskey matches, then tx_flags can
> be set safely.
>
> If the skb is not shared, coming from the write queue, then both tskey
> and tx_flags are set, as before.
>
> Fixes: 838eb9687691 ("tcp: tcp_tx_timestamp() must look at the rtx queue")
> Assisted-by: LLM
> Signed-off-by: David Wei <dw@davidwei.uk>
pw-bot: cr
Sorry, this patch significantly hurts TCP performance and has many bugs.
I am sure your LLM can find a cheaper solution if you ask it to try harder.
Some hints if needed:
First idea:
1) In tcp_tx_timestamp():
Keep shinfo / tcb accesses inside if (unlikely(tsflags)) so normal
traffic pays zero cost.
When falling back to skb = skb_rb_last(&sk->tcp_rtx_queue):
Verify TCP_SKB_CB(skb)->end_seq == tp->write_seq and
!tcp_has_tx_tstamp(skb). If either fails, do not touch skb.
Always write shinfo->tskey before sock_tx_timestamp(sk, sockc,
&shinfo->tx_flags) (and do the same in
bpf_sock_ops_enable_tx_tstamp()).
If pre-initializing shinfo->tskey in __tcp_transmit_skb() before
skb_clone(oskb, gfp_mask) (which already dirties shinfo->dataref in
the same cache line), guard it with !tcp_has_tx_tstamp(oskb) and use
TCP_SKB_CB(oskb)->end_seq - 1 (or seq + len - 1), without the bogus if
(shinfo->tskey != tskey) check in tcp_tx_timestamp().
2) In tcp_sendmsg_locked() (to preserve TX_SCHED / TX_SOFTWARE /
TX_HARDWARE on non-blocking partial sends):
Before calling tcp_push_one() / __tcp_push_pending_frames() at the end
of the loop (when skb->len >= size_goal), check if
(!sk_stream_memory_free(sk)) goto wait_for_space;.
At wait_for_space:, only call tcp_push() if timeo != 0 (since when
!timeo, sk_stream_wait_memory() immediately returns -EAGAIN and out:
will run tcp_tx_timestamp() before tcp_push()).
Second idea:
We can solve this cleanly and minimally by:
Reordering the writes in tcp_tx_timestamp() (and
bpf_sock_ops_enable_tx_tstamp()) so shinfo->tskey is written first via
WRITE_ONCE(), followed by publishing shinfo->tx_flags via
smp_store_release() in __sock_tx_timestamp().
Guarding tcp_rtx_queue fallback with if (skb &&
tcp_has_tx_tstamp(skb)) return; so we never overwrite tskey on an
in-flight cloned skb that already has active timestamp flags.
Performing a symmetric smp_load_acquire() on
skb_shinfo(orig_skb)->tx_flags in __skb_tstamp_tx() (and
skb_complete_tx_timestamp()) before reading tskey.
This avoids touching __tcp_transmit_skb() or the tcp_sendmsg_locked()
loop, adds zero overhead to the TX fastpath (skb_tx_timestamp()
remains a relaxed check), and compiles to plain MOVs on x86 while
providing proper release-acquire ordering (stlrb/ldarb) on ARM64.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net] tcp: initialize skb tx timestamp key before cloning
2026-09-15 22:21 ` Eric Dumazet
@ 2026-09-16 17:23 ` David Wei
2026-09-16 17:31 ` Eric Dumazet
0 siblings, 1 reply; 5+ messages in thread
From: David Wei @ 2026-09-16 17:23 UTC (permalink / raw)
To: Eric Dumazet
Cc: netdev, Neal Cardwell, Kuniyuki Iwashima, David S. Miller,
Jakub Kicinski, Paolo Abeni, Simon Horman, Jason Xing
On 2026-09-15 15:21, Eric Dumazet wrote:
> On Tue, Sep 15, 2026 at 2:44 PM David Wei <dw@davidwei.uk> wrote:
>>
>> tcp_tx_timestamp() can select skbs from the rtx queue when all the
>> copied data has been sent in tcp_sendmsg_locked(). These skbs may be
>> already cloned, sharing the same shinfo, and handed off into the lower
>> tx layers.
>>
>> tcp_tx_timestamp() sets tx_flags before setting skb tskey, racing with
>> any reader of both. It is possible to observe a valid tx_flag, but an
>> uninitialized tskey, which produces a large underflow after subtracting
>> the socket tskey.
>>
>> Initialize skb tskey just before cloning while its shinfo is still
>> private. In tcp_tx_timestamp(), if it selects a cloned skb from the rtx
>> queue from the current tcp_sendmsg_locked() call (i.e. tx_flags and
>> txstamp_ack are 0) and the pre-init skb tskey matches, then tx_flags can
>> be set safely.
>>
>> If the skb is not shared, coming from the write queue, then both tskey
>> and tx_flags are set, as before.
>>
>> Fixes: 838eb9687691 ("tcp: tcp_tx_timestamp() must look at the rtx queue")
>> Assisted-by: LLM
>> Signed-off-by: David Wei <dw@davidwei.uk>
>
> pw-bot: cr
>
> Sorry, this patch significantly hurts TCP performance and has many bugs.
>
> I am sure your LLM can find a cheaper solution if you ask it to try harder.
This wasn't a yolo patch straight from an LLM, I tried quite hard to
reason with the bug and possible solutions. Maybe that's the problem -
the LLM is actually smarter than me :)
>
> Some hints if needed:
>
> First idea:
>
> 1) In tcp_tx_timestamp():
>
> Keep shinfo / tcb accesses inside if (unlikely(tsflags)) so normal
> traffic pays zero cost.
> When falling back to skb = skb_rb_last(&sk->tcp_rtx_queue):
> Verify TCP_SKB_CB(skb)->end_seq == tp->write_seq and
> !tcp_has_tx_tstamp(skb). If either fails, do not touch skb.
> Always write shinfo->tskey before sock_tx_timestamp(sk, sockc,
> &shinfo->tx_flags) (and do the same in
> bpf_sock_ops_enable_tx_tstamp()).
> If pre-initializing shinfo->tskey in __tcp_transmit_skb() before
> skb_clone(oskb, gfp_mask) (which already dirties shinfo->dataref in
> the same cache line), guard it with !tcp_has_tx_tstamp(oskb) and use
> TCP_SKB_CB(oskb)->end_seq - 1 (or seq + len - 1), without the bogus if
> (shinfo->tskey != tskey) check in tcp_tx_timestamp().
>
> 2) In tcp_sendmsg_locked() (to preserve TX_SCHED / TX_SOFTWARE /
> TX_HARDWARE on non-blocking partial sends):
>
> Before calling tcp_push_one() / __tcp_push_pending_frames() at the end
> of the loop (when skb->len >= size_goal), check if
> (!sk_stream_memory_free(sk)) goto wait_for_space;.
> At wait_for_space:, only call tcp_push() if timeo != 0 (since when
> !timeo, sk_stream_wait_memory() immediately returns -EAGAIN and out:
> will run tcp_tx_timestamp() before tcp_push()).
>
> Second idea:
>
> We can solve this cleanly and minimally by:
>
> Reordering the writes in tcp_tx_timestamp() (and
> bpf_sock_ops_enable_tx_tstamp()) so shinfo->tskey is written first via
> WRITE_ONCE(), followed by publishing shinfo->tx_flags via
> smp_store_release() in __sock_tx_timestamp().
I did think about this solution. I still don't fully grasp the memory
model and how READ/WRITE_ONCE and acquire-release works, so I wasn't
entirely sure if it was sufficient.
There could be situations where the skb is pushed early, and the lower
layers copy the shinfo e.g. GSO and COW. I don't have experience with
this code, so I find it hard to evaluate plausible paths that LLMs
suggest.
That's why I opted for the pre-set tskey before clone approach in this
RFC. I didn't think this would affect perf that badly, but yeah it is a
really hot function.
I believe I can make the GSO path work with acquire-release. For COW, do
you think it is actually relevant?
>
> Guarding tcp_rtx_queue fallback with if (skb &&
> tcp_has_tx_tstamp(skb)) return; so we never overwrite tskey on an
> in-flight cloned skb that already has active timestamp flags.
>
> Performing a symmetric smp_load_acquire() on
> skb_shinfo(orig_skb)->tx_flags in __skb_tstamp_tx() (and
> skb_complete_tx_timestamp()) before reading tskey.
> This avoids touching __tcp_transmit_skb() or the tcp_sendmsg_locked()
> loop, adds zero overhead to the TX fastpath (skb_tx_timestamp()
> remains a relaxed check), and compiles to plain MOVs on x86 while
> providing proper release-acquire ordering (stlrb/ldarb) on ARM64.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net] tcp: initialize skb tx timestamp key before cloning
2026-09-16 17:23 ` David Wei
@ 2026-09-16 17:31 ` Eric Dumazet
2026-09-17 8:26 ` Jason Xing
0 siblings, 1 reply; 5+ messages in thread
From: Eric Dumazet @ 2026-09-16 17:31 UTC (permalink / raw)
To: David Wei
Cc: netdev, Neal Cardwell, Kuniyuki Iwashima, David S. Miller,
Jakub Kicinski, Paolo Abeni, Simon Horman, Jason Xing
On Wed, Sep 16, 2026 at 10:23 AM David Wei <dw@davidwei.uk> wrote:
>
> On 2026-09-15 15:21, Eric Dumazet wrote:
> > On Tue, Sep 15, 2026 at 2:44 PM David Wei <dw@davidwei.uk> wrote:
> >>
> >> tcp_tx_timestamp() can select skbs from the rtx queue when all the
> >> copied data has been sent in tcp_sendmsg_locked(). These skbs may be
> >> already cloned, sharing the same shinfo, and handed off into the lower
> >> tx layers.
> >>
> >> tcp_tx_timestamp() sets tx_flags before setting skb tskey, racing with
> >> any reader of both. It is possible to observe a valid tx_flag, but an
> >> uninitialized tskey, which produces a large underflow after subtracting
> >> the socket tskey.
> >>
> >> Initialize skb tskey just before cloning while its shinfo is still
> >> private. In tcp_tx_timestamp(), if it selects a cloned skb from the rtx
> >> queue from the current tcp_sendmsg_locked() call (i.e. tx_flags and
> >> txstamp_ack are 0) and the pre-init skb tskey matches, then tx_flags can
> >> be set safely.
> >>
> >> If the skb is not shared, coming from the write queue, then both tskey
> >> and tx_flags are set, as before.
> >>
> >> Fixes: 838eb9687691 ("tcp: tcp_tx_timestamp() must look at the rtx queue")
> >> Assisted-by: LLM
> >> Signed-off-by: David Wei <dw@davidwei.uk>
> >
> > pw-bot: cr
> >
> > Sorry, this patch significantly hurts TCP performance and has many bugs.
> >
> > I am sure your LLM can find a cheaper solution if you ask it to try harder.
>
> This wasn't a yolo patch straight from an LLM, I tried quite hard to
> reason with the bug and possible solutions. Maybe that's the problem -
> the LLM is actually smarter than me :)
>
> >
> > Some hints if needed:
> >
> > First idea:
> >
> > 1) In tcp_tx_timestamp():
> >
> > Keep shinfo / tcb accesses inside if (unlikely(tsflags)) so normal
> > traffic pays zero cost.
> > When falling back to skb = skb_rb_last(&sk->tcp_rtx_queue):
> > Verify TCP_SKB_CB(skb)->end_seq == tp->write_seq and
> > !tcp_has_tx_tstamp(skb). If either fails, do not touch skb.
> > Always write shinfo->tskey before sock_tx_timestamp(sk, sockc,
> > &shinfo->tx_flags) (and do the same in
> > bpf_sock_ops_enable_tx_tstamp()).
> > If pre-initializing shinfo->tskey in __tcp_transmit_skb() before
> > skb_clone(oskb, gfp_mask) (which already dirties shinfo->dataref in
> > the same cache line), guard it with !tcp_has_tx_tstamp(oskb) and use
> > TCP_SKB_CB(oskb)->end_seq - 1 (or seq + len - 1), without the bogus if
> > (shinfo->tskey != tskey) check in tcp_tx_timestamp().
> >
> > 2) In tcp_sendmsg_locked() (to preserve TX_SCHED / TX_SOFTWARE /
> > TX_HARDWARE on non-blocking partial sends):
> >
> > Before calling tcp_push_one() / __tcp_push_pending_frames() at the end
> > of the loop (when skb->len >= size_goal), check if
> > (!sk_stream_memory_free(sk)) goto wait_for_space;.
> > At wait_for_space:, only call tcp_push() if timeo != 0 (since when
> > !timeo, sk_stream_wait_memory() immediately returns -EAGAIN and out:
> > will run tcp_tx_timestamp() before tcp_push()).
> >
> > Second idea:
> >
> > We can solve this cleanly and minimally by:
> >
> > Reordering the writes in tcp_tx_timestamp() (and
> > bpf_sock_ops_enable_tx_tstamp()) so shinfo->tskey is written first via
> > WRITE_ONCE(), followed by publishing shinfo->tx_flags via
> > smp_store_release() in __sock_tx_timestamp().
>
> I did think about this solution. I still don't fully grasp the memory
> model and how READ/WRITE_ONCE and acquire-release works, so I wasn't
> entirely sure if it was sufficient.
>
> There could be situations where the skb is pushed early, and the lower
> layers copy the shinfo e.g. GSO and COW. I don't have experience with
> this code, so I find it hard to evaluate plausible paths that LLMs
> suggest.
>
> That's why I opted for the pre-set tskey before clone approach in this
> RFC. I didn't think this would affect perf that badly, but yeah it is a
> really hot function.
>
> I believe I can make the GSO path work with acquire-release. For COW, do
> you think it is actually relevant?
I am traveling today back to France, I will answer to your email in
~24 hours or so.
Thanks.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net] tcp: initialize skb tx timestamp key before cloning
2026-09-16 17:31 ` Eric Dumazet
@ 2026-09-17 8:26 ` Jason Xing
0 siblings, 0 replies; 5+ messages in thread
From: Jason Xing @ 2026-09-17 8:26 UTC (permalink / raw)
To: Eric Dumazet
Cc: David Wei, netdev, Neal Cardwell, Kuniyuki Iwashima,
David S. Miller, Jakub Kicinski, Paolo Abeni, Simon Horman
On Thu, Sep 17, 2026 at 1:32 AM Eric Dumazet <edumazet@google.com> wrote:
>
> On Wed, Sep 16, 2026 at 10:23 AM David Wei <dw@davidwei.uk> wrote:
> >
> > On 2026-09-15 15:21, Eric Dumazet wrote:
> > > On Tue, Sep 15, 2026 at 2:44 PM David Wei <dw@davidwei.uk> wrote:
> > >>
> > >> tcp_tx_timestamp() can select skbs from the rtx queue when all the
> > >> copied data has been sent in tcp_sendmsg_locked(). These skbs may be
> > >> already cloned, sharing the same shinfo, and handed off into the lower
> > >> tx layers.
> > >>
> > >> tcp_tx_timestamp() sets tx_flags before setting skb tskey, racing with
> > >> any reader of both. It is possible to observe a valid tx_flag, but an
> > >> uninitialized tskey, which produces a large underflow after subtracting
> > >> the socket tskey.
> > >>
> > >> Initialize skb tskey just before cloning while its shinfo is still
> > >> private. In tcp_tx_timestamp(), if it selects a cloned skb from the rtx
> > >> queue from the current tcp_sendmsg_locked() call (i.e. tx_flags and
> > >> txstamp_ack are 0) and the pre-init skb tskey matches, then tx_flags can
> > >> be set safely.
> > >>
> > >> If the skb is not shared, coming from the write queue, then both tskey
> > >> and tx_flags are set, as before.
> > >>
> > >> Fixes: 838eb9687691 ("tcp: tcp_tx_timestamp() must look at the rtx queue")
> > >> Assisted-by: LLM
> > >> Signed-off-by: David Wei <dw@davidwei.uk>
The commit 838eb9687691 is a best-effort attempt because it's likely
to miss reporting qdisc/driver timestamps. Everything is caused by
failing to accurately spot the last portion of each send syscall.
Things like this (when memory is under pressure) happen.
> > >
> > > pw-bot: cr
> > >
> > > Sorry, this patch significantly hurts TCP performance and has many bugs.
> > >
> > > I am sure your LLM can find a cheaper solution if you ask it to try harder.
> >
> > This wasn't a yolo patch straight from an LLM, I tried quite hard to
> > reason with the bug and possible solutions. Maybe that's the problem -
> > the LLM is actually smarter than me :)
> >
> > >
> > > Some hints if needed:
> > >
> > > First idea:
> > >
> > > 1) In tcp_tx_timestamp():
> > >
> > > Keep shinfo / tcb accesses inside if (unlikely(tsflags)) so normal
> > > traffic pays zero cost.
> > > When falling back to skb = skb_rb_last(&sk->tcp_rtx_queue):
> > > Verify TCP_SKB_CB(skb)->end_seq == tp->write_seq and
> > > !tcp_has_tx_tstamp(skb). If either fails, do not touch skb.
> > > Always write shinfo->tskey before sock_tx_timestamp(sk, sockc,
> > > &shinfo->tx_flags) (and do the same in
> > > bpf_sock_ops_enable_tx_tstamp()).
> > > If pre-initializing shinfo->tskey in __tcp_transmit_skb() before
> > > skb_clone(oskb, gfp_mask) (which already dirties shinfo->dataref in
> > > the same cache line), guard it with !tcp_has_tx_tstamp(oskb) and use
> > > TCP_SKB_CB(oskb)->end_seq - 1 (or seq + len - 1), without the bogus if
> > > (shinfo->tskey != tskey) check in tcp_tx_timestamp().
It minimizes the scope of handling this case without hurting normal
traffic performance. Indeed.
> > >
> > > 2) In tcp_sendmsg_locked() (to preserve TX_SCHED / TX_SOFTWARE /
> > > TX_HARDWARE on non-blocking partial sends):
> > >
> > > Before calling tcp_push_one() / __tcp_push_pending_frames() at the end
> > > of the loop (when skb->len >= size_goal), check if
> > > (!sk_stream_memory_free(sk)) goto wait_for_space;.
> > > At wait_for_space:, only call tcp_push() if timeo != 0 (since when
> > > !timeo, sk_stream_wait_memory() immediately returns -EAGAIN and out:
> > > will run tcp_tx_timestamp() before tcp_push()).
The minor flaw of net timestamping is just the case where
wait_for_space and do_error conditions are met. Moving
sk_stream_wait_memory ahead of tcp_push is probably a good idea to
detect if this time the skb happens to be the last one. But it only
covers the non-blocking scenario, right?
> > >
> > > Second idea:
> > >
> > > We can solve this cleanly and minimally by:
> > >
> > > Reordering the writes in tcp_tx_timestamp() (and
> > > bpf_sock_ops_enable_tx_tstamp()) so shinfo->tskey is written first via
> > > WRITE_ONCE(), followed by publishing shinfo->tx_flags via
> > > smp_store_release() in __sock_tx_timestamp().
It takes care of all the readers and writers tx_flags. I'm not sure if
adding the pairs here is worth it. Admittedly it's cleaner and easy to
understand.
No offense, Eric, I'm wondering if we need to revert the commit
838eb9687691 or just let the bad case happen silently, which both are
actually a best effort. If so, the users indeed lose one timestamp
record for that send syscall, but fortunately, the bad latency will
soon be reflected on the next send syscall. I think this is how we
discussed it at Netconf a few months ago? I'm not so sure if we need
to add too much code just to work around the minor flaw of net
timestamping feature.
Thanks,
Jason
> >
> > I did think about this solution. I still don't fully grasp the memory
> > model and how READ/WRITE_ONCE and acquire-release works, so I wasn't
> > entirely sure if it was sufficient.
> >
> > There could be situations where the skb is pushed early, and the lower
> > layers copy the shinfo e.g. GSO and COW. I don't have experience with
> > this code, so I find it hard to evaluate plausible paths that LLMs
> > suggest.
> >
> > That's why I opted for the pre-set tskey before clone approach in this
> > RFC. I didn't think this would affect perf that badly, but yeah it is a
> > really hot function.
> >
> > I believe I can make the GSO path work with acquire-release. For COW, do
> > you think it is actually relevant?
>
> I am traveling today back to France, I will answer to your email in
> ~24 hours or so.
>
> Thanks.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2026-09-17 8:27 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-09-15 21:44 [PATCH net] tcp: initialize skb tx timestamp key before cloning David Wei
2026-09-15 22:21 ` Eric Dumazet
2026-09-16 17:23 ` David Wei
2026-09-16 17:31 ` Eric Dumazet
2026-09-17 8:26 ` Jason Xing
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox