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

[Martin KaFai Lau <martin.lau@linux.dev>]

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?