Re: [PATCH net-next v4 06/11] net-timestamp: support SCM_TSTAMP_ACK for bpf extension

Martin KaFai Lau <martin.lau@xxxxxxxxx> · Fri, 13 Dec 2024 14:46:09 -0800

On 12/13/24 6:49 AM, Jason Xing wrote:

diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 5bdf13ac26ef..82bb26f5b214 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -3321,7 +3321,8 @@ static void tcp_ack_tstamp(struct sock *sk, struct sk_buff *skb,
       const struct skb_shared_info *shinfo;

       /* Avoid cache line misses to get skb_shinfo() and shinfo->tx_flags */
Please take a look at the above comment.

-     if (likely(!TCP_SKB_CB(skb)->txstamp_ack))
+     if (likely(!TCP_SKB_CB(skb)->txstamp_ack &&
+                !TCP_SKB_CB(skb)->txstamp_ack_bpf))
Change the test here to:
         if (likely(!TCP_SKB_CB(skb)->txstamp_ack &&
                    !(skb_shinfo(skb)->tx_flags & SKBTX_BPF)))

Does it make sense?
It surely works. Talking about the result only, introducing SKBTX_BPF
can work for all the cases. However, in the ACK case, the above code
snippet will access the shinfo->tx_flags, which triggers cache line
misses. I also mentioned this on purpose in the patch [06/11].

ah. my bad. I somehow totally skipped the comment and message in this patch when 
jumping between patch 6 and 7.

Not an expert. so curious if it matters testing skb_shinfo(skb)->tx_flags or not 
here? e.g. The tcp_v4_fill_cb() in the earlier rx path, 
skb_hwtstamps(skb)->hwtstamp is also read. Willem?







