Re: [PATCH] mm/hugetlb: Convert &folio->page to folio_page(folio, 0)

[David Hildenbrand <david@redhat.com>]

On 09.04.25 05:15, Matthew Wilcox wrote:
> On Tue, Apr 08, 2025 at 05:49:10PM -0700, nifan.cxl@gmail.com wrote:
>> From: Fan Ni <fan.ni@samsung.com>
>>
>> Convert the use of &folio->page to folio_page(folio, 0) where struct
>> filio fits in. This is part of the efforts to move some fields out of
>> struct page to reduce its size.
> 
> Thanks for sending the patch.  You've mixed together quite a few things;
> I'd suggest focusing on one API at a time.

Agreed.

> 
>>   		folio_get(folio);
>> -		folio_add_file_rmap_pmd(folio, &folio->page, vma);
>> +		folio_add_file_rmap_pmd(folio, folio_page(folio, 0), vma);
>>   		add_mm_counter(mm, mm_counter_file(folio), HPAGE_PMD_NR);
> 
> I think this is fine, but would defer to David Hildenbrand.

For now this should be fine. We want a pointer at the actual first page. 
In some future (with folios spanning multiple PMDs), this will not be 
correct.

But the THP changes should *absolutely* not be included in this hugetlb 
patch. I was severly confused staring at the usage of 
folio_add_file_rmap_pmd() in hugetlb context/

Actually, having to go  back to my comments below to fix them up now 
that I see that this is

  mm/huge_memory.c     | 30 +++++++++++++++++-------------
  mm/hugetlb.c         | 20 +++++++++++---------
  mm/hugetlb_vmemmap.c | 12 ++++++------

Makes me angry.

> 
>>   		folio_get(folio);
>> -		folio_add_file_rmap_pud(folio, &folio->page, vma);
>> +		folio_add_file_rmap_pud(folio, folio_page(folio, 0), vma);
>>   		add_mm_counter(mm, mm_counter_file(folio), HPAGE_PUD_NR);
> 
> If that is fine, then so is this (put them in the same patchset).
> 
>>   		spin_unlock(ptl);
>> -		if (flush_needed)
>> -			tlb_remove_page_size(tlb, &folio->page, HPAGE_PMD_SIZE);
>> +		if (flush_needed) {
>> +			tlb_remove_page_size(tlb, folio_page(folio, 0),
>> +					     HPAGE_PMD_SIZE);
>> +		}
> 
> You don't need to add the extra braces here.  I haven't looked into this
> family of APIs; not sure if we should be passing the folio here or
> if it should be taking a folio argument.
> 
>>   		if (folio_maybe_dma_pinned(src_folio) ||
>> -		    !PageAnonExclusive(&src_folio->page)) {
>> +		    !PageAnonExclusive(folio_page(src_folio, 0))) {
>>   			err = -EBUSY;
> 
> mmm.  Another David question.

For now this should be correct. (first page mapped by the PMD stores the 
flag)

> 
>>   	for (i = new_nr_pages; i < nr_pages; i += new_nr_pages) {
>> -		struct page *new_head = &folio->page + i;
>> +		struct page *new_head = folio_page(folio, i);
>>   
> 
> This is definitely the right thing to do.
> 
>> @@ -3403,7 +3405,7 @@ static void __split_folio_to_order(struct folio *folio, int old_order,
>>   	if (new_order)
>>   		folio_set_order(folio, new_order);
>>   	else
>> -		ClearPageCompound(&folio->page);
>> +		ClearPageCompound(folio_page(folio, 0));
>>   }
> 
> I might be inclined to leave this one alone; this whole function needs
> to be rewritten as part of the folio split.
> 
>>   		folio_split_memcg_refs(folio, old_order, split_order);
>> -		split_page_owner(&folio->page, old_order, split_order);
>> +		split_page_owner(folio_page(folio, 0), old_order, split_order);
>>   		pgalloc_tag_split(folio, old_order, split_order);
> 
> Not sure if split_folio_owner is something that should exist.  Haven't
> looked into it.
> 
>>   		 */
>> -		free_page_and_swap_cache(&new_folio->page);
>> +		free_page_and_swap_cache(folio_page(new_folio, 0));
>>   	}
> 
> free_page_and_swap_cache() should be converted to be
> free_folio_and_swap_cache().
> 
>>   
>> -	return __folio_split(folio, new_order, &folio->page, page, list, true);
>> +	return __folio_split(folio, new_order, folio_page(folio, 0), page,
>> +			     list, true);
>>   }
> 
> Probably right.
> 
>>   {
>> -	return __folio_split(folio, new_order, split_at, &folio->page, list,
>> -			false);
>> +	return __folio_split(folio, new_order, split_at, folio_page(folio, 0),
>> +			     list, false);
>>   }
> 
> Ditto.
> 
>>   
>> -	return split_huge_page_to_list_to_order(&folio->page, list, ret);
>> +	return split_huge_page_to_list_to_order(folio_page(folio, 0), list,
>> +						ret);
>>   }
> 
> Ditto.
> 
>>   
>> -		if (is_migrate_isolate_page(&folio->page))
>> +		if (is_migrate_isolate_page(folio_page(folio, 0)))
>>   			continue;
> 
> I think we need an is_migrate_isolate_folio() instead of this.
> 
>>   	if (folio_test_anon(folio))
>> -		__ClearPageAnonExclusive(&folio->page);
>> +		__ClearPageAnonExclusive(folio_page(folio, 0));
>>   	folio->mapping = NULL;
> 
> ... David.

See above.

> 
>>   
>> -		split_page_owner(&folio->page, huge_page_order(src), huge_page_order(dst));
>> +		split_page_owner(folio_page(folio, 0), huge_page_order(src),
>> +				 huge_page_order(dst));
> 
> See earlier.
> 
>>   	if (folio_mapcount(old_folio) == 1 && folio_test_anon(old_folio)) {
>> -		if (!PageAnonExclusive(&old_folio->page)) {
>> +		if (!PageAnonExclusive(folio_page(old_folio, 0))) {
>>   			folio_move_anon_rmap(old_folio, vma);
>> -			SetPageAnonExclusive(&old_folio->page);
>> +			SetPageAnonExclusive(folio_page(old_folio, 0));
>>   		}
> 
> David.

See above.

> 
>>   	}
>>   	VM_BUG_ON_PAGE(folio_test_anon(old_folio) &&
>> -		       PageAnonExclusive(&old_folio->page), &old_folio->page);
>> +		       PageAnonExclusive(folio_page(old_folio, 0)),
>> +		       folio_page(old_folio, 0));
> 
> The PageAnonExclusive() part of this change is for David to comment on,
> but this should be a VM_BUG_ON_FOLIO() instead of calling folio_page()
> to keep this a VM_BUG_ON_PAGE().

Agreed.



-- 
Cheers,

David / dhildenb