Re: [PATCH v2] mm: alloc_pages_bulk: remove assumption of populating only NULL elements

[Gao Xiang <hsiangkao@linux.alibaba.com>]

On 2025/3/10 20:31, Yunsheng Lin wrote:
> On 2025/3/10 8:32, Gao Xiang wrote:
> 
> ...
> 
>>>
>>> Also, it seems the fstests doesn't support erofs yet?
>>
>> erofs is an read-only filesystem, and almost all xfstests
>> cases is unsuitable for erofs since erofs needs to preset
>> dataset in advance for runtime testing and only
>> read-related interfaces are cared:
>>
>> You could check erofs-specfic test cases here:
>> https://git.kernel.org/pub/scm/linux/kernel/git/xiang/erofs-utils.git/log/?h=experimental-tests
>>
>> Also the stress test:
>> https://git.kernel.org/pub/scm/linux/kernel/git/xiang/erofs-utils.git/commit/?id=6fa861e282408f8df9ab1654b77b563444b17ea1
> 
> Thanks.
> 
>>
>> BTW, I don't like your new interface either, I don't know
>> why you must insist on this work now that others are
>> already nak this.  Why do you insist on it so much?
> 
> If the idea was not making any sense to me and it was nack'ed
> with clearer reasoning and without any supporting of the idea,
> I would have stopped working on it.
> 
> The background I started working at is something like below
> in the commit log:
> "As mentioned in [1], it seems odd to check NULL elements in
> the middle of page bulk allocating, and it seems caller can
> do a better job of bulk allocating pages into a whole array
> sequentially without checking NULL elements first before
> doing the page bulk allocation for most of existing users."
> 
> "Remove assumption of populating only NULL elements and treat
> page_array as output parameter like kmem_cache_alloc_bulk().
> Remove the above assumption also enable the caller to not
> zero the array before calling the page bulk allocating API,
> which has about 1~2 ns performance improvement for the test
> case of time_bench_page_pool03_slow() for page_pool in a
> x86 vm system, this reduces some performance impact of
> fixing the DMA API misuse problem in [2], performance
> improves from 87.886 ns to 86.429 ns."
> 
> 1. https://lore.kernel.org/all/bd8c2f5c-464d-44ab-b607-390a87ea4cd5@huawei.com/
> 2. https://lore.kernel.org/all/20250212092552.1779679-1-linyunsheng@huawei.com/
> 
> There is no 'must' here, it is just me taking some of my
> hoppy time and some of my work time trying to make the
> alloc_pages_bulk API simpler and more efficient here, and I
> also learnt a lot during that process.


Here are my own premature thoughts just for reference:

  - If you'd like to provide some performance gain, it would
    be much better to get a better end-to-end case to show
    your improvement is important and attractive to some
    in-tree user (rather than show 1~2ns instruction-level
    micro-benchmark margin, is it really important to some
    end use case? At least, the new api is not important to
    erofs since it may only impact our mount time by only
    1~2ns, which is almost nothing, so I have no interest
    to follow the whole thread) since it involves some api
    behavior changes rather than some trivial cleanups.

  - Your new api covers narrow cases compared to the existing
    api, although all in-tree callers may be converted
    properly, but it increases mental burden of all users.
    And maybe complicate future potential users again which
    really have to "check NULL elements in the middle of page
    bulk allocating" again.

To make it clearer, it's not nak from me. But I don't have
any interest to follow your work due to "the real benefit vs
behavior changes".

Thanks,
Gao Xiang