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

[SeongJae Park <sj@kernel.org>]

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.

> 
> >>
> >> 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;
 }

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


Thanks,
SJ

[...]