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

SeongJae Park
2026-01-14T15:20:56Z

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