Re: [PATCH net] net: bcmgenet: allocate RX buffers as page fragments

[Nicolai Buchwitz]

Hi Simon

On 25.9.2026 15:47, Simon Horman wrote:
> On Thu, Sep 24, 2026 at 12:19:21PM +0200, Nicolai Buchwitz wrote:
> > Since the page_pool conversion every RX buffer is a whole page and the
> > skb truesize is the page, although the hardware writes at most 2 KiB 
> > of
> > it. On 64 KiB pages a packet therefore counts 65792 bytes against the
> > socket buffer where it used to count 2752. As a result a UDP socket
> > with the default buffer starts to drop after three packets, and the RX
> > rings pin 16 MiB for 512 KiB of buffers.
> >
> > Fix this and allocate the buffers as page fragments, so the truesize 
> > is
> > what a packet occupies. 4 KiB pages stay one buffer per page.
> >
> > Sync each buffer in the refill path, as page_pool can only sync a 
> > whole
> > page on recycle. On 4 KiB pages that is twice what the hardware wrote.
> >
> > Fixes: 7bc054c2d4ed ("net: bcmgenet: convert RX path to page_pool")
> > Reported-by: Karl Mehltretter <kmehltretter@xxxxxxxxx>
> > Closes: 
> > https://lore.kernel.org/all/20260924065839.56793-1-kmehltretter@xxxxxxxxx/
> > Signed-off-by: Nicolai Buchwitz <nb@xxxxxxxxxxx>
>
> ...
>
> > @@ -2254,11 +2257,12 @@ static int bcmgenet_rx_refill(struct 
> > bcmgenet_rx_ring *ring,
> >  			      struct enet_cb *cb)
> >  {
> >  	struct bcmgenet_priv *priv = ring->priv;
> > +	unsigned int size = GENET_RX_BUF_SIZE;
> > +	unsigned int offset;
> >  	dma_addr_t mapping;
> >  	struct page *page;
> >
> > -	page = page_pool_alloc_pages(ring->page_pool,
> > -				     GFP_ATOMIC);
> > +	page = page_pool_dev_alloc(ring->page_pool, &offset, &size);
>
> I think it is not so important either way, but I'm wondering
> if you considered using page_pool_alloc_frag() here.

Yes, I did, but page_pool_alloc() seemed a better fit as it hands back 
the usable
size in *size. Also it falls back to a whole page above half a page and 
avoids an
underestimate at the end of a page.

> >  	if (!page) {
> >  		priv->mib.alloc_rx_buff_failed++;
> >  		netif_err(priv, rx_err, priv->dev,
>
> ...

Thanks,
Nicolai