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

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

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.

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.

> +		}
>  
>  		err = sk_stream_wait_memory(sk, &timeo);
>  		if (err != 0)
> -- 
> 2.41.3
>