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

[Honggyu Kim <honggyu.kim@sk.com>]

Hi SeongJae,

On 7/2/2025 2:00 AM, SeongJae Park wrote:
> 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.

Sure. I've just made it as you mentioned in v3.

> 
> 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@.

I've cc'ed stable@ in v3. Thanks for your help!

Thanks,
Honggyu

> 
>>        return access_events * 10000 / target_access_events;
>>   }
>>
>> --
>> 2.34.1
> 
> 
> Thanks,
> SJ