[RFC PATCH] mm/damon/sysfs-schemes: fix double increment of nr_regions

[RFC PATCH] mm/damon/sysfs-schemes: fix double increment of nr_regions

[Vineet Agarwal]

damos_sysfs_populate_region_dir() increments
sysfs_regions->nr_regions twice when adding a new region:
once explicitly before kobject_init_and_add(), and once
again through the post-increment used for the kobject name.

As a result, nr_regions no longer matches the actual
number of live regions, and region directory names skip
numbers (1, 3, 5, ...).

Use the already incremented value for naming instead of
incrementing nr_regions a second time.

Signed-off-by: Vineet Agarwal <agarwal.vineet2006@gmail.com>
---
 mm/damon/sysfs-schemes.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/damon/sysfs-schemes.c b/mm/damon/sysfs-schemes.c
index 622c3799db87..5d966ac86419 100644
--- a/mm/damon/sysfs-schemes.c
+++ b/mm/damon/sysfs-schemes.c
@@ -2998,7 +2998,7 @@ void damos_sysfs_populate_region_dir(struct damon_sysfs_schemes *sysfs_schemes,
 	if (kobject_init_and_add(&region->kobj,
 				&damon_sysfs_scheme_region_ktype,
 				&sysfs_regions->kobj, "%d",
-				sysfs_regions->nr_regions++)) {
+				sysfs_regions->nr_regions)) {
 		kobject_put(&region->kobj);
 	}
 }
-- 
2.54.0

Re: [RFC PATCH] mm/damon/sysfs-schemes: fix double increment of nr_regions

[SeongJae Park]

On Tue, 12 May 2026 00:42:15 +0530 Vineet Agarwal <agarwal.vineet2006@gmail.com> wrote:

> damos_sysfs_populate_region_dir() increments
> sysfs_regions->nr_regions twice when adding a new region:
> once explicitly before kobject_init_and_add(), and once
> again through the post-increment used for the kobject name.

Nice catch!

> 
> As a result, nr_regions no longer matches the actual
> number of live regions,

Nonetheless, this is not causing real issues.  The the nr_regions is not
exposed to the user space.  Similar data structures in the file use arrays for
managing sub-directories and use the number field as the size of the array.  If
that's the case for these region directory data, the code might allocating
more-than-needed memory or having some problems.  But luckily region
directories are managed using linked list.  I find no real problems here.

> and region directory names skip
> numbers (1, 3, 5, ...).

This is also not causing any real issue.  This could arguably make users
confused.  But still user space could work with this in a reasonable way, by
sorting the regions based on the index and/or the address of regions.  Actually
DAMON user-space tool is handling this.  That's why I didn't realize this issue
by myself.  We recommend human beings to use tools rather than directly
interacting with the sysfs interface.

The admin usage document is also not giving wrong assumptions about the names.
It only says the names will be integers starting from 0.  The real names start
from 1, but arguably that doesn't break something, becuase the document doesn't
say there will be directory of name '0'...?  

> 
> Use the already incremented value for naming instead of
> incrementing nr_regions a second time.

So, there is no real problems.  Please let me know if I'm missing something.

But it is also super clear that this is not an intended behavior but a bug.
That needs to be fixed unless it causes unnecessary overhead.  Meanwhile, this
fix is simple and nice.

So, nice finding and fix!  Thank you for sharing, Vineet!

> 
> Signed-off-by: Vineet Agarwal <agarwal.vineet2006@gmail.com>

Seems the current behavior has started since commit 66178e4ec30a
("mm/damon/sysfs: use damos_walk() for update_schemes_tried_{bytes,regions}").
I think it might deserve 'Fixes:' tag if someone claims, but Cc: stable@ may
not really needed because this is not causing any real user issue, as I
mentioned above.

So, this looks good to me.  Vineet, is there anything that you want to sort out
before dropping the RFC tag?  If not, please feel free to post this again
without the RFC tag.


Thanks,
SJ

[...]

Re: [RFC PATCH] mm/damon/sysfs-schemes: fix double increment of nr_regions

[Vineet Agarwal]

[-- Attachment #1: Type: text/plain, Size: 3352 bytes --]

Hi SJ,

Thanks for the detailed review and explanation.

I also couldn't find any path where nr_regions affects allocation sizing or
other important behavior, so this does seem limited to the incorrect count
and skipped numbering.

Nothing else to sort out on my end — I've resent the patch without the RFC
tag and with a Fixes tag for the commit that introduced the behavior.

Thanks again for taking a look.

Vineet


On Tue, 12 May 2026 at 06:15, SeongJae Park <sj@kernel.org> wrote:

> On Tue, 12 May 2026 00:42:15 +0530 Vineet Agarwal <
> agarwal.vineet2006@gmail.com> wrote:
>
> > damos_sysfs_populate_region_dir() increments
> > sysfs_regions->nr_regions twice when adding a new region:
> > once explicitly before kobject_init_and_add(), and once
> > again through the post-increment used for the kobject name.
>
> Nice catch!
>
> >
> > As a result, nr_regions no longer matches the actual
> > number of live regions,
>
> Nonetheless, this is not causing real issues.  The the nr_regions is not
> exposed to the user space.  Similar data structures in the file use arrays
> for
> managing sub-directories and use the number field as the size of the
> array.  If
> that's the case for these region directory data, the code might allocating
> more-than-needed memory or having some problems.  But luckily region
> directories are managed using linked list.  I find no real problems here.
>
> > and region directory names skip
> > numbers (1, 3, 5, ...).
>
> This is also not causing any real issue.  This could arguably make users
> confused.  But still user space could work with this in a reasonable way,
> by
> sorting the regions based on the index and/or the address of regions.
> Actually
> DAMON user-space tool is handling this.  That's why I didn't realize this
> issue
> by myself.  We recommend human beings to use tools rather than directly
> interacting with the sysfs interface.
>
> The admin usage document is also not giving wrong assumptions about the
> names.
> It only says the names will be integers starting from 0.  The real names
> start
> from 1, but arguably that doesn't break something, becuase the document
> doesn't
> say there will be directory of name '0'...?
>
> >
> > Use the already incremented value for naming instead of
> > incrementing nr_regions a second time.
>
> So, there is no real problems.  Please let me know if I'm missing
> something.
>
> But it is also super clear that this is not an intended behavior but a bug.
> That needs to be fixed unless it causes unnecessary overhead.  Meanwhile,
> this
> fix is simple and nice.
>
> So, nice finding and fix!  Thank you for sharing, Vineet!
>
> >
> > Signed-off-by: Vineet Agarwal <agarwal.vineet2006@gmail.com>
>
> Seems the current behavior has started since commit 66178e4ec30a
> ("mm/damon/sysfs: use damos_walk() for
> update_schemes_tried_{bytes,regions}").
> I think it might deserve 'Fixes:' tag if someone claims, but Cc: stable@
> may
> not really needed because this is not causing any real user issue, as I
> mentioned above.
>
> So, this looks good to me.  Vineet, is there anything that you want to
> sort out
> before dropping the RFC tag?  If not, please feel free to post this again
> without the RFC tag.
>
>
> Thanks,
> SJ
>
> [...]
>

[-- Attachment #2: Type: text/html, Size: 4657 bytes --]