Re: [PATCH net-next v2 3/4] bpf-timestamp: keep track of the skb when wait_for_space occurs

Jason Xing
2026-04-07T07:43:44Z

On Tue, Apr 7, 2026 at 11:33 AM Jason Xing <kerneljasonxing@gmail.com> wrote:
>
> On Mon, Apr 6, 2026 at 10:37 PM Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
> >
> > Jason Xing wrote:
> > > On Mon, Apr 6, 2026 at 10:28 AM Willem de Bruijn
> > > <willemdebruijn.kernel@gmail.com> wrote:
> > > >
> > > > Jason Xing wrote:
> > > > > From: Jason Xing <kernelxing@tencent.com>
> > > > >
> > > > > The patch is the 1/2 part of push-level granularity feature.
> > > > >
> > > > > Tag the skb in tcp_sendmsg_locked() when wait_for_space occurs even
> > > > > though it might not carry the last byte of the sendmsg.
> > > > >
> > > > > Prior to the patch, BPF timestamping cannot cover this case:
> > > > > The following steps reproduce this:
> > > > > 1) skb A is the current last skb before entering wait_for_space process
> > > > > 2) tcp_push() pushes A without any tag
> > > > > 3) A is transmitted from TCP to driver without putting any skb carrying
> > > > > timestamps in the error queue, like SCHED, DRV/HARDWARE.
> > > > > 4) sk_stream_wait_memory() sleeps for a while and then returns with an
> > > > > error code. Note that the socket lock is released.
> > > > > 5) skb A finally gets acked and removed from the rtx queue.
> > > > > 6) continue with the rest of tcp_sendmsg_locked(): it will jump to(goto)
> > > > > 'do_error' label and then 'out' label.
> > > > > 7) at this moment, skb A turns out to be the last one in this send
> > > > > syscall, and miss the following tcp_bpf_tx_timestamp() opportunity
> > > > > before the final tcp_push()
> > > > > 8) BPF script fails to see any timestamps this time
> > > > >
> > > > > Signed-off-by: Yushan Zhou <katrinzhou@tencent.com>
> > > > > Signed-off-by: Jason Xing <kernelxing@tencent.com>
> > > > > ---
> > > > > net/ipv4/tcp.c | 4 +++-
> > > > > 1 file changed, 3 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> > > > > index c603b90057f6..7d030a11d004 100644
> > > > > --- a/net/ipv4/tcp.c
> > > > > +++ b/net/ipv4/tcp.c
> > > > > @@ -1400,9 +1400,11 @@ int tcp_sendmsg_locked(struct sock *sk, struct msghdr *msg, size_t size)
> > > > > wait_for_space:
> > > > > set_bit(SOCK_NOSPACE, &sk->sk_socket->flags);
> > > > > tcp_remove_empty_skb(sk);
> > > > > - if (copied)
> > > > > + if (copied) {
> > > > > + tcp_bpf_tx_timestamp(sk);
> > > > > tcp_push(sk, flags & ~MSG_MORE, mss_now,
> > > > > TCP_NAGLE_PUSH, size_goal);
> > > >
> > > > Now the number of skbs that will be tracked will be unpredictable,
> > > > varying based on memory pressure.
> > >
> > > Right, I put some effort into writing a selftests to check how many
> > > push functions get called at one time and failed to do so.
> > >
> > > >
> > > > That sounds hard to use to me. Especially if these extra pushes
> > > > cannot be identified as such.
> > > >
> > > > Perhaps if all skbs from the same sendmsg call can be identified,
> > > > that would help explain pattern in data resulting from these
> > > > uncommon extra data points.
> > >
> > > You meant move tcp_bpf_tx_timestamp before tcp_skb_entail()? That is
> > > close to packet basis without considering fragmentation of skb :)
> >
> > No, I meant somehow in the notification having a way to identify all
> > the skbs belonging to the same sendmsg call, to allow filtering on
> > that. But I also don't immediately see how to do that (without adding
> > yet another counter say).
>
> If we don't build the relationship between skb and sendmsg (just like
> the SENDMSG sock option), we will have no clue on how to calculate. If
> we only take care of the skb from the view of the syscall layer, it's
> fine by moving tcp_bpf_tx_timestamp() before tcp_skb_entail(). But in
> terms of per skb even generated beneath TCP due to gso/tso, there is
> only one way to correlate: adding an additional member in the skb
> structure to store its sendmsg time. This discussion is only suitable
> for use cases like net_timestamping.
>
> Well, my key point is that, I have to admit, the above (including
> existing bpf script net_timestamping) is a less effective way which
> definitely harms the performance because of the extremely frequent
> look-up process. It's not suitable for 7x24 observability in
> production. What we've done internally is make the kernel layer as
> lightweight/easy as possible and let the timestamping feature throw
> each record into a ring buffer that the application can read, sort and
> calculate. This arch survives the performance. But that's simply what
> the design of the kernel module is, given the fast deployment in
> production. I suppose in the future we could build a userspace tool
> like blktrace to monitor efficiently instead of the selftest sample.
> Honestly I don't like look-up action.
>
> Since we're modifying the kernel, how about adding a new member to
> record sendmsg time which bpf script is able to read. The whole
> scenario looks like this:
> 1) in tcp_sendmsg_locked(), record the sendmsg time for each skb
> 2) in either tso_fragment() or tcp_gso_tstamp(), each new skb will get
> a copy of its original skb
> 3) in each stage, bpf script reads the skb's sendmsg time and the
> current time, and then effortlessly do the math.
>
> At this point, what I had in mind is we have two options:
> 1) only handle the skb from the view of the send syscall layer, which
> is, for sure, very simple but not thorough.
> 2) stick to a pure authentic packet basis, then adding a new member
> seems inevitable. so the question would be where to add? The space of
> the skb structure is very precious :(

Finding a suitable place to put this timestamp is really hard. IIRC,
we can't expand the size of struct skb_shared_info so easily since
it's a global effect.

I'm wondering if we can turn the per-packet mode into a non-compatible
feature by reusing 'u32 tskey' to store a microsecond timestamp of
sendmsg.

Thanks,
Jason