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

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

Hi SeongJae,

Thanks for the kind review as always!

On 6/23/2025 1:04 AM, SeongJae Park wrote:
> 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?

The initial values of node*_start_addr and node*_end_addr inside
/sys/module/mtier/parameters/ are all zeros so I saw this problem easily by
setting Y to "enabled".

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

Hmm... I don't get what you mean by setting "target_access_events" to 1 for such
cases.  Could you please explain more?

> 
>>
>> 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;
If you're okay, then I'd like to modify this as follows again.

+     if (start >= end)
+             return NULL;

What do you think about this?

>>        region->ar.start = start;
>>        region->ar.end = end;
>> +
> 
> Above new line is unnecessary in my opinion.

Sure. will remove it in the next version.

Thanks,
Honggyu

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