Message-id: <174156544867.33508.5386967459254083056@noble.neil.brown.name>
Date: Mon, 10 Mar 2025 11:10:48 +1100
From: "NeilBrown" <neilb@...e.de>
To: "Yunsheng Lin" <yunshenglin0825@...il.com>
Cc: "Yunsheng Lin" <linyunsheng@...wei.com>, "Qu Wenruo" <wqu@...e.com>,
 "Yishai Hadas" <yishaih@...dia.com>, "Jason Gunthorpe" <jgg@...pe.ca>,
 "Shameer Kolothum" <shameerali.kolothum.thodi@...wei.com>,
 "Kevin Tian" <kevin.tian@...el.com>,
 "Alex Williamson" <alex.williamson@...hat.com>, "Chris Mason" <clm@...com>,
 "Josef Bacik" <josef@...icpanda.com>, "David Sterba" <dsterba@...e.com>,
 "Gao Xiang" <xiang@...nel.org>, "Chao Yu" <chao@...nel.org>,
 "Yue Hu" <zbestahu@...il.com>, "Jeffle Xu" <jefflexu@...ux.alibaba.com>,
 "Sandeep Dhavale" <dhavale@...gle.com>, "Carlos Maiolino" <cem@...nel.org>,
 "Darrick J. Wong" <djwong@...nel.org>,
 "Andrew Morton" <akpm@...ux-foundation.org>,
 "Jesper Dangaard Brouer" <hawk@...nel.org>,
 "Ilias Apalodimas" <ilias.apalodimas@...aro.org>,
 "David S. Miller" <davem@...emloft.net>, "Eric Dumazet" <edumazet@...gle.com>,
 "Jakub Kicinski" <kuba@...nel.org>, "Paolo Abeni" <pabeni@...hat.com>,
 "Simon Horman" <horms@...nel.org>, "Trond Myklebust" <trondmy@...nel.org>,
 "Anna Schumaker" <anna@...nel.org>, "Chuck Lever" <chuck.lever@...cle.com>,
 "Jeff Layton" <jlayton@...nel.org>, "Olga Kornievskaia" <okorniev@...hat.com>,
 "Dai Ngo" <Dai.Ngo@...cle.com>, "Tom Talpey" <tom@...pey.com>,
 "Luiz Capitulino" <luizcap@...hat.com>,
 "Mel Gorman" <mgorman@...hsingularity.net>,
 "Dave Chinner" <david@...morbit.com>, kvm@...r.kernel.org,
 virtualization@...ts.linux.dev, linux-kernel@...r.kernel.org,
 linux-btrfs@...r.kernel.org, linux-erofs@...ts.ozlabs.org,
 linux-xfs@...r.kernel.org, linux-mm@...ck.org, netdev@...r.kernel.org,
 linux-nfs@...r.kernel.org
Subject: Re: [PATCH v2] mm: alloc_pages_bulk: remove assumption of populating
 only NULL elements

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

Powered by blists - more mailing lists