Re: [RFC PATCH 02/27] mm/damon/core: implement data attribute report queue

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Medium] mm/damon: damon_init() leaks damon_region_cache on queue
  alloc failure
- [Low] mm/damon: missing static qualifier on damon_report_queue in
  core.c
- [Low] mm/damon: struct damon_report_queue comment documents head not
  nr_reports
--

commit 10a61bc9bab6e48534e4a8552380aecaccbee97a
Author: SJ Park <sj@kernel.org>

mm/damon/core: implement data attribute report queue

This commit adds an internal damon_report_queue structure, allocates its
buffer for up to 1,000 reports in damon_init(), and checks that buffer in
damon_initialized().

> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index 0a056949604f9..3f87a7eec442e 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
> @@ -2334,6 +2334,25 @@ int damos_walk(struct damon_ctx *ctx, struct damos_walk_control *control)
>  	return 0;
>  }
>  
> +/*
> + * struct damon_report_queue - &struct damon_report queue.
> + *
> + * @lock:	Represent if this queue is being used by someone.
> + * @head:	Head of the queue.
> + * @tail:	Tail of the queue.
> + * @size:	Size of the queue.
> + * @reports:	Array of &struct damon_report.
> + */
> +struct damon_report_queue {
> +	atomic_t lock;
> +	unsigned int tail;
> +	unsigned int size;
> +	unsigned int nr_reports;
> +	struct damon_report *reports;
> +};

[Severity: Low]
The comment above struct damon_report_queue documents @head, whereas the
struct defines nr_reports instead.

> +
> +struct damon_report_queue damon_report_queue;

[Severity: Low]
Should damon_report_queue be marked static?  struct damon_report_queue is
private to mm/damon/core.c, so leaving damon_report_queue non-static
exports it into the global namespace and triggers sparse and W=1 missing
variable declaration warnings.

> +
>  /*
>   * Reset the aggregated monitoring results ('nr_accesses' of each region).
>   */
> @@ -4308,7 +4327,7 @@ void damon_update_region_access_rate(struct damon_region *r, bool accessed)
>   */
>  bool damon_initialized(void)
>  {
> -	return damon_region_cache != NULL;
> +	return damon_region_cache && damon_report_queue.reports;
>  }
>  
>  static int __init damon_init(void)
> @@ -4319,6 +4338,14 @@ static int __init damon_init(void)
>  		return -ENOMEM;
>  	}
>  
> +	damon_report_queue.size = 1000;
> +	damon_report_queue.reports = kmalloc_objs(*damon_report_queue.reports,
> +			1000, GFP_KERNEL);
> +	if (!damon_report_queue.reports) {
> +		pr_err("report queue alloc fail\n");
> +		return -ENOMEM;
> +	}

[Severity: Medium]
Does this error path leak damon_region_cache when kmalloc_objs() fails?
Destroying damon_region_cache and resetting it (along with
damon_report_queue.size) before returning -ENOMEM would avoid leaking the
slab cache and leaving partially initialized state.

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