Message ID | 20230410100939.331833-4-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:b0ea:0:b0:3b6:4342:cba0 with SMTP id b10csp1796929vqo; Mon, 10 Apr 2023 03:21:28 -0700 (PDT) X-Google-Smtp-Source: AKy350YkdiefNsv4Nuuon1SPZQGNBnjZ5vL0dThNy+c14/qElSOT61ArkiVu1Xv791i+t2TFVLc0 X-Received: by 2002:a17:903:294c:b0:1a2:8924:2259 with SMTP id li12-20020a170903294c00b001a289242259mr11460206plb.23.1681122088656; Mon, 10 Apr 2023 03:21:28 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1681122088; cv=none; d=google.com; s=arc-20160816; b=GYKnAdFFe97FO00V42gwkueWH5UECHAwQXNczMeCPEV7Ih8Yk1Xhh0NWDGsriyHb+l atl4PegIBRkSmb1wYv5ooh9W6iJLYDSeNZPSySbF2nYNgtJHu0OrbhT74YtIyOoycY+R W39qJDIMkFw/WLt3ei+Ige8lyGgC9DUBDPKKbw6oOpYsw9xe7iWIcZnOVX1MqkwzwUUx g8fzmakgRKxp7ronRfo64JrSo4H0ORUCcqe+kuZcZnBVTfPVRzMdI3GG8BnZCP2Ihueb KnOzAFSAGPAy9NRC4E1dWRWo+E9ZPAzl6n6mPxLWERYNlg+5gpIuFExOVIkY7ygALEXV 1igw== 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=vq1f7P7DFp7BL6iQSdzbtfMFoWa8u99Dl1A1Ob9d3vk=; b=HcXaQuMtsiSTYkYqdA+76fR58XRFySjoXj0D/KRXBVbN0h+o6qVLAKs9H1LGUamPYq 17lRb7Snjh1WBQpI+DeZ+pbV5My0jw/iwYQ8g8wkAb7Fff4+xwv7GlLvFZ3SFvbqcjVb Enm5pZ6MZqVzDMIIlsbxgCX2L98Y3llCwgZizEJiScQVL7ovBuXCnuwg3qQY6vDuhIEc KDtsIy99Bs+2rXanf49KjnXCC1feUGx+wWffaWjOm4/MW+rsowsmlVFiYuAKtkKcYuQr cr6HsZUfBytF9vvje9geAh2h8XLMnp2Vgv7iddFhZ6DAyr1NsbjTpeSgHcPGSc2vE/Qn 2efw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@intel.com header.s=Intel header.b=MigQtS8k; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 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 (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id o17-20020a170902d4d100b001a1bbc5bea5si11439209plg.537.2023.04.10.03.21.16; Mon, 10 Apr 2023 03:21:28 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@intel.com header.s=Intel header.b=MigQtS8k; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=intel.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229661AbjDJKLf (ORCPT <rfc822;a1648639935@gmail.com> + 99 others); Mon, 10 Apr 2023 06:11:35 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:56408 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229630AbjDJKLd (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Mon, 10 Apr 2023 06:11:33 -0400 Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 706246197; Mon, 10 Apr 2023 03:11:12 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1681121472; x=1712657472; h=from:to:cc:subject:date:message-id:in-reply-to: references:mime-version:content-transfer-encoding; bh=CnnmBEFT4Gdze5o43Pjrwyz8lLC6ClCHyehlDppYg+k=; b=MigQtS8kRNslOWGZc3xAYSWAlsBXzQCIwqTm8Vk6C93ESjUviP+1siuN 2iqt3OHmBgRSGfg+Ar+VaywmYL6whkPB/nVB3BDLgdskCc1+kxlvvlSSj 86+xmoZl6p8mcln8E8BOn+K0PIS9fG0JhWh1v2wAuG6UFjIEUmJzvIt0f ces3Wo8/d3UJCKnMvJ2JhszHe6MlNdvtjWa1AzrgRAGtHXAwAvOf+NIvE wLG7OXcDx1ubP/zmplU/f85cuklYAbGdI5eGs+6rseOAAvKwGotmo+ezq 4ngDjIyC8UKAkhZXZb13VJeyt9Jm6KDYB6bV3MI3x4JYBW+Q2Kzlpnwyf A==; X-IronPort-AV: E=McAfee;i="6600,9927,10675"; a="340815381" X-IronPort-AV: E=Sophos;i="5.98,333,1673942400"; d="scan'208";a="340815381" Received: from fmsmga008.fm.intel.com ([10.253.24.58]) by fmsmga102.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 10 Apr 2023 03:10:52 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10675"; a="752716162" X-IronPort-AV: E=Sophos;i="5.98,333,1673942400"; d="scan'208";a="752716162" Received: from p12ill20yoongsia.png.intel.com ([10.88.227.28]) by fmsmga008.fm.intel.com with ESMTP; 10 Apr 2023 03:10:47 -0700 From: Song Yoong Siang <yoong.siang.song@intel.com> To: Giuseppe Cavallaro <peppe.cavallaro@st.com>, Alexandre Torgue <alexandre.torgue@foss.st.com>, Jose Abreu <joabreu@synopsys.com>, "David S . Miller" <davem@davemloft.net>, Eric Dumazet <edumazet@google.com>, Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>, Maxime Coquelin <mcoquelin.stm32@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>, Alexander Duyck <alexanderduyck@fb.com>, Ong Boon Leong <boon.leong.ong@intel.com> Cc: netdev@vger.kernel.org, linux-stm32@st-md-mailman.stormreply.com, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, bpf@vger.kernel.org, xdp-hints@xdp-project.net, Song Yoong Siang <yoong.siang.song@intel.com> Subject: [PATCH net-next 3/4] net: stmmac: add Rx HWTS metadata to XDP receive pkt Date: Mon, 10 Apr 2023 18:09:38 +0800 Message-Id: <20230410100939.331833-4-yoong.siang.song@intel.com> X-Mailer: git-send-email 2.34.1 In-Reply-To: <20230410100939.331833-1-yoong.siang.song@intel.com> References: <20230410100939.331833-1-yoong.siang.song@intel.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-2.5 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,SPF_HELO_NONE, SPF_NONE autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1762784275035992873?= X-GMAIL-MSGID: =?utf-8?q?1762784275035992873?= |
Series |
XDP Rx HWTS metadata for stmmac driver
|
|
Commit Message
Song Yoong Siang
April 10, 2023, 10:09 a.m. UTC
Add receive hardware timestamp metadata support via kfunc to XDP receive
packets.
Signed-off-by: Song Yoong Siang <yoong.siang.song@intel.com>
---
drivers/net/ethernet/stmicro/stmmac/stmmac.h | 1 +
.../net/ethernet/stmicro/stmmac/stmmac_main.c | 24 +++++++++++++++++--
2 files changed, 23 insertions(+), 2 deletions(-)
Comments
On Mon, Apr 10, 2023 at 06:09:38PM +0800, Song Yoong Siang wrote: > Add receive hardware timestamp metadata support via kfunc to XDP receive > packets. > > Signed-off-by: Song Yoong Siang <yoong.siang.song@intel.com> ... > @@ -7071,6 +7073,22 @@ void stmmac_fpe_handshake(struct stmmac_priv *priv, bool enable) > } > } > > +static int stmmac_xdp_rx_timestamp(const struct xdp_md *_ctx, u64 *timestamp) > +{ > + const struct stmmac_xdp_buff *ctx = (void *)_ctx; > + > + if (ctx->rx_hwts) { > + *timestamp = ctx->rx_hwts; > + return 0; > + } > + > + return -ENODATA; > +} > + > +const struct xdp_metadata_ops stmmac_xdp_metadata_ops = { > + .xmo_rx_timestamp = stmmac_xdp_rx_timestamp, > +}; sparse seems to think this should be static. drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:7082:31: warning: symbol 'stmmac_xdp_metadata_ops' was not declared. Should it be static? Link: https://patchwork.kernel.org/project/netdevbpf/patch/20230410100939.331833-4-yoong.siang.song@intel.com/ > + > /** > * stmmac_dvr_probe > * @device: device pointer
>On Mon, Apr 10, 2023 at 06:09:38PM +0800, Song Yoong Siang wrote: >> Add receive hardware timestamp metadata support via kfunc to XDP >> receive packets. >> >> Signed-off-by: Song Yoong Siang <yoong.siang.song@intel.com> > >... > >> @@ -7071,6 +7073,22 @@ void stmmac_fpe_handshake(struct stmmac_priv >*priv, bool enable) >> } >> } >> >> +static int stmmac_xdp_rx_timestamp(const struct xdp_md *_ctx, u64 >> +*timestamp) { >> + const struct stmmac_xdp_buff *ctx = (void *)_ctx; >> + >> + if (ctx->rx_hwts) { >> + *timestamp = ctx->rx_hwts; >> + return 0; >> + } >> + >> + return -ENODATA; >> +} >> + >> +const struct xdp_metadata_ops stmmac_xdp_metadata_ops = { >> + .xmo_rx_timestamp = stmmac_xdp_rx_timestamp, >> +}; > >sparse seems to think this should be static. > >drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:7082:31: warning: symbol >'stmmac_xdp_metadata_ops' was not declared. Should it be static? Yes, you are right. It should be static. I will add correct it in v2. Thank you. Thanks & Regards Siang > >Link: >https://patchwork.kernel.org/project/netdevbpf/patch/20230410100939.331833 >-4-yoong.siang.song@intel.com/ > >> + >> /** >> * stmmac_dvr_probe >> * @device: device pointer
On 04/10, Song Yoong Siang wrote: > Add receive hardware timestamp metadata support via kfunc to XDP receive > packets. > > Signed-off-by: Song Yoong Siang <yoong.siang.song@intel.com> > --- > drivers/net/ethernet/stmicro/stmmac/stmmac.h | 1 + > .../net/ethernet/stmicro/stmmac/stmmac_main.c | 24 +++++++++++++++++-- > 2 files changed, 23 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h > index ac8ccf851708..760445275da8 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h > @@ -94,6 +94,7 @@ struct stmmac_rx_buffer { > > struct stmmac_xdp_buff { > struct xdp_buff xdp; > + ktime_t rx_hwts; > }; > > struct stmmac_rx_queue { > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > index f7bbdf04d20c..ca183fbfde85 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > @@ -5307,6 +5307,8 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue) > } > } > [..] > + stmmac_get_rx_hwtstamp(priv, p, np, &ctx.rx_hwts); Do we want to pay this cost for every packet? The preferred alternative is to store enough state in the stmmac_xdp_buff so we can get to this data from stmmac_xdp_rx_timestamp. I haven't read this code, but tentatively: - move priv, p, np into stmmac_xdp_buff, assign them here instead of calling stmmac_get_rx_hwtstamp - call stmmac_get_rx_hwtstamp from stmmac_xdp_rx_timestamp with the stored priv, p, np That would ensure that we won't waste the cycles pulling out the rx timestamp for every packet if the higher levels / users don't care. Would something like this work? > + > if (!skb) { > unsigned int pre_len, sync_len; > > @@ -5315,7 +5317,7 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue) > > xdp_init_buff(&ctx.xdp, buf_sz, &rx_q->xdp_rxq); > xdp_prepare_buff(&ctx.xdp, page_address(buf->page), > - buf->page_offset, buf1_len, false); > + buf->page_offset, buf1_len, true); > > pre_len = ctx.xdp.data_end - ctx.xdp.data_hard_start - > buf->page_offset; > @@ -5411,7 +5413,7 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue) > > shhwtstamp = skb_hwtstamps(skb); > memset(shhwtstamp, 0, sizeof(struct skb_shared_hwtstamps)); > - stmmac_get_rx_hwtstamp(priv, p, np, &shhwtstamp->hwtstamp); > + shhwtstamp->hwtstamp = ctx.rx_hwts; > > stmmac_rx_vlan(priv->dev, skb); > skb->protocol = eth_type_trans(skb, priv->dev); > @@ -7071,6 +7073,22 @@ void stmmac_fpe_handshake(struct stmmac_priv *priv, bool enable) > } > } > > +static int stmmac_xdp_rx_timestamp(const struct xdp_md *_ctx, u64 *timestamp) > +{ > + const struct stmmac_xdp_buff *ctx = (void *)_ctx; > + > + if (ctx->rx_hwts) { > + *timestamp = ctx->rx_hwts; > + return 0; > + } > + > + return -ENODATA; > +} > + > +const struct xdp_metadata_ops stmmac_xdp_metadata_ops = { > + .xmo_rx_timestamp = stmmac_xdp_rx_timestamp, > +}; > + > /** > * stmmac_dvr_probe > * @device: device pointer > @@ -7178,6 +7196,8 @@ int stmmac_dvr_probe(struct device *device, > > ndev->netdev_ops = &stmmac_netdev_ops; > > + ndev->xdp_metadata_ops = &stmmac_xdp_metadata_ops; > + > ndev->hw_features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM | > NETIF_F_RXCSUM; > ndev->xdp_features = NETDEV_XDP_ACT_BASIC | NETDEV_XDP_ACT_REDIRECT | > -- > 2.34.1 >
On Mon, Apr 10, 2023 at 03:33:48PM +0000, Song, Yoong Siang wrote: > >On Mon, Apr 10, 2023 at 06:09:38PM +0800, Song Yoong Siang wrote: > >> Add receive hardware timestamp metadata support via kfunc to XDP > >> receive packets. > >> > >> Signed-off-by: Song Yoong Siang <yoong.siang.song@intel.com> > > > >... > > > >> @@ -7071,6 +7073,22 @@ void stmmac_fpe_handshake(struct stmmac_priv > >*priv, bool enable) > >> } > >> } > >> > >> +static int stmmac_xdp_rx_timestamp(const struct xdp_md *_ctx, u64 > >> +*timestamp) { > >> + const struct stmmac_xdp_buff *ctx = (void *)_ctx; > >> + > >> + if (ctx->rx_hwts) { > >> + *timestamp = ctx->rx_hwts; > >> + return 0; > >> + } > >> + > >> + return -ENODATA; > >> +} > >> + > >> +const struct xdp_metadata_ops stmmac_xdp_metadata_ops = { > >> + .xmo_rx_timestamp = stmmac_xdp_rx_timestamp, > >> +}; > > > >sparse seems to think this should be static. > > > >drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:7082:31: warning: symbol > >'stmmac_xdp_metadata_ops' was not declared. Should it be static? > Yes, you are right. It should be static. I will add correct it in v2. Thank you. Thanks
On Tuesday, April 11, 2023 12:25 AM, Stanislav Fomichev <sdf@google.com> wrote: >On 04/10, Song Yoong Siang wrote: >> Add receive hardware timestamp metadata support via kfunc to XDP >> receive packets. >> >> Signed-off-by: Song Yoong Siang <yoong.siang.song@intel.com> >> --- >> drivers/net/ethernet/stmicro/stmmac/stmmac.h | 1 + >> .../net/ethernet/stmicro/stmmac/stmmac_main.c | 24 +++++++++++++++++-- >> 2 files changed, 23 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h >> b/drivers/net/ethernet/stmicro/stmmac/stmmac.h >> index ac8ccf851708..760445275da8 100644 >> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h >> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h >> @@ -94,6 +94,7 @@ struct stmmac_rx_buffer { >> >> struct stmmac_xdp_buff { >> struct xdp_buff xdp; >> + ktime_t rx_hwts; >> }; >> >> struct stmmac_rx_queue { >> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c >> b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c >> index f7bbdf04d20c..ca183fbfde85 100644 >> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c >> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c >> @@ -5307,6 +5307,8 @@ static int stmmac_rx(struct stmmac_priv *priv, int >limit, u32 queue) >> } >> } >> > >[..] > >> + stmmac_get_rx_hwtstamp(priv, p, np, &ctx.rx_hwts); > >Do we want to pay this cost for every packet? > >The preferred alternative is to store enough state in the stmmac_xdp_buff so we >can get to this data from stmmac_xdp_rx_timestamp. > >I haven't read this code, but tentatively: >- move priv, p, np into stmmac_xdp_buff, assign them here instead of > calling stmmac_get_rx_hwtstamp >- call stmmac_get_rx_hwtstamp from stmmac_xdp_rx_timestamp with the > stored priv, p, np > >That would ensure that we won't waste the cycles pulling out the rx timestamp >for every packet if the higher levels / users don't care. > >Would something like this work? Hi Stanislav Fomichev, Thanks for your comments. Original stmmac_rx() function is already calling stmmac_get_rx_hwtstamp() for every packet. This patch move the calling of stmmac_get_rx_hwtstamp() earlier so that rx timestamp is available before running bpf_prog_run_xdp(). So, i think no additional cost introduced here. Any other thoughts? Furthermore, stmmac_get_rx_hwtstamp() will check whether hw timestamp is enabled in driver and available in the descriptor before getting the hw timestamp. > >> + >> if (!skb) { >> unsigned int pre_len, sync_len; >> >> @@ -5315,7 +5317,7 @@ static int stmmac_rx(struct stmmac_priv *priv, >> int limit, u32 queue) >> >> xdp_init_buff(&ctx.xdp, buf_sz, &rx_q->xdp_rxq); >> xdp_prepare_buff(&ctx.xdp, page_address(buf->page), >> - buf->page_offset, buf1_len, false); >> + buf->page_offset, buf1_len, true); >> >> pre_len = ctx.xdp.data_end - ctx.xdp.data_hard_start - >> buf->page_offset; >> @@ -5411,7 +5413,7 @@ static int stmmac_rx(struct stmmac_priv *priv, >> int limit, u32 queue) >> >> shhwtstamp = skb_hwtstamps(skb); >> memset(shhwtstamp, 0, sizeof(struct skb_shared_hwtstamps)); >> - stmmac_get_rx_hwtstamp(priv, p, np, &shhwtstamp->hwtstamp); Original stmmac_get_rx_hwtstamp() function is called here. Thanks & Regards Siang >> + shhwtstamp->hwtstamp = ctx.rx_hwts; >> >> stmmac_rx_vlan(priv->dev, skb); >> skb->protocol = eth_type_trans(skb, priv->dev); @@ -7071,6 >+7073,22 >> @@ void stmmac_fpe_handshake(struct stmmac_priv *priv, bool enable) >> } >> } >> >> +static int stmmac_xdp_rx_timestamp(const struct xdp_md *_ctx, u64 >> +*timestamp) { >> + const struct stmmac_xdp_buff *ctx = (void *)_ctx; >> + >> + if (ctx->rx_hwts) { >> + *timestamp = ctx->rx_hwts; >> + return 0; >> + } >> + >> + return -ENODATA; >> +} >> + >> +const struct xdp_metadata_ops stmmac_xdp_metadata_ops = { >> + .xmo_rx_timestamp = stmmac_xdp_rx_timestamp, >> +}; >> + >> /** >> * stmmac_dvr_probe >> * @device: device pointer >> @@ -7178,6 +7196,8 @@ int stmmac_dvr_probe(struct device *device, >> >> ndev->netdev_ops = &stmmac_netdev_ops; >> >> + ndev->xdp_metadata_ops = &stmmac_xdp_metadata_ops; >> + >> ndev->hw_features = NETIF_F_SG | NETIF_F_IP_CSUM | >NETIF_F_IPV6_CSUM | >> NETIF_F_RXCSUM; >> ndev->xdp_features = NETDEV_XDP_ACT_BASIC | >NETDEV_XDP_ACT_REDIRECT >> | >> -- >> 2.34.1 >>
On Wednesday, April 12, 2023 9:31 AM, Song Yoong Siang <yoong.siang.song@intel.com> wrote: >On Tuesday, April 11, 2023 12:25 AM, Stanislav Fomichev <sdf@google.com> >wrote: >>On 04/10, Song Yoong Siang wrote: >>> Add receive hardware timestamp metadata support via kfunc to XDP >>> receive packets. >>> >>> Signed-off-by: Song Yoong Siang <yoong.siang.song@intel.com> >>> --- >>> drivers/net/ethernet/stmicro/stmmac/stmmac.h | 1 + >>> .../net/ethernet/stmicro/stmmac/stmmac_main.c | 24 >>> +++++++++++++++++-- >>> 2 files changed, 23 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h >>> b/drivers/net/ethernet/stmicro/stmmac/stmmac.h >>> index ac8ccf851708..760445275da8 100644 >>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h >>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h >>> @@ -94,6 +94,7 @@ struct stmmac_rx_buffer { >>> >>> struct stmmac_xdp_buff { >>> struct xdp_buff xdp; >>> + ktime_t rx_hwts; >>> }; >>> >>> struct stmmac_rx_queue { >>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c >>> b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c >>> index f7bbdf04d20c..ca183fbfde85 100644 >>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c >>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c >>> @@ -5307,6 +5307,8 @@ static int stmmac_rx(struct stmmac_priv *priv, >>> int >>limit, u32 queue) >>> } >>> } >>> >> >>[..] >> >>> + stmmac_get_rx_hwtstamp(priv, p, np, &ctx.rx_hwts); >> >>Do we want to pay this cost for every packet? >> >>The preferred alternative is to store enough state in the >>stmmac_xdp_buff so we can get to this data from stmmac_xdp_rx_timestamp. >> >>I haven't read this code, but tentatively: >>- move priv, p, np into stmmac_xdp_buff, assign them here instead of >> calling stmmac_get_rx_hwtstamp >>- call stmmac_get_rx_hwtstamp from stmmac_xdp_rx_timestamp with the >> stored priv, p, np >> >>That would ensure that we won't waste the cycles pulling out the rx >>timestamp for every packet if the higher levels / users don't care. >> >>Would something like this work? > >Hi Stanislav Fomichev, > >Thanks for your comments. > >Original stmmac_rx() function is already calling stmmac_get_rx_hwtstamp() for >every packet. This patch move the calling of stmmac_get_rx_hwtstamp() earlier >so that rx timestamp is available before running bpf_prog_run_xdp(). So, i think >no additional cost introduced here. Any other thoughts? > >Furthermore, stmmac_get_rx_hwtstamp() will check whether hw timestamp is >enabled in driver and available in the descriptor before getting the hw timestamp. > Hi Stanislav Fomichev, I think twice. It might add some latency for certain verdict if the hw timestamp is enabled but the user app dint need it. I will take your suggestion and try to pull the timestamp per need basic. Will submit v3 soon. Thanks & Regards Siang >> >>> + >>> if (!skb) { >>> unsigned int pre_len, sync_len; >>> >>> @@ -5315,7 +5317,7 @@ static int stmmac_rx(struct stmmac_priv *priv, >>> int limit, u32 queue) >>> >>> xdp_init_buff(&ctx.xdp, buf_sz, &rx_q->xdp_rxq); >>> xdp_prepare_buff(&ctx.xdp, page_address(buf->page), >>> - buf->page_offset, buf1_len, false); >>> + buf->page_offset, buf1_len, true); >>> >>> pre_len = ctx.xdp.data_end - ctx.xdp.data_hard_start - >>> buf->page_offset; >>> @@ -5411,7 +5413,7 @@ static int stmmac_rx(struct stmmac_priv *priv, >>> int limit, u32 queue) >>> >>> shhwtstamp = skb_hwtstamps(skb); >>> memset(shhwtstamp, 0, sizeof(struct skb_shared_hwtstamps)); >>> - stmmac_get_rx_hwtstamp(priv, p, np, &shhwtstamp->hwtstamp); > >Original stmmac_get_rx_hwtstamp() function is called here. > >Thanks & Regards >Siang > >>> + shhwtstamp->hwtstamp = ctx.rx_hwts; >>> >>> stmmac_rx_vlan(priv->dev, skb); >>> skb->protocol = eth_type_trans(skb, priv->dev); @@ -7071,6 >>+7073,22 >>> @@ void stmmac_fpe_handshake(struct stmmac_priv *priv, bool enable) >>> } >>> } >>> >>> +static int stmmac_xdp_rx_timestamp(const struct xdp_md *_ctx, u64 >>> +*timestamp) { >>> + const struct stmmac_xdp_buff *ctx = (void *)_ctx; >>> + >>> + if (ctx->rx_hwts) { >>> + *timestamp = ctx->rx_hwts; >>> + return 0; >>> + } >>> + >>> + return -ENODATA; >>> +} >>> + >>> +const struct xdp_metadata_ops stmmac_xdp_metadata_ops = { >>> + .xmo_rx_timestamp = stmmac_xdp_rx_timestamp, >>> +}; >>> + >>> /** >>> * stmmac_dvr_probe >>> * @device: device pointer >>> @@ -7178,6 +7196,8 @@ int stmmac_dvr_probe(struct device *device, >>> >>> ndev->netdev_ops = &stmmac_netdev_ops; >>> >>> + ndev->xdp_metadata_ops = &stmmac_xdp_metadata_ops; >>> + >>> ndev->hw_features = NETIF_F_SG | NETIF_F_IP_CSUM | >>NETIF_F_IPV6_CSUM | >>> NETIF_F_RXCSUM; >>> ndev->xdp_features = NETDEV_XDP_ACT_BASIC | >>NETDEV_XDP_ACT_REDIRECT >>> | >>> -- >>> 2.34.1 >>>
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h index ac8ccf851708..760445275da8 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h @@ -94,6 +94,7 @@ struct stmmac_rx_buffer { struct stmmac_xdp_buff { struct xdp_buff xdp; + ktime_t rx_hwts; }; struct stmmac_rx_queue { diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c index f7bbdf04d20c..ca183fbfde85 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c @@ -5307,6 +5307,8 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue) } } + stmmac_get_rx_hwtstamp(priv, p, np, &ctx.rx_hwts); + if (!skb) { unsigned int pre_len, sync_len; @@ -5315,7 +5317,7 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue) xdp_init_buff(&ctx.xdp, buf_sz, &rx_q->xdp_rxq); xdp_prepare_buff(&ctx.xdp, page_address(buf->page), - buf->page_offset, buf1_len, false); + buf->page_offset, buf1_len, true); pre_len = ctx.xdp.data_end - ctx.xdp.data_hard_start - buf->page_offset; @@ -5411,7 +5413,7 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue) shhwtstamp = skb_hwtstamps(skb); memset(shhwtstamp, 0, sizeof(struct skb_shared_hwtstamps)); - stmmac_get_rx_hwtstamp(priv, p, np, &shhwtstamp->hwtstamp); + shhwtstamp->hwtstamp = ctx.rx_hwts; stmmac_rx_vlan(priv->dev, skb); skb->protocol = eth_type_trans(skb, priv->dev); @@ -7071,6 +7073,22 @@ void stmmac_fpe_handshake(struct stmmac_priv *priv, bool enable) } } +static int stmmac_xdp_rx_timestamp(const struct xdp_md *_ctx, u64 *timestamp) +{ + const struct stmmac_xdp_buff *ctx = (void *)_ctx; + + if (ctx->rx_hwts) { + *timestamp = ctx->rx_hwts; + return 0; + } + + return -ENODATA; +} + +const struct xdp_metadata_ops stmmac_xdp_metadata_ops = { + .xmo_rx_timestamp = stmmac_xdp_rx_timestamp, +}; + /** * stmmac_dvr_probe * @device: device pointer @@ -7178,6 +7196,8 @@ int stmmac_dvr_probe(struct device *device, ndev->netdev_ops = &stmmac_netdev_ops; + ndev->xdp_metadata_ops = &stmmac_xdp_metadata_ops; + ndev->hw_features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM | NETIF_F_RXCSUM; ndev->xdp_features = NETDEV_XDP_ACT_BASIC | NETDEV_XDP_ACT_REDIRECT |