Re: [PATCH 2/2] mm/page_alloc: rename FPI_TRYLOCK -> FPI_NOLOCK

["Zi Yan" <ziy@nvidia.com>]

On Fri Jul 10, 2026 at 8:40 AM EDT, Vlastimil Babka (SUSE) wrote:
> On 7/10/26 12:42, Brendan Jackman wrote:
>> As discussed in the linked patch, the there is some inconsistency between
>> "trylock" and "nolock" nomenclature, let's align it. Since "nolock" is
>> used in the public API it seems to have more mindshare so do that.
>> 
>> The linked patch did this for the ALLOC_ flag but forgot about FPI_.
>> 
>> Link: https://lore.kernel.org/all/20260703-alloc-trylock-v5-1-c87b714e19d3@google.com/
>> Signed-off-by: Brendan Jackman <jackmanb@google.com>
>
> Naming things is hard. Maybe it should have all been called "nospin". I
> don't know anymore :)
> _nolock() functions and ALLOC_NOLOCK are part of API, FPI_ is internal so
> it's not that urgent. Furthermore:

I had a similar concern when reading ALLOC_TRYLOCK -> ALLOC_NOLOCK[1],
since the name is _NOLOCK, but the comment says spin_trylock.

I agree that "nospin" is better and less confusing. But whether we want
to churn it again, TBD. :)

[1] https://lore.kernel.org/all/DJOZN5H048UX.1ZVSFD48QZN05@nvidia.com/
>
>> ---
>>  mm/page_alloc.c | 18 +++++++++---------
>>  1 file changed, 9 insertions(+), 9 deletions(-)
>> 
>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>> index 5fe1c11f919d7..ba8d882072de5 100644
>> --- a/mm/page_alloc.c
>> +++ b/mm/page_alloc.c
>> @@ -90,7 +90,7 @@ typedef int __bitwise fpi_t;
>>  #define FPI_TO_TAIL		((__force fpi_t)BIT(1))
>>  
>>  /* Free the page without taking locks. Rely on trylock only. */
>
> here's a "trylock"
>
>> -#define FPI_TRYLOCK		((__force fpi_t)BIT(2))
>> +#define FPI_NOLOCK		((__force fpi_t)BIT(2))
>
> And here's not anymore.
>
>>  
>>  /* free_pages_prepare() has already been called for page(s) being freed. */
>>  #define FPI_PREPARED		((__force fpi_t)BIT(3))
>> @@ -1419,7 +1419,7 @@ static __always_inline bool __free_pages_prepare(struct page *page,
>>  	page_table_check_free(page, order);
>>  	pgalloc_tag_sub(page, 1 << order);
>>  
>> -	if (!PageHighMem(page) && !(fpi_flags & FPI_TRYLOCK)) {
>> +	if (!PageHighMem(page) && !(fpi_flags & FPI_NOLOCK)) {
>>  		debug_check_no_locks_freed(page_address(page),
>>  					   PAGE_SIZE << order);
>>  		debug_check_no_obj_freed(page_address(page),
>> @@ -1558,7 +1558,7 @@ static void free_one_page(struct zone *zone, struct page *page,
>>  	struct llist_head *llhead;
>>  	unsigned long flags;
>>  
>> -	if (unlikely(fpi_flags & FPI_TRYLOCK)) {
>> +	if (unlikely(fpi_flags & FPI_NOLOCK)) {
>>  		if (!spin_trylock_irqsave(&zone->lock, flags)) {
>>  			add_page_to_zone_llist(zone, page, order);
>>  			return;
>> @@ -1569,7 +1569,7 @@ static void free_one_page(struct zone *zone, struct page *page,
>>  
>>  	/* The lock succeeded. Process deferred pages. */
>>  	llhead = &zone->trylock_free_pages;
>> -	if (unlikely(!llist_empty(llhead) && !(fpi_flags & FPI_TRYLOCK))) {
>> +	if (unlikely(!llist_empty(llhead) && !(fpi_flags & FPI_NOLOCK))) {
>>  		struct llist_node *llnode;
>>  		struct page *p, *tmp;
>>  
>> @@ -2882,7 +2882,7 @@ static bool free_frozen_page_commit(struct zone *zone,
>>  	if (pcp->free_count < (batch << CONFIG_PCP_BATCH_SCALE_MAX))
>>  		pcp->free_count += (1 << order);
>>  
>> -	if (unlikely(fpi_flags & FPI_TRYLOCK)) {
>> +	if (unlikely(fpi_flags & FPI_NOLOCK)) {
>>  		/*
>>  		 * Do not attempt to take a zone lock. Let pcp->count get
>>  		 * over high mark temporarily.
>> @@ -2979,7 +2979,7 @@ static void __free_frozen_pages(struct page *page, unsigned int order,
>>  		migratetype = MIGRATE_MOVABLE;
>>  	}
>>  
>> -	if (unlikely((fpi_flags & FPI_TRYLOCK) && !can_spin_trylock())) {
>> +	if (unlikely((fpi_flags & FPI_NOLOCK) && !can_spin_trylock())) {
>
> can_spin_trylock() was matched with FPI_TRYLOCK, now not anymore
>
>>  		add_page_to_zone_llist(zone, page, order);
>>  		return;
>>  	}
>> @@ -3001,7 +3001,7 @@ void free_frozen_pages(struct page *page, unsigned int order)
>>  
>>  void free_frozen_pages_nolock(struct page *page, unsigned int order)
>>  {
>> -	__free_frozen_pages(page, order, FPI_TRYLOCK);
>> +	__free_frozen_pages(page, order, FPI_NOLOCK);
>>  }
>>  
>>  /*
>> @@ -5409,7 +5409,7 @@ struct page *__alloc_frozen_pages_noprof(gfp_t gfp, unsigned int order,
>>  	if (memcg_kmem_online() && (gfp & __GFP_ACCOUNT) && page &&
>>  	    unlikely(__memcg_kmem_charge_page(page, gfp, order) != 0)) {
>>  		__free_frozen_pages(page, order,
>> -				    alloc_flags & ALLOC_NOLOCK ? FPI_TRYLOCK : 0);
>> +				    alloc_flags & ALLOC_NOLOCK ? FPI_NOLOCK : 0);
>
> Although here it does improve things. Sigh.
>
>>  		page = NULL;
>>  	}
>>  
>> @@ -5532,7 +5532,7 @@ EXPORT_SYMBOL(__free_pages);
>>   */
>>  void free_pages_nolock(struct page *page, unsigned int order)
>>  {
>> -	___free_pages(page, order, FPI_TRYLOCK);
>> +	___free_pages(page, order, FPI_NOLOCK);
>>  }
>>  
>>  /**
>> 




-- 
Best Regards,
Yan, Zi