Re: [RFC PATCH v2 3/4] mm/damon: New module with hot application detection

[SeongJae Park <sj@kernel.org>]

On Wed, 11 Mar 2026 16:45:06 +0300 Gutierrez Asier <gutierrez.asier@huawei-partners.com> wrote:

> 
> 
> On 3/11/2026 7:11 AM, SeongJae Park wrote:
> > On Tue, 10 Mar 2026 16:24:19 +0000 <gutierrez.asier@huawei-partners.com> wrote:
> > 
> >> From: Asier Gutierrez <gutierrez.asier@huawei-partners.com>
> > 
> > I'd like to discuss about this in a high level, but I'd like to do that on the
> > cover letter.  To this patch, I'm therefore adding comments for only details
> > that stand out immeadiately to me.
> 
> I will update the cover letter to add more details for the next version.
> 
> >>
> >> 1. It first launches a new kthread called damon_dynamic. This thread
> >> will behave as a supervisor, launching new kdamond threads for all
> >> the processes we want to montiored. The tasks are sorted
> >> by utime delta. For the top N tasks, a new kdamond thread will be
> >> launched.
> > 
> > If I read the cover letter and the code of this patch correctly, seems the last
> > two sentences of the above paragraph is outdated.  Please correct me if I'm
> > wrong.
> 
> Correct, I forgot to delete it.

Thank you for confirming that, and accepting my suggestions below.  Most of
your answers to my comments make sense, so I'm cutting most of the part except
places where I want to clarify my points or answer your questions below.

[...]
> >> +};
> >> +DEFINE_DAMON_MODULES_DAMOS_TIME_QUOTA(damon_hugepage_quota);
> >> +
> >> +static struct damos_watermarks damon_hugepage_wmarks = {
> >> +	.metric = DAMOS_WMARK_FREE_MEM_RATE,
> >> +	.interval = 5000000, /* 5 seconds */
> >> +	.high = 900, /* 90 percent */
> >> +	.mid = 800, /* 80 percent */
> >> +	.low = 50, /* 5 percent */
> >> +};
> >> +DEFINE_DAMON_MODULES_WMARKS_PARAMS(damon_hugepage_wmarks);
> > 
> > In the RFC v1 discussion, you also mentioned you will remove the above, but
> > seems it is forgotten?
> > 
> > Also, my understadning of your reply on the previous discussion was that you
> > don't really want to use watermarks in this module, and therefore you want to
> > remove it.  Am I correct?  If I'm not, maybe I'm still not understanding the
> > intention of the watermarks.  Can you please enlighten me?
> 
> Since I refactored heavily the previous code, I forgot to delete this bit.
> I will remove it now.

Thank you for clarifying this :)

> 
> >> +
> >> +static struct damon_attrs damon_hugepage_mon_attrs = {
> >> +	.sample_interval = 5000, /* 5 ms */
> >> +	.aggr_interval = 100000, /* 100 ms */
> >> +	.ops_update_interval = 0,
> > 
> > Is ops_update_interval = 0 really your intention?
> 
> I didn't think about it. I will set it to 60 seconds, as in other modules.

Sounds good.

When 'vaddr' operation set is used, for every ops_update_interval, 'vaddr'
updates monitoring target regions boundary to cover all virtual address
mappings.  If it is '0', it will do the update on every iteration of the
kdamond main loop.  It can result in unnecessary overhead.  So I was asking the
above question.  Setting it to 60 seconds sounds good to me.

[...]
> >> +	err = damon_modules_new_ctx_target(&param_ctx, &param_target,
> >> +					   DAMON_OPS_VADDR);
> >> +	if (err)
> >> +		return err;
> > 
> > You should deallocate param_ctx before returning the error.
> 
> Not sure about this. As far as I know, damon_modules_new_paddr_ctx_target
> will destroy the context in case there is an error. In fact, reclaim and 
> lru_sort modules just return error when damon_modules_new_paddr_ctx_target
> fails, no cleanup used.

You're correct, I was trying to comment only below, but somehow confused the
place.  Sorry for the confusion.

[...]
> >> +
> >> +		list_for_each_entry_safe(monitored_task, tmp, monitored_tasks, list) {
> > 
> > Please wrap lines for the 80 columns limit.
> 
> Interesting, when check it, i was less than 80 columns. Even if I copy this line
> and paste it into any text editor, I still get less than 80 characters.

Oh, you must be correct.  Maybe my mailing tool was showing it incorrectly.
Please keep it as is if it doesn't exceed 80 columns.

Thanks,
SJ

[...]