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

[SeongJae Park <sj@kernel.org>]

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! :)

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


Thanks,
SJ

[...]