Re: [PATCH 2/2] mm/page_alloc: rename FPI_TRYLOCK -> FPI_NOLOCK
"Vlastimil Babka (SUSE)" <[email protected]>
| Newsgroups | dev.linux.lists.linux-rt-devel,org.kernel.vger.linux-kernel,org.kvack.linux-mm |
|---|---|
| Message-ID | <[email protected]> |
On 7/13/26 15:30, Brendan Jackman wrote: > 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/[email protected]/ >>>> Signed-off-by: Brendan Jackman <[email protected]> >>> >>> 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/[email protected]/ > > Yeah I also dunno what's best here. I guess this is a decision for Vlastimil? Let's keep the FPI_NOLOCK rename then, but separately from the fix. > > 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 ALLOC_NOLOCK is better to me than ALLOC_TRYLOCK. > 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.