Re: [PATCH 1/2] mm/damon: fix nr_snapshots counting using tried_applied flag

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Logic error: `tried_applied` is set unconditionally, ignoring the access pattern guard check.
- [Medium] Uninitialized boolean `tried_applied` in `struct damos` leads to spurious `nr_snapshots` increment.
- [Low] Build failure due to removed function `damon_is_last_region()` still being referenced in KUnit tests.
--

--- Patch [1]: [PATCH 1/2] mm/damon: fix nr_snapshots counting using tried_applied flag ---
commit b2745ffce878c131daca165cb2a6674f9ded0bc7
Author: Liew Rui Yan <[email protected]>

mm/damon: fix nr_snapshots counting using tried_applied flag

Currently, nr_snapshots is only incremented when damon_is_last_region()
returns true. This is semantically incorrect because nr_snapshots should
count the number of DAMON snapshots (aggregation intervals) where the
scheme was tried to be applied, as documented in design.rst.

> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index 644daf5a16560..7230483e771f7 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
> @@ -398,12 +398,6 @@ static void damon_destroy_region(struct damon_region *r,
>  	damon_free_region(r);
>  }
>  
> -static bool damon_is_last_region(struct damon_region *r,
> -		struct damon_target *t)
> -{
> -	return list_is_last(&r->list, &t->regions_list);
> -}
> -

[Severity: Low]
Does removing this function break the KUnit tests?

Looking at mm/damon/tests/core-kunit.h:damon_test_is_last_region(), it
still calls this function:

    KUNIT_EXPECT_TRUE(test, damon_is_last_region(r, t));

Since the test header is included at the end of core.c, compiling with
CONFIG_DAMON_KUNIT_TEST=y would result in an implicit declaration or
undefined symbol error.

[ ... ]

> @@ -2668,8 +2662,7 @@ static void damon_do_apply_schemes(struct damon_ctx *c,
>  		if (damos_valid_target(c, r, s))
>  			damos_apply_scheme(c, t, r, s);
>  
> -		if (damon_is_last_region(r, t))
> -			s->stat.nr_snapshots++;
> +		s->tried_applied = true;
>  	}
>  }

[Severity: High]
Does this bypass the access pattern guard check?

The commit message states the scheme should pass all guard checks including
the access pattern before being considered tried. However, s->tried_applied
is set to true unconditionally here, even if damos_valid_target() returns
false. This would cause the max_nr_snapshots limit to be reached
prematurely.

[ ... ]

> @@ -3249,6 +3242,9 @@ static void kdamond_apply_schemes(struct damon_ctx *c)
>  	damon_for_each_scheme(s, c) {
>  		if (time_before(c->passed_sample_intervals, s->next_apply_sis))
>  			continue;
> +		if (s->tried_applied)
> +			s->stat.nr_snapshots++;
> +		s->tried_applied = false;

[Severity: Medium]
Could this result in a spurious increment of nr_snapshots due to
uninitialized memory?

When struct damos is allocated in mm/damon/core.c:damon_new_scheme() using:

    scheme = kmalloc_obj(*scheme);

the memory is not zero-initialized. During the first execution here, if a
scheme was skipped before damon_do_apply_schemes() explicitly sets the flag
(for example, due to zero valid targets or fully consumed quotas), the
uninitialized value might evaluate to true.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1