Re: [PATCH 1/3] mm/damon: do not allow creating zero size region

[SeongJae Park <sj@kernel.org>]

On Sun, 22 Jun 2025 21:09:23 +0900 Honggyu Kim <honggyu.kim@sk.com> wrote:

> Creating zero size region leads a divide by zero error inside
> damon_get_intervals_score() as follows.
> 
>   static unsigned long damon_get_intervals_score(struct damon_ctx *c)
>   {
>       struct damon_target *t;
>       struct damon_region *r;
>       unsigned long sz_region, max_access_events = 0, access_events = 0;
>       unsigned long target_access_events;
>       unsigned long goal_bp = c->attrs.intervals_goal.access_bp;
> 
>       damon_for_each_target(t, c) {
>           damon_for_each_region(r, t) {
>               sz_region = damon_sz_region(r);
>               max_access_events += sz_region * c->attrs.aggr_samples;
>               access_events += sz_region * r->nr_accesses;
>           }
>       }
>       target_access_events = max_access_events * goal_bp / 10000;
>       return access_events * 10000 / target_access_events;  /* divide by zero! */
>   }

Thank you for finding this issue!  Coudl you please further share how zero size
region can be made, and if user-space can make the situation?

> 
> This patch makes a NULL return for such cases when creating a region
> inside damon_new_region().

I agree zero size region could look silly.  But I don't really think it should
be prohibited.  What about modifying damon_get_intervals_score() instead?
Maybe we can set target_access_events as 1 in this case.

> 
> Signed-off-by: Honggyu Kim <honggyu.kim@sk.com>
> Cc: Yunjeong Mun <yunjeong.mun@sk.com>
> ---
>  mm/damon/core.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index b217e0120e09..44740da337fd 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
> @@ -126,8 +126,11 @@ struct damon_region *damon_new_region(unsigned long start, unsigned long end)
>  	if (!region)
>  		return NULL;
>  
> +	if (start == end)
> +		return NULL;
>  	region->ar.start = start;
>  	region->ar.end = end;
> +

Above new line is unnecessary in my opinion.

>  	region->nr_accesses = 0;
>  	region->nr_accesses_bp = 0;
>  	INIT_LIST_HEAD(&region->list);
> -- 
> 2.34.1


Thanks,
SJ