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

[David Laight <david.laight.linux@gmail.com>]

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)