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

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;
}

[ ... ]

> @@ -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?

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