Date: Tue, 05 Dec 2023 09:55:18 -0500
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: "Song, Yoong Siang" <yoong.siang.song@...el.com>, 
 Willem de Bruijn <willemdebruijn.kernel@...il.com>, 
 Jesper Dangaard Brouer <hawk@...nel.org>, 
 "David S . Miller" <davem@...emloft.net>, 
 Eric Dumazet <edumazet@...gle.com>, 
 Jakub Kicinski <kuba@...nel.org>, 
 Paolo Abeni <pabeni@...hat.com>, 
 Jonathan Corbet <corbet@....net>, 
 Bjorn Topel <bjorn@...nel.org>, 
 "Karlsson, Magnus" <magnus.karlsson@...el.com>, 
 "Fijalkowski, Maciej" <maciej.fijalkowski@...el.com>, 
 Jonathan Lemon <jonathan.lemon@...il.com>, 
 Alexei Starovoitov <ast@...nel.org>, 
 Daniel Borkmann <daniel@...earbox.net>, 
 John Fastabend <john.fastabend@...il.com>, 
 Stanislav Fomichev <sdf@...gle.com>, 
 Lorenzo Bianconi <lorenzo@...nel.org>, 
 Tariq Toukan <tariqt@...dia.com>, 
 Willem de Bruijn <willemb@...gle.com>, 
 Maxime Coquelin <mcoquelin.stm32@...il.com>, 
 Andrii Nakryiko <andrii@...nel.org>, 
 Mykola Lysenko <mykolal@...com>, 
 Martin KaFai Lau <martin.lau@...ux.dev>, 
 Song Liu <song@...nel.org>, 
 Yonghong Song <yonghong.song@...ux.dev>, 
 KP Singh <kpsingh@...nel.org>, 
 Hao Luo <haoluo@...gle.com>, 
 Jiri Olsa <jolsa@...nel.org>, 
 Shuah Khan <shuah@...nel.org>, 
 Alexandre Torgue <alexandre.torgue@...s.st.com>, 
 Jose Abreu <joabreu@...opsys.com>
Cc: "netdev@...r.kernel.org" <netdev@...r.kernel.org>, 
 "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, 
 "linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>, 
 "bpf@...r.kernel.org" <bpf@...r.kernel.org>, 
 "xdp-hints@...-project.net" <xdp-hints@...-project.net>, 
 "linux-stm32@...md-mailman.stormreply.com" <linux-stm32@...md-mailman.stormreply.com>, 
 "linux-arm-kernel@...ts.infradead.org" <linux-arm-kernel@...ts.infradead.org>, 
 "linux-kselftest@...r.kernel.org" <linux-kselftest@...r.kernel.org>
Subject: RE: [PATCH bpf-next v2 2/3] net: stmmac: Add txtime support to XDP ZC

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@...el.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.

Powered by blists - more mailing lists