Re: [PATCH v2 1/2] mm: alloc_pages_bulk_noprof: drop page_list argument

[Yunsheng Lin <linyunsheng@huawei.com>]

On 2025/1/3 0:38, Luiz Capitulino wrote:
> On 2024-12-25 07:36, Yunsheng Lin wrote:
>> On 2024/12/24 6:00, Luiz Capitulino wrote:
>>
>>>   /*
>>> - * __alloc_pages_bulk - Allocate a number of order-0 pages to a list or array
>>> + * __alloc_pages_bulk - Allocate a number of order-0 pages to an array
>>>    * @gfp: GFP flags for the allocation
>>>    * @preferred_nid: The preferred NUMA node ID to allocate from
>>>    * @nodemask: Set of nodes to allocate from, may be NULL
>>> - * @nr_pages: The number of pages desired on the list or array
>>> - * @page_list: Optional list to store the allocated pages
>>> - * @page_array: Optional array to store the pages
>>> + * @nr_pages: The number of pages desired in the array
>>> + * @page_array: Array to store the pages
>>>    *
>>>    * This is a batched version of the page allocator that attempts to
>>> - * allocate nr_pages quickly. Pages are added to page_list if page_list
>>> - * is not NULL, otherwise it is assumed that the page_array is valid.
>>> + * allocate nr_pages quickly. Pages are added to the page_array.
>>>    *
>>> - * For lists, nr_pages is the number of pages that should be allocated.
>>> - *
>>> - * For arrays, only NULL elements are populated with pages and nr_pages
>>> + * Note that only NULL elements are populated with pages and nr_pages
>>
>> It is not really related to this patch, but while we are at this, the above
>> seems like an odd behavior. By roughly looking at all the callers of that
>> API, it seems like only the below callers rely on that?
>> fs/erofs/zutil.c: z_erofs_gbuf_growsize()
>> fs/xfs/xfs_buf.c: xfs_buf_alloc_pages()
>>
>> It seems it is quite straight forward to change the above callers to not
>> rely on the above behavior, and we might be able to avoid more checking
>> by removing the above behavior?
> 
> Hi Yunsheng,
> 
> Assuming the use-case is valid, I think we might want to keep common code
> in the API vs. duplicating it in callers?

I was thinking maybe adding a wrapper/helper around the __alloc_pages_bulk()
to avoid the overhead for other usecase and make the semantics more obvious
if it is an valid use-case.

> 
> In any case, even if we decide to go for your suggestion, I'd prefer not
> to grow the scope of this series since this could delay its inclusion.
> Dropping the list-API (and dead code) is actually important so IMHO it
> should go in first.

Sure.

>