linux-kernel - Re: [PATCH -next] mm/damon/paddr: minor refactor of damon_pa

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Fri, 3 Mar 2023 09:42:04 +0800
From:   Kefeng Wang <wangkefeng.wang@...wei.com>
To:     SeongJae Park <sj@...nel.org>
CC:     Andrew Morton <akpm@...ux-foundation.org>, <linux-mm@...ck.org>,
        <linux-kernel@...r.kernel.org>, <damon@...ts.linux.dev>
Subject: Re: [PATCH -next] mm/damon/paddr: minor refactor of
 damon_pa_pageout()



On 2023/3/3 0:47, SeongJae Park wrote:
> Hi Kefeng,
> 
> On Thu, 2 Mar 2023 22:49:26 +0800 Kefeng Wang <wangkefeng.wang@...wei.com> wrote:
> 
>> Omit two lines by converting if(!folio_isolate_lru()) to
>> if(folio_isolate_lru()).
>>
>> Signed-off-by: Kefeng Wang <wangkefeng.wang@...wei.com>
>> ---
>>   mm/damon/paddr.c | 12 +++++-------
>>   1 file changed, 5 insertions(+), 7 deletions(-)
>>
>> diff --git a/mm/damon/paddr.c b/mm/damon/paddr.c
>> index 6c655d9b5639..a557f3c9300f 100644
>> --- a/mm/damon/paddr.c
>> +++ b/mm/damon/paddr.c
>> @@ -246,14 +246,12 @@ static unsigned long damon_pa_pageout(struct damon_region *r, struct damos *s)
>>   
>>   		folio_clear_referenced(folio);
>>   		folio_test_clear_young(folio);
>> -		if (!folio_isolate_lru(folio)) {
>> -			folio_put(folio);
>> -			continue;
>> +		if (folio_isolate_lru(folio)) {
>> +			if (folio_test_unevictable(folio))
>> +				folio_putback_lru(folio);
>> +			else
>> +				list_add(&folio->lru, &folio_list);
>>   		}
>> -		if (folio_test_unevictable(folio))
>> -			folio_putback_lru(folio);
>> -		else
>> -			list_add(&folio->lru, &folio_list);
>>   		folio_put(folio);
>>   	}
> 
> This looks ok to me, thank you for your effort!  Nevertheless, I don't like
> increasing depth as much as number of lines.  The
> more-than-3-levels-of-indentation[1] warning is not always right, but this 4
> levels of indentation is not making me so exciting.
> 
> What do you think about adding put_continue label and using it, not only here,
> but also above part?  For example:
> 
>      --- a/mm/damon/paddr.c
>      +++ b/mm/damon/paddr.c
>      @@ -239,21 +239,18 @@ static unsigned long damon_pa_pageout(struct damon_region *r, struct damos *s)
>                      if (!folio)
>                              continue;
>      
>      -               if (damos_pa_filter_out(s, folio)) {
>      -                       folio_put(folio);
>      -                       continue;
>      -               }
>      +               if (damos_pa_filter_out(s, folio))
>      +                       goto put_continue;
>      
>                      folio_clear_referenced(folio);
>                      folio_test_clear_young(folio);
>      -               if (!folio_isolate_lru(folio)) {
>      -                       folio_put(folio);
>      -                       continue;
>      -               }
>      +               if (!folio_isolate_lru(folio))
>      +                       goto put_continue;
>                      if (folio_test_unevictable(folio))
>                              folio_putback_lru(folio);
>                      else
>                              list_add(&folio->lru, &folio_list);
>      +put_continue:
>                      folio_put(folio);
>              }
>              applied = reclaim_pages(&folio_list);
> 
> It omits three lines.

It looks good, will update, thanks

> 
>      $ git diff --stat
>       mm/damon/paddr.c | 13 +++++--------
>       1 file changed, 5 insertions(+), 8 deletions(-)
> 
> [1] https://docs.kernel.org/process/coding-style.html#indentation
> 
> 
> Thanks,
> SJ
> 
>>   	applied = reclaim_pages(&folio_list);
>> -- 
>> 2.35.3
>>
>>