Re: [PATCH net-next v23 3/7] mm: page_frag: use initial zero offset for page_frag_alloc_align()

[Yunsheng Lin <linyunsheng@huawei.com>]

On 2025/1/24 3:15, Florian Fainelli wrote:
> 
> Sorry for the late feedback, this patch causes the bgmac driver in is .ndo_open() function to return -ENOMEM, the call trace looks like this:

Hi, Florian
Thanks for the report.

> 
>  bgmac_open
>   -> bgmac_dma_init
>     -> bgmac_dma_rx_skb_for_slot
>       -> netdev_alloc_frag
> 
> BGMAC_RX_ALLOC_SIZE = 10048 and PAGE_FRAG_CACHE_MAX_SIZE = 32768.

I guess BGMAC_RX_ALLOC_SIZE being bigger than PAGE_SIZE is the
problem here, as the frag API is not really supporting allocating
fragment that is bigger than PAGE_SIZE, as it will fall back to
allocate the base page when order-3 compound page allocation fails,
see __page_frag_cache_refill().

Also, it seems strange that bgmac driver seems to always use jumbo
frame size to allocate fragment, isn't more  appropriate to allocate
the fragment according to MTU?

> 
> Eventually we land into __page_frag_alloc_align() with the following parameters across multiple successive calls:
> 
> __page_frag_alloc_align: fragsz=10048, align_mask=-1, size=32768, offset=0
> __page_frag_alloc_align: fragsz=10048, align_mask=-1, size=32768, offset=10048
> __page_frag_alloc_align: fragsz=10048, align_mask=-1, size=32768, offset=20096
> __page_frag_alloc_align: fragsz=10048, align_mask=-1, size=32768, offset=30144
> 
> So in that case we do indeed have offset + fragsz (40192) > size (32768) and so we would eventually return NULL.

It seems the changing of '(unlikely(offset < 0))' checking to
"fragsz > PAGE_SIZE" causes bgmac driver to break more easily
here. But bgmac driver might likely break too if the system
memory is severely fragmented when falling back to alllocate
base page before this patch.

> 
> Any idea on how to best fix that within the bgmac driver?

Maybe use the page allocator API directly when allocating fragment
with BGMAC_RX_ALLOC_SIZE > PAGE_SIZE for a quick fix.

In the long term, maybe it makes sense to use the page_pool API
as more drivers are converting to use page_pool API already.