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

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

Hi SeongJae,

On 6/27/2025 12:27 AM, SeongJae Park wrote:
> On Thu, 26 Jun 2025 07:24:14 +0900 Honggyu Kim <honggyu.kim@sk.com> wrote:
> 
>> Hi SeongJae,
>>
>> Sorry for the late response.
> 
> No worry!
> 
>> 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:
> [...]
>>> 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.
> 
> Thank you for clarifying, looking forward to your fix! :)

Will send it within a few days.  Thanks for your patience.

> 
> [...]
>>> 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?
> 
> In short, I don't anticipate special benefits of allowing zero size region.
> But that's smae to this change.
> 
> I even have small concern about this change.
> 
> This change might make people assume any damon_region would have non-zero
> positive size.  I think that's wrong assumption.  Any DAMON core and API caller
> code can set the start and the end addresses with arbitrary values.
> 
> I think making the assumption true could be beneficial since it will help
> writing code with less corner cases.  But to make the assumption true, we
> should first check if any existing code is violating it, and if any existing
> code that written with current assumption (region size can be zero) can be
> broken.  After that, we should also prevent future code violating it.
> 
> And if the assumption becomes truth, we get one more rule.  I, at least, prefer
> having as less rules as possible.  My taste may be weird, but this is my humble
> feeling and opinion.

I see.  You don't want to make breaking changes even in this case, then I can
leave it as is.  Thanks for the explanation!

Thanks,
Honggyu

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