| Message ID | 20240223054407.14829-1-byungchul@sk.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] mm, vmscan: do not turn on cache_trim_mode if it doesn't work | expand |
Byungchul Park <byungchul@sk.com> writes: > Changes from v2: > 1. Change the condition to stop cache_trim_mode. > > From - Stop it if it's at high scan priorities, 0 or 1. > To - Stop it if it's at high scan priorities, 0 or 1, and > the mode didn't work in the previous turn. > > (feedbacked by Huang Ying) > > 2. Change the test result in the commit message after testing > with the new logic. > > Changes from v1: > 1. Add a comment describing why this change is necessary in code > and rewrite the commit message with how to reproduce and what > the result is using vmstat. (feedbacked by Andrew Morton and > Yu Zhao) > 2. Change the condition to avoid cache_trim_mode from > 'sc->priority != 1' to 'sc->priority > 1' to reflect cases > where the priority goes to zero all the way. (feedbacked by > Yu Zhao) > > --->8--- > From 05846e34bf02ac9b3e254324dc2d7afd97a025d9 Mon Sep 17 00:00:00 2001 > From: Byungchul Park <byungchul@sk.com> > Date: Fri, 23 Feb 2024 13:47:16 +0900 > Subject: [PATCH v3] mm, vmscan: do not turn on cache_trim_mode if it doesn't work > > With cache_trim_mode on, reclaim logic doesn't bother reclaiming anon > pages. However, it should be more careful to turn on the mode because > it's going to prevent anon pages from being reclaimed even if there are > a huge number of anon pages that are cold and should be reclaimed. Even > worse, that leads kswapd_failures to reach MAX_RECLAIM_RETRIES and > stopping kswapd from functioning until direct reclaim eventually works > to resume kswapd. > > So do not turn on cache_trim_mode if the mode doesn't work, especially > while the sytem is struggling against reclaim. > > The problematic behavior can be reproduced by: > > CONFIG_NUMA_BALANCING enabled > sysctl_numa_balancing_mode set to NUMA_BALANCING_MEMORY_TIERING > numa node0 (8GB local memory, 16 CPUs) > numa node1 (8GB slow tier memory, no CPUs) > > Sequence: > > 1) echo 3 > /proc/sys/vm/drop_caches > 2) To emulate the system with full of cold memory in local DRAM, run > the following dummy program and never touch the region: > > mmap(0, 8 * 1024 * 1024 * 1024, PROT_READ | PROT_WRITE, > MAP_ANONYMOUS | MAP_PRIVATE | MAP_POPULATE, -1, 0); > > 3) Run any memory intensive work e.g. XSBench. > 4) Check if numa balancing is working e.i. promotion/demotion. > 5) Iterate 1) ~ 4) until numa balancing stops. > > With this, you could see that promotion/demotion are not working because > kswapd has stopped due to ->kswapd_failures >= MAX_RECLAIM_RETRIES. > > Interesting vmstat delta's differences between before and after are like: > > +-----------------------+-------------------------------+ > | interesting vmstat | before | after | > +-----------------------+-------------------------------+ > | nr_inactive_anon | 321935 | 1636737 | > | nr_active_anon | 1780700 | 465913 | > | nr_inactive_file | 30425 | 35711 | > | nr_active_file | 14961 | 8698 | > | pgpromote_success | 356 | 1267785 | > | pgpromote_candidate | 21953245 | 1745631 | > | pgactivate | 1844523 | 3309867 | > | pgdeactivate | 50634 | 1545041 | > | pgfault | 31100294 | 6411036 | > | pgdemote_kswapd | 30856 | 2267467 | > | pgscan_kswapd | 1861981 | 7729231 | > | pgscan_anon | 1822930 | 7667544 | > | pgscan_file | 39051 | 61687 | > | pgsteal_anon | 386 | 2227217 | > | pgsteal_file | 30470 | 40250 | > | pageoutrun | 30 | 457 | > | numa_hint_faults | 27418279 | 2752289 | > | numa_pages_migrated | 356 | 1267785 | > +-----------------------+-------------------------------+ > > Signed-off-by: Byungchul Park <byungchul@sk.com> > --- > mm/vmscan.c | 24 +++++++++++++++++++----- > 1 file changed, 19 insertions(+), 5 deletions(-) > > diff --git a/mm/vmscan.c b/mm/vmscan.c > index bba207f41b14..f7312d831fed 100644 > --- a/mm/vmscan.c > +++ b/mm/vmscan.c > @@ -127,6 +127,9 @@ struct scan_control { > /* One of the zones is ready for compaction */ > unsigned int compaction_ready:1; > > + /* If the last try was reclaimable */ > + unsigned int reclaimable:1; > + > /* There is easily reclaimable cold cache in the current node */ > unsigned int cache_trim_mode:1; > > @@ -2266,9 +2269,14 @@ static void prepare_scan_control(pg_data_t *pgdat, struct scan_control *sc) > * If we have plenty of inactive file pages that aren't > * thrashing, try to reclaim those first before touching > * anonymous pages. > + * > + * It doesn't make sense to keep cache_trim_mode on if the mode > + * is not working while struggling against reclaim. So do not > + * turn it on if so. Note the highest priority of kswapd is 1. > */ > file = lruvec_page_state(target_lruvec, NR_INACTIVE_FILE); > - if (file >> sc->priority && !(sc->may_deactivate & DEACTIVATE_FILE)) > + if (file >> sc->priority && !(sc->may_deactivate & DEACTIVATE_FILE) && > + !(sc->cache_trim_mode && !sc->reclaimable && sc->priority <= 1)) It's a little hard to digest the logic above for me, is it better to use the following logic? if (file >> sc->priority && !(sc->may_deactivate & DEACTIVATE_FILE) && (!sc->cache_trim_mode || sc->reclaimable || sc->priority > 1)) Otherwise, this looks good to me, feel free to add, Acked-by: "Huang, Ying" <ying.huang@intel.com> > sc->cache_trim_mode = 1; > else > sc->cache_trim_mode = 0; > @@ -5862,7 +5870,6 @@ static void shrink_node(pg_data_t *pgdat, struct scan_control *sc) > { > unsigned long nr_reclaimed, nr_scanned, nr_node_reclaimed; > struct lruvec *target_lruvec; > - bool reclaimable = false; > > if (lru_gen_enabled() && root_reclaim(sc)) { > lru_gen_shrink_node(pgdat, sc); > @@ -5877,6 +5884,14 @@ static void shrink_node(pg_data_t *pgdat, struct scan_control *sc) > nr_reclaimed = sc->nr_reclaimed; > nr_scanned = sc->nr_scanned; > > + /* > + * Reset to the default values at the start. > + */ > + if (sc->priority == DEF_PRIORITY) { > + sc->reclaimable = 1; > + sc->cache_trim_mode = 0; > + } > + > prepare_scan_control(pgdat, sc); > > shrink_node_memcgs(pgdat, sc); > @@ -5890,8 +5905,7 @@ static void shrink_node(pg_data_t *pgdat, struct scan_control *sc) > vmpressure(sc->gfp_mask, sc->target_mem_cgroup, true, > sc->nr_scanned - nr_scanned, nr_node_reclaimed); > > - if (nr_node_reclaimed) > - reclaimable = true; > + sc->reclaimable = !!nr_node_reclaimed; > > if (current_is_kswapd()) { > /* > @@ -5965,7 +5979,7 @@ static void shrink_node(pg_data_t *pgdat, struct scan_control *sc) > * sleep. On reclaim progress, reset the failure counter. A > * successful direct reclaim run will revive a dormant kswapd. > */ > - if (reclaimable) > + if (sc->reclaimable) > pgdat->kswapd_failures = 0; > } -- Best Regards, Huang, Ying
On Fri, Feb 23, 2024 at 05:25:54PM +0800, Huang, Ying wrote: > Byungchul Park <byungchul@sk.com> writes: > > > Changes from v2: > > 1. Change the condition to stop cache_trim_mode. > > > > From - Stop it if it's at high scan priorities, 0 or 1. > > To - Stop it if it's at high scan priorities, 0 or 1, and > > the mode didn't work in the previous turn. > > > > (feedbacked by Huang Ying) > > > > 2. Change the test result in the commit message after testing > > with the new logic. > > > > Changes from v1: > > 1. Add a comment describing why this change is necessary in code > > and rewrite the commit message with how to reproduce and what > > the result is using vmstat. (feedbacked by Andrew Morton and > > Yu Zhao) > > 2. Change the condition to avoid cache_trim_mode from > > 'sc->priority != 1' to 'sc->priority > 1' to reflect cases > > where the priority goes to zero all the way. (feedbacked by > > Yu Zhao) > > > > --->8--- > > From 05846e34bf02ac9b3e254324dc2d7afd97a025d9 Mon Sep 17 00:00:00 2001 > > From: Byungchul Park <byungchul@sk.com> > > Date: Fri, 23 Feb 2024 13:47:16 +0900 > > Subject: [PATCH v3] mm, vmscan: do not turn on cache_trim_mode if it doesn't work > > > > With cache_trim_mode on, reclaim logic doesn't bother reclaiming anon > > pages. However, it should be more careful to turn on the mode because > > it's going to prevent anon pages from being reclaimed even if there are > > a huge number of anon pages that are cold and should be reclaimed. Even > > worse, that leads kswapd_failures to reach MAX_RECLAIM_RETRIES and > > stopping kswapd from functioning until direct reclaim eventually works > > to resume kswapd. > > > > So do not turn on cache_trim_mode if the mode doesn't work, especially > > while the sytem is struggling against reclaim. > > > > The problematic behavior can be reproduced by: > > > > CONFIG_NUMA_BALANCING enabled > > sysctl_numa_balancing_mode set to NUMA_BALANCING_MEMORY_TIERING > > numa node0 (8GB local memory, 16 CPUs) > > numa node1 (8GB slow tier memory, no CPUs) > > > > Sequence: > > > > 1) echo 3 > /proc/sys/vm/drop_caches > > 2) To emulate the system with full of cold memory in local DRAM, run > > the following dummy program and never touch the region: > > > > mmap(0, 8 * 1024 * 1024 * 1024, PROT_READ | PROT_WRITE, > > MAP_ANONYMOUS | MAP_PRIVATE | MAP_POPULATE, -1, 0); > > > > 3) Run any memory intensive work e.g. XSBench. > > 4) Check if numa balancing is working e.i. promotion/demotion. > > 5) Iterate 1) ~ 4) until numa balancing stops. > > > > With this, you could see that promotion/demotion are not working because > > kswapd has stopped due to ->kswapd_failures >= MAX_RECLAIM_RETRIES. > > > > Interesting vmstat delta's differences between before and after are like: > > > > +-----------------------+-------------------------------+ > > | interesting vmstat | before | after | > > +-----------------------+-------------------------------+ > > | nr_inactive_anon | 321935 | 1636737 | > > | nr_active_anon | 1780700 | 465913 | > > | nr_inactive_file | 30425 | 35711 | > > | nr_active_file | 14961 | 8698 | > > | pgpromote_success | 356 | 1267785 | > > | pgpromote_candidate | 21953245 | 1745631 | > > | pgactivate | 1844523 | 3309867 | > > | pgdeactivate | 50634 | 1545041 | > > | pgfault | 31100294 | 6411036 | > > | pgdemote_kswapd | 30856 | 2267467 | > > | pgscan_kswapd | 1861981 | 7729231 | > > | pgscan_anon | 1822930 | 7667544 | > > | pgscan_file | 39051 | 61687 | > > | pgsteal_anon | 386 | 2227217 | > > | pgsteal_file | 30470 | 40250 | > > | pageoutrun | 30 | 457 | > > | numa_hint_faults | 27418279 | 2752289 | > > | numa_pages_migrated | 356 | 1267785 | > > +-----------------------+-------------------------------+ > > > > Signed-off-by: Byungchul Park <byungchul@sk.com> > > --- > > mm/vmscan.c | 24 +++++++++++++++++++----- > > 1 file changed, 19 insertions(+), 5 deletions(-) > > > > diff --git a/mm/vmscan.c b/mm/vmscan.c > > index bba207f41b14..f7312d831fed 100644 > > --- a/mm/vmscan.c > > +++ b/mm/vmscan.c > > @@ -127,6 +127,9 @@ struct scan_control { > > /* One of the zones is ready for compaction */ > > unsigned int compaction_ready:1; > > > > + /* If the last try was reclaimable */ > > + unsigned int reclaimable:1; > > + > > /* There is easily reclaimable cold cache in the current node */ > > unsigned int cache_trim_mode:1; > > > > @@ -2266,9 +2269,14 @@ static void prepare_scan_control(pg_data_t *pgdat, struct scan_control *sc) > > * If we have plenty of inactive file pages that aren't > > * thrashing, try to reclaim those first before touching > > * anonymous pages. > > + * > > + * It doesn't make sense to keep cache_trim_mode on if the mode > > + * is not working while struggling against reclaim. So do not > > + * turn it on if so. Note the highest priority of kswapd is 1. > > */ > > file = lruvec_page_state(target_lruvec, NR_INACTIVE_FILE); > > - if (file >> sc->priority && !(sc->may_deactivate & DEACTIVATE_FILE)) > > + if (file >> sc->priority && !(sc->may_deactivate & DEACTIVATE_FILE) && > > + !(sc->cache_trim_mode && !sc->reclaimable && sc->priority <= 1)) > > It's a little hard to digest the logic above for me, is it better to use > the following logic? > > if (file >> sc->priority && !(sc->may_deactivate & DEACTIVATE_FILE) && > (!sc->cache_trim_mode || sc->reclaimable || sc->priority > 1)) Looks fine to me. Thanks. Byungchul > Otherwise, this looks good to me, feel free to add, > > Acked-by: "Huang, Ying" <ying.huang@intel.com> > > > sc->cache_trim_mode = 1; > > else > > sc->cache_trim_mode = 0; > > @@ -5862,7 +5870,6 @@ static void shrink_node(pg_data_t *pgdat, struct scan_control *sc) > > { > > unsigned long nr_reclaimed, nr_scanned, nr_node_reclaimed; > > struct lruvec *target_lruvec; > > - bool reclaimable = false; > > > > if (lru_gen_enabled() && root_reclaim(sc)) { > > lru_gen_shrink_node(pgdat, sc); > > @@ -5877,6 +5884,14 @@ static void shrink_node(pg_data_t *pgdat, struct scan_control *sc) > > nr_reclaimed = sc->nr_reclaimed; > > nr_scanned = sc->nr_scanned; > > > > + /* > > + * Reset to the default values at the start. > > + */ > > + if (sc->priority == DEF_PRIORITY) { > > + sc->reclaimable = 1; > > + sc->cache_trim_mode = 0; > > + } > > + > > prepare_scan_control(pgdat, sc); > > > > shrink_node_memcgs(pgdat, sc); > > @@ -5890,8 +5905,7 @@ static void shrink_node(pg_data_t *pgdat, struct scan_control *sc) > > vmpressure(sc->gfp_mask, sc->target_mem_cgroup, true, > > sc->nr_scanned - nr_scanned, nr_node_reclaimed); > > > > - if (nr_node_reclaimed) > > - reclaimable = true; > > + sc->reclaimable = !!nr_node_reclaimed; > > > > if (current_is_kswapd()) { > > /* > > @@ -5965,7 +5979,7 @@ static void shrink_node(pg_data_t *pgdat, struct scan_control *sc) > > * sleep. On reclaim progress, reset the failure counter. A > > * successful direct reclaim run will revive a dormant kswapd. > > */ > > - if (reclaimable) > > + if (sc->reclaimable) > > pgdat->kswapd_failures = 0; > > } > > -- > Best Regards, > Huang, Ying
[CC Mel, Vlastimil and Johannes for awareness] On Fri 23-02-24 14:44:07, Byungchul Park wrote: > Changes from v2: > 1. Change the condition to stop cache_trim_mode. > > From - Stop it if it's at high scan priorities, 0 or 1. > To - Stop it if it's at high scan priorities, 0 or 1, and > the mode didn't work in the previous turn. > > (feedbacked by Huang Ying) > > 2. Change the test result in the commit message after testing > with the new logic. > > Changes from v1: > 1. Add a comment describing why this change is necessary in code > and rewrite the commit message with how to reproduce and what > the result is using vmstat. (feedbacked by Andrew Morton and > Yu Zhao) > 2. Change the condition to avoid cache_trim_mode from > 'sc->priority != 1' to 'sc->priority > 1' to reflect cases > where the priority goes to zero all the way. (feedbacked by > Yu Zhao) > > --->8--- > >From 05846e34bf02ac9b3e254324dc2d7afd97a025d9 Mon Sep 17 00:00:00 2001 > From: Byungchul Park <byungchul@sk.com> > Date: Fri, 23 Feb 2024 13:47:16 +0900 > Subject: [PATCH v3] mm, vmscan: do not turn on cache_trim_mode if it doesn't work > > With cache_trim_mode on, reclaim logic doesn't bother reclaiming anon > pages. However, it should be more careful to turn on the mode because > it's going to prevent anon pages from being reclaimed even if there are > a huge number of anon pages that are cold and should be reclaimed. Even > worse, that leads kswapd_failures to reach MAX_RECLAIM_RETRIES and > stopping kswapd from functioning until direct reclaim eventually works > to resume kswapd. > > So do not turn on cache_trim_mode if the mode doesn't work, especially > while the sytem is struggling against reclaim. > > The problematic behavior can be reproduced by: > > CONFIG_NUMA_BALANCING enabled > sysctl_numa_balancing_mode set to NUMA_BALANCING_MEMORY_TIERING > numa node0 (8GB local memory, 16 CPUs) > numa node1 (8GB slow tier memory, no CPUs) > > Sequence: > > 1) echo 3 > /proc/sys/vm/drop_caches > 2) To emulate the system with full of cold memory in local DRAM, run > the following dummy program and never touch the region: > > mmap(0, 8 * 1024 * 1024 * 1024, PROT_READ | PROT_WRITE, > MAP_ANONYMOUS | MAP_PRIVATE | MAP_POPULATE, -1, 0); > > 3) Run any memory intensive work e.g. XSBench. > 4) Check if numa balancing is working e.i. promotion/demotion. > 5) Iterate 1) ~ 4) until numa balancing stops. > > With this, you could see that promotion/demotion are not working because > kswapd has stopped due to ->kswapd_failures >= MAX_RECLAIM_RETRIES. > > Interesting vmstat delta's differences between before and after are like: > > +-----------------------+-------------------------------+ > | interesting vmstat | before | after | > +-----------------------+-------------------------------+ > | nr_inactive_anon | 321935 | 1636737 | > | nr_active_anon | 1780700 | 465913 | > | nr_inactive_file | 30425 | 35711 | > | nr_active_file | 14961 | 8698 | > | pgpromote_success | 356 | 1267785 | > | pgpromote_candidate | 21953245 | 1745631 | > | pgactivate | 1844523 | 3309867 | > | pgdeactivate | 50634 | 1545041 | > | pgfault | 31100294 | 6411036 | > | pgdemote_kswapd | 30856 | 2267467 | > | pgscan_kswapd | 1861981 | 7729231 | > | pgscan_anon | 1822930 | 7667544 | > | pgscan_file | 39051 | 61687 | > | pgsteal_anon | 386 | 2227217 | > | pgsteal_file | 30470 | 40250 | > | pageoutrun | 30 | 457 | > | numa_hint_faults | 27418279 | 2752289 | > | numa_pages_migrated | 356 | 1267785 | > +-----------------------+-------------------------------+ > > Signed-off-by: Byungchul Park <byungchul@sk.com> > --- > mm/vmscan.c | 24 +++++++++++++++++++----- > 1 file changed, 19 insertions(+), 5 deletions(-) > > diff --git a/mm/vmscan.c b/mm/vmscan.c > index bba207f41b14..f7312d831fed 100644 > --- a/mm/vmscan.c > +++ b/mm/vmscan.c > @@ -127,6 +127,9 @@ struct scan_control { > /* One of the zones is ready for compaction */ > unsigned int compaction_ready:1; > > + /* If the last try was reclaimable */ > + unsigned int reclaimable:1; > + > /* There is easily reclaimable cold cache in the current node */ > unsigned int cache_trim_mode:1; > > @@ -2266,9 +2269,14 @@ static void prepare_scan_control(pg_data_t *pgdat, struct scan_control *sc) > * If we have plenty of inactive file pages that aren't > * thrashing, try to reclaim those first before touching > * anonymous pages. > + * > + * It doesn't make sense to keep cache_trim_mode on if the mode > + * is not working while struggling against reclaim. So do not > + * turn it on if so. Note the highest priority of kswapd is 1. > */ > file = lruvec_page_state(target_lruvec, NR_INACTIVE_FILE); > - if (file >> sc->priority && !(sc->may_deactivate & DEACTIVATE_FILE)) > + if (file >> sc->priority && !(sc->may_deactivate & DEACTIVATE_FILE) && > + !(sc->cache_trim_mode && !sc->reclaimable && sc->priority <= 1)) > sc->cache_trim_mode = 1; > else > sc->cache_trim_mode = 0; > @@ -5862,7 +5870,6 @@ static void shrink_node(pg_data_t *pgdat, struct scan_control *sc) > { > unsigned long nr_reclaimed, nr_scanned, nr_node_reclaimed; > struct lruvec *target_lruvec; > - bool reclaimable = false; > > if (lru_gen_enabled() && root_reclaim(sc)) { > lru_gen_shrink_node(pgdat, sc); > @@ -5877,6 +5884,14 @@ static void shrink_node(pg_data_t *pgdat, struct scan_control *sc) > nr_reclaimed = sc->nr_reclaimed; > nr_scanned = sc->nr_scanned; > > + /* > + * Reset to the default values at the start. > + */ > + if (sc->priority == DEF_PRIORITY) { > + sc->reclaimable = 1; > + sc->cache_trim_mode = 0; > + } > + > prepare_scan_control(pgdat, sc); > > shrink_node_memcgs(pgdat, sc); > @@ -5890,8 +5905,7 @@ static void shrink_node(pg_data_t *pgdat, struct scan_control *sc) > vmpressure(sc->gfp_mask, sc->target_mem_cgroup, true, > sc->nr_scanned - nr_scanned, nr_node_reclaimed); > > - if (nr_node_reclaimed) > - reclaimable = true; > + sc->reclaimable = !!nr_node_reclaimed; > > if (current_is_kswapd()) { > /* > @@ -5965,7 +5979,7 @@ static void shrink_node(pg_data_t *pgdat, struct scan_control *sc) > * sleep. On reclaim progress, reset the failure counter. A > * successful direct reclaim run will revive a dormant kswapd. > */ > - if (reclaimable) > + if (sc->reclaimable) > pgdat->kswapd_failures = 0; > } > > -- > 2.17.1 >
On Mon, Feb 26, 2024 at 02:06:30PM +0100, Michal Hocko wrote: > [CC Mel, Vlastimil and Johannes for awareness] > > On Fri 23-02-24 14:44:07, Byungchul Park wrote: > > Changes from v2: > > 1. Change the condition to stop cache_trim_mode. > > > > From - Stop it if it's at high scan priorities, 0 or 1. > > To - Stop it if it's at high scan priorities, 0 or 1, and > > the mode didn't work in the previous turn. > > > > (feedbacked by Huang Ying) > > > > 2. Change the test result in the commit message after testing > > with the new logic. > > > > Changes from v1: > > 1. Add a comment describing why this change is necessary in code > > and rewrite the commit message with how to reproduce and what > > the result is using vmstat. (feedbacked by Andrew Morton and > > Yu Zhao) > > 2. Change the condition to avoid cache_trim_mode from > > 'sc->priority != 1' to 'sc->priority > 1' to reflect cases > > where the priority goes to zero all the way. (feedbacked by > > Yu Zhao) > > > > --->8--- > > >From 05846e34bf02ac9b3e254324dc2d7afd97a025d9 Mon Sep 17 00:00:00 2001 > > From: Byungchul Park <byungchul@sk.com> > > Date: Fri, 23 Feb 2024 13:47:16 +0900 > > Subject: [PATCH v3] mm, vmscan: do not turn on cache_trim_mode if it doesn't work > > > > With cache_trim_mode on, reclaim logic doesn't bother reclaiming anon > > pages. However, it should be more careful to turn on the mode because > > it's going to prevent anon pages from being reclaimed even if there are > > a huge number of anon pages that are cold and should be reclaimed. Even > > worse, that leads kswapd_failures to reach MAX_RECLAIM_RETRIES and > > stopping kswapd from functioning until direct reclaim eventually works > > to resume kswapd. > > > > So do not turn on cache_trim_mode if the mode doesn't work, especially > > while the sytem is struggling against reclaim. > > > > The problematic behavior can be reproduced by: > > > > CONFIG_NUMA_BALANCING enabled > > sysctl_numa_balancing_mode set to NUMA_BALANCING_MEMORY_TIERING > > numa node0 (8GB local memory, 16 CPUs) > > numa node1 (8GB slow tier memory, no CPUs) > > > > Sequence: > > > > 1) echo 3 > /proc/sys/vm/drop_caches > > 2) To emulate the system with full of cold memory in local DRAM, run > > the following dummy program and never touch the region: > > > > mmap(0, 8 * 1024 * 1024 * 1024, PROT_READ | PROT_WRITE, > > MAP_ANONYMOUS | MAP_PRIVATE | MAP_POPULATE, -1, 0); > > > > 3) Run any memory intensive work e.g. XSBench. > > 4) Check if numa balancing is working e.i. promotion/demotion. > > 5) Iterate 1) ~ 4) until numa balancing stops. > > > > With this, you could see that promotion/demotion are not working because > > kswapd has stopped due to ->kswapd_failures >= MAX_RECLAIM_RETRIES. > > > > Interesting vmstat delta's differences between before and after are like: > > > > +-----------------------+-------------------------------+ > > | interesting vmstat | before | after | > > +-----------------------+-------------------------------+ > > | nr_inactive_anon | 321935 | 1636737 | > > | nr_active_anon | 1780700 | 465913 | > > | nr_inactive_file | 30425 | 35711 | > > | nr_active_file | 14961 | 8698 | > > | pgpromote_success | 356 | 1267785 | > > | pgpromote_candidate | 21953245 | 1745631 | > > | pgactivate | 1844523 | 3309867 | > > | pgdeactivate | 50634 | 1545041 | > > | pgfault | 31100294 | 6411036 | > > | pgdemote_kswapd | 30856 | 2267467 | > > | pgscan_kswapd | 1861981 | 7729231 | > > | pgscan_anon | 1822930 | 7667544 | > > | pgscan_file | 39051 | 61687 | > > | pgsteal_anon | 386 | 2227217 | > > | pgsteal_file | 30470 | 40250 | > > | pageoutrun | 30 | 457 | > > | numa_hint_faults | 27418279 | 2752289 | > > | numa_pages_migrated | 356 | 1267785 | > > +-----------------------+-------------------------------+ > > > > Signed-off-by: Byungchul Park <byungchul@sk.com> > > --- > > mm/vmscan.c | 24 +++++++++++++++++++----- > > 1 file changed, 19 insertions(+), 5 deletions(-) > > > > diff --git a/mm/vmscan.c b/mm/vmscan.c > > index bba207f41b14..f7312d831fed 100644 > > --- a/mm/vmscan.c > > +++ b/mm/vmscan.c > > @@ -127,6 +127,9 @@ struct scan_control { > > /* One of the zones is ready for compaction */ > > unsigned int compaction_ready:1; > > > > + /* If the last try was reclaimable */ > > + unsigned int reclaimable:1; > > + > > /* There is easily reclaimable cold cache in the current node */ > > unsigned int cache_trim_mode:1; > > > > @@ -2266,9 +2269,14 @@ static void prepare_scan_control(pg_data_t *pgdat, struct scan_control *sc) > > * If we have plenty of inactive file pages that aren't > > * thrashing, try to reclaim those first before touching > > * anonymous pages. > > + * > > + * It doesn't make sense to keep cache_trim_mode on if the mode > > + * is not working while struggling against reclaim. So do not > > + * turn it on if so. Note the highest priority of kswapd is 1. > > */ > > file = lruvec_page_state(target_lruvec, NR_INACTIVE_FILE); > > - if (file >> sc->priority && !(sc->may_deactivate & DEACTIVATE_FILE)) > > + if (file >> sc->priority && !(sc->may_deactivate & DEACTIVATE_FILE) && > > + !(sc->cache_trim_mode && !sc->reclaimable && sc->priority <= 1)) > > sc->cache_trim_mode = 1; > > else > > sc->cache_trim_mode = 0; The overall goal makes sense to me. file >> priority is just a heuristic that there are enough potential candidate pages, not a guarantee that any forward progress will happen. So it makes sense to retry without before failing. The way you wrote this conditional kind of hurts my head, though. Please don't write negations of complex terms like this. It expands to this: !sc->cache_trim_mode || sc->reclaimable || sc->priority > 1 which I'm not sure makes sense. Surely it should be something like !sc->cache_trim_mode && sc->reclaimable && sc->priority > 1 instead? Also if (!sc->cache_trim_mode) sc->cache_trim_mode = 1 else sc->cache_trim_mode = 0 will toggle on every loop. So if direct reclaim runs through a zonelist, it'll cache trim every other numa node...? > > @@ -5862,7 +5870,6 @@ static void shrink_node(pg_data_t *pgdat, struct scan_control *sc) > > { > > unsigned long nr_reclaimed, nr_scanned, nr_node_reclaimed; > > struct lruvec *target_lruvec; > > - bool reclaimable = false; > > > > if (lru_gen_enabled() && root_reclaim(sc)) { > > lru_gen_shrink_node(pgdat, sc); > > @@ -5877,6 +5884,14 @@ static void shrink_node(pg_data_t *pgdat, struct scan_control *sc) > > nr_reclaimed = sc->nr_reclaimed; > > nr_scanned = sc->nr_scanned; > > > > + /* > > + * Reset to the default values at the start. > > + */ > > + if (sc->priority == DEF_PRIORITY) { > > + sc->reclaimable = 1; > > + sc->cache_trim_mode = 0; > > + } > > + > > prepare_scan_control(pgdat, sc); > > > > shrink_node_memcgs(pgdat, sc); > > @@ -5890,8 +5905,7 @@ static void shrink_node(pg_data_t *pgdat, struct scan_control *sc) > > vmpressure(sc->gfp_mask, sc->target_mem_cgroup, true, > > sc->nr_scanned - nr_scanned, nr_node_reclaimed); > > > > - if (nr_node_reclaimed) > > - reclaimable = true; > > + sc->reclaimable = !!nr_node_reclaimed; The scope of this doesn't quite make sense. If direct reclaim scans multiple nodes, reclaim failure on the first node would disable cache trim mode on the second node, which is totally unrelated. I think it needs separate paths for direct reclaim and kswapd. For direct reclaim, the retry should be before these similar retry catches in do_try_to_free_pages(), after all zones have been considered: /* * We make inactive:active ratio decisions based on the node's * composition of memory, but a restrictive reclaim_idx or a * memory.low cgroup setting can exempt large amounts of * memory from reclaim. Neither of which are very common, so * instead of doing costly eligibility calculations of the * entire cgroup subtree up front, we assume the estimates are * good, and retry with forcible deactivation if that fails. */ if (sc->skipped_deactivate) { sc->priority = initial_priority; sc->force_deactivate = 1; sc->skipped_deactivate = 0; goto retry; } /* Untapped cgroup reserves? Don't OOM, retry. */ if (sc->memcg_low_skipped) { sc->priority = initial_priority; sc->force_deactivate = 0; sc->memcg_low_reclaim = 1; sc->memcg_low_skipped = 0; goto retry; } and for kswapd it looks like it should be in balance_pgdat(), after the priority loop, before increasing kswapd_failures. Instead of sc->reclaimable, which is very broad, it would be better to call it sc->may_cache_trim_mode. This is in line with a bunch of other such mechanisms in scan_control.
On Wed, Feb 28, 2024 at 05:36:01PM -0500, Johannes Weiner wrote: > On Mon, Feb 26, 2024 at 02:06:30PM +0100, Michal Hocko wrote: > > [CC Mel, Vlastimil and Johannes for awareness] > > > > On Fri 23-02-24 14:44:07, Byungchul Park wrote: > > > Changes from v2: > > > 1. Change the condition to stop cache_trim_mode. > > > > > > From - Stop it if it's at high scan priorities, 0 or 1. > > > To - Stop it if it's at high scan priorities, 0 or 1, and > > > the mode didn't work in the previous turn. > > > > > > (feedbacked by Huang Ying) > > > > > > 2. Change the test result in the commit message after testing > > > with the new logic. > > > > > > Changes from v1: > > > 1. Add a comment describing why this change is necessary in code > > > and rewrite the commit message with how to reproduce and what > > > the result is using vmstat. (feedbacked by Andrew Morton and > > > Yu Zhao) > > > 2. Change the condition to avoid cache_trim_mode from > > > 'sc->priority != 1' to 'sc->priority > 1' to reflect cases > > > where the priority goes to zero all the way. (feedbacked by > > > Yu Zhao) > > > > > > --->8--- > > > >From 05846e34bf02ac9b3e254324dc2d7afd97a025d9 Mon Sep 17 00:00:00 2001 > > > From: Byungchul Park <byungchul@sk.com> > > > Date: Fri, 23 Feb 2024 13:47:16 +0900 > > > Subject: [PATCH v3] mm, vmscan: do not turn on cache_trim_mode if it doesn't work > > > > > > With cache_trim_mode on, reclaim logic doesn't bother reclaiming anon > > > pages. However, it should be more careful to turn on the mode because > > > it's going to prevent anon pages from being reclaimed even if there are > > > a huge number of anon pages that are cold and should be reclaimed. Even > > > worse, that leads kswapd_failures to reach MAX_RECLAIM_RETRIES and > > > stopping kswapd from functioning until direct reclaim eventually works > > > to resume kswapd. > > > > > > So do not turn on cache_trim_mode if the mode doesn't work, especially > > > while the sytem is struggling against reclaim. > > > > > > The problematic behavior can be reproduced by: > > > > > > CONFIG_NUMA_BALANCING enabled > > > sysctl_numa_balancing_mode set to NUMA_BALANCING_MEMORY_TIERING > > > numa node0 (8GB local memory, 16 CPUs) > > > numa node1 (8GB slow tier memory, no CPUs) > > > > > > Sequence: > > > > > > 1) echo 3 > /proc/sys/vm/drop_caches > > > 2) To emulate the system with full of cold memory in local DRAM, run > > > the following dummy program and never touch the region: > > > > > > mmap(0, 8 * 1024 * 1024 * 1024, PROT_READ | PROT_WRITE, > > > MAP_ANONYMOUS | MAP_PRIVATE | MAP_POPULATE, -1, 0); > > > > > > 3) Run any memory intensive work e.g. XSBench. > > > 4) Check if numa balancing is working e.i. promotion/demotion. > > > 5) Iterate 1) ~ 4) until numa balancing stops. > > > > > > With this, you could see that promotion/demotion are not working because > > > kswapd has stopped due to ->kswapd_failures >= MAX_RECLAIM_RETRIES. > > > > > > Interesting vmstat delta's differences between before and after are like: > > > > > > +-----------------------+-------------------------------+ > > > | interesting vmstat | before | after | > > > +-----------------------+-------------------------------+ > > > | nr_inactive_anon | 321935 | 1636737 | > > > | nr_active_anon | 1780700 | 465913 | > > > | nr_inactive_file | 30425 | 35711 | > > > | nr_active_file | 14961 | 8698 | > > > | pgpromote_success | 356 | 1267785 | > > > | pgpromote_candidate | 21953245 | 1745631 | > > > | pgactivate | 1844523 | 3309867 | > > > | pgdeactivate | 50634 | 1545041 | > > > | pgfault | 31100294 | 6411036 | > > > | pgdemote_kswapd | 30856 | 2267467 | > > > | pgscan_kswapd | 1861981 | 7729231 | > > > | pgscan_anon | 1822930 | 7667544 | > > > | pgscan_file | 39051 | 61687 | > > > | pgsteal_anon | 386 | 2227217 | > > > | pgsteal_file | 30470 | 40250 | > > > | pageoutrun | 30 | 457 | > > > | numa_hint_faults | 27418279 | 2752289 | > > > | numa_pages_migrated | 356 | 1267785 | > > > +-----------------------+-------------------------------+ > > > > > > Signed-off-by: Byungchul Park <byungchul@sk.com> > > > --- > > > mm/vmscan.c | 24 +++++++++++++++++++----- > > > 1 file changed, 19 insertions(+), 5 deletions(-) > > > > > > diff --git a/mm/vmscan.c b/mm/vmscan.c > > > index bba207f41b14..f7312d831fed 100644 > > > --- a/mm/vmscan.c > > > +++ b/mm/vmscan.c > > > @@ -127,6 +127,9 @@ struct scan_control { > > > /* One of the zones is ready for compaction */ > > > unsigned int compaction_ready:1; > > > > > > + /* If the last try was reclaimable */ > > > + unsigned int reclaimable:1; > > > + > > > /* There is easily reclaimable cold cache in the current node */ > > > unsigned int cache_trim_mode:1; > > > > > > @@ -2266,9 +2269,14 @@ static void prepare_scan_control(pg_data_t *pgdat, struct scan_control *sc) > > > * If we have plenty of inactive file pages that aren't > > > * thrashing, try to reclaim those first before touching > > > * anonymous pages. > > > + * > > > + * It doesn't make sense to keep cache_trim_mode on if the mode > > > + * is not working while struggling against reclaim. So do not > > > + * turn it on if so. Note the highest priority of kswapd is 1. > > > */ > > > file = lruvec_page_state(target_lruvec, NR_INACTIVE_FILE); > > > - if (file >> sc->priority && !(sc->may_deactivate & DEACTIVATE_FILE)) > > > + if (file >> sc->priority && !(sc->may_deactivate & DEACTIVATE_FILE) && > > > + !(sc->cache_trim_mode && !sc->reclaimable && sc->priority <= 1)) > > > sc->cache_trim_mode = 1; > > > else > > > sc->cache_trim_mode = 0; > > The overall goal makes sense to me. > > file >> priority is just a heuristic that there are enough potential > candidate pages, not a guarantee that any forward progress will > happen. So it makes sense to retry without before failing. > > The way you wrote this conditional kind of hurts my head, > though. Please don't write negations of complex terms like this. Okay. I won't. > It expands to this: > > !sc->cache_trim_mode || sc->reclaimable || sc->priority > 1 > > which I'm not sure makes sense. Surely it should be something like > > !sc->cache_trim_mode && sc->reclaimable && sc->priority > 1 It's a totally different condition as you know. > instead? > > Also > > if (!sc->cache_trim_mode) > sc->cache_trim_mode = 1 > else > sc->cache_trim_mode = 0 > > will toggle on every loop. So if direct reclaim runs through a > zonelist, it'll cache trim every other numa node...? No way to toggle on every loop. What I tried was that: 1. Don't turn it on again if it didn't work in the previous try. 2. Let it go as it was if the priority is not that high though, to keep the code as conservatively as possible. So again, the following condition is needed. (the original condition) && (!sc->cache_trim_mode || sc->reclaimable || sc->priority > 1) > > > @@ -5862,7 +5870,6 @@ static void shrink_node(pg_data_t *pgdat, struct scan_control *sc) > > > { > > > unsigned long nr_reclaimed, nr_scanned, nr_node_reclaimed; > > > struct lruvec *target_lruvec; > > > - bool reclaimable = false; > > > > > > if (lru_gen_enabled() && root_reclaim(sc)) { > > > lru_gen_shrink_node(pgdat, sc); > > > @@ -5877,6 +5884,14 @@ static void shrink_node(pg_data_t *pgdat, struct scan_control *sc) > > > nr_reclaimed = sc->nr_reclaimed; > > > nr_scanned = sc->nr_scanned; > > > > > > + /* > > > + * Reset to the default values at the start. > > > + */ > > > + if (sc->priority == DEF_PRIORITY) { This might need to be fixed if reclaim happens to start with other than DEF_PRIORITY. For now, reclaim always starts with the priority though. > > > + sc->reclaimable = 1; > > > + sc->cache_trim_mode = 0; > > > + } For each shrink_node(), initialize all the variable at the start. > > > + > > > prepare_scan_control(pgdat, sc); > > > > > > shrink_node_memcgs(pgdat, sc); > > > @@ -5890,8 +5905,7 @@ static void shrink_node(pg_data_t *pgdat, struct scan_control *sc) > > > vmpressure(sc->gfp_mask, sc->target_mem_cgroup, true, > > > sc->nr_scanned - nr_scanned, nr_node_reclaimed); > > > > > > - if (nr_node_reclaimed) > > > - reclaimable = true; > > > + sc->reclaimable = !!nr_node_reclaimed; > > The scope of this doesn't quite make sense. If direct reclaim scans > multiple nodes, reclaim failure on the first node would disable cache > trim mode on the second node, which is totally unrelated. As I mentioned, reclaim for every node would start with an initialized value because *each node is totally unrelated to another*. > I think it needs separate paths for direct reclaim and kswapd. For > direct reclaim, the retry should be before these similar retry catches > in do_try_to_free_pages(), after all zones have been considered: Remind sc->reclaimable and sc->cache_trim_mode will be initialized every shrink_node() in my patch. > /* > * We make inactive:active ratio decisions based on the node's > * composition of memory, but a restrictive reclaim_idx or a > * memory.low cgroup setting can exempt large amounts of > * memory from reclaim. Neither of which are very common, so > * instead of doing costly eligibility calculations of the > * entire cgroup subtree up front, we assume the estimates are > * good, and retry with forcible deactivation if that fails. > */ > if (sc->skipped_deactivate) { > sc->priority = initial_priority; > sc->force_deactivate = 1; > sc->skipped_deactivate = 0; > goto retry; > } > > /* Untapped cgroup reserves? Don't OOM, retry. */ > if (sc->memcg_low_skipped) { > sc->priority = initial_priority; > sc->force_deactivate = 0; > sc->memcg_low_reclaim = 1; > sc->memcg_low_skipped = 0; > goto retry; > } > > and for kswapd it looks like it should be in balance_pgdat(), after > the priority loop, before increasing kswapd_failures. > > Instead of sc->reclaimable, which is very broad, it would be better to > call it sc->may_cache_trim_mode. This is in line with a bunch of other > such mechanisms in scan_control. Or am I missing something? Byungchul
Byungchul Park <byungchul@sk.com> writes: > On Wed, Feb 28, 2024 at 05:36:01PM -0500, Johannes Weiner wrote: >> On Mon, Feb 26, 2024 at 02:06:30PM +0100, Michal Hocko wrote: >> > [CC Mel, Vlastimil and Johannes for awareness] >> > >> > On Fri 23-02-24 14:44:07, Byungchul Park wrote: >> > > Changes from v2: >> > > 1. Change the condition to stop cache_trim_mode. >> > > >> > > From - Stop it if it's at high scan priorities, 0 or 1. >> > > To - Stop it if it's at high scan priorities, 0 or 1, and >> > > the mode didn't work in the previous turn. >> > > >> > > (feedbacked by Huang Ying) >> > > >> > > 2. Change the test result in the commit message after testing >> > > with the new logic. >> > > >> > > Changes from v1: >> > > 1. Add a comment describing why this change is necessary in code >> > > and rewrite the commit message with how to reproduce and what >> > > the result is using vmstat. (feedbacked by Andrew Morton and >> > > Yu Zhao) >> > > 2. Change the condition to avoid cache_trim_mode from >> > > 'sc->priority != 1' to 'sc->priority > 1' to reflect cases >> > > where the priority goes to zero all the way. (feedbacked by >> > > Yu Zhao) >> > > >> > > --->8--- >> > > >From 05846e34bf02ac9b3e254324dc2d7afd97a025d9 Mon Sep 17 00:00:00 2001 >> > > From: Byungchul Park <byungchul@sk.com> >> > > Date: Fri, 23 Feb 2024 13:47:16 +0900 >> > > Subject: [PATCH v3] mm, vmscan: do not turn on cache_trim_mode if it doesn't work >> > > >> > > With cache_trim_mode on, reclaim logic doesn't bother reclaiming anon >> > > pages. However, it should be more careful to turn on the mode because >> > > it's going to prevent anon pages from being reclaimed even if there are >> > > a huge number of anon pages that are cold and should be reclaimed. Even >> > > worse, that leads kswapd_failures to reach MAX_RECLAIM_RETRIES and >> > > stopping kswapd from functioning until direct reclaim eventually works >> > > to resume kswapd. >> > > >> > > So do not turn on cache_trim_mode if the mode doesn't work, especially >> > > while the sytem is struggling against reclaim. >> > > >> > > The problematic behavior can be reproduced by: >> > > >> > > CONFIG_NUMA_BALANCING enabled >> > > sysctl_numa_balancing_mode set to NUMA_BALANCING_MEMORY_TIERING >> > > numa node0 (8GB local memory, 16 CPUs) >> > > numa node1 (8GB slow tier memory, no CPUs) >> > > >> > > Sequence: >> > > >> > > 1) echo 3 > /proc/sys/vm/drop_caches >> > > 2) To emulate the system with full of cold memory in local DRAM, run >> > > the following dummy program and never touch the region: >> > > >> > > mmap(0, 8 * 1024 * 1024 * 1024, PROT_READ | PROT_WRITE, >> > > MAP_ANONYMOUS | MAP_PRIVATE | MAP_POPULATE, -1, 0); >> > > >> > > 3) Run any memory intensive work e.g. XSBench. >> > > 4) Check if numa balancing is working e.i. promotion/demotion. >> > > 5) Iterate 1) ~ 4) until numa balancing stops. >> > > >> > > With this, you could see that promotion/demotion are not working because >> > > kswapd has stopped due to ->kswapd_failures >= MAX_RECLAIM_RETRIES. >> > > >> > > Interesting vmstat delta's differences between before and after are like: >> > > >> > > +-----------------------+-------------------------------+ >> > > | interesting vmstat | before | after | >> > > +-----------------------+-------------------------------+ >> > > | nr_inactive_anon | 321935 | 1636737 | >> > > | nr_active_anon | 1780700 | 465913 | >> > > | nr_inactive_file | 30425 | 35711 | >> > > | nr_active_file | 14961 | 8698 | >> > > | pgpromote_success | 356 | 1267785 | >> > > | pgpromote_candidate | 21953245 | 1745631 | >> > > | pgactivate | 1844523 | 3309867 | >> > > | pgdeactivate | 50634 | 1545041 | >> > > | pgfault | 31100294 | 6411036 | >> > > | pgdemote_kswapd | 30856 | 2267467 | >> > > | pgscan_kswapd | 1861981 | 7729231 | >> > > | pgscan_anon | 1822930 | 7667544 | >> > > | pgscan_file | 39051 | 61687 | >> > > | pgsteal_anon | 386 | 2227217 | >> > > | pgsteal_file | 30470 | 40250 | >> > > | pageoutrun | 30 | 457 | >> > > | numa_hint_faults | 27418279 | 2752289 | >> > > | numa_pages_migrated | 356 | 1267785 | >> > > +-----------------------+-------------------------------+ >> > > >> > > Signed-off-by: Byungchul Park <byungchul@sk.com> >> > > --- >> > > mm/vmscan.c | 24 +++++++++++++++++++----- >> > > 1 file changed, 19 insertions(+), 5 deletions(-) >> > > >> > > diff --git a/mm/vmscan.c b/mm/vmscan.c >> > > index bba207f41b14..f7312d831fed 100644 >> > > --- a/mm/vmscan.c >> > > +++ b/mm/vmscan.c >> > > @@ -127,6 +127,9 @@ struct scan_control { >> > > /* One of the zones is ready for compaction */ >> > > unsigned int compaction_ready:1; >> > > >> > > + /* If the last try was reclaimable */ >> > > + unsigned int reclaimable:1; >> > > + >> > > /* There is easily reclaimable cold cache in the current node */ >> > > unsigned int cache_trim_mode:1; >> > > >> > > @@ -2266,9 +2269,14 @@ static void prepare_scan_control(pg_data_t *pgdat, struct scan_control *sc) >> > > * If we have plenty of inactive file pages that aren't >> > > * thrashing, try to reclaim those first before touching >> > > * anonymous pages. >> > > + * >> > > + * It doesn't make sense to keep cache_trim_mode on if the mode >> > > + * is not working while struggling against reclaim. So do not >> > > + * turn it on if so. Note the highest priority of kswapd is 1. >> > > */ >> > > file = lruvec_page_state(target_lruvec, NR_INACTIVE_FILE); >> > > - if (file >> sc->priority && !(sc->may_deactivate & DEACTIVATE_FILE)) >> > > + if (file >> sc->priority && !(sc->may_deactivate & DEACTIVATE_FILE) && >> > > + !(sc->cache_trim_mode && !sc->reclaimable && sc->priority <= 1)) >> > > sc->cache_trim_mode = 1; >> > > else >> > > sc->cache_trim_mode = 0; >> >> The overall goal makes sense to me. >> >> file >> priority is just a heuristic that there are enough potential >> candidate pages, not a guarantee that any forward progress will >> happen. So it makes sense to retry without before failing. >> >> The way you wrote this conditional kind of hurts my head, >> though. Please don't write negations of complex terms like this. > > Okay. I won't. > >> It expands to this: >> >> !sc->cache_trim_mode || sc->reclaimable || sc->priority > 1 >> >> which I'm not sure makes sense. Surely it should be something like >> >> !sc->cache_trim_mode && sc->reclaimable && sc->priority > 1 > > It's a totally different condition as you know. > >> instead? >> >> Also >> >> if (!sc->cache_trim_mode) >> sc->cache_trim_mode = 1 >> else >> sc->cache_trim_mode = 0 >> >> will toggle on every loop. So if direct reclaim runs through a >> zonelist, it'll cache trim every other numa node...? > > No way to toggle on every loop. > > What I tried was that: > > 1. Don't turn it on again if it didn't work in the previous try. > 2. Let it go as it was if the priority is not that high though, > to keep the code as conservatively as possible. > > So again, the following condition is needed. > > (the original condition) && > (!sc->cache_trim_mode || sc->reclaimable || sc->priority > 1) > >> > > @@ -5862,7 +5870,6 @@ static void shrink_node(pg_data_t *pgdat, struct scan_control *sc) >> > > { >> > > unsigned long nr_reclaimed, nr_scanned, nr_node_reclaimed; >> > > struct lruvec *target_lruvec; >> > > - bool reclaimable = false; >> > > >> > > if (lru_gen_enabled() && root_reclaim(sc)) { >> > > lru_gen_shrink_node(pgdat, sc); >> > > @@ -5877,6 +5884,14 @@ static void shrink_node(pg_data_t *pgdat, struct scan_control *sc) >> > > nr_reclaimed = sc->nr_reclaimed; >> > > nr_scanned = sc->nr_scanned; >> > > >> > > + /* >> > > + * Reset to the default values at the start. >> > > + */ >> > > + if (sc->priority == DEF_PRIORITY) { > > This might need to be fixed if reclaim happens to start with other than > DEF_PRIORITY. For now, reclaim always starts with the priority though. > >> > > + sc->reclaimable = 1; >> > > + sc->cache_trim_mode = 0; >> > > + } > > For each shrink_node(), initialize all the variable at the start. > >> > > + >> > > prepare_scan_control(pgdat, sc); >> > > >> > > shrink_node_memcgs(pgdat, sc); >> > > @@ -5890,8 +5905,7 @@ static void shrink_node(pg_data_t *pgdat, struct scan_control *sc) >> > > vmpressure(sc->gfp_mask, sc->target_mem_cgroup, true, >> > > sc->nr_scanned - nr_scanned, nr_node_reclaimed); >> > > >> > > - if (nr_node_reclaimed) >> > > - reclaimable = true; >> > > + sc->reclaimable = !!nr_node_reclaimed; >> >> The scope of this doesn't quite make sense. If direct reclaim scans >> multiple nodes, reclaim failure on the first node would disable cache >> trim mode on the second node, which is totally unrelated. > > As I mentioned, reclaim for every node would start with an initialized > value because *each node is totally unrelated to another*. No. Please take a look at do_try_to_free_pages(), for each priority, it will iterate every node. But fortunately, we may not need this heuristics for direct reclaiming. -- Best Regards, Huang, Ying
Johannes Weiner <hannes@cmpxchg.org> writes: > On Mon, Feb 26, 2024 at 02:06:30PM +0100, Michal Hocko wrote: >> [CC Mel, Vlastimil and Johannes for awareness] >> >> On Fri 23-02-24 14:44:07, Byungchul Park wrote: >> > Changes from v2: >> > 1. Change the condition to stop cache_trim_mode. >> > >> > From - Stop it if it's at high scan priorities, 0 or 1. >> > To - Stop it if it's at high scan priorities, 0 or 1, and >> > the mode didn't work in the previous turn. >> > >> > (feedbacked by Huang Ying) >> > >> > 2. Change the test result in the commit message after testing >> > with the new logic. >> > >> > Changes from v1: >> > 1. Add a comment describing why this change is necessary in code >> > and rewrite the commit message with how to reproduce and what >> > the result is using vmstat. (feedbacked by Andrew Morton and >> > Yu Zhao) >> > 2. Change the condition to avoid cache_trim_mode from >> > 'sc->priority != 1' to 'sc->priority > 1' to reflect cases >> > where the priority goes to zero all the way. (feedbacked by >> > Yu Zhao) >> > >> > --->8--- >> > >From 05846e34bf02ac9b3e254324dc2d7afd97a025d9 Mon Sep 17 00:00:00 2001 >> > From: Byungchul Park <byungchul@sk.com> >> > Date: Fri, 23 Feb 2024 13:47:16 +0900 >> > Subject: [PATCH v3] mm, vmscan: do not turn on cache_trim_mode if it doesn't work >> > >> > With cache_trim_mode on, reclaim logic doesn't bother reclaiming anon >> > pages. However, it should be more careful to turn on the mode because >> > it's going to prevent anon pages from being reclaimed even if there are >> > a huge number of anon pages that are cold and should be reclaimed. Even >> > worse, that leads kswapd_failures to reach MAX_RECLAIM_RETRIES and >> > stopping kswapd from functioning until direct reclaim eventually works >> > to resume kswapd. >> > >> > So do not turn on cache_trim_mode if the mode doesn't work, especially >> > while the sytem is struggling against reclaim. >> > >> > The problematic behavior can be reproduced by: >> > >> > CONFIG_NUMA_BALANCING enabled >> > sysctl_numa_balancing_mode set to NUMA_BALANCING_MEMORY_TIERING >> > numa node0 (8GB local memory, 16 CPUs) >> > numa node1 (8GB slow tier memory, no CPUs) >> > >> > Sequence: >> > >> > 1) echo 3 > /proc/sys/vm/drop_caches >> > 2) To emulate the system with full of cold memory in local DRAM, run >> > the following dummy program and never touch the region: >> > >> > mmap(0, 8 * 1024 * 1024 * 1024, PROT_READ | PROT_WRITE, >> > MAP_ANONYMOUS | MAP_PRIVATE | MAP_POPULATE, -1, 0); >> > >> > 3) Run any memory intensive work e.g. XSBench. >> > 4) Check if numa balancing is working e.i. promotion/demotion. >> > 5) Iterate 1) ~ 4) until numa balancing stops. >> > >> > With this, you could see that promotion/demotion are not working because >> > kswapd has stopped due to ->kswapd_failures >= MAX_RECLAIM_RETRIES. >> > >> > Interesting vmstat delta's differences between before and after are like: >> > >> > +-----------------------+-------------------------------+ >> > | interesting vmstat | before | after | >> > +-----------------------+-------------------------------+ >> > | nr_inactive_anon | 321935 | 1636737 | >> > | nr_active_anon | 1780700 | 465913 | >> > | nr_inactive_file | 30425 | 35711 | >> > | nr_active_file | 14961 | 8698 | >> > | pgpromote_success | 356 | 1267785 | >> > | pgpromote_candidate | 21953245 | 1745631 | >> > | pgactivate | 1844523 | 3309867 | >> > | pgdeactivate | 50634 | 1545041 | >> > | pgfault | 31100294 | 6411036 | >> > | pgdemote_kswapd | 30856 | 2267467 | >> > | pgscan_kswapd | 1861981 | 7729231 | >> > | pgscan_anon | 1822930 | 7667544 | >> > | pgscan_file | 39051 | 61687 | >> > | pgsteal_anon | 386 | 2227217 | >> > | pgsteal_file | 30470 | 40250 | >> > | pageoutrun | 30 | 457 | >> > | numa_hint_faults | 27418279 | 2752289 | >> > | numa_pages_migrated | 356 | 1267785 | >> > +-----------------------+-------------------------------+ >> > >> > Signed-off-by: Byungchul Park <byungchul@sk.com> >> > --- >> > mm/vmscan.c | 24 +++++++++++++++++++----- >> > 1 file changed, 19 insertions(+), 5 deletions(-) >> > >> > diff --git a/mm/vmscan.c b/mm/vmscan.c >> > index bba207f41b14..f7312d831fed 100644 >> > --- a/mm/vmscan.c >> > +++ b/mm/vmscan.c >> > @@ -127,6 +127,9 @@ struct scan_control { >> > /* One of the zones is ready for compaction */ >> > unsigned int compaction_ready:1; >> > >> > + /* If the last try was reclaimable */ >> > + unsigned int reclaimable:1; >> > + >> > /* There is easily reclaimable cold cache in the current node */ >> > unsigned int cache_trim_mode:1; >> > >> > @@ -2266,9 +2269,14 @@ static void prepare_scan_control(pg_data_t *pgdat, struct scan_control *sc) >> > * If we have plenty of inactive file pages that aren't >> > * thrashing, try to reclaim those first before touching >> > * anonymous pages. >> > + * >> > + * It doesn't make sense to keep cache_trim_mode on if the mode >> > + * is not working while struggling against reclaim. So do not >> > + * turn it on if so. Note the highest priority of kswapd is 1. >> > */ >> > file = lruvec_page_state(target_lruvec, NR_INACTIVE_FILE); >> > - if (file >> sc->priority && !(sc->may_deactivate & DEACTIVATE_FILE)) >> > + if (file >> sc->priority && !(sc->may_deactivate & DEACTIVATE_FILE) && >> > + !(sc->cache_trim_mode && !sc->reclaimable && sc->priority <= 1)) >> > sc->cache_trim_mode = 1; >> > else >> > sc->cache_trim_mode = 0; > > The overall goal makes sense to me. > > file >> priority is just a heuristic that there are enough potential > candidate pages, not a guarantee that any forward progress will > happen. So it makes sense to retry without before failing. > > The way you wrote this conditional kind of hurts my head, > though. Please don't write negations of complex terms like this. > > It expands to this: > > !sc->cache_trim_mode || sc->reclaimable || sc->priority > 1 > > which I'm not sure makes sense. Surely it should be something like > > !sc->cache_trim_mode && sc->reclaimable && sc->priority > 1 > > instead? > > Also > > if (!sc->cache_trim_mode) > sc->cache_trim_mode = 1 > else > sc->cache_trim_mode = 0 > > will toggle on every loop. So if direct reclaim runs through a > zonelist, it'll cache trim every other numa node...? > >> > @@ -5862,7 +5870,6 @@ static void shrink_node(pg_data_t *pgdat, struct scan_control *sc) >> > { >> > unsigned long nr_reclaimed, nr_scanned, nr_node_reclaimed; >> > struct lruvec *target_lruvec; >> > - bool reclaimable = false; >> > >> > if (lru_gen_enabled() && root_reclaim(sc)) { >> > lru_gen_shrink_node(pgdat, sc); >> > @@ -5877,6 +5884,14 @@ static void shrink_node(pg_data_t *pgdat, struct scan_control *sc) >> > nr_reclaimed = sc->nr_reclaimed; >> > nr_scanned = sc->nr_scanned; >> > >> > + /* >> > + * Reset to the default values at the start. >> > + */ >> > + if (sc->priority == DEF_PRIORITY) { >> > + sc->reclaimable = 1; >> > + sc->cache_trim_mode = 0; >> > + } >> > + >> > prepare_scan_control(pgdat, sc); >> > >> > shrink_node_memcgs(pgdat, sc); >> > @@ -5890,8 +5905,7 @@ static void shrink_node(pg_data_t *pgdat, struct scan_control *sc) >> > vmpressure(sc->gfp_mask, sc->target_mem_cgroup, true, >> > sc->nr_scanned - nr_scanned, nr_node_reclaimed); >> > >> > - if (nr_node_reclaimed) >> > - reclaimable = true; >> > + sc->reclaimable = !!nr_node_reclaimed; > > The scope of this doesn't quite make sense. If direct reclaim scans > multiple nodes, reclaim failure on the first node would disable cache > trim mode on the second node, which is totally unrelated. > > I think it needs separate paths for direct reclaim and kswapd. For > direct reclaim, the retry should be before these similar retry catches > in do_try_to_free_pages(), after all zones have been considered: > > /* > * We make inactive:active ratio decisions based on the node's > * composition of memory, but a restrictive reclaim_idx or a > * memory.low cgroup setting can exempt large amounts of > * memory from reclaim. Neither of which are very common, so > * instead of doing costly eligibility calculations of the > * entire cgroup subtree up front, we assume the estimates are > * good, and retry with forcible deactivation if that fails. > */ > if (sc->skipped_deactivate) { > sc->priority = initial_priority; > sc->force_deactivate = 1; > sc->skipped_deactivate = 0; > goto retry; > } > > /* Untapped cgroup reserves? Don't OOM, retry. */ > if (sc->memcg_low_skipped) { > sc->priority = initial_priority; > sc->force_deactivate = 0; > sc->memcg_low_reclaim = 1; > sc->memcg_low_skipped = 0; > goto retry; > } In get_scan_count(), we have if (!sc->priority && swappiness) { scan_balance = SCAN_EQUAL; goto out; } So, we don't really need the heuristics in direct reclaim path. So, one choice is to constrain this in kswapd reclaim only. -- Best Regards, Huang, Ying > and for kswapd it looks like it should be in balance_pgdat(), after > the priority loop, before increasing kswapd_failures. > > Instead of sc->reclaimable, which is very broad, it would be better to > call it sc->may_cache_trim_mode. This is in line with a bunch of other > such mechanisms in scan_control.
On Thu, Feb 29, 2024 at 02:19:07PM +0800, Huang, Ying wrote: > Byungchul Park <byungchul@sk.com> writes: > > > On Wed, Feb 28, 2024 at 05:36:01PM -0500, Johannes Weiner wrote: > >> On Mon, Feb 26, 2024 at 02:06:30PM +0100, Michal Hocko wrote: > >> > [CC Mel, Vlastimil and Johannes for awareness] > >> > > >> > On Fri 23-02-24 14:44:07, Byungchul Park wrote: > >> > > Changes from v2: > >> > > 1. Change the condition to stop cache_trim_mode. > >> > > > >> > > From - Stop it if it's at high scan priorities, 0 or 1. > >> > > To - Stop it if it's at high scan priorities, 0 or 1, and > >> > > the mode didn't work in the previous turn. > >> > > > >> > > (feedbacked by Huang Ying) > >> > > > >> > > 2. Change the test result in the commit message after testing > >> > > with the new logic. > >> > > > >> > > Changes from v1: > >> > > 1. Add a comment describing why this change is necessary in code > >> > > and rewrite the commit message with how to reproduce and what > >> > > the result is using vmstat. (feedbacked by Andrew Morton and > >> > > Yu Zhao) > >> > > 2. Change the condition to avoid cache_trim_mode from > >> > > 'sc->priority != 1' to 'sc->priority > 1' to reflect cases > >> > > where the priority goes to zero all the way. (feedbacked by > >> > > Yu Zhao) > >> > > > >> > > --->8--- > >> > > >From 05846e34bf02ac9b3e254324dc2d7afd97a025d9 Mon Sep 17 00:00:00 2001 > >> > > From: Byungchul Park <byungchul@sk.com> > >> > > Date: Fri, 23 Feb 2024 13:47:16 +0900 > >> > > Subject: [PATCH v3] mm, vmscan: do not turn on cache_trim_mode if it doesn't work > >> > > > >> > > With cache_trim_mode on, reclaim logic doesn't bother reclaiming anon > >> > > pages. However, it should be more careful to turn on the mode because > >> > > it's going to prevent anon pages from being reclaimed even if there are > >> > > a huge number of anon pages that are cold and should be reclaimed. Even > >> > > worse, that leads kswapd_failures to reach MAX_RECLAIM_RETRIES and > >> > > stopping kswapd from functioning until direct reclaim eventually works > >> > > to resume kswapd. > >> > > > >> > > So do not turn on cache_trim_mode if the mode doesn't work, especially > >> > > while the sytem is struggling against reclaim. > >> > > > >> > > The problematic behavior can be reproduced by: > >> > > > >> > > CONFIG_NUMA_BALANCING enabled > >> > > sysctl_numa_balancing_mode set to NUMA_BALANCING_MEMORY_TIERING > >> > > numa node0 (8GB local memory, 16 CPUs) > >> > > numa node1 (8GB slow tier memory, no CPUs) > >> > > > >> > > Sequence: > >> > > > >> > > 1) echo 3 > /proc/sys/vm/drop_caches > >> > > 2) To emulate the system with full of cold memory in local DRAM, run > >> > > the following dummy program and never touch the region: > >> > > > >> > > mmap(0, 8 * 1024 * 1024 * 1024, PROT_READ | PROT_WRITE, > >> > > MAP_ANONYMOUS | MAP_PRIVATE | MAP_POPULATE, -1, 0); > >> > > > >> > > 3) Run any memory intensive work e.g. XSBench. > >> > > 4) Check if numa balancing is working e.i. promotion/demotion. > >> > > 5) Iterate 1) ~ 4) until numa balancing stops. > >> > > > >> > > With this, you could see that promotion/demotion are not working because > >> > > kswapd has stopped due to ->kswapd_failures >= MAX_RECLAIM_RETRIES. > >> > > > >> > > Interesting vmstat delta's differences between before and after are like: > >> > > > >> > > +-----------------------+-------------------------------+ > >> > > | interesting vmstat | before | after | > >> > > +-----------------------+-------------------------------+ > >> > > | nr_inactive_anon | 321935 | 1636737 | > >> > > | nr_active_anon | 1780700 | 465913 | > >> > > | nr_inactive_file | 30425 | 35711 | > >> > > | nr_active_file | 14961 | 8698 | > >> > > | pgpromote_success | 356 | 1267785 | > >> > > | pgpromote_candidate | 21953245 | 1745631 | > >> > > | pgactivate | 1844523 | 3309867 | > >> > > | pgdeactivate | 50634 | 1545041 | > >> > > | pgfault | 31100294 | 6411036 | > >> > > | pgdemote_kswapd | 30856 | 2267467 | > >> > > | pgscan_kswapd | 1861981 | 7729231 | > >> > > | pgscan_anon | 1822930 | 7667544 | > >> > > | pgscan_file | 39051 | 61687 | > >> > > | pgsteal_anon | 386 | 2227217 | > >> > > | pgsteal_file | 30470 | 40250 | > >> > > | pageoutrun | 30 | 457 | > >> > > | numa_hint_faults | 27418279 | 2752289 | > >> > > | numa_pages_migrated | 356 | 1267785 | > >> > > +-----------------------+-------------------------------+ > >> > > > >> > > Signed-off-by: Byungchul Park <byungchul@sk.com> > >> > > --- > >> > > mm/vmscan.c | 24 +++++++++++++++++++----- > >> > > 1 file changed, 19 insertions(+), 5 deletions(-) > >> > > > >> > > diff --git a/mm/vmscan.c b/mm/vmscan.c > >> > > index bba207f41b14..f7312d831fed 100644 > >> > > --- a/mm/vmscan.c > >> > > +++ b/mm/vmscan.c > >> > > @@ -127,6 +127,9 @@ struct scan_control { > >> > > /* One of the zones is ready for compaction */ > >> > > unsigned int compaction_ready:1; > >> > > > >> > > + /* If the last try was reclaimable */ > >> > > + unsigned int reclaimable:1; > >> > > + > >> > > /* There is easily reclaimable cold cache in the current node */ > >> > > unsigned int cache_trim_mode:1; > >> > > > >> > > @@ -2266,9 +2269,14 @@ static void prepare_scan_control(pg_data_t *pgdat, struct scan_control *sc) > >> > > * If we have plenty of inactive file pages that aren't > >> > > * thrashing, try to reclaim those first before touching > >> > > * anonymous pages. > >> > > + * > >> > > + * It doesn't make sense to keep cache_trim_mode on if the mode > >> > > + * is not working while struggling against reclaim. So do not > >> > > + * turn it on if so. Note the highest priority of kswapd is 1. > >> > > */ > >> > > file = lruvec_page_state(target_lruvec, NR_INACTIVE_FILE); > >> > > - if (file >> sc->priority && !(sc->may_deactivate & DEACTIVATE_FILE)) > >> > > + if (file >> sc->priority && !(sc->may_deactivate & DEACTIVATE_FILE) && > >> > > + !(sc->cache_trim_mode && !sc->reclaimable && sc->priority <= 1)) > >> > > sc->cache_trim_mode = 1; > >> > > else > >> > > sc->cache_trim_mode = 0; > >> > >> The overall goal makes sense to me. > >> > >> file >> priority is just a heuristic that there are enough potential > >> candidate pages, not a guarantee that any forward progress will > >> happen. So it makes sense to retry without before failing. > >> > >> The way you wrote this conditional kind of hurts my head, > >> though. Please don't write negations of complex terms like this. > > > > Okay. I won't. > > > >> It expands to this: > >> > >> !sc->cache_trim_mode || sc->reclaimable || sc->priority > 1 > >> > >> which I'm not sure makes sense. Surely it should be something like > >> > >> !sc->cache_trim_mode && sc->reclaimable && sc->priority > 1 > > > > It's a totally different condition as you know. > > > >> instead? > >> > >> Also > >> > >> if (!sc->cache_trim_mode) > >> sc->cache_trim_mode = 1 > >> else > >> sc->cache_trim_mode = 0 > >> > >> will toggle on every loop. So if direct reclaim runs through a > >> zonelist, it'll cache trim every other numa node...? > > > > No way to toggle on every loop. > > > > What I tried was that: > > > > 1. Don't turn it on again if it didn't work in the previous try. > > 2. Let it go as it was if the priority is not that high though, > > to keep the code as conservatively as possible. > > > > So again, the following condition is needed. > > > > (the original condition) && > > (!sc->cache_trim_mode || sc->reclaimable || sc->priority > 1) > > > >> > > @@ -5862,7 +5870,6 @@ static void shrink_node(pg_data_t *pgdat, struct scan_control *sc) > >> > > { > >> > > unsigned long nr_reclaimed, nr_scanned, nr_node_reclaimed; > >> > > struct lruvec *target_lruvec; > >> > > - bool reclaimable = false; > >> > > > >> > > if (lru_gen_enabled() && root_reclaim(sc)) { > >> > > lru_gen_shrink_node(pgdat, sc); > >> > > @@ -5877,6 +5884,14 @@ static void shrink_node(pg_data_t *pgdat, struct scan_control *sc) > >> > > nr_reclaimed = sc->nr_reclaimed; > >> > > nr_scanned = sc->nr_scanned; > >> > > > >> > > + /* > >> > > + * Reset to the default values at the start. > >> > > + */ > >> > > + if (sc->priority == DEF_PRIORITY) { > > > > This might need to be fixed if reclaim happens to start with other than > > DEF_PRIORITY. For now, reclaim always starts with the priority though. > > > >> > > + sc->reclaimable = 1; > >> > > + sc->cache_trim_mode = 0; > >> > > + } > > > > For each shrink_node(), initialize all the variable at the start. > > > >> > > + > >> > > prepare_scan_control(pgdat, sc); > >> > > > >> > > shrink_node_memcgs(pgdat, sc); > >> > > @@ -5890,8 +5905,7 @@ static void shrink_node(pg_data_t *pgdat, struct scan_control *sc) > >> > > vmpressure(sc->gfp_mask, sc->target_mem_cgroup, true, > >> > > sc->nr_scanned - nr_scanned, nr_node_reclaimed); > >> > > > >> > > - if (nr_node_reclaimed) > >> > > - reclaimable = true; > >> > > + sc->reclaimable = !!nr_node_reclaimed; > >> > >> The scope of this doesn't quite make sense. If direct reclaim scans > >> multiple nodes, reclaim failure on the first node would disable cache > >> trim mode on the second node, which is totally unrelated. > > > > As I mentioned, reclaim for every node would start with an initialized > > value because *each node is totally unrelated to another*. > > No. Please take a look at do_try_to_free_pages(), for each priority, it > will iterate every node. But fortunately, we may not need this > heuristics for direct reclaiming. Indeed. You guys are right. Sorry for noise. Let me respin. Thanks. Byungchul > -- > Best Regards, > Huang, Ying
On Thu, Feb 29, 2024 at 02:21:15PM +0800, Huang, Ying wrote: > Johannes Weiner <hannes@cmpxchg.org> writes: > > > On Mon, Feb 26, 2024 at 02:06:30PM +0100, Michal Hocko wrote: > >> [CC Mel, Vlastimil and Johannes for awareness] > >> > >> On Fri 23-02-24 14:44:07, Byungchul Park wrote: > >> > Changes from v2: > >> > 1. Change the condition to stop cache_trim_mode. > >> > > >> > From - Stop it if it's at high scan priorities, 0 or 1. > >> > To - Stop it if it's at high scan priorities, 0 or 1, and > >> > the mode didn't work in the previous turn. > >> > > >> > (feedbacked by Huang Ying) > >> > > >> > 2. Change the test result in the commit message after testing > >> > with the new logic. > >> > > >> > Changes from v1: > >> > 1. Add a comment describing why this change is necessary in code > >> > and rewrite the commit message with how to reproduce and what > >> > the result is using vmstat. (feedbacked by Andrew Morton and > >> > Yu Zhao) > >> > 2. Change the condition to avoid cache_trim_mode from > >> > 'sc->priority != 1' to 'sc->priority > 1' to reflect cases > >> > where the priority goes to zero all the way. (feedbacked by > >> > Yu Zhao) > >> > > >> > --->8--- > >> > >From 05846e34bf02ac9b3e254324dc2d7afd97a025d9 Mon Sep 17 00:00:00 2001 > >> > From: Byungchul Park <byungchul@sk.com> > >> > Date: Fri, 23 Feb 2024 13:47:16 +0900 > >> > Subject: [PATCH v3] mm, vmscan: do not turn on cache_trim_mode if it doesn't work > >> > > >> > With cache_trim_mode on, reclaim logic doesn't bother reclaiming anon > >> > pages. However, it should be more careful to turn on the mode because > >> > it's going to prevent anon pages from being reclaimed even if there are > >> > a huge number of anon pages that are cold and should be reclaimed. Even > >> > worse, that leads kswapd_failures to reach MAX_RECLAIM_RETRIES and > >> > stopping kswapd from functioning until direct reclaim eventually works > >> > to resume kswapd. > >> > > >> > So do not turn on cache_trim_mode if the mode doesn't work, especially > >> > while the sytem is struggling against reclaim. > >> > > >> > The problematic behavior can be reproduced by: > >> > > >> > CONFIG_NUMA_BALANCING enabled > >> > sysctl_numa_balancing_mode set to NUMA_BALANCING_MEMORY_TIERING > >> > numa node0 (8GB local memory, 16 CPUs) > >> > numa node1 (8GB slow tier memory, no CPUs) > >> > > >> > Sequence: > >> > > >> > 1) echo 3 > /proc/sys/vm/drop_caches > >> > 2) To emulate the system with full of cold memory in local DRAM, run > >> > the following dummy program and never touch the region: > >> > > >> > mmap(0, 8 * 1024 * 1024 * 1024, PROT_READ | PROT_WRITE, > >> > MAP_ANONYMOUS | MAP_PRIVATE | MAP_POPULATE, -1, 0); > >> > > >> > 3) Run any memory intensive work e.g. XSBench. > >> > 4) Check if numa balancing is working e.i. promotion/demotion. > >> > 5) Iterate 1) ~ 4) until numa balancing stops. > >> > > >> > With this, you could see that promotion/demotion are not working because > >> > kswapd has stopped due to ->kswapd_failures >= MAX_RECLAIM_RETRIES. > >> > > >> > Interesting vmstat delta's differences between before and after are like: > >> > > >> > +-----------------------+-------------------------------+ > >> > | interesting vmstat | before | after | > >> > +-----------------------+-------------------------------+ > >> > | nr_inactive_anon | 321935 | 1636737 | > >> > | nr_active_anon | 1780700 | 465913 | > >> > | nr_inactive_file | 30425 | 35711 | > >> > | nr_active_file | 14961 | 8698 | > >> > | pgpromote_success | 356 | 1267785 | > >> > | pgpromote_candidate | 21953245 | 1745631 | > >> > | pgactivate | 1844523 | 3309867 | > >> > | pgdeactivate | 50634 | 1545041 | > >> > | pgfault | 31100294 | 6411036 | > >> > | pgdemote_kswapd | 30856 | 2267467 | > >> > | pgscan_kswapd | 1861981 | 7729231 | > >> > | pgscan_anon | 1822930 | 7667544 | > >> > | pgscan_file | 39051 | 61687 | > >> > | pgsteal_anon | 386 | 2227217 | > >> > | pgsteal_file | 30470 | 40250 | > >> > | pageoutrun | 30 | 457 | > >> > | numa_hint_faults | 27418279 | 2752289 | > >> > | numa_pages_migrated | 356 | 1267785 | > >> > +-----------------------+-------------------------------+ > >> > > >> > Signed-off-by: Byungchul Park <byungchul@sk.com> > >> > --- > >> > mm/vmscan.c | 24 +++++++++++++++++++----- > >> > 1 file changed, 19 insertions(+), 5 deletions(-) > >> > > >> > diff --git a/mm/vmscan.c b/mm/vmscan.c > >> > index bba207f41b14..f7312d831fed 100644 > >> > --- a/mm/vmscan.c > >> > +++ b/mm/vmscan.c > >> > @@ -127,6 +127,9 @@ struct scan_control { > >> > /* One of the zones is ready for compaction */ > >> > unsigned int compaction_ready:1; > >> > > >> > + /* If the last try was reclaimable */ > >> > + unsigned int reclaimable:1; > >> > + > >> > /* There is easily reclaimable cold cache in the current node */ > >> > unsigned int cache_trim_mode:1; > >> > > >> > @@ -2266,9 +2269,14 @@ static void prepare_scan_control(pg_data_t *pgdat, struct scan_control *sc) > >> > * If we have plenty of inactive file pages that aren't > >> > * thrashing, try to reclaim those first before touching > >> > * anonymous pages. > >> > + * > >> > + * It doesn't make sense to keep cache_trim_mode on if the mode > >> > + * is not working while struggling against reclaim. So do not > >> > + * turn it on if so. Note the highest priority of kswapd is 1. > >> > */ > >> > file = lruvec_page_state(target_lruvec, NR_INACTIVE_FILE); > >> > - if (file >> sc->priority && !(sc->may_deactivate & DEACTIVATE_FILE)) > >> > + if (file >> sc->priority && !(sc->may_deactivate & DEACTIVATE_FILE) && > >> > + !(sc->cache_trim_mode && !sc->reclaimable && sc->priority <= 1)) > >> > sc->cache_trim_mode = 1; > >> > else > >> > sc->cache_trim_mode = 0; > > > > The overall goal makes sense to me. > > > > file >> priority is just a heuristic that there are enough potential > > candidate pages, not a guarantee that any forward progress will > > happen. So it makes sense to retry without before failing. > > > > The way you wrote this conditional kind of hurts my head, > > though. Please don't write negations of complex terms like this. > > > > It expands to this: > > > > !sc->cache_trim_mode || sc->reclaimable || sc->priority > 1 > > > > which I'm not sure makes sense. Surely it should be something like > > > > !sc->cache_trim_mode && sc->reclaimable && sc->priority > 1 > > > > instead? > > > > Also > > > > if (!sc->cache_trim_mode) > > sc->cache_trim_mode = 1 > > else > > sc->cache_trim_mode = 0 > > > > will toggle on every loop. So if direct reclaim runs through a > > zonelist, it'll cache trim every other numa node...? > > > >> > @@ -5862,7 +5870,6 @@ static void shrink_node(pg_data_t *pgdat, struct scan_control *sc) > >> > { > >> > unsigned long nr_reclaimed, nr_scanned, nr_node_reclaimed; > >> > struct lruvec *target_lruvec; > >> > - bool reclaimable = false; > >> > > >> > if (lru_gen_enabled() && root_reclaim(sc)) { > >> > lru_gen_shrink_node(pgdat, sc); > >> > @@ -5877,6 +5884,14 @@ static void shrink_node(pg_data_t *pgdat, struct scan_control *sc) > >> > nr_reclaimed = sc->nr_reclaimed; > >> > nr_scanned = sc->nr_scanned; > >> > > >> > + /* > >> > + * Reset to the default values at the start. > >> > + */ > >> > + if (sc->priority == DEF_PRIORITY) { > >> > + sc->reclaimable = 1; > >> > + sc->cache_trim_mode = 0; > >> > + } > >> > + > >> > prepare_scan_control(pgdat, sc); > >> > > >> > shrink_node_memcgs(pgdat, sc); > >> > @@ -5890,8 +5905,7 @@ static void shrink_node(pg_data_t *pgdat, struct scan_control *sc) > >> > vmpressure(sc->gfp_mask, sc->target_mem_cgroup, true, > >> > sc->nr_scanned - nr_scanned, nr_node_reclaimed); > >> > > >> > - if (nr_node_reclaimed) > >> > - reclaimable = true; > >> > + sc->reclaimable = !!nr_node_reclaimed; > > > > The scope of this doesn't quite make sense. If direct reclaim scans > > multiple nodes, reclaim failure on the first node would disable cache > > trim mode on the second node, which is totally unrelated. > > > > I think it needs separate paths for direct reclaim and kswapd. For > > direct reclaim, the retry should be before these similar retry catches > > in do_try_to_free_pages(), after all zones have been considered: > > > > /* > > * We make inactive:active ratio decisions based on the node's > > * composition of memory, but a restrictive reclaim_idx or a > > * memory.low cgroup setting can exempt large amounts of > > * memory from reclaim. Neither of which are very common, so > > * instead of doing costly eligibility calculations of the > > * entire cgroup subtree up front, we assume the estimates are > > * good, and retry with forcible deactivation if that fails. > > */ > > if (sc->skipped_deactivate) { > > sc->priority = initial_priority; > > sc->force_deactivate = 1; > > sc->skipped_deactivate = 0; > > goto retry; > > } > > > > /* Untapped cgroup reserves? Don't OOM, retry. */ > > if (sc->memcg_low_skipped) { > > sc->priority = initial_priority; > > sc->force_deactivate = 0; > > sc->memcg_low_reclaim = 1; > > sc->memcg_low_skipped = 0; > > goto retry; > > } > > In get_scan_count(), we have > > if (!sc->priority && swappiness) { > scan_balance = SCAN_EQUAL; > goto out; > } Even though this can mitigate the issue for direct reclaim, it's still suffering from the problem while direct reclaim goes from DEF_PRIORITY to 1. What we need is not a mitigation but making things right. Thoughts? Byungchul > So, we don't really need the heuristics in direct reclaim path. So, one > choice is to constrain this in kswapd reclaim only. > > -- > Best Regards, > Huang, Ying > > > > and for kswapd it looks like it should be in balance_pgdat(), after > > the priority loop, before increasing kswapd_failures. > > > > Instead of sc->reclaimable, which is very broad, it would be better to > > call it sc->may_cache_trim_mode. This is in line with a bunch of other > > such mechanisms in scan_control.
Byungchul Park <byungchul@sk.com> writes: > On Thu, Feb 29, 2024 at 02:21:15PM +0800, Huang, Ying wrote: >> Johannes Weiner <hannes@cmpxchg.org> writes: >> >> > On Mon, Feb 26, 2024 at 02:06:30PM +0100, Michal Hocko wrote: >> >> [CC Mel, Vlastimil and Johannes for awareness] >> >> >> >> On Fri 23-02-24 14:44:07, Byungchul Park wrote: >> >> > Changes from v2: >> >> > 1. Change the condition to stop cache_trim_mode. >> >> > >> >> > From - Stop it if it's at high scan priorities, 0 or 1. >> >> > To - Stop it if it's at high scan priorities, 0 or 1, and >> >> > the mode didn't work in the previous turn. >> >> > >> >> > (feedbacked by Huang Ying) >> >> > >> >> > 2. Change the test result in the commit message after testing >> >> > with the new logic. >> >> > >> >> > Changes from v1: >> >> > 1. Add a comment describing why this change is necessary in code >> >> > and rewrite the commit message with how to reproduce and what >> >> > the result is using vmstat. (feedbacked by Andrew Morton and >> >> > Yu Zhao) >> >> > 2. Change the condition to avoid cache_trim_mode from >> >> > 'sc->priority != 1' to 'sc->priority > 1' to reflect cases >> >> > where the priority goes to zero all the way. (feedbacked by >> >> > Yu Zhao) >> >> > >> >> > --->8--- >> >> > >From 05846e34bf02ac9b3e254324dc2d7afd97a025d9 Mon Sep 17 00:00:00 2001 >> >> > From: Byungchul Park <byungchul@sk.com> >> >> > Date: Fri, 23 Feb 2024 13:47:16 +0900 >> >> > Subject: [PATCH v3] mm, vmscan: do not turn on cache_trim_mode if it doesn't work >> >> > >> >> > With cache_trim_mode on, reclaim logic doesn't bother reclaiming anon >> >> > pages. However, it should be more careful to turn on the mode because >> >> > it's going to prevent anon pages from being reclaimed even if there are >> >> > a huge number of anon pages that are cold and should be reclaimed. Even >> >> > worse, that leads kswapd_failures to reach MAX_RECLAIM_RETRIES and >> >> > stopping kswapd from functioning until direct reclaim eventually works >> >> > to resume kswapd. >> >> > >> >> > So do not turn on cache_trim_mode if the mode doesn't work, especially >> >> > while the sytem is struggling against reclaim. >> >> > >> >> > The problematic behavior can be reproduced by: >> >> > >> >> > CONFIG_NUMA_BALANCING enabled >> >> > sysctl_numa_balancing_mode set to NUMA_BALANCING_MEMORY_TIERING >> >> > numa node0 (8GB local memory, 16 CPUs) >> >> > numa node1 (8GB slow tier memory, no CPUs) >> >> > >> >> > Sequence: >> >> > >> >> > 1) echo 3 > /proc/sys/vm/drop_caches >> >> > 2) To emulate the system with full of cold memory in local DRAM, run >> >> > the following dummy program and never touch the region: >> >> > >> >> > mmap(0, 8 * 1024 * 1024 * 1024, PROT_READ | PROT_WRITE, >> >> > MAP_ANONYMOUS | MAP_PRIVATE | MAP_POPULATE, -1, 0); >> >> > >> >> > 3) Run any memory intensive work e.g. XSBench. >> >> > 4) Check if numa balancing is working e.i. promotion/demotion. >> >> > 5) Iterate 1) ~ 4) until numa balancing stops. >> >> > >> >> > With this, you could see that promotion/demotion are not working because >> >> > kswapd has stopped due to ->kswapd_failures >= MAX_RECLAIM_RETRIES. >> >> > >> >> > Interesting vmstat delta's differences between before and after are like: >> >> > >> >> > +-----------------------+-------------------------------+ >> >> > | interesting vmstat | before | after | >> >> > +-----------------------+-------------------------------+ >> >> > | nr_inactive_anon | 321935 | 1636737 | >> >> > | nr_active_anon | 1780700 | 465913 | >> >> > | nr_inactive_file | 30425 | 35711 | >> >> > | nr_active_file | 14961 | 8698 | >> >> > | pgpromote_success | 356 | 1267785 | >> >> > | pgpromote_candidate | 21953245 | 1745631 | >> >> > | pgactivate | 1844523 | 3309867 | >> >> > | pgdeactivate | 50634 | 1545041 | >> >> > | pgfault | 31100294 | 6411036 | >> >> > | pgdemote_kswapd | 30856 | 2267467 | >> >> > | pgscan_kswapd | 1861981 | 7729231 | >> >> > | pgscan_anon | 1822930 | 7667544 | >> >> > | pgscan_file | 39051 | 61687 | >> >> > | pgsteal_anon | 386 | 2227217 | >> >> > | pgsteal_file | 30470 | 40250 | >> >> > | pageoutrun | 30 | 457 | >> >> > | numa_hint_faults | 27418279 | 2752289 | >> >> > | numa_pages_migrated | 356 | 1267785 | >> >> > +-----------------------+-------------------------------+ >> >> > >> >> > Signed-off-by: Byungchul Park <byungchul@sk.com> >> >> > --- >> >> > mm/vmscan.c | 24 +++++++++++++++++++----- >> >> > 1 file changed, 19 insertions(+), 5 deletions(-) >> >> > >> >> > diff --git a/mm/vmscan.c b/mm/vmscan.c >> >> > index bba207f41b14..f7312d831fed 100644 >> >> > --- a/mm/vmscan.c >> >> > +++ b/mm/vmscan.c >> >> > @@ -127,6 +127,9 @@ struct scan_control { >> >> > /* One of the zones is ready for compaction */ >> >> > unsigned int compaction_ready:1; >> >> > >> >> > + /* If the last try was reclaimable */ >> >> > + unsigned int reclaimable:1; >> >> > + >> >> > /* There is easily reclaimable cold cache in the current node */ >> >> > unsigned int cache_trim_mode:1; >> >> > >> >> > @@ -2266,9 +2269,14 @@ static void prepare_scan_control(pg_data_t *pgdat, struct scan_control *sc) >> >> > * If we have plenty of inactive file pages that aren't >> >> > * thrashing, try to reclaim those first before touching >> >> > * anonymous pages. >> >> > + * >> >> > + * It doesn't make sense to keep cache_trim_mode on if the mode >> >> > + * is not working while struggling against reclaim. So do not >> >> > + * turn it on if so. Note the highest priority of kswapd is 1. >> >> > */ >> >> > file = lruvec_page_state(target_lruvec, NR_INACTIVE_FILE); >> >> > - if (file >> sc->priority && !(sc->may_deactivate & DEACTIVATE_FILE)) >> >> > + if (file >> sc->priority && !(sc->may_deactivate & DEACTIVATE_FILE) && >> >> > + !(sc->cache_trim_mode && !sc->reclaimable && sc->priority <= 1)) >> >> > sc->cache_trim_mode = 1; >> >> > else >> >> > sc->cache_trim_mode = 0; >> > >> > The overall goal makes sense to me. >> > >> > file >> priority is just a heuristic that there are enough potential >> > candidate pages, not a guarantee that any forward progress will >> > happen. So it makes sense to retry without before failing. >> > >> > The way you wrote this conditional kind of hurts my head, >> > though. Please don't write negations of complex terms like this. >> > >> > It expands to this: >> > >> > !sc->cache_trim_mode || sc->reclaimable || sc->priority > 1 >> > >> > which I'm not sure makes sense. Surely it should be something like >> > >> > !sc->cache_trim_mode && sc->reclaimable && sc->priority > 1 >> > >> > instead? >> > >> > Also >> > >> > if (!sc->cache_trim_mode) >> > sc->cache_trim_mode = 1 >> > else >> > sc->cache_trim_mode = 0 >> > >> > will toggle on every loop. So if direct reclaim runs through a >> > zonelist, it'll cache trim every other numa node...? >> > >> >> > @@ -5862,7 +5870,6 @@ static void shrink_node(pg_data_t *pgdat, struct scan_control *sc) >> >> > { >> >> > unsigned long nr_reclaimed, nr_scanned, nr_node_reclaimed; >> >> > struct lruvec *target_lruvec; >> >> > - bool reclaimable = false; >> >> > >> >> > if (lru_gen_enabled() && root_reclaim(sc)) { >> >> > lru_gen_shrink_node(pgdat, sc); >> >> > @@ -5877,6 +5884,14 @@ static void shrink_node(pg_data_t *pgdat, struct scan_control *sc) >> >> > nr_reclaimed = sc->nr_reclaimed; >> >> > nr_scanned = sc->nr_scanned; >> >> > >> >> > + /* >> >> > + * Reset to the default values at the start. >> >> > + */ >> >> > + if (sc->priority == DEF_PRIORITY) { >> >> > + sc->reclaimable = 1; >> >> > + sc->cache_trim_mode = 0; >> >> > + } >> >> > + >> >> > prepare_scan_control(pgdat, sc); >> >> > >> >> > shrink_node_memcgs(pgdat, sc); >> >> > @@ -5890,8 +5905,7 @@ static void shrink_node(pg_data_t *pgdat, struct scan_control *sc) >> >> > vmpressure(sc->gfp_mask, sc->target_mem_cgroup, true, >> >> > sc->nr_scanned - nr_scanned, nr_node_reclaimed); >> >> > >> >> > - if (nr_node_reclaimed) >> >> > - reclaimable = true; >> >> > + sc->reclaimable = !!nr_node_reclaimed; >> > >> > The scope of this doesn't quite make sense. If direct reclaim scans >> > multiple nodes, reclaim failure on the first node would disable cache >> > trim mode on the second node, which is totally unrelated. >> > >> > I think it needs separate paths for direct reclaim and kswapd. For >> > direct reclaim, the retry should be before these similar retry catches >> > in do_try_to_free_pages(), after all zones have been considered: >> > >> > /* >> > * We make inactive:active ratio decisions based on the node's >> > * composition of memory, but a restrictive reclaim_idx or a >> > * memory.low cgroup setting can exempt large amounts of >> > * memory from reclaim. Neither of which are very common, so >> > * instead of doing costly eligibility calculations of the >> > * entire cgroup subtree up front, we assume the estimates are >> > * good, and retry with forcible deactivation if that fails. >> > */ >> > if (sc->skipped_deactivate) { >> > sc->priority = initial_priority; >> > sc->force_deactivate = 1; >> > sc->skipped_deactivate = 0; >> > goto retry; >> > } >> > >> > /* Untapped cgroup reserves? Don't OOM, retry. */ >> > if (sc->memcg_low_skipped) { >> > sc->priority = initial_priority; >> > sc->force_deactivate = 0; >> > sc->memcg_low_reclaim = 1; >> > sc->memcg_low_skipped = 0; >> > goto retry; >> > } >> >> In get_scan_count(), we have >> >> if (!sc->priority && swappiness) { >> scan_balance = SCAN_EQUAL; >> goto out; >> } > > Even though this can mitigate the issue for direct reclaim, it's still > suffering from the problem while direct reclaim goes from DEF_PRIORITY > to 1. What we need is not a mitigation but making things right. Whether does it cause issue for you? IMHO, it's the first step to prove this is a real problem. -- Best Regards, Huang, Ying > Thoughts? > > Byungchul > >> So, we don't really need the heuristics in direct reclaim path. So, one >> choice is to constrain this in kswapd reclaim only. >> >> -- >> Best Regards, >> Huang, Ying >> >> >> > and for kswapd it looks like it should be in balance_pgdat(), after >> > the priority loop, before increasing kswapd_failures. >> > >> > Instead of sc->reclaimable, which is very broad, it would be better to >> > call it sc->may_cache_trim_mode. This is in line with a bunch of other >> > such mechanisms in scan_control.
On Thu, Feb 29, 2024 at 04:41:18PM +0800, Huang, Ying wrote: > Byungchul Park <byungchul@sk.com> writes: > > > On Thu, Feb 29, 2024 at 02:21:15PM +0800, Huang, Ying wrote: > >> Johannes Weiner <hannes@cmpxchg.org> writes: > >> > >> > On Mon, Feb 26, 2024 at 02:06:30PM +0100, Michal Hocko wrote: > >> >> [CC Mel, Vlastimil and Johannes for awareness] > >> >> > >> >> On Fri 23-02-24 14:44:07, Byungchul Park wrote: > >> >> > Changes from v2: > >> >> > 1. Change the condition to stop cache_trim_mode. > >> >> > > >> >> > From - Stop it if it's at high scan priorities, 0 or 1. > >> >> > To - Stop it if it's at high scan priorities, 0 or 1, and > >> >> > the mode didn't work in the previous turn. > >> >> > > >> >> > (feedbacked by Huang Ying) > >> >> > > >> >> > 2. Change the test result in the commit message after testing > >> >> > with the new logic. > >> >> > > >> >> > Changes from v1: > >> >> > 1. Add a comment describing why this change is necessary in code > >> >> > and rewrite the commit message with how to reproduce and what > >> >> > the result is using vmstat. (feedbacked by Andrew Morton and > >> >> > Yu Zhao) > >> >> > 2. Change the condition to avoid cache_trim_mode from > >> >> > 'sc->priority != 1' to 'sc->priority > 1' to reflect cases > >> >> > where the priority goes to zero all the way. (feedbacked by > >> >> > Yu Zhao) > >> >> > > >> >> > --->8--- > >> >> > >From 05846e34bf02ac9b3e254324dc2d7afd97a025d9 Mon Sep 17 00:00:00 2001 > >> >> > From: Byungchul Park <byungchul@sk.com> > >> >> > Date: Fri, 23 Feb 2024 13:47:16 +0900 > >> >> > Subject: [PATCH v3] mm, vmscan: do not turn on cache_trim_mode if it doesn't work > >> >> > > >> >> > With cache_trim_mode on, reclaim logic doesn't bother reclaiming anon > >> >> > pages. However, it should be more careful to turn on the mode because > >> >> > it's going to prevent anon pages from being reclaimed even if there are > >> >> > a huge number of anon pages that are cold and should be reclaimed. Even > >> >> > worse, that leads kswapd_failures to reach MAX_RECLAIM_RETRIES and > >> >> > stopping kswapd from functioning until direct reclaim eventually works > >> >> > to resume kswapd. > >> >> > > >> >> > So do not turn on cache_trim_mode if the mode doesn't work, especially > >> >> > while the sytem is struggling against reclaim. > >> >> > > >> >> > The problematic behavior can be reproduced by: > >> >> > > >> >> > CONFIG_NUMA_BALANCING enabled > >> >> > sysctl_numa_balancing_mode set to NUMA_BALANCING_MEMORY_TIERING > >> >> > numa node0 (8GB local memory, 16 CPUs) > >> >> > numa node1 (8GB slow tier memory, no CPUs) > >> >> > > >> >> > Sequence: > >> >> > > >> >> > 1) echo 3 > /proc/sys/vm/drop_caches > >> >> > 2) To emulate the system with full of cold memory in local DRAM, run > >> >> > the following dummy program and never touch the region: > >> >> > > >> >> > mmap(0, 8 * 1024 * 1024 * 1024, PROT_READ | PROT_WRITE, > >> >> > MAP_ANONYMOUS | MAP_PRIVATE | MAP_POPULATE, -1, 0); > >> >> > > >> >> > 3) Run any memory intensive work e.g. XSBench. > >> >> > 4) Check if numa balancing is working e.i. promotion/demotion. > >> >> > 5) Iterate 1) ~ 4) until numa balancing stops. > >> >> > > >> >> > With this, you could see that promotion/demotion are not working because > >> >> > kswapd has stopped due to ->kswapd_failures >= MAX_RECLAIM_RETRIES. > >> >> > > >> >> > Interesting vmstat delta's differences between before and after are like: > >> >> > > >> >> > +-----------------------+-------------------------------+ > >> >> > | interesting vmstat | before | after | > >> >> > +-----------------------+-------------------------------+ > >> >> > | nr_inactive_anon | 321935 | 1636737 | > >> >> > | nr_active_anon | 1780700 | 465913 | > >> >> > | nr_inactive_file | 30425 | 35711 | > >> >> > | nr_active_file | 14961 | 8698 | > >> >> > | pgpromote_success | 356 | 1267785 | > >> >> > | pgpromote_candidate | 21953245 | 1745631 | > >> >> > | pgactivate | 1844523 | 3309867 | > >> >> > | pgdeactivate | 50634 | 1545041 | > >> >> > | pgfault | 31100294 | 6411036 | > >> >> > | pgdemote_kswapd | 30856 | 2267467 | > >> >> > | pgscan_kswapd | 1861981 | 7729231 | > >> >> > | pgscan_anon | 1822930 | 7667544 | > >> >> > | pgscan_file | 39051 | 61687 | > >> >> > | pgsteal_anon | 386 | 2227217 | > >> >> > | pgsteal_file | 30470 | 40250 | > >> >> > | pageoutrun | 30 | 457 | > >> >> > | numa_hint_faults | 27418279 | 2752289 | > >> >> > | numa_pages_migrated | 356 | 1267785 | > >> >> > +-----------------------+-------------------------------+ > >> >> > > >> >> > Signed-off-by: Byungchul Park <byungchul@sk.com> > >> >> > --- > >> >> > mm/vmscan.c | 24 +++++++++++++++++++----- > >> >> > 1 file changed, 19 insertions(+), 5 deletions(-) > >> >> > > >> >> > diff --git a/mm/vmscan.c b/mm/vmscan.c > >> >> > index bba207f41b14..f7312d831fed 100644 > >> >> > --- a/mm/vmscan.c > >> >> > +++ b/mm/vmscan.c > >> >> > @@ -127,6 +127,9 @@ struct scan_control { > >> >> > /* One of the zones is ready for compaction */ > >> >> > unsigned int compaction_ready:1; > >> >> > > >> >> > + /* If the last try was reclaimable */ > >> >> > + unsigned int reclaimable:1; > >> >> > + > >> >> > /* There is easily reclaimable cold cache in the current node */ > >> >> > unsigned int cache_trim_mode:1; > >> >> > > >> >> > @@ -2266,9 +2269,14 @@ static void prepare_scan_control(pg_data_t *pgdat, struct scan_control *sc) > >> >> > * If we have plenty of inactive file pages that aren't > >> >> > * thrashing, try to reclaim those first before touching > >> >> > * anonymous pages. > >> >> > + * > >> >> > + * It doesn't make sense to keep cache_trim_mode on if the mode > >> >> > + * is not working while struggling against reclaim. So do not > >> >> > + * turn it on if so. Note the highest priority of kswapd is 1. > >> >> > */ > >> >> > file = lruvec_page_state(target_lruvec, NR_INACTIVE_FILE); > >> >> > - if (file >> sc->priority && !(sc->may_deactivate & DEACTIVATE_FILE)) > >> >> > + if (file >> sc->priority && !(sc->may_deactivate & DEACTIVATE_FILE) && > >> >> > + !(sc->cache_trim_mode && !sc->reclaimable && sc->priority <= 1)) > >> >> > sc->cache_trim_mode = 1; > >> >> > else > >> >> > sc->cache_trim_mode = 0; > >> > > >> > The overall goal makes sense to me. > >> > > >> > file >> priority is just a heuristic that there are enough potential > >> > candidate pages, not a guarantee that any forward progress will > >> > happen. So it makes sense to retry without before failing. > >> > > >> > The way you wrote this conditional kind of hurts my head, > >> > though. Please don't write negations of complex terms like this. > >> > > >> > It expands to this: > >> > > >> > !sc->cache_trim_mode || sc->reclaimable || sc->priority > 1 > >> > > >> > which I'm not sure makes sense. Surely it should be something like > >> > > >> > !sc->cache_trim_mode && sc->reclaimable && sc->priority > 1 > >> > > >> > instead? > >> > > >> > Also > >> > > >> > if (!sc->cache_trim_mode) > >> > sc->cache_trim_mode = 1 > >> > else > >> > sc->cache_trim_mode = 0 > >> > > >> > will toggle on every loop. So if direct reclaim runs through a > >> > zonelist, it'll cache trim every other numa node...? > >> > > >> >> > @@ -5862,7 +5870,6 @@ static void shrink_node(pg_data_t *pgdat, struct scan_control *sc) > >> >> > { > >> >> > unsigned long nr_reclaimed, nr_scanned, nr_node_reclaimed; > >> >> > struct lruvec *target_lruvec; > >> >> > - bool reclaimable = false; > >> >> > > >> >> > if (lru_gen_enabled() && root_reclaim(sc)) { > >> >> > lru_gen_shrink_node(pgdat, sc); > >> >> > @@ -5877,6 +5884,14 @@ static void shrink_node(pg_data_t *pgdat, struct scan_control *sc) > >> >> > nr_reclaimed = sc->nr_reclaimed; > >> >> > nr_scanned = sc->nr_scanned; > >> >> > > >> >> > + /* > >> >> > + * Reset to the default values at the start. > >> >> > + */ > >> >> > + if (sc->priority == DEF_PRIORITY) { > >> >> > + sc->reclaimable = 1; > >> >> > + sc->cache_trim_mode = 0; > >> >> > + } > >> >> > + > >> >> > prepare_scan_control(pgdat, sc); > >> >> > > >> >> > shrink_node_memcgs(pgdat, sc); > >> >> > @@ -5890,8 +5905,7 @@ static void shrink_node(pg_data_t *pgdat, struct scan_control *sc) > >> >> > vmpressure(sc->gfp_mask, sc->target_mem_cgroup, true, > >> >> > sc->nr_scanned - nr_scanned, nr_node_reclaimed); > >> >> > > >> >> > - if (nr_node_reclaimed) > >> >> > - reclaimable = true; > >> >> > + sc->reclaimable = !!nr_node_reclaimed; > >> > > >> > The scope of this doesn't quite make sense. If direct reclaim scans > >> > multiple nodes, reclaim failure on the first node would disable cache > >> > trim mode on the second node, which is totally unrelated. > >> > > >> > I think it needs separate paths for direct reclaim and kswapd. For > >> > direct reclaim, the retry should be before these similar retry catches > >> > in do_try_to_free_pages(), after all zones have been considered: > >> > > >> > /* > >> > * We make inactive:active ratio decisions based on the node's > >> > * composition of memory, but a restrictive reclaim_idx or a > >> > * memory.low cgroup setting can exempt large amounts of > >> > * memory from reclaim. Neither of which are very common, so > >> > * instead of doing costly eligibility calculations of the > >> > * entire cgroup subtree up front, we assume the estimates are > >> > * good, and retry with forcible deactivation if that fails. > >> > */ > >> > if (sc->skipped_deactivate) { > >> > sc->priority = initial_priority; > >> > sc->force_deactivate = 1; > >> > sc->skipped_deactivate = 0; > >> > goto retry; > >> > } > >> > > >> > /* Untapped cgroup reserves? Don't OOM, retry. */ > >> > if (sc->memcg_low_skipped) { > >> > sc->priority = initial_priority; > >> > sc->force_deactivate = 0; > >> > sc->memcg_low_reclaim = 1; > >> > sc->memcg_low_skipped = 0; > >> > goto retry; > >> > } > >> > >> In get_scan_count(), we have > >> > >> if (!sc->priority && swappiness) { > >> scan_balance = SCAN_EQUAL; > >> goto out; > >> } > > > > Even though this can mitigate the issue for direct reclaim, it's still > > suffering from the problem while direct reclaim goes from DEF_PRIORITY > > to 1. What we need is not a mitigation but making things right. > > Whether does it cause issue for you? IMHO, it's the first step to prove > this is a real problem. I'm not strongly arguing. It'd be still okay to fix the real problem that I faced with kswapd. Byungchul > -- > Best Regards, > Huang, Ying > > > Thoughts? > > > > Byungchul > > > >> So, we don't really need the heuristics in direct reclaim path. So, one > >> choice is to constrain this in kswapd reclaim only. > >> > >> -- > >> Best Regards, > >> Huang, Ying > >> > >> > >> > and for kswapd it looks like it should be in balance_pgdat(), after > >> > the priority loop, before increasing kswapd_failures. > >> > > >> > Instead of sc->reclaimable, which is very broad, it would be better to > >> > call it sc->may_cache_trim_mode. This is in line with a bunch of other > >> > such mechanisms in scan_control.
diff --git a/mm/vmscan.c b/mm/vmscan.c index bba207f41b14..f7312d831fed 100644 --- a/mm/vmscan.c +++ b/mm/vmscan.c @@ -127,6 +127,9 @@ struct scan_control { /* One of the zones is ready for compaction */ unsigned int compaction_ready:1; + /* If the last try was reclaimable */ + unsigned int reclaimable:1; + /* There is easily reclaimable cold cache in the current node */ unsigned int cache_trim_mode:1; @@ -2266,9 +2269,14 @@ static void prepare_scan_control(pg_data_t *pgdat, struct scan_control *sc) * If we have plenty of inactive file pages that aren't * thrashing, try to reclaim those first before touching * anonymous pages. + * + * It doesn't make sense to keep cache_trim_mode on if the mode + * is not working while struggling against reclaim. So do not + * turn it on if so. Note the highest priority of kswapd is 1. */ file = lruvec_page_state(target_lruvec, NR_INACTIVE_FILE); - if (file >> sc->priority && !(sc->may_deactivate & DEACTIVATE_FILE)) + if (file >> sc->priority && !(sc->may_deactivate & DEACTIVATE_FILE) && + !(sc->cache_trim_mode && !sc->reclaimable && sc->priority <= 1)) sc->cache_trim_mode = 1; else sc->cache_trim_mode = 0; @@ -5862,7 +5870,6 @@ static void shrink_node(pg_data_t *pgdat, struct scan_control *sc) { unsigned long nr_reclaimed, nr_scanned, nr_node_reclaimed; struct lruvec *target_lruvec; - bool reclaimable = false; if (lru_gen_enabled() && root_reclaim(sc)) { lru_gen_shrink_node(pgdat, sc); @@ -5877,6 +5884,14 @@ static void shrink_node(pg_data_t *pgdat, struct scan_control *sc) nr_reclaimed = sc->nr_reclaimed; nr_scanned = sc->nr_scanned; + /* + * Reset to the default values at the start. + */ + if (sc->priority == DEF_PRIORITY) { + sc->reclaimable = 1; + sc->cache_trim_mode = 0; + } + prepare_scan_control(pgdat, sc); shrink_node_memcgs(pgdat, sc); @@ -5890,8 +5905,7 @@ static void shrink_node(pg_data_t *pgdat, struct scan_control *sc) vmpressure(sc->gfp_mask, sc->target_mem_cgroup, true, sc->nr_scanned - nr_scanned, nr_node_reclaimed); - if (nr_node_reclaimed) - reclaimable = true; + sc->reclaimable = !!nr_node_reclaimed; if (current_is_kswapd()) { /* @@ -5965,7 +5979,7 @@ static void shrink_node(pg_data_t *pgdat, struct scan_control *sc) * sleep. On reclaim progress, reset the failure counter. A * successful direct reclaim run will revive a dormant kswapd. */ - if (reclaimable) + if (sc->reclaimable) pgdat->kswapd_failures = 0; }