Re: [PATCH 1/2] mm: convert clear_huge_page() to clear_large_folio()

[Kefeng Wang <wangkefeng.wang@huawei.com>]

On 2024/6/13 22:51, Matthew Wilcox wrote:
> On Thu, Jun 13, 2024 at 06:53:43PM +0800, Kefeng Wang wrote:
>> +++ b/include/linux/mm.h
>> @@ -4071,9 +4071,7 @@ enum mf_action_page_type {
>>   };
>>   
>>   #if defined(CONFIG_TRANSPARENT_HUGEPAGE) || defined(CONFIG_HUGETLBFS)
>> -extern void clear_huge_page(struct page *page,
>> -			    unsigned long addr_hint,
>> -			    unsigned int pages_per_huge_page);
>> +void clear_large_folio(struct folio *folio, unsigned long addr_hint);
> 
> I think this is properly called
> 
> void folio_zero_user(struct folio *folio, unsigned long user_addr);

OK,

> 
>> -void clear_huge_page(struct page *page,
>> -		     unsigned long addr_hint, unsigned int pages_per_huge_page)
>> +void clear_large_folio(struct folio *folio, unsigned long addr_hint)
>>   {
>> +	unsigned int pages_per_huge_page = folio_nr_pages(folio);
> 
> I think it's worth renaming to nr_pages here. >
>>   	unsigned long addr = addr_hint &
>>   		~(((unsigned long)pages_per_huge_page << PAGE_SHIFT) - 1);
> 
> This can just be:
> 
> 	unsigned long addr = user_addr & ~(folio_size(folio) - 1);
> 
> Umm.  Except this assumes that the folio is always mapped to userspace
> at its natural alignment.  And that's true for hugetlb, but not true
> for THP.  So I think this needs to be moved into the callers for which
> it is true, and we need documentation that user_addr is expected to be
> the base address that the folio is mapped at.
> 

It's better to move it into the callers, but one more question, the
user_addr is that the app will access, or the base addres if we can't
know it, so even in hugetlb, we should use vmf->real_address in
hugetlb_no_page(), also Cced Huang Ying, correct me if I'm wrong.

Thanks.