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()?
--
Best Regards,
Yan, Zi
