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

[Simon Horman]

On Fri, Sep 25, 2026 at 03:59:57PM +0200, Nicolai Buchwitz wrote:
> 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.

Thanks for the clarification.
All looks good to me.

Reviewed-by: Simon Horman <horms@xxxxxxxxxx>