Re: [PATCH net-next v4 07/11] net-timestamp: support hwtstamp print for bpf extension

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

On 12/12/24 3:25 PM, Martin KaFai Lau wrote:
> A more subtle thing for the hwtstamps case is, afaik the bpf prog will not be 
> called. All drivers are still only testing SKBTX_HW_TSTAMP instead of testing
> (SKBTX_HW_TSTAMP | SKBTX_BPF).
> 
> There are a lot of drivers to change though. A quick thought is to rename the 
> existing SKBTX_HW_TSTAMP (e.g. __SKBTX_HW_TSTAMP = 1 << 0) and define 
> SKBTX_HW_TSTAMP like:
> 
> #define SKBTX_HW_TSTAMP (__SKBTX_HW_TSTAMP | SKBTX_BPF)
> 
> Then change some of the existing skb_shinfo(skb)->tx_flags "setting" site to use 
> __SKBTX_HW_TSTAMP instead. e.g. in __sock_tx_timestamp(). Not very pretty but 
> may be still better than changing many drivers. May be there is a better way...
> 
> While talking about where to test the SKBTX_BPF bit, I wonder if the new 
> skb_tstamp_is_set() is needed. For the non SKBTX_HW_TSTAMP case, the number of 
> tx_flags testing sites should be limited, so should be easy to add the SKBTX_BPF 
> bit test. e.g. at the __dev_queue_xmit, test "if (unlikely(skb_shinfo(skb)- 
>  >tx_flags & (SKBTX_SCHED_TSTAMP | SKBTX_BPF)))". Patch 6 has also tested the 
> bpf specific bit at tcp_ack_tstamp() before calling the __skb_tstamp_tx().
> 
> At the beginning of __skb_tstamp_tx(), do something like this:
> 
> void __skb_tstamp_tx(struct sk_buff *orig_skb,
>               const struct sk_buff *ack_skb,
>               struct skb_shared_hwtstamps *hwtstamps,
>               struct sock *sk, int tstype)
> {
>      if (cgroup_bpf_enabled(CGROUP_SOCK_OPS) &&
>          unlikely(skb_shinfo(skb)->tx_flags & SKBTX_BPF))
>          __skb_tstamp_tx_bpf(sk, orig_skb, hwtstamps, tstype);
> 
>      if (unlikely(!(skb_shinfo(skb)->tx_flags & ~SKBTX_BPF)))

This is not enough. I was wrong here. The test in skb_tstamp_is_set() is needed 
when SKBTX_BPF is not set.

>          return;