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.
Right. i40e uses skb_get() in i40e_tsyn() introduced by commit
beb0dff1251d. bnx2x uses it in bnx2x_start_xmit() introduced by commit
eeed018cbfa3.
Here are all the drivers listed to be reviewed:
1. drivers/net/ethernet/amd/xgbe/xgbe-drv.c
It uses skb_get() in xgbe_prep_tx_tstamp().
2. drivers/net/ethernet/aquantia/atlantic/aq_ptp.c
Please see aq_ptp_xmit()->__aq_ptp_skb_put(). Every skb enqueued into
ring->buff will be 'skb_get()' here.
3. drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
In this case, I cannot see the skb is 'skb_get()' before bnxt_tx_ts_cmp().
bnxt_start_xmit()
-> tx_buf->skb = skb;
I stopped here and found out about this special case and then pondered
over this point.
Willem, does this mean that we are unable to safely modify the field
in skb? I'm afraid not. Sorry for my limited knowledge about drivers
here... I feel skb_get() cannot be used to know if the skb is safely
accessed. Because, let me put in this way, if skb passed to
skb_tstamp_tx() can be destroyed in the meantime, that means skb in
skb_tstamp_tx() is not safe anymore, which also means all readers
accessing this skb are not safe anymore. Based on the analysis, I
think accessing the skb by BPF program is safe.
Thanks,
Jason
>
> > >
> > > > this patch do the assignment and then BPF program
> > > > can acquire the hwstamp from skb directly.
> > >
> > > If the above is not the case and it is safe to write to the skb_shinfo,
> > > and only if respinning anyway, grammar:
> >
> > From what I've known about various drivers (although very limited),
> > it's safe to do the assignment.
> >
> > >
> > > s/doesn't/don't/
> > > s/do/does/
> >
> > Thanks for catching these things. If the re-spin is necessary, I will
> > fix them all for sure.
> >
> > Thanks,
> > Jason
>
>
