Message ID | 20231201062421.1074768-3-yoong.siang.song@intel.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:bcd1:0:b0:403:3b70:6f57 with SMTP id r17csp915463vqy; Thu, 30 Nov 2023 22:25:31 -0800 (PST) X-Google-Smtp-Source: AGHT+IEeWkYwKJDDmCgojnYiWH6lisjXaotMhV1gaVl/IhcTOl3hP3gpmomZUXdFwDk5ZIjNCu7T X-Received: by 2002:a05:6808:d54:b0:3b8:456a:d4ba with SMTP id w20-20020a0568080d5400b003b8456ad4bamr2108701oik.44.1701411931141; Thu, 30 Nov 2023 22:25:31 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1701411931; cv=none; d=google.com; s=arc-20160816; b=hGZ7p5NY6wbpZcv9TWAAjTKxGD+HvXWT7dyXWlDIKGtB/gjct3Kpy5oZPMmQj7++aP hTGLvZaHbXPsODWfp8rKA6o+BjHbALB9V1G/9My2wKe8MuaBTbP3JPA29ZB8TR4zdgeA L8AHxDVFt3a/MH9lhe4GDPtpBKegoj9WkC4y0JAO8XTyLxvb11b8ClkBtt5z8xrVxCXZ luwSR356iJ+Y6unZeMxB+son3F1q6e6XpCYpd5KzWwJKLF0TLY9Gy8t8BGCbYnc3QDjL 83HBrB81ZR7SZ/wlSS/btuoq/QCEFi+4CCrgn9PbhtFlQeeurOPXgk3rmGvb+4VePKFp lrhw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from :dkim-signature; bh=k/lYEBDVstmevWVzRIxp7/MOhRPDiH4lSP/sGsfsX1k=; fh=YTkuaskr0lgfdZdN3raKefSjBLb6uuCywA/U9wJpjjU=; b=ofsK9/hm0SBmHIrZsZEeIXFElBkk9Oh1toNQzQoo6x8zXDK7PC3vLmhRoOEkUmFC04 dmj2VpWA3OsrAqLxI+JG/AhW0NJnD+ud9Li1D7UmBL5/+jcTF62CMmJjU0oC7XfA6fHS 4BOMkkNgHGZChp+DW85ANdmV3+MMfqPxXlMvYZlqNwb5ztzNIJMzq8ac59qFMvquZPVC RPyjqr0mSnzEW5d4tezUHdOEAPRuGMoYMjRCkcezJseAdNXXkci6MKC6GGWNhWpVYTQG lnC5OShZ0yLxVUFXQoxZ/dH07j+8eFzSC1EatYd7weossY6K6vO1HZWhcEhy1lhkpBMQ M7fQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@intel.com header.s=Intel header.b=MaJgUXva; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:2 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=intel.com Received: from agentk.vger.email (agentk.vger.email. [2620:137:e000::3:2]) by mx.google.com with ESMTPS id s9-20020a656449000000b005be1ee5ef0dsi2943533pgv.15.2023.11.30.22.25.30 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 30 Nov 2023 22:25:31 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:2 as permitted sender) client-ip=2620:137:e000::3:2; Authentication-Results: mx.google.com; dkim=pass header.i=@intel.com header.s=Intel header.b=MaJgUXva; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:2 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=intel.com Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by agentk.vger.email (Postfix) with ESMTP id 33F3B8055742; Thu, 30 Nov 2023 22:25:27 -0800 (PST) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.11 at agentk.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229830AbjLAGZS (ORCPT <rfc822;ruipengqi7@gmail.com> + 99 others); Fri, 1 Dec 2023 01:25:18 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:41786 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229506AbjLAGZR (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Fri, 1 Dec 2023 01:25:17 -0500 Received: from mgamail.intel.com (mgamail.intel.com [198.175.65.10]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id C0585171B; Thu, 30 Nov 2023 22:25:23 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1701411924; x=1732947924; h=from:to:cc:subject:date:message-id:in-reply-to: references:mime-version:content-transfer-encoding; bh=JMS58PGHOZBnURZ97D6TgdTtnRy5SWvzM7nhRzWNkfo=; b=MaJgUXva9Y+9dSYYZOiqNmIWvTAuHOEnht0Gn5SoMAWOG2Pal3x0c5yB GdOiyzR4QyHtZhsjVCSvIoCFp5xrHH6hD1XGwV0q384QHSBirJQ6Ovlz/ 7aPWpNFp9dHoro32L8KeuENHqDGeiHArZqmfpsa4n4C6VxTYCaAZu5qyx nmBjKmLp/tfiBKdrFjIdPgELDVP64pLVX6wltNZeLZHKCucSm+7o4gDDV 8CaEJ9ORj7XrQ4CgEF6rZry2/tJLHMw8veT4O3KFlk1hunD/P3yGFoZsf 6d1M8H3dNvwVUTsaIuR0Xk9l+TQoyD4T18UQ7zPJqQnC3NqhoFl+A1yuS g==; X-IronPort-AV: E=McAfee;i="6600,9927,10910"; a="6722936" X-IronPort-AV: E=Sophos;i="6.04,241,1695711600"; d="scan'208";a="6722936" Received: from orsmga001.jf.intel.com ([10.7.209.18]) by orvoesa102.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 30 Nov 2023 22:25:18 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10910"; a="803945100" X-IronPort-AV: E=Sophos;i="6.04,241,1695711600"; d="scan'208";a="803945100" Received: from p12ill20yoongsia.png.intel.com ([10.88.227.28]) by orsmga001.jf.intel.com with ESMTP; 30 Nov 2023 22:25:06 -0800 From: Song Yoong Siang <yoong.siang.song@intel.com> To: "David S . Miller" <davem@davemloft.net>, Eric Dumazet <edumazet@google.com>, Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>, Jonathan Corbet <corbet@lwn.net>, Bjorn Topel <bjorn@kernel.org>, Magnus Karlsson <magnus.karlsson@intel.com>, Maciej Fijalkowski <maciej.fijalkowski@intel.com>, Jonathan Lemon <jonathan.lemon@gmail.com>, Alexei Starovoitov <ast@kernel.org>, Daniel Borkmann <daniel@iogearbox.net>, Jesper Dangaard Brouer <hawk@kernel.org>, John Fastabend <john.fastabend@gmail.com>, Stanislav Fomichev <sdf@google.com>, Lorenzo Bianconi <lorenzo@kernel.org>, Tariq Toukan <tariqt@nvidia.com>, Willem de Bruijn <willemb@google.com>, Maxime Coquelin <mcoquelin.stm32@gmail.com>, Andrii Nakryiko <andrii@kernel.org>, Mykola Lysenko <mykolal@fb.com>, Martin KaFai Lau <martin.lau@linux.dev>, Song Liu <song@kernel.org>, Yonghong Song <yonghong.song@linux.dev>, KP Singh <kpsingh@kernel.org>, Hao Luo <haoluo@google.com>, Jiri Olsa <jolsa@kernel.org>, Shuah Khan <shuah@kernel.org>, Alexandre Torgue <alexandre.torgue@foss.st.com>, Jose Abreu <joabreu@synopsys.com> Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org, bpf@vger.kernel.org, xdp-hints@xdp-project.net, linux-stm32@st-md-mailman.stormreply.com, linux-arm-kernel@lists.infradead.org, linux-kselftest@vger.kernel.org, Song Yoong Siang <yoong.siang.song@intel.com> Subject: [PATCH bpf-next v2 2/3] net: stmmac: Add txtime support to XDP ZC Date: Fri, 1 Dec 2023 14:24:20 +0800 Message-Id: <20231201062421.1074768-3-yoong.siang.song@intel.com> X-Mailer: git-send-email 2.34.1 In-Reply-To: <20231201062421.1074768-1-yoong.siang.song@intel.com> References: <20231201062421.1074768-1-yoong.siang.song@intel.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on agentk.vger.email Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (agentk.vger.email [0.0.0.0]); Thu, 30 Nov 2023 22:25:27 -0800 (PST) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1784059716891343981 X-GMAIL-MSGID: 1784059716891343981 |
Series |
xsk: TX metadata txtime support
|
|
Commit Message
Song Yoong Siang
Dec. 1, 2023, 6:24 a.m. UTC
This patch enables txtime support to XDP zero copy via XDP Tx
metadata framework.
Signed-off-by: Song Yoong Siang <yoong.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(+)
Comments
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@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
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@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 Good point Jesper. Can we also explicitly document what the type of the field is? Nanoseconds against the NIC hardware clock, it sounds like. We have some experience with this, too. Something needs to do the conversion from host clock to NIC clock. It is not sufficent to just assume that the host clock is synced against the NIC clock by PTP.
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@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?
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@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. 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. See tools/testing/selftests/net/so_txtime.sh for how the FQ and ETF qdiscs already disagree on the clock source that they use.
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@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 >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. >
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@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.
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@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. I see. Thank for the explanation. I will try to add clock source arguments In next version.
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h index 686c94c2e8a7..e8538af6e207 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h @@ -105,6 +105,8 @@ struct stmmac_metadata_request { struct stmmac_priv *priv; struct dma_desc *tx_desc; bool *set_ic; + struct dma_edesc *edesc; + int tbs; }; struct stmmac_xsk_tx_complete { diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c index c2ac88aaffed..c7b9338be9e0 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c @@ -2465,9 +2465,20 @@ static u64 stmmac_xsk_fill_timestamp(void *_priv) return 0; } +static void stmmac_xsk_request_txtime(u64 txtime, void *_priv) +{ + struct stmmac_metadata_request *meta_req = _priv; + struct timespec64 ts = ns_to_timespec64(txtime); + + if (meta_req->tbs & STMMAC_TBS_EN) + stmmac_set_desc_tbs(meta_req->priv, meta_req->edesc, ts.tv_sec, + ts.tv_nsec); +} + static const struct xsk_tx_metadata_ops stmmac_xsk_tx_metadata_ops = { .tmo_request_timestamp = stmmac_xsk_request_timestamp, .tmo_fill_timestamp = stmmac_xsk_fill_timestamp, + .tmo_request_txtime = stmmac_xsk_request_txtime, }; static bool stmmac_xdp_xmit_zc(struct stmmac_priv *priv, u32 queue, u32 budget) @@ -2545,6 +2556,8 @@ static bool stmmac_xdp_xmit_zc(struct stmmac_priv *priv, u32 queue, u32 budget) meta_req.priv = priv; meta_req.tx_desc = tx_desc; meta_req.set_ic = &set_ic; + meta_req.tbs = tx_q->tbs; + meta_req.edesc = &tx_q->dma_entx[entry]; xsk_tx_metadata_request(meta, &stmmac_xsk_tx_metadata_ops, &meta_req); if (set_ic) {