[PATCH net-next] eth: bnxt: take page size into account for page pool recycling rings

[PATCH net-next] eth: bnxt: take page size into account for page pool recycling rings

[Jakub Kicinski]

The Rx rings are filled with Rx buffers. Which are supposed to fit
packet headers (or MTU if HW-GRO is disabled). The aggregation buffers
are filled with "device pages". Adjust the sizes of the page pool
recycling ring appropriately, based on ratio of the size of the
buffer on given ring vs system page size. Otherwise on a system
with 64kB pages we end up with >700MB of memory sitting in every
single page pool cache.

Correct the size calculation for the head_pool. Since the buffers
there are always small I'm pretty sure I meant to cap the size
at 1k, rather than make it the lowest possible size. With 64k pages
1k cache with a 1k ring is 64x larger than we need.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: michael.chan@broadcom.com
CC: pavan.chebbi@broadcom.com
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index c5026fa7e6e6..1c6a3ebcda16 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -3807,12 +3807,14 @@ static int bnxt_alloc_rx_page_pool(struct bnxt *bp,
 				   struct bnxt_rx_ring_info *rxr,
 				   int numa_node)
 {
+	const unsigned int agg_size_fac = PAGE_SIZE / BNXT_RX_PAGE_SIZE;
+	const unsigned int rx_size_fac = PAGE_SIZE / SZ_4K;
 	struct page_pool_params pp = { 0 };
 	struct page_pool *pool;
 
-	pp.pool_size = bp->rx_agg_ring_size;
+	pp.pool_size = bp->rx_agg_ring_size / agg_size_fac;
 	if (BNXT_RX_PAGE_MODE(bp))
-		pp.pool_size += bp->rx_ring_size;
+		pp.pool_size += bp->rx_ring_size / rx_size_fac;
 	pp.nid = numa_node;
 	pp.napi = &rxr->bnapi->napi;
 	pp.netdev = bp->dev;
@@ -3830,7 +3832,7 @@ static int bnxt_alloc_rx_page_pool(struct bnxt *bp,
 
 	rxr->need_head_pool = page_pool_is_unreadable(pool);
 	if (bnxt_separate_head_pool(rxr)) {
-		pp.pool_size = max(bp->rx_ring_size, 1024);
+		pp.pool_size = min(bp->rx_ring_size / rx_size_fac, 1024);
 		pp.flags = PP_FLAG_DMA_MAP | PP_FLAG_DMA_SYNC_DEV;
 		pool = page_pool_create(&pp);
 		if (IS_ERR(pool))
-- 
2.50.0

Re: [PATCH net-next] eth: bnxt: take page size into account for page pool recycling rings

[Michael Chan]

[-- Attachment #1: Type: text/plain, Size: 2053 bytes --]

On Thu, Jun 26, 2025 at 9:54 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> The Rx rings are filled with Rx buffers. Which are supposed to fit
> packet headers (or MTU if HW-GRO is disabled). The aggregation buffers
> are filled with "device pages". Adjust the sizes of the page pool
> recycling ring appropriately, based on ratio of the size of the
> buffer on given ring vs system page size. Otherwise on a system
> with 64kB pages we end up with >700MB of memory sitting in every
> single page pool cache.
>
> Correct the size calculation for the head_pool. Since the buffers
> there are always small I'm pretty sure I meant to cap the size
> at 1k, rather than make it the lowest possible size. With 64k pages
> 1k cache with a 1k ring is 64x larger than we need.
>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
> CC: michael.chan@broadcom.com
> CC: pavan.chebbi@broadcom.com
> ---
>  drivers/net/ethernet/broadcom/bnxt/bnxt.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> index c5026fa7e6e6..1c6a3ebcda16 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> @@ -3807,12 +3807,14 @@ static int bnxt_alloc_rx_page_pool(struct bnxt *bp,
>                                    struct bnxt_rx_ring_info *rxr,
>                                    int numa_node)
>  {
> +       const unsigned int agg_size_fac = PAGE_SIZE / BNXT_RX_PAGE_SIZE;
> +       const unsigned int rx_size_fac = PAGE_SIZE / SZ_4K;
>         struct page_pool_params pp = { 0 };
>         struct page_pool *pool;
>
> -       pp.pool_size = bp->rx_agg_ring_size;
> +       pp.pool_size = bp->rx_agg_ring_size / agg_size_fac;

The bp->rx_agg_ring_size has already taken the system PAGE_SIZE into
consideration to some extent in bnxt_set_ring_params().  The
jumbo_factor and agg_factor will be smaller when PAGE_SIZE is larger.
Will this overcompensate?

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4196 bytes --]

Re: [PATCH net-next] eth: bnxt: take page size into account for page pool recycling rings

[Jakub Kicinski]

On Thu, 26 Jun 2025 14:52:17 -0700 Michael Chan wrote:
> >  {
> > +       const unsigned int agg_size_fac = PAGE_SIZE / BNXT_RX_PAGE_SIZE;
> > +       const unsigned int rx_size_fac = PAGE_SIZE / SZ_4K;
> >         struct page_pool_params pp = { 0 };
> >         struct page_pool *pool;
> >
> > -       pp.pool_size = bp->rx_agg_ring_size;
> > +       pp.pool_size = bp->rx_agg_ring_size / agg_size_fac;  
> 
> The bp->rx_agg_ring_size has already taken the system PAGE_SIZE into
> consideration to some extent in bnxt_set_ring_params().  The
> jumbo_factor and agg_factor will be smaller when PAGE_SIZE is larger.
> Will this overcompensate?

My understanding is basically that bnxt_set_ring_params() operates
on BNXT_RX_PAGE_SIZE so it takes care of 4k .. 32k range pretty well.
But for 64k pages we will use 32k buffers, so 2 agg ring entries
per system page. If our heuristic is that we want the same number
of pages on the device ring as in the pp cache we should divide 
the cache size by two. Hope that makes sense.

My initial temptation was to say that agg ring is always shown to 
the user in 4kB units, regardless of system page. The driver would
divide and multiply the parameter in the ethtool callbacks. Otherwise
even with this patch existing configs for bnxt have to be adjusted
based on system page size :( But I suspect you may have existing users
on systems with 64kB pages, so this would be too risky? WDYT?

Re: [PATCH net-next] eth: bnxt: take page size into account for page pool recycling rings

[Michael Chan]

[-- Attachment #1: Type: text/plain, Size: 1762 bytes --]

On Thu, Jun 26, 2025 at 3:40 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Thu, 26 Jun 2025 14:52:17 -0700 Michael Chan wrote:
> > >  {
> > > +       const unsigned int agg_size_fac = PAGE_SIZE / BNXT_RX_PAGE_SIZE;
> > > +       const unsigned int rx_size_fac = PAGE_SIZE / SZ_4K;
> > >         struct page_pool_params pp = { 0 };
> > >         struct page_pool *pool;
> > >
> > > -       pp.pool_size = bp->rx_agg_ring_size;
> > > +       pp.pool_size = bp->rx_agg_ring_size / agg_size_fac;
> >
> > The bp->rx_agg_ring_size has already taken the system PAGE_SIZE into
> > consideration to some extent in bnxt_set_ring_params().  The
> > jumbo_factor and agg_factor will be smaller when PAGE_SIZE is larger.
> > Will this overcompensate?
>
> My understanding is basically that bnxt_set_ring_params() operates
> on BNXT_RX_PAGE_SIZE so it takes care of 4k .. 32k range pretty well.
> But for 64k pages we will use 32k buffers, so 2 agg ring entries
> per system page. If our heuristic is that we want the same number
> of pages on the device ring as in the pp cache we should divide
> the cache size by two. Hope that makes sense.

Ah, got it.  agg_size_fac will be at most 2 when PAGE_SIZE is 64K.

>
> My initial temptation was to say that agg ring is always shown to
> the user in 4kB units, regardless of system page. The driver would
> divide and multiply the parameter in the ethtool callbacks. Otherwise
> even with this patch existing configs for bnxt have to be adjusted
> based on system page size :( But I suspect you may have existing users
> on systems with 64kB pages, so this would be too risky? WDYT?

Agreed.  Too risky and too confusing.  Thanks.

Reviewed-by: Michael Chan <michael.chan@broadcom.com>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4196 bytes --]

Re: [PATCH net-next] eth: bnxt: take page size into account for page pool recycling rings

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Thu, 26 Jun 2025 09:54:41 -0700 you wrote:
> The Rx rings are filled with Rx buffers. Which are supposed to fit
> packet headers (or MTU if HW-GRO is disabled). The aggregation buffers
> are filled with "device pages". Adjust the sizes of the page pool
> recycling ring appropriately, based on ratio of the size of the
> buffer on given ring vs system page size. Otherwise on a system
> with 64kB pages we end up with >700MB of memory sitting in every
> single page pool cache.
> 
> [...]

Here is the summary with links:
  - [net-next] eth: bnxt: take page size into account for page pool recycling rings
    https://git.kernel.org/netdev/net-next/c/f7dbedba6312

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html