Re: [RFC PATCH v5 01/15] mm: Consolidate freeing of typed folios on final folio_put()

[David Hildenbrand <david@redhat.com>]

On 17.01.25 23:05, Elliot Berman wrote:
> On Fri, Jan 17, 2025 at 04:29:47PM +0000, Fuad Tabba wrote:
>> Some folio types, such as hugetlb, handle freeing their own
>> folios. Moreover, guest_memfd will require being notified once a
>> folio's reference count reaches 0 to facilitate shared to private
>> folio conversion, without the folio actually being freed at that
>> point.
>>
>> As a first step towards that, this patch consolidates freeing
>> folios that have a type. The first user is hugetlb folios. Later
>> in this patch series, guest_memfd will become the second user of
>> this.
>>
>> Suggested-by: David Hildenbrand <david@redhat.com>
>> Signed-off-by: Fuad Tabba <tabba@google.com>
>> ---
>>   include/linux/page-flags.h | 15 +++++++++++++++
>>   mm/swap.c                  | 24 +++++++++++++++++++-----
>>   2 files changed, 34 insertions(+), 5 deletions(-)
>>
>> diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
>> index 691506bdf2c5..6615f2f59144 100644
>> --- a/include/linux/page-flags.h
>> +++ b/include/linux/page-flags.h
>> @@ -962,6 +962,21 @@ static inline bool page_has_type(const struct page *page)
>>   	return page_mapcount_is_type(data_race(page->page_type));
>>   }
>>   
>> +static inline int page_get_type(const struct page *page)
>> +{
>> +	return page->page_type >> 24;
>> +}
>> +
>> +static inline bool folio_has_type(const struct folio *folio)
>> +{
>> +	return page_has_type(&folio->page);
>> +}
>> +
>> +static inline int folio_get_type(const struct folio *folio)
>> +{
>> +	return page_get_type(&folio->page);
>> +}
>> +
>>   #define FOLIO_TYPE_OPS(lname, fname)					\
>>   static __always_inline bool folio_test_##fname(const struct folio *folio) \
>>   {									\
>> diff --git a/mm/swap.c b/mm/swap.c
>> index 10decd9dffa1..6f01b56bce13 100644
>> --- a/mm/swap.c
>> +++ b/mm/swap.c
>> @@ -94,6 +94,20 @@ static void page_cache_release(struct folio *folio)
>>   		unlock_page_lruvec_irqrestore(lruvec, flags);
>>   }
>>   
>> +static void free_typed_folio(struct folio *folio)
>> +{
>> +	switch (folio_get_type(folio)) {
> 
> I think you need:
> 
> +#if IS_ENABLED(CONFIG_HUGETLBFS)
>> +	case PGTY_hugetlb:
>> +		free_huge_folio(folio);
>> +		return;
> +#endif
> 
> I think this worked before because folio_test_hugetlb was defined by:
> FOLIO_TEST_FLAG_FALSE(hugetlb)
> and evidently compiler optimizes out the free_huge_folio(folio) before
> linking.

Likely, we should be using

	case PGTY_hugetlb:
		if(IF_ENABLED(CONFIG_HUGETLBFS))
			free_huge_folio(folio);
		return:

if possible (I assume so).

-- 
Cheers,

David / dhildenb