Re: [PATCH v5 05/18] mm/page_alloc: unify __alloc_frozen_pages[_nolock]_noprof()

[Brendan Jackman]

On Fri Jul 3, 2026 at 2:42 PM UTC, Zi Yan wrote:
> On Fri Jul 3, 2026 at 8:31 AM EDT, Brendan Jackman wrote:
>> Currently the core allocator code is controlled by ALLOC_NOLOCK, but the
>> main entry point function is significantly different from the normal
>> __alloc_frozen_pages_nolock(), this is tiring when reading the code.
>>
>> Plumb the ALLOC_NOLOCK control one layer up in the call stack: create
>> an alloc_flags argument to __alloc_frozen_pages_nolock() (which is only
>> exposed to mm/) and then turn the nolock variant into a thin wrapper
>> that just sets that flag (as well as handling NUMA_NO_NODE, similar to
>> how some of the wrappers in gfp.h do).
>>
>> For consistency, set ALLOC_WMARK_MIN explicitly in fastpath_alloc_flags
>> for the new ALLOC_NOLOCK path. This was already "done" silently in
>> __alloc_frozen_pages_nolock_noprof(): ALLOC_WMARK_MIN is 0.
>>
>> Rationale that this doesn't change anything:
>>
>> 1. Simple bits: A bunch of the nolock-specific handling is just moved to
>>    the new alloc_order_allowed(), alloc_nolock_allowed() and
>>    gfp_nolock.
>>
>> 2. __alloc_frozen_pages_noprof() has some extra logic that wasn't
>>    previously in the nolock variant:
>>
>>    a. Application of gfp_allowed_mask; this only affects early boot,
>>       only flags that affect the slowpath get changed here, and the
>>       nolock allocation path isn't allowed to the GFP_BOOT_MASK flags.
>>
>>    b. Application of current_gfp_context() - also only affects the
>>       slowpath
>>
>> 3. The slowpath itself: this is now just explicitly skipped under
>>    !ALLOC_TRYLOCK.
>
> s/TRYLOCK/NOLOCK

Thanks - Andrew would you mind fixing this up in mm-new?

>>
>> Ulterior motive: adding an alloc_flags arg to the allocator's
>> mm-internal entrypoint can later be used to do more allocation
>> customisation without needing to create new GFP flags.
>>
>> No functional change intended.
>>
>> Reviewed-by: Vlastimil Babka (SUSE) <vbabka@xxxxxxxxxx>
>> Signed-off-by: Brendan Jackman <jackmanb@xxxxxxxxxx>
>> ---
>>  mm/hugetlb.c    |   3 +-
>>  mm/mempolicy.c  |  10 +--
>>  mm/page_alloc.c | 192 +++++++++++++++++++++++++++++---------------------------
>>  mm/page_alloc.h |   6 +-
>>  mm/slub.c       |   6 +-
>>  5 files changed, 117 insertions(+), 100 deletions(-)
>>
>
> <snip>
>
>> +/*
>> + * This is the 'heart' of the zoned buddy allocator.
>> + */
>> +struct page *__alloc_frozen_pages_noprof(gfp_t gfp, unsigned int order,
>> +		int preferred_nid, nodemask_t *nodemask, unsigned int alloc_flags)
>> +{
>> +	struct page *page;
>> +	gfp_t alloc_gfp; /* The gfp_t that was actually used for allocation */
>> +	struct alloc_context ac = { };
>> +	unsigned int fastpath_alloc_flags = alloc_flags;
>> +
>> +	/* Other flags could be supported later if needed. */
>> +	if (WARN_ON(alloc_flags & ~ALLOC_NOLOCK))
>>  		return NULL;
>>  
>> +	if (!alloc_order_allowed(gfp, order, alloc_flags))
>> +		return NULL;
>> +
>> +	if (alloc_flags & ALLOC_NOLOCK) {
>> +		VM_WARN_ON_ONCE(gfp & ~__GFP_ACCOUNT);
>> +		if (!alloc_nolock_allowed())
>> +			return NULL;
>
> At first look, I wonder why __alloc_frozen_pages_noprof() needs to care
> about alloc_nolock_allowed(). But the patch's idea is to centralize all
> allocation policies, so it makes sense.
>
> Ideally, I would want alloc_frozen_pages_nolock_noprof() to filter as
> much as possible, so that __alloc_frozen_pages_noprof() has minimal/no
> awareness of ALLOC_NOLOCK. But ALLOC_NOLOCK has different preferences
> compared to the default __alloc_frozen_pages_noprof() policy like
> ALLOC_WMARK_MIN vs ALLOC_WMARK_LOW, skip slowpath, and more. Maybe we
> could do something like:
>
> __alloc_frozen_pages_noprof()
> {
>      alloc_fastpath();
>      alloc_slowpath();
> }
>
> alloc_frozen_pages_nolock_noprof()
> {
>     alloc_order_allowed();
>     alloc_nolock_allow();
>     alloc_fastpath();
> }
>
> But it still cannot remove ALLOC_NOLOCK completely from
> __alloc_frozen_pages_noprof(), like the nofragment skip. Anyway, this
> patch is a reasonable cleanup. Thanks.

Yeah, thanks this does look sensible but I think it's a separate
cleanup.