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

[Florian Fainelli <florian.fainelli@broadcom.com>]

On 1/24/25 01:52, Yunsheng Lin wrote:
> 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?

Totally, even though my email domain would suggest otherwise, I am just 
an user of that driver here, not its maintainer, though I do have some 
familiarity with it, I don't know why that choice was made.

> 
>>
>> 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.

Short term, I think I am going to submit a quick fix that is inspired by 
the out of tree patches carried in OpenWrt:

https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob;f=target/linux/bcm53xx/patches-6.6/700-bgmac-reduce-max-frame-size-to-support-just-MTU-1500.patch;h=3a2f4b06ed6d8cda1f3f0be23e1066267234766b;hb=HEAD

Thanks!
-- 
Florian