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

[Brendan Jackman]

On Fri Jul 10, 2026 at 2:14 PM UTC, Zi Yan wrote:
> 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@xxxxxxxxxx/
>>> Signed-off-by: Brendan Jackman <jackmanb@xxxxxxxxxx>
>>
>> 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.

Yeah I do actually think "nolock" is a bad name here, it takes a lock.
But I never cared very much about the _bad_ ame. On the other hand
_inconsistent_ naming is a concrete problem IMO.

> 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@xxxxxxxxxx/

Yeah I also dunno what's best here. I guess this is a decision for Vlastimil?

>>> ---
>>>  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"

This is accurate though. It's talking about the implementation which
calls spin_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

IMO can_spin_trylock() is matched with spin_trlock() while FPI_NOLOCK is
matched with ALLOC_NOLOCK which is matched with alloc_pages_nolock().

If you like, we could just drop the ALLOC_ and FPI_ renames and just
rename alloc_pages_nolock(). I steered away from that because "the
latter is public API", but... it's not like it would be a huge treewide
patch, it only has one user.