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

[SeongJae Park]

On Mon, 9 Jun 2025 21:39:50 +0900 Honggyu Kim <honggyu.kim@xxxxxx> wrote:

> Hi SeongJae and Simon,
> 
> On 5/31/2025 4:40 AM, SeongJae Park wrote:
[...]
> > On Fri, 30 May 2025 08:04:42 +0000 Simon Wang (王传国) <wangchuanguo@xxxxxxxxxx> wrote:
[...]
> > 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?

Yes, and also alloc_migration_target()'s internal optimizations for demotion
use case.

Nonetheless, I'm saying about the differences between DAMOS_MIGRATE_{HOT,COLD}
and move_pages() behaviors in the bigger context.

> 
> > 
> > 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 will send an RFC for this soon, to make discussions easier and unblocked.


Thanks,
SJ

[...]