[PATCH net-next v2] eth: bnxt: disable rx-copybreak by default

[PATCH net-next v2] eth: bnxt: disable rx-copybreak by default

[Jakub Kicinski]

rx-copybreak requires an extra slab allocation. Since bnxt uses
page pool frags and HDS by default, the rx-copybreak doesn't
buy us anything. The extra pressure on slab causes overload
on pre-sheaves kernels on modern AMD platforms.

In synthetic testing on net-next this patch shows little difference
but I think copybreak is "obvious waste" at this point.

Default rx-copybreak threshold to 0 / disabled.

The "copybreak" defines are really the size bounds for the Rx header
buffer. Rename them.

Reviewed-by: Pavan Chebbi <pavan.chebbi@broadcom.com>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
v2:
 - rename the defines instead of changing the DEFAULT to 0,
   we only want the default setting to change, but the define
   is also used as min rx buffer bound in the code
v1: https://lore.kernel.org/20260529205553.999672-1-kuba@kernel.org
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.h         | 4 ++--
 drivers/net/ethernet/broadcom/bnxt/bnxt.c         | 6 +++---
 drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 4 ++--
 3 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.h b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
index 61c847b36b9f..1920a161841e 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
@@ -36,8 +36,8 @@
 #include <linux/firmware/broadcom/tee_bnxt_fw.h>
 #endif
 
