Re: [PATCH v3 14/15] mm/mempolicy: Add alloc_frozen_pages()

[David Hildenbrand <david@xxxxxxxxxx>]

On 29.11.24 17:29, Zi Yan wrote:
> On 29 Nov 2024, at 9:44, David Hildenbrand wrote:
>
> > On 25.11.24 22:01, Matthew Wilcox (Oracle) wrote:
> > > Provide an interface to allocate pages from the page allocator without
> > > incrementing their refcount.  This saves an atomic operation on free,
> > > which may be beneficial to some users (eg slab).
> > >
> > > Reviewed-by: William Kucharski <william.kucharski@xxxxxxxxxx>
> > > Signed-off-by: Matthew Wilcox (Oracle) <willy@xxxxxxxxxxxxx>
> > > ---
> > >    mm/internal.h  | 12 ++++++++++++
> > >    mm/mempolicy.c | 49 ++++++++++++++++++++++++++++++++-----------------
> > >    2 files changed, 44 insertions(+), 17 deletions(-)
> > >
> > > diff --git a/mm/internal.h b/mm/internal.h
> > > index 55e03f8f41d9..74713b44bedb 100644
> > > --- a/mm/internal.h
> > > +++ b/mm/internal.h
> > > @@ -747,6 +747,18 @@ struct page *__alloc_frozen_pages_noprof(gfp_t, unsigned int order, int nid,
> > >    void free_frozen_pages(struct page *page, unsigned int order);
> > >    void free_unref_folios(struct folio_batch *fbatch);
> > >   +#ifdef CONFIG_NUMA
> > > +struct page *alloc_frozen_pages_noprof(gfp_t, unsigned int order);
> > > +#else
> > > +static inline struct page *alloc_frozen_pages_noprof(gfp_t gfp, unsigned int order)
> > > +{
> > > +	return __alloc_frozen_pages_noprof(gfp, order, numa_node_id(), NULL);
> > > +}
> > > +#endif
> > > +
> > > +#define alloc_frozen_pages(...) \
> > > +	alloc_hooks(alloc_frozen_pages_noprof(__VA_ARGS__))
> > > +
> > >    extern void zone_pcp_reset(struct zone *zone);
> > >    extern void zone_pcp_disable(struct zone *zone);
> > >    extern void zone_pcp_enable(struct zone *zone);
> > > diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> > > index cda5f56085e6..3682184993dd 100644
> > > --- a/mm/mempolicy.c
> > > +++ b/mm/mempolicy.c
> > > @@ -2201,9 +2201,9 @@ static struct page *alloc_pages_preferred_many(gfp_t gfp, unsigned int order,
> > >    	 */
> > >    	preferred_gfp = gfp | __GFP_NOWARN;
> > >    	preferred_gfp &= ~(__GFP_DIRECT_RECLAIM | __GFP_NOFAIL);
> > > -	page = __alloc_pages_noprof(preferred_gfp, order, nid, nodemask);
> > > +	page = __alloc_frozen_pages_noprof(preferred_gfp, order, nid, nodemask);
> > >    	if (!page)
> > > -		page = __alloc_pages_noprof(gfp, order, nid, NULL);
> > > +		page = __alloc_frozen_pages_noprof(gfp, order, nid, NULL);
> > >     	return page;
> > >    }
> > > @@ -2249,8 +2249,9 @@ static struct page *alloc_pages_mpol(gfp_t gfp, unsigned int order,
> > >    			 * First, try to allocate THP only on local node, but
> > >    			 * don't reclaim unnecessarily, just compact.
> > >    			 */
> > > -			page = __alloc_pages_node_noprof(nid,
> > > -				gfp | __GFP_THISNODE | __GFP_NORETRY, order);
> > > +			page = __alloc_frozen_pages_noprof(
> > > +				gfp | __GFP_THISNODE | __GFP_NORETRY, order,
> > > +				nid, NULL);
> > >    			if (page || !(gfp & __GFP_DIRECT_RECLAIM))
> > >    				return page;
> > >    			/*
> > > @@ -2262,7 +2263,7 @@ static struct page *alloc_pages_mpol(gfp_t gfp, unsigned int order,
> > >    		}
> > >    	}
> > >   -	page = __alloc_pages_noprof(gfp, order, nid, nodemask);
> > > +	page = __alloc_frozen_pages_noprof(gfp, order, nid, nodemask);
> > >     	if (unlikely(pol->mode == MPOL_INTERLEAVE) && page) {
> > >    		/* skip NUMA_INTERLEAVE_HIT update if numa stats is disabled */
> > > @@ -2280,8 +2281,13 @@ static struct page *alloc_pages_mpol(gfp_t gfp, unsigned int order,
> > >    struct folio *folio_alloc_mpol_noprof(gfp_t gfp, unsigned int order,
> > >    		struct mempolicy *pol, pgoff_t ilx, int nid)
> > >    {
> > > -	return page_rmappable_folio(alloc_pages_mpol(gfp | __GFP_COMP,
> > > -							order, pol, ilx, nid));
> > > +	struct page *page = alloc_pages_mpol(gfp | __GFP_COMP, order, pol,
> > > +			ilx, nid);
> > > +	if (!page)
> > > +		return NULL;
> > > +
> > > +	set_page_refcounted(page);
> > > +	return page_rmappable_folio(page);
> >
> > What I don't quite like is that we now have a bit of an inconsistency, that makes it harder to understand what gives you frozen and what doesn't give you frozen pages.
> >
> > alloc_pages(): non-frozen/refcounted
> > alloc_frozen_pages(): frozen
> > folio_alloc_mpol_noprof(): non-frozen/refcounted
> > alloc_pages_mpol(): ... frozen pages?
> >
> >
> > Ideally, it would be "alloc_pages": non-frozen, "alloc_frozen_pages": frozen.
> >
> > The same concern applies to other internal functions, like "__alloc_pages_cpuset_fallback".
>
> Or all internal functions (with "__" prefix) give frozen pages and external
> ones give non-frozen ones, unless with frozen in its name, i.e., alloc_frozen_pages()?

If we go that path, we should make it consistent for __free_pages() as 
well I am afraid.

--
Cheers,

David / dhildenb