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

Chris Mason
2026-01-14T12:30:25Z

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:

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

> }
> }