Re: [PATCH v2 4/4] mm/damon: fix divide by zero in damon_get_intervals_score()

[SeongJae Park <sj@kernel.org>]

Hi Honggyu,

On Tue,  1 Jul 2025 17:19:26 +0900 Honggyu Kim <honggyu.kim@sk.com> wrote:

> The current implementation allows having zero size regions with no
> special reasons, but damon_get_intervals_score() gets crashed by divide
> by zero when the region size is zero.
> 
>   [   29.403950] Oops: divide error: 0000 [#1] SMP NOPTI
> 
> This patch fixes the bug, but does not disallow zero size regions to
> keep the backward compatibility since disallowing zero size regions
> might be a breaking change for some users.
> 
> Fixes: f04b0fedbe71 ("mm/damon/core: implement intervals auto-tuning")
> Signed-off-by: Honggyu Kim <honggyu.kim@sk.com>
> Cc: Yunjeong Mun <yunjeong.mun@sk.com>
> ---
>  mm/damon/core.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index b217e0120e09..e274a4d958d6 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
> @@ -1448,7 +1448,10 @@ static unsigned long damon_get_intervals_score(struct damon_ctx *c)
>  			access_events += sz_region * r->nr_accesses;
>  		}
>  	}
> -	target_access_events = max_access_events * goal_bp / 10000;
> +	if (likely(max_access_events) > 0)

This makes checkpatch.pl complaints as below:

WARNING: Using likely should generally have parentheses around the comparison
#39: FILE: mm/damon/core.c:1451:
+       if (likely(max_access_events) > 0)

This is not performance critical part, so I think we can simply drop likely().

> +		target_access_events = max_access_events * goal_bp / 10000;
> +	else
> +		target_access_events = 1;

But, target_access_events could still be zero if goal_bp is zero or
max_access_events * goal_bp is less thatn 10000?

I'd like to simply do the target_access_events calculation as is, and set
target_access_events as 1, if it is zero.  Since this is not a performance
critical path, I think that's ok.

Also, in the previous version, we discussed the reproduction step requires
sample modules.  But seems we can also reproduce this with the official DAMON
sysfs ABI by setting the intervals goal zero, and I just confirmed that.  Let's
Cc stable@.

>  	return access_events * 10000 / target_access_events;
>  }
>  
> -- 
> 2.34.1


Thanks,
SJ