Re: [PATCH -next v2 2/7] mm: damon: introduce damon_get_folio()

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

On 2022/12/28 3:42, SeongJae Park wrote:
> Hi Kefeng,
> 
> Could we use 'mm/damon:' prefix instead of 'mm: damon:' for the patch subjects?

Sure.

> 
> On Tue, 27 Dec 2022 20:27:09 +0800 Kefeng Wang <wangkefeng.wang@huawei.com> wrote:
> 
>> Introduce damon_get_folio(), and the temporary wrapper function
>> damon_get_page(), which help us to convert damon related function
>> to use folios, and it will be droped once the conversion is completed.
>>
>> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
>> ---
>>   mm/damon/ops-common.c | 14 ++++++++------
>>   mm/damon/ops-common.h |  8 +++++++-
>>   2 files changed, 15 insertions(+), 7 deletions(-)
>>
>> diff --git a/mm/damon/ops-common.c b/mm/damon/ops-common.c
>> index 75409601f934..ff38a19aa92e 100644
>> --- a/mm/damon/ops-common.c
>> +++ b/mm/damon/ops-common.c
>> @@ -16,21 +16,23 @@
>>    * Get an online page for a pfn if it's in the LRU list.  Otherwise, returns
>>    * NULL.
>>    *
>> - * The body of this function is stolen from the 'page_idle_get_page()'.  We
>> + * The body of this function is stolen from the 'page_idle_get_folio()'.  We
>>    * steal rather than reuse it because the code is quite simple.
>>    */
>> -struct page *damon_get_page(unsigned long pfn)
>> +struct folio *damon_get_folio(unsigned long pfn)
>>   {
>>   	struct page *page = pfn_to_online_page(pfn);
>> +	struct folio *folio;
>>   
>>   	if (!page || !PageLRU(page) || !get_page_unless_zero(page))
>>   		return NULL;
>>   
>> -	if (unlikely(!PageLRU(page))) {
>> -		put_page(page);
>> -		page = NULL;
>> +	folio = page_folio(page);
>> +	if (unlikely(!folio_test_lru(folio))) {
>> +		folio_put(folio);
>> +		folio = NULL;
>>   	}
> 
> I think Matthew's comment for 'page_idle_get_folio()'[1] could applied here?
> 

Will change too.

> 
> [1] https://lore.kernel.org/damon/Y6s2HPjrON2Sx%2Fgr@casper.infradead.org/
> 
>> -	return page;
>> +	return folio;
>>   }
>>   
>>   void damon_ptep_mkold(pte_t *pte, struct mm_struct *mm, unsigned long addr)
>> diff --git a/mm/damon/ops-common.h b/mm/damon/ops-common.h
>> index 8d82d3722204..4ee607813981 100644
>> --- a/mm/damon/ops-common.h
>> +++ b/mm/damon/ops-common.h
>> @@ -7,7 +7,13 @@
>>   
>>   #include <linux/damon.h>
>>   
>> -struct page *damon_get_page(unsigned long pfn);
>> +struct folio *damon_get_folio(unsigned long pfn);
>> +static inline struct page *damon_get_page(unsigned long pfn)
>> +{
>> +	struct folio *folio = damon_get_folio(pfn);
>> +
>> +	return &folio->page;
> 
> I think we should check if folio is NULL before dereferencing it?

I could add comment here, thanks.

> 
>> +}
>>   
>>   void damon_ptep_mkold(pte_t *pte, struct mm_struct *mm, unsigned long addr);
>>   void damon_pmdp_mkold(pmd_t *pmd, struct mm_struct *mm, unsigned long addr);
>> -- 
>> 2.35.3
>>