Re: [PATCH -next] mm/damon/paddr: minor refactor of damon_pa_pageout()

[Kefeng Wang]

On 2023/3/3 0:47, SeongJae Park wrote:
> Hi Kefeng,
>
> On Thu, 2 Mar 2023 22:49:26 +0800 Kefeng Wang <wangkefeng.wang@xxxxxxxxxx> wrote:
>
> > Omit two lines by converting if(!folio_isolate_lru()) to
> > if(folio_isolate_lru()).
> >
> > Signed-off-by: Kefeng Wang <wangkefeng.wang@xxxxxxxxxx>
> > ---
> >   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