[v2] mm: huge_memory: convert split_huge_pages_all() to use a folio

  Straightforwardly convert split_huge_pages_all() to use a folio.

Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
---
- adjust indenting of code pointed by Matthew

 mm/huge_memory.c | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)
  

On Fri, 30 Dec 2022 17:30:20 +0800 Kefeng Wang <wangkefeng.wang@huawei.com> wrote:

> Straightforwardly convert split_huge_pages_all() to use a folio.
> 
> ...
>
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -2932,6 +2932,7 @@ static void split_huge_pages_all(void)
>  {
>  	struct zone *zone;
>  	struct page *page;
> +	struct folio *folio;
>  	unsigned long pfn, max_zone_pfn;
>  	unsigned long total = 0, split = 0;
>  
> @@ -2944,24 +2945,32 @@ static void split_huge_pages_all(void)
>  			int nr_pages;
>  
>  			page = pfn_to_online_page(pfn);
> -			if (!page || !get_page_unless_zero(page))
> +			if (!page || PageTail(page))
> +				continue;
> +			folio = page_folio(page);
> +			if (!folio_try_get(folio))
>  				continue;
>  
> -			if (zone != page_zone(page))
> +			if (unlikely(page_folio(page) != folio))
> +				goto next;
> +
> +			if (zone != folio_zone(folio))
>  				goto next;

I'm still not understanding the above hunk.  Why is the
"page_folio(page) != folio" check added?  Should it be commented?


> -			if (!PageHead(page) || PageHuge(page) || !PageLRU(page))
> +			if (!folio_test_large(folio) ||
> +			    folio_test_hugetlb(folio) ||
> +			    !folio_test_lru(folio))
>  				goto next;
>  
>  			total++;
> -			lock_page(page);
> -			nr_pages = thp_nr_pages(page);
> -			if (!split_huge_page(page))
> +			folio_lock(folio);
> +			nr_pages = folio_nr_pages(folio);
> +			if (!split_folio(folio))
>  				split++;
>  			pfn += nr_pages - 1;
> -			unlock_page(page);
> +			folio_unlock(folio);
>  next:
> -			put_page(page);
> +			folio_put(folio);
>  			cond_resched();
>  		}
>  	}
> -- 
> 2.35.3
  

