net: stmmac: RX performance improvement

Avoid memcpy in non-XDP RX path by marking all allocated SKBs to be recycled in the upper network stack.

This patch brings ~11.5% driver performance improvement in a TCP RX throughput test with iPerf tool on a single isolated Cortex-A65 CPU core, from 2.18 Gbits/sec increased to 2.43 Gbits/sec.

Signed-off-by Furong Xu <[email protected]>

Reviewed-by Alexander Lobakin <[email protected]>

---
 drivers/net/ethernet/stmicro/stmmac/stmmac.h  |  1 +
 .../net/ethernet/stmicro/stmmac/stmmac_main.c | 26 ++++++++++++-------
 2 files changed, 18 insertions(+), 9 deletions(-)

drivers/net/ethernet/stmicro/stmmac/stmmac.h +1 -0

index e8dbce20129c..f05cae103d83 100644
@@ -126,6 +126,7 @@ struct stmmac_rx_queue {
 	unsigned int cur_rx;
 	unsigned int dirty_rx;
 	unsigned int buf_alloc_num;
+	unsigned int napi_skb_frag_size;
 	dma_addr_t dma_rx_phy;
 	u32 rx_tail_addr;
 	unsigned int state_saved;

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +17 -9

index acd6994c1764..1d98a5e8c98c 100644
@@ -1341,7 +1341,7 @@ static unsigned int stmmac_rx_offset(struct stmmac_priv *priv)
 	if (stmmac_xdp_is_enabled(priv))
 		return XDP_PACKET_HEADROOM;
 
-	return 0;
+	return NET_SKB_PAD;
 }
 
 static int stmmac_set_bfsize(int mtu, int bufsize)

@@ -2040,17 +2040,21 @@ static int __alloc_dma_rx_desc_resources(struct stmmac_priv *priv,
 	struct stmmac_channel *ch = &priv->channel[queue];
 	bool xdp_prog = stmmac_xdp_is_enabled(priv);
 	struct page_pool_params pp_params = { 0 };
-	unsigned int num_pages;
+	unsigned int dma_buf_sz_pad, num_pages;
 	unsigned int napi_id;
 	int ret;
 
+	dma_buf_sz_pad = stmmac_rx_offset(priv) + dma_conf->dma_buf_sz +
+			 SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
+	num_pages = DIV_ROUND_UP(dma_buf_sz_pad, PAGE_SIZE);
+
 	rx_q->queue_index = queue;
 	rx_q->priv_data = priv;
+	rx_q->napi_skb_frag_size = num_pages * PAGE_SIZE;
 
 	pp_params.flags = PP_FLAG_DMA_MAP | PP_FLAG_DMA_SYNC_DEV;
 	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);
+	pp_params.order = order_base_2(num_pages);
 	pp_params.nid = dev_to_node(priv->device);
 	pp_params.dev = priv->device;
 	pp_params.dma_dir = xdp_prog ? DMA_BIDIRECTIONAL : DMA_FROM_DEVICE;

