Date: Tue, 5 Dec 2023 15:28:14 +0000
From: "Song, Yoong Siang" <yoong.siang.song@...el.com>
To: 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

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

I see. Thank for the explanation. I will try to add clock source arguments
In next version.

Powered by blists - more mailing lists