Hi SeongJae and Simon,

On 5/31/2025 4:40 AM, SeongJae Park wrote:

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.

Sorry for the late response, I also had some difficulty to find its original
patch and I just found it and replied at the following links.
https://lore.kernel.org/linux-mm/20250528111038.18378-3-wangchuanguo@inspur.com (local)
https://lore.kernel.org/linux-mm/74a7db85-8fcc-4bd5-8656-0f4d0670f205@sk.com (local)

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 (王传国) [off-list ref] wrote:

[...]

quoted

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.

quoted

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

I agree this is a behavior change.

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 can see alloc_migrate_folio() also calls alloc_migration_target(), but do you
mean alloc_migrate_folio() setting mtc->nmask to NULL is the difference?

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.

quoted

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 also think restoring the both mtc->nmask and mtc->gfp_mask are needed.

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

quoted

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

I also think this makes our use case unexpected and cannot prevent migration is
done beyond other side of socket.

quoted

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

I believe hope this is answered above.


Thanks,
SJ

[...]

Thanks,
Honggyu