Re: [PATCH v2 2/2] mm/damon/core: Copy only installed probe in damon_split_region_at()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] mm: damon: performance degradation replacing memcpy with linked list traversal
- [Low] mm: damon: build failure in KUnit test due to missing argument in damos_skip_charged_region()
--

commit 2a85f15b49d0115f937574a91868632edfb71225
Author: Suhaas Joshi <suhaas@s-joshi.in>

mm/damon/core: Copy only installed probe in damon_split_region_at()

This commit updates damon_split_region_at() to iterate only over installed
probes when copying probe hits, instead of copying the entire array. It
introduces a struct damon_ctx pointer parameter to the function to facilitate
this change. It also updates its callers to pass the new argument.

> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index fc202f90991b0..9e5a19c2a19d0 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c

[ ... ]

> @@ -3686,11 +3691,13 @@ static int damon_split_region_at(struct damon_target *t,
>  	new->age = r->age;
>  	new->last_nr_accesses = r->last_nr_accesses;
>  	new->nr_accesses = r->nr_accesses;
> -	/* todo: do this for only installed probes */
> -	memcpy(new->probe_hits, r->probe_hits, sizeof(r->probe_hits));
> -	memcpy(new->last_probe_hits, r->last_probe_hits,
> -			sizeof(r->last_probe_hits));
>  
> +	i = 0;
> +	damon_for_each_probe(p, ctx) {
> +		new->probe_hits[i] = r->probe_hits[i];
> +		new->last_probe_hits[i] = r->last_probe_hits[i];
> +		++i;
> +	}

[Severity: Low]
Since DAMON_MAX_PROBES is fixed to 4, the original memcpy() copied exactly
4 bytes and was typically optimized into a single register load/store.

Does replacing this with a damon_for_each_probe() linked list traversal
introduce unnecessary execution overhead in this hot path? The indirection
and branch checks to serve as an array copy counter might be a performance
degradation compared to the optimized memcpy().

>  	damon_insert_region(new, r, damon_next_region(r), t);
>  	return 0;
>  }

[ ... ]

> diff --git a/mm/damon/tests/core-kunit.h b/mm/damon/tests/core-kunit.h
> index ec953426b8403..a3dd31f3b112e 100644
> --- a/mm/damon/tests/core-kunit.h
> +++ b/mm/damon/tests/core-kunit.h

[ ... ]

> @@ -1868,6 +1899,7 @@ static void damos_test_filter_out(struct kunit *test)
>  	damon_destroy_region(r2, t);
>  
>  out:
> +	damon_destroy_ctx(ctx);
>  	damon_free_target(t);
>  	damos_free_filter(f);
>  }

[Severity: Low]
The patch modifies the signature of damos_skip_charged_region() to accept a
new struct damon_ctx pointer as its first parameter, but it looks like the
invocation in damon_test_last_region_quota_reset() was missed:

mm/damon/tests/core-kunit.h:damon_test_last_region_quota_reset() {
	...
	skipped = damos_skip_charged_region(t, r2, &s, min_region_sz);
	...
}

Will this cause a build failure for too few arguments when the kernel is
built with CONFIG_DAMON_KUNIT_TEST=y?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261008151944.113714-2-suhaas@s-joshi.in?part=2