Skip to contentSkip to conversation

net: stmmac: set PP_FLAG_DMA_SYNC_DEV only if XDP is enabled

PATCH Applied Furong Xu 4 participants 4 messages Sep 19, 2024 – Oct 4, 2024

Subsystems (6)
Networking Network drivers Tegra architecture support Arm STM32 +2 more subsystems
lore
Share & tools
lore

Furong Xu

author
#0 lore raw

PATCH v2 1/1 net: stmmac: set PP_FLAG_DMA_SYNC_DEV only if XDP is enabled

Commit 5fabb01207a2 ("net: stmmac: Add initial XDP support") sets PP_FLAG_DMA_SYNC_DEV flag for page_pool unconditionally, page_pool_recycle_direct() will call page_pool_dma_sync_for_device() on every page even the page is not going to be reused by XDP program.

When XDP is not enabled, the page which holds the received buffer will be recycled once the buffer is copied into new SKB by skb_copy_to_linear_data(), then the MAC core will never reuse this page any longer. Always setting PP_FLAG_DMA_SYNC_DEV wastes CPU cycles on unnecessary calling of page_pool_dma_sync_for_device().

After this patch, up to 9% noticeable performance improvement was observed on certain platforms.

Fixes 5fabb01207a2 ("net: stmmac: Add initial XDP support")
Signed-off-by Furong Xu <[email protected]>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +1 -1
index f3a1b179aaea..95d3d1081727 100644
@@ -2022,7 +2022,7 @@ static int __alloc_dma_rx_desc_resources(struct stmmac_priv *priv,
 	rx_q->queue_index = queue;
 	rx_q->priv_data = priv;
 
-	pp_params.flags = PP_FLAG_DMA_MAP | PP_FLAG_DMA_SYNC_DEV;
+	pp_params.flags = PP_FLAG_DMA_MAP | (xdp_prog ? PP_FLAG_DMA_SYNC_DEV : 0);
 	pp_params.pool_size = dma_conf->dma_rx_size;
 	num_pages = DIV_ROUND_UP(dma_conf->dma_buf_sz, PAGE_SIZE);
 	pp_params.order = ilog2(num_pages);
Signature · 1 line
2.34.1
CI Applied
#1 lore raw
This patch was applied to netdev/net.git (main) by Paolo Abeni <[email protected]>:

Hello:

This patch was applied to netdev/net.git (main)
by Paolo Abeni <[email protected]>:

Quoted 5 lines · On Thu, 19 Sep 2024 20:10:28 +0800 you wrote:

Commit 5fabb01207a2 ("net: stmmac: Add initial XDP support") sets PP_FLAG_DMA_SYNC_DEV flag for page_pool unconditionally, page_pool_recycle_direct() will call page_pool_dma_sync_for_device() on every page even the page is not going to be reused by XDP program.

Quoted 7 lines

When XDP is not enabled, the page which holds the received buffer will be recycled once the buffer is copied into new SKB by skb_copy_to_linear_data(), then the MAC core will never reuse this page any longer. Always setting PP_FLAG_DMA_SYNC_DEV wastes CPU cycles on unnecessary calling of page_pool_dma_sync_for_device().

[...]

Here is the summary with links:
- [net,v2] net: stmmac: set PP_FLAG_DMA_SYNC_DEV only if XDP is enabled

    https://git.kernel.org/netdev/net/c/b514c47ebf41

You are awesome, thank you!

Signature · 2 lines
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
maintainer
#2 lore raw
In reply to Furong Xu

Hi Furong,

Quoted 25 lines · Furong Xu

Commit 5fabb01207a2 ("net: stmmac: Add initial XDP support") sets PP_FLAG_DMA_SYNC_DEV flag for page_pool unconditionally, page_pool_recycle_direct() will call page_pool_dma_sync_for_device() on every page even the page is not going to be reused by XDP program.

When XDP is not enabled, the page which holds the received buffer will be recycled once the buffer is copied into new SKB by skb_copy_to_linear_data(), then the MAC core will never reuse this page any longer. Always setting PP_FLAG_DMA_SYNC_DEV wastes CPU cycles on unnecessary calling of page_pool_dma_sync_for_device().

After this patch, up to 9% noticeable performance improvement was observed on certain platforms.

Fixes 5fabb01207a2 ("net: stmmac: Add initial XDP support")
Signed-off-by Furong Xu <[email protected]>
---
  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +0 -0
index f3a1b179aaea..95d3d1081727 100644

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c static int __alloc_dma_rx_desc_resources(struct stmmac_priv *priv, · line 2028

Quoted 8 lines
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +1 -1
@@ -2022,7 +2022,7 @@ static int __alloc_dma_rx_desc_resources(struct stmmac_priv *priv,
  	rx_q->queue_index = queue;
  	rx_q->priv_data = priv;
 
-	pp_params.flags = PP_FLAG_DMA_MAP | PP_FLAG_DMA_SYNC_DEV;
+	pp_params.flags = PP_FLAG_DMA_MAP | (xdp_prog ? PP_FLAG_DMA_SYNC_DEV : 0);
  	pp_params.pool_size = dma_conf->dma_rx_size;
  	num_pages = DIV_ROUND_UP(dma_conf->dma_buf_sz, PAGE_SIZE);
  	pp_params.order = ilog2(num_pages);

We have noticed a boot regression in both -next and mainline v6.12-rc1. Bisect is pointing to this commit. Reverting this commit fixes the problem.

This boot regression is seen on our Tegra234 Jetson AGX Orin platform that uses the drivers/net/ethernet/stmicro/stmmac/dwmac-tegra.c driver. We are booting with NFS and although the network interface does come up, we fail to mount the rootfs via NFS.

So it would appear that we need to set this flag for this device. Any thoughts?

Thanks
Jon

Signature · 1 line
nvpublic
maintainer
#3 lore raw
Quoted 10 lines · On Tue, 1 Oct 2024 11:22:32 +0100 Jon Hunter wrote:

We have noticed a boot regression in both -next and mainline v6.12-rc1. Bisect is pointing to this commit. Reverting this commit fixes the problem.

This boot regression is seen on our Tegra234 Jetson AGX Orin platform that uses the drivers/net/ethernet/stmicro/stmmac/dwmac-tegra.c driver. We are booting with NFS and although the network interface does come up, we fail to mount the rootfs via NFS.

So it would appear that we need to set this flag for this device. Any thoughts?

This patch doesn't make sense to me. I'll send a revert shortly.