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

On Sun, 26 Jul 2026 17:52:27 +0000 [email protected] wrote:

> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Medium] Missing synchronization of the `preps` list during DAMON context commit operations.
> - [Low] The parameter names in `damon_for_each_prep` and `damon_for_each_prep_safe` are reversed compared to their semantic meaning.
> --
> 
> --- Patch [5]: [RFC PATCH v2 05/17] mm/damon/core: introduce damon_prep struct ---
> commit 9f7bafc6bbe2ee2b7391d751512d413fcf3bdf98
> Author: SJ Park <[email protected]>
> 
> mm/damon/core: introduce damon_prep struct
> 
> This commit introduces a new data type damon_prep to specify preparation
> actions for DAMON probes. It adds lists and iteration macros for these
> actions, as well as an initial action DAMON_PREP_SET_PGIDLE.
> 
> > diff --git a/include/linux/damon.h b/include/linux/damon.h
> > index a8bb9ee6979e3..24b362fcf53b1 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]
> Since this patch introduces the preps list to struct damon_probe, should
> damon_commit_probes() in mm/damon/core.c be updated to synchronize this new
> list?
> 
> Currently, damon_commit_probes() copies probe parameters and filters, but
> it appears to omit the preps list. If a context is updated via
> damon_commit_ctx(), wouldn't any configured preparation actions be silently
> dropped for new probes, and existing probes fail to update their preps lists?
> 
> For example:
> 
> mm/damon/core.c:damon_commit_probes() {
>     ...
>     damon_for_each_probe_safe(dst_probe, next, dst) {
>         ...
>         err = damon_commit_filters(dst_probe, src_probe);
>         ...
>     }
> 
>     damon_for_each_probe_safe(src_probe, next, src) {
>         ...
>         new_probe = damon_new_probe();
>         ...
>         err = damon_commit_filters(new_probe, src_probe);
>     }
>     return 0;
> }

A later patch of this series does that.

> 
> [ ... ]
> 
> > @@ -959,6 +982,12 @@ static inline unsigned long damon_sz_region(struct damon_region *r)
> >  	return r->ar.end - r->ar.start;
> >  }
> >  
> > +#define damon_for_each_prep(p, prep) \
> > +	list_for_each_entry(p, &(prep)->preps, list)
> > +
> > +#define damon_for_each_prep_safe(p, next, prep) \
> > +	list_for_each_entry_safe(p, next, &(prep)->preps, list)
> > +
> 
> [Severity: Low]
> Are the macro parameter names here reversed compared to their semantic meaning?
> 
> The parameter 'p' is used as the list iterator (which represents a prep), and
> 'prep' is used as the parent structure (which represents a probe). This differs
> from the convention used in damon_for_each_filter(f, p). 
> 
> Will this force usage like damon_for_each_prep_safe(prep, prep_next, p), which
> could be confusing to callers?

Good catch!  I will s/prep/probe/ like below.

'''
--- a/include/linux/damon.h
+++ b/include/linux/damon.h
@@ -1123,11 +1123,11 @@ static inline unsigned long damon_sz_region(struct damon_region *r)
        return r->ar.end - r->ar.start;
 }

-#define damon_for_each_prep(p, prep) \
-       list_for_each_entry(p, &(prep)->preps, list)
+#define damon_for_each_prep(p, probe) \
+       list_for_each_entry(p, &(probe)->preps, list)

-#define damon_for_each_prep_safe(p, next, prep) \
-       list_for_each_entry_safe(p, next, &(prep)->preps, list)
+#define damon_for_each_prep_safe(p, next, probe) \
+       list_for_each_entry_safe(p, next, &(probe)->preps, list)

 #define damon_for_each_filter(f, p) \
        list_for_each_entry(f, &(p)->filters, list)
'''

> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5


Thanks,
SJ