On Sat, Dec 14, 2024 at 7:15 AM Martin KaFai Lau <martin.lau@xxxxxxxxx> wrote: > > On 12/13/24 7:13 AM, Jason Xing wrote: > >>> -static void __skb_tstamp_tx_bpf(struct sock *sk, struct sk_buff *skb, int tstype) > >>> +static void __skb_tstamp_tx_bpf(struct sock *sk, struct sk_buff *skb, > >>> + struct skb_shared_hwtstamps *hwtstamps, > >>> + int tstype) > >>> { > >>> + struct timespec64 tstamp; > >>> + u32 args[2] = {0, 0}; > >>> int op; > >>> > >>> if (!sk) > >>> @@ -5552,6 +5556,11 @@ static void __skb_tstamp_tx_bpf(struct sock *sk, struct sk_buff *skb, int tstype > >>> break; > >>> case SCM_TSTAMP_SND: > >>> op = BPF_SOCK_OPS_TS_SW_OPT_CB; > >>> + if (hwtstamps) { > >>> + tstamp = ktime_to_timespec64(hwtstamps->hwtstamp); > >> Avoid this conversion which is likely not useful to the bpf prog. Directly pass > >> hwtstamps->hwtstamp (in ns?) to the bpf prog. Put lower 32bits in args[0] and > >> higher 32bits in args[1]. > > It makes sense. > > When replying the patch 2 thread, I noticed it may not even have to pass the > hwtstamps in args here. > > Can "*skb_hwtstamps(skb) = *hwtstamps;" be done before calling the bpf prog? > Then the bpf prog can directly get it from skb_shinfo(skb)->hwtstamps. > It is like reading other fields in skb_shinfo(skb), e.g. the > skb_shinfo(skb)->tskey discussed in patch 10. The bpf prog will have a more > consistent experience in reading different fields of the skb_shinfo(skb). > skb_shinfo(skb)->hwtstamps is a more intuitive place to obtain the hwtstamp than > the broken up args[0] and args[1]. On top of that, there is also an older > "skb_hwtstamp" field in "struct bpf_sock_ops". Right, right, last night, fortunately, I also spotted it. Let the bpf prog parse the shared info from skb then. A new callback for hwtstamp is needed, I suppose.