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

[Zi Yan <ziy@nvidia.com>]

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@oracle.com>
>> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
>> ---
>>   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()?


--
Best Regards,
Yan, Zi