Re: [RFC PATCH v2.2 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] 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 <[email protected]>

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?

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

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