On 2025/3/7 5:14, NeilBrown wrote:

On Thu, 06 Mar 2025, Yunsheng Lin wrote:

quoted

On 2025/3/6 7:41, NeilBrown wrote:

quoted

On Wed, 05 Mar 2025, Yunsheng Lin wrote:

quoted

For the existing btrfs and sunrpc case, I am agreed that there
might be valid use cases too, we just need to discuss how to
meet the requirements of different use cases using simpler, more
unified and effective APIs.

We don't need "more unified".

What I meant about 'more unified' is how to avoid duplicated code as
much as possible for two different interfaces with similar‌ functionality.

The best way I tried to avoid duplicated code as much as possible is
to defragment the page_array before calling the alloc_pages_bulk()
for the use case of btrfs and sunrpc so that alloc_pages_bulk() can
be removed of the assumption populating only NULL elements, so that
the API is simpler and more efficient.

quoted

If there are genuinely two different use cases with clearly different
needs - even if only slightly different - then it is acceptable to have
two different interfaces.  Be sure to choose names which emphasise the
differences.

The best name I can come up with for the use case of btrfs and sunrpc
is something like alloc_pages_bulk_refill(), any better suggestion about
the naming?

I think alloc_pages_bulk_refill() is a good name.

So:
- alloc_pages_bulk() would be given an uninitialised array of page
  pointers and a required count and would return the number of pages
  that were allocated
- alloc_pages_bulk_refill() would be given an initialised array of page
  pointers some of which might be NULL.  It would attempt to allocate
  pages for the non-NULL pointers and return the total number of

You meant 'NULL pointers' instead of 'non-NULL pointers' above?

  allocated pages in the array - just like the current
  alloc_pages_bulk().

I guess 'the total number of allocated pages in the array ' include
the pages which are already in the array before calling the above
API?

I guess it is worth mentioning that the current alloc_pages_bulk()
may return different value with the same size of arrays, but with
different layout of the same number of NULL pointers.
For the same size of arrays with different layout for the NULL pointer
below('*' indicate NULL pointer), and suppose buddy allocator is only
able to allocate two pages:
1. P**P*P: will return 4.
2. P*PP**: will return 5.

If the new API do the page defragmentation, then it will always return
the same value for different layout of the same number of NULL pointers.
I guess the new one is the more perfered behavior as it provides a more
defined semantic.

sunrpc could usefully use both of these interfaces.

alloc_pages_bulk() could be implemented by initialising the array and
then calling alloc_pages_bulk_refill().  Or alloc_pages_bulk_refill()
could be implemented by compacting the pages and then calling
alloc_pages_bulk().
If we could duplicate the code and have two similar but different
functions.

The documentation for _refill() should make it clear that the pages
might get re-ordered.

Does 'the pages might get re-ordered' mean defragmenting the page_array?
If yes, it makes sense to make it clear.

Having looked at some of the callers I agree that the current interface
is not ideal for many of them, and that providing a simpler interface
would help.

+1

Thanks,
NeilBrown