[PATCH 0/3] mm/damon/core: fix wrong and/or useless damos_walk() behaviors

[PATCH 0/3] mm/damon/core: fix wrong and/or useless damos_walk() behaviors

[SeongJae Park]

damos_walk() can finish working earlier or later than expected, and
start earlier than practical.  First two behaviors are clearly wrong
behavior (doesn't follow the documentation) and all three behaviors are
only making the feature useless.  Fix those.

SeongJae Park (3):
  mm/damon/core: unset damos->walk_completed after confimed set
  mm/damon/core: do not call damos_walk_control->walk() if walk is
    completed
  mm/damon/core: do damos walking in entire regions granularity

 mm/damon/core.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)


base-commit: 920155d8318aaec9f7c227e617bc7b399dec502e
-- 
2.39.5

[PATCH 1/3] mm/damon/core: unset damos->walk_completed after confimed set

[SeongJae Park]

damos->walk_completed is only set, not unset.  This can cause next
damos_walk() finish earlier than expected.  Unset it after all
walk_completed is confirmed.

Fixes: bf0eaba0ff9c ("mm/damon/core: implement damos_walk()")
Signed-off-by: SeongJae Park <sj@kernel.org>
---
 mm/damon/core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/mm/damon/core.c b/mm/damon/core.c
index 1a4dd644949b..1d9025d14d83 100644
--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -1489,6 +1489,9 @@ static void damos_walk_complete(struct damon_ctx *ctx, struct damos *s)
 		if (!siter->walk_completed)
 			return;
 	}
+	damon_for_each_scheme(siter, ctx)
+		siter->walk_completed = false;
+
 	complete(&control->completion);
 	mutex_lock(&ctx->walk_control_lock);
 	ctx->walk_control = NULL;
-- 
2.39.5

[PATCH 2/3] mm/damon/core: do not call damos_walk_control->walk() if walk is completed

[SeongJae Park]

damos_walk() invokes callback functions of schemes until all schemes
finishes at least one round of walks.  If there are multiple DAMOS
schemes having different apply_interval, the callback functions for
longer apply interval scheme will be called for more than a round of the
walk.

The behavior is different from the document (see damos_walk() kernel-doc
comment), and not useful.  Make the behavior be same to the documented
one, by stopping invoking the callback if the walk for the given scheme
is completed.

Fixes: bf0eaba0ff9c ("mm/damon/core: implement damos_walk()")
Signed-off-by: SeongJae Park <sj@kernel.org>
---
 mm/damon/core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/mm/damon/core.c b/mm/damon/core.c
index 1d9025d14d83..4b865b2558d9 100644
--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -1453,6 +1453,9 @@ static void damos_walk_call_walk(struct damon_ctx *ctx, struct damon_target *t,
 {
 	struct damos_walk_control *control;
 
+	if (s->walk_completed)
+		return;
+
 	mutex_lock(&ctx->walk_control_lock);
 	control = ctx->walk_control;
 	mutex_unlock(&ctx->walk_control_lock);
-- 
2.39.5

[PATCH 3/3] mm/damon/core: do damos walking in entire regions granularity

[SeongJae Park]

damos_walk_control can be installed while DAMOS is walking the regions.
This means the walk callback function invocations can be started from a
region at the middle of the regions list.  This makes it hard to be used
reliably.  Particularly, DAMOS tried regions update for collecting
monitoring results gets problematic results.  Increase the
walk_control_lock critical section to do walking in entire regions
granularity.

Fixes: bf0eaba0ff9c ("mm/damon/core: implement damos_walk()")
Signed-off-by: SeongJae Park <sj@kernel.org>
---
 mm/damon/core.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/mm/damon/core.c b/mm/damon/core.c
index 4b865b2558d9..c3d9b96b786a 100644
--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -1456,11 +1456,10 @@ static void damos_walk_call_walk(struct damon_ctx *ctx, struct damon_target *t,
 	if (s->walk_completed)
 		return;
 
-	mutex_lock(&ctx->walk_control_lock);
 	control = ctx->walk_control;
-	mutex_unlock(&ctx->walk_control_lock);
 	if (!control)
 		return;
+
 	control->walk_fn(control->data, ctx, t, r, s, sz_filter_passed);
 }
 
@@ -1480,9 +1479,7 @@ static void damos_walk_complete(struct damon_ctx *ctx, struct damos *s)
 	struct damos *siter;
 	struct damos_walk_control *control;
 
-	mutex_lock(&ctx->walk_control_lock);
 	control = ctx->walk_control;
-	mutex_unlock(&ctx->walk_control_lock);
 	if (!control)
 		return;
 
@@ -1496,9 +1493,7 @@ static void damos_walk_complete(struct damon_ctx *ctx, struct damos *s)
 		siter->walk_completed = false;
 
 	complete(&control->completion);
-	mutex_lock(&ctx->walk_control_lock);
 	ctx->walk_control = NULL;
-	mutex_unlock(&ctx->walk_control_lock);
 }
 
 /*
@@ -1845,6 +1840,7 @@ static void kdamond_apply_schemes(struct damon_ctx *c)
 	if (!has_schemes_to_apply)
 		return;
 
+	mutex_lock(&c->walk_control_lock);
 	damon_for_each_target(t, c) {
 		damon_for_each_region_safe(r, next_r, t)
 			damon_do_apply_schemes(c, t, r);
@@ -1859,6 +1855,7 @@ static void kdamond_apply_schemes(struct damon_ctx *c)
 			 c->attrs.aggr_interval) / sample_interval;
 		s->last_applied = NULL;
 	}
+	mutex_unlock(&c->walk_control_lock);
 }
 
 /*
-- 
2.39.5