Re: [PATCH 2/2] mm/damon/sysfs-schemes: add use_nodes_of_tier on sysfs-schemes

[Honggyu Kim]

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@xxxxxxxxxx
https://lore.kernel.org/linux-mm/74a7db85-8fcc-4bd5-8656-0f4d0670f205@xxxxxx

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

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

> > ​​Can we confirm these two points are agreed upon?​
>
> I believe hope this is answered above.
>
>
> Thanks,
> SJ
>
> [...]

Thanks,
Honggyu