Re: [PATCH 01/12] mm/damon/core: introduce nr_snapshots damos stat

[SeongJae Park <sj@kernel.org>]

On Wed, 14 Jan 2026 04:29:56 -0800 Chris Mason <clm@meta.com> wrote:

> On Tue, 16 Dec 2025 00:01:14 -0800 SeongJae Park <sj@kernel.org> wrote:
> 
> > DAMON generates monitoring results snapshots for every sampling
> > interval.  DAMOS applies given schemes on the regions of the snapshots,
> > for every apply interval of the scheme.
> > 
> > DAMOS stat informs a given scheme has tried to how many memory entities
> > and applied, in the region and byte level.  In some use cases including
> > user-space oriented tuning and investigations, it is useful to know that
> > in the DAMON-snapshot level.  Introduce a new stat, namely nr_snapshots
> > for DAMON core API callers.
> 
> Hi everyone,
> 
> I ran mm-new through the AI review prompts and this one was flagged:

Thank you Chris, this is a very helpful and great gift for the community!

> 
> > diff --git a/mm/damon/core.c b/mm/damon/core.c
> > --- a/mm/damon/core.c
> > +++ b/mm/damon/core.c
> > @@ -157,6 +157,12 @@ void damon_destroy_region(struct damon_region *r, struct damon_target *t)
> >  	damon_free_region(r);
> >  }
> >
> > +static bool damon_is_last_region(struct damon_region *r,
> > +		struct damon_target *t)
> > +{
> > +	return list_is_last(&t->regions_list, &r->list);
>                            ^^^^^^^^^^^^^^^^  ^^^^^^^^
> 
> Are the arguments to list_is_last() swapped here? The kernel's
> list_is_last() expects (entry, head) but this passes (head, entry).
> 
> Looking at include/linux/list.h:
> 
>     static inline int list_is_last(const struct list_head *list,
>                                    const struct list_head *head)
>     {
>         return list->next == head;
>     }
> 
> The current code checks if t->regions_list.next == &r->list, which will
> be true when r is the first region in the list, not the last.
> 
> Compare with the existing damon_last_region() in include/linux/damon.h
> which correctly uses &t->regions_list as the head:
> 
>     static inline struct damon_region *damon_last_region(struct damon_target *t)
>     {
>         return list_last_entry(&t->regions_list, struct damon_region, list);
>     }
> 
> Should this be:
> 
>     return list_is_last(&r->list, &t->regions_list);
> 
> > +}
> > +
> 
> [ ... ]
> 
> > @@ -1978,10 +1984,11 @@ static void damon_do_apply_schemes(struct damon_ctx *c,
> 
> [ ... ]
> 
> > +		if (damon_is_last_region(r, t))
> > +			s->stat.nr_snapshots++;
> 
> If the list_is_last() arguments are indeed swapped, this will increment
> nr_snapshots when processing the first region of each target rather than
> the last, causing the stat to be counted once per target instead of once
> per complete snapshot.

AI is all correct.  Note that this is not making user-visible issues, though it
is clearly a bug that better to be fixed.  I explain the reason in below fixup
patch.

Andrew, the commit to fix is in mm-unstable.  Could you please squash below
attaching fixup to the commit?


Thanks,
SJ

[...]
=== >8 ===
From 551b89fd05f0c40c824d2a5c0ca6c2a63766fbab Mon Sep 17 00:00:00 2001
From: SeongJae Park <sj@kernel.org>
Date: Wed, 14 Jan 2026 06:57:38 -0800
Subject: [PATCH] mm/damon/core: fix wrong list_is_last() call in
 damons_is_last_region()

damon_is_last_region() is calling list_is_last() with swapped arguments.
As a result, it works like damon_is_first_region().  Not that this is
not making user-visible impact, because the function is only used to
increment nr_snapshots stat once per DAMOS' regions iteration, and the
stat is exposed users only after the iteration is completed.  That's why
it passed a user space test.  Nonetheless, the bug is a bug and could
cause real user issues in future.  Fix it by correcting the order of
arguments for list_lis_last().

Reported-by: Chris Mason <clm@meta.com>
Closes: https://lore.kernel.org/20260114122959.1164957-1-clm@meta.com
Fixes: bbb3a45f1923 ("mm/damon/core: introduce nr_snapshots damos stat") # mm-unstable
Signed-off-by: SeongJae Park <sj@kernel.org>
---
 mm/damon/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/damon/core.c b/mm/damon/core.c
index 385900733a0b..5d7e58cbd9fe 100644
--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -190,7 +190,7 @@ void damon_destroy_region(struct damon_region *r, struct damon_target *t)
 static bool damon_is_last_region(struct damon_region *r,
 		struct damon_target *t)
 {
-	return list_is_last(&t->regions_list, &r->list);
+	return list_is_last(&r->list, &t->regions_list);
 }
 
 /*
-- 
2.47.3