2023-03-02 14:29:20

by Kefeng Wang

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

Omit two lines by converting if(!folio_isolate_lru()) to
if(folio_isolate_lru()).

Signed-off-by: Kefeng Wang <[email protected]>
---
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);
}
applied = reclaim_pages(&folio_list);
--
2.35.3

2023-03-02 16:47:15

by SeongJae Park

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

Hi Kefeng,

On Thu, 2 Mar 2023 22:49:26 +0800 Kefeng Wang <[email protected]> wrote:

> Omit two lines by converting if(!folio_isolate_lru()) to
> if(folio_isolate_lru()).
>
> Signed-off-by: Kefeng Wang <[email protected]>
> ---
> 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
>
>

2023-03-03 01:43:03

by Kefeng Wang

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 <[email protected]> wrote:
>
>> Omit two lines by converting if(!folio_isolate_lru()) to
>> if(folio_isolate_lru()).
>>
>> Signed-off-by: Kefeng Wang <[email protected]>
>> ---
>> 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
>>
>>