Message-Id: <20250530194016.51798-1-sj@kernel.org>
Date: Fri, 30 May 2025 12:40:16 -0700
From: SeongJae Park <sj@...nel.org>
To: Simon Wang <wangchuanguo@...pur.com>
Cc: SeongJae Park <sj@...nel.org>,
	"akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
	"hannes@...xchg.org" <hannes@...xchg.org>,
	"david@...hat.com" <david@...hat.com>,
	"mhocko@...nel.org" <mhocko@...nel.org>,
	"zhengqi.arch@...edance.com" <zhengqi.arch@...edance.com>,
	"shakeel.butt@...ux.dev" <shakeel.butt@...ux.dev>,
	"lorenzo.stoakes@...cle.com" <lorenzo.stoakes@...cle.com>,
	"linux-mm@...ck.org" <linux-mm@...ck.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"damon@...ts.linux.dev" <damon@...ts.linux.dev>,
	Honggyu Kim <honggyu.kim@...com>
Subject: Re: [PATCH 2/2] mm/damon/sysfs-schemes: add use_nodes_of_tier on sysfs-schemes

Hi Simon,


Thank you for continuing this important discussion.

Before starting, though, seems your mail client is not setting 'In-Reply-To'
field of your mails.  For people who uses 'In-Reply-To' field based threads
displaying tools, ths thread could be difficult to read the whole contents.
Please consider using tools that set the field correctly if possible.

You could get more information about available mailing tools from
https://docs.kernel.org/process/email-clients.html

Btw, I use hkml
(https://docs.kernel.org/process/email-clients.html#hackermail-tui) ;)

On Fri, 30 May 2025 08:04:42 +0000 Simon Wang (王传国) <wangchuanguo@...pur.com> wrote:

[...]
> Your concern is that adding the bool use_nodes_of_tier variable and introducing 
> an additional parameter to multiple functions would cause ABI changes, correct?​​

You are correct.

> 
> ​​I propose avoiding the creation of the 'use_nodes_of_tier' sysfs
> file. Instead, we can modify the __damon_pa_migrate_folio_list() function to
> change the allowed_mask from NODE_MASK_NONE to the full node mask of the
> entire tier where the target_nid resides.  This approach would be similar to
> the implementation in commit 320080272892 ('mm/demotion: demote pages
> according to allocation fallback order').

Then, this causes a behavior change, which we should not allow if it can be
considered a regression.  In other words, we could do this if it is a clear
improvement.

So, let's think about if your proposed change is an improvement.  As the commit
320080272892 is nicely explaining, I think that it is an improved behavior for
demotion.  Actually it seems good behavior for promotion, too.  But, the
behavior we are discussing here is not for the demotion but general migration
(specifically, DAMOS_MIGRATE_{HOT,COLD}).

In my opinion, DAMOS_MIGRATE_{HOT,COLD} behavior should be somewhat similar to
that of move_pages() syscall, to make its behavior easy to expect.  So I think
having commit 320080272892's behavior improvement to DAMOS_MIGRATE_{HOT,COLD}
is not a right thing to do.

And this asks me a question.  Is current DAMOS_MIGRATE_{HOT,COLD} behavior
similar to move_pages() syscall?  Not really, since do_move_pages_to_node(),
which is called from move_pages() syscall and calls migrate_pages() is setting
mtc->nmask as NULL, while DAMOS_MIGRATE_{HOT,COLD} set it as NODE_MASK_NONE.
Also, do_move_pages_to_node() uses alloc_migration_target() while
DAMOS_MIGRATE_{HOT,COLD} uses alloc_migrate_folio().

I overlooked this different behavior while reviewing this code, sorry.  And I
don't think this difference is what we need to keep, unless there are good
rasons that well documented.  Thank you for let us find this, Simon.

So I suggest to set mtc->nmask as NULL, and use alloc_migration_target() from
__damon_pa_migrate_folio_list(), same to move_pages() system call.  To use
alloc_migrate_folio() from __damon_pa_migrate_folio_list(), we renamed it from
alloc_demote_folio(), and made it none-static.  If we use
alloc_migration_target() from __damon_pa_migrate_folio_list(), there is no
reason to keep the changes.  Let's revert those too.

Cc-ing Honggyu, who originally implemented the current behavior of
__damon_pa_migrate().  Honggyu, could you please let us know if the above
suggested changes are not ok for you?

If Honggyu has no problem at the suggested change, Simon, would you mind doing
that?  I can also make the patches.  I don't really care who do that.  I just
think someone should do that.  This shouldn't be urgent real issue, in my
opinion, though.

> 
> I'd like to confirm two modification points with you:
> ​​1.Regarding alloc_migrate_folio()​​:
> Restoring the original nodemask and gfp_mask in this function is the correct approach, correct?

I think that's correct, but let's discuss about the patch on the patch's
thread.

> ​​2.Regarding DAMON's migration logic​​:
> The target scope should be expanded from a single specified node to the entire memory tier
>  (where the target node resides), correct?

I don't think so, as abovely explained.

> ​​Can we confirm these two points are agreed upon?​

I believe hope this is answered above.


Thanks,
SJ

[...]

Powered by blists - more mailing lists