-#define BNXT_DEFAULT_RX_COPYBREAK 256
-#define BNXT_MAX_RX_COPYBREAK 1024
+#define BNXT_MIN_RX_HDR_BUF	256
+#define BNXT_MAX_RX_HDR_BUF	1024
 
 extern struct list_head bnxt_block_cb_list;
 
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index d4f93e62f583..3587f39202d2 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -4846,11 +4846,11 @@ static void bnxt_init_ring_params(struct bnxt *bp)
 {
 	unsigned int rx_size;
 
-	bp->rx_copybreak = BNXT_DEFAULT_RX_COPYBREAK;
+	bp->rx_copybreak = 0; /* rx-copybreak disabled by default */
 	/* Try to fit 4 chunks into a 4k page */
 	rx_size = SZ_1K -
 		NET_SKB_PAD - SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
-	bp->dev->cfg->hds_thresh = max(BNXT_DEFAULT_RX_COPYBREAK, rx_size);
+	bp->dev->cfg->hds_thresh = max(BNXT_MIN_RX_HDR_BUF, rx_size);
 }
 
 /* bp->rx_ring_size, bp->tx_ring_size, dev->mtu, BNXT_FLAG_{G|L}RO flags must
@@ -4911,7 +4911,7 @@ void bnxt_set_ring_params(struct bnxt *bp)
 				  ALIGN(max(NET_SKB_PAD, XDP_PACKET_HEADROOM), 8) -
 				  SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
 		} else {
-			rx_size = max3(BNXT_DEFAULT_RX_COPYBREAK,
+			rx_size = max3(BNXT_MIN_RX_HDR_BUF,
 				       bp->rx_copybreak,
 				       bp->dev->cfg_pending->hds_thresh);
 			rx_size = SKB_DATA_ALIGN(rx_size + NET_IP_ALIGN);
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
index 9b14134d62d2..edafa79f636c 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
@@ -4594,7 +4594,7 @@ static int bnxt_set_tunable(struct net_device *dev,
 	switch (tuna->id) {
 	case ETHTOOL_RX_COPYBREAK:
 		rx_copybreak = *(u32 *)data;
-		if (rx_copybreak > BNXT_MAX_RX_COPYBREAK)
+		if (rx_copybreak > BNXT_MAX_RX_HDR_BUF)
 			return -ERANGE;
 		if (rx_copybreak != bp->rx_copybreak) {
 			if (netif_running(dev))
@@ -5172,7 +5172,7 @@ static int bnxt_run_loopback(struct bnxt *bp)
 	cpr = &rxr->bnapi->cp_ring;
 	if (bp->flags & BNXT_FLAG_CHIP_P5_PLUS)
 		cpr = rxr->rx_cpr;
-	pkt_size = min(bp->dev->mtu + ETH_HLEN, max(BNXT_DEFAULT_RX_COPYBREAK,
+	pkt_size = min(bp->dev->mtu + ETH_HLEN, max(BNXT_MIN_RX_HDR_BUF,
 						    bp->rx_copybreak));
 	skb = netdev_alloc_skb(bp->dev, pkt_size);
 	if (!skb)
-- 
2.54.0

Re: [PATCH net-next v2] eth: bnxt: disable rx-copybreak by default

[David Laight]

On Mon,  1 Jun 2026 17:37:59 -0700
Jakub Kicinski <kuba@kernel.org> wrote:

> rx-copybreak requires an extra slab allocation. Since bnxt uses
> page pool frags and HDS by default, the rx-copybreak doesn't
> buy us anything. The extra pressure on slab causes overload
> on pre-sheaves kernels on modern AMD platforms.

Confused.
Isn't rx-copybreak also about limiting the amount of kernel memory
that gets queued on sockets for small messages?
(see skb->truesize)

There should also be the advantage that the memory the frame is read
into can be re-used for another receive frame without having to change
the iommu.

For systems with an iommu I'd guess that it is worth copying packets
that are even a few 100 bytes long.

Isn't allocating the new skb for the copy just a simple kmalloc().
That ought to be taking items of a per-cpu list and be cheap.
If there is serious pressure on sub-page kmalloc() you've got
bigger problems.

-- David

> 
> In synthetic testing on net-next this patch shows little difference
> but I think copybreak is "obvious waste" at this point.
> 
> Default rx-copybreak threshold to 0 / disabled.
> 
> The "copybreak" defines are really the size bounds for the Rx header
> buffer. Rename them.
> 
> Reviewed-by: Pavan Chebbi <pavan.chebbi@broadcom.com>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
> v2:
>  - rename the defines instead of changing the DEFAULT to 0,
>    we only want the default setting to change, but the define
>    is also used as min rx buffer bound in the code
> v1: https://lore.kernel.org/20260529205553.999672-1-kuba@kernel.org
> ---
>  drivers/net/ethernet/broadcom/bnxt/bnxt.h         | 4 ++--
>  drivers/net/ethernet/broadcom/bnxt/bnxt.c         | 6 +++---
>  drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 4 ++--
>  3 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.h b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
> index 61c847b36b9f..1920a161841e 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.h
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
> @@ -36,8 +36,8 @@
>  #include <linux/firmware/broadcom/tee_bnxt_fw.h>
>  #endif
>  
> -#define BNXT_DEFAULT_RX_COPYBREAK 256
> -#define BNXT_MAX_RX_COPYBREAK 1024
> +#define BNXT_MIN_RX_HDR_BUF	256
> +#define BNXT_MAX_RX_HDR_BUF	1024
>  
>  extern struct list_head bnxt_block_cb_list;
>  
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> index d4f93e62f583..3587f39202d2 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> @@ -4846,11 +4846,11 @@ static void bnxt_init_ring_params(struct bnxt *bp)
>  {
>  	unsigned int rx_size;
>  
> -	bp->rx_copybreak = BNXT_DEFAULT_RX_COPYBREAK;
> +	bp->rx_copybreak = 0; /* rx-copybreak disabled by default */
>  	/* Try to fit 4 chunks into a 4k page */
>  	rx_size = SZ_1K -
>  		NET_SKB_PAD - SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
> -	bp->dev->cfg->hds_thresh = max(BNXT_DEFAULT_RX_COPYBREAK, rx_size);
> +	bp->dev->cfg->hds_thresh = max(BNXT_MIN_RX_HDR_BUF, rx_size);
>  }
>  
>  /* bp->rx_ring_size, bp->tx_ring_size, dev->mtu, BNXT_FLAG_{G|L}RO flags must
> @@ -4911,7 +4911,7 @@ void bnxt_set_ring_params(struct bnxt *bp)
>  				  ALIGN(max(NET_SKB_PAD, XDP_PACKET_HEADROOM), 8) -
>  				  SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
>  		} else {
> -			rx_size = max3(BNXT_DEFAULT_RX_COPYBREAK,
> +			rx_size = max3(BNXT_MIN_RX_HDR_BUF,
>  				       bp->rx_copybreak,
>  				       bp->dev->cfg_pending->hds_thresh);
>  			rx_size = SKB_DATA_ALIGN(rx_size + NET_IP_ALIGN);
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> index 9b14134d62d2..edafa79f636c 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> @@ -4594,7 +4594,7 @@ static int bnxt_set_tunable(struct net_device *dev,
>  	switch (tuna->id) {
>  	case ETHTOOL_RX_COPYBREAK:
>  		rx_copybreak = *(u32 *)data;
> -		if (rx_copybreak > BNXT_MAX_RX_COPYBREAK)
> +		if (rx_copybreak > BNXT_MAX_RX_HDR_BUF)
>  			return -ERANGE;
>  		if (rx_copybreak != bp->rx_copybreak) {
>  			if (netif_running(dev))
> @@ -5172,7 +5172,7 @@ static int bnxt_run_loopback(struct bnxt *bp)
>  	cpr = &rxr->bnapi->cp_ring;
>  	if (bp->flags & BNXT_FLAG_CHIP_P5_PLUS)
>  		cpr = rxr->rx_cpr;
> -	pkt_size = min(bp->dev->mtu + ETH_HLEN, max(BNXT_DEFAULT_RX_COPYBREAK,
> +	pkt_size = min(bp->dev->mtu + ETH_HLEN, max(BNXT_MIN_RX_HDR_BUF,
>  						    bp->rx_copybreak));
>  	skb = netdev_alloc_skb(bp->dev, pkt_size);
>  	if (!skb)