Re: [RFC PATCH v2 01/18] mm: free PG_rcu_free folios only after an RCU grace period

[David Hildenbrand (Arm)]

On 11/6/26 11:23, Kiryl Shutsemau wrote:
> On Thu, Nov 05, 2026 at 09:37:31PM +0100, David Hildenbrand (Arm) wrote:
>>> diff --git a/mm/Kconfig b/mm/Kconfig
>>> index 604c58199acb..6e5341b0bf64 100644
>>> --- a/mm/Kconfig
>>> +++ b/mm/Kconfig
>>> @@ -718,6 +718,21 @@ config PCP_BATCH_SCALE_MAX
>>>  	  latency.  This option sets the upper limit of scale factor to limit
>>>  	  the maximum latency.
>>>  
>>> +config PAGE_RCU_FREE
>>
>> I think we really want to focus this only on folios (like the current code does).
>>
>> FOLIO_RCU_FREE ?
> 
> Sure.
> 
>>> +	def_bool y
>>> +	depends on 64BIT
>>> +	depends on !PREEMPT_RT
>>> +	help
>>> +	  Reads from the page cache can copy from folios that are read
>>> +	  again and again without taking a reference on them, so small
>>> +	  reads of one file from many CPUs do not contend on the folio's
>>> +	  reference count.  The cost is that memory freed from such
>>> +	  folios becomes reusable only after an RCU grace period.
>>
>> While that description is helpful, that's not really what this patch, or rather
>> the basic infrastructure in page_alloc, does.
>>
>> I'd suggest to have a dedicated FOLIO_RCU_FREE config and maybe another one
>> (that selects/depends on) FOLIO_RCU_FREE that actually enables the pagecache use
>> and documents the details.
> 
> I will make the help describe what this patch does.  The page cache is
> the only user, and both symbols would be def_bool without a prompt, so
> I would rather add the second one with a second user.
> 
> Does that work for you?
> 
>>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
>>> index 1e5d68acf62a..25082949a4ca 100644
>>> --- a/mm/huge_memory.c
>>> +++ b/mm/huge_memory.c
>>> @@ -3743,6 +3743,7 @@ static void __split_folio_to_order(struct folio *folio, int old_order,
>>>  #endif
>>>  				 (1L << PG_dirty) |
>>>  				 (1L << PG_dropbehind) |
>>> +				 __PG_RCU_FREE |
>>
>> That looks a bit odd at first, as it's very different to the other once. But no
>> easy way around assuming that the flag is not always around.
>>
>> Maybe we should just have this be a const mask constructed somewhere using BIT()
>> and __PG_RCU_FREE etc, instead of having this growing list in this user here.
> 
> Makes sense.  Something like the diff below, on top of this patch.
> Introducing split_copy_flags would be a preparatory patch, and this one
> would only add __PG_RCU_FREE to it.
> 
>>> @@ -1327,6 +1417,19 @@ static __always_inline bool __free_pages_prepare(struct page *page,
>>>  
>>>  	VM_BUG_ON_PAGE(PageTail(page), page);
>>>  
>>> +	/*
>>> +	 * Not for a hwpoisoned order-0 page: it never reaches the allocator,
>>> +	 * and page_handle_poison() takes a reference right after this free.
>>> +	 */
>>> +	if (unlikely(folio_test_rcu_free(folio)) &&
>>> +	    !(PageHWPoison(page) && !order)) {
>>> +		VM_WARN_ON_ONCE(fpi_flags & FPI_NOLOCK);
>>> +		__folio_clear_rcu_free(folio);
>>> +		count_vm_events(PGFREE_RCU, 1 << order);
>>> +		rcu_free_defer(folio);
>>> +		return false;
>>> +	}
>> I'm wondering whether we want to intercept on the folio freeing path instead?
>> That would be __folio_put(), before we go into free_frozen_pages() used by other
>> machinery that does not apply (slab etc).
> 
> Not all page cache folios are freed through __folio_put().  Reclaim
> frees them with free_unref_folios() directly (shrink_folio_list(),
> move_folios_to_lru()), and so do folios_put_refs() and the LRU batches.
> __free_pages_prepare() is the one place they all pass.

Can we just rework that code first to have some clear folio freeing path, IOW
doing this in a folio-specific helper?

folios_put_refs() is just a specialized and batched variant of the __folio_put()
freeing path in the end. So we can hook into these two easily.

For free_unref_folios() would need another hook or a cleaner rework.


> 
> diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
> index a1e52a1f8a52..6d4e534c9ccc 100644
> --- a/include/linux/page-flags.h
> +++ b/include/linux/page-flags.h
> @@ -1185,6 +1185,18 @@ static __always_inline void __ClearPageAnonExclusive(struct page *page)
>  #define __PG_MLOCKED		0
>  #endif
>  
> +#ifdef CONFIG_ARCH_USES_PG_ARCH_2
> +#define __PG_ARCH_2		BIT(PG_arch_2)
> +#else
> +#define __PG_ARCH_2		0
> +#endif
> +
> +#ifdef CONFIG_ARCH_USES_PG_ARCH_3
> +#define __PG_ARCH_3		BIT(PG_arch_3)
> +#else
> +#define __PG_ARCH_3		0
> +#endif
> +
>  /*
>   * Flags checked when a page is freed.  Pages being freed should not have
>   * these flags set.  If they are, there is a problem.
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index 25082949a4ca..44a1aaa3f834 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -3689,6 +3689,23 @@ static void __split_folio_to_order(struct folio *folio, int old_order,
>  	const bool handle_hwpoison = folio_test_has_hwpoisoned(folio) && new_order;
>  	long new_nr_pages = 1 << new_order;
>  	long nr_pages = 1 << old_order;
> +	const unsigned long split_copy_flags =
> +		BIT(PG_referenced) |
> +		BIT(PG_swapbacked) |
> +		BIT(PG_swapcache) |
> +		__PG_MLOCKED |
> +		BIT(PG_uptodate) |
> +		BIT(PG_active) |
> +		BIT(PG_workingset) |
> +		BIT(PG_locked) |
> +		BIT(PG_unevictable) |
> +		__PG_ARCH_2 |
> +		__PG_ARCH_3 |
> +		BIT(PG_dirty) |
> +		BIT(PG_dropbehind) |
> +		__PG_RCU_FREE |
> +		LRU_GEN_MASK |
> +		LRU_REFS_MASK;


I was rather thinking of a


#define FOLIO_SPLIT_FLAGS	BIT(PG_referenced) | BIT(PG_swapbacked) | \

...

To make this function actually shorter :)

(ofc, you could also have a static const unsigned long, if that's what we want)

-- 
Cheers,

David