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

[Honggyu Kim <honggyu.kim@sk.com>]

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

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

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