Message-ID: <d8e3000cfadb443681fabad65093b462@inspur.com>
Date: Fri, 30 May 2025 08:04:42 +0000
From: Simon Wang (王传国) <wangchuanguo@...pur.com>
To: SeongJae Park <sj@...nel.org>
CC: "akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
	"hannes@...xchg.org" <hannes@...xchg.org>, "david@...hat.com"
	<david@...hat.com>, "mhocko@...nel.org" <mhocko@...nel.org>,
	"zhengqi.arch@...edance.com" <zhengqi.arch@...edance.com>,
	"shakeel.butt@...ux.dev" <shakeel.butt@...ux.dev>,
	"lorenzo.stoakes@...cle.com" <lorenzo.stoakes@...cle.com>,
	"linux-mm@...ck.org" <linux-mm@...ck.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "damon@...ts.linux.dev"
	<damon@...ts.linux.dev>
Subject: Re: [PATCH 2/2] mm/damon/sysfs-schemes: add use_nodes_of_tier on
 sysfs-schemes


> > > > This patch adds use_nodes_of_tier under
> > > >
> > >
> /sys/kernel/mm/damon/admin/kdamonds/<N>/contexts/<N>/schemes/<N>/
> > > >
> > > > The 'use_nodes_of_tier' can be used to select nodes within the
> > > > same memory tier of target_nid for DAMOS actions such as
> > > DAMOS_MIGRATE_{HOT,COLD}.
> > >
> > > Could you please elaborate in what setup you think this option is
> > > useful, and measurement of the usefulness if you have?
> > >
> > > I'm asking the above question because of below reasons.  My
> > > anticiapted usage of DAMOS_MIGRATE_{HOT,COLD} is for not only memory
> > > tiering but generic NUMA node management.  And my proposed usage of
> > > these for memory tiering is making per-node promotion/demotion for
> > > gradually promoting and demoting pages step by step between node.
> > > It could be slow but I anticipate such slow but continued
> > > promotion/demotion is more important for reliable performance on
> production systems of large time scale.
> > > And I believe the approach can be applied to general NUMA nodes
> > > management, once DAMON is extended for per-CPU access monitoring.
> > >
> > > I'm not saying this change is not useful, but asking you to give me
> > > a chance to learn your changes, better.
> >
> > I believe some users may want to ​​use only the target node's memory​​
> > and reserve other nodes in the same tier for specific applications.
> > Therefore, I added a switch file use_nodes_of_tier.
> 
> Thank you for clarifying, Simon.
> 
> Because this is an ABI change that difficult to revert and therefore we may
> need to support for long term, I'd like to have more clear theory and/or data if
> possible.  In my humble opinion, above clarification doesn't sound like a
> strong enough justification for ABI change.
> 
> More specifically, it would be better if you could answer below questions.
> Who would be such users, how common the use case would be, and what are
> the benefit of doing so?  Is that only theory?  Or, a real existing use case?
> Can you share measurement of the benefit from this change that measured
> from real workloads or benchmarks?  Is there an alternative way to do this
> without ABI change?


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

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

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?
​​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?
​​Can we confirm these two points are agreed upon?​
 
> > I think it might be better to set the default value of
> > use_nodes_of_tier to true (i.e., allow using fallback nodes). What do
> > you think
> 
> In my humble opinion, we can consider setting it true by default, if we agree
> the benefit of the change is significant.  With only currently given information,
> I cannot easily say if I think this can really be useful.  As asked abovely, more
> clear thoery and/or real data would be helpful.
> 
> >
> > > >
> > > > Signed-off-by: wangchuanguo <wangchuanguo@...pur.com>
> > > > ---
> > > >  include/linux/damon.h        |  9 ++++++++-
> > > >  include/linux/memory-tiers.h |  5 +++++
> > > >  mm/damon/core.c              |  6 ++++--
> > > >  mm/damon/lru_sort.c          |  3 ++-
> > > >  mm/damon/paddr.c             | 19 ++++++++++++-------
> > > >  mm/damon/reclaim.c           |  3 ++-
> > > >  mm/damon/sysfs-schemes.c     | 31
> > > ++++++++++++++++++++++++++++++-
> > > >  mm/memory-tiers.c            | 13 +++++++++++++
> > > >  samples/damon/mtier.c        |  3 ++-
> > > >  samples/damon/prcl.c         |  3 ++-
> > > >  10 files changed, 80 insertions(+), 15 deletions(-)
> > >
> > > Can we please make this change more separated?  Maybe we can split
> > > the change for memory-tiers.c, DAMON core layer, and DAMON sysfs
> interface.
> > > That will make review much easier.
> >
> > Yes,I'll split this patch to be 2 patches.
> 
> Thank you for accepting my suggestion.  But I think it deserves 3 patches,
> each for
> 
> - memory-tiers.c,
> - DAMON core layer, and
> - and DAMON sysfs interface.
> 
> But, let's further discuss on the high level topic (if this change is really
> beneficial enough to make ABI change).
> 
> 
> Thanks,
> SJ
> 
> [...]

Powered by blists - more mailing lists