Re: [PATCH v1 6/6] mm/hugetlb: use folio->lru int demote_free_hugetlb_folios()

[David Hildenbrand <david@redhat.com>]

On 10.01.25 19:59, Matthew Wilcox wrote:
> On Fri, Jan 10, 2025 at 07:21:49PM +0100, David Hildenbrand wrote:
>> We are demoting hugetlb folios to smaller hugetlb folios; let's avoid
>> messing with pages where avoidable.
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
> 
> Good stuff.  I have questions.
> 
>> +++ b/mm/hugetlb.c
>> @@ -3822,13 +3822,15 @@ static long demote_free_hugetlb_folios(struct hstate *src, struct hstate *dst,
>>   
>>   		for (i = 0; i < pages_per_huge_page(src); i += pages_per_huge_page(dst)) {
>>   			struct page *page = folio_page(folio, i);
>> +			struct folio *new_folio;
> 
> I'm usually very against casting from page to folio, but I think it
> might be the better option in this case ...
> 
>>   			page->mapping = NULL;
> 
> because then we could do new_folio->mapping = NULL.
> 
> We're going to have to do serious changes to this function anyway to
> convert from Ottawa to the New York interpretation, so the cast doesn't
> give me the feeling of danger that it would elsewhere.

Hm, that makes me wonder if we should do it even more similar like our
other split function (__split_huge_page_tail)?


diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 60617eecb99dd..23fe5654f632c 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -3821,14 +3821,18 @@ static long demote_free_hugetlb_folios(struct hstate *src, struct hstate *dst,
                 pgalloc_tag_split(folio, huge_page_order(src), huge_page_order(dst));
  
                 for (i = 0; i < pages_per_huge_page(src); i += pages_per_huge_page(dst)) {
                         struct page *page = folio_page(folio, i);
+                       /* Careful: see __split_huge_page_tail() */
+                       struct folio *new_folio = (struct folio *)page;
  
-                       page->mapping = NULL;
                         clear_compound_head(page);
                         prep_compound_page(page, dst->order);
  
-                       init_new_hugetlb_folio(dst, page_folio(page));
-                       list_add(&page->lru, &dst_list);
+                       new_folio->mapping = NULL;
+                       init_new_hugetlb_folio(dst, new_folio);
+                       list_add(&new_folio->lru, &dst_list);
                 }
         }
  

I was even wondering if we should be using nth_page() instead of folio_page() --
similar to __split_huge_page_tail.

If we'd add sanity checking in current code to folio_page() to verify that i
falls inside the folio, the current code would blow up as we modify the
folio using prep_compound_page().

-- 
Cheers,

David / dhildenb