netdev - RE: [PATCH bpf-next v2 2/3] net: stmmac: Add txtime support to XDP ZC

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <PH0PR11MB58305C7D394FD264F1634819D885A@PH0PR11MB5830.namprd11.prod.outlook.com>
Date: Tue, 5 Dec 2023 14:43:31 +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 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

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

>
>See tools/testing/selftests/net/so_txtime.sh for how the FQ and ETF
>qdiscs already disagree on the clock source that they use.
>