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

Jason Xing
2026-04-07T03:34:03Z

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 :(

>
> Right now, push-based seems rather arbitrary to me, informed more by
> technical limitations than a clear design. Perhaps per-packet makes
> more sense, esp. since BPF calls are cheap (compared to the other
> errqueue mechanism).

Agreed.

Thanks,
Jason