On 2022/12/31 5:45, Andrew Morton wrote:
> On Fri, 30 Dec 2022 17:30:20 +0800 Kefeng Wang <wangkefeng.wang@huawei.com> wrote:
> 
>> Straightforwardly convert split_huge_pages_all() to use a folio.
>>
>> ...
>>
>> --- a/mm/huge_memory.c
>> +++ b/mm/huge_memory.c
>> @@ -2932,6 +2932,7 @@ static void split_huge_pages_all(void)
>>   {
>>   	struct zone *zone;
>>   	struct page *page;
>> +	struct folio *folio;
>>   	unsigned long pfn, max_zone_pfn;
>>   	unsigned long total = 0, split = 0;
>>   
>> @@ -2944,24 +2945,32 @@ static void split_huge_pages_all(void)
>>   			int nr_pages;
>>   
>>   			page = pfn_to_online_page(pfn);
>> -			if (!page || !get_page_unless_zero(page))
>> +			if (!page || PageTail(page))
>> +				continue;
>> +			folio = page_folio(page);
>> +			if (!folio_try_get(folio))
>>   				continue;
>>   
>> -			if (zone != page_zone(page))
>> +			if (unlikely(page_folio(page) != folio))
>> +				goto next;
>> +
>> +			if (zone != folio_zone(folio))
>>   				goto next;
> 
> I'm still not understanding the above hunk.  Why is the
> "page_folio(page) != folio" check added?  Should it be commented?

There is a comment in try_get_folio(), is it enough?
  

On Tue, 3 Jan 2023 09:58:36 +0800 Kefeng Wang <wangkefeng.wang@huawei.com> wrote:

> 
> 
> On 2022/12/31 5:45, Andrew Morton wrote:
> > On Fri, 30 Dec 2022 17:30:20 +0800 Kefeng Wang <wangkefeng.wang@huawei.com> wrote:
> > 
> >> Straightforwardly convert split_huge_pages_all() to use a folio.
> >>
> >> ...
> >>
> >> --- a/mm/huge_memory.c
> >> +++ b/mm/huge_memory.c
> >> @@ -2932,6 +2932,7 @@ static void split_huge_pages_all(void)
> >>   {
> >>   	struct zone *zone;
> >>   	struct page *page;
> >> +	struct folio *folio;
> >>   	unsigned long pfn, max_zone_pfn;
> >>   	unsigned long total = 0, split = 0;
> >>   
> >> @@ -2944,24 +2945,32 @@ static void split_huge_pages_all(void)
> >>   			int nr_pages;
> >>   
> >>   			page = pfn_to_online_page(pfn);
> >> -			if (!page || !get_page_unless_zero(page))
> >> +			if (!page || PageTail(page))
> >> +				continue;
> >> +			folio = page_folio(page);
> >> +			if (!folio_try_get(folio))
> >>   				continue;
> >>   
> >> -			if (zone != page_zone(page))
> >> +			if (unlikely(page_folio(page) != folio))
> >> +				goto next;
> >> +
> >> +			if (zone != folio_zone(folio))
> >>   				goto next;
> > 
> > I'm still not understanding the above hunk.  Why is the
> > "page_folio(page) != folio" check added?  Should it be commented?
> 
> There is a comment in try_get_folio(), is it enough?

Nobody would think to look at a private function in gup.c.

I suggest this folio_try_get() rule be documented at the
folio_try_get() implementation site.

Most callers do perform this check, but not the ones in vmscan.c?
  

On 2023/1/6 11:06, Andrew Morton wrote:
> On Tue, 3 Jan 2023 09:58:36 +0800 Kefeng Wang <wangkefeng.wang@huawei.com> wrote:
> 
>>
>>
>> On 2022/12/31 5:45, Andrew Morton wrote:
>>> On Fri, 30 Dec 2022 17:30:20 +0800 Kefeng Wang <wangkefeng.wang@huawei.com> wrote:
>>>
>>>> Straightforwardly convert split_huge_pages_all() to use a folio.
>>>>
>>>> ...
>>>>
>>>> --- a/mm/huge_memory.c
>>>> +++ b/mm/huge_memory.c
>>>> @@ -2932,6 +2932,7 @@ static void split_huge_pages_all(void)
>>>>    {
>>>>    	struct zone *zone;
>>>>    	struct page *page;
>>>> +	struct folio *folio;
>>>>    	unsigned long pfn, max_zone_pfn;
>>>>    	unsigned long total = 0, split = 0;
>>>>    
>>>> @@ -2944,24 +2945,32 @@ static void split_huge_pages_all(void)
>>>>    			int nr_pages;
>>>>    
>>>>    			page = pfn_to_online_page(pfn);
>>>> -			if (!page || !get_page_unless_zero(page))
>>>> +			if (!page || PageTail(page))
>>>> +				continue;
>>>> +			folio = page_folio(page);
>>>> +			if (!folio_try_get(folio))
>>>>    				continue;
>>>>    
>>>> -			if (zone != page_zone(page))
>>>> +			if (unlikely(page_folio(page) != folio))
>>>> +				goto next;
>>>> +
>>>> +			if (zone != folio_zone(folio))
>>>>    				goto next;
>>>
>>> I'm still not understanding the above hunk.  Why is the
>>> "page_folio(page) != folio" check added?  Should it be commented?
>>
>> There is a comment in try_get_folio(), is it enough?
> 
> Nobody would think to look at a private function in gup.c.

Sorry，I mean that comment is to explain why adding a new recheck.

> 
> I suggest this folio_try_get() rule be documented at the
> folio_try_get() implementation site.
> 
> Most callers do perform this check, but not the ones in vmscan.c ?

Hope Matthew could give some advise, thanks