| Message ID | 20230302144926.40012-1-wangkefeng.wang@huawei.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [-next] mm/damon/paddr: minor refactor of damon_pa_pageout() | expand |
Hi Kefeng, On Thu, 2 Mar 2023 22:49:26 +0800 Kefeng Wang <wangkefeng.wang@huawei.com> wrote: > Omit two lines by converting if(!folio_isolate_lru()) to > if(folio_isolate_lru()). > > Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.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. $ 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 > >
On 2023/3/3 0:47, SeongJae Park wrote: > Hi Kefeng, > > On Thu, 2 Mar 2023 22:49:26 +0800 Kefeng Wang <wangkefeng.wang@huawei.com> wrote: > >> Omit two lines by converting if(!folio_isolate_lru()) to >> if(folio_isolate_lru()). >> >> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.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 >> >>
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); } applied = reclaim_pages(&folio_list);
Omit two lines by converting if(!folio_isolate_lru()) to if(folio_isolate_lru()). Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> --- mm/damon/paddr.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-)