@@ -5582,22 +5586,26 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
 		}
 
 		if (!skb) {
+			unsigned int head_pad_len;
+
 			/* XDP program may expand or reduce tail */
 			buf1_len = ctx.xdp.data_end - ctx.xdp.data;
 
-			skb = napi_alloc_skb(&ch->rx_napi, buf1_len);
+			skb = napi_build_skb(page_address(buf->page),
+					     rx_q->napi_skb_frag_size);
 			if (!skb) {
+				page_pool_recycle_direct(rx_q->page_pool,
+							 buf->page);
 				rx_dropped++;
 				count++;
 				goto drain_data;
 			}
 
 			/* XDP program may adjust header */
-			skb_copy_to_linear_data(skb, ctx.xdp.data, buf1_len);
+			head_pad_len = ctx.xdp.data - ctx.xdp.data_hard_start;
+			skb_reserve(skb, head_pad_len);
 			skb_put(skb, buf1_len);
-
-			/* Data payload copied into SKB, page ready for recycle */
-			page_pool_recycle_direct(rx_q->page_pool, buf->page);
+			skb_mark_for_recycle(skb);
 			buf->page = NULL;
 		} else if (buf1_len) {
 			dma_sync_single_for_cpu(priv->device, buf->addr,

Signature · 1 line

2.34.1

Avoid memcpy in non-XDP RX path by marking all allocated SKBs to be recycled in the upper network stack.

This patch brings ~11.5% driver performance improvement in a TCP RX throughput test with iPerf tool on a single isolated Cortex-A65 CPU core, from 2.18 Gbits/sec increased to 2.43 Gbits/sec.

Signed-off-by Furong Xu <[email protected]>

Reviewed-by Alexander Lobakin <[email protected]>

---
  drivers/net/ethernet/stmicro/stmmac/stmmac.h  |  1 +
  .../net/ethernet/stmicro/stmmac/stmmac_main.c | 26 ++++++++++++-------
  2 files changed, 18 insertions(+), 9 deletions(-)

drivers/net/ethernet/stmicro/stmmac/stmmac.h +1 -0

index e8dbce20129c..f05cae103d83 100644
@@ -126,6 +126,7 @@ struct stmmac_rx_queue {
  	unsigned int cur_rx;
  	unsigned int dirty_rx;
  	unsigned int buf_alloc_num;
+	unsigned int napi_skb_frag_size;
  	dma_addr_t dma_rx_phy;
  	u32 rx_tail_addr;
  	unsigned int state_saved;

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +17 -9

index acd6994c1764..1d98a5e8c98c 100644
@@ -1341,7 +1341,7 @@ static unsigned int stmmac_rx_offset(struct stmmac_priv *priv)
  	if (stmmac_xdp_is_enabled(priv))
  		return XDP_PACKET_HEADROOM;
 
-	return 0;
+	return NET_SKB_PAD;
  }
 
  static int stmmac_set_bfsize(int mtu, int bufsize)

@@ -2040,17 +2040,21 @@ static int __alloc_dma_rx_desc_resources(struct stmmac_priv *priv,
  	struct stmmac_channel *ch = &priv->channel[queue];
  	bool xdp_prog = stmmac_xdp_is_enabled(priv);
  	struct page_pool_params pp_params = { 0 };
-	unsigned int num_pages;
+	unsigned int dma_buf_sz_pad, num_pages;
  	unsigned int napi_id;
  	int ret;
 
+	dma_buf_sz_pad = stmmac_rx_offset(priv) + dma_conf->dma_buf_sz +
+			 SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
+	num_pages = DIV_ROUND_UP(dma_buf_sz_pad, PAGE_SIZE);
+
  	rx_q->queue_index = queue;
  	rx_q->priv_data = priv;
+	rx_q->napi_skb_frag_size = num_pages * PAGE_SIZE;
 
  	pp_params.flags = PP_FLAG_DMA_MAP | PP_FLAG_DMA_SYNC_DEV;
  	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);
+	pp_params.order = order_base_2(num_pages);
  	pp_params.nid = dev_to_node(priv->device);
  	pp_params.dev = priv->device;
  	pp_params.dma_dir = xdp_prog ? DMA_BIDIRECTIONAL : DMA_FROM_DEVICE;

@@ -5582,22 +5586,26 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
  		}
 
  		if (!skb) {
+			unsigned int head_pad_len;
+
  			/* XDP program may expand or reduce tail */
  			buf1_len = ctx.xdp.data_end - ctx.xdp.data;
 
-			skb = napi_alloc_skb(&ch->rx_napi, buf1_len);
+			skb = napi_build_skb(page_address(buf->page),
+					     rx_q->napi_skb_frag_size);
  			if (!skb) {
+				page_pool_recycle_direct(rx_q->page_pool,
+							 buf->page);
  				rx_dropped++;
  				count++;
  				goto drain_data;
  			}
 
  			/* XDP program may adjust header */
-			skb_copy_to_linear_data(skb, ctx.xdp.data, buf1_len);
+			head_pad_len = ctx.xdp.data - ctx.xdp.data_hard_start;
+			skb_reserve(skb, head_pad_len);
  			skb_put(skb, buf1_len);
-
-			/* Data payload copied into SKB, page ready for recycle */
-			page_pool_recycle_direct(rx_q->page_pool, buf->page);
+			skb_mark_for_recycle(skb);
  			buf->page = NULL;
  		} else if (buf1_len) {
  			dma_sync_single_for_cpu(priv->device, buf->addr,

Avoid memcpy in non-XDP RX path by marking all allocated SKBs to be recycled in the upper network stack.

This patch brings ~11.5% driver performance improvement in a TCP RX throughput test with iPerf tool on a single isolated Cortex-A65 CPU core, from 2.18 Gbits/sec increased to 2.43 Gbits/sec.

Signed-off-by Furong Xu <[email protected]>

Reviewed-by Alexander Lobakin <[email protected]>

---
  drivers/net/ethernet/stmicro/stmmac/stmmac.h  |  1 +
  .../net/ethernet/stmicro/stmmac/stmmac_main.c | 26 ++++++++++++-------
  2 files changed, 18 insertions(+), 9 deletions(-)

drivers/net/ethernet/stmicro/stmmac/stmmac.h +1 -0

index e8dbce20129c..f05cae103d83 100644
@@ -126,6 +126,7 @@ struct stmmac_rx_queue {
  	unsigned int cur_rx;
  	unsigned int dirty_rx;
  	unsigned int buf_alloc_num;
+	unsigned int napi_skb_frag_size;
  	dma_addr_t dma_rx_phy;
  	u32 rx_tail_addr;
  	unsigned int state_saved;

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +14 -6

index acd6994c1764..1d98a5e8c98c 100644
@@ -1341,7 +1341,7 @@ static unsigned int stmmac_rx_offset(struct stmmac_priv *priv)
  	if (stmmac_xdp_is_enabled(priv))
  		return XDP_PACKET_HEADROOM;
 
-	return 0;
+	return NET_SKB_PAD;
  }
 
  static int stmmac_set_bfsize(int mtu, int bufsize)

@@ -2040,17 +2040,21 @@ static int __alloc_dma_rx_desc_resources(struct stmmac_priv *priv,
  	struct stmmac_channel *ch = &priv->channel[queue];
  	bool xdp_prog = stmmac_xdp_is_enabled(priv);
  	struct page_pool_params pp_params = { 0 };
-	unsigned int num_pages;
+	unsigned int dma_buf_sz_pad, num_pages;
  	unsigned int napi_id;
  	int ret;
 
+	dma_buf_sz_pad = stmmac_rx_offset(priv) + dma_conf->dma_buf_sz +
+			 SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
+	num_pages = DIV_ROUND_UP(dma_buf_sz_pad, PAGE_SIZE);
+
  	rx_q->queue_index = queue;
  	rx_q->priv_data = priv;
+	rx_q->napi_skb_frag_size = num_pages * PAGE_SIZE;
 
  	pp_params.flags = PP_FLAG_DMA_MAP | PP_FLAG_DMA_SYNC_DEV;
  	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);
+	pp_params.order = order_base_2(num_pages);
  	pp_params.nid = dev_to_node(priv->device);
  	pp_params.dev = priv->device;
  	pp_params.dma_dir = xdp_prog ? DMA_BIDIRECTIONAL : DMA_FROM_DEVICE;

@@ -5582,22 +5586,26 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
  		}
 
  		if (!skb) {
+			unsigned int head_pad_len;
+
  			/* XDP program may expand or reduce tail */
  			buf1_len = ctx.xdp.data_end - ctx.xdp.data;
 
-			skb = napi_alloc_skb(&ch->rx_napi, buf1_len);
+			skb = napi_build_skb(page_address(buf->page),
+					     rx_q->napi_skb_frag_size);
  			if (!skb) {
+				page_pool_recycle_direct(rx_q->page_pool,
+							 buf->page);
  				rx_dropped++;
  				count++;
  				goto drain_data;
  			}
 
  			/* XDP program may adjust header */
-			skb_copy_to_linear_data(skb, ctx.xdp.data, buf1_len);

We have noticed a boot regression on -next when booting with NFS. Bisect is pointing to this commit and reverting this on top of -next does fix the problem.

I only see this on Tegra234 which uses the drivers/net/ethernet/stmicro/stmmac/dwmac-tegra.c driver. Tegra194 which uses the drivers/net/ethernet/stmicro/stmmac/dwmac-dwc-qos-eth.c driver appears to be fine.

What is the MTU of Tegra234 and NFS server? Are they both 1500?

Could you please try attached patch to confirm if this regression is fixed?

If the attached patch fixes this regression, and so it seems to be a cache coherence issue specific to Tegra234, since this patch avoid memcpy and the page buffers may be modified by upper network stack of course, then cache lines of page buffers may become dirty. But by reverting this patch, cache lines of page buffers never become dirty, this is the core difference.

Just to clarify, the patch that you had us try was not intended as an actual fix, correct? It was only for diagnostic purposes, i.e. to see if there is some kind of cache coherence issue, which seems to be the case? So perhaps the only fix needed is to add dma-coherent to our device tree?

Quoted 4 lines

Just to clarify, the patch that you had us try was not intended as an actual fix, correct? It was only for diagnostic purposes, i.e. to see if there is some kind of cache coherence issue, which seems to be the case? So perhaps the only fix needed is to add dma-coherent to our device tree?

That sounds quite error prone. How many other DT blobs are missing the property? If the memory should be coherent, i would expect the driver to allocate coherent memory. Or the driver needs to handle non-coherent memory and add the necessary flush/invalidates etc.

Quoted 9 lines · Andrew Lunn

Quoted 4 lines

Just to clarify, the patch that you had us try was not intended as an actual fix, correct? It was only for diagnostic purposes, i.e. to see if there is some kind of cache coherence issue, which seems to be the case? So perhaps the only fix needed is to add dma-coherent to our device tree?

That sounds quite error prone. How many other DT blobs are missing the property? If the memory should be coherent, i would expect the driver to allocate coherent memory. Or the driver needs to handle non-coherent memory and add the necessary flush/invalidates etc.

stmmac driver does the necessary cache flush/invalidates to maintain cache lines explicitly.

See dma_sync_single_for_cpu():
https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/include/linux/dma-mapping.h#n297

dma_dev_need_sync() is supposed to return false for Tegra234, since the ethernet controller on Tegra234 is dma coherent by SoC design as Brad said their downstream device tree has dma-coherent turned on by default, and after add dma-coherent to mainline ethernet node, stmmac driver works fine. But dma-coherent property is missing in mainline Tegra234 ethernet device tree node, dma_dev_need_sync() returns true and this is not the expected behavior.

The dma-coherent property in device tree node is SoC specific, so only the vendors know if their stmmac ethernet controller is dma coherent and whether their device tree are missing the critical dma-coherent property.

Quoted 3 lines

The dma-coherent property in device tree node is SoC specific, so only the vendors know if their stmmac ethernet controller is dma coherent and whether their device tree are missing the critical dma-coherent property.

What I fail to understand is how dma-coherent can make a difference in this case. If it's not present, then the driver is supposed to maintain caches explicitly. But it seems like explicit cache maintenance actually causes things to break. So do we need to assume that DMA coherent devices in generally won't work if the driver manages caches explicitly?

I always figured dma-coherent was more of an optimization, but this seems to indicate it isn't.

Quoted 5 lines · Andrew Lunn

Quoted 4 lines

Just to clarify, the patch that you had us try was not intended as an actual fix, correct? It was only for diagnostic purposes, i.e. to see if there is some kind of cache coherence issue, which seems to be the case? So perhaps the only fix needed is to add dma-coherent to our device tree?

Quoted 4 lines

That sounds quite error prone. How many other DT blobs are missing the property? If the memory should be coherent, i would expect the driver to allocate coherent memory. Or the driver needs to handle non-coherent memory and add the necessary flush/invalidates etc.

stmmac driver does the necessary cache flush/invalidates to maintain cache lines explicitly.

Quoted 8 lines · Furong Xu

Quoted 6 lines · Andrew Lunn

Quoted 5 lines

Just to clarify, the patch that you had us try was not intended as an actual fix, correct? It was only for diagnostic purposes, i.e. to see if there is some kind of cache coherence issue, which seems to be the case? So perhaps the only fix needed is to add dma-coherent to our device tree?

Quoted 8 lines

Quoted 5 lines

That sounds quite error prone. How many other DT blobs are missing the property? If the memory should be coherent, i would expect the driver to allocate coherent memory. Or the driver needs to handle non-coherent memory and add the necessary flush/invalidates etc.

stmmac driver does the necessary cache flush/invalidates to maintain cache lines explicitly.

Given the problem happens when the kernel performs syncing, is it possible that there is a problem with how the syncing is performed?

I am not familiar with this driver, but it seems to allocate multiple buffers per packet when split header is enabled and these buffers are allocated from the same page pool (see stmmac_init_rx_buffers()). Despite that, the driver is creating the page pool with a non-zero offset (see __alloc_dma_rx_desc_resources()) to avoid syncing the headroom, which is only present in the head buffer.

I asked Thierry to test the following patch [1] and initial testing seems OK. He also confirmed that "SPH feature enabled" shows up in the kernel log.
BTW, the commit that added split header support (67afd6d1cfdf0) says that it "reduces CPU usage because without the feature all the entire packet is memcpy'ed, while that with the feature only the header is". This is no longer correct after your patch, so is there still value in the split header feature? With two large buffers being allocated from

Hi Ido

Quoted 28 lines · Ido Schimmel

Quoted 8 lines · Furong Xu

Quoted 6 lines · Andrew Lunn

Quoted 5 lines

Just to clarify, the patch that you had us try was not intended as an actual fix, correct? It was only for diagnostic purposes, i.e. to see if there is some kind of cache coherence issue, which seems to be the case? So perhaps the only fix needed is to add dma-coherent to our device tree?

Quoted 8 lines

Quoted 5 lines

That sounds quite error prone. How many other DT blobs are missing the property? If the memory should be coherent, i would expect the driver to allocate coherent memory. Or the driver needs to handle non-coherent memory and add the necessary flush/invalidates etc.

stmmac driver does the necessary cache flush/invalidates to maintain cache lines explicitly.

Given the problem happens when the kernel performs syncing, is it possible that there is a problem with how the syncing is performed?

I am not familiar with this driver, but it seems to allocate multiple buffers per packet when split header is enabled and these buffers are allocated from the same page pool (see stmmac_init_rx_buffers()). Despite that, the driver is creating the page pool with a non-zero offset (see __alloc_dma_rx_desc_resources()) to avoid syncing the headroom, which is only present in the head buffer.

Quoted 8 lines

I asked Thierry to test the following patch [1] and initial testing seems OK. He also confirmed that "SPH feature enabled" shows up in the kernel log.
BTW, the commit that added split header support (67afd6d1cfdf0) says that it "reduces CPU usage because without the feature all the entire packet is memcpy'ed, while that with the feature only the header is". This is no longer correct after your patch, so is there still value in the split header feature? With two large buffers being allocated from

Thanks for these great insights!

Yes, when "SPH feature enabled", it is not correct after my patch, pp_params.offset should be updated to match the offset of split payload.

But I would like to let pp_params.max_len remains to dma_conf->dma_buf_sz since the sizes of both header and payload are limited to dma_conf->dma_buf_sz by DMA engine, no more than dma_conf->dma_buf_sz bytes will be written into a page buffer. So my patch would be like [2]:

BTW, the split header feature will be very useful on some certain cases, stmmac driver should support this feature always.

[2]

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +0 -0

index edbf8994455d..def0d893efbb 100644

SPH is the only scenario in which the driver uses multiple buffers per packet?

Quoted 1 line

        pp_params.max_len = dma_conf->dma_buf_sz;  

Are you sure this is correct? Page pool documentation says that "For pages recycled on the XDP xmit and skb paths the page pool will use the max_len member of struct page_pool_params to decide how much of the page needs to be synced (starting at offset)" [1].

While "no more than dma_conf->dma_buf_sz bytes will be written into a page buffer", for the head buffer they will be written starting at a non-zero offset unlike buffers used for the data, no?

The "SPH feature" splits header into buf->page (non-zero offset) and splits payload into buf->sec_page (zero offset).

For buf->page, pp_params.max_len should be the size of L3/L4 header, and with a offset of NET_SKB_PAD.

For buf->sec_page, pp_params.max_len should be dma_conf->dma_buf_sz, and with a offset of 0.

This is always true:
sizeof(L3/L4 header) + NET_SKB_PAD < dma_conf->dma_buf_sz + 0

Quoted 9 lines · Furong Xu

The "SPH feature" splits header into buf->page (non-zero offset) and splits payload into buf->sec_page (zero offset).

For buf->page, pp_params.max_len should be the size of L3/L4 header, and with a offset of NET_SKB_PAD.

For buf->sec_page, pp_params.max_len should be dma_conf->dma_buf_sz, and with a offset of 0.

Quoted 2 lines

This is always true:
sizeof(L3/L4 header) + NET_SKB_PAD < dma_conf->dma_buf_sz + 0

Thanks, understood, but are there situations where the device is unable to split a packet? For example, a large L2 packet. I am trying to understand if there are situations where the device will write more than "dma_conf->dma_buf_sz - NET_SKB_PAD" to the head buffer.

Quoted 8 lines · Furong Xu

Quoted 6 lines · Andrew Lunn

Quoted 5 lines

Just to clarify, the patch that you had us try was not intended as an actual fix, correct? It was only for diagnostic purposes, i.e. to see if there is some kind of cache coherence issue, which seems to be the case? So perhaps the only fix needed is to add dma-coherent to our device tree?

Quoted 8 lines

Quoted 5 lines

That sounds quite error prone. How many other DT blobs are missing the property? If the memory should be coherent, i would expect the driver to allocate coherent memory. Or the driver needs to handle non-coherent memory and add the necessary flush/invalidates etc.

stmmac driver does the necessary cache flush/invalidates to maintain cache lines explicitly.

Given the problem happens when the kernel performs syncing, is it possible that there is a problem with how the syncing is performed?

I am not familiar with this driver, but it seems to allocate multiple buffers per packet when split header is enabled and these buffers are allocated from the same page pool (see stmmac_init_rx_buffers()). Despite that, the driver is creating the page pool with a non-zero offset (see __alloc_dma_rx_desc_resources()) to avoid syncing the headroom, which is only present in the head buffer.

I asked Thierry to test the following patch [1] and initial testing seems OK. He also confirmed that "SPH feature enabled" shows up in the kernel log.

It is recommended to disable the "SPH feature" by default unless some certain cases depend on it. Like Ido said, two large buffers being allocated from the same page pool for each packet, this is a huge waste of memory, and brings performance drops for most of general cases.

Quoted 5 lines

It is recommended to disable the "SPH feature" by default unless some certain cases depend on it. Like Ido said, two large buffers being allocated from the same page pool for each packet, this is a huge waste of memory, and brings performance drops for most of general cases.

I don't know this driver, but it looks like SPH is required for NETIF_F_GRO? Can you add this flag to hw_features, but not wanted_features and leave SPH disabled until ethtool is used to enable GRO?

Are there other use cases where SPH is needed?

Hi Thierry

Quoted 28 lines · Ido Schimmel

Quoted 8 lines · Furong Xu

Quoted 6 lines · Andrew Lunn

Quoted 5 lines

Just to clarify, the patch that you had us try was not intended as an actual fix, correct? It was only for diagnostic purposes, i.e. to see if there is some kind of cache coherence issue, which seems to be the case? So perhaps the only fix needed is to add dma-coherent to our device tree?

Quoted 8 lines

Quoted 5 lines

That sounds quite error prone. How many other DT blobs are missing the property? If the memory should be coherent, i would expect the driver to allocate coherent memory. Or the driver needs to handle non-coherent memory and add the necessary flush/invalidates etc.

stmmac driver does the necessary cache flush/invalidates to maintain cache lines explicitly.

Given the problem happens when the kernel performs syncing, is it possible that there is a problem with how the syncing is performed?

I am not familiar with this driver, but it seems to allocate multiple buffers per packet when split header is enabled and these buffers are allocated from the same page pool (see stmmac_init_rx_buffers()). Despite that, the driver is creating the page pool with a non-zero offset (see __alloc_dma_rx_desc_resources()) to avoid syncing the headroom, which is only present in the head buffer.

Quoted 3 lines

I asked Thierry to test the following patch [1] and initial testing seems OK. He also confirmed that "SPH feature enabled" shows up in the kernel log.

It is recommended to disable the "SPH feature" by default unless some certain cases depend on it. Like Ido said, two large buffers being allocated from the same page pool for each packet, this is a huge waste of memory, and brings performance drops for most of general cases.

Our downstream driver and two mainline drivers disable SPH by default:
https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/drivers/net/ethernet/stmicro/stmmac/dwmac-dwc-qos-eth.c#n357
https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c#n471

Quoted 36 lines · Furong Xu

Hi Thierry

Quoted 28 lines · Ido Schimmel

Quoted 8 lines · Furong Xu

Quoted 6 lines · Andrew Lunn

Quoted 5 lines

Just to clarify, the patch that you had us try was not intended as an actual fix, correct? It was only for diagnostic purposes, i.e. to see if there is some kind of cache coherence issue, which seems to be the case? So perhaps the only fix needed is to add dma-coherent to our device tree?

Quoted 8 lines

Quoted 5 lines

That sounds quite error prone. How many other DT blobs are missing the property? If the memory should be coherent, i would expect the driver to allocate coherent memory. Or the driver needs to handle non-coherent memory and add the necessary flush/invalidates etc.

stmmac driver does the necessary cache flush/invalidates to maintain cache lines explicitly.

Given the problem happens when the kernel performs syncing, is it possible that there is a problem with how the syncing is performed?

I am not familiar with this driver, but it seems to allocate multiple buffers per packet when split header is enabled and these buffers are allocated from the same page pool (see stmmac_init_rx_buffers()). Despite that, the driver is creating the page pool with a non-zero offset (see __alloc_dma_rx_desc_resources()) to avoid syncing the headroom, which is only present in the head buffer.

Quoted 3 lines

I asked Thierry to test the following patch [1] and initial testing seems OK. He also confirmed that "SPH feature enabled" shows up in the kernel log.

Quoted 8 lines

It is recommended to disable the "SPH feature" by default unless some certain cases depend on it. Like Ido said, two large buffers being allocated from the same page pool for each packet, this is a huge waste of memory, and brings performance drops for most of general cases.

Our downstream driver and two mainline drivers disable SPH by default:
https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/drivers/net/ethernet/stmicro/stmmac/dwmac-dwc-qos-eth.c#n357
https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c#n471

Okay, that's something we can look into changing. What would be an example of a use-case depending on SPH? Also, isn't this something that should be a policy that users can configure?

Irrespective of that we should fix the problems we are seeing with SPH enabled.

Hi Furong,

Quoted 50 lines · Thierry Reding

Quoted 36 lines · Furong Xu

Hi Thierry

Quoted 28 lines · Ido Schimmel

Quoted 8 lines · Furong Xu

Quoted 6 lines · Andrew Lunn

Quoted 5 lines

Just to clarify, the patch that you had us try was not intended as an actual fix, correct? It was only for diagnostic purposes, i.e. to see if there is some kind of cache coherence issue, which seems to be the case? So perhaps the only fix needed is to add dma-coherent to our device tree?

Quoted 8 lines

Quoted 5 lines

That sounds quite error prone. How many other DT blobs are missing the property? If the memory should be coherent, i would expect the driver to allocate coherent memory. Or the driver needs to handle non-coherent memory and add the necessary flush/invalidates etc.

stmmac driver does the necessary cache flush/invalidates to maintain cache lines explicitly.

Given the problem happens when the kernel performs syncing, is it possible that there is a problem with how the syncing is performed?

I am not familiar with this driver, but it seems to allocate multiple buffers per packet when split header is enabled and these buffers are allocated from the same page pool (see stmmac_init_rx_buffers()). Despite that, the driver is creating the page pool with a non-zero offset (see __alloc_dma_rx_desc_resources()) to avoid syncing the headroom, which is only present in the head buffer.

Quoted 3 lines

I asked Thierry to test the following patch [1] and initial testing seems OK. He also confirmed that "SPH feature enabled" shows up in the kernel log.

Quoted 8 lines

It is recommended to disable the "SPH feature" by default unless some certain cases depend on it. Like Ido said, two large buffers being allocated from the same page pool for each packet, this is a huge waste of memory, and brings performance drops for most of general cases.

Our downstream driver and two mainline drivers disable SPH by default:
https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/drivers/net/ethernet/stmicro/stmmac/dwmac-dwc-qos-eth.c#n357
https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c#n471

Okay, that's something we can look into changing. What would be an example of a use-case depending on SPH? Also, isn't this something that should be a policy that users can configure?

Quoted 2 lines

Irrespective of that we should fix the problems we are seeing with SPH enabled.

Any update on this?

Hi Jon,

Quoted 53 lines · Jon Hunter

Hi Furong,

Quoted 50 lines · Thierry Reding

Quoted 36 lines · Furong Xu

Hi Thierry

Quoted 28 lines · Ido Schimmel

Quoted 8 lines · Furong Xu

Quoted 6 lines · Andrew Lunn

Quoted 5 lines

Just to clarify, the patch that you had us try was not intended as an actual fix, correct? It was only for diagnostic purposes, i.e. to see if there is some kind of cache coherence issue, which seems to be the case? So perhaps the only fix needed is to add dma-coherent to our device tree?

Quoted 8 lines

Quoted 5 lines

That sounds quite error prone. How many other DT blobs are missing the property? If the memory should be coherent, i would expect the driver to allocate coherent memory. Or the driver needs to handle non-coherent memory and add the necessary flush/invalidates etc.

stmmac driver does the necessary cache flush/invalidates to maintain cache lines explicitly.

Given the problem happens when the kernel performs syncing, is it possible that there is a problem with how the syncing is performed?

I am not familiar with this driver, but it seems to allocate multiple buffers per packet when split header is enabled and these buffers are allocated from the same page pool (see stmmac_init_rx_buffers()). Despite that, the driver is creating the page pool with a non-zero offset (see __alloc_dma_rx_desc_resources()) to avoid syncing the headroom, which is only present in the head buffer.

Quoted 3 lines

I asked Thierry to test the following patch [1] and initial testing seems OK. He also confirmed that "SPH feature enabled" shows up in the kernel log.

Quoted 8 lines

It is recommended to disable the "SPH feature" by default unless some certain cases depend on it. Like Ido said, two large buffers being allocated from the same page pool for each packet, this is a huge waste of memory, and brings performance drops for most of general cases.

Our downstream driver and two mainline drivers disable SPH by default:
https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/drivers/net/ethernet/stmicro/stmmac/dwmac-dwc-qos-eth.c#n357
https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c#n471

Okay, that's something we can look into changing. What would be an example of a use-case depending on SPH? Also, isn't this something that should be a policy that users can configure?

Quoted 5 lines

Quoted 2 lines

Irrespective of that we should fix the problems we are seeing with SPH enabled.

Any update on this?

Sorry for my late response, I was on Chinese New Year holiday.

The fix is sent, and it will be so nice to have your Tested-by: tag there:
https://lore.kernel.org/all/[email protected]/

Quoted 1 line · Furong Xu

What is the MTU of Tegra234 and NFS server? Are they both 1500?

I see the same issue. Yes, both are 1500.

Quoted 2 lines

Could you please try attached patch to confirm if this regression is fixed?

Patch fixes the issue.

Quoted 6 lines

If the attached patch fixes this regression, and so it seems to be a cache coherence issue specific to Tegra234, since this patch avoid memcpy and the page buffers may be modified by upper network stack of course, then cache lines of page buffers may become dirty. But by reverting this patch, cache lines of page buffers never become dirty, this is the core difference.

Thanks for these insights. I don't have specific experience in this driver, but I see we have dma-coherent turned on for this driver in our downstream device tree files (i.e. dtbs that coincide with our out-of-tree implementation of this driver). I went back to the original code and verified that the issue was there. I did a new test where I added dma-coherent to this ethernet node in the dtb and retested. It worked!

Just to clarify, the patch that you had us try was not intended as an actual fix, correct? It was only for diagnostic purposes, i.e. to see if there is some kind of cache coherence issue, which seems to be the case?

So perhaps the only fix needed is to add dma-coherent to our device tree?

It is not an actual fix, it is only for diagnostic purposes.

Quoted 1 line

So perhaps the only fix needed is to add dma-coherent to our device tree?

Yes, add dma-coherent to ethernet node is the correct fix.

DMA engine will always write no more than dma_buf_sz bytes of a received frame into a page buffer, the remaining spaces are unused or used by CPU exclusively.
Setting page_pool_params.max_len to almost the full size of page(s) helps nothing more, but wastes more CPU cycles on cache maintenance.

For a standard MTU of 1500, then dma_buf_sz is assigned to 1536, and this patch brings ~16.9% driver performance improvement in a TCP RX throughput test with iPerf tool on a single isolated Cortex-A65 CPU core, from 2.43 Gbits/sec increased to 2.84 Gbits/sec.

Signed-off-by Furong Xu <[email protected]>

Reviewed-by Alexander Lobakin <[email protected]>

---
  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 2 +-
  drivers/net/ethernet/stmicro/stmmac/stmmac_xdp.h  | 1 -
  2 files changed, 1 insertion(+), 2 deletions(-)

Current code prefetches cache lines for the received frame first, and then dma_sync_single_for_cpu() against this frame, this is wrong. Cache prefetch should be triggered after dma_sync_single_for_cpu().

This patch brings ~2.8% driver performance improvement in a TCP RX throughput test with iPerf tool on a single isolated Cortex-A65 CPU core, 2.84 Gbits/sec increased to 2.92 Gbits/sec.

Signed-off-by Furong Xu <[email protected]>

Reviewed-by Alexander Lobakin <[email protected]>

---
  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 5 +----
  1 file changed, 1 insertion(+), 4 deletions(-)

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +1 -4

index 811e2d372abf..ad928e8e21a9 100644
@@ -5508,10 +5508,6 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
 
  		/* Buffer is good. Go on. */
 
-		prefetch(page_address(buf->page) + buf->page_offset);
-		if (buf->sec_page)
-			prefetch(page_address(buf->sec_page));
-
  		buf1_len = stmmac_rx_buf1_len(priv, p, status, len);
  		len += buf1_len;
  		buf2_len = stmmac_rx_buf2_len(priv, p, status, len);

@@ -5533,6 +5529,7 @@ 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);
+			prefetch(page_address(buf->page) + buf->page_offset);
 
  			xdp_init_buff(&ctx.xdp, buf_sz, &rx_q->xdp_rxq);
  			xdp_prepare_buff(&ctx.xdp, page_address(buf->page),

The size of DMA descriptors is 32 bytes at most. net_prefetch() for received frames, and keep prefetch() for descriptors.

This patch brings ~4.8% driver performance improvement in a TCP RX throughput test with iPerf tool on a single isolated Cortex-A65 CPU core, 2.92 Gbits/sec increased to 3.06 Gbits/sec.

Suggested-by Joe Damato <[email protected]>

Signed-off-by Furong Xu <[email protected]>

---
  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +2 -1

index ad928e8e21a9..49b41148d594 100644
@@ -5529,7 +5529,8 @@ 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);
-			prefetch(page_address(buf->page) + buf->page_offset);
+			net_prefetch(page_address(buf->page) +
+				     buf->page_offset);
 
  			xdp_init_buff(&ctx.xdp, buf_sz, &rx_q->xdp_rxq);
  			xdp_prepare_buff(&ctx.xdp, page_address(buf->page),

The size of DMA descriptors is 32 bytes at most. net_prefetch() for received frames, and keep prefetch() for descriptors.

This patch brings ~4.8% driver performance improvement in a TCP RX throughput test with iPerf tool on a single isolated Cortex-A65 CPU core, 2.92 Gbits/sec increased to 3.06 Gbits/sec.

Suggested-by Joe Damato <[email protected]>

Signed-off-by Furong Xu <[email protected]>

---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +2 -1

index ad928e8e21a9..49b41148d594 100644
@@ -5529,7 +5529,8 @@ 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);
-			prefetch(page_address(buf->page) + buf->page_offset);
+			net_prefetch(page_address(buf->page) +
+				     buf->page_offset);
 
 			xdp_init_buff(&ctx.xdp, buf_sz, &rx_q->xdp_rxq);
 			xdp_prepare_buff(&ctx.xdp, page_address(buf->page),

Signature · 1 line

2.34.1

The size of DMA descriptors is 32 bytes at most. net_prefetch() for received frames, and keep prefetch() for descriptors.

This patch brings ~4.8% driver performance improvement in a TCP RX throughput test with iPerf tool on a single isolated Cortex-A65 CPU core, 2.92 Gbits/sec increased to 3.06 Gbits/sec.

Suggested-by Joe Damato <[email protected]>

Signed-off-by Furong Xu <[email protected]>

---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)