Re: [PATCH v2 2/3] mm/damon/paddr: minor refactor of damon_pa_young()

From: Kefeng Wang
Date: Tue Mar 07 2023 - 20:03:30 EST




On 2023/3/8 2:00, SeongJae Park wrote:
On Tue, 7 Mar 2023 09:22:33 +0800 Kefeng Wang <wangkefeng.wang@xxxxxxxxxx> wrote:



On 2023/3/7 5:27, SeongJae Park wrote:
Hi Kefeng,

On Mon, 6 Mar 2023 09:56:49 +0800 Kefeng Wang <wangkefeng.wang@xxxxxxxxxx> wrote:



On 2023/3/6 9:10, Kefeng Wang wrote:


On 2023/3/4 2:39, SeongJae Park wrote:
Hi Kefeng,

On Fri, 3 Mar 2023 16:43:42 +0800 Kefeng Wang
<wangkefeng.wang@xxxxxxxxxx> wrote:

Omit three lines by unified folio_put(), and make code more clear.

Signed-off-by: Kefeng Wang <wangkefeng.wang@xxxxxxxxxx>
---
mm/damon/paddr.c | 11 ++++-------
1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/mm/damon/paddr.c b/mm/damon/paddr.c
index 3fda00a0f786..2ef9db0189ca 100644
--- a/mm/damon/paddr.c
+++ b/mm/damon/paddr.c
@@ -130,24 +130,21 @@ static bool damon_pa_young(unsigned long paddr,
unsigned long *folio_sz)
accessed = false;
else
accessed = true;
- folio_put(folio);
goto out;

Because you moved 'out' label to not include *folio_sz setting,
folio_sz will
not set in this case. It should be set.
oh, it should be fixed.

}
need_lock = !folio_test_anon(folio) || folio_test_ksm(folio);
- if (need_lock && !folio_trylock(folio)) {
- folio_put(folio);
- return false;
- }

Hi SJ, apart from above issue, it looks that this branch need the
folio_size() setting, right?

folio_sz is effectively used by caller of damon_pa_young() only if this
function returns true, so this branch doesn't need to set folio_sz.

__damon_pa_check_access() store last_addr, last_accessed and
last_folio_sz, even damon_pa_young() return false, the following check
still use last_folio_sz,

ALIGN_DOWN(last_addr, last_folio_sz) == ALIGN_DOWN(r->sampling_addr,
last_folio_sz)

but last_folio_sz is not up to date, so I think it need to update, and
update last_folio_sz is harmless, which could let's unify the return
path, correct me if I am wrong.

Ah, you're right. Thank you for kind explanation. I was out of my mind for
some reason. Maybe we could just do 'goto out' in the branch.

Yes, will update this patchset with this change.


Thanks,
SJ