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

[David Hildenbrand <david@redhat.com>]

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@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()?

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

-- 
Cheers,

David / dhildenb