[xdp-hints] [PATCH net-next 0/4] XDP Rx HWTS metadata for stmmac driver

[xdp-hints] [PATCH net-next 0/4] XDP Rx HWTS metadata for stmmac driver

[Song Yoong Siang]

Implemented XDP receive hardware timestamp metadata for stmmac driver.

This patchset is tested with tools/testing/selftests/bpf/xdp_hw_metadata.
Below are the test steps and results.

Command on DUT:
	sudo ./xdp_hw_metadata <interface name>

Command on Link Partner:
	echo -n xdp | nc -u -q1 <destination IPv4 addr> 9091
	echo -n skb | nc -u -q1 <destination IPv4 addr> 9092

Result for port 9091:
	0x55fdb5f006d0: rx_desc[3]->addr=1000000003bd000 addr=3bd100 comp_addr=3bd000
	rx_timestamp: 1677762474360150047
	rx_hash: 0
	0x55fdb5f006d0: complete idx=515 addr=3bd000

Result for port 9092:
	found skb hwtstamp = 1677762476.320146161

---

Ong Boon Leong (1):
  net: stmmac: restructure Rx hardware timestamping function

Song Yoong Siang (3):
  net: stmmac: introduce wrapper for struct xdp_buff
  net: stmmac: add Rx HWTS metadata to XDP receive pkt
  net: stmmac: add Rx HWTS metadata to XDP ZC receive pkt

 drivers/net/ethernet/stmicro/stmmac/stmmac.h  |  5 ++
 .../net/ethernet/stmicro/stmmac/stmmac_main.c | 88 ++++++++++++++-----
 2 files changed, 73 insertions(+), 20 deletions(-)

-- 
2.34.1

[xdp-hints] [PATCH net-next 1/4] net: stmmac: restructure Rx hardware timestamping function

[Song Yoong Siang]

From: Ong Boon Leong <boon.leong.ong@intel.com>

Rearrange the function of getting Rx hardware timestamp for skb so
that it can be reused for XDP later.

