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 Siangyoong.siang.song@intel.com
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.