[PATCH] mm/damon/core: do not merge regions when the number of regions is small

[PATCH] mm/damon/core: do not merge regions when the number of regions is small

[Akinobu Mita]

This change prevents adaptive region merging when the total number of
regions is less than the minimum number of regions setting.

Even in such cases, the kdamond_merge_regions() function must proceed by
setting the upper limit of the region size to zero to prevent any actual
region merging, since it is responsible for updating the region ages rather
than just merging regions.

Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
 mm/damon/core.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/mm/damon/core.c b/mm/damon/core.c
index 52a48c9316bc..34f1844094cf 100644
--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -2247,9 +2247,15 @@ static void kdamond_merge_regions(struct damon_ctx *c, unsigned int threshold,
 				  unsigned long sz_limit)
 {
 	struct damon_target *t;
-	unsigned int nr_regions;
+	unsigned int nr_regions = 0;
 	unsigned int max_thres;
 
+	damon_for_each_target(t, c)
+		nr_regions += damon_nr_regions(t);
+
+	if (nr_regions < c->attrs.min_nr_regions)
+		sz_limit = 0;
+
 	max_thres = c->attrs.aggr_interval /
 		(c->attrs.sample_interval ?  c->attrs.sample_interval : 1);
 	do {
-- 
2.43.0

Re: [PATCH] mm/damon/core: do not merge regions when the number of regions is small

[Sang-Heon Jeon]

Hello, Akinobu

On Mon, Aug 11, 2025 at 4:46 PM Akinobu Mita <akinobu.mita@gmail.com> wrote:
>
> This change prevents adaptive region merging when the total number of
> regions is less than the minimum number of regions setting.
>
> Even in such cases, the kdamond_merge_regions() function must proceed by
> setting the upper limit of the region size to zero to prevent any actual
> region merging, since it is responsible for updating the region ages rather
> than just merging regions.
>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> ---
>  mm/damon/core.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index 52a48c9316bc..34f1844094cf 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
> @@ -2247,9 +2247,15 @@ static void kdamond_merge_regions(struct damon_ctx *c, unsigned int threshold,
>                                   unsigned long sz_limit)
>  {
>         struct damon_target *t;
> -       unsigned int nr_regions;
> +       unsigned int nr_regions = 0;
>         unsigned int max_thres;
>
> +       damon_for_each_target(t, c)
> +               nr_regions += damon_nr_regions(t);
> +
> +       if (nr_regions < c->attrs.min_nr_regions)
> +               sz_limit = 0;
> +

I just have a question. Please don't feel bad.

1. Doesn't `sz_limit` which is already calculated with
`min_nr_regions` guarantee a lower bound generally? If you meet some
counterparts, could you share the situation?
2. Even in that case, doesn't the split/merge loop generally guarantee
a lower bound? Are there any special cases where you can't wait for
that loop?

I might be wrong. Even if my question is silly, could you please
answer kindly? I want to learn more.

>         max_thres = c->attrs.aggr_interval /
>                 (c->attrs.sample_interval ?  c->attrs.sample_interval : 1);
>         do {
> --
> 2.43.0
>
>

Have a nice day.

Best Regards.
Sang-Heon Jeon

Re: [PATCH] mm/damon/core: do not merge regions when the number of regions is small

[Akinobu Mita]

2025年8月12日(火) 0:52 Sang-Heon Jeon <ekffu200098@gmail.com>:
>
> Hello, Akinobu
>
> On Mon, Aug 11, 2025 at 4:46 PM Akinobu Mita <akinobu.mita@gmail.com> wrote:
> >
> > This change prevents adaptive region merging when the total number of
> > regions is less than the minimum number of regions setting.
> >
> > Even in such cases, the kdamond_merge_regions() function must proceed by
> > setting the upper limit of the region size to zero to prevent any actual
> > region merging, since it is responsible for updating the region ages rather
> > than just merging regions.
> >
> > Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> > ---
> >  mm/damon/core.c | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/mm/damon/core.c b/mm/damon/core.c
> > index 52a48c9316bc..34f1844094cf 100644
> > --- a/mm/damon/core.c
> > +++ b/mm/damon/core.c
> > @@ -2247,9 +2247,15 @@ static void kdamond_merge_regions(struct damon_ctx *c, unsigned int threshold,
> >                                   unsigned long sz_limit)
> >  {
> >         struct damon_target *t;
> > -       unsigned int nr_regions;
> > +       unsigned int nr_regions = 0;
> >         unsigned int max_thres;
> >
> > +       damon_for_each_target(t, c)
> > +               nr_regions += damon_nr_regions(t);
> > +
> > +       if (nr_regions < c->attrs.min_nr_regions)
> > +               sz_limit = 0;
> > +
>
> I just have a question. Please don't feel bad.
>
> 1. Doesn't `sz_limit` which is already calculated with
> `min_nr_regions` guarantee a lower bound generally? If you meet some
> counterparts, could you share the situation?
> 2. Even in that case, doesn't the split/merge loop generally guarantee
> a lower bound? Are there any special cases where you can't wait for
> that loop?

Thank you for the explanation.
I now understand that the patch was incorrect.

What I was trying to address was the problem when monitoring at page
granularity, as described in Documentation/mm/damon/faq.rst.

When paddr is used and the total size of the target regions remains constant,
I expected the number of regions to increase monotonically because I had set
the min_nr_regions to a sufficiently large value.  However, the number of
regions appeared to increase and decrease.

I checked again and found that the region merge that the patch was trying to
address had not occurred, so perhaps the previous settings were incorrect.

Re: [PATCH] mm/damon/core: do not merge regions when the number of regions is small

[SeongJae Park]

On Mon, 11 Aug 2025 16:46:12 +0900 Akinobu Mita <akinobu.mita@gmail.com> wrote:

> This change prevents adaptive region merging when the total number of
> regions is less than the minimum number of regions setting.

I agree having the number of regions lower than min_nr_regions is not a good
user experience.  We actually made a fix of a case where the number be higher
than the max_nr_regions, namely commit 310d6c15e910 ("mm/damon/core: merge
regions aggressively when max_nr_regions is unmet").

But, could you please elaborate when the situation can happen, why it is bad,
and if you see this from a real world?

I can expect this situation can happen when the user increases min_nr_regions
to somewhat higher than current number of regions while DAMON is running, using
runtime commit feature.  And it is bad because anyway it is confusing and could
take time to real expected minimum accuracy until DAMON splits regions to keep
the min_nr_regions?  I have no idea for the last question, though.

Anyway, I think this kind of details would be good to be added to the
changelog of this patch, in the next version.

> 
> Even in such cases, the kdamond_merge_regions() function must proceed by
> setting the upper limit of the region size to zero to prevent any actual
> region merging, since it is responsible for updating the region ages rather
> than just merging regions.
> 
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> ---
>  mm/damon/core.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index 52a48c9316bc..34f1844094cf 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
> @@ -2247,9 +2247,15 @@ static void kdamond_merge_regions(struct damon_ctx *c, unsigned int threshold,
>  				  unsigned long sz_limit)
>  {
>  	struct damon_target *t;
> -	unsigned int nr_regions;
> +	unsigned int nr_regions = 0;
>  	unsigned int max_thres;
>  
> +	damon_for_each_target(t, c)
> +		nr_regions += damon_nr_regions(t);
> +
> +	if (nr_regions < c->attrs.min_nr_regions)
> +		sz_limit = 0;
> +

sz_limit is already set based on the current min_nr_regions.  Setting this
smaller may help avoiding a few corner case merges, but ain't really avoid
having number of regions lower than min_nr_regions?

Instead, what about making kdamond_split_regions() repeat splitting in the case
until min_nr_regions is met, like kdamond_merge_regions() does for
max_nr_regions?

>  	max_thres = c->attrs.aggr_interval /
>  		(c->attrs.sample_interval ?  c->attrs.sample_interval : 1);
>  	do {
> -- 
> 2.43.0


Thanks,
SJ

Re: [PATCH] mm/damon/core: do not merge regions when the number of regions is small

[Akinobu Mita]

2025年8月12日(火) 4:26 SeongJae Park <sj@kernel.org>:
>
> On Mon, 11 Aug 2025 16:46:12 +0900 Akinobu Mita <akinobu.mita@gmail.com> wrote:
>
> > This change prevents adaptive region merging when the total number of
> > regions is less than the minimum number of regions setting.
>
> I agree having the number of regions lower than min_nr_regions is not a good
> user experience.  We actually made a fix of a case where the number be higher
> than the max_nr_regions, namely commit 310d6c15e910 ("mm/damon/core: merge
> regions aggressively when max_nr_regions is unmet").
>
> But, could you please elaborate when the situation can happen, why it is bad,
> and if you see this from a real world?
>
> I can expect this situation can happen when the user increases min_nr_regions
> to somewhat higher than current number of regions while DAMON is running, using
> runtime commit feature.  And it is bad because anyway it is confusing and could
> take time to real expected minimum accuracy until DAMON splits regions to keep
> the min_nr_regions?  I have no idea for the last question, though.
>
> Anyway, I think this kind of details would be good to be added to the
> changelog of this patch, in the next version.
>
> >
> > Even in such cases, the kdamond_merge_regions() function must proceed by
> > setting the upper limit of the region size to zero to prevent any actual
> > region merging, since it is responsible for updating the region ages rather
> > than just merging regions.
> >
> > Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> > ---
> >  mm/damon/core.c | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/mm/damon/core.c b/mm/damon/core.c
> > index 52a48c9316bc..34f1844094cf 100644
> > --- a/mm/damon/core.c
> > +++ b/mm/damon/core.c
> > @@ -2247,9 +2247,15 @@ static void kdamond_merge_regions(struct damon_ctx *c, unsigned int threshold,
> >                                 unsigned long sz_limit)
> >  {
> >       struct damon_target *t;
> > -     unsigned int nr_regions;
> > +     unsigned int nr_regions = 0;
> >       unsigned int max_thres;
> >
> > +     damon_for_each_target(t, c)
> > +             nr_regions += damon_nr_regions(t);
> > +
> > +     if (nr_regions < c->attrs.min_nr_regions)
> > +             sz_limit = 0;
> > +
>
> sz_limit is already set based on the current min_nr_regions.  Setting this
> smaller may help avoiding a few corner case merges, but ain't really avoid
> having number of regions lower than min_nr_regions?

As I've already replied to Sang-Heon, this patch did not fix anything.
I was trying to work around a behavior that was probably caused by an
unintended setting.

> Instead, what about making kdamond_split_regions() repeat splitting in the case
> until min_nr_regions is met, like kdamond_merge_regions() does for
> max_nr_regions?

I think that's a good idea.