Signed-off-by: Ong Boon Leong <boon.leong.ong@intel.com>
Signed-off-by: Song Yoong Siang <yoong.siang.song@intel.com>
---
 .../net/ethernet/stmicro/stmmac/stmmac_main.c | 21 ++++++++++++-------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 04fbb7770618..2cc6237a9c28 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -570,15 +570,14 @@ static void stmmac_get_tx_hwtstamp(struct stmmac_priv *priv,
  * @priv: driver private structure
  * @p : descriptor pointer
  * @np : next descriptor pointer
- * @skb : the socket buffer
+ * @hwtstamp : hardware timestamp
  * Description :
  * This function will read received packet's timestamp from the descriptor
  * and pass it to stack. It also perform some sanity checks.
  */
 static void stmmac_get_rx_hwtstamp(struct stmmac_priv *priv, struct dma_desc *p,
-				   struct dma_desc *np, struct sk_buff *skb)
+				   struct dma_desc *np, ktime_t *hwtstamp)
 {
-	struct skb_shared_hwtstamps *shhwtstamp = NULL;
 	struct dma_desc *desc = p;
 	u64 ns = 0;
 
@@ -595,9 +594,7 @@ static void stmmac_get_rx_hwtstamp(struct stmmac_priv *priv, struct dma_desc *p,
 		ns -= priv->plat->cdc_error_adj;
 
 		netdev_dbg(priv->dev, "get valid RX hw timestamp %llu\n", ns);
-		shhwtstamp = skb_hwtstamps(skb);
-		memset(shhwtstamp, 0, sizeof(struct skb_shared_hwtstamps));
-		shhwtstamp->hwtstamp = ns_to_ktime(ns);
+		*hwtstamp = ns_to_ktime(ns);
 	} else  {
 		netdev_dbg(priv->dev, "cannot get RX hw timestamp\n");
 	}
@@ -4909,6 +4906,7 @@ static void stmmac_dispatch_skb_zc(struct stmmac_priv *priv, u32 queue,
 				   struct xdp_buff *xdp)
 {
 	struct stmmac_channel *ch = &priv->channel[queue];
+	struct skb_shared_hwtstamps *shhwtstamp = NULL;
 	unsigned int len = xdp->data_end - xdp->data;
 	enum pkt_hash_types hash_type;
 	int coe = priv->hw->rx_csum;
@@ -4921,7 +4919,10 @@ static void stmmac_dispatch_skb_zc(struct stmmac_priv *priv, u32 queue,
 		return;
 	}
 
-	stmmac_get_rx_hwtstamp(priv, p, np, skb);
+	shhwtstamp = skb_hwtstamps(skb);
+	memset(shhwtstamp, 0, sizeof(struct skb_shared_hwtstamps));
+	stmmac_get_rx_hwtstamp(priv, p, np, &shhwtstamp->hwtstamp);
+
 	stmmac_rx_vlan(priv->dev, skb);
 	skb->protocol = eth_type_trans(skb, priv->dev);
 
@@ -5213,6 +5214,7 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
 				    rx_q->dma_rx_phy, desc_size);
 	}
 	while (count < limit) {
+		struct skb_shared_hwtstamps *shhwtstamp = NULL;
 		unsigned int buf1_len = 0, buf2_len = 0;
 		enum pkt_hash_types hash_type;
 		struct stmmac_rx_buffer *buf;
@@ -5407,7 +5409,10 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
 
 		/* Got entire packet into SKB. Finish it. */
 
-		stmmac_get_rx_hwtstamp(priv, p, np, skb);
+		shhwtstamp = skb_hwtstamps(skb);
+		memset(shhwtstamp, 0, sizeof(struct skb_shared_hwtstamps));
+		stmmac_get_rx_hwtstamp(priv, p, np, &shhwtstamp->hwtstamp);
+
 		stmmac_rx_vlan(priv->dev, skb);
 		skb->protocol = eth_type_trans(skb, priv->dev);
 
-- 
2.34.1

[xdp-hints] [PATCH net-next 2/4] net: stmmac: introduce wrapper for struct xdp_buff

[Song Yoong Siang]

Introduce struct stmmac_xdp_buff as a preparation to support XDP Rx
metadata via kfuncs.

Signed-off-by: Song Yoong Siang <yoong.siang.song@intel.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac.h   |  4 ++++
 .../net/ethernet/stmicro/stmmac/stmmac_main.c  | 18 +++++++++---------
 2 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
index 3d15e1e92e18..ac8ccf851708 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
@@ -92,6 +92,10 @@ struct stmmac_rx_buffer {
 	dma_addr_t sec_addr;
 };
 
+struct stmmac_xdp_buff {
+	struct xdp_buff xdp;
+};
+
 struct stmmac_rx_queue {
 	u32 rx_count_frames;
 	u32 queue_index;
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 2cc6237a9c28..f7bbdf04d20c 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -5189,9 +5189,9 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
 	int status = 0, coe = priv->hw->rx_csum;
 	unsigned int next_entry = rx_q->cur_rx;
 	enum dma_data_direction dma_dir;
+	struct stmmac_xdp_buff ctx = {};
 	unsigned int desc_size;
 	struct sk_buff *skb = NULL;
-	struct xdp_buff xdp;
 	int xdp_status = 0;
 	int buf_sz;
 
@@ -5313,17 +5313,17 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
 			dma_sync_single_for_cpu(priv->device, buf->addr,
 						buf1_len, dma_dir);
 
-			xdp_init_buff(&xdp, buf_sz, &rx_q->xdp_rxq);
-			xdp_prepare_buff(&xdp, page_address(buf->page),
+			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);
 
-			pre_len = xdp.data_end - xdp.data_hard_start -
+			pre_len = ctx.xdp.data_end - ctx.xdp.data_hard_start -
 				  buf->page_offset;
-			skb = stmmac_xdp_run_prog(priv, &xdp);
+			skb = stmmac_xdp_run_prog(priv, &ctx.xdp);
 			/* Due xdp_adjust_tail: DMA sync for_device
 			 * cover max len CPU touch
 			 */
-			sync_len = xdp.data_end - xdp.data_hard_start -
+			sync_len = ctx.xdp.data_end - ctx.xdp.data_hard_start -
 				   buf->page_offset;
 			sync_len = max(sync_len, pre_len);
 
@@ -5333,7 +5333,7 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
 
 				if (xdp_res & STMMAC_XDP_CONSUMED) {
 					page_pool_put_page(rx_q->page_pool,
-							   virt_to_head_page(xdp.data),
+							   virt_to_head_page(ctx.xdp.data),
 							   sync_len, true);
 					buf->page = NULL;
 					priv->dev->stats.rx_dropped++;
@@ -5361,7 +5361,7 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
 
 		if (!skb) {
 			/* XDP program may expand or reduce tail */
-			buf1_len = xdp.data_end - xdp.data;
+			buf1_len = ctx.xdp.data_end - ctx.xdp.data;
 
 			skb = napi_alloc_skb(&ch->rx_napi, buf1_len);
 			if (!skb) {
@@ -5371,7 +5371,7 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
 			}
 
 			/* XDP program may adjust header */
-			skb_copy_to_linear_data(skb, xdp.data, buf1_len);
+			skb_copy_to_linear_data(skb, ctx.xdp.data, buf1_len);
 			skb_put(skb, buf1_len);
 
 			/* Data payload copied into SKB, page ready for recycle */
-- 
2.34.1

[xdp-hints] [PATCH net-next 3/4] net: stmmac: add Rx HWTS metadata to XDP receive pkt

[Song Yoong Siang]

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);
+
 		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

[xdp-hints] [PATCH net-next 4/4] net: stmmac: add Rx HWTS metadata to XDP ZC receive pkt

[Song Yoong Siang]

Add receive hardware timestamp metadata support via kfunc to XDP Zero Copy
receive packets.

Signed-off-by: Song Yoong Siang <yoong.siang.song@intel.com>
---
 .../net/ethernet/stmicro/stmmac/stmmac_main.c | 29 +++++++++++++++++--
 1 file changed, 26 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index ca183fbfde85..a4545e9eb8fd 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -1611,6 +1611,12 @@ static int stmmac_alloc_rx_buffers_zc(struct stmmac_priv *priv,
 	struct stmmac_rx_queue *rx_q = &dma_conf->rx_queue[queue];
 	int i;
 
+	/* struct stmmac_xdp_buff is using cb field (maximum size of 24 bytes)
+	 * in struct xdp_buff_xsk to stash driver specific information. Thus,
+	 * use this macro to make sure no size violations.
+	 */
+	XSK_CHECK_PRIV_TYPE(struct stmmac_xdp_buff);
+
 	for (i = 0; i < dma_conf->dma_rx_size; i++) {
 		struct stmmac_rx_buffer *buf;
 		dma_addr_t dma_addr;
@@ -4903,7 +4909,7 @@ static struct sk_buff *stmmac_construct_skb_zc(struct stmmac_channel *ch,
 
 static void stmmac_dispatch_skb_zc(struct stmmac_priv *priv, u32 queue,
 				   struct dma_desc *p, struct dma_desc *np,
-				   struct xdp_buff *xdp)
+				   struct xdp_buff *xdp, ktime_t rx_hwts)
 {
 	struct stmmac_channel *ch = &priv->channel[queue];
 	struct skb_shared_hwtstamps *shhwtstamp = NULL;
@@ -4921,7 +4927,7 @@ static void stmmac_dispatch_skb_zc(struct stmmac_priv *priv, 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 = rx_hwts;
 
 	stmmac_rx_vlan(priv->dev, skb);
 	skb->protocol = eth_type_trans(skb, priv->dev);
@@ -4999,6 +5005,16 @@ static bool stmmac_rx_refill_zc(struct stmmac_priv *priv, u32 queue, u32 budget)
 	return ret;
 }
 
+static struct stmmac_xdp_buff *xsk_buff_to_stmmac_ctx(struct xdp_buff *xdp)
+{
+	/* In XDP zero copy data path, xdp field in struct xdp_buff_xsk is used
+	 * to represent incoming packet, whereas cb field in the same structure
+	 * is used to store driver specific info. Thus, struct stmmac_xdp_buff
+	 * is laid on top of xdp and cb fields of struct xdp_buff_xsk.
+	 */
+	return (struct stmmac_xdp_buff *)xdp;
+}
+
 static int stmmac_rx_zc(struct stmmac_priv *priv, int limit, u32 queue)
 {
 	struct stmmac_rx_queue *rx_q = &priv->dma_conf.rx_queue[queue];
@@ -5028,8 +5044,10 @@ static int stmmac_rx_zc(struct stmmac_priv *priv, int limit, u32 queue)
 	}
 	while (count < limit) {
 		struct stmmac_rx_buffer *buf;
+		struct stmmac_xdp_buff *ctx;
 		unsigned int buf1_len = 0;
 		struct dma_desc *np, *p;
+		ktime_t rx_hwts = 0;
 		int entry;
 		int res;
 
@@ -5113,6 +5131,10 @@ static int stmmac_rx_zc(struct stmmac_priv *priv, int limit, u32 queue)
 			goto read_again;
 		}
 
+		stmmac_get_rx_hwtstamp(priv, p, np, &rx_hwts);
+		ctx = xsk_buff_to_stmmac_ctx(buf->xdp);
+		ctx->rx_hwts = rx_hwts;
+
 		/* XDP ZC Frame only support primary buffers for now */
 		buf1_len = stmmac_rx_buf1_len(priv, p, status, len);
 		len += buf1_len;
@@ -5132,7 +5154,8 @@ static int stmmac_rx_zc(struct stmmac_priv *priv, int limit, u32 queue)
 
 		switch (res) {
 		case STMMAC_XDP_PASS:
-			stmmac_dispatch_skb_zc(priv, queue, p, np, buf->xdp);
+			stmmac_dispatch_skb_zc(priv, queue, p, np, buf->xdp,
+					       rx_hwts);
 			xsk_buff_free(buf->xdp);
 			break;
 		case STMMAC_XDP_CONSUMED:
-- 
2.34.1

[xdp-hints] Re: [PATCH net-next 3/4] net: stmmac: add Rx HWTS metadata to XDP receive pkt

[Simon Horman]

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

[xdp-hints] Re: [PATCH net-next 3/4] net: stmmac: add Rx HWTS metadata to XDP receive pkt

[Song, Yoong Siang]

>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

[xdp-hints] Re: [PATCH net-next 3/4] net: stmmac: add Rx HWTS metadata to XDP receive pkt

[Stanislav Fomichev]

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
> 

[xdp-hints] Re: [PATCH net-next 3/4] net: stmmac: add Rx HWTS metadata to XDP receive pkt

[Simon Horman]

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

[xdp-hints] Re: [PATCH net-next 3/4] net: stmmac: add Rx HWTS metadata to XDP receive pkt

[Song, Yoong Siang]

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

[xdp-hints] Re: [PATCH net-next 3/4] net: stmmac: add Rx HWTS metadata to XDP receive pkt

[Song, Yoong Siang]

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