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

[David Hildenbrand <david@xxxxxxxxxx>]

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


I'll note that that's one of the reasons I thought of having GFP_FROZEN 
instead (function name doesn't really matter). The only ugly thing with 
GFP_FROZEN is that we still need separate free*() functions. Well, or 
detect during free_*() that the refcount is 0 and assume that it was an 
frozen allocation ...

Anyhow, if we could make this more consistent, that would help.

--
Cheers,

David / dhildenb