On Sat, Jan 18, 2025 at 9:43 AM Jason Xing <kerneljasonxing@xxxxxxxxx> wrote: > > On Sat, Jan 18, 2025 at 8:47 AM Martin KaFai Lau <martin.lau@xxxxxxxxx> wrote: > > > > On 1/15/25 3:56 PM, Jason Xing wrote: > > > On Thu, Jan 16, 2025 at 6:48 AM Martin KaFai Lau <martin.lau@xxxxxxxxx> wrote: > > >> > > >> On 1/12/25 3:37 AM, Jason Xing wrote: > > >>> Support SCM_TSTAMP_SND case. Then we will get the software > > >>> timestamp when the driver is about to send the skb. Later, I > > >>> will support the hardware timestamp. > > >> > > >>> diff --git a/net/core/skbuff.c b/net/core/skbuff.c > > >>> index 169c6d03d698..0fb31df4ed95 100644 > > >>> --- a/net/core/skbuff.c > > >>> +++ b/net/core/skbuff.c > > >>> @@ -5578,6 +5578,9 @@ static void __skb_tstamp_tx_bpf(struct sk_buff *skb, struct sock *sk, int tstype > > >>> case SCM_TSTAMP_SCHED: > > >>> op = BPF_SOCK_OPS_TS_SCHED_OPT_CB; > > >>> break; > > >>> + case SCM_TSTAMP_SND: > > >>> + op = BPF_SOCK_OPS_TS_SW_OPT_CB; > > >> > > >> For the hwtstamps case, is skb_hwtstamps(skb) set? From looking at a few > > >> drivers, it does not look like it. I don't see the hwtstamps support in patch 10 > > >> either. What did I miss ? > > > > > > Sorry, I missed adding a new flag, namely, BPF_SOCK_OPS_TS_HW_OPT_CB. > > > I can also skip adding that new one and rename > > > BPF_SOCK_OPS_TS_SW_OPT_CB accordingly for sw and hw cases if we > > > finally decide to use hwtstamps parameter to distinguish two different > > > cases. > > > > I think having a separate BPF_SOCK_OPS_TS_HW_OPT_CB is better considering your > > earlier hwtstamps may be NULL comment. I don't see the drivers I looked at > > passing NULL though but the comment of skb_tstamp_tx did say it may be NULL :/ > > Yep, I was trying not to rely on or trust the hardware/driver's > implementation, or else it will let the bpf program fetch the software > timestamp instead of hardware timestamp which will cause unexpected > behaviour. > > After re-reading this part, I reckon that using this SKBTX_IN_PROGRESS > flag is enough to recognize if we are in the hardware timestamping > generation period. I will try this one since it requires much less > modification. For the record only, SKBTX_IN_PROGRESS cannot represent if we're generating the hardware timestamp. For example, in I40e driver, i40e_xmit_frame_ring() calls i40e_tsyn() to set SKBTX_IN_PROGRESS before generating the software SND timestamp in i40e_tx_map() by calling skb_tx_timestamp(). Therefore, I will use the current patch directly. Thanks, Jason
