2024-01-15 04:53:21

by Honggyu Kim

Subject: [RFC PATCH 0/4] DAMON based 2-tier memory management for CXL memory

There was an RFC IDEA "DAMOS-based Tiered-Memory Management" previously
posted at [1].

It says there is no implementation of the demote/promote DAMOS action
are made. This RFC is about its implementation for physical address
space.


Introduction
============

With the advent of CXL/PCIe attached DRAM, which will be called simply
as CXL memory in this cover letter, some systems are becoming more
heterogenous having memory systems with different latency and bandwidth
characteristics. They are usually handled as different NUMA nodes in
separate memory tiers and CXL memory is used as slow tiers because of
its protocol overhead compared to local DRAM.

In this kind of systems, we need to be careful placing memory pages on
proper NUMA nodes based on the memory access frequency. Otherwise, some
frequently accessed pages might reside on slow tiers and it makes
performance degradation unexpectedly. Moreover, the memory access
patterns can be changed at runtime.

To handle this problem, we need a way to monitor the memory access
patterns and migrate pages based on their access temperature. The
DAMON(Data Access MONitor) framework and its DAMOS(DAMON-based Operation
Schemes) can be useful features for monitoring and migrating pages.
DAMOS provides multiple actions based on DAMON monitoring results and it
can be used for proactive reclaim, which means swapping cold pages out
with DAMOS_PAGEOUT action, but it doesn't support migration actions such
as demotion and promotion between tiered memory nodes.

This series supports two new DAMOS actions; DAMOS_DEMOTE for demotion
from fast tiers and DAMOS_PROMOTE for promotion from slow tiers. This
prevents hot pages from being stuck on slow tiers, which makes
performance degradation and cold pages can be proactively demoted to
slow tiers so that the system can increase the chance to allocate more
hot pages to fast tiers.

The DAMON provides various tuning knobs but we found that the proactive
demotion for cold pages is especially useful when the system is running
out of memory on its fast tier nodes.

Our evaluation result shows that it reduces the performance slowdown
compared to the default memory policy from 15~17% to 4~5% when the
system runs under high memory pressure on its fast tier DRAM nodes.


DAMON configuration
===================
The specific DAMON configuration doesn't have to be in the scope of this
patch series, but some rough idea is better to be shared to explain the
evaluation result.

The DAMON provides many knobs for fine tuning but its configuration file
is generated by HMSDK[2]. It includes gen_config.py script that
generates a json file with the full config of DAMON knobs and it creates
multiple kdamonds for each NUMA node when the DAMON is enabled so that
it can run hot/cold based migration for tiered memory.


Evaluation Workload
===================

The performance evaluation is done with redis[3], which is a widely used
in-memory database and the memory access patterns are generated via
YCSB[4]. We have measured two different workloads with zipfian and
latest distributions but their configs are slightly modified to make
memory usage higher and execution time longer for better evaluation.

The idea of evaluation using these demote and promote actions covers
system-wide memory management rather than partitioning hot/cold pages of
a single workload. The default memory allocation policy creates pages
to the fast tier DRAM node first, then allocates newly created pages to
the slow tier CXL node when the DRAM node has insufficient free space.
Once the page allocation is done then those pages never move between
NUMA nodes. It's not true when using numa balancing, but it is not the
scope of this DAMON based 2-tier memory management support.

If the working set of redis can be fit fully into the DRAM node, then
the redis will access the fast DRAM only. Since the performance of DRAM
only is faster than partially accessing CXL memory in slow tiers, this
environment is not useful to evaluate this patch series.

To make pages of redis be distributed across fast DRAM node and slow
CXL node to evaluate our demote and promote actions, we pre-allocate
some cold memory externally using mmap and memset before launching
redis-server. We assumed that there are enough amount of cold memory in
datacenters as TMO[5] and TPP[6] papers mentioned.

The evaluation sequence is as follows.

1. Turn on DAMON with DAMOS_DEMOTE action for DRAM node and
DAMOS_PROMOTE action for CXL node. It demotes cold pages on DRAM
node and promotes hot pages on CXL node in a regular interval.
2. Allocate a huge block of cold memory by calling mmap and memset at
the fast tier DRAM node, then make the process sleep to make the fast
tier has insufficient memory for redis-server.
3. Launch redis-server and load prebaked snapshot image, dump.rdb. The
redis-server consumes 52GB of anon pages and 33GB of file pages, but
due to the cold memory allocated at 2, it fails allocating the entire
memory of redis-server on the fast tier DRAM node so it partially
allocates the remaining on the slow tier CXL node. The ratio of
DRAM:CXL depends on the size of the pre-allocated cold memory.
4. Run YCSB to make zipfian or latest distribution of memory accesses to
redis-server, then measure its execution time when it's completed.
5. Repeat 4 over 50 times to measure the average execution time for each
run.
6. Increase the cold memory size then repeat goes to 2.

For each test at 4 took about a minute so repeating it 50 times almost
took about 1 hour for each test with a specific cold memory from 440GB
to 500GB in 10GB increments for each evaluation. So it took about more
than 10 hours for both zipfian and latest workloads to get the entire
evaluation results. Repeating the same test set multiple times doesn't
show much difference so I think it might be enough to make the result
reliable.


Evaluation Results
==================

All the result values are normalized to DRAM-only execution time because
the workload cannot be faster than DRAM-only unless the workload hits
the bandwidth peak but our redis test doesn't go beyond the bandwidth
limit.

So the DRAM-only execution time is the ideal result without affected by
the gap between DRAM and CXL performance difference. The NUMA node
environment is as follows.

node0 - local DRAM, 512GB with a CPU socket (fast tier)
node1 - disabled
node2 - CXL DRAM, 96GB, no CPU attached (slow tier)

The following is the result of generating zipfian distribution to
redis-server and the numbers are averaged by 50 times of execution.

1. YCSB zipfian distribution read only workload
memory pressure with cold memory on node0 with 512GB of local DRAM.
=============+================================================+=========
| cold memory occupied by mmap and memset |
| 0G 440G 450G 460G 470G 480G 490G 500G |
=============+================================================+=========
Execution time normalized to DRAM-only values | GEOMEAN
-------------+------------------------------------------------+---------
DRAM-only | 1.00 - - - - - - - | 1.00
CXL-only | 1.21 - - - - - - - | 1.21
default | - 1.09 1.10 1.13 1.15 1.18 1.21 1.21 | 1.15
DAMON 2-tier | - 1.02 1.04 1.05 1.04 1.05 1.05 1.06 | 1.04
=============+================================================+=========
CXL usage of redis-server in GB | AVERAGE
-------------+------------------------------------------------+---------
DRAM-only | 0.0 - - - - - - - | 0.0
CXL-only | 52.6 - - - - - - - | 52.6
default | - 19.4 26.1 32.3 38.5 44.7 50.5 50.3 | 37.4
DAMON 2-tier | - 0.1 1.6 5.2 8.0 9.1 11.8 13.6 | 7.1
=============+================================================+=========

Each test result is based on the exeuction environment as follows.

DRAM-only : redis-server uses only local DRAM memory.
CXL-only : redis-server uses only CXL memory.
default : default memory policy(MPOL_DEFAULT).
numa balancing disabled.
DAMON 2-tier: DAMON enabled with DAMOS_DEMOTE for DRAM nodes and
DAMOS_PROMOTE for CXL nodes.

The above result shows the "default" execution time goes up as the size
of cold memory is increased from 440G to 500G because the more cold
memory used, the more CXL memory is used for the target redis workload
and this makes the execution time increase.

However, "DAMON 2-tier" result shows less slowdown because the
DAMOS_DEMOTE action at DRAM node proactively demotes pre-allocated cold
memory to CXL node and this free space at DRAM increases more chance to
allocate hot or warm pages of redis-server to fast DRAM node. Moreover,
DEMOS_PROMOTE action at CXL node also promotes hot pages of redis-server
to DRAM node actively.

As a result, it makes more memory of redis-server stay in DRAM node
compared to "default" memory policy and this makes the performance
improvement.

The following result of latest distribution workload shows similar data.

2. YCSB latest distribution read only workload
memory pressure with cold memory on node0 with 512GB of local DRAM.
=============+================================================+=========
| cold memory occupied by mmap and memset |
| 0G 440G 450G 460G 470G 480G 490G 500G |
=============+================================================+=========
Execution time normalized to DRAM-only values | GEOMEAN
-------------+------------------------------------------------+---------
DRAM-only | 1.00 - - - - - - - | 1.00
CXL-only | 1.18 - - - - - - - | 1.18
default | - 1.16 1.15 1.17 1.18 1.16 1.18 1.15 | 1.17
DAMON 2-tier | - 1.04 1.04 1.05 1.05 1.06 1.05 1.06 | 1.05
=============+================================================+=========
CXL usage of redis-server in GB | AVERAGE
-------------+------------------------------------------------+---------
DRAM-only | 0.0 - - - - - - - | 0.0
CXL-only | 52.6 - - - - - - - | 52.6
default | - 19.3 26.1 32.2 38.5 44.6 50.5 50.6 | 37.4
DAMON 2-tier | - 1.3 3.8 7.0 4.1 9.4 12.5 16.7 | 7.8
=============+================================================+=========

In summary of both results, our evaluation shows that "DAMON 2-tier"
memory management reduces the performance slowdown compared to the
"default" memory policy from 15~17% to 4~5% when the system runs with
high memory pressure on its fast tier DRAM nodes.

The similar evaluation was done in another machine that has 256GB of
local DRAM and 96GB of CXL memory. The performance slowdown is reduced
from 20~24% for "default" to 5~7% for "DAMON 2-tier".

Having these DAMOS_DEMOTE and DAMOS_PROMOTE actions can make 2-tier
memory systems run more efficiently under high memory pressures.

Signed-off-by: Honggyu Kim <[email protected]>
Signed-off-by: Hyeongtak Ji <[email protected]>
Signed-off-by: Rakie Kim <[email protected]>

[1] https://lore.kernel.org/damon/[email protected]
[2] https://github.com/skhynix/hmsdk
[3] https://github.com/redis/redis/tree/7.0.0
[4] https://github.com/brianfrankcooper/YCSB/tree/0.17.0
[5] https://dl.acm.org/doi/10.1145/3503222.3507731
[6] https://dl.acm.org/doi/10.1145/3582016.3582063

Honggyu Kim (2):
mm/vmscan: refactor reclaim_pages with reclaim_or_migrate_folios
mm/damon: introduce DAMOS_DEMOTE action for demotion

Hyeongtak Ji (2):
mm/memory-tiers: add next_promotion_node to find promotion target
mm/damon: introduce DAMOS_PROMOTE action for promotion

include/linux/damon.h | 4 +
include/linux/memory-tiers.h | 11 ++
include/linux/migrate_mode.h | 1 +
include/linux/vm_event_item.h | 1 +
include/trace/events/migrate.h | 3 +-
mm/damon/paddr.c | 46 ++++++-
mm/damon/sysfs-schemes.c | 2 +
mm/internal.h | 2 +
mm/memory-tiers.c | 43 ++++++
mm/vmscan.c | 231 +++++++++++++++++++++++++++++++--
mm/vmstat.c | 1 +
11 files changed, 330 insertions(+), 15 deletions(-)


base-commit: 0dd3ee31125508cd67f7e7172247f05b7fd1753a
--
2.34.1

2024-01-15 04:53:38

by Honggyu Kim

Subject: [RFC PATCH 3/4] mm/memory-tiers: add next_promotion_node to find promotion target

From: Hyeongtak Ji <[email protected]>

This patch adds next_promotion_node that can be used to identify the
appropriate promotion target based on memory tiers. When multiple
promotion target nodes are available, the nearest node is selected based
on numa distance.

Signed-off-by: Hyeongtak Ji <[email protected]>
---
include/linux/memory-tiers.h | 11 +++++++++
mm/memory-tiers.c | 43 ++++++++++++++++++++++++++++++++++++
2 files changed, 54 insertions(+)

diff --git a/include/linux/memory-tiers.h b/include/linux/memory-tiers.h
index 1e39d27bee41..0788e435fc50 100644
--- a/include/linux/memory-tiers.h
+++ b/include/linux/memory-tiers.h
@@ -50,6 +50,7 @@ int mt_set_default_dram_perf(int nid, struct node_hmem_attrs *perf,
int mt_perf_to_adistance(struct node_hmem_attrs *perf, int *adist);
#ifdef CONFIG_MIGRATION
int next_demotion_node(int node);
+int next_promotion_node(int node);
void node_get_allowed_targets(pg_data_t *pgdat, nodemask_t *targets);
bool node_is_toptier(int node);
#else
@@ -58,6 +59,11 @@ static inline int next_demotion_node(int node)
return NUMA_NO_NODE;
}

