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

["NeilBrown" <neilb@suse.de>]

On Mon, 10 Mar 2025, Yunsheng Lin wrote:
> On 3/8/2025 5:02 AM, NeilBrown wrote:
> 
> ...
> 
> >>
> >>>    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?
> > 
> > Yes - just what the current function does.
> > Though I don't know that we really need that detail.
> > I think there are three interesting return values:
> > 
> > - hard failure - don't bother trying again soon:   maybe -ENOMEM
> > - success - all pages are allocated:  maybe 0 (or 1?)
> > - partial success - at least one page allocated, ok to try again
> >    immediately - maybe -EAGAIN (or 0).
> 
> Yes, the above makes sense. And I guess returning '-ENOMEM' & '0' &
> '-EAGAIN' seems like a more explicit value.
> 
> > 
> >>
> 
> ...
> 
> >>
> > 
> > If I were do work on this (and I'm not, so you don't have to follow my
> > ideas) I would separate the bulk_alloc into several inline functions and
> > combine them into the different interfaces that you want.  This will
> > result in duplicated object code without duplicated source code.  The
> > object code should be optimal.
> 
> Thanks for the detailed suggestion, it seems feasible.
> If the 'add to a linked list' dispose was not removed in the [1],
> I guess it is worth trying.
> But I am not sure if it is still worth it at the cost of the above
> mentioned 'duplicated object code' considering the array defragmenting
> seem to be able to unify the dispose of 'add to end of array' and
> 'add to next hole in array'.
> 
> I guess I can try with the easier one using array defragmenting first,
> and try below if there is more complicated use case.

Your post observes a performance improvement - slight though it is.
I might be worth measuring the performance change for a case that
requires defragmenting to see how that compares.

Thanks,
NeilBrown


> 
> 1. 
> https://lore.kernel.org/all/f1c75db91d08cafd211eca6a3b199b629d4ffe16.1734991165.git.luizcap@redhat.com/
> 
> > 
> > The parts of the function are:
> >   - validity checks - fallback to single page allocation
> >   - select zone - fallback to single page allocation
> >   - allocate multiple pages in the zone and dispose of them
> >   - allocate a single page
> > 
> > The "dispose of them" is one of
> >    - add to a linked list
> >    - add to end of array
> >    - add to next hole in array
> > 
> > These three could be inline functions that the "allocate multiple pages"
> > and "allocate single page" functions call.  We can pass these as
> > function arguments and the compile will inline them.
> > I imagine these little function would take one page and return
> > a bool indicating if any more are wanted.
> > 
> > The three functions: alloc_bulk_array alloc_bulk_list
> > alloc_bulk_refill_array would each look like:
> > 
> >    validity checks: do we need to allocate anything?
> > 
> >    if want more than one page &&
> >       am allowed to do mulipage (e.g. not __GFP_ACCOUNT) &&
> >       zone = choose_zone() {
> >          alloc_multi_from_zone(zone, dispose_function)
> >    }
> >    if nothing allocated
> >       alloc_single_page(dispose_function)
> > 
> > Each would have a different dispose_function and the initial checks
> > would be quite different, as would the return value.
> > 
> > Thanks for working on this.
> > 
> > NeilBrown
> > 
> 
>