Re: [RFC PATCH v2.2 05/17] mm/damon/core: introduce damon_prep struct

[SJ Park <sj@kernel.org>]

On Tue, 28 Jul 2026 04:25:18 +0000 sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Medium] The newly introduced `preps` list in `struct damon_probe` is not synchronized during `damon_commit_probes()`.
> - [Medium] Missing public API to remove or destroy individual `damon_prep` instances dynamically.
> --
> 
> --- Patch [5]: [RFC PATCH v2.2 05/17] mm/damon/core: introduce damon_prep struct ---
> commit d7a749334ed0e3303b955b00673d30f87ca5091c
> Author: SJ Park <sj@kernel.org>
> 
> mm/damon/core: introduce damon_prep struct
> 
> Some DAMON probe filter types require preparatory actions to operate
> effectively, such as setting the PG_Idle flag at a specific time. This
> commit introduces a new data type called damon_prep to specify these
> preparation actions for each probe. DAMON will execute the action for
> each region per sampling interval.
> 
> > diff --git a/include/linux/damon.h b/include/linux/damon.h
> > index a8bb9ee6979e3..1a8de735f955b 100644
> > --- a/include/linux/damon.h
> > +++ b/include/linux/damon.h
> 
> [ ... ]
> 
> > @@ -780,6 +801,8 @@ struct damon_filter {
> >  struct damon_probe {
> >  	unsigned int weight;
> >  /* private: */
> > +	/* Preparation actions to apply to each probing memory. */
> > +	struct list_head preps;
> >  	/* Filters for assessing if a given region is for this probe. */
> >  	struct list_head filters;
> >  	/* Siblings list. */
> 
> [Severity: Medium]
> Will this preps list be synchronized when a DAMON context is updated or
> cloned?
> 
> Looking at damon_commit_probes() in mm/damon/core.c, it only seems to copy
> the weight and filters from the source probe to the destination probe:
> 
> mm/damon/core.c:damon_commit_probes() {
>     ...
>     if (src_probe) {
>         dst_probe->weight = src_probe->weight;
>         err = damon_commit_filters(dst_probe, src_probe);
>         if (err)
>             return err;
>     } else {
>         damon_destroy_probe(dst_probe);
>     }
>     ...
> }
> 
> It appears there is no logic to copy the new preps list. Could this leave
> the running context's preparation actions out of sync and silently discard
> configurations when damon_commit_ctx() is called?

A later patch of this series does that.

> 
> > diff --git a/mm/damon/core.c b/mm/damon/core.c
> > index 644daf5a16560..0b396c2456d05 100644
> > --- a/mm/damon/core.c
> > +++ b/mm/damon/core.c
> 
> [ ... ]
> 
> > @@ -111,6 +111,28 @@ int damon_select_ops(struct damon_ctx *ctx, enum damon_ops_id id)
> >  	return err;
> >  }
> >  
> > +struct damon_prep *damon_new_prep(enum damon_prep_action action)
> > +{
> > +	struct damon_prep *prep;
> > +
> > +	prep = kmalloc_obj(*prep);
> > +	if (!prep)
> > +		return NULL;
> > +	prep->action = action;
> > +	INIT_LIST_HEAD(&prep->list);
> > +	return prep;
> > +}
> > +
> > +void damon_add_prep(struct damon_probe *p, struct damon_prep *prep)
> > +{
> > +	list_add_tail(&prep->list, &p->preps);
> > +}
> > +
> > +static void damon_free_prep(struct damon_prep *p)
> > +{
> > +	kfree(p);
> > +}
> 
> [Severity: Medium]
> Should there be a public API to remove or destroy individual damon_prep
> instances dynamically?
> 
> Since damon_add_prep() is provided to dynamically add preparation actions
> to a probe, it seems there might be a need for a corresponding
> damon_destroy_prep() or damon_del_prep(). Without exporting such functions
> and with damon_free_prep() restricted to static scope, is there a way for
> API consumers to dynamically remove or free a prep without destroying the
> entire probe?

Users will be able to do that via damon_destroy_ctx().

> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260728041242.187380-1-sj@kernel.org?part=5


Thanks,
SJ