On Sun, Feb 16, 2025 at 6:58 AM Martin KaFai Lau <martin.lau@xxxxxxxxx> wrote: > > On 2/15/25 2:23 PM, Jason Xing wrote: > > On Sun, Feb 16, 2025 at 2:08 AM Willem de Bruijn > > <willemdebruijn.kernel@xxxxxxxxx> wrote: > >> > >> Jason Xing wrote: > >>> On Sat, Feb 15, 2025 at 11:06 PM Willem de Bruijn > >>> <willemdebruijn.kernel@xxxxxxxxx> wrote: > >>>> > >>>> Jason Xing wrote: > >>>>> Support hw SCM_TSTAMP_SND case for bpf timestamping. > >>>>> > >>>>> Add a new sock_ops callback, BPF_SOCK_OPS_TS_HW_OPT_CB. This > >>>>> callback will occur at the same timestamping point as the user > >>>>> space's hardware SCM_TSTAMP_SND. The BPF program can use it to > >>>>> get the same SCM_TSTAMP_SND timestamp without modifying the > >>>>> user-space application. > >>>>> > >>>>> To avoid increasing the code complexity, replace SKBTX_HW_TSTAMP > >>>>> with SKBTX_HW_TSTAMP_NOBPF instead of changing numerous callers > >>>>> from driver side using SKBTX_HW_TSTAMP. The new definition of > >>>>> SKBTX_HW_TSTAMP means the combination tests of socket timestamping > >>>>> and bpf timestamping. After this patch, drivers can work under the > >>>>> bpf timestamping. > >>>>> > >>>>> Considering some drivers doesn't assign the skb with hardware > >>>>> timestamp, > >>>> > >>>> This is not for a real technical limitation, like the skb perhaps > >>>> being cloned or shared? > >>> > >>> Agreed on this point. I'm kind of familiar with I40E, so I dare to say > >>> the reason why it doesn't assign the hwtstamp is because the skb will > >>> soon be destroyed, that is to say, it's pointless to assign the > >>> timestamp. > >> > >> Makes sense. > >> > >> But that does not ensure that the skb is exclusively owned. Nor that > >> the same is true for all drivers using this API (which is not small, > >> but small enough to manually review if need be). > >> > >> The first two examples I happened to look at, i40e and bnx2x, both use > >> skb_get() to get a non-exclusive skb reference for their ptp_tx_skb. > > I think the existing __skb_tstamp_tx() function is also assigning to > skb_hwtstamps(skb). The skb may be cloned from the orig_skb first, but they > still share the same shinfo. My understanding is that this patch is assigning to > the shinfo earlier, so it should not have changed the driver's expectation on > the skb_hwtstamps(skb) after calling __skb_tstamp_tx(). If there are drivers > assuming exclusive access to the skb_hwtstamps(skb), probably it is something > that needs to be addressed regardless and should not be the common case? Right, it's also what I was trying to say but missed. Thanks for the supplementary info:) Thanks, Jason
