On Tuesday, December 5, 2023 10:55 PM, Willem de Bruijn wrote: >Song, Yoong Siang wrote: >> On Monday, December 4, 2023 10:58 PM, Willem de Bruijn wrote: >> >Song, Yoong Siang wrote: >> >> On Friday, December 1, 2023 11:02 PM, Jesper Dangaard Brouer wrote: >> >> >On 12/1/23 07:24, Song Yoong Siang wrote: >> >> >> This patch enables txtime support to XDP zero copy via XDP Tx >> >> >> metadata framework. >> >> >> >> >> >> Signed-off-by: Song Yoong Siang<yoong.siang.song@xxxxxxxxx> >> >> >> --- >> >> >> drivers/net/ethernet/stmicro/stmmac/stmmac.h | 2 ++ >> >> >> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 13 +++++++++++++ >> >> >> 2 files changed, 15 insertions(+) >> >> > >> >> >I think we need to see other drivers using this new feature to evaluate >> >> >if API is sane. >> >> > >> >> >I suggest implementing this for igc driver (chip i225) and also for igb >> >> >(i210 chip) that both support this kind of LaunchTime feature in HW. >> >> > >> >> >The API and stmmac driver takes a u64 as time. >> >> >I'm wondering how this applies to i210 that[1] have 25-bit for >> >> >LaunchTime (with 32 nanosec granularity) limiting LaunchTime max 0.5 >> >> >second into the future. >> >> >And i225 that [1] have 30-bit max 1 second into the future. >> >> > >> >> > >> >> >[1] >> >> >https://github.com/xdp-project/xdp- >> >> >project/blob/master/areas/tsn/code01_follow_qdisc_TSN_offload.org >> >> >> >> I am using u64 for launch time because existing EDT framework is using it. >> >> Refer to struct sk_buff below. Both u64 and ktime_t can be used as launch time. >> >> I choose u64 because ktime_t often requires additional type conversion and >> >> we didn't expect negative value of time. >> >> >> >> include/linux/skbuff.h-744- * @tstamp: Time we arrived/left >> >> include/linux/skbuff.h:745- * @skb_mstamp_ns: (aka @tstamp) earliest >departure >> >time; start point >> >> include/linux/skbuff.h-746- * for retransmit timer >> >> -- >> >> include/linux/skbuff.h-880- union { >> >> include/linux/skbuff.h-881- ktime_t tstamp; >> >> include/linux/skbuff.h:882- u64 skb_mstamp_ns; /* earliest >departure >> >time */ >> >> include/linux/skbuff.h-883- }; >> >> >> >> tstamp/skb_mstamp_ns are used by various drivers for launch time support >> >> on normal packet, so I think u64 should be "friendly" to all the drivers. For an >> >> example, igc driver will take launch time from tstamp and recalculate it >> >> accordingly (i225 expect user to program "delta time" instead of "time" into >> >> HW register). >> >> >> >> drivers/net/ethernet/intel/igc/igc_main.c-1602- txtime = skb->tstamp; >> >> drivers/net/ethernet/intel/igc/igc_main.c-1603- skb->tstamp = ktime_set(0, 0); >> >> drivers/net/ethernet/intel/igc/igc_main.c:1604- launch_time = >> >igc_tx_launchtime(tx_ring, txtime, &first_flag, &insert_empty); >> >> >> >> Do you think this is enough to say the API is sane? >> > >> >u64 nsec sounds sane to be. It must be made explicit with clock source >> >it is against. >> > >> >> The u64 launch time should base on NIC PTP hardware clock (PHC). >> I will add documentation saying which clock source it is against > >It's not that obvious to me that that is the right and only choice. >See below. > >> >Some applications could want to do the conversion from a clock source >> >to raw NIC cycle counter in userspace or BPF and program the raw >> >value. So it may be worthwhile to add an clock source argument -- even >> >if initially only CLOCK_MONOTONIC is supported. >> >> Sorry, not so understand your suggestion on adding clock source argument. >> Are you suggesting to add clock source for the selftest xdp_hw_metadata apps? >> IMHO, no need to add clock source as the clock source for launch time >> should always base on NIC PHC. > >This is not how FQ and ETF qdiscs pass timestamps to drivers today. > >Those are in CLOCK_MONOTONIC or CLOCK_TAI. The driver is expected to >convert from that to its descriptor format, both to the reduced bit >width and the NIC PHC. > >See also for instance how sch_etf has an explicit q->clock_id match, >and SO_TXTIME added an sk_clock_id for the same purpose: to agree on >which clock source is being used. I see. Thank for the explanation. I will try to add clock source arguments In next version.
