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

[SJ Park <sj@kernel.org>]

On Sun, 15 Nov 2026 20:41:23 +0000 sashiko-bot@kernel.org wrote:

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

Nice catch.  I will remove the comment for @head and add a comment for
@nr_reports.
> 
> > +
> > +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.

Good suggestion, I will mark it as static.

> 
> > +
> >  /*
> >   * 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.

Good suggestion.  I will do destroy damon_region_cache.

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


Thanks,
SJ