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

[Chris Mason <clm@meta.com>]

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.

>  	}
>  }