+static inline int next_promotion_node(int node)
+{
+ return NUMA_NO_NODE;
+}
+
static inline void node_get_allowed_targets(pg_data_t *pgdat, nodemask_t *targets)
{
*targets = NODE_MASK_NONE;
@@ -101,6 +107,11 @@ static inline int next_demotion_node(int node)
return NUMA_NO_NODE;
}

+static inline int next_promotion_node(int node)
+{
+ return NUMA_NO_NODE;
+}
+
static inline void node_get_allowed_targets(pg_data_t *pgdat, nodemask_t *targets)
{
*targets = NODE_MASK_NONE;
diff --git a/mm/memory-tiers.c b/mm/memory-tiers.c
index 8d5291add2bc..0060ee571cf4 100644
--- a/mm/memory-tiers.c
+++ b/mm/memory-tiers.c
@@ -335,6 +335,49 @@ int next_demotion_node(int node)
return target;
}

+/*
+ * Select a promotion target that is close to the from node among the given
+ * two nodes.
+ *
+ * TODO: consider other decision policy as node_distance may not be precise.
+ */
+static int select_promotion_target(int a, int b, int from)
+{
+ if (node_distance(from, a) < node_distance(from, b))
+ return a;
+ else
+ return b;
+}
+
+/**
+ * next_promotion_node() - Get the next node in the promotion path
+ * @node: The starting node to lookup the next node
+ *
+ * Return: node id for next memory node in the promotion path hierarchy
+ * from @node; NUMA_NO_NODE if @node is the toptier.
+ */
+int next_promotion_node(int node)
+{
+ int target = NUMA_NO_NODE;
+ int nid;
+
+ if (node_is_toptier(node))
+ return NUMA_NO_NODE;
+
+ rcu_read_lock();
+ for_each_node_state(nid, N_MEMORY) {
+ if (node_isset(node, node_demotion[nid].preferred)) {
+ if (target == NUMA_NO_NODE)
+ target = nid;
+ else
+ target = select_promotion_target(nid, target, node);
+ }
+ }
+ rcu_read_unlock();
+
+ return target;
+}
+
static void disable_all_demotion_targets(void)
{
struct memory_tier *memtier;
--
2.34.1

2024-01-15 04:53:47

by Honggyu Kim

Subject: [RFC PATCH 2/4] mm/damon: introduce DAMOS_DEMOTE action for demotion

This patch introduces DAMOS_DEMOTE action, which is similar to
DAMOS_PAGEOUT, but demote folios instead of swapping them out.

Since there are some common routines with pageout, many functions have
similar logics between pageout and demote.

The execution sequence of DAMOS_PAGEOUT and DAMOS_DEMOTE look as follows.

DAMOS_PAGEOUT action
damo_pa_apply_scheme
-> damon_pa_reclaim
-> reclaim_pages
-> reclaim_folio_list
-> shrink_folio_list

DAMOS_DEMOTE action
damo_pa_apply_scheme
-> damon_pa_reclaim
-> demote_pages
-> do_demote_folio_list
-> __demote_folio_list
-> demote_folio_list

__demote_folio_list() is a minimized version of shrink_folio_list(), but
it's minified only for demotion.

Signed-off-by: Honggyu Kim <[email protected]>
---
include/linux/damon.h | 2 +
mm/damon/paddr.c | 17 +++++---
mm/damon/sysfs-schemes.c | 1 +
mm/internal.h | 1 +
mm/vmscan.c | 84 ++++++++++++++++++++++++++++++++++++++++
5 files changed, 99 insertions(+), 6 deletions(-)

diff --git a/include/linux/damon.h b/include/linux/damon.h
index e00ddf1ed39c..4c0a0fef09c5 100644
--- a/include/linux/damon.h
+++ b/include/linux/damon.h
@@ -106,6 +106,7 @@ struct damon_target {
* @DAMOS_LRU_PRIO: Prioritize the region on its LRU lists.
* @DAMOS_LRU_DEPRIO: Deprioritize the region on its LRU lists.
* @DAMOS_STAT: Do nothing but count the stat.
+ * @DAMOS_DEMOTE: Do demotion for the current region.
* @NR_DAMOS_ACTIONS: Total number of DAMOS actions
*
* The support of each action is up to running &struct damon_operations.
@@ -123,6 +124,7 @@ enum damos_action {
DAMOS_LRU_PRIO,
DAMOS_LRU_DEPRIO,
DAMOS_STAT, /* Do nothing but only record the stat */
+ DAMOS_DEMOTE,
NR_DAMOS_ACTIONS,
};

diff --git a/mm/damon/paddr.c b/mm/damon/paddr.c
index 081e2a325778..d3e3f077cd00 100644
--- a/mm/damon/paddr.c
+++ b/mm/damon/paddr.c
@@ -224,7 +224,7 @@ static bool damos_pa_filter_out(struct damos *scheme, struct folio *folio)
return false;
}

-static unsigned long damon_pa_pageout(struct damon_region *r, struct damos *s)
+static unsigned long damon_pa_reclaim(struct damon_region *r, struct damos *s, bool is_demote)
{
unsigned long addr, applied;
LIST_HEAD(folio_list);
@@ -242,14 +242,17 @@ static unsigned long damon_pa_pageout(struct damon_region *r, struct damos *s)
folio_test_clear_young(folio);
if (!folio_isolate_lru(folio))
goto put_folio;
- if (folio_test_unevictable(folio))
+ if (folio_test_unevictable(folio) && !is_demote)
folio_putback_lru(folio);
else
list_add(&folio->lru, &folio_list);
put_folio:
folio_put(folio);
}
- applied = reclaim_pages(&folio_list);
+ if (is_demote)
+ applied = demote_pages(&folio_list);
+ else
+ applied = reclaim_pages(&folio_list);
cond_resched();
return applied * PAGE_SIZE;
}
@@ -297,13 +300,15 @@ static unsigned long damon_pa_apply_scheme(struct damon_ctx *ctx,
{
switch (scheme->action) {
case DAMOS_PAGEOUT:
- return damon_pa_pageout(r, scheme);
+ return damon_pa_reclaim(r, scheme, false);
case DAMOS_LRU_PRIO:
return damon_pa_mark_accessed(r, scheme);
case DAMOS_LRU_DEPRIO:
return damon_pa_deactivate_pages(r, scheme);
case DAMOS_STAT:
break;
+ case DAMOS_DEMOTE:
+ return damon_pa_reclaim(r, scheme, true);
default:
/* DAMOS actions that not yet supported by 'paddr'. */
break;
@@ -317,11 +322,11 @@ static int damon_pa_scheme_score(struct damon_ctx *context,
{
switch (scheme->action) {
case DAMOS_PAGEOUT:
+ case DAMOS_LRU_DEPRIO:
+ case DAMOS_DEMOTE:
return damon_cold_score(context, r, scheme);
case DAMOS_LRU_PRIO:
return damon_hot_score(context, r, scheme);
- case DAMOS_LRU_DEPRIO:
- return damon_cold_score(context, r, scheme);
default:
break;
}
diff --git a/mm/damon/sysfs-schemes.c b/mm/damon/sysfs-schemes.c
index fe0fe2562000..ac7cd3f17b12 100644
--- a/mm/damon/sysfs-schemes.c
+++ b/mm/damon/sysfs-schemes.c
@@ -1187,6 +1187,7 @@ static const char * const damon_sysfs_damos_action_strs[] = {
"lru_prio",
"lru_deprio",
"stat",
+ "demote",
};

static struct damon_sysfs_scheme *damon_sysfs_scheme_alloc(
diff --git a/mm/internal.h b/mm/internal.h
index b61034bd50f5..2380397ec2f3 100644
--- a/mm/internal.h
+++ b/mm/internal.h
@@ -869,6 +869,7 @@ extern void set_pageblock_order(void);
unsigned long reclaim_pages(struct list_head *folio_list);
unsigned int reclaim_clean_pages_from_list(struct zone *zone,
struct list_head *folio_list);
+unsigned long demote_pages(struct list_head *folio_list);
/* The ALLOC_WMARK bits are used as an index to zone->watermark */
#define ALLOC_WMARK_MIN WMARK_MIN
#define ALLOC_WMARK_LOW WMARK_LOW
diff --git a/mm/vmscan.c b/mm/vmscan.c
index 7ca2396ccc3b..eaa3dd6b7562 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -998,6 +998,66 @@ static bool may_enter_fs(struct folio *folio, gfp_t gfp_mask)
return !data_race(folio_swap_flags(folio) & SWP_FS_OPS);
}

+/*
+ * __demote_folio_list() returns the number of demoted pages
+ */
+static unsigned int __demote_folio_list(struct list_head *folio_list,
+ struct pglist_data *pgdat, struct scan_control *sc)
+{
+ LIST_HEAD(ret_folios);
+ LIST_HEAD(demote_folios);
+ unsigned int nr_demoted = 0;
+
+ if (next_demotion_node(pgdat->node_id) == NUMA_NO_NODE)
+ return 0;
+
+ cond_resched();
+
+ while (!list_empty(folio_list)) {
+ struct folio *folio;
+ enum folio_references references;
+
+ cond_resched();
+
+ folio = lru_to_folio(folio_list);
+ list_del(&folio->lru);
+
+ if (!folio_trylock(folio))
+ goto keep;
+
+ VM_BUG_ON_FOLIO(folio_test_active(folio), folio);
+
+ references = folio_check_references(folio, sc);
+ if (references == FOLIOREF_KEEP)
+ goto keep_locked;
+
+ /* Relocate its contents to another node. */
+ list_add(&folio->lru, &demote_folios);
+ folio_unlock(folio);
+ continue;
+keep_locked:
+ folio_unlock(folio);
+keep:
+ list_add(&folio->lru, &ret_folios);
+ VM_BUG_ON_FOLIO(folio_test_lru(folio), folio);
+ }
+ /* 'folio_list' is always empty here */
+
+ /* Migrate folios selected for demotion */
+ nr_demoted += demote_folio_list(&demote_folios, pgdat);
+ /* Folios that could not be demoted are still in @demote_folios */
+ if (!list_empty(&demote_folios)) {
+ /* Folios which weren't demoted go back on @folio_list */
+ list_splice_init(&demote_folios, folio_list);
+ }
+
+ try_to_unmap_flush();
+
+ list_splice(&ret_folios, folio_list);
+
+ return nr_demoted;
+}
+
/*
* shrink_folio_list() returns the number of reclaimed pages
*/
@@ -2107,6 +2167,25 @@ static unsigned int reclaim_folio_list(struct list_head *folio_list,
return nr_reclaimed;
}

+static unsigned int do_demote_folio_list(struct list_head *folio_list,
+ struct pglist_data *pgdat)
+{
+ unsigned int nr_demoted;
+ struct folio *folio;
+ struct scan_control sc = {
+ .gfp_mask = GFP_KERNEL,
+ };
+
+ nr_demoted = __demote_folio_list(folio_list, pgdat, &sc);
+ while (!list_empty(folio_list)) {
+ folio = lru_to_folio(folio_list);
+ list_del(&folio->lru);
+ folio_putback_lru(folio);
+ }
+
+ return nr_demoted;
+}
+
static unsigned long reclaim_or_migrate_folios(struct list_head *folio_list,
unsigned int (*handler)(struct list_head *, struct pglist_data *))
{
@@ -2146,6 +2225,11 @@ unsigned long reclaim_pages(struct list_head *folio_list)
return reclaim_or_migrate_folios(folio_list, reclaim_folio_list);
}

+unsigned long demote_pages(struct list_head *folio_list)
+{
+ return reclaim_or_migrate_folios(folio_list, do_demote_folio_list);
+}
+
static unsigned long shrink_list(enum lru_list lru, unsigned long nr_to_scan,
struct lruvec *lruvec, struct scan_control *sc)
{
--
2.34.1

2024-01-15 04:54:11

by Honggyu Kim

Subject: [RFC PATCH 4/4] mm/damon: introduce DAMOS_PROMOTE action for promotion

From: Hyeongtak Ji <[email protected]>

This patch introduces DAMOS_PROMOTE action for paddr mode.

It includes renaming alloc_demote_folio to alloc_migrate_folio to use it
for promotion as well.

The execution sequence of DAMOS_DEMOTE and DAMOS_PROMOTE look as
follows for comparison.

DAMOS_DEMOTE action
damo_pa_apply_scheme
-> damon_pa_reclaim
-> demote_pages
-> do_demote_folio_list
-> __demote_folio_list
-> demote_folio_list

DAMOS_PROMOTE action
damo_pa_apply_scheme
-> damon_pa_promote
-> promote_pages
-> do_promote_folio_list
-> __promote_folio_list
-> promote_folio_list

Signed-off-by: Hyeongtak Ji <[email protected]>
Signed-off-by: Honggyu Kim <[email protected]>
---
include/linux/damon.h | 2 +
include/linux/migrate_mode.h | 1 +
include/linux/vm_event_item.h | 1 +
include/trace/events/migrate.h | 3 +-
mm/damon/paddr.c | 29 ++++++++
mm/damon/sysfs-schemes.c | 1 +
mm/internal.h | 1 +
mm/vmscan.c | 129 ++++++++++++++++++++++++++++++++-
mm/vmstat.c | 1 +
9 files changed, 165 insertions(+), 3 deletions(-)

diff --git a/include/linux/damon.h b/include/linux/damon.h
index 4c0a0fef09c5..477060bb6718 100644
--- a/include/linux/damon.h
+++ b/include/linux/damon.h
@@ -107,6 +107,7 @@ struct damon_target {
* @DAMOS_LRU_DEPRIO: Deprioritize the region on its LRU lists.
* @DAMOS_STAT: Do nothing but count the stat.
* @DAMOS_DEMOTE: Do demotion for the current region.
+ * @DAMOS_PROMOTE: Do promotion if possible, otherwise do nothing.
* @NR_DAMOS_ACTIONS: Total number of DAMOS actions
*
* The support of each action is up to running &struct damon_operations.
@@ -125,6 +126,7 @@ enum damos_action {
DAMOS_LRU_DEPRIO,
DAMOS_STAT, /* Do nothing but only record the stat */
DAMOS_DEMOTE,
+ DAMOS_PROMOTE,
NR_DAMOS_ACTIONS,
};

diff --git a/include/linux/migrate_mode.h b/include/linux/migrate_mode.h
index f37cc03f9369..63f75eb9abf3 100644
--- a/include/linux/migrate_mode.h
+++ b/include/linux/migrate_mode.h
@@ -29,6 +29,7 @@ enum migrate_reason {
MR_CONTIG_RANGE,
MR_LONGTERM_PIN,
MR_DEMOTION,
+ MR_PROMOTION,
MR_TYPES
};

diff --git a/include/linux/vm_event_item.h b/include/linux/vm_event_item.h
index 8abfa1240040..63cf920afeaa 100644
--- a/include/linux/vm_event_item.h
+++ b/include/linux/vm_event_item.h
@@ -44,6 +44,7 @@ enum vm_event_item { PGPGIN, PGPGOUT, PSWPIN, PSWPOUT,
PGDEMOTE_KSWAPD,
PGDEMOTE_DIRECT,
PGDEMOTE_KHUGEPAGED,
+ PGPROMOTE,
PGSCAN_KSWAPD,
PGSCAN_DIRECT,
PGSCAN_KHUGEPAGED,
diff --git a/include/trace/events/migrate.h b/include/trace/events/migrate.h
index 0190ef725b43..f0dd569c1e62 100644
--- a/include/trace/events/migrate.h
+++ b/include/trace/events/migrate.h
@@ -22,7 +22,8 @@
EM( MR_NUMA_MISPLACED, "numa_misplaced") \
EM( MR_CONTIG_RANGE, "contig_range") \
EM( MR_LONGTERM_PIN, "longterm_pin") \
- EMe(MR_DEMOTION, "demotion")
+ EM( MR_DEMOTION, "demotion") \
+ EMe(MR_PROMOTION, "promotion")

/*
* First define the enums in the above macros to be exported to userspace
diff --git a/mm/damon/paddr.c b/mm/damon/paddr.c
index d3e3f077cd00..360ce69d5898 100644
--- a/mm/damon/paddr.c
+++ b/mm/damon/paddr.c
@@ -257,6 +257,32 @@ static unsigned long damon_pa_reclaim(struct damon_region *r, struct damos *s, b
return applied * PAGE_SIZE;
}

+static unsigned long damon_pa_promote(struct damon_region *r, struct damos *s)
+{
+ unsigned long addr, applied;
+ LIST_HEAD(folio_list);
+
+ for (addr = r->ar.start; addr < r->ar.end; addr += PAGE_SIZE) {
+ struct folio *folio = damon_get_folio(PHYS_PFN(addr));
+
+ if (!folio)
+ continue;
+
+ if (damos_pa_filter_out(s, folio))
+ goto put_folio;
+
+ if (!folio_isolate_lru(folio))
+ goto put_folio;
+
+ list_add(&folio->lru, &folio_list);
+put_folio:
+ folio_put(folio);
+ }
+ applied = promote_pages(&folio_list);
+ cond_resched();
+ return applied * PAGE_SIZE;
+}
+
static inline unsigned long damon_pa_mark_accessed_or_deactivate(
struct damon_region *r, struct damos *s, bool mark_accessed)
{
@@ -309,6 +335,8 @@ static unsigned long damon_pa_apply_scheme(struct damon_ctx *ctx,
break;
case DAMOS_DEMOTE:
return damon_pa_reclaim(r, scheme, true);
+ case DAMOS_PROMOTE:
+ return damon_pa_promote(r, scheme);
default:
/* DAMOS actions that not yet supported by 'paddr'. */
break;
@@ -326,6 +354,7 @@ static int damon_pa_scheme_score(struct damon_ctx *context,
case DAMOS_DEMOTE:
return damon_cold_score(context, r, scheme);
case DAMOS_LRU_PRIO:
+ case DAMOS_PROMOTE:
return damon_hot_score(context, r, scheme);
default:
break;
diff --git a/mm/damon/sysfs-schemes.c b/mm/damon/sysfs-schemes.c
index ac7cd3f17b12..1b84d0af7e1f 100644
--- a/mm/damon/sysfs-schemes.c
+++ b/mm/damon/sysfs-schemes.c
@@ -1188,6 +1188,7 @@ static const char * const damon_sysfs_damos_action_strs[] = {
"lru_deprio",
"stat",
"demote",
+ "promote",
};

static struct damon_sysfs_scheme *damon_sysfs_scheme_alloc(
diff --git a/mm/internal.h b/mm/internal.h
index 2380397ec2f3..f159455e63d4 100644
--- a/mm/internal.h
+++ b/mm/internal.h
@@ -870,6 +870,7 @@ unsigned long reclaim_pages(struct list_head *folio_list);
unsigned int reclaim_clean_pages_from_list(struct zone *zone,
struct list_head *folio_list);
unsigned long demote_pages(struct list_head *folio_list);
+unsigned long promote_pages(struct list_head *folio_list);
/* The ALLOC_WMARK bits are used as an index to zone->watermark */
#define ALLOC_WMARK_MIN WMARK_MIN
#define ALLOC_WMARK_LOW WMARK_LOW
diff --git a/mm/vmscan.c b/mm/vmscan.c
index eaa3dd6b7562..f03be320f9ad 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -910,7 +910,7 @@ static void folio_check_dirty_writeback(struct folio *folio,
mapping->a_ops->is_dirty_writeback(folio, dirty, writeback);
}

-static struct folio *alloc_demote_folio(struct folio *src,
+static struct folio *alloc_migrate_folio(struct folio *src,
unsigned long private)
{
struct folio *dst;
@@ -973,7 +973,7 @@ static unsigned int demote_folio_list(struct list_head *demote_folios,
node_get_allowed_targets(pgdat, &allowed_mask);

/* Demotion ignores all cpuset and mempolicy settings */
- migrate_pages(demote_folios, alloc_demote_folio, NULL,
+ migrate_pages(demote_folios, alloc_migrate_folio, NULL,
(unsigned long)&mtc, MIGRATE_ASYNC, MR_DEMOTION,
&nr_succeeded);

@@ -982,6 +982,48 @@ static unsigned int demote_folio_list(struct list_head *demote_folios,
return nr_succeeded;
}

+/*
+ * Take folios on @promote_folios and attempt to promote them to another node.
+ * Folios which are not promoted are left on @promote_folios.
+ */
+static unsigned int promote_folio_list(struct list_head *promote_folios,
+ struct pglist_data *pgdat)
+{
+ int target_nid = next_promotion_node(pgdat->node_id);
+ unsigned int nr_succeeded;
+ nodemask_t allowed_mask = NODE_MASK_NONE;
+
+ struct migration_target_control mtc = {
+ /*
+ * Allocate from 'node', or fail quickly and quietly.
+ * When this happens, 'page' will likely be stayed
+ * instead of migrated.
+ */
+ .gfp_mask = (GFP_HIGHUSER_MOVABLE & ~__GFP_RECLAIM) | __GFP_NOWARN |
+ __GFP_NOMEMALLOC | GFP_NOWAIT,
+ .nid = target_nid,
+ .nmask = &allowed_mask
+ };
+
+ if (pgdat->node_id == target_nid)
+ return 0;
+
+ if (list_empty(promote_folios))
+ return 0;
+
+ if (target_nid == NUMA_NO_NODE)
+ return 0;
+
+ /* Promotion ignores all cpuset and mempolicy settings */
+ migrate_pages(promote_folios, alloc_migrate_folio, NULL,
+ (unsigned long)&mtc, MIGRATE_ASYNC, MR_PROMOTION,
+ &nr_succeeded);
+
+ __count_vm_events(PGPROMOTE, nr_succeeded);
+
+ return nr_succeeded;
+}
+
static bool may_enter_fs(struct folio *folio, gfp_t gfp_mask)
{
if (gfp_mask & __GFP_FS)
@@ -1058,6 +1100,65 @@ static unsigned int __demote_folio_list(struct list_head *folio_list,
return nr_demoted;
}

+/*
+ * __promote_folio_list() returns the number of promoted pages
+ */
+static unsigned int __promote_folio_list(struct list_head *folio_list,
+ struct pglist_data *pgdat, struct scan_control *sc)
+{
+ LIST_HEAD(ret_folios);
+ LIST_HEAD(promote_folios);
+ unsigned int nr_promoted = 0;
+
+ cond_resched();
+
+ while (!list_empty(folio_list)) {
+ struct folio *folio;
+ enum folio_references references;
+
+ cond_resched();
+
+ folio = lru_to_folio(folio_list);
+ list_del(&folio->lru);
+
+ if (!folio_trylock(folio))
+ goto keep;
+
+ VM_BUG_ON_FOLIO(folio_test_active(folio), folio);
+
+ references = folio_check_references(folio, sc);
+ if (references == FOLIOREF_KEEP ||
+ references == FOLIOREF_RECLAIM ||
+ references == FOLIOREF_RECLAIM_CLEAN)
+ goto keep_locked;
+
+ /* Relocate its contents to another node. */
+ list_add(&folio->lru, &promote_folios);
+ folio_unlock(folio);
+ continue;
+keep_locked:
+ folio_unlock(folio);
+keep:
+ list_add(&folio->lru, &ret_folios);
+ VM_BUG_ON_FOLIO(folio_test_lru(folio), folio);
+ }
+ /* 'folio_list' is always empty here */
+
+ /* Migrate folios selected for promotion */
+ nr_promoted += promote_folio_list(&promote_folios, pgdat);
+ /* Folios that could not be promoted are still in @promote_folios */
+ if (!list_empty(&promote_folios)) {
+ /* Folios which weren't promoted go back on @folio_list */
+ list_splice_init(&promote_folios, folio_list);
+ }
+
+ try_to_unmap_flush();
+
+ list_splice(&ret_folios, folio_list);
+
+ return nr_promoted;
+}
+
/*
* shrink_folio_list() returns the number of reclaimed pages
*/
@@ -2186,6 +2287,25 @@ static unsigned int do_demote_folio_list(struct list_head *folio_list,
return nr_demoted;
}

+static unsigned int do_promote_folio_list(struct list_head *folio_list,
+ struct pglist_data *pgdat)
+{
+ unsigned int nr_promoted;
+ struct folio *folio;
+ struct scan_control sc = {
+ .gfp_mask = GFP_KERNEL,
+ };
+
+ nr_promoted = __promote_folio_list(folio_list, pgdat, &sc);
+ while (!list_empty(folio_list)) {
+ folio = lru_to_folio(folio_list);
+ list_del(&folio->lru);
+ folio_putback_lru(folio);
+ }
+
+ return nr_promoted;
+}
+
static unsigned long reclaim_or_migrate_folios(struct list_head *folio_list,
unsigned int (*handler)(struct list_head *, struct pglist_data *))
{
@@ -2230,6 +2350,11 @@ unsigned long demote_pages(struct list_head *folio_list)
return reclaim_or_migrate_folios(folio_list, do_demote_folio_list);
}

+unsigned long promote_pages(struct list_head *folio_list)
+{
+ return reclaim_or_migrate_folios(folio_list, do_promote_folio_list);
+}
+
static unsigned long shrink_list(enum lru_list lru, unsigned long nr_to_scan,
struct lruvec *lruvec, struct scan_control *sc)
{
diff --git a/mm/vmstat.c b/mm/vmstat.c
index 359460deb377..c703abdb8137 100644
--- a/mm/vmstat.c
+++ b/mm/vmstat.c
@@ -1282,6 +1282,7 @@ const char * const vmstat_text[] = {
"pgdemote_kswapd",
"pgdemote_direct",
"pgdemote_khugepaged",
+ "pgpromote",
"pgscan_kswapd",
"pgscan_direct",
"pgscan_khugepaged",
--
2.34.1

2024-01-16 22:13:17

by SeongJae Park

Subject: Re: [RFC PATCH 2/4] mm/damon: introduce DAMOS_DEMOTE action for demotion

On Mon, 15 Jan 2024 13:52:50 +0900 Honggyu Kim <[email protected]> wrote:

> This patch introduces DAMOS_DEMOTE action, which is similar to
> DAMOS_PAGEOUT, but demote folios instead of swapping them out.
>
> Since there are some common routines with pageout, many functions have
> similar logics between pageout and demote.
>
> The execution sequence of DAMOS_PAGEOUT and DAMOS_DEMOTE look as follows.
>
> DAMOS_PAGEOUT action
> damo_pa_apply_scheme

Nit. s/damo/damon/

> -> damon_pa_reclaim
> -> reclaim_pages
> -> reclaim_folio_list
> -> shrink_folio_list
>
> DAMOS_DEMOTE action
> damo_pa_apply_scheme

Ditto.

> -> damon_pa_reclaim
> -> demote_pages
> -> do_demote_folio_list
> -> __demote_folio_list
> -> demote_folio_list

I think implementation of 'demote_pages()' might better to be separated.

I'm also feeling the naming a bit strange, since I was usually thinking '__'
prefix is for functions that will internally used. That is, I'd assume
__demote_folio_list() is called from demote_folio_list(), but this function is
doing in an opposite way.

>
> __demote_folio_list() is a minimized version of shrink_folio_list(), but
> it's minified only for demotion.
>
> Signed-off-by: Honggyu Kim <[email protected]>
> ---
> include/linux/damon.h | 2 +
> mm/damon/paddr.c | 17 +++++---
> mm/damon/sysfs-schemes.c | 1 +
> mm/internal.h | 1 +
> mm/vmscan.c | 84 ++++++++++++++++++++++++++++++++++++++++
> 5 files changed, 99 insertions(+), 6 deletions(-)
>
> diff --git a/include/linux/damon.h b/include/linux/damon.h
> index e00ddf1ed39c..4c0a0fef09c5 100644
> --- a/include/linux/damon.h
> +++ b/include/linux/damon.h
> @@ -106,6 +106,7 @@ struct damon_target {
> * @DAMOS_LRU_PRIO: Prioritize the region on its LRU lists.
> * @DAMOS_LRU_DEPRIO: Deprioritize the region on its LRU lists.
> * @DAMOS_STAT: Do nothing but count the stat.
> + * @DAMOS_DEMOTE: Do demotion for the current region.

I'd prefer defining DEMOTE before STAT, like we introduced LRU_PRIO/DEPRIO
after STAT but defined there. It would help keeping the two different groups
of operations separated (STAT is different from other actions since it is not
for makeing real changes but only get statistics and monitoring results
querying).

> * @NR_DAMOS_ACTIONS: Total number of DAMOS actions
> *
> * The support of each action is up to running &struct damon_operations.
> @@ -123,6 +124,7 @@ enum damos_action {
> DAMOS_LRU_PRIO,
> DAMOS_LRU_DEPRIO,
> DAMOS_STAT, /* Do nothing but only record the stat */
> + DAMOS_DEMOTE,

Ditto.

> NR_DAMOS_ACTIONS,
> };
>
> diff --git a/mm/damon/paddr.c b/mm/damon/paddr.c
> index 081e2a325778..d3e3f077cd00 100644
> --- a/mm/damon/paddr.c
> +++ b/mm/damon/paddr.c
> @@ -224,7 +224,7 @@ static bool damos_pa_filter_out(struct damos *scheme, struct folio *folio)
> return false;
> }
>
> -static unsigned long damon_pa_pageout(struct damon_region *r, struct damos *s)
> +static unsigned long damon_pa_reclaim(struct damon_region *r, struct damos *s, bool is_demote)

I understand that reclamation could include both pageout and demotion, but not
sure if that is making its purpose clearer or more ambiguous. What about
renaming to '..._demote_or_pageout()', like
'damon_pa_mark_accessed_or_deactivate()'? Also, 'is_demote' could be simply
'demote'.

I think having a separate function, say, damon_pa_demote() is also ok, if it
makes code easier to read and not intorduce too much duplicated lines of code.

Also, I'd prefer keeping the 80 columns limit[1] by breaking this line.

[1] https://docs.kernel.org/process/coding-style.html?highlight=coding+style#breaking-long-lines-and-strings

> {
> unsigned long addr, applied;
> LIST_HEAD(folio_list);
> @@ -242,14 +242,17 @@ static unsigned long damon_pa_pageout(struct damon_region *r, struct damos *s)
> folio_test_clear_young(folio);
> if (!folio_isolate_lru(folio))
> goto put_folio;
> - if (folio_test_unevictable(folio))
> + if (folio_test_unevictable(folio) && !is_demote)
> folio_putback_lru(folio);
> else
> list_add(&folio->lru, &folio_list);
> put_folio:
> folio_put(folio);
> }
> - applied = reclaim_pages(&folio_list);
> + if (is_demote)
> + applied = demote_pages(&folio_list);
> + else
> + applied = reclaim_pages(&folio_list);
> cond_resched();
> return applied * PAGE_SIZE;
> }
> @@ -297,13 +300,15 @@ static unsigned long damon_pa_apply_scheme(struct damon_ctx *ctx,
> {
> switch (scheme->action) {
> case DAMOS_PAGEOUT:
> - return damon_pa_pageout(r, scheme);
> + return damon_pa_reclaim(r, scheme, false);
> case DAMOS_LRU_PRIO:
> return damon_pa_mark_accessed(r, scheme);
> case DAMOS_LRU_DEPRIO:
> return damon_pa_deactivate_pages(r, scheme);
> case DAMOS_STAT:
> break;
> + case DAMOS_DEMOTE:
> + return damon_pa_reclaim(r, scheme, true);

I'd like to keep the order of the branches aligned with that of 'enum
damos_action'.

> default:
> /* DAMOS actions that not yet supported by 'paddr'. */
> break;
> @@ -317,11 +322,11 @@ static int damon_pa_scheme_score(struct damon_ctx *context,
> {
> switch (scheme->action) {
> case DAMOS_PAGEOUT:
> + case DAMOS_LRU_DEPRIO:
> + case DAMOS_DEMOTE:
> return damon_cold_score(context, r, scheme);
> case DAMOS_LRU_PRIO:
> return damon_hot_score(context, r, scheme);
> - case DAMOS_LRU_DEPRIO:
> - return damon_cold_score(context, r, scheme);

I'd slightly prefer having yet another dedicated 'case' branch for
DAMOS_DEMOTE, to keep the order same to that of 'enum damos_action', and the
old style.

> default:
> break;
> }
> diff --git a/mm/damon/sysfs-schemes.c b/mm/damon/sysfs-schemes.c
> index fe0fe2562000..ac7cd3f17b12 100644
> --- a/mm/damon/sysfs-schemes.c
> +++ b/mm/damon/sysfs-schemes.c
> @@ -1187,6 +1187,7 @@ static const char * const damon_sysfs_damos_action_strs[] = {
> "lru_prio",
> "lru_deprio",
> "stat",
> + "demote",

If we define DEMOTE before STAT on enum damos_action, this would also need to
be updated.

> };
>
> static struct damon_sysfs_scheme *damon_sysfs_scheme_alloc(
> diff --git a/mm/internal.h b/mm/internal.h
> index b61034bd50f5..2380397ec2f3 100644
> --- a/mm/internal.h
> +++ b/mm/internal.h
> @@ -869,6 +869,7 @@ extern void set_pageblock_order(void);
> unsigned long reclaim_pages(struct list_head *folio_list);
> unsigned int reclaim_clean_pages_from_list(struct zone *zone,
> struct list_head *folio_list);
> +unsigned long demote_pages(struct list_head *folio_list);
> /* The ALLOC_WMARK bits are used as an index to zone->watermark */
> #define ALLOC_WMARK_MIN WMARK_MIN
> #define ALLOC_WMARK_LOW WMARK_LOW
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 7ca2396ccc3b..eaa3dd6b7562 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -998,6 +998,66 @@ static bool may_enter_fs(struct folio *folio, gfp_t gfp_mask)
> return !data_race(folio_swap_flags(folio) & SWP_FS_OPS);
> }
>
> +/*
> + * __demote_folio_list() returns the number of demoted pages
> + */
> +static unsigned int __demote_folio_list(struct list_head *folio_list,
> + struct pglist_data *pgdat, struct scan_control *sc)
> +{
> + LIST_HEAD(ret_folios);
> + LIST_HEAD(demote_folios);
> + unsigned int nr_demoted = 0;
> +
> + if (next_demotion_node(pgdat->node_id) == NUMA_NO_NODE)
> + return 0;
> +
> + cond_resched();
> +
> + while (!list_empty(folio_list)) {
> + struct folio *folio;
> + enum folio_references references;
> +
> + cond_resched();
> +
> + folio = lru_to_folio(folio_list);
> + list_del(&folio->lru);
> +
> + if (!folio_trylock(folio))
> + goto keep;
> +
> + VM_BUG_ON_FOLIO(folio_test_active(folio), folio);
> +
> + references = folio_check_references(folio, sc);
> + if (references == FOLIOREF_KEEP)
> + goto keep_locked;
> +
> + /* Relocate its contents to another node. */
> + list_add(&folio->lru, &demote_folios);
> + folio_unlock(folio);
> + continue;
> +keep_locked:
> + folio_unlock(folio);
> +keep:
> + list_add(&folio->lru, &ret_folios);
> + VM_BUG_ON_FOLIO(folio_test_lru(folio), folio);
> + }
> + /* 'folio_list' is always empty here */
> +
> + /* Migrate folios selected for demotion */
> + nr_demoted += demote_folio_list(&demote_folios, pgdat);

As mentioned above, I was assuming demote_folio_list() would call
__demote_folio_list(). I think the name of this function might better to be
changed.

> + /* Folios that could not be demoted are still in @demote_folios */
> + if (!list_empty(&demote_folios)) {
> + /* Folios which weren't demoted go back on @folio_list */
> + list_splice_init(&demote_folios, folio_list);
> + }
> +
> + try_to_unmap_flush();
> +
> + list_splice(&ret_folios, folio_list);
> +
> + return nr_demoted;
> +}
> +
> /*
> * shrink_folio_list() returns the number of reclaimed pages
> */
> @@ -2107,6 +2167,25 @@ static unsigned int reclaim_folio_list(struct list_head *folio_list,
> return nr_reclaimed;
> }
>
> +static unsigned int do_demote_folio_list(struct list_head *folio_list,
> + struct pglist_data *pgdat)
> +{
> + unsigned int nr_demoted;
> + struct folio *folio;
> + struct scan_control sc = {
> + .gfp_mask = GFP_KERNEL,
> + };
> +
> + nr_demoted = __demote_folio_list(folio_list, pgdat, &sc);
> + while (!list_empty(folio_list)) {
> + folio = lru_to_folio(folio_list);
> + list_del(&folio->lru);
> + folio_putback_lru(folio);
> + }
> +
> + return nr_demoted;
> +}
> +
> static unsigned long reclaim_or_migrate_folios(struct list_head *folio_list,
> unsigned int (*handler)(struct list_head *, struct pglist_data *))
> {
> @@ -2146,6 +2225,11 @@ unsigned long reclaim_pages(struct list_head *folio_list)
> return reclaim_or_migrate_folios(folio_list, reclaim_folio_list);
> }
>
> +unsigned long demote_pages(struct list_head *folio_list)
> +{
> + return reclaim_or_migrate_folios(folio_list, do_demote_folio_list);
> +}
> +

If DAMON is the only user of this function and we have no plan for adding more
user of this function, I think implementing this in mm/damon/ might make sense.

> static unsigned long shrink_list(enum lru_list lru, unsigned long nr_to_scan,
> struct lruvec *lruvec, struct scan_control *sc)
> {
> --
> 2.34.1

2024-01-16 22:13:31

by SeongJae Park

Subject: Re: [RFC PATCH 0/4] DAMON based 2-tier memory management for CXL memory

Hello,

On Mon, 15 Jan 2024 13:52:48 +0900 Honggyu Kim <[email protected]> wrote:

> There was an RFC IDEA "DAMOS-based Tiered-Memory Management" previously
> posted at [1].
>
> It says there is no implementation of the demote/promote DAMOS action
> are made. This RFC is about its implementation for physical address
> space.
>
[...]
> Evaluation Results
> ==================
>
[...]
> In summary of both results, our evaluation shows that "DAMON 2-tier"
> memory management reduces the performance slowdown compared to the
> "default" memory policy from 15~17% to 4~5% when the system runs with
> high memory pressure on its fast tier DRAM nodes.
>
> The similar evaluation was done in another machine that has 256GB of
> local DRAM and 96GB of CXL memory. The performance slowdown is reduced
> from 20~24% for "default" to 5~7% for "DAMON 2-tier".
>
> Having these DAMOS_DEMOTE and DAMOS_PROMOTE actions can make 2-tier
> memory systems run more efficiently under high memory pressures.


Thank you so much for this great patches and the above nice test results. I
believe the test setup and results make sense, and merging a revised version of
this patchset would provide real benefits to the users.

In a high level, I think it might better to separate DAMON internal changes
from DAMON external changes.

For DAMON part changes, I have no big concern other than trivial coding style
level comments.

For DAMON-external changes that implementing demote_pages() and
promote_pages(), I'm unsure if the implementation is reusing appropriate
functions, and if those are placee in right source file. Especially, I'm
unsure if vmscan.c is the right place for promotion code. Also I don't know if
there is a good agreement on the promotion/demotion target node decision. That
should be because I'm not that familiar with the areas and the files, but I
feel this might because our discussions on the promotion and the demotion
operations are having rooms for being more matured. Because I'm not very
faimiliar with the part, I'd like to hear others' comments, too.

To this end, I feel the problem might be able to be simpler, because this
patchset is trying to provide two sophisticated operations, while I think a
simpler approach might be possible. My humble simpler idea is adding a DAMOS
operation for moving pages to a given node (like sys_move_phy_pages RFC[1]),
instead of the promote/demote. Because the general pages migration can handle
multiple cases including the promote/demote in my humble assumption. In more
detail, users could decide which is the appropriate node for promotion or
demotion and use the new DAMOS action to do promotion and demotion. Users
would requested to decide which node is the proper promotion/demotion target
nodes, but that decision wouldn't be that hard in my opinion.

For this, 'struct damos' would need to be updated for such argument-dependent
actions, like 'struct damos_filter' is haing a union.

In future, we could extend the operation to the promotion and the demotion
after the dicussion around the promotion and demotion is matured, if required.
And assuming DAMON be extended for originating CPU-aware access monitoring, the
new DAMOS action would also cover more use cases such as general NUMA nodes
balancing (extending DAMON for CPU-aware monitoring would required), and some
complex configurations where having both CPU affinity and tiered memory. I
also think that may well fit with my RFC idea[2] for tiered memory management.

Looking forward to opinions from you and others. I admig I miss many things,
and more than happy to be enlightened.

[1] https://lwn.net/Articles/944007/
[2] https://lore.kernel.org/damon/[email protected]/


Thanks,
SJ

>
> Signed-off-by: Honggyu Kim <[email protected]>
> Signed-off-by: Hyeongtak Ji <[email protected]>
> Signed-off-by: Rakie Kim <[email protected]>
>
> [1] https://lore.kernel.org/damon/[email protected]
> [2] https://github.com/skhynix/hmsdk
> [3] https://github.com/redis/redis/tree/7.0.0
> [4] https://github.com/brianfrankcooper/YCSB/tree/0.17.0
> [5] https://dl.acm.org/doi/10.1145/3503222.3507731
> [6] https://dl.acm.org/doi/10.1145/3582016.3582063
>
> Honggyu Kim (2):
> mm/vmscan: refactor reclaim_pages with reclaim_or_migrate_folios
> mm/damon: introduce DAMOS_DEMOTE action for demotion
>
> Hyeongtak Ji (2):
> mm/memory-tiers: add next_promotion_node to find promotion target
> mm/damon: introduce DAMOS_PROMOTE action for promotion
>
> include/linux/damon.h | 4 +
> include/linux/memory-tiers.h | 11 ++
> include/linux/migrate_mode.h | 1 +
> include/linux/vm_event_item.h | 1 +
> include/trace/events/migrate.h | 3 +-
> mm/damon/paddr.c | 46 ++++++-
> mm/damon/sysfs-schemes.c | 2 +
> mm/internal.h | 2 +
> mm/memory-tiers.c | 43 ++++++
> mm/vmscan.c | 231 +++++++++++++++++++++++++++++++--
> mm/vmstat.c | 1 +
> 11 files changed, 330 insertions(+), 15 deletions(-)
>
>
> base-commit: 0dd3ee31125508cd67f7e7172247f05b7fd1753a
> --
> 2.34.1

2024-01-16 22:13:43

by SeongJae Park

Subject: Re: [RFC PATCH 4/4] mm/damon: introduce DAMOS_PROMOTE action for promotion

On Mon, 15 Jan 2024 13:52:52 +0900 Honggyu Kim <[email protected]> wrote:

> From: Hyeongtak Ji <[email protected]>
>
> This patch introduces DAMOS_PROMOTE action for paddr mode.
>
> It includes renaming alloc_demote_folio to alloc_migrate_folio to use it
> for promotion as well.
>
> The execution sequence of DAMOS_DEMOTE and DAMOS_PROMOTE look as
> follows for comparison.
>
> DAMOS_DEMOTE action
> damo_pa_apply_scheme
> -> damon_pa_reclaim
> -> demote_pages
> -> do_demote_folio_list
> -> __demote_folio_list
> -> demote_folio_list
>
> DAMOS_PROMOTE action
> damo_pa_apply_scheme
> -> damon_pa_promote
> -> promote_pages
> -> do_promote_folio_list
> -> __promote_folio_list
> -> promote_folio_list
>
> Signed-off-by: Hyeongtak Ji <[email protected]>
> Signed-off-by: Honggyu Kim <[email protected]>
> ---
> include/linux/damon.h | 2 +
> include/linux/migrate_mode.h | 1 +
> include/linux/vm_event_item.h | 1 +
> include/trace/events/migrate.h | 3 +-
> mm/damon/paddr.c | 29 ++++++++
> mm/damon/sysfs-schemes.c | 1 +
> mm/internal.h | 1 +
> mm/vmscan.c | 129 ++++++++++++++++++++++++++++++++-
> mm/vmstat.c | 1 +
> 9 files changed, 165 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/damon.h b/include/linux/damon.h
> index 4c0a0fef09c5..477060bb6718 100644
> --- a/include/linux/damon.h
> +++ b/include/linux/damon.h
> @@ -107,6 +107,7 @@ struct damon_target {
> * @DAMOS_LRU_DEPRIO: Deprioritize the region on its LRU lists.
> * @DAMOS_STAT: Do nothing but count the stat.
> * @DAMOS_DEMOTE: Do demotion for the current region.
> + * @DAMOS_PROMOTE: Do promotion if possible, otherwise do nothing.

Like LRU_PRIO is defined before LRU_DEPRIO, what about defining PROMOTE before
DEMOTE?

> * @NR_DAMOS_ACTIONS: Total number of DAMOS actions
> *
> * The support of each action is up to running &struct damon_operations.
> @@ -125,6 +126,7 @@ enum damos_action {
> DAMOS_LRU_DEPRIO,
> DAMOS_STAT, /* Do nothing but only record the stat */
> DAMOS_DEMOTE,
> + DAMOS_PROMOTE,
> NR_DAMOS_ACTIONS,
> };
>
> diff --git a/include/linux/migrate_mode.h b/include/linux/migrate_mode.h
> index f37cc03f9369..63f75eb9abf3 100644
> --- a/include/linux/migrate_mode.h
> +++ b/include/linux/migrate_mode.h
> @@ -29,6 +29,7 @@ enum migrate_reason {
> MR_CONTIG_RANGE,
> MR_LONGTERM_PIN,
> MR_DEMOTION,
> + MR_PROMOTION,
> MR_TYPES
> };
>
> diff --git a/include/linux/vm_event_item.h b/include/linux/vm_event_item.h
> index 8abfa1240040..63cf920afeaa 100644
> --- a/include/linux/vm_event_item.h
> +++ b/include/linux/vm_event_item.h
> @@ -44,6 +44,7 @@ enum vm_event_item { PGPGIN, PGPGOUT, PSWPIN, PSWPOUT,
> PGDEMOTE_KSWAPD,
> PGDEMOTE_DIRECT,
> PGDEMOTE_KHUGEPAGED,
> + PGPROMOTE,
> PGSCAN_KSWAPD,
> PGSCAN_DIRECT,
> PGSCAN_KHUGEPAGED,
> diff --git a/include/trace/events/migrate.h b/include/trace/events/migrate.h
> index 0190ef725b43..f0dd569c1e62 100644
> --- a/include/trace/events/migrate.h
> +++ b/include/trace/events/migrate.h
> @@ -22,7 +22,8 @@
> EM( MR_NUMA_MISPLACED, "numa_misplaced") \
> EM( MR_CONTIG_RANGE, "contig_range") \
> EM( MR_LONGTERM_PIN, "longterm_pin") \
> - EMe(MR_DEMOTION, "demotion")
> + EM( MR_DEMOTION, "demotion") \
> + EMe(MR_PROMOTION, "promotion")
>
> /*
> * First define the enums in the above macros to be exported to userspace
> diff --git a/mm/damon/paddr.c b/mm/damon/paddr.c
> index d3e3f077cd00..360ce69d5898 100644
> --- a/mm/damon/paddr.c
> +++ b/mm/damon/paddr.c
> @@ -257,6 +257,32 @@ static unsigned long damon_pa_reclaim(struct damon_region *r, struct damos *s, b
> return applied * PAGE_SIZE;
> }
>
> +static unsigned long damon_pa_promote(struct damon_region *r, struct damos *s)
> +{
> + unsigned long addr, applied;
> + LIST_HEAD(folio_list);
> +
> + for (addr = r->ar.start; addr < r->ar.end; addr += PAGE_SIZE) {
> + struct folio *folio = damon_get_folio(PHYS_PFN(addr));
> +
> + if (!folio)
> + continue;
> +
> + if (damos_pa_filter_out(s, folio))
> + goto put_folio;
> +
> + if (!folio_isolate_lru(folio))
> + goto put_folio;
> +
> + list_add(&folio->lru, &folio_list);
> +put_folio:
> + folio_put(folio);
> + }
> + applied = promote_pages(&folio_list);
> + cond_resched();
> + return applied * PAGE_SIZE;
> +}
> +
> static inline unsigned long damon_pa_mark_accessed_or_deactivate(
> struct damon_region *r, struct damos *s, bool mark_accessed)
> {
> @@ -309,6 +335,8 @@ static unsigned long damon_pa_apply_scheme(struct damon_ctx *ctx,
> break;
> case DAMOS_DEMOTE:
> return damon_pa_reclaim(r, scheme, true);
> + case DAMOS_PROMOTE:
> + return damon_pa_promote(r, scheme);
> default:
> /* DAMOS actions that not yet supported by 'paddr'. */
> break;
> @@ -326,6 +354,7 @@ static int damon_pa_scheme_score(struct damon_ctx *context,
> case DAMOS_DEMOTE:
> return damon_cold_score(context, r, scheme);
> case DAMOS_LRU_PRIO:
> + case DAMOS_PROMOTE:
> return damon_hot_score(context, r, scheme);

As mentioned on the previous patch, I'd prefer keeping the order of operations
and having dedicated branches for each operation.

> default:
> break;
> diff --git a/mm/damon/sysfs-schemes.c b/mm/damon/sysfs-schemes.c
> index ac7cd3f17b12..1b84d0af7e1f 100644
> --- a/mm/damon/sysfs-schemes.c
> +++ b/mm/damon/sysfs-schemes.c
> @@ -1188,6 +1188,7 @@ static const char * const damon_sysfs_damos_action_strs[] = {
> "lru_deprio",
> "stat",
> "demote",
> + "promote",
> };
>
> static struct damon_sysfs_scheme *damon_sysfs_scheme_alloc(
> diff --git a/mm/internal.h b/mm/internal.h
> index 2380397ec2f3..f159455e63d4 100644
> --- a/mm/internal.h
> +++ b/mm/internal.h
> @@ -870,6 +870,7 @@ unsigned long reclaim_pages(struct list_head *folio_list);
> unsigned int reclaim_clean_pages_from_list(struct zone *zone,
> struct list_head *folio_list);
> unsigned long demote_pages(struct list_head *folio_list);
> +unsigned long promote_pages(struct list_head *folio_list);
> /* The ALLOC_WMARK bits are used as an index to zone->watermark */
> #define ALLOC_WMARK_MIN WMARK_MIN
> #define ALLOC_WMARK_LOW WMARK_LOW
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index eaa3dd6b7562..f03be320f9ad 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -910,7 +910,7 @@ static void folio_check_dirty_writeback(struct folio *folio,
> mapping->a_ops->is_dirty_writeback(folio, dirty, writeback);
> }
>
> -static struct folio *alloc_demote_folio(struct folio *src,
> +static struct folio *alloc_migrate_folio(struct folio *src,
> unsigned long private)

As also mentioned on the previous patch, I'm unsure if vmscan.c is the right
place for general migration.

> {
> struct folio *dst;
> @@ -973,7 +973,7 @@ static unsigned int demote_folio_list(struct list_head *demote_folios,
> node_get_allowed_targets(pgdat, &allowed_mask);
>
> /* Demotion ignores all cpuset and mempolicy settings */
> - migrate_pages(demote_folios, alloc_demote_folio, NULL,
> + migrate_pages(demote_folios, alloc_migrate_folio, NULL,
> (unsigned long)&mtc, MIGRATE_ASYNC, MR_DEMOTION,
> &nr_succeeded);
>
> @@ -982,6 +982,48 @@ static unsigned int demote_folio_list(struct list_head *demote_folios,
> return nr_succeeded;
> }
>
> +/*
> + * Take folios on @promote_folios and attempt to promote them to another node.
> + * Folios which are not promoted are left on @promote_folios.
> + */
> +static unsigned int promote_folio_list(struct list_head *promote_folios,
> + struct pglist_data *pgdat)
> +{
> + int target_nid = next_promotion_node(pgdat->node_id);
> + unsigned int nr_succeeded;
> + nodemask_t allowed_mask = NODE_MASK_NONE;
> +
> + struct migration_target_control mtc = {
> + /*
> + * Allocate from 'node', or fail quickly and quietly.
> + * When this happens, 'page' will likely be stayed
> + * instead of migrated.
> + */
> + .gfp_mask = (GFP_HIGHUSER_MOVABLE & ~__GFP_RECLAIM) | __GFP_NOWARN |
> + __GFP_NOMEMALLOC | GFP_NOWAIT,
> + .nid = target_nid,
> + .nmask = &allowed_mask
> + };
> +
> + if (pgdat->node_id == target_nid)
> + return 0;
> +
> + if (list_empty(promote_folios))
> + return 0;
> +
> + if (target_nid == NUMA_NO_NODE)
> + return 0;
> +
> + /* Promotion ignores all cpuset and mempolicy settings */
> + migrate_pages(promote_folios, alloc_migrate_folio, NULL,
> + (unsigned long)&mtc, MIGRATE_ASYNC, MR_PROMOTION,
> + &nr_succeeded);
> +
> + __count_vm_events(PGPROMOTE, nr_succeeded);
> +
> + return nr_succeeded;
> +}
> +
> static bool may_enter_fs(struct folio *folio, gfp_t gfp_mask)
> {
> if (gfp_mask & __GFP_FS)
> @@ -1058,6 +1100,65 @@ static unsigned int __demote_folio_list(struct list_head *folio_list,
> return nr_demoted;
> }
>
> +/*
> + * __promote_folio_list() returns the number of promoted pages
> + */
> +static unsigned int __promote_folio_list(struct list_head *folio_list,
> + struct pglist_data *pgdat, struct scan_control *sc)
> +{
> + LIST_HEAD(ret_folios);
> + LIST_HEAD(promote_folios);
> + unsigned int nr_promoted = 0;
> +
> + cond_resched();
> +
> + while (!list_empty(folio_list)) {
> + struct folio *folio;
> + enum folio_references references;
> +
> + cond_resched();
> +
> + folio = lru_to_folio(folio_list);
> + list_del(&folio->lru);
> +
> + if (!folio_trylock(folio))
> + goto keep;
> +
> + VM_BUG_ON_FOLIO(folio_test_active(folio), folio);
> +
> + references = folio_check_references(folio, sc);
> + if (references == FOLIOREF_KEEP ||
> + references == FOLIOREF_RECLAIM ||
> + references == FOLIOREF_RECLAIM_CLEAN)
> + goto keep_locked;
> +
> + /* Relocate its contents to another node. */
> + list_add(&folio->lru, &promote_folios);
> + folio_unlock(folio);
> + continue;
> +keep_locked:
> + folio_unlock(folio);
> +keep:
> + list_add(&folio->lru, &ret_folios);
> + VM_BUG_ON_FOLIO(folio_test_lru(folio), folio);
> + }
> + /* 'folio_list' is always empty here */
> +
> + /* Migrate folios selected for promotion */
> + nr_promoted += promote_folio_list(&promote_folios, pgdat);

Again as mentioned on the previous patch, I was thinking promote_folios_list()
may call __promote_folio_list(). Making the __ prefix usage consistent with
other functions might be better, in my opinion.

> + /* Folios that could not be promoted are still in @promote_folios */
> + if (!list_empty(&promote_folios)) {
> + /* Folios which weren't promoted go back on @folio_list */
> + list_splice_init(&promote_folios, folio_list);
> + }
> +
> + try_to_unmap_flush();
> +
> + list_splice(&ret_folios, folio_list);
> +
> + return nr_promoted;
> +}
> +
> /*
> * shrink_folio_list() returns the number of reclaimed pages
> */
> @@ -2186,6 +2287,25 @@ static unsigned int do_demote_folio_list(struct list_head *folio_list,
> return nr_demoted;
> }
>
> +static unsigned int do_promote_folio_list(struct list_head *folio_list,
> + struct pglist_data *pgdat)
> +{
> + unsigned int nr_promoted;
> + struct folio *folio;
> + struct scan_control sc = {
> + .gfp_mask = GFP_KERNEL,
> + };
> +
> + nr_promoted = __promote_folio_list(folio_list, pgdat, &sc);
> + while (!list_empty(folio_list)) {
> + folio = lru_to_folio(folio_list);
> + list_del(&folio->lru);
> + folio_putback_lru(folio);
> + }
> +
> + return nr_promoted;
> +}
> +
> static unsigned long reclaim_or_migrate_folios(struct list_head *folio_list,
> unsigned int (*handler)(struct list_head *, struct pglist_data *))
> {
> @@ -2230,6 +2350,11 @@ unsigned long demote_pages(struct list_head *folio_list)
> return reclaim_or_migrate_folios(folio_list, do_demote_folio_list);
> }
>
> +unsigned long promote_pages(struct list_head *folio_list)
> +{
> + return reclaim_or_migrate_folios(folio_list, do_promote_folio_list);
> +}
> +
> static unsigned long shrink_list(enum lru_list lru, unsigned long nr_to_scan,
> struct lruvec *lruvec, struct scan_control *sc)
> {
> diff --git a/mm/vmstat.c b/mm/vmstat.c
> index 359460deb377..c703abdb8137 100644
> --- a/mm/vmstat.c
> +++ b/mm/vmstat.c
> @@ -1282,6 +1282,7 @@ const char * const vmstat_text[] = {
> "pgdemote_kswapd",
> "pgdemote_direct",
> "pgdemote_khugepaged",
> + "pgpromote",
> "pgscan_kswapd",
> "pgscan_direct",
> "pgscan_khugepaged",
> --
> 2.34.1

2024-01-16 22:37:56

by SeongJae Park

Subject: Re: [RFC PATCH 3/4] mm/memory-tiers: add next_promotion_node to find promotion target

On Mon, 15 Jan 2024 13:52:51 +0900 Honggyu Kim <[email protected]> wrote:

> From: Hyeongtak Ji <[email protected]>
>
> This patch adds next_promotion_node that can be used to identify the
> appropriate promotion target based on memory tiers. When multiple
> promotion target nodes are available, the nearest node is selected based
> on numa distance.
>
> Signed-off-by: Hyeongtak Ji <[email protected]>
> ---
> include/linux/memory-tiers.h | 11 +++++++++
> mm/memory-tiers.c | 43 ++++++++++++++++++++++++++++++++++++
> 2 files changed, 54 insertions(+)
>
> diff --git a/include/linux/memory-tiers.h b/include/linux/memory-tiers.h
> index 1e39d27bee41..0788e435fc50 100644
> --- a/include/linux/memory-tiers.h
> +++ b/include/linux/memory-tiers.h
> @@ -50,6 +50,7 @@ int mt_set_default_dram_perf(int nid, struct node_hmem_attrs *perf,
> int mt_perf_to_adistance(struct node_hmem_attrs *perf, int *adist);
> #ifdef CONFIG_MIGRATION
> int next_demotion_node(int node);
> +int next_promotion_node(int node);
> void node_get_allowed_targets(pg_data_t *pgdat, nodemask_t *targets);
> bool node_is_toptier(int node);
> #else
> @@ -58,6 +59,11 @@ static inline int next_demotion_node(int node)
> return NUMA_NO_NODE;
> }
>
> +static inline int next_promotion_node(int node)
> +{
> + return NUMA_NO_NODE;
> +}
> +
> static inline void node_get_allowed_targets(pg_data_t *pgdat, nodemask_t *targets)
> {
> *targets = NODE_MASK_NONE;
> @@ -101,6 +107,11 @@ static inline int next_demotion_node(int node)
> return NUMA_NO_NODE;
> }
>
> +static inline int next_promotion_node(int node)
> +{
> + return NUMA_NO_NODE;
> +}
> +
> static inline void node_get_allowed_targets(pg_data_t *pgdat, nodemask_t *targets)
> {
> *targets = NODE_MASK_NONE;
> diff --git a/mm/memory-tiers.c b/mm/memory-tiers.c
> index 8d5291add2bc..0060ee571cf4 100644
> --- a/mm/memory-tiers.c
> +++ b/mm/memory-tiers.c
> @@ -335,6 +335,49 @@ int next_demotion_node(int node)
> return target;
> }
>
> +/*
> + * Select a promotion target that is close to the from node among the given
> + * two nodes.
> + *
> + * TODO: consider other decision policy as node_distance may not be precise.
> + */
> +static int select_promotion_target(int a, int b, int from)
> +{
> + if (node_distance(from, a) < node_distance(from, b))
> + return a;
> + else
> + return b;
> +}
> +
> +/**
> + * next_promotion_node() - Get the next node in the promotion path
> + * @node: The starting node to lookup the next node
> + *
> + * Return: node id for next memory node in the promotion path hierarchy
> + * from @node; NUMA_NO_NODE if @node is the toptier.
> + */
> +int next_promotion_node(int node)
> +{
> + int target = NUMA_NO_NODE;
> + int nid;
> +
> + if (node_is_toptier(node))
> + return NUMA_NO_NODE;
> +
> + rcu_read_lock();
> + for_each_node_state(nid, N_MEMORY) {
> + if (node_isset(node, node_demotion[nid].preferred)) {
> + if (target == NUMA_NO_NODE)
> + target = nid;
> + else
> + target = select_promotion_target(nid, target, node);
> + }
> + }
> + rcu_read_unlock();
> +
> + return target;
> +}
> +

If this is gonna used by only DAMON and we don't have a concrete plan to making
this used by others, I think implementing this in mm/damon/ might make sense.

> static void disable_all_demotion_targets(void)
> {
> struct memory_tier *memtier;
> --
> 2.34.1

2024-01-17 11:50:36

by Honggyu Kim

Subject: Re: [RFC PATCH 0/4] DAMON based 2-tier memory management for CXL memory

Hi SeongJae,

Thanks very much for your comments in details.

On Tue, 16 Jan 2024 12:31:59 -0800 SeongJae Park <[email protected]> wrote:

> Thank you so much for this great patches and the above nice test results. I
> believe the test setup and results make sense, and merging a revised version of
> this patchset would provide real benefits to the users.

Glad to hear that!

> In a high level, I think it might better to separate DAMON internal changes
> from DAMON external changes.

I agree. I can't guarantee but I can move all the external changes
inside mm/damon, but will try that as much as possible.

> For DAMON part changes, I have no big concern other than trivial coding style
> level comments.

Sure. I will fix those.

> For DAMON-external changes that implementing demote_pages() and
> promote_pages(), I'm unsure if the implementation is reusing appropriate
> functions, and if those are placee in right source file. Especially, I'm
> unsure if vmscan.c is the right place for promotion code. Also I don't know if
> there is a good agreement on the promotion/demotion target node decision. That
> should be because I'm not that familiar with the areas and the files, but I
> feel this might because our discussions on the promotion and the demotion
> operations are having rooms for being more matured. Because I'm not very
> faimiliar with the part, I'd like to hear others' comments, too.

I would also like to hear others' comments, but this might not be needed
if most of external code can be moved to mm/damon.

> To this end, I feel the problem might be able te be simpler, because this
> patchset is trying to provide two sophisticated operations, while I think a
> simpler approach might be possible. My humble simpler idea is adding a DAMOS
> operation for moving pages to a given node (like sys_move_phy_pages RFC[1]),
> instead of the promote/demote. Because the general pages migration can handle
> multiple cases including the promote/demote in my humble assumption.

My initial implementation was similar but I found that it's not accurate
enough due to the nature of inaccuracy of DAMON regions. I saw that
many pages were demoted and promoted back and forth because migration
target regions include both hot and cold pages together.

So I have implemented the demotion and promotion logics based on the
shrink_folio_list, which contains many corner case handling logics for
reclaim.

Having the current demotion and promotion logics makes the hot/cold
migration pretty accurate as expected. We made a simple program called
"hot_cold" and it receives 2 arguments for hot size and cold size in MB.
For example, "hot_cold 200 500" allocates 200MB of hot memory and 500MB
of cold memory. It basically allocates 2 large blocks of memory with
mmap, then repeat memset for the initial 200MB to make it accessed in an
infinite loop.

Let's say there are 3 nodes in the system and the first node0 and node1
are the first tier, and node2 is the second tier.

$ cat /sys/devices/virtual/memory_tiering/memory_tier4/nodelist
0-1

$ cat /sys/devices/virtual/memory_tiering/memory_tier22/nodelist
2

Here is the result of partitioning hot/cold memory and I put execution
command at the right side of numastat result. I initially ran each
hot_cold program with preferred setting so that they initially allocate
memory on one of node0 or node2, but they gradually migrated based on
their access frequencies.

$ numastat -c -p hot_cold
Per-node process memory usage (in MBs)
PID Node 0 Node 1 Node 2 Total
--------------- ------ ------ ------ -----
754 (hot_cold) 1800 0 2000 3800 <- hot_cold 1800 2000
1184 (hot_cold) 300 0 500 800 <- hot_cold 300 500
1818 (hot_cold) 801 0 3199 4000 <- hot_cold 800 3200
30289 (hot_cold) 4 0 5 10 <- hot_cold 3 5
30325 (hot_cold) 31 0 51 81 <- hot_cold 30 50
--------------- ------ ------ ------ -----
Total 2938 0 5756 8695

The final node placement result shows that DAMON accurately migrated
pages by their hotness for multiple processes.

> In more detail, users could decide which is the appropriate node for promotion
> or demotion and use the new DAMOS action to do promotion and demotion. Users
> would requested to decide which node is the proper promotion/demotion target
> nodes, but that decision wouldn't be that hard in my opinion.
>
> For this, 'struct damos' would need to be updated for such argument-dependent
> actions, like 'struct damos_filter' is haing a union.

That might be a better solution. I will think about it.

> In future, we could extend the operation to the promotion and the demotion
> after the dicussion around the promotion and demotion is matured, if required.
> And assuming DAMON be extended for originating CPU-aware access monitoring, the
> new DAMOS action would also cover more use cases such as general NUMA nodes
> balancing (extending DAMON for CPU-aware monitoring would required), and some
> complex configurations where having both CPU affinity and tiered memory. I
> also think that may well fit with my RFC idea[2] for tiered memory management.
>
> Looking forward to opinions from you and others. I admig I miss many things,
> and more than happy to be enlightened.
>
> [1] https://lwn.net/Articles/944007/
> [2] https://lore.kernel.org/damon/[email protected]/

Thanks very much for your comments. I will need a few more days for the
update but will try to address your concerns as much as possible.

Thanks,
Honggyu

2024-01-17 21:11:13

by SeongJae Park

Subject: Re: [RFC PATCH 0/4] DAMON based 2-tier memory management for CXL memory

Hi Honggyu,

On Wed, 17 Jan 2024 20:49:25 +0900 Honggyu Kim <[email protected]> wrote:

> Hi SeongJae,
>
> Thanks very much for your comments in details.
>
> On Tue, 16 Jan 2024 12:31:59 -0800 SeongJae Park <[email protected]> wrote:
>
> > Thank you so much for this great patches and the above nice test results. I
> > believe the test setup and results make sense, and merging a revised version of
> > this patchset would provide real benefits to the users.
>
> Glad to hear that!
>
> > In a high level, I think it might better to separate DAMON internal changes
> > from DAMON external changes.
>
> I agree. I can't guarantee but I can move all the external changes
> inside mm/damon, but will try that as much as possible.
>
> > For DAMON part changes, I have no big concern other than trivial coding style
> > level comments.
>
> Sure. I will fix those.
>
> > For DAMON-external changes that implementing demote_pages() and
> > promote_pages(), I'm unsure if the implementation is reusing appropriate
> > functions, and if those are placee in right source file. Especially, I'm
> > unsure if vmscan.c is the right place for promotion code. Also I don't know if
> > there is a good agreement on the promotion/demotion target node decision. That
> > should be because I'm not that familiar with the areas and the files, but I
> > feel this might because our discussions on the promotion and the demotion
> > operations are having rooms for being more matured. Because I'm not very
> > faimiliar with the part, I'd like to hear others' comments, too.
>
> I would also like to hear others' comments, but this might not be needed
> if most of external code can be moved to mm/damon.
>
> > To this end, I feel the problem might be able te be simpler, because this
> > patchset is trying to provide two sophisticated operations, while I think a
> > simpler approach might be possible. My humble simpler idea is adding a DAMOS
> > operation for moving pages to a given node (like sys_move_phy_pages RFC[1]),
> > instead of the promote/demote. Because the general pages migration can handle
> > multiple cases including the promote/demote in my humble assumption.
>
> My initial implementation was similar but I found that it's not accurate
> enough due to the nature of inaccuracy of DAMON regions. I saw that
> many pages were demoted and promoted back and forth because migration
> target regions include both hot and cold pages together.
>
> So I have implemented the demotion and promotion logics based on the
> shrink_folio_list, which contains many corner case handling logics for
> reclaim.
>
> Having the current demotion and promotion logics makes the hot/cold
> migration pretty accurate as expected. We made a simple program called
> "hot_cold" and it receives 2 arguments for hot size and cold size in MB.
> For example, "hot_cold 200 500" allocates 200MB of hot memory and 500MB
> of cold memory. It basically allocates 2 large blocks of memory with
> mmap, then repeat memset for the initial 200MB to make it accessed in an
> infinite loop.
>
> Let's say there are 3 nodes in the system and the first node0 and node1
> are the first tier, and node2 is the second tier.
>
> $ cat /sys/devices/virtual/memory_tiering/memory_tier4/nodelist
> 0-1
>
> $ cat /sys/devices/virtual/memory_tiering/memory_tier22/nodelist
> 2
>
> Here is the result of partitioning hot/cold memory and I put execution
> command at the right side of numastat result. I initially ran each
> hot_cold program with preferred setting so that they initially allocate
> memory on one of node0 or node2, but they gradually migrated based on
> their access frequencies.
>
> $ numastat -c -p hot_cold
> Per-node process memory usage (in MBs)
> PID Node 0 Node 1 Node 2 Total
> --------------- ------ ------ ------ -----
> 754 (hot_cold) 1800 0 2000 3800 <- hot_cold 1800 2000
> 1184 (hot_cold) 300 0 500 800 <- hot_cold 300 500
> 1818 (hot_cold) 801 0 3199 4000 <- hot_cold 800 3200
> 30289 (hot_cold) 4 0 5 10 <- hot_cold 3 5
> 30325 (hot_cold) 31 0 51 81 <- hot_cold 30 50
> --------------- ------ ------ ------ -----
> Total 2938 0 5756 8695
>
> The final node placement result shows that DAMON accurately migrated
> pages by their hotness for multiple processes.

What was the result when the corner cases handling logics were not applied?

And, what are the corner cases handling logic that seemed essential? I show
the page granularity active/reference check could indeed provide many
improvements, but that's only my humble assumption.

If the corner cases are indeed better to be applied in page granularity, I
agree we need some more efforts since DAMON monitoring results are not page
granularity aware by the design. Users could increase min_nr_regions to make
it more accurate, and we have plan to support page granularity monitoring,
though. But maybe the overhead could be unacceptable.

Ideal solution would be making DAMON more accurate while keeping current level
of overhead. We indeed have TODO items for DAMON accuracy improvement, but
this may take some time that might unacceptable for your case.

If that's the case, I think the additional corner handling (or, page gran
additional access check) could be made as DAMOS filters[1], since DAMOS filters
can be applied in page granularity, and designed for this kind of handling of
information that DAMON monitoring results cannot provide. More specifically,
we could have filters for promotion-qualifying pages and demotion-qualifying
pages. In this way, I think we can keep the action more flexible while the
filters can be applied in creative ways.

[1] https://git.kernel.org/sj/c/98def236f63c66629fb6b2d4b69cecffc5b46539

>
> > In more detail, users could decide which is the appropriate node for promotion
> > or demotion and use the new DAMOS action to do promotion and demotion. Users
> > would requested to decide which node is the proper promotion/demotion target
> > nodes, but that decision wouldn't be that hard in my opinion.
> >
> > For this, 'struct damos' would need to be updated for such argument-dependent
> > actions, like 'struct damos_filter' is haing a union.
>
> That might be a better solution. I will think about it.

More specifically, I think receiving an address range as the argument might
more flexible than just NUMA node. Maybe we can imagine proactively migrating
cold movable pages from normal zones to movable zones, to avoid normal zone
memory pressure.

>
> > In future, we could extend the operation to the promotion and the demotion
> > after the dicussion around the promotion and demotion is matured, if required.
> > And assuming DAMON be extended for originating CPU-aware access monitoring, the
> > new DAMOS action would also cover more use cases such as general NUMA nodes
> > balancing (extending DAMON for CPU-aware monitoring would required), and some
> > complex configurations where having both CPU affinity and tiered memory. I
> > also think that may well fit with my RFC idea[2] for tiered memory management.
> >
> > Looking forward to opinions from you and others. I admig I miss many things,
> > and more than happy to be enlightened.
> >
> > [1] https://lwn.net/Articles/944007/
> > [2] https://lore.kernel.org/damon/[email protected]/
>
> Thanks very much for your comments. I will need a few more days for the
> update but will try to address your concerns as much as possible.

No problem, please take your time. I'm looking forward to the next version :)


Thanks,
SJ

>
> Thanks,
> Honggyu

2024-01-17 21:24:29

by SeongJae Park

Subject: Re: [RFC PATCH 0/4] DAMON based 2-tier memory management for CXL memory

On Wed, 17 Jan 2024 13:11:03 -0800 SeongJae Park <[email protected]> wrote:

[...]
> Hi Honggyu,
>
> On Wed, 17 Jan 2024 20:49:25 +0900 Honggyu Kim <[email protected]> wrote:
>
> > Hi SeongJae,
> >
> > Thanks very much for your comments in details.
> >
> > On Tue, 16 Jan 2024 12:31:59 -0800 SeongJae Park <[email protected]> wrote:
> >
[...]
> > > To this end, I feel the problem might be able te be simpler, because this
> > > patchset is trying to provide two sophisticated operations, while I think a
> > > simpler approach might be possible. My humble simpler idea is adding a DAMOS
> > > operation for moving pages to a given node (like sys_move_phy_pages RFC[1]),
> > > instead of the promote/demote. Because the general pages migration can handle
> > > multiple cases including the promote/demote in my humble assumption.
[...]
> > > In more detail, users could decide which is the appropriate node for promotion
> > > or demotion and use the new DAMOS action to do promotion and demotion. Users
> > > would requested to decide which node is the proper promotion/demotion target
> > > nodes, but that decision wouldn't be that hard in my opinion.
> > >
> > > For this, 'struct damos' would need to be updated for such argument-dependent
> > > actions, like 'struct damos_filter' is haing a union.
> >
> > That might be a better solution. I will think about it.
>
> More specifically, I think receiving an address range as the argument might
> more flexible than just NUMA node. Maybe we can imagine proactively migrating
> cold movable pages from normal zones to movable zones, to avoid normal zone
> memory pressure.

Yet another crazy idea. Finding hot regions in the middle of cold region and
move to besides of other hot pages. As a result, memory is sorted by access
temperature even in same node, and the system gains more spatial locality,
which benefits general locality-based algorithms including DAMON's adaptive
regions adjustment.


Thanks,
SJ

[...]

2024-01-18 10:55:48

by Hyeongtak Ji

Subject: Re: [RFC PATCH 0/4] DAMON based 2-tier memory management for CXL memory

Hi SeongJae,

On Wed, 17 Jan 2024 SeongJae Park <[email protected]> wrote:

[...]
>> Let's say there are 3 nodes in the system and the first node0 and node1
>> are the first tier, and node2 is the second tier.
>>
>> $ cat /sys/devices/virtual/memory_tiering/memory_tier4/nodelist
>> 0-1
>>
>> $ cat /sys/devices/virtual/memory_tiering/memory_tier22/nodelist
>> 2
>>
>> Here is the result of partitioning hot/cold memory and I put execution
>> command at the right side of numastat result. I initially ran each
>> hot_cold program with preferred setting so that they initially allocate
>> memory on one of node0 or node2, but they gradually migrated based on
>> their access frequencies.
>>
>> $ numastat -c -p hot_cold
>> Per-node process memory usage (in MBs)
>> PID Node 0 Node 1 Node 2 Total
>> --------------- ------ ------ ------ -----
>> 754 (hot_cold) 1800 0 2000 3800 <- hot_cold 1800 2000
>> 1184 (hot_cold) 300 0 500 800 <- hot_cold 300 500
>> 1818 (hot_cold) 801 0 3199 4000 <- hot_cold 800 3200
>> 30289 (hot_cold) 4 0 5 10 <- hot_cold 3 5
>> 30325 (hot_cold) 31 0 51 81 <- hot_cold 30 50
>> --------------- ------ ------ ------ -----
>> Total 2938 0 5756 8695
>>
>> The final node placement result shows that DAMON accurately migrated
>> pages by their hotness for multiple processes.
>
> What was the result when the corner cases handling logics were not applied?

This is the result of the same test that Honggyu did, but with an insufficient
corner cases handling logics.

$ numastat -c -p hot_cold

Per-node process memory usage (in MBs)
PID Node 0 Node 1 Node 2 Total
-------------- ------ ------ ------ -----
862 (hot_cold) 2256 0 1545 3801 <- hot_cold 1800 2000
863 (hot_cold) 403 0 398 801 <- hot_cold 300 500
864 (hot_cold) 1520 0 2482 4001 <- hot_cold 800 3200
865 (hot_cold) 6 0 3 9 <- hot_cold 3 5
866 (hot_cold) 29 0 52 81 <- hot_cold 30 50
-------------- ------ ------ ------ -----
Total 4215 0 4480 8695

As time goes by, DAMON keeps trying to split the hot/cold region, but it does
not seem to be enough.

$ numastat -c -p hot_cold

Per-node process memory usage (in MBs)
PID Node 0 Node 1 Node 2 Total
-------------- ------ ------ ------ -----
862 (hot_cold) 2022 0 1780 3801 <- hot_cold 1800 2000
863 (hot_cold) 351 0 450 801 <- hot_cold 300 500
864 (hot_cold) 1134 0 2868 4001 <- hot_cold 800 3200
865 (hot_cold) 7 0 2 9 <- hot_cold 3 5
866 (hot_cold) 43 0 39 81 <- hot_cold 30 50
-------------- ------ ------ ------ -----
Total 3557 0 5138 8695

>
> And, what are the corner cases handling logic that seemed essential? I show
> the page granularity active/reference check could indeed provide many
> improvements, but that's only my humble assumption.

Yes, the page granularity active/reference check is essential. To make the
above "insufficient" result, the only thing I did was to promote
inactive/not_referenced pages.

diff --git a/mm/vmscan.c b/mm/vmscan.c
index f03be320f9ad..c2aefb883c54 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -1127,9 +1127,7 @@ static unsigned int __promote_folio_list(struct list_head *folio_list,
VM_BUG_ON_FOLIO(folio_test_active(folio), folio);

references = folio_check_references(folio, sc);
- if (references == FOLIOREF_KEEP ||
- references == FOLIOREF_RECLAIM ||
- references == FOLIOREF_RECLAIM_CLEAN)
+ if (references == FOLIOREF_KEEP )
goto keep_locked;

/* Relocate its contents to another node. */

>
> If the corner cases are indeed better to be applied in page granularity, I
> agree we need some more efforts since DAMON monitoring results are not page
> granularity aware by the design. Users could increase min_nr_regions to make
> it more accurate, and we have plan to support page granularity monitoring,
> though. But maybe the overhead could be unacceptable.
>
> Ideal solution would be making DAMON more accurate while keeping current level
> of overhead. We indeed have TODO items for DAMON accuracy improvement, but
> this may take some time that might unacceptable for your case.
>
> If that's the case, I think the additional corner handling (or, page gran
> additional access check) could be made as DAMOS filters[1], since DAMOS filters
> can be applied in page granularity, and designed for this kind of handling of
> information that DAMON monitoring results cannot provide. More specifically,
> we could have filters for promotion-qualifying pages and demotion-qualifying
> pages. In this way, I think we can keep the action more flexible while the
> filters can be applied in creative ways.

Making corner handling as a new DAMOS filters is a good idea. I'm just a bit
concerned if adding new filters might cause users to care more.

Kind regards,
Hyeongtak

2024-01-18 17:18:07

by SeongJae Park

Subject: Re: [RFC PATCH 0/4] DAMON based 2-tier memory management for CXL memory

On Thu, 18 Jan 2024 19:40:16 +0900 Hyeongtak Ji <[email protected]> wrote:

> Hi SeongJae,
>
> On Wed, 17 Jan 2024 SeongJae Park <[email protected]> wrote:
>
> [...]
> >> Let's say there are 3 nodes in the system and the first node0 and node1
> >> are the first tier, and node2 is the second tier.
> >>
> >> $ cat /sys/devices/virtual/memory_tiering/memory_tier4/nodelist
> >> 0-1
> >>
> >> $ cat /sys/devices/virtual/memory_tiering/memory_tier22/nodelist
> >> 2
> >>
> >> Here is the result of partitioning hot/cold memory and I put execution
> >> command at the right side of numastat result. I initially ran each
> >> hot_cold program with preferred setting so that they initially allocate
> >> memory on one of node0 or node2, but they gradually migrated based on
> >> their access frequencies.
> >>
> >> $ numastat -c -p hot_cold
> >> Per-node process memory usage (in MBs)
> >> PID Node 0 Node 1 Node 2 Total
> >> --------------- ------ ------ ------ -----
> >> 754 (hot_cold) 1800 0 2000 3800 <- hot_cold 1800 2000
> >> 1184 (hot_cold) 300 0 500 800 <- hot_cold 300 500
> >> 1818 (hot_cold) 801 0 3199 4000 <- hot_cold 800 3200
> >> 30289 (hot_cold) 4 0 5 10 <- hot_cold 3 5
> >> 30325 (hot_cold) 31 0 51 81 <- hot_cold 30 50
> >> --------------- ------ ------ ------ -----
> >> Total 2938 0 5756 8695
> >>
> >> The final node placement result shows that DAMON accurately migrated
> >> pages by their hotness for multiple processes.
> >
> > What was the result when the corner cases handling logics were not applied?
>
> This is the result of the same test that Honggyu did, but with an insufficient
> corner cases handling logics.
>
> $ numastat -c -p hot_cold
>
> Per-node process memory usage (in MBs)
> PID Node 0 Node 1 Node 2 Total
> -------------- ------ ------ ------ -----
> 862 (hot_cold) 2256 0 1545 3801 <- hot_cold 1800 2000
> 863 (hot_cold) 403 0 398 801 <- hot_cold 300 500
> 864 (hot_cold) 1520 0 2482 4001 <- hot_cold 800 3200
> 865 (hot_cold) 6 0 3 9 <- hot_cold 3 5
> 866 (hot_cold) 29 0 52 81 <- hot_cold 30 50
> -------------- ------ ------ ------ -----
> Total 4215 0 4480 8695
>
> As time goes by, DAMON keeps trying to split the hot/cold region, but it does
> not seem to be enough.
>
> $ numastat -c -p hot_cold
>
> Per-node process memory usage (in MBs)
> PID Node 0 Node 1 Node 2 Total
> -------------- ------ ------ ------ -----
> 862 (hot_cold) 2022 0 1780 3801 <- hot_cold 1800 2000
> 863 (hot_cold) 351 0 450 801 <- hot_cold 300 500
> 864 (hot_cold) 1134 0 2868 4001 <- hot_cold 800 3200
> 865 (hot_cold) 7 0 2 9 <- hot_cold 3 5
> 866 (hot_cold) 43 0 39 81 <- hot_cold 30 50
> -------------- ------ ------ ------ -----
> Total 3557 0 5138 8695
>
> >
> > And, what are the corner cases handling logic that seemed essential? I show
> > the page granularity active/reference check could indeed provide many
> > improvements, but that's only my humble assumption.
>
> Yes, the page granularity active/reference check is essential. To make the
> above "insufficient" result, the only thing I did was to promote
> inactive/not_referenced pages.
>
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index f03be320f9ad..c2aefb883c54 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -1127,9 +1127,7 @@ static unsigned int __promote_folio_list(struct list_head *folio_list,
> VM_BUG_ON_FOLIO(folio_test_active(folio), folio);
>
> references = folio_check_references(folio, sc);
> - if (references == FOLIOREF_KEEP ||
> - references == FOLIOREF_RECLAIM ||
> - references == FOLIOREF_RECLAIM_CLEAN)
> + if (references == FOLIOREF_KEEP )
> goto keep_locked;
>
> /* Relocate its contents to another node. */

Thank you for sharing the details :) I think DAMOS filters based approach
could be worthy to try, then.

>
> >
> > If the corner cases are indeed better to be applied in page granularity, I
> > agree we need some more efforts since DAMON monitoring results are not page
> > granularity aware by the design. Users could increase min_nr_regions to make
> > it more accurate, and we have plan to support page granularity monitoring,
> > though. But maybe the overhead could be unacceptable.
> >
> > Ideal solution would be making DAMON more accurate while keeping current level
> > of overhead. We indeed have TODO items for DAMON accuracy improvement, but
> > this may take some time that might unacceptable for your case.
> >
> > If that's the case, I think the additional corner handling (or, page gran
> > additional access check) could be made as DAMOS filters[1], since DAMOS filters
> > can be applied in page granularity, and designed for this kind of handling of
> > information that DAMON monitoring results cannot provide. More specifically,
> > we could have filters for promotion-qualifying pages and demotion-qualifying
> > pages. In this way, I think we can keep the action more flexible while the
> > filters can be applied in creative ways.
>
> Making corner handling as a new DAMOS filters is a good idea. I'm just a bit
> concerned if adding new filters might cause users to care more.

I prefer keeping DAMON API and Sysfs interface flexible and easy to extended
even if it increases number of parameters, while providing simplified
high level interfaces for end users aiming to use DAMON for specific use cases,
like DAMON_RECLAIM, DAMON_LRU_SORT, and damo do. Hence I'm not very concerned.


Thanks,
SJ

>
> Kind regards,
> Hyeongtak