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

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

Hi SeongJae,

Sorry for the late response.

On 6/24/2025 3:03 AM, SeongJae Park wrote:
> On Mon, 23 Jun 2025 11:58:53 +0900 Honggyu Kim <honggyu.kim@sk.com> wrote:
> 
>> Hi SeongJae,
>>
>> Thanks for the kind review as always!
> 
> My pleasure :)
> 
>>
>> 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".
> 
> Thank you for clarifying!  Please add this description on the commit message if
> you send a next version of this patch, with Fixes: tag.

OK. I will add "Fixes: tag.

>>>>
>>>> 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?
> 
> I mean, something like below.
> 
> @@ -1449,6 +1449,7 @@ static unsigned long damon_get_intervals_score(struct damon_ctx *c)
>                  }
>          }
>          target_access_events = max_access_events * goal_bp / 10000;
> +       target_access_events = target_access_events ? : 1;
>          return access_events * 10000 / target_access_events;
>   }

I actually didn't mean the code, but just wondered if setting 
"target_access_events" to 1 makes sense in this context.

I now think that it doesn't make any difference because applying DAMOS actions
to zero size regions as it's just no-ops.  So I can take your change.

>>>
>>>>
>>>> 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?
> 
> I still prefer fixing the found bug on the spot.  I don't think having zero or
> negative size regions is really somewhat we always prohibit.

I can split "target_access_events" change patch from this regardless of this
with "Fixes" tag.

But I don't get why you think zero size region is acceptable.  Do you see any 
benefits or have special reasons allowing zero size regions?

Thanks,
Honggyu

> 
> 
> Thanks,
> SJ
> 
> [...]
>