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

[wangchuanguo]

On Fri, 30 May 2025 12:40:16 -0700 SeongJae Park <sj@xxxxxxxxxx> 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.
> 
> 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.
> 
> 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
> 
> [...]

Sent using hkml (https://github.com/sjp38/